feat(calendar): emit typed error envelopes across the calendar domain#1232
Merged
Conversation
evandance
added
domain/calendar
|
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:
📝 WalkthroughWalkthroughMigrate calendar shortcut commands and supporting infra from legacy envelope/string error helpers to typed ChangesCalendar Error Handling Migration to Typed Errors
Sequence diagrams: none. 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@86ca6600d4de1b0a410ffef6c444f66c6936575d🧩 Skill updatenpx skills add larksuite/cli#feat/errs-migrate-calendar -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1232 +/- ##
==========================================
+ Coverage 70.17% 70.33% +0.15%
==========================================
Files 671 672 +1
Lines 65329 65322 -7
==========================================
+ Hits 45843 45941 +98
+ Misses 15796 15728 -68
+ Partials 3690 3653 -37 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
shortcuts/calendar/calendar_create.go (1)
39-68: ⚡ Quick winConsider adding parameter context to parseAttendees errors.
The
parseAttendeesfunction returns typed validation errors (line 64) but does not include.WithParam(...)because it's called from multiple contexts (--attendee-idsin create,--add-attendee-idsin update). To preserve parameter attribution in the final error envelope, consider one of:
- Add a
paramName stringargument toparseAttendeesand call.WithParam(paramName)on line 64- Return the error as-is and let callers add the param via type assertion if needed
This would align with the PR's goal of structured parameter binding in validation errors.
🤖 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/calendar/calendar_create.go` around lines 39 - 68, parseAttendees currently returns a typed validation error without parameter context; change its signature to parseAttendees(attendeesStr string, currentUserId string, paramName string) and, where you construct the validation error in the default case, call .WithParam(paramName) on the errs.NewValidationError result; then update callers (the create path using "--attendee-ids" and the update path using "--add-attendee-ids") to pass the appropriate param name so the final error envelope preserves parameter attribution.
🤖 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.
Inline comments:
In `@shortcuts/calendar/calendar_create.go`:
- Around line 217-220: The code currently wraps the typed *errs.ValidationError
returned by parseAttendees(attendeesStr, currentUserId) into a new
errs.NewValidationError, losing the original error type and nesting messages;
instead, return the parseAttendees error directly, and if you need parameter
context call WithParam("--attendee-ids") on the returned error via a type
assertion (e.g., if ve, ok := err.(*errs.ValidationError); ok { return
ve.WithParam("--attendee-ids") } else { return err }), or modify parseAttendees
to accept the parameter name so it can produce a validation error with the param
already set.
In `@shortcuts/calendar/calendar_update.go`:
- Around line 327-330: The code in calendar_update.go is wrapping the typed
*errs.ValidationError from parseAttendees into a new errs.NewValidationError
which loses the original type; change this to return the original error directly
(return err) or, if you need parameter context, type-assert err to
*errs.ValidationError and call .WithParam("--add-attendee-ids") before
returning, or alternatively update parseAttendees to accept a parameterName and
set the param there; specifically modify the block around parseAttendees(addStr,
"") to avoid creating a new ValidationError and preserve the original error
type.
---
Nitpick comments:
In `@shortcuts/calendar/calendar_create.go`:
- Around line 39-68: parseAttendees currently returns a typed validation error
without parameter context; change its signature to parseAttendees(attendeesStr
string, currentUserId string, paramName string) and, where you construct the
validation error in the default case, call .WithParam(paramName) on the
errs.NewValidationError result; then update callers (the create path using
"--attendee-ids" and the update path using "--add-attendee-ids") to pass the
appropriate param name so the final error envelope preserves parameter
attribution.
🪄 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: 795dec8c-fecd-4e85-a2e1-98f9a42a38de
📒 Files selected for processing (8)
shortcuts/calendar/calendar_agenda.goshortcuts/calendar/calendar_create.goshortcuts/calendar/calendar_freebusy.goshortcuts/calendar/calendar_room_find.goshortcuts/calendar/calendar_rsvp.goshortcuts/calendar/calendar_suggestion.goshortcuts/calendar/calendar_test.goshortcuts/calendar/calendar_update.go
evandance
force-pushed
the
feat/errs-migrate-calendar
branch
from
June 3, 2026 07:46
c4c0438 to
82dc685
Compare
evandance
changed the title
github-actions
Bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/calendar/calendar_suggestion.go (1)
235-245:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
.WithParam("--" + flagAttendees)for attendee ID format errors.Other validation errors in this file attribute the offending flag; this one should as well.
Proposed fix
if !strings.HasPrefix(id, "ou_") && !strings.HasPrefix(id, "oc_") { - return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid attendee id format %q: should start with 'ou_' or 'oc_'", id) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid attendee id format %q: should start with 'ou_' or 'oc_'", id).WithParam("--" + flagAttendees) }🤖 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/calendar/calendar_suggestion.go` around lines 235 - 245, The attendee ID validation in the loop that reads runtime.Str(flagAttendees) returns errs.NewValidationError without attributing the offending flag; update the error creation (the return that calls errs.NewValidationError where it currently reports "invalid attendee id format %q") to chain .WithParam("--"+flagAttendees) so the error matches other validations. Locate the loop using attendeesStr and flagAttendees and replace the current errs.NewValidationError return with the same call followed by .WithParam("--"+flagAttendees).
🤖 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.
Inline comments:
In @.golangci.yml:
- Around line 79-84: The errs-no-legacy-helper lint rule is only exempting
shortcuts/drive/ and must also cover shortcuts/calendar/ so migrated calendar
code can't reintroduce legacy helpers; update the path-except for the
errs-no-legacy-helper rule (or add a sibling entry) to include
shortcuts/calendar/ alongside shortcuts/drive/, and mirror the same
configuration used for errs-no-legacy-helper (also apply the same change for the
duplicate rule instance around the 110-120 block) so the forbidigo linter bans
common.FlagErrorf, common.WrapInputStatError, and common.WrapSaveErrorByCategory
in calendar code as well.
In `@shortcuts/calendar/calendar_agenda.go`:
- Around line 156-165: parseTimeRange currently parses startTime and endTime but
doesn't validate that start <= end, allowing inverted ranges to propagate into
fetchInstanceViewRange and produce an empty agenda; add a validation after
parsing in parseTimeRange that checks if startInt > endInt and return an
errs.NewValidationError (SubtypeInvalidArgument) with a clear message like
"start time must be <= end time" and attach the relevant param(s) (e.g.,
"--start" or both) so callers (and users) get a proper validation error instead
of silent empty results.
In `@shortcuts/calendar/calendar_suggestion.go`:
- Around line 77-80: The validation error for parsing the start timestamp is
missing the flag attribution; update the error construction where
strconv.ParseInt(timeMin, 10, 64) returns an error (the minSec, err block) to
attach the parameter by calling .WithParam("--start") on the
errs.NewValidationError so the error mirrors the handling on line 75 and
attributes the failure to the --start flag.
- Around line 227-233: The validation error returned inside the loop over flags
flagEventRrule and flagTimezone should include the machine-readable parameter
via .WithParam("--"+flag); locate the block that calls runtime.Str(flag) and
common.RejectDangerousChars, and when returning errs.NewValidationError(...)
wrap or chain it with .WithParam("--"+flag) so the typed envelope's param field
is populated (keep the existing message format and error payload from
RejectDangerousChars).
- Around line 95-98: The validation error for parsing the end timestamp (where
maxSec, timeMax are used) is missing the flag attribution; update the error
returned by errs.NewValidationError to include .WithParam("--end") so it matches
the start timestamp handling (see the analogous case around line 92) and clearly
attributes the invalid argument to the --end flag.
In `@shortcuts/calendar/calendar_test.go`:
- Around line 1333-1338: The test currently only rejects nonzero APIError codes;
instead assert that the returned error is the specific internal error subtype
for invalid/unparseable responses: use errors.As to check that err is an
*errs.InternalError (not just *errs.APIError) and verify its Subtype equals
errs.SubtypeInvalidResponse (or the package's named constant for
invalid-response internal errors) so the test fails if the path returns
APIError{Code:0} or a generic error rather than the intended InternalError
subtype.
---
Outside diff comments:
In `@shortcuts/calendar/calendar_suggestion.go`:
- Around line 235-245: The attendee ID validation in the loop that reads
runtime.Str(flagAttendees) returns errs.NewValidationError without attributing
the offending flag; update the error creation (the return that calls
errs.NewValidationError where it currently reports "invalid attendee id format
%q") to chain .WithParam("--"+flagAttendees) so the error matches other
validations. Locate the loop using attendeesStr and flagAttendees and replace
the current errs.NewValidationError return with the same call followed by
.WithParam("--"+flagAttendees).
🪄 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: ef6d367c-85bd-46fb-9d6b-d6edec54704d
📒 Files selected for processing (16)
.golangci.ymlinternal/errclass/classify.gointernal/errclass/classify_test.gointernal/errclass/codemeta_calendar.gointernal/errclass/codemeta_calendar_test.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rules_test.goshortcuts/calendar/calendar_agenda.goshortcuts/calendar/calendar_create.goshortcuts/calendar/calendar_freebusy.goshortcuts/calendar/calendar_room_find.goshortcuts/calendar/calendar_rsvp.goshortcuts/calendar/calendar_suggestion.goshortcuts/calendar/calendar_test.goshortcuts/calendar/calendar_update.goshortcuts/calendar/errors.go
✅ Files skipped from review due to trivial changes (1)
- lint/errscontract/rules_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/calendar/calendar_rsvp.go
- shortcuts/calendar/calendar_freebusy.go
- shortcuts/calendar/calendar_room_find.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/calendar/calendar_suggestion.go (1)
235-245:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
.WithParam("--" + flagAttendees)for attendee ID format errors.Other validation errors in this file attribute the offending flag; this one should as well.
Proposed fix
if !strings.HasPrefix(id, "ou_") && !strings.HasPrefix(id, "oc_") { - return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid attendee id format %q: should start with 'ou_' or 'oc_'", id) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid attendee id format %q: should start with 'ou_' or 'oc_'", id).WithParam("--" + flagAttendees) }🤖 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/calendar/calendar_suggestion.go` around lines 235 - 245, The attendee ID validation in the loop that reads runtime.Str(flagAttendees) returns errs.NewValidationError without attributing the offending flag; update the error creation (the return that calls errs.NewValidationError where it currently reports "invalid attendee id format %q") to chain .WithParam("--"+flagAttendees) so the error matches other validations. Locate the loop using attendeesStr and flagAttendees and replace the current errs.NewValidationError return with the same call followed by .WithParam("--"+flagAttendees).
🤖 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.
Inline comments:
In @.golangci.yml:
- Around line 79-84: The errs-no-legacy-helper lint rule is only exempting
shortcuts/drive/ and must also cover shortcuts/calendar/ so migrated calendar
code can't reintroduce legacy helpers; update the path-except for the
errs-no-legacy-helper rule (or add a sibling entry) to include
shortcuts/calendar/ alongside shortcuts/drive/, and mirror the same
configuration used for errs-no-legacy-helper (also apply the same change for the
duplicate rule instance around the 110-120 block) so the forbidigo linter bans
common.FlagErrorf, common.WrapInputStatError, and common.WrapSaveErrorByCategory
in calendar code as well.
In `@shortcuts/calendar/calendar_agenda.go`:
- Around line 156-165: parseTimeRange currently parses startTime and endTime but
doesn't validate that start <= end, allowing inverted ranges to propagate into
fetchInstanceViewRange and produce an empty agenda; add a validation after
parsing in parseTimeRange that checks if startInt > endInt and return an
errs.NewValidationError (SubtypeInvalidArgument) with a clear message like
"start time must be <= end time" and attach the relevant param(s) (e.g.,
"--start" or both) so callers (and users) get a proper validation error instead
of silent empty results.
In `@shortcuts/calendar/calendar_suggestion.go`:
- Around line 77-80: The validation error for parsing the start timestamp is
missing the flag attribution; update the error construction where
strconv.ParseInt(timeMin, 10, 64) returns an error (the minSec, err block) to
attach the parameter by calling .WithParam("--start") on the
errs.NewValidationError so the error mirrors the handling on line 75 and
attributes the failure to the --start flag.
- Around line 227-233: The validation error returned inside the loop over flags
flagEventRrule and flagTimezone should include the machine-readable parameter
via .WithParam("--"+flag); locate the block that calls runtime.Str(flag) and
common.RejectDangerousChars, and when returning errs.NewValidationError(...)
wrap or chain it with .WithParam("--"+flag) so the typed envelope's param field
is populated (keep the existing message format and error payload from
RejectDangerousChars).
- Around line 95-98: The validation error for parsing the end timestamp (where
maxSec, timeMax are used) is missing the flag attribution; update the error
returned by errs.NewValidationError to include .WithParam("--end") so it matches
the start timestamp handling (see the analogous case around line 92) and clearly
attributes the invalid argument to the --end flag.
In `@shortcuts/calendar/calendar_test.go`:
- Around line 1333-1338: The test currently only rejects nonzero APIError codes;
instead assert that the returned error is the specific internal error subtype
for invalid/unparseable responses: use errors.As to check that err is an
*errs.InternalError (not just *errs.APIError) and verify its Subtype equals
errs.SubtypeInvalidResponse (or the package's named constant for
invalid-response internal errors) so the test fails if the path returns
APIError{Code:0} or a generic error rather than the intended InternalError
subtype.
---
Outside diff comments:
In `@shortcuts/calendar/calendar_suggestion.go`:
- Around line 235-245: The attendee ID validation in the loop that reads
runtime.Str(flagAttendees) returns errs.NewValidationError without attributing
the offending flag; update the error creation (the return that calls
errs.NewValidationError where it currently reports "invalid attendee id format
%q") to chain .WithParam("--"+flagAttendees) so the error matches other
validations. Locate the loop using attendeesStr and flagAttendees and replace
the current errs.NewValidationError return with the same call followed by
.WithParam("--"+flagAttendees).
🪄 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: ef6d367c-85bd-46fb-9d6b-d6edec54704d
📒 Files selected for processing (16)
.golangci.ymlinternal/errclass/classify.gointernal/errclass/classify_test.gointernal/errclass/codemeta_calendar.gointernal/errclass/codemeta_calendar_test.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rules_test.goshortcuts/calendar/calendar_agenda.goshortcuts/calendar/calendar_create.goshortcuts/calendar/calendar_freebusy.goshortcuts/calendar/calendar_room_find.goshortcuts/calendar/calendar_rsvp.goshortcuts/calendar/calendar_suggestion.goshortcuts/calendar/calendar_test.goshortcuts/calendar/calendar_update.goshortcuts/calendar/errors.go
✅ Files skipped from review due to trivial changes (1)
- lint/errscontract/rules_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/calendar/calendar_rsvp.go
- shortcuts/calendar/calendar_freebusy.go
- shortcuts/calendar/calendar_room_find.go
🛑 Comments failed to post (6)
.golangci.yml (1)
79-84:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winExtend
errs-no-legacy-helpertoshortcuts/calendar/.Calendar is now treated as migrated by
errs-typed-onlyanderrs-no-bare-wrap, but this rule is still drive-only. That leavescommon.FlagErrorf,common.WrapInputStatError, andcommon.WrapSaveErrorByCategoryre-introducible undershortcuts/calendar/without CI catching it, which undermines the migration-enforcement contract this PR is adding.Suggested config diff
- - path-except: (shortcuts/drive/) + - path-except: (shortcuts/drive/|shortcuts/calendar/) text: errs-no-legacy-helper linters: - forbidigoBased on learnings: migrated paths rely on
.golangci.ymlenforcement so they keep returning typederrs.NewXxxError(...)directly rather than legacy helper/output shapes.Also applies to: 110-120
🤖 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 - 84, The errs-no-legacy-helper lint rule is only exempting shortcuts/drive/ and must also cover shortcuts/calendar/ so migrated calendar code can't reintroduce legacy helpers; update the path-except for the errs-no-legacy-helper rule (or add a sibling entry) to include shortcuts/calendar/ alongside shortcuts/drive/, and mirror the same configuration used for errs-no-legacy-helper (also apply the same change for the duplicate rule instance around the 110-120 block) so the forbidigo linter bans common.FlagErrorf, common.WrapInputStatError, and common.WrapSaveErrorByCategory in calendar code as well.shortcuts/calendar/calendar_agenda.go (1)
156-165:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject inverted
--start/--endranges during validation.
parseTimeRangenow types parse failures, but it still accepts--start > --end. That bad input then falls through tofetchInstanceViewRange(Lines 34-35), which returnsnil, niland produces a misleading empty agenda instead of a validation error.Proposed fix
startInt, err := strconv.ParseInt(startTime, 10, 64) if err != nil { return 0, 0, errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid start time: %v", err).WithParam("--start") } endInt, err := strconv.ParseInt(endTime, 10, 64) if err != nil { return 0, 0, errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid end time: %v", err).WithParam("--end") } + if startInt > endInt { + return 0, 0, errs.NewValidationError( + errs.SubtypeInvalidArgument, + "--start must be earlier than or equal to --end", + ) + } return startInt, endInt, nil }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.startInt, err := strconv.ParseInt(startTime, 10, 64) if err != nil { return 0, 0, errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid start time: %v", err).WithParam("--start") } endInt, err := strconv.ParseInt(endTime, 10, 64) if err != nil { return 0, 0, errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid end time: %v", err).WithParam("--end") } if startInt > endInt { return 0, 0, errs.NewValidationError( errs.SubtypeInvalidArgument, "--start must be earlier than or equal to --end", ) } return startInt, endInt, nil🤖 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/calendar/calendar_agenda.go` around lines 156 - 165, parseTimeRange currently parses startTime and endTime but doesn't validate that start <= end, allowing inverted ranges to propagate into fetchInstanceViewRange and produce an empty agenda; add a validation after parsing in parseTimeRange that checks if startInt > endInt and return an errs.NewValidationError (SubtypeInvalidArgument) with a clear message like "start time must be <= end time" and attach the relevant param(s) (e.g., "--start" or both) so callers (and users) get a proper validation error instead of silent empty results.shortcuts/calendar/calendar_suggestion.go (3)
77-80:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
.WithParam("--start")for consistency with line 75.This error also originates from the
--startflag input but lacks flag attribution, unlike the error on line 75.Proposed fix
minSec, err := strconv.ParseInt(timeMin, 10, 64) if err != nil { - return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid start timestamp: %v", err) + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid start timestamp: %v", err).WithParam("--start") }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.minSec, err := strconv.ParseInt(timeMin, 10, 64) if err != nil { return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid start timestamp: %v", err).WithParam("--start") }🤖 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/calendar/calendar_suggestion.go` around lines 77 - 80, The validation error for parsing the start timestamp is missing the flag attribution; update the error construction where strconv.ParseInt(timeMin, 10, 64) returns an error (the minSec, err block) to attach the parameter by calling .WithParam("--start") on the errs.NewValidationError so the error mirrors the handling on line 75 and attributes the failure to the --start flag.
95-98:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
.WithParam("--end")for consistency with line 92.Same issue as the start timestamp error—this also originates from
--endbut lacks flag attribution.Proposed fix
maxSec, err := strconv.ParseInt(timeMax, 10, 64) if err != nil { - return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid end timestamp: %v", err) + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid end timestamp: %v", err).WithParam("--end") }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.maxSec, err := strconv.ParseInt(timeMax, 10, 64) if err != nil { return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid end timestamp: %v", err).WithParam("--end") }🤖 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/calendar/calendar_suggestion.go` around lines 95 - 98, The validation error for parsing the end timestamp (where maxSec, timeMax are used) is missing the flag attribution; update the error returned by errs.NewValidationError to include .WithParam("--end") so it matches the start timestamp handling (see the analogous case around line 92) and clearly attributes the invalid argument to the --end flag.
227-233:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
.WithParam("--" + flag)for machine-readable flag attribution.The error message contains the flag name, but the typed envelope's
paramfield won't be set without.WithParam().Proposed fix
if val := runtime.Str(flag); val != "" { if err := common.RejectDangerousChars("--"+flag, val); err != nil { - return errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err).WithParam("--" + flag) } }🤖 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/calendar/calendar_suggestion.go` around lines 227 - 233, The validation error returned inside the loop over flags flagEventRrule and flagTimezone should include the machine-readable parameter via .WithParam("--"+flag); locate the block that calls runtime.Str(flag) and common.RejectDangerousChars, and when returning errs.NewValidationError(...) wrap or chain it with .WithParam("--"+flag) so the typed envelope's param field is populated (keep the existing message format and error payload from RejectDangerousChars).shortcuts/calendar/calendar_test.go (1)
1333-1338:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert the internal-error contract here, not just “no nonzero API code.”
Line 1336 only fails when the regression becomes
*errs.APIErrorwith a nonzero code. This still passes if the path starts returning*errs.APIError{Code:0}or an untyped/generic error, even though this PR’s contract says unparseable API responses are typed internal failures. Please pin this to*errs.InternalError(ideallySubtypeInvalidResponse) so the migration can’t silently drift.Proposed tightening
if err == nil { t.Fatal("expected error for non-JSON response, got nil") } - // A non-JSON 200 body is not an API business error: it must not surface as - // a typed APIError carrying a Lark business code. - var ae *errs.APIError - if errors.As(err, &ae) && ae.Code != 0 { - t.Fatalf("expected non-API error passthrough, got API error code %d", ae.Code) - } + // A non-JSON 200 body is an internal invalid-response failure, not an API business error. + var ie *errs.InternalError + if !errors.As(err, &ie) { + t.Fatalf("expected *errs.InternalError, got %T", err) + } + if ie.Subtype != errs.SubtypeInvalidResponse { + t.Fatalf("subtype=%q, want invalid_response", ie.Subtype) + }🤖 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/calendar/calendar_test.go` around lines 1333 - 1338, The test currently only rejects nonzero APIError codes; instead assert that the returned error is the specific internal error subtype for invalid/unparseable responses: use errors.As to check that err is an *errs.InternalError (not just *errs.APIError) and verify its Subtype equals errs.SubtypeInvalidResponse (or the package's named constant for invalid-response internal errors) so the test fails if the path returns APIError{Code:0} or a generic error rather than the intended InternalError subtype.
evandance
force-pushed
the
feat/errs-migrate-calendar
branch
from
June 3, 2026 08:32
82dc685 to
75cac51
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/calendar/calendar_test.go (1)
1314-1339: ⚡ Quick winConsider asserting the expected error type more precisely.
The current assertion checks that the error is not an
*errs.APIErrorwith a non-zero business code, but it doesn't verify the expected type. For a non-JSON unmarshal failure, the error should be*errs.InternalErrorwithSubtypeInvalidResponse(consistent with the pattern inTestSuggestion_UnmarshalFail_TypedandTestRoomFind_UnmarshalFail_Typedbelow).🔍 Suggested assertion improvement
- // A non-JSON 200 body is not an API business error: it must not surface as - // a typed APIError carrying a Lark business code. - var ae *errs.APIError - if errors.As(err, &ae) && ae.Code != 0 { - t.Fatalf("expected non-API error passthrough, got API error code %d", ae.Code) + // A non-JSON 200 body should surface as an InternalError with invalid_response subtype. + var ie *errs.InternalError + if !errors.As(err, &ie) { + t.Fatalf("expected *errs.InternalError, got %T", err) + } + if ie.Subtype != errs.SubtypeInvalidResponse { + t.Errorf("subtype=%q, want invalid_response", ie.Subtype) }🤖 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/calendar/calendar_test.go` around lines 1314 - 1339, The test TestAgenda_NonAPIError_Passthrough currently only ensures a non-JSON response isn't treated as an API business error; update it to assert the concrete error type and subtype expected for unmarshal failures instead: after mountAndRun returns an error, assert that errors.As(err, &errs.InternalError{}) (or use a variable of type *errs.InternalError) succeeds and that the InternalError.Subtype equals errs.SubtypeInvalidResponse, mirroring the checks in TestSuggestion_UnmarshalFail_Typed and TestRoomFind_UnmarshalFail_Typed so the test precisely verifies the unmarshal failure is mapped to an InternalError with SubtypeInvalidResponse.
🤖 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.
Nitpick comments:
In `@shortcuts/calendar/calendar_test.go`:
- Around line 1314-1339: The test TestAgenda_NonAPIError_Passthrough currently
only ensures a non-JSON response isn't treated as an API business error; update
it to assert the concrete error type and subtype expected for unmarshal failures
instead: after mountAndRun returns an error, assert that errors.As(err,
&errs.InternalError{}) (or use a variable of type *errs.InternalError) succeeds
and that the InternalError.Subtype equals errs.SubtypeInvalidResponse, mirroring
the checks in TestSuggestion_UnmarshalFail_Typed and
TestRoomFind_UnmarshalFail_Typed so the test precisely verifies the unmarshal
failure is mapped to an InternalError with SubtypeInvalidResponse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c0c1732-05d3-460d-81b5-1a4830a33736
📒 Files selected for processing (17)
.golangci.ymlinternal/errclass/classify.gointernal/errclass/classify_test.gointernal/errclass/codemeta_calendar.gointernal/errclass/codemeta_calendar_test.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rules_test.goshortcuts/calendar/calendar_agenda.goshortcuts/calendar/calendar_create.goshortcuts/calendar/calendar_freebusy.goshortcuts/calendar/calendar_room_find.goshortcuts/calendar/calendar_rsvp.goshortcuts/calendar/calendar_suggestion.goshortcuts/calendar/calendar_test.goshortcuts/calendar/calendar_update.goshortcuts/calendar/errors.goshortcuts/calendar/errors_attribution_test.go
✅ Files skipped from review due to trivial changes (1)
- .golangci.yml
🚧 Files skipped from review as they are similar to previous changes (11)
- lint/errscontract/rules_test.go
- lint/errscontract/rule_no_legacy_envelope_literal.go
- internal/errclass/codemeta_calendar_test.go
- shortcuts/calendar/calendar_freebusy.go
- internal/errclass/classify.go
- internal/errclass/codemeta_calendar.go
- shortcuts/calendar/calendar_create.go
- shortcuts/calendar/calendar_update.go
- shortcuts/calendar/errors.go
- shortcuts/calendar/calendar_agenda.go
- shortcuts/calendar/calendar_suggestion.go
evandance
force-pushed
the
feat/errs-migrate-calendar
branch
7 times, most recently
from
June 4, 2026 13:31
0a114c7 to
546ef53
Compare
Calendar commands now return structured, typed error envelopes for every failure mode — input validation, internal faults, and API responses — instead of legacy generic errors. Callers and AI agents get consistent exit codes and a machine-readable shape (type / subtype / code / hint), and can tell bad input, an internal fault, and an API rejection apart. Validation errors are attributed to the offending flag. Server-supplied error details (e.g. why an event time was rejected) are surfaced on the typed error's hint via a shared classifier improvement that benefits every domain. Multi-step operations (create-with-attendees rollback, multi-field update) preserve the real failure's classification and report which steps completed. The whole calendar domain is now lint-locked against reintroducing legacy error constructors.
evandance
force-pushed
the
feat/errs-migrate-calendar
branch
from
June 5, 2026 03:50
546ef53 to
86ca660
Compare
calendar-assistant
approved these changes
Jun 5, 2026
tuxedomm
pushed a commit
to zhumiaoxin/cli
that referenced
this pull request
Jun 6, 2026
…larksuite#1232) Calendar commands now return structured, typed error envelopes for every failure mode — input validation, internal faults, and API responses — instead of legacy generic errors. Callers and AI agents get consistent exit codes and a machine-readable shape (type / subtype / code / hint), and can tell bad input, an internal fault, and an API rejection apart. Validation errors are attributed to the offending flag. Server-supplied error details (e.g. why an event time was rejected) are surfaced on the typed error's hint via a shared classifier improvement that benefits every domain. Multi-step operations (create-with-attendees rollback, multi-field update) preserve the real failure's classification and report which steps completed. The whole calendar domain is now lint-locked against reintroducing legacy error constructors.
This was referenced Jun 9, 2026
5 tasks
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
Calendar commands now return structured, typed error envelopes across the whole domain — input validation, internal faults, and API-response failures — replacing the previous legacy/generic errors. Callers, including AI agents, get consistent exit codes and a machine-readable shape (
type/subtype/code/hint), so bad input, an internal fault, and an API rejection are reliably distinguishable.This also lands a small shared classifier improvement (server
error.details→ the typedhint) that benefits every domain, and lint-locks the calendar domain against reintroducing legacy error constructors.Changes
+agenda,+create,+freebusy,+rsvp,+room-find,+suggestion,+update→ typed validation errors with the offending flag attributed.CallAPITyped/ the shared classifier); the legacy predefined-error wrapper is removed.+agenda's recursive time-window split now keys on the typed error code.+createwith-attendees rollback, multi-field+update) preserve the underlying failure's real classification and report which steps completed plus rollback diagnostics.error.detailsinto the typedhint(generic — all domains benefit).+agendasplit and the create rollback-failure path are covered.Test Plan
go build ./...go vet ./shortcuts/calendar/... ./internal/errclass/...go test ./shortcuts/calendar/... ./internal/errclass/...— all passgofmt -l .— cleangolangci-lint run --new-from-rev=origin/main— 0 issueserrscontract(go run -C lint . ..) — 0 violations (calendar now in scope)Related Issues
Part of the typed-error envelope rollout across CLI domains (follows the drive domain). No linked issue.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Refactor