feat: unify minute artifacts output to ./minutes/{minute_token}/#604
Merged
feat: unify minute artifacts output to ./minutes/{minute_token}/#604
Conversation
github-actions
Bot
added
domain/vc
|
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:
📝 WalkthroughWalkthroughCentralizes minute-scoped artifact path constants and helper; introduces Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (user)
participant Shortcut as ShortcutHandler
participant MinutesSvc as MinutesService
participant FS as Filesystem
participant JSON as ResultEmitter
CLI->>Shortcut: invoke +download / vc +notes (flags, minuteToken)
Shortcut->>MinutesSvc: request artifact URL or transcript (minuteToken)
MinutesSvc-->>Shortcut: return download URL or raw bytes + headers
Shortcut->>FS: ValidateSafePath / Stat / create dirs if needed
Shortcut->>FS: write file at resolved path (minutes/{token}/... or explicit)
Shortcut->>JSON: emit result { minute_token, artifact_type, saved_path | download_url, size_bytes }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #604 +/- ##
==========================================
- Coverage 61.05% 60.37% -0.68%
==========================================
Files 400 408 +8
Lines 34130 43411 +9281
==========================================
+ Hits 20838 26210 +5372
- Misses 11368 15159 +3791
- Partials 1924 2042 +118 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@e7dfcee34f85fc2405aa170ad4841ec185895c57🧩 Skill updatenpx skills add larksuite/cli#feat/minutes_artifacts_path_unify -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/vc/vc_notes.go (1)
505-505:⚠️ Potential issue | 🟡 MinorStale
--output-dirdescription after default-layout change.The help text still says
default: current dir, but when--output-diris omitted the transcript now lands under./minutes/{minute_token}/transcript.txt(viacommon.DefaultMinuteArtifactDir). Align the description with the new default so+notes --helpdoesn't mislead users/Agents.📝 Suggested wording
- {Name: "output-dir", Desc: "output directory for artifact files (default: current dir)"}, + {Name: "output-dir", Desc: "output directory for artifact files. Default when omitted: ./minutes/{minute_token}/ (transcript filename: transcript.txt). When set, legacy layout ./{output-dir}/artifact-{title}-{token}/transcript.txt is used."},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/vc/vc_notes.go` at line 505, Update the help text for the CLI flag with Name "output-dir" in vc_notes.go so it no longer says "default: current dir" and instead reflects the new default path used when omitted (the code now writes transcripts to common.DefaultMinuteArtifactDir, e.g. "./minutes/{minute_token}/transcript.txt"); locate the flag definition where {Name: "output-dir", Desc: "..."} is declared and change Desc to mention the new default layout (or reference common.DefaultMinuteArtifactDir) so the `+notes --help` output is accurate.
🧹 Nitpick comments (1)
shortcuts/minutes/minutes_download.go (1)
51-65: Consider fail-fast path safety checks for--output/--output-dirinValidate.Path safety for user-supplied
--output/--output-diris currently only enforced implicitly viaruntime.FileIO().Save/Statat execution time. For consistency withvc +notes(which callscommon.ValidateSafeOutputDirinValidate) and to fail fast before contacting the Minutes API, consider adding equivalent checks here.Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { tokens := common.SplitCSV(runtime.Str("minute-tokens")) if len(tokens) == 0 { return output.ErrValidation("--minute-tokens is required") } ... for _, token := range tokens { if !validMinuteToken.MatchString(token) { return output.ErrValidation("invalid minute token %q: ...", token) } } + if outDir := runtime.Str("output-dir"); outDir != "" { + if err := common.ValidateSafeOutputDir(runtime.FileIO(), outDir); err != nil { + return err + } + } return nil },🤖 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 51 - 65, The Validate function for minutes_download.go currently checks minute-tokens but does not perform early path-safety checks for user-supplied --output/--output-dir; add a fail-fast check by calling common.ValidateSafeOutputDir (or equivalent) using runtime.Str("output") and runtime.Str("output-dir") (or the exact flag names used) inside Validate before contacting the API, returning any validation error; this mirrors vc +notes behavior and prevents unsafe/invalid paths from proceeding to execution where runtime.FileIO().Save/Stat would surface them.
🤖 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 96-100: The new auto-upgrade that rewrites explicitOutputPath into
explicitOutputDir in batch mode weakens the original validation expected by
TestDownload_Validation_OutputWithBatch; restore the intended behavior by adding
an explicit validation check (e.g., in the command's Validate or flag-parsing
logic) that if single == false and explicitOutputPath != "" then return an error
telling the user that --output must be a directory in batch mode (or use
--output-dir), rather than silently converting the flag; reference variables
single, explicitOutputPath and explicitOutputDir and ensure the test
TestDownload_Validation_OutputWithBatch still fails when non-dir --output is
provided in batch mode.
- Around line 81-104: The Stat call on explicitOutputPath currently swallows all
errors; change the logic in the block that inspects explicitOutputPath so that
when calling runtime.FileIO().Stat(explicitOutputPath) you treat a
PathValidationError (or any other non-nil error that is not os.IsNotExist) as a
hard failure and return the error (or wrap it via output.ErrValidation) instead
of ignoring it; only fall through to “treat as file” when Stat returns not found
(os.IsNotExist) or when Stat succeeds and is not a dir. This affects the
explicitOutputPath handling and prevents delayed errors surfaced later in Save.
---
Outside diff comments:
In `@shortcuts/vc/vc_notes.go`:
- Line 505: Update the help text for the CLI flag with Name "output-dir" in
vc_notes.go so it no longer says "default: current dir" and instead reflects the
new default path used when omitted (the code now writes transcripts to
common.DefaultMinuteArtifactDir, e.g.
"./minutes/{minute_token}/transcript.txt"); locate the flag definition where
{Name: "output-dir", Desc: "..."} is declared and change Desc to mention the new
default layout (or reference common.DefaultMinuteArtifactDir) so the `+notes
--help` output is accurate.
---
Nitpick comments:
In `@shortcuts/minutes/minutes_download.go`:
- Around line 51-65: The Validate function for minutes_download.go currently
checks minute-tokens but does not perform early path-safety checks for
user-supplied --output/--output-dir; add a fail-fast check by calling
common.ValidateSafeOutputDir (or equivalent) using runtime.Str("output") and
runtime.Str("output-dir") (or the exact flag names used) inside Validate before
contacting the API, returning any validation error; this mirrors vc +notes
behavior and prevents unsafe/invalid paths from proceeding to execution where
runtime.FileIO().Save/Stat would surface them.
🪄 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: 22c5f986-793e-4cef-aa72-e93793c409fe
📒 Files selected for processing (5)
shortcuts/common/artifact_path.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.goshortcuts/vc/vc_notes.goshortcuts/vc/vc_notes_test.go
Collaborator
Author
|
@coderabbitai All feedback addressed in 3184006: Inline review comments (2)
Outside-diff (1) Nitpick (1) Verification:
|
|
Tip For best results, initiate chat on the files or code changes.
The race-clean test run and clean lint check are great signals. 🎉 (´｡• ᵕ •｡`) ♡ ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/minutes/minutes_download.go (1)
201-203:⚠️ Potential issue | 🟡 MinorKeep
--url-onlyJSON consistent with the new artifact metadata.Line 202 drops
ArtifactType, and the single-token--url-onlyresponse on Line 245 still emits onlydownload_url. That meansminutes +download --url-onlyis the one success path that does not include the newminute_token+artifact_type: "recording"fields intended for Agent indexing.Suggested fix
if urlOnly { - results[i] = result{MinuteToken: token, DownloadURL: downloadURL} + results[i] = result{ + MinuteToken: token, + ArtifactType: common.ArtifactTypeRecording, + DownloadURL: downloadURL, + } continue } @@ if urlOnly { - runtime.Out(map[string]interface{}{"download_url": r.DownloadURL}, nil) + runtime.Out(map[string]interface{}{ + "minute_token": r.MinuteToken, + "artifact_type": r.ArtifactType, + "download_url": r.DownloadURL, + }, nil) } else {Also applies to: 244-252
🤖 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 201 - 203, When urlOnly is true the code only sets MinuteToken and DownloadURL but omits ArtifactType, and the single-token (--url-only) response also emits only download_url; update the urlOnly branch that writes into the results slice (the result struct) to also set ArtifactType = "recording" (or the appropriate constant) so the JSON includes artifact_type, and update the single-token response path that returns only download_url to include minute_token and artifact_type:"recording" as well so both the multi-result (results) and single-token response paths emit minute_token, download_url, and artifact_type for Agent indexing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@shortcuts/minutes/minutes_download.go`:
- Around line 201-203: When urlOnly is true the code only sets MinuteToken and
DownloadURL but omits ArtifactType, and the single-token (--url-only) response
also emits only download_url; update the urlOnly branch that writes into the
results slice (the result struct) to also set ArtifactType = "recording" (or the
appropriate constant) so the JSON includes artifact_type, and update the
single-token response path that returns only download_url to include
minute_token and artifact_type:"recording" as well so both the multi-result
(results) and single-token response paths emit minute_token, download_url, and
artifact_type for Agent indexing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd19c4d2-8134-400b-92c5-1e24c6606db7
📒 Files selected for processing (3)
shortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.goshortcuts/vc/vc_notes.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/minutes/minutes_download_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/vc/vc_notes.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/vc/vc_notes.go (1)
339-347: Minutetoken validation: inconsistency withminutes +download, but path safety is already enforced by FileIO layer.The concern about
minuteTokenescaping theminutes/tree is mitigated:runtime.FileIO().Save()internally callsSafeOutputPath(), which rejects any path traversal attempts via canonicalization and CWD-boundary validation. However,vc +notesinconsistently lacks the upfront token validation thatminutes +downloadapplies (regex requiring lowercase alphanumeric only). Consider validating tokens in theValidatestep to matchminutes +downloadfor better fail-fast UX and consistency, rather than relying on downstream FileIO protection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/vc/vc_notes.go` around lines 339 - 347, Add a fail-fast validation for minuteToken in the vc +notes command's Validate step (before building dirName/transcriptPath) to mirror the minutes +download behavior: check the minuteToken value matches the same lowercase alphanumeric regex used by minutes +download (reject anything with non-lowercase-alnum characters) and return a validation error if it fails. This should be done alongside existing uses of minuteToken/sanitizeDirName and not rely solely on runtime.FileIO().Save()’s SafeOutputPath; update the Validate logic that runs prior to constructing dirName/transcriptPath to enforce the token rule for consistency and better UX.
🤖 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/vc/vc_notes.go`:
- Around line 339-347: Add a fail-fast validation for minuteToken in the vc
+notes command's Validate step (before building dirName/transcriptPath) to
mirror the minutes +download behavior: check the minuteToken value matches the
same lowercase alphanumeric regex used by minutes +download (reject anything
with non-lowercase-alnum characters) and return a validation error if it fails.
This should be done alongside existing uses of minuteToken/sanitizeDirName and
not rely solely on runtime.FileIO().Save()’s SafeOutputPath; update the Validate
logic that runs prior to constructing dirName/transcriptPath to enforce the
token rule for consistency and better UX.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c478858e-f99a-4917-9b6b-731134cc8ea7
📒 Files selected for processing (5)
shortcuts/common/artifact_path.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.goshortcuts/vc/vc_notes.goshortcuts/vc/vc_notes_test.go
✅ Files skipped from review due to trivial changes (3)
- shortcuts/common/artifact_path.go
- shortcuts/minutes/minutes_download_test.go
- shortcuts/minutes/minutes_download.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 206-218: The server-provided filenames must be constrained to the
per-token artifact directory: in the download flow that builds downloadOpts (see
downloadOpts struct usage and code paths setting opts.outputDir =
common.DefaultMinuteArtifactDir(token) and opts.outputPath =
explicitOutputPath), sanitize and validate any filename extracted from
Content-Disposition before joining it to opts.outputDir; specifically, replace
the filename with filepath.Base(filename) (or reject if it contains path
separators or traversal components like ".."), then join via
filepath.Join(opts.outputDir, sanitizedName) and verify the resulting path is
inside the intended directory (e.g., via filepath.Clean + filepath.Rel or an
equivalent containment check) before passing to FileIO.Save; if validation
fails, return an error instead of writing outside the per-token directory.
🪄 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: 31a505a2-b2c0-4150-acea-2fb56e973967
📒 Files selected for processing (6)
shortcuts/common/artifact_path.goshortcuts/common/validate.goshortcuts/common/validate_test.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.goshortcuts/vc/vc_notes.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/common/artifact_path.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/minutes/minutes_download_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/minutes/minutes_download.go (1)
133-140: Result schema and per-token JSON envelope changes look consistent.
artifact_type/minute_token/saved_path/size_bytesare set only on the success path (line 226–231); early-error paths only populateMinuteToken+Error, andomitemptykeeps the JSON clean. Thesinglebranch emitsdownload_urlforurl-onlyand the artifact triple otherwise — matches the PR-documented Agent-indexing contract.Minor nit (non-blocking):
size_bytesusesomitempty, so a legitimate 0-byte download would silently omit the field. Unlikely in practice for media, but if the Agent parses this strictly, you may want to dropomitemptyonSizeBytesfor the single-success envelope. Happy to skip if intentional.Also applies to: 226-252
🤖 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 133 - 140, The result struct's SizeBytes field currently has `omitempty` which will drop a legitimate 0 value from JSON; update the result struct (type result) so that SizeBytes is always emitted for successful single-download envelopes — remove `omitempty` from SizeBytes (or otherwise ensure the success-path JSON builder explicitly includes size_bytes=0) so agents parsing the response can rely on the field being present for success cases.
🤖 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 133-140: The result struct's SizeBytes field currently has
`omitempty` which will drop a legitimate 0 value from JSON; update the result
struct (type result) so that SizeBytes is always emitted for successful
single-download envelopes — remove `omitempty` from SizeBytes (or otherwise
ensure the success-path JSON builder explicitly includes size_bytes=0) so agents
parsing the response can rely on the field being present for success cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90eb1fba-0662-4ed4-b517-4a47b8d8250c
📒 Files selected for processing (2)
shortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/minutes/minutes_download_test.go
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Ren1104
force-pushed
the
feat/minutes_artifacts_path_unify
branch
from
b7a2ff8 to
89593ed
Compare
Ren1104
force-pushed
the
feat/minutes_artifacts_path_unify
branch
from
89593ed to
e7dfcee
Compare
zhaoleibd
approved these changes
Apr 23, 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
2 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
Unify the default output layout for
minutes +downloadandvc +notesso that the recording and transcript of the same meeting land under a single./minutes/{minute_token}/directory. Zero config, zero env var — works out of the box for Agent pipelines that chain the two commands.Changes
shortcuts/common/artifact_path.go— sharedDefaultMinuteArtifactDir()helper andartifact_typeenum constants.minutes +download:--output-dirflag (mutually exclusive with--output).--outputpointing at an existing directory reportedcannot create parent directory; the path is now promoted to--output-dir(cp semantics)../minutes/{minute_token}/{server-provided-filename}. Filename resolution (Content-Disposition / Content-Type /{token}.media) is unchanged; only the directory moves.minute_token+artifact_type: \"recording\"for Agent indexing.vc +notes: When--output-diris empty, transcript lands to./minutes/{minute_token}/transcript.txt(unified with the recording path above). Explicit--output-dir ./xpreserves the legacy./x/artifact-{title}-{token}/transcript.txtlayout for backwards compatibility.Test Plan
Related
Internal design doc: VC/Minutes 下载产物统一方案 v1
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests