feat(minutes,vc): emit typed error envelopes across both domains#1234
Merged
Conversation
evandance
added
domain/minutes
|
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:
📝 WalkthroughWalkthroughMigrates minutes and VC shortcuts from legacy output-based errors to typed ChangesTyped Error Framework Migration for Minutes and VC Shortcuts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
github-actions
Bot
added
size/L
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1234 +/- ##
==========================================
+ Coverage 71.14% 71.24% +0.09%
==========================================
Files 685 687 +2
Lines 65770 65714 -56
==========================================
+ Hits 46793 46815 +22
+ Misses 15326 15258 -68
+ Partials 3651 3641 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
evandance
force-pushed
the
feat/errs-migrate-minutes-vc
branch
from
b60ea07 to
0d4fab3
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@501f2ba21a7e845456b3d81640dd6637950a4d3f🧩 Skill updatenpx skills add larksuite/cli#feat/errs-migrate-minutes-vc -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
shortcuts/vc/vc_recording.go (1)
119-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the active credential resolver for the missing-scope precheck.
This block only inspects
auth.GetStoredToken(appID, userOpenID), so the new typedmissing_scopeerror is skipped whenever the current user credential comes from another resolver path or a refreshed token that is not mirrored in the store.shortcuts/vc/vc_notes.goalready usesruntime.Factory.Credential.ResolveToken(...)for the same check, so these two VC commands will diverge for the same login state unless this one resolves scopes the same way.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/vc/vc_recording.go` around lines 119 - 129, The missing-scope precheck currently uses auth.GetStoredToken(appID, userOpenID) which misses credentials resolved via the active resolver; update this block to call runtime.Factory.Credential.ResolveToken(runtime.As(), appID, userOpenID) (matching shortcuts/vc/vc_notes.go) and use the returned token when invoking auth.MissingScopes, falling back to auth.GetStoredToken only if ResolveToken returns nil or an error; keep the same error construction (errs.NewPermissionError ... WithHint ... WithMissingScopes ... WithIdentity(string(runtime.As()))) and ensure appID, userOpenID, auth.MissingScopes, runtime.Factory.Credential.ResolveToken and runtime.As() are the referenced symbols to locate the change.shortcuts/vc/vc_notes.go (1)
372-381:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't count echoed
minute_tokeninputs as successful payload.On the error path,
fetchNoteByMinuteTokenstill returns the queriedminute_token, andhasNotesPayloadlater treats any non-emptyminute_tokenas success. A batch where every/minutes/{token}call fails will therefore skipOutPartialFailureand exit 0, even though nothing was actually fetched. Count only fetched note/minute data here, or make the success test mode-aware so an echoed identifier does not satisfy it.Also applies to: 390-397, 750-761
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/vc/vc_notes.go` around lines 372 - 381, The current success test treats any non-empty "minute_token" as a valid payload which lets echoed tokens from fetchNoteByMinuteToken count as success; update hasNotesPayload to ignore "minute_token" (or require it be accompanied by real content like "meeting_notes", "note_doc_token", "verbatim_doc_token", "shared_doc_tokens", or "artifacts") so only actual fetched note data marks success, and adjust any other places that relied on minute_token-as-success (including the other occurrences you flagged) and related logic around fetchNoteByMinuteToken to ensure echoed tokens do not short-circuit error handling.
🧹 Nitpick comments (1)
.golangci.yml (1)
79-82: 💤 Low valueStale "drive-only" comment after adding minutes/vc.
Line 81 now enforces
errs-no-legacy-helperonshortcuts/minutes/andshortcuts/vc/, but the preceding comment still states the rule is "drive-only". The related comments in theforbidblock (Lines 110-115, "this is drive-scoped") and the suggestion text pointing only to the drive-local helpers inshortcuts/drive/drive_errors.go(Lines 119-120) are now misleading for minutes/vc maintainers. Please update these comments to reflect the broadened scope.📝 Suggested comment update
- # errs-no-legacy-helper is drive-only: the shared helpers it bans are - # still used by other domains until their later migration phase. + # errs-no-legacy-helper enforced on migrated domains (drive, minutes, vc): + # the shared helpers it bans are still used by not-yet-migrated domains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.golangci.yml around lines 79 - 82, Update the stale comment that says "drive-only" to reflect that the errs-no-legacy-helper rule now applies to shortcuts/drive/, shortcuts/minutes/, and shortcuts/vc/ (i.e., broaden the wording to mention drive/minutes/vc or "drive-scoped and minutes/vc"), and likewise adjust the related comments in the forbid block and the suggestion text that currently point only to shortcuts/drive/drive_errors.go so they no longer imply the rule is drive-exclusive; locate the comments around the path-except entry for the errs-no-legacy-helper rule and the forbid/suggestion comments and change their wording to accurately state the rule's expanded scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@shortcuts/vc/vc_notes.go`:
- Around line 372-381: The current success test treats any non-empty
"minute_token" as a valid payload which lets echoed tokens from
fetchNoteByMinuteToken count as success; update hasNotesPayload to ignore
"minute_token" (or require it be accompanied by real content like
"meeting_notes", "note_doc_token", "verbatim_doc_token", "shared_doc_tokens", or
"artifacts") so only actual fetched note data marks success, and adjust any
other places that relied on minute_token-as-success (including the other
occurrences you flagged) and related logic around fetchNoteByMinuteToken to
ensure echoed tokens do not short-circuit error handling.
In `@shortcuts/vc/vc_recording.go`:
- Around line 119-129: The missing-scope precheck currently uses
auth.GetStoredToken(appID, userOpenID) which misses credentials resolved via the
active resolver; update this block to call
runtime.Factory.Credential.ResolveToken(runtime.As(), appID, userOpenID)
(matching shortcuts/vc/vc_notes.go) and use the returned token when invoking
auth.MissingScopes, falling back to auth.GetStoredToken only if ResolveToken
returns nil or an error; keep the same error construction
(errs.NewPermissionError ... WithHint ... WithMissingScopes ...
WithIdentity(string(runtime.As()))) and ensure appID, userOpenID,
auth.MissingScopes, runtime.Factory.Credential.ResolveToken and runtime.As() are
the referenced symbols to locate the change.
---
Nitpick comments:
In @.golangci.yml:
- Around line 79-82: Update the stale comment that says "drive-only" to reflect
that the errs-no-legacy-helper rule now applies to shortcuts/drive/,
shortcuts/minutes/, and shortcuts/vc/ (i.e., broaden the wording to mention
drive/minutes/vc or "drive-scoped and minutes/vc"), and likewise adjust the
related comments in the forbid block and the suggestion text that currently
point only to shortcuts/drive/drive_errors.go so they no longer imply the rule
is drive-exclusive; locate the comments around the path-except entry for the
errs-no-legacy-helper rule and the forbid/suggestion comments and change their
wording to accurately state the rule's expanded scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5af2352e-e67a-4001-aba4-ee40fd098393
📒 Files selected for processing (25)
.golangci.ymlinternal/errclass/codemeta_minutes.gointernal/errclass/codemeta_vc.golint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.goshortcuts/minutes/minutes_search.goshortcuts/minutes/minutes_search_test.goshortcuts/minutes/minutes_speaker_replace.goshortcuts/minutes/minutes_speaker_replace_test.goshortcuts/minutes/minutes_update.goshortcuts/minutes/minutes_update_test.goshortcuts/minutes/minutes_upload.goshortcuts/minutes/minutes_upload_test.goshortcuts/vc/vc_meeting_events.goshortcuts/vc/vc_meeting_events_test.goshortcuts/vc/vc_meeting_join.goshortcuts/vc/vc_meeting_leave.goshortcuts/vc/vc_meeting_test.goshortcuts/vc/vc_notes.goshortcuts/vc/vc_notes_test.goshortcuts/vc/vc_recording.goshortcuts/vc/vc_recording_test.goshortcuts/vc/vc_search.goshortcuts/vc/vc_search_test.go
evandance
force-pushed
the
feat/errs-migrate-minutes-vc
branch
from
0d4fab3 to
e8db1f3
Compare
evandance
changed the title
evandance
force-pushed
the
feat/errs-migrate-minutes-vc
branch
3 times, most recently
from
2c1271c to
754a64a
Compare
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)
108-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject file paths passed to
--output-dirbefore starting downloads.Line 108 only re-stats
--output. If--output-dirpoints to an existing file, validation passes and the command does network work beforeSave(...)finally fails with a file-I/O error. Please short-circuit that case here with a typed validation error on--output-dir.💡 Suggested fix
if explicitOutputPath != "" { fi, statErr := runtime.FileIO().Stat(explicitOutputPath) switch { case statErr == nil && fi.IsDir(): explicitOutputDir = explicitOutputPath explicitOutputPath = "" case statErr == nil && !fi.IsDir(): if !single { return errs.NewValidationError(errs.SubtypeInvalidArgument, "--output %q is a file; batch mode expects a directory (use --output-dir)", explicitOutputPath).WithParam("--output") } case errors.Is(statErr, fs.ErrNotExist): if !single { explicitOutputDir = explicitOutputPath explicitOutputPath = "" } default: return errs.NewInternalError(errs.SubtypeFileIO, "cannot access --output %q: %s", explicitOutputPath, statErr).WithCause(statErr) } } + if explicitOutputDir != "" { + fi, statErr := runtime.FileIO().Stat(explicitOutputDir) + switch { + case statErr == nil && !fi.IsDir(): + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--output-dir %q is a file; expected a directory", explicitOutputDir).WithParam("--output-dir") + case statErr == nil || errors.Is(statErr, fs.ErrNotExist): + // ok + default: + return errs.NewInternalError(errs.SubtypeFileIO, "cannot access --output-dir %q: %s", explicitOutputDir, statErr).WithCause(statErr) + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/minutes/minutes_download.go` around lines 108 - 125, The code currently only validates explicitOutputPath; add a pre-download validation for explicitOutputDir so that if explicitOutputDir != "" you call runtime.FileIO().Stat(explicitOutputDir) and if statErr == nil && !fi.IsDir() return a validation error (use errs.NewValidationError(errs.SubtypeInvalidArgument, "--output-dir %q is a file; expected a directory", explicitOutputDir).WithParam("--output-dir")). Also handle errors.Is(statErr, fs.ErrNotExist) (treat as OK or create dir elsewhere per existing behavior) and propagate other statErr values as an internal file-IO error similar to the existing explicitOutputPath default case; reference variables explicitOutputDir, explicitOutputPath and the same runtime.FileIO().Stat usage so the check runs before any network/download work.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@shortcuts/minutes/minutes_download.go`:
- Around line 108-125: The code currently only validates explicitOutputPath; add
a pre-download validation for explicitOutputDir so that if explicitOutputDir !=
"" you call runtime.FileIO().Stat(explicitOutputDir) and if statErr == nil &&
!fi.IsDir() return a validation error (use
errs.NewValidationError(errs.SubtypeInvalidArgument, "--output-dir %q is a file;
expected a directory", explicitOutputDir).WithParam("--output-dir")). Also
handle errors.Is(statErr, fs.ErrNotExist) (treat as OK or create dir elsewhere
per existing behavior) and propagate other statErr values as an internal file-IO
error similar to the existing explicitOutputPath default case; reference
variables explicitOutputDir, explicitOutputPath and the same
runtime.FileIO().Stat usage so the check runs before any network/download work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb801158-5630-4ab6-a153-46c0d1de8668
📒 Files selected for processing (27)
.golangci.ymlinternal/errclass/codemeta_minutes.gointernal/errclass/codemeta_test.gointernal/errclass/codemeta_vc.golint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.goshortcuts/minutes/minutes_search.goshortcuts/minutes/minutes_search_test.goshortcuts/minutes/minutes_speaker_replace.goshortcuts/minutes/minutes_speaker_replace_test.goshortcuts/minutes/minutes_update.goshortcuts/minutes/minutes_update_test.goshortcuts/minutes/minutes_upload.goshortcuts/minutes/minutes_upload_test.goshortcuts/vc/vc_meeting_events.goshortcuts/vc/vc_meeting_events_test.goshortcuts/vc/vc_meeting_join.goshortcuts/vc/vc_meeting_leave.goshortcuts/vc/vc_meeting_test.goshortcuts/vc/vc_notes.goshortcuts/vc/vc_notes_test.goshortcuts/vc/vc_recording.goshortcuts/vc/vc_recording_test.goshortcuts/vc/vc_search.goshortcuts/vc/vc_search_test.go
✅ Files skipped from review due to trivial changes (1)
- lint/errscontract/rule_no_legacy_common_helper_call.go
🚧 Files skipped from review as they are similar to previous changes (20)
- internal/errclass/codemeta_minutes.go
- internal/errclass/codemeta_test.go
- shortcuts/minutes/minutes_upload_test.go
- internal/errclass/codemeta_vc.go
- shortcuts/minutes/minutes_update.go
- shortcuts/minutes/minutes_speaker_replace_test.go
- shortcuts/minutes/minutes_upload.go
- shortcuts/minutes/minutes_search_test.go
- shortcuts/minutes/minutes_search.go
- shortcuts/vc/vc_recording_test.go
- shortcuts/vc/vc_recording.go
- shortcuts/minutes/minutes_update_test.go
- shortcuts/minutes/minutes_download_test.go
- shortcuts/vc/vc_search.go
- shortcuts/vc/vc_meeting_events.go
- shortcuts/vc/vc_search_test.go
- shortcuts/vc/vc_notes.go
- shortcuts/minutes/minutes_speaker_replace.go
- shortcuts/vc/vc_notes_test.go
- shortcuts/vc/vc_meeting_events_test.go
evandance
force-pushed
the
feat/errs-migrate-minutes-vc
branch
8 times, most recently
from
75802ca to
bf8d1d3
Compare
Failures from the minutes and video-conference commands now surface as structured, typed errors carrying a stable category and subtype — spanning input validation, missing permissions, network and file-I/O failures, and remote API errors — so callers can branch on the error kind instead of parsing free-form text. Batch commands report partial failures explicitly, emitting per-item results with a non-zero exit instead of masking them.
evandance
force-pushed
the
feat/errs-migrate-minutes-vc
branch
from
bf8d1d3 to
501f2ba
Compare
zhaoleibd
approved these changes
Jun 8, 2026
SunPeiYang996
pushed a commit
that referenced
this pull request
Jun 8, 2026
Failures from the minutes and video-conference commands now surface as structured, typed errors carrying a stable category and subtype — spanning input validation, missing permissions, network and file-I/O failures, and remote API errors — so callers can branch on the error kind instead of parsing free-form text. Batch commands report partial failures explicitly, emitting per-item results with a non-zero exit instead of masking them.
This was referenced Jun 9, 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
Migrate error handling in the
minutesandvc(video-conference) command domains from the legacyoutput.Err*/output.Exit*/fmt.Errorfstyle to typederrs.*errors. Failures now carry a stable category and subtype, so callers can branch on the error kind — invalid input, missing permissions, network/file-I/O failures, remote API errors — instead of scraping free-form text.The migration covers both domains end to end: input/flag validation, network/transport, local file I/O, missing-scope, and remote API-boundary errors are all typed, and batch commands report partial failures explicitly.
Changes
shortcuts/minutes/(+upload,+update,+speaker-replace,+search,+download) andshortcuts/vc/(+meeting-join,+meeting-leave,+meeting-events,+search,+recording,+notes) now build typed errors for validation, network, and file-I/O failures, and route their API calls through the typed request path (runtime.CallAPITyped) so remote API errors are classified rather than surfaced as free-form envelopes.2091001are enriched at the command site; cross-command codes (2091005, vc121004/121005) are registered ininternal/errclass/codemeta_{minutes,vc}.gofor stable classification.+download,+recording,+notes) report all-failed runs as an explicit partial-failure result — per-item results on stdout with a non-zero exit — instead of a success-shaped envelope.common.*Typedvariants, so the migrated paths neither construct nor forward legacy errors. The only remaining intermediatefmt.Errorfsites (annet/httpredirect callback and a timestamp parser whose error is re-wrapped into a typed validation error) are annotated.errscontractmigrated-envelope and legacy-common-helper rules plus golangci forbidigo now forbid legacy error construction and legacy shared-helper calls onshortcuts/minutes/andshortcuts/vc/, so regressions cannot reappear.Behavioral notes
ok:falseand the per-item results still on stdout (previously a success-shaped envelope plus a generic API error).Test Plan
go build ./...— clean.go vet ./shortcuts/minutes/... ./shortcuts/vc/... ./internal/errclass/...— clean.gofmt -lon the migrated dirs — no output.go test ./shortcuts/minutes/... ./shortcuts/vc/... ./internal/errclass/...— green; tests assert the typed shape (category, subtype, param, exit code) and the partial-failure envelope.go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/main— 0 issues.go run -C lint . ..(errscontract: migrated-envelope + legacy-common-helper rules) — 0 violations on the migrated paths.Related Issues
Part of the typed-errors migration; builds on the drive-domain pattern (#1205) and the typed shared pre-checks (#1242).
Summary by CodeRabbit