feat: add +download shortcut for minutes media download#101
Merged
Conversation
|
|
Ren1104
force-pushed
the
feat/minutes-media-download
branch
from
c397651 to
52cf828
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI Tool (minutes +download)
participant MinAPI as Minutes API
participant Presigned as Presigned URL Service
participant FS as File System
User->>CLI: invoke minutes +download --minute-tokens=<tokens> [--url-only|--output|--output-dir]
CLI->>CLI: Validate flags & batch constraints
loop per token
CLI->>MinAPI: GET /open-apis/minutes/v1/minutes/<token>/media
MinAPI-->>CLI: { download_url }
end
par Concurrent downloads (max 3 workers)
CLI->>Presigned: GET <download_url>
Presigned-->>CLI: HTTP 2xx + headers/body or HTTP error
CLI->>CLI: validate URL (SSRF) & safe redirect policy
CLI->>CLI: resolve filename (Content-Disposition/Content-Type/title) & dedupe
CLI->>FS: atomic write to output path
FS-->>CLI: saved_path, size_bytes
and
Presigned-->>CLI: HTTP >=400
CLI->>CLI: read error body (partial) and record per-token error
end
CLI-->>User: JSON summary (per-token success/error) or single-mode saved_path/size_bytes or URL-only output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
skills/lark-vc/SKILL.md (1)
34-43: Clarify the<title>placeholder in artifact directory naming.The artifact path
./artifact-<title>/coveruses<title>as a placeholder, but the documentation doesn't explain:
- Whether it should be the actual meeting title from the API response
- How to handle special characters, spaces, or other path-unsafe characters in the title
- Whether there's a maximum length limit for filesystem compatibility
Add explicit guidance on sanitizing the title for filesystem safety (e.g., replacing spaces with hyphens, removing special characters, truncating if needed). This pattern is used consistently in the skill (
./artifact-<title>/transcript.txt,./artifact-<title>/cover), so clarifying it once would benefit all use cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-vc/SKILL.md` around lines 34 - 43, The docs use ./artifact-<title>/cover and ./artifact-<title>/transcript.txt but don't define what <title> should be; update SKILL.md to state that <title> is the meeting title extracted from the API response (from the result of the `lark-cli docs +fetch --doc <note_doc_token>` call) and add an explicit sanitization rule: trim and lowercase the title, replace whitespace with hyphens, remove or percent-encode filesystem-unsafe characters (keep only alphanumeric, hyphen, underscore, dot), and truncate to a safe max length (e.g., 100 chars); show one concrete example mapping a raw title to the sanitized directory name and note that if sanitization yields an empty string, fall back to a timestamp or note token.shortcuts/minutes/minutes_download_test.go (1)
227-227: URL scheme inconsistency in test stubs.The
mediaStubreturns a full URL with scheme (https://example.com/presigned/download), butdownloadStubis registered with a URL without the scheme (example.com/presigned/download). This works because the mock registry likely matches on the host+path portion, but it's inconsistent and could cause confusion.Consider using consistent URL formats in the stubs, or document why the scheme is omitted for
downloadStub.Also applies to: 254-254, 279-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/minutes/minutes_download_test.go` at line 227, The test uses inconsistent URL formats between mediaStub (which returns "https://example.com/presigned/download") and the downloadStub registrations (which use "example.com/presigned/download"); update the downloadStub registrations (the calls to reg.Register(downloadStub(...))) to use the same full URL with scheme ("https://example.com/presigned/download") or change mediaStub to match the scheme-less form so both stubs are consistent, and apply the same change for the other occurrences referenced (the downloadStub registrations at the other locations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/minutes/minutes_download.go`:
- Around line 21-23: The code currently sets const disableClientTimeout = 0
which removes the HTTP client timeout and relies on the caller context
(cmd.Context()) that typically has no deadline; update the logic so downloads
have a fallback deadline (suggest 5 minutes) by either changing
disableClientTimeout to an actual timeout value (e.g., 5*time.Minute) or,
better, wrap the context inside the Execute path (where Execute / cmd.Context()
is used to start the download) with context.WithTimeout when ctx has no deadline
so the HTTP client call(s) in the minutes download code (e.g., where Execute
invokes the download request) get a bounded context and the client Timeout
remains safe.
---
Nitpick comments:
In `@shortcuts/minutes/minutes_download_test.go`:
- Line 227: The test uses inconsistent URL formats between mediaStub (which
returns "https://example.com/presigned/download") and the downloadStub
registrations (which use "example.com/presigned/download"); update the
downloadStub registrations (the calls to reg.Register(downloadStub(...))) to use
the same full URL with scheme ("https://example.com/presigned/download") or
change mediaStub to match the scheme-less form so both stubs are consistent, and
apply the same change for the other occurrences referenced (the downloadStub
registrations at the other locations).
In `@skills/lark-vc/SKILL.md`:
- Around line 34-43: The docs use ./artifact-<title>/cover and
./artifact-<title>/transcript.txt but don't define what <title> should be;
update SKILL.md to state that <title> is the meeting title extracted from the
API response (from the result of the `lark-cli docs +fetch --doc
<note_doc_token>` call) and add an explicit sanitization rule: trim and
lowercase the title, replace whitespace with hyphens, remove or percent-encode
filesystem-unsafe characters (keep only alphanumeric, hyphen, underscore, dot),
and truncate to a safe max length (e.g., 100 chars); show one concrete example
mapping a raw title to the sanitized directory name and note that if
sanitization yields an empty string, fall back to a timestamp or note token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee69dd44-c8a0-401b-a4e4-2ce27150d928
📒 Files selected for processing (11)
shortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.goshortcuts/minutes/shortcuts.goshortcuts/register.goshortcuts/vc/artifact-Empty Artifacts-tok003/transcript.txtshortcuts/vc/artifact-No Note Meeting-tok002/transcript.txtshortcuts/vc/artifact-Test Minutes-tok001/transcript.txtskills/lark-minutes/SKILL.mdskills/lark-minutes/references/lark-minutes-download.mdskills/lark-vc/SKILL.mdskills/lark-vc/references/lark-vc-notes.md
💤 Files with no reviewable changes (3)
- shortcuts/vc/artifact-Empty Artifacts-tok003/transcript.txt
- shortcuts/vc/artifact-Test Minutes-tok001/transcript.txt
- shortcuts/vc/artifact-No Note Meeting-tok002/transcript.txt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
shortcuts/minutes/minutes_download_test.go (2)
103-114: Minor:chdirmodifies global state, preventing parallel test execution.Tests using
chdircannot safely uset.Parallel(). This is fine for now since no parallel calls exist, but documenting this constraint would help future maintainers.♻️ Add a comment to document the constraint
-// chdir 切换工作目录并在测试结束后恢复 +// chdir 切换工作目录并在测试结束后恢复 +// NOTE: Tests using chdir cannot be run in parallel (no t.Parallel). func chdir(t *testing.T, dir string) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/minutes/minutes_download_test.go` around lines 103 - 114, The chdir helper modifies process-wide working directory (uses os.Chdir and t.Cleanup to restore) which makes tests using it incompatible with t.Parallel; update the chdir function comment to state that it mutates global state and therefore tests that call chdir must not run in parallel (or suggest using a temp process/workdir isolation alternative), and include references to chdir, os.Chdir and t.Cleanup so future maintainers understand the constraint.
54-60: Consider checking theExecute()error to surface setup failures.Ignoring the error from
parent.Execute()in cache warming could mask initialization problems, leading to confusing failures in subsequent tests.♻️ Proposed fix
parent.SetArgs([]string{"+warm"}) parent.SilenceErrors = true parent.SilenceUsage = true - parent.Execute() + if err := parent.Execute(); err != nil { + t.Logf("warmTokenCache: %v", err) + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/minutes/minutes_download_test.go` around lines 54 - 60, The test currently ignores the error returned by parent.Execute(), which can hide setup failures; update the test to capture and assert the Execute() error (e.g., err := parent.Execute()) and fail the test if err != nil (using t.Fatalf or require.NoError) after mounting the command via s.Mount and setting args with parent.SetArgs([]string{"+warm"}), so any cache-warm initialization errors surface immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/minutes/minutes_download_test.go`:
- Around line 316-338: The test setup in TestDownload_OverwriteProtection
currently ignores the error from os.WriteFile when creating "existing.mp4";
update the TestDownload_OverwriteProtection function to check the error returned
by os.WriteFile and fail the test immediately (e.g., t.Fatal or t.Fatalf) if it
is non-nil so the test fails fast and doesn’t produce misleading results—locate
the os.WriteFile call near the start of TestDownload_OverwriteProtection and add
the error check there.
---
Nitpick comments:
In `@shortcuts/minutes/minutes_download_test.go`:
- Around line 103-114: The chdir helper modifies process-wide working directory
(uses os.Chdir and t.Cleanup to restore) which makes tests using it incompatible
with t.Parallel; update the chdir function comment to state that it mutates
global state and therefore tests that call chdir must not run in parallel (or
suggest using a temp process/workdir isolation alternative), and include
references to chdir, os.Chdir and t.Cleanup so future maintainers understand the
constraint.
- Around line 54-60: The test currently ignores the error returned by
parent.Execute(), which can hide setup failures; update the test to capture and
assert the Execute() error (e.g., err := parent.Execute()) and fail the test if
err != nil (using t.Fatalf or require.NoError) after mounting the command via
s.Mount and setting args with parent.SetArgs([]string{"+warm"}), so any
cache-warm initialization errors surface immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e070cad9-dd8c-4bb1-af08-880883a07ac0
📒 Files selected for processing (2)
shortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/minutes/minutes_download.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/minutes/minutes_download.go (1)
270-287: Edge-case collision if multiple tokens share same 6-character prefix.If three or more tokens share the same 6-character prefix and all resolve to the same filename (from Content-Disposition), the deduplication could generate identical filenames. Line 285 uses
Storewithout verifying the deduped name is unique, potentially causing file overwrites.While unlikely in practice, consider using
LoadOrStorefor the deduped name or using the full token as the suffix:♻️ Proposed fix using LoadOrStore for deduped name
func deduplicateFilename(name, minuteToken string, usedNames *sync.Map) string { if _, loaded := usedNames.LoadOrStore(name, true); !loaded { return name } // collision: insert _<token_prefix> before extension ext := filepath.Ext(name) base := strings.TrimSuffix(name, ext) - suffix := minuteToken - if len(suffix) > tokenSuffixLen { - suffix = suffix[:tokenSuffixLen] - } + // Use full token to guarantee uniqueness since tokens are unique + suffix := minuteToken deduped := base + "_" + suffix + ext - usedNames.Store(deduped, true) + usedNames.LoadOrStore(deduped, true) return deduped }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/minutes/minutes_download.go` around lines 270 - 287, The deduplicateFilename function can still produce duplicate names when multiple minuteToken values share the same tokenPrefix because it unconditionally usedNames.Store(deduped, true); change this to attempt to reserve the deduped name with usedNames.LoadOrStore and, if LoadOrStore indicates a collision, retry by expanding the suffix (e.g., use a longer slice of minuteToken beyond tokenSuffixLen or the full minuteToken) or append an incrementing counter until LoadOrStore returns not-loaded, then return that reserved name; update the logic around minuteToken, tokenSuffixLen, and usedNames accordingly to loop until a unique name is successfully stored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/minutes/minutes_download.go`:
- Around line 270-287: The deduplicateFilename function can still produce
duplicate names when multiple minuteToken values share the same tokenPrefix
because it unconditionally usedNames.Store(deduped, true); change this to
attempt to reserve the deduped name with usedNames.LoadOrStore and, if
LoadOrStore indicates a collision, retry by expanding the suffix (e.g., use a
longer slice of minuteToken beyond tokenSuffixLen or the full minuteToken) or
append an incrementing counter until LoadOrStore returns not-loaded, then return
that reserved name; update the logic around minuteToken, tokenSuffixLen, and
usedNames accordingly to loop until a unique name is successfully stored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a67a8fa-59a4-425b-8349-298a64da9ba0
📒 Files selected for processing (1)
shortcuts/minutes/minutes_download.go
…n, and smart filename resolution
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@6637afc506db016d2b9105c70682fef6e3ff828b🧩 Skill updatenpx skills add larksuite/cli#feat/minutes-media-download -y -g |
Greptile SummaryThis PR adds a Confidence Score: 5/5Safe to merge; prior P0/P1 issues are resolved and only minor style findings remain. All previously flagged critical issues have been addressed: the shared client mutation is fixed by struct-copy cloning, concurrency problems are eliminated by making the loop sequential, and the intentional omission of transport-level IP validation is clearly documented. The two remaining findings are both P2 style issues (orphaned comment and unused export) that do not affect correctness or security. shortcuts/minutes/minutes_download.go (stale comment block at line 319), internal/validate/url.go (unexported candidate WrapDialContextWithIPCheck) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([minutes +download]) --> B{Validate tokens}
B -- invalid --> Z1([ErrValidation])
B -- valid --> C{--dry-run?}
C -- yes --> Z2([Print API shape])
C -- no --> D[Clone HTTP client\nset Timeout=0\nset CheckRedirect]
D --> E[For each token\n rate-limit 5 req/s]
E --> F[fetchDownloadURL\nDoAPIJSON GET /media]
F -- error --> G[record error]
F -- ok --> H{--url-only?}
H -- yes --> I[record URL]
H -- no --> J[downloadMediaFile]
J --> K[ValidateDownloadSourceURL]
K --> L[HTTP GET presigned URL]
L --> M[resolveFilenameFromResponse\nContent-Disposition > Content-Type > token.media]
M --> N[deduplicateFilename\nif batch collision]
N --> O[SafeOutputPath\npath traversal guard]
O --> P[AtomicWriteFromReader]
P --> Q[record saved_path + size]
G & I & Q --> R{single mode?}
R -- yes --> S([Output single result or error])
R -- no --> T([Output batch results array])
Reviews (9): Last reviewed commit: "fix(minutes): add transport-level SSRF p..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/minutes/minutes_download.go`:
- Around line 133-136: executeBatch currently serializes URL fetching but allows
concurrent workers to call runtime via resolveOutputFromResponse ->
fetchMinuteTitle, reintroducing shared RuntimeContext access; fix by preloading
the title (or filename stem) for each token during phase 1 using
fetchMinuteTitle (or equivalent) and attach that value to the per-download
metadata so that downloadMediaFile no longer calls runtime.DoAPIJSON. Thread the
preloaded title/filename into downloadOpts (and into resolveOutputFromResponse
where it builds the final filename) so phase 2 only performs pure HTTP I/O;
apply the same change to other places that call
resolveOutputFromResponse/downloadMediaFile to keep RuntimeContext off the
concurrent path.
- Around line 154-190: The loop that calls fetchDownloadURL over tokens can
exceed the documented 5 req/s limit; modify the phase-1 URL fetch to rate-limit
calls to 5 requests per second (e.g., use a token bucket or time.Ticker) while
iterating tokens in the for i, token := range tokens loop, and add retry logic
for rate-limit responses from fetchDownloadURL (detect HTTP 429 or equivalent
error, retry with exponential backoff and jitter up to a small max attempts) so
transient rate limits are retried before marking results[i] as an error; ensure
duplicate detection (seen map), urlOnly handling, and appending to tasks remain
unchanged, only pacing and retry wrapping are added around the fetchDownloadURL
call.
- Around line 263-275: The deduplication can still collide because deduped :=
base+"_"+suffix+ext is stored without checking; modify deduplicateFilename to
probe in a loop using usedNames.LoadOrStore on each candidate until you actually
reserve a free name: try the original name, if taken compute suffix :=
minuteToken[:tokenSuffixLen], form candidate := base+"_"+suffix+ext and call
usedNames.LoadOrStore(candidate, true); if that returns loaded==true, append or
increment a numeric counter (e.g., base+"_"+suffix+"_"+strconv.Itoa(i)+ext) and
retry until LoadOrStore returns loaded==false, then return the reserved
candidate. Ensure references to usedNames, minuteToken, tokenSuffixLen and the
function deduplicateFilename are used so the change is applied in the right
place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13a75ae5-fd65-48b9-9e3c-1111b6cf01a7
📒 Files selected for processing (4)
shortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.goskills/lark-minutes/SKILL.mdskills/lark-minutes/references/lark-minutes-download.md
✅ Files skipped from review due to trivial changes (1)
- shortcuts/minutes/minutes_download_test.go
…ncy safety, rate limiting, dedup robustness
Ren1104
force-pushed
the
feat/minutes-media-download
branch
from
fc62027 to
cc47e84
Compare
zhaoleibd
reviewed
Apr 1, 2026
…el download, merge output flags
github-actions
Bot
added
domain/vc
Ren1104
force-pushed
the
feat/minutes-media-download
branch
2 times, most recently
from
ee60cbb to
ec09b3c
Compare
Ren1104
force-pushed
the
feat/minutes-media-download
branch
from
ec09b3c to
6637afc
Compare
liangshuo-1
approved these changes
Apr 2, 2026
2 tasks
This was referenced Apr 4, 2026
This was referenced Apr 13, 2026
This was referenced Apr 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a
minutes +downloadshortcut to download audio/video media files from Lark Minutes, or retrieve a 1-day valid download URL without downloading.Changes
shortcuts/minutes/minutes_download.go— implements the+downloadshortcut with--minute-token,--output,--url-only,--overwriteflagsshortcuts/minutes/shortcuts.go— minutes shortcut registryshortcuts/register.goshortcuts/minutes/minutes_download_test.go— 10 unit and integration tests (81.8% coverage)skills/lark-minutes/SKILL.md— add+downloaddocumentation, Shortcuts table, andminutes:minutes.media:exportscopeskills/lark-minutes/references/lark-minutes-download.md— detailed reference for the download shortcutskills/lark-vc/SKILL.md— clarify note_doc_token vs verbatim_doc_token, add cover image guidanceTest Plan
go vet ./...passesgo test -race -count=1 ./shortcuts/minutes/...— 10/10 tests pass (81.8% coverage)go test -race -count=1 -timeout=5m ./cmd/... ./internal/... ./shortcuts/...passes (excluding pre-existing mail test timeout)lark-cli minutes +download --dry-runoutputs correct API requestlark-cli minutes +download --minute-token <token> --url-onlyreturns valid download URLlark-cli minutes +download --minute-token <token>downloads media file with correct filenameSummary by CodeRabbit
New Features
Documentation
Tests
Chores