feat(okr,whiteboard): emit typed error envelopes across both domains#1236
Merged
Conversation
|
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 CLI validation to typed errs with flag metadata, replace runtime.DoAPI/DoAPIJSON and larkcore QueryParams with runtime.CallAPITyped and map-based params across OKR and Whiteboard shortcuts; add local error mappers, expand lint scope, and update tests to assert typed error envelopes. ChangesCLI Validation Error Type Migration & API call migration
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 |
github-actions
Bot
added
the
size/L
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1236 +/- ##
==========================================
+ Coverage 70.33% 70.40% +0.07%
==========================================
Files 672 674 +2
Lines 65322 65334 +12
==========================================
+ Hits 45941 45996 +55
+ Misses 15728 15697 -31
+ Partials 3653 3641 -12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@81e48c7365458d505631fe944432c1b4be61cf51🧩 Skill updatenpx skills add larksuite/cli#feat/errs-migrate-okr-whiteboard -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: 1
♻️ Duplicate comments (1)
shortcuts/okr/okr_progress_update.go (1)
108-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGrammar: "must provided" → "must be provided".
Same wording issue as
okr_progress_create.goLine 158.✏️ Proposed fix
- return errs.NewValidationError(errs.SubtypeInvalidArgument, "--progress-percent must provided with --progress-status").WithParam("--progress-percent") + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--progress-percent must be provided with --progress-status").WithParam("--progress-percent")🤖 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/okr/okr_progress_update.go` around lines 108 - 110, Update the validation error message that currently reads "--progress-percent must provided with --progress-status" to correct grammar: "--progress-percent must be provided with --progress-status". Change the string in the code path where runtime.Str("progress-percent") == "" (the errs.NewValidationError(...) call) and make the identical change in the similar message in okr_progress_create.go to keep wording consistent.
🤖 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/okr/okr_progress_create.go`:
- Around line 157-159: Update the user-facing validation error message to
correct the grammar: change the string returned when
runtime.Str("progress-percent") is empty inside the create handler (the block
that returns errs.NewValidationError(...).WithParam("--progress-percent")) from
" --progress-percent must provided with --progress-status" to "
--progress-percent must be provided with --progress-status" so the message reads
" --progress-percent must be provided with --progress-status".
---
Duplicate comments:
In `@shortcuts/okr/okr_progress_update.go`:
- Around line 108-110: Update the validation error message that currently reads
"--progress-percent must provided with --progress-status" to correct grammar:
"--progress-percent must be provided with --progress-status". Change the string
in the code path where runtime.Str("progress-percent") == "" (the
errs.NewValidationError(...) call) and make the identical change in the similar
message in okr_progress_create.go to keep wording consistent.
🪄 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: 905c6ac6-b988-44a3-b7aa-4fed629d6643
📒 Files selected for processing (13)
shortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_detail_test.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_image_upload.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_delete.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_list.goshortcuts/okr/okr_progress_update.goshortcuts/whiteboard/whiteboard_query.goshortcuts/whiteboard/whiteboard_query_test.goshortcuts/whiteboard/whiteboard_update.goshortcuts/whiteboard/whiteboard_update_test.go
evandance
force-pushed
the
feat/errs-migrate-okr-whiteboard
branch
from
0b7e027 to
bbfed60
Compare
evandance
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/whiteboard/whiteboard_query.go (1)
321-349:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat every
Stat(outPath)failure as “path does not exist”.The initial
runtime.FileIO().Stat(outPath)probe is only deciding “existing directory vs not”, but the currentelsebranch also swallows path-validation and other filesystem failures, then later may return the rawruntime.ValidatePath(finalPath)error instead of a typed envelope. That breaks the typed-error contract this PR is migrating to and can turn a bad--outputinto a misleading save/create failure. Only fall through on not-exist; surface all other probe/validation failures as typed--outputerrors.Based on learnings: when checking if a user-supplied
--outputpath is an existing directory, explicitly distinguishfs.ErrNotExistfrom path-validation and other filesystem errors instead of swallowing them.Also applies to: 363-367
🤖 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/whiteboard/whiteboard_query.go` around lines 321 - 349, The initial runtime.FileIO().Stat(outPath) call in saveOutputFile is currently treating all errors as “not a directory” and falling through; change that so you only continue when os.IsNotExist(err) and immediately return a typed validation error for any other Stat/FileIO error (e.g., using errs.NewValidationError with SubtypeInvalidArgument and include the original error and the "--output" param/context). Do the same fix for the similar Stat probe later (the existence check for finalPath) so non-NotExist probe failures are surfaced as typed --output/path validation errors rather than swallowed or turned into generic internal errors; keep runtime.ValidatePath(finalPath) usage for a separate validation step.
🤖 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 110-120: The lint rule message for the banned legacy helpers
(pattern matching common.FlagErrorf, common.WrapInputStatError,
common.WrapSaveErrorByCategory) still refers to "drive-only" helpers
(driveInputStatError / driveSaveError) but the rule now also applies to
shortcuts/okr and shortcuts/whiteboard; update the msg text to remove or broaden
the drive-only guidance and point to the correct, non-drive-specific
alternatives (use typed errs.NewXxxError builders or the appropriate
shortcuts-local helpers) so developers in OKR/Whiteboard see accurate migration
guidance instead of being directed only to drive helpers.
In `@shortcuts/okr/okr_errors.go`:
- Around line 16-23: The function okrInputStatError currently returns validation
errors for file path/open failures without the typed-input param metadata;
update both branches (the errors.Is(err, fileio.ErrPathValidation) case and the
generic "cannot read file" return) to attach the stable param metadata "--file"
to the returned errs.ValidationError (e.g., by calling the library method that
adds param metadata such as WithParam("--file") or constructing the error with
param) so the typed envelope keeps the "--file" param for downstream branching.
In `@shortcuts/okr/okr_image_upload.go`:
- Around line 43-65: In Validate (function Validate in okr_image_upload.go) add
a runtime.FileIO().Stat(filePath) check to reject unsafe absolute or traversal
paths at validation time: call runtime.FileIO().Stat(filePath) and if it returns
an error other than os.IsNotExist(err) propagate a validation error (same
subtype/WithParam pattern used for other flags), but ignore os.IsNotExist so
dry-run continues to work; mirror the behavior of validateMediaFlagPath by
treating only missing-file as acceptable while rejecting other Stat errors and
unsafe paths.
In `@shortcuts/whiteboard/whiteboard_query.go`:
- Around line 127-128: The fallback error message uses the wrong flag name
("--as")—update the default case in the switch inside whiteboard_query.go so the
error text references the actual flag "--output_as" (the WithParam call already
uses "--output_as"); ensure the returned errs.NewValidationError(...) message
string and the WithParam(...) value consistently use "--output_as" to avoid
pointing users to a nonexistent flag.
- Around line 139-151: The preview-path error handling currently collapses all
non-5xx responses into errs.NewNetworkError (transport), which hides real 4xx
API errors; update the resp.StatusCode handling in the block after runtime.DoAPI
so that 500+ still returns a server/network error via errs.NewNetworkError, but
404 is returned as the typed not_found error and other 4xx are returned as the
API/status error type used by shortcuts/common/runner.go (reuse that
classification logic or its helper), preserving the HTTP body/message in the
error formatting instead of turning everything into a transport error; reference
symbols: runtime.DoAPI, wrapWbNetworkErr, resp.StatusCode, resp.RawBody, and
replace the non-5xx errs.NewNetworkError call with the appropriate errs.*
API/status/not_found constructors.
---
Outside diff comments:
In `@shortcuts/whiteboard/whiteboard_query.go`:
- Around line 321-349: The initial runtime.FileIO().Stat(outPath) call in
saveOutputFile is currently treating all errors as “not a directory” and falling
through; change that so you only continue when os.IsNotExist(err) and
immediately return a typed validation error for any other Stat/FileIO error
(e.g., using errs.NewValidationError with SubtypeInvalidArgument and include the
original error and the "--output" param/context). Do the same fix for the
similar Stat probe later (the existence check for finalPath) so non-NotExist
probe failures are surfaced as typed --output/path validation errors rather than
swallowed or turned into generic internal errors; keep
runtime.ValidatePath(finalPath) usage for a separate validation step.
🪄 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: b14377dd-6332-4ad6-86cd-8749805d6444
📒 Files selected for processing (17)
.golangci.ymlshortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_detail_test.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_errors.goshortcuts/okr/okr_image_upload.goshortcuts/okr/okr_image_upload_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_delete.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_list.goshortcuts/okr/okr_progress_update.goshortcuts/whiteboard/whiteboard_errors.goshortcuts/whiteboard/whiteboard_query.goshortcuts/whiteboard/whiteboard_query_test.goshortcuts/whiteboard/whiteboard_update.goshortcuts/whiteboard/whiteboard_update_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/okr/okr_cycle_detail_test.go
- shortcuts/whiteboard/whiteboard_query_test.go
- shortcuts/okr/okr_progress_create.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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/whiteboard/whiteboard_query.go (1)
321-349:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat every
Stat(outPath)failure as “path does not exist”.The initial
runtime.FileIO().Stat(outPath)probe is only deciding “existing directory vs not”, but the currentelsebranch also swallows path-validation and other filesystem failures, then later may return the rawruntime.ValidatePath(finalPath)error instead of a typed envelope. That breaks the typed-error contract this PR is migrating to and can turn a bad--outputinto a misleading save/create failure. Only fall through on not-exist; surface all other probe/validation failures as typed--outputerrors.Based on learnings: when checking if a user-supplied
--outputpath is an existing directory, explicitly distinguishfs.ErrNotExistfrom path-validation and other filesystem errors instead of swallowing them.Also applies to: 363-367
🤖 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/whiteboard/whiteboard_query.go` around lines 321 - 349, The initial runtime.FileIO().Stat(outPath) call in saveOutputFile is currently treating all errors as “not a directory” and falling through; change that so you only continue when os.IsNotExist(err) and immediately return a typed validation error for any other Stat/FileIO error (e.g., using errs.NewValidationError with SubtypeInvalidArgument and include the original error and the "--output" param/context). Do the same fix for the similar Stat probe later (the existence check for finalPath) so non-NotExist probe failures are surfaced as typed --output/path validation errors rather than swallowed or turned into generic internal errors; keep runtime.ValidatePath(finalPath) usage for a separate validation step.
🤖 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 110-120: The lint rule message for the banned legacy helpers
(pattern matching common.FlagErrorf, common.WrapInputStatError,
common.WrapSaveErrorByCategory) still refers to "drive-only" helpers
(driveInputStatError / driveSaveError) but the rule now also applies to
shortcuts/okr and shortcuts/whiteboard; update the msg text to remove or broaden
the drive-only guidance and point to the correct, non-drive-specific
alternatives (use typed errs.NewXxxError builders or the appropriate
shortcuts-local helpers) so developers in OKR/Whiteboard see accurate migration
guidance instead of being directed only to drive helpers.
In `@shortcuts/okr/okr_errors.go`:
- Around line 16-23: The function okrInputStatError currently returns validation
errors for file path/open failures without the typed-input param metadata;
update both branches (the errors.Is(err, fileio.ErrPathValidation) case and the
generic "cannot read file" return) to attach the stable param metadata "--file"
to the returned errs.ValidationError (e.g., by calling the library method that
adds param metadata such as WithParam("--file") or constructing the error with
param) so the typed envelope keeps the "--file" param for downstream branching.
In `@shortcuts/okr/okr_image_upload.go`:
- Around line 43-65: In Validate (function Validate in okr_image_upload.go) add
a runtime.FileIO().Stat(filePath) check to reject unsafe absolute or traversal
paths at validation time: call runtime.FileIO().Stat(filePath) and if it returns
an error other than os.IsNotExist(err) propagate a validation error (same
subtype/WithParam pattern used for other flags), but ignore os.IsNotExist so
dry-run continues to work; mirror the behavior of validateMediaFlagPath by
treating only missing-file as acceptable while rejecting other Stat errors and
unsafe paths.
In `@shortcuts/whiteboard/whiteboard_query.go`:
- Around line 127-128: The fallback error message uses the wrong flag name
("--as")—update the default case in the switch inside whiteboard_query.go so the
error text references the actual flag "--output_as" (the WithParam call already
uses "--output_as"); ensure the returned errs.NewValidationError(...) message
string and the WithParam(...) value consistently use "--output_as" to avoid
pointing users to a nonexistent flag.
- Around line 139-151: The preview-path error handling currently collapses all
non-5xx responses into errs.NewNetworkError (transport), which hides real 4xx
API errors; update the resp.StatusCode handling in the block after runtime.DoAPI
so that 500+ still returns a server/network error via errs.NewNetworkError, but
404 is returned as the typed not_found error and other 4xx are returned as the
API/status error type used by shortcuts/common/runner.go (reuse that
classification logic or its helper), preserving the HTTP body/message in the
error formatting instead of turning everything into a transport error; reference
symbols: runtime.DoAPI, wrapWbNetworkErr, resp.StatusCode, resp.RawBody, and
replace the non-5xx errs.NewNetworkError call with the appropriate errs.*
API/status/not_found constructors.
---
Outside diff comments:
In `@shortcuts/whiteboard/whiteboard_query.go`:
- Around line 321-349: The initial runtime.FileIO().Stat(outPath) call in
saveOutputFile is currently treating all errors as “not a directory” and falling
through; change that so you only continue when os.IsNotExist(err) and
immediately return a typed validation error for any other Stat/FileIO error
(e.g., using errs.NewValidationError with SubtypeInvalidArgument and include the
original error and the "--output" param/context). Do the same fix for the
similar Stat probe later (the existence check for finalPath) so non-NotExist
probe failures are surfaced as typed --output/path validation errors rather than
swallowed or turned into generic internal errors; keep
runtime.ValidatePath(finalPath) usage for a separate validation step.
🪄 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: b14377dd-6332-4ad6-86cd-8749805d6444
📒 Files selected for processing (17)
.golangci.ymlshortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_detail_test.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_errors.goshortcuts/okr/okr_image_upload.goshortcuts/okr/okr_image_upload_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_delete.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_list.goshortcuts/okr/okr_progress_update.goshortcuts/whiteboard/whiteboard_errors.goshortcuts/whiteboard/whiteboard_query.goshortcuts/whiteboard/whiteboard_query_test.goshortcuts/whiteboard/whiteboard_update.goshortcuts/whiteboard/whiteboard_update_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/okr/okr_cycle_detail_test.go
- shortcuts/whiteboard/whiteboard_query_test.go
- shortcuts/okr/okr_progress_create.go
🛑 Comments failed to post (5)
.golangci.yml (1)
110-120:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale drive-only lint guidance to match expanded domains.
Line 110 and Line 118 still describe this rule as drive-only, but Line 81 now enforces it for
shortcuts/okr/andshortcuts/whiteboard/too. The current message can send developers to the wrong helper implementation.Suggested diff
- # ── legacy shared error helpers banned on drive ── + # ── legacy shared error helpers banned on migrated shortcut domains ── # These helpers internally produce legacy output.Err* shapes, so they - # are invisible to the errs-typed-only ban above. Drive has migrated its - # calls to typed errs.* (drive-local driveInputStatError / driveSaveError); - # this prevents reintroduction. Other domains still use the shared - # helpers (migrated globally in a later phase), so this is drive-scoped. + # are invisible to the errs-typed-only ban above. Drive/OKR/Whiteboard + # have migrated to typed errs.* and domain-local wrappers; this prevents + # reintroduction in those domains. - pattern: (common\.FlagErrorf|common\.WrapInputStatError|common\.WrapSaveErrorByCategory)\b msg: >- [errs-no-legacy-helper] these shared helpers emit legacy output.Err* - shapes. Use the typed errs.NewXxxError builders or the drive-local - driveInputStatError / driveSaveError helpers (shortcuts/drive/drive_errors.go). + shapes. Use typed errs.NewXxxError builders or the domain-local typed + helper wrappers in the migrated shortcut package.🤖 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 110 - 120, The lint rule message for the banned legacy helpers (pattern matching common.FlagErrorf, common.WrapInputStatError, common.WrapSaveErrorByCategory) still refers to "drive-only" helpers (driveInputStatError / driveSaveError) but the rule now also applies to shortcuts/okr and shortcuts/whiteboard; update the msg text to remove or broaden the drive-only guidance and point to the correct, non-drive-specific alternatives (use typed errs.NewXxxError builders or the appropriate shortcuts-local helpers) so developers in OKR/Whiteboard see accurate migration guidance instead of being directed only to drive helpers.shortcuts/okr/okr_errors.go (1)
16-23:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
--fileon the typed envelope.
okrInputStatErrordrops the offending flag from file-path failures, so--filestat/open errors no longer carry the stableparammetadata that the rest of this migration emits. That makes these envelopes harder for callers to branch on and inconsistent with the new typed-input contract.🔧 Proposed fix
func okrInputStatError(err error) error { if err == nil { return nil } if errors.Is(err, fileio.ErrPathValidation) { - return errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe file path: %s", err).WithCause(err) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe file path: %s", err).WithParam("--file").WithCause(err) } - return errs.NewValidationError(errs.SubtypeInvalidArgument, "cannot read file: %s", err).WithCause(err) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "cannot read file: %s", err).WithParam("--file").WithCause(err) }🤖 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/okr/okr_errors.go` around lines 16 - 23, The function okrInputStatError currently returns validation errors for file path/open failures without the typed-input param metadata; update both branches (the errors.Is(err, fileio.ErrPathValidation) case and the generic "cannot read file" return) to attach the stable param metadata "--file" to the returned errs.ValidationError (e.g., by calling the library method that adds param metadata such as WithParam("--file") or constructing the error with param) so the typed envelope keeps the "--file" param for downstream branching.shortcuts/okr/okr_image_upload.go (1)
43-65:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject unsafe
--filepaths duringValidate, not only inExecute.Right now
Validateonly checks the extension. On--dry-run, this shortcut never reachesruntime.FileIO().Stat/Open, so absolute or traversal paths pass validation and look runnable even though the real upload path would reject them later. Please run a validation-timeruntime.FileIO().Stat(filePath)here and ignore only missing-file errors so dry-run keeps working without the file existing on disk.🔧 Proposed fix
ext := strings.ToLower(filepath.Ext(filePath)) if !allowedImageExts[ext] { return errs.NewValidationError(errs.SubtypeInvalidArgument, "--file must be an image (supported: JPG, JPEG, PNG, GIF, BMP), got %q", ext).WithParam("--file") } + if _, err := runtime.FileIO().Stat(filePath); err != nil && !os.IsNotExist(err) { + return okrInputStatError(err) + } targetID := runtime.Str("target-id")Based on learnings:
validateMediaFlagPathintentionally skips onlyos.IsNotExistfromfio.Statso dry-run still enforces path safety without requiring the local file to exist.🤖 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/okr/okr_image_upload.go` around lines 43 - 65, In Validate (function Validate in okr_image_upload.go) add a runtime.FileIO().Stat(filePath) check to reject unsafe absolute or traversal paths at validation time: call runtime.FileIO().Stat(filePath) and if it returns an error other than os.IsNotExist(err) propagate a validation error (same subtype/WithParam pattern used for other flags), but ignore os.IsNotExist so dry-run continues to work; mirror the behavior of validateMediaFlagPath by treating only missing-file as acceptable while rejecting other Stat errors and unsafe paths.shortcuts/whiteboard/whiteboard_query.go (2)
127-128:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the actual flag name in this fallback error.
This message says
--as, but the command only exposes--output_as, so this path points users at a nonexistent 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/whiteboard/whiteboard_query.go` around lines 127 - 128, The fallback error message uses the wrong flag name ("--as")—update the default case in the switch inside whiteboard_query.go so the error text references the actual flag "--output_as" (the WithParam call already uses "--output_as"); ensure the returned errs.NewValidationError(...) message string and the WithParam(...) value consistently use "--output_as" to avoid pointing users to a nonexistent flag.
139-151:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't collapse preview HTTP 4xx responses into
network/transport.
shortcuts/common/runner.goalready classifies HTTP status failures into stable typed shapes (404→ not_found, other4xx→ API/status error,5xx→ server_error). This branch now reports every non-5xx preview failure as a transport error, so a bad token or missing permission becomes indistinguishable from a real network failure.🤖 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/whiteboard/whiteboard_query.go` around lines 139 - 151, The preview-path error handling currently collapses all non-5xx responses into errs.NewNetworkError (transport), which hides real 4xx API errors; update the resp.StatusCode handling in the block after runtime.DoAPI so that 500+ still returns a server/network error via errs.NewNetworkError, but 404 is returned as the typed not_found error and other 4xx are returned as the API/status error type used by shortcuts/common/runner.go (reuse that classification logic or its helper), preserving the HTTP body/message in the error formatting instead of turning everything into a transport error; reference symbols: runtime.DoAPI, wrapWbNetworkErr, resp.StatusCode, resp.RawBody, and replace the non-5xx errs.NewNetworkError call with the appropriate errs.* API/status/not_found constructors.
evandance
force-pushed
the
feat/errs-migrate-okr-whiteboard
branch
from
bbfed60 to
4b19089
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/whiteboard/whiteboard_errors_test.go (1)
14-26: ⚡ Quick winVerify cause preservation for consistency with OKR tests.
Unlike the similar OKR test (
TestWrapOkrNetworkErrinokr_errors_test.go:44-46), this test doesn't verify that the original error is retained viaerrors.Is(got, raw). The implementation uses.WithCause(err), so the cause chain exists and should be tested for consistency.✨ Add cause verification
raw := errors.New("connection reset by peer") got := wrapWbNetworkErr(raw, "fetch failed: %v", raw) var ne *errs.NetworkError if !errors.As(got, &ne) || ne.Subtype != errs.SubtypeNetworkTransport { t.Fatalf("raw error: got %T (%v)", got, got) } + if !errors.Is(got, raw) { + t.Fatal("raw cause should be retained via WithCause") + } }🤖 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/whiteboard/whiteboard_errors_test.go` around lines 14 - 26, Add a check that the original raw error is preserved in the wrapped error by using errors.Is; inside TestWrapWbNetworkErr after obtaining got from wrapWbNetworkErr(raw, ...), assert that errors.Is(got, raw) is true (use the existing raw variable), so the test verifies the cause chain created by wrapWbNetworkErr and matches the OKR test behavior for TestWrapOkrNetworkErr.shortcuts/okr/okr_errors_test.go (1)
14-30: 💤 Low valueOptional: Verify cause preservation for consistency.
The test correctly checks type and subtype, but doesn't verify that the original error is preserved in the unwrap chain via
errors.Is, unlikeTestWrapOkrNetworkErr(lines 44-46). Adding cause checks would improve consistency and match the thoroughness of the network error test.✨ Optional enhancement
pathErr := okrInputStatError(&fileio.PathValidationError{Err: errors.New("traversal")}) if !errors.As(pathErr, &ve) || ve.Subtype != errs.SubtypeInvalidArgument { t.Fatalf("path validation: got %T (%v)", pathErr, pathErr) } + if !errors.Is(pathErr, fileio.ErrPathValidation) { + t.Fatal("path validation: cause should be retained") + } - genericErr := okrInputStatError(errors.New("permission denied")) + origErr := errors.New("permission denied") + genericErr := okrInputStatError(origErr) if !errors.As(genericErr, &ve) || ve.Subtype != errs.SubtypeInvalidArgument { t.Fatalf("generic: got %T (%v)", genericErr, genericErr) } + if !errors.Is(genericErr, origErr) { + t.Fatal("generic: cause should be retained") + }🤖 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/okr/okr_errors_test.go` around lines 14 - 30, Update TestOkrInputStatError to also assert that the original causes are preserved by using errors.Is on the returned error from okrInputStatError; specifically, after calling okrInputStatError with &fileio.PathValidationError{Err: errors.New("traversal")} and with errors.New("permission denied"), add checks that errors.Is(pathErr, theOriginalErr) and errors.Is(genericErr, theOriginalErr) succeed (while keeping the existing ValidationError type/subtype assertions for errs.ValidationError), so the test verifies the unwrap/causal chain is preserved.
🤖 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/okr/okr_errors_test.go`:
- Around line 14-30: Update TestOkrInputStatError to also assert that the
original causes are preserved by using errors.Is on the returned error from
okrInputStatError; specifically, after calling okrInputStatError with
&fileio.PathValidationError{Err: errors.New("traversal")} and with
errors.New("permission denied"), add checks that errors.Is(pathErr,
theOriginalErr) and errors.Is(genericErr, theOriginalErr) succeed (while keeping
the existing ValidationError type/subtype assertions for errs.ValidationError),
so the test verifies the unwrap/causal chain is preserved.
In `@shortcuts/whiteboard/whiteboard_errors_test.go`:
- Around line 14-26: Add a check that the original raw error is preserved in the
wrapped error by using errors.Is; inside TestWrapWbNetworkErr after obtaining
got from wrapWbNetworkErr(raw, ...), assert that errors.Is(got, raw) is true
(use the existing raw variable), so the test verifies the cause chain created by
wrapWbNetworkErr and matches the OKR test behavior for TestWrapOkrNetworkErr.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c82db5b-02bd-4552-8576-d0d8748f1d74
📒 Files selected for processing (20)
.golangci.ymlshortcuts/common/runner.goshortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_detail_test.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_errors.goshortcuts/okr/okr_errors_test.goshortcuts/okr/okr_image_upload.goshortcuts/okr/okr_image_upload_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_delete.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_list.goshortcuts/okr/okr_progress_update.goshortcuts/whiteboard/whiteboard_errors.goshortcuts/whiteboard/whiteboard_errors_test.goshortcuts/whiteboard/whiteboard_query.goshortcuts/whiteboard/whiteboard_query_test.goshortcuts/whiteboard/whiteboard_update.goshortcuts/whiteboard/whiteboard_update_test.go
🚧 Files skipped from review as they are similar to previous changes (17)
- shortcuts/okr/okr_progress_delete.go
- shortcuts/okr/okr_errors.go
- shortcuts/okr/okr_cycle_detail_test.go
- shortcuts/okr/okr_image_upload_test.go
- .golangci.yml
- shortcuts/okr/okr_image_upload.go
- shortcuts/okr/okr_cycle_detail.go
- shortcuts/okr/okr_progress_create.go
- shortcuts/whiteboard/whiteboard_update_test.go
- shortcuts/okr/okr_progress_list.go
- shortcuts/whiteboard/whiteboard_query_test.go
- shortcuts/okr/okr_progress_get.go
- shortcuts/whiteboard/whiteboard_query.go
- shortcuts/whiteboard/whiteboard_update.go
- shortcuts/okr/okr_progress_update.go
- shortcuts/whiteboard/whiteboard_errors.go
- shortcuts/okr/okr_cycle_list.go
evandance
force-pushed
the
feat/errs-migrate-okr-whiteboard
branch
from
4b19089 to
687de0f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/whiteboard/whiteboard_query.go (1)
146-159: ⚡ Quick winMap HTTP 401/403 to stable typed subtypes for whiteboard preview errors (shortcuts/whiteboard/whiteboard_query.go:146-159)
This binary/non-JSON path currently mirrors
shortcuts/common/runner.go’shttpStatusError: only404becomeserrs.SubtypeNotFound, while other4xx(including401/403) are downgraded toerrs.SubtypeUnknown. To keep the preview-download error contract branchable, add explicit401/403mapping (e.g.,403→errs.SubtypePermissionDenied, and401→ an appropriate authentication/authorization typed error subtype instead ofSubtypeUnknown).🤖 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/whiteboard/whiteboard_query.go` around lines 146 - 159, The HTTP 4xx handling in the whiteboard preview path currently maps only 404 to errs.SubtypeNotFound and leaves 401/403 as errs.SubtypeUnknown; update the conditional that sets subtype (using resp.StatusCode) to explicitly map 403 to errs.SubtypePermissionDenied and 401 to an authentication-related subtype (e.g., errs.SubtypeUnauthenticated or errs.SubtypeUnauthorized depending on the available constant), then return errs.NewAPIError(subtype, ...) with that subtype so callers can branch on permission/auth errors (reference resp.StatusCode, errs.SubtypeNotFound, errs.SubtypePermissionDenied, errs.SubtypeUnauthenticated/errs.SubtypeUnauthorized in whiteboard_query.go).
🤖 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/whiteboard/whiteboard_query.go`:
- Around line 146-159: The HTTP 4xx handling in the whiteboard preview path
currently maps only 404 to errs.SubtypeNotFound and leaves 401/403 as
errs.SubtypeUnknown; update the conditional that sets subtype (using
resp.StatusCode) to explicitly map 403 to errs.SubtypePermissionDenied and 401
to an authentication-related subtype (e.g., errs.SubtypeUnauthenticated or
errs.SubtypeUnauthorized depending on the available constant), then return
errs.NewAPIError(subtype, ...) with that subtype so callers can branch on
permission/auth errors (reference resp.StatusCode, errs.SubtypeNotFound,
errs.SubtypePermissionDenied,
errs.SubtypeUnauthenticated/errs.SubtypeUnauthorized in whiteboard_query.go).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f925811-4e0d-44c0-922f-1402431c3e21
📒 Files selected for processing (21)
.golangci.ymlshortcuts/common/runner.goshortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_detail_test.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_errors.goshortcuts/okr/okr_errors_test.goshortcuts/okr/okr_image_upload.goshortcuts/okr/okr_image_upload_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_delete.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_get_test.goshortcuts/okr/okr_progress_list.goshortcuts/okr/okr_progress_update.goshortcuts/whiteboard/whiteboard_errors.goshortcuts/whiteboard/whiteboard_errors_test.goshortcuts/whiteboard/whiteboard_query.goshortcuts/whiteboard/whiteboard_query_test.goshortcuts/whiteboard/whiteboard_update.goshortcuts/whiteboard/whiteboard_update_test.go
💤 Files with no reviewable changes (1)
- shortcuts/common/runner.go
🚧 Files skipped from review as they are similar to previous changes (15)
- shortcuts/whiteboard/whiteboard_errors_test.go
- shortcuts/okr/okr_progress_list.go
- shortcuts/whiteboard/whiteboard_errors.go
- shortcuts/okr/okr_progress_update.go
- shortcuts/okr/okr_progress_get.go
- shortcuts/okr/okr_errors_test.go
- shortcuts/okr/okr_cycle_detail_test.go
- shortcuts/okr/okr_errors.go
- shortcuts/okr/okr_progress_delete.go
- .golangci.yml
- shortcuts/okr/okr_cycle_list.go
- shortcuts/whiteboard/whiteboard_update_test.go
- shortcuts/okr/okr_image_upload.go
- shortcuts/okr/okr_cycle_detail.go
- shortcuts/whiteboard/whiteboard_update.go
evandance
force-pushed
the
feat/errs-migrate-okr-whiteboard
branch
from
687de0f to
a7d8c95
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/okr/okr_cycle_list.go (1)
119-123: ⚡ Quick winKeep
page_sizetype consistent betweenDryRunandExecute.
DryRunuses an integer forpage_size(Line 94), butExecutesets it as a string. Aligning both avoids dry-run/request-shape drift.Proposed fix
queryParams := map[string]interface{}{ "user_id": userID, "user_id_type": userIDType, - "page_size": "100", + "page_size": 100, }🤖 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/okr/okr_cycle_list.go` around lines 119 - 123, The queryParams map in Execute sets "page_size" as a string causing a type mismatch with DryRun which uses an integer; update the "page_size" entry in the queryParams map inside Execute (the map assigned to variable queryParams in the Execute function) to use an integer (e.g., 100) instead of the string "100" so both DryRun and Execute use the same numeric type for page_size.
🤖 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/okr/okr_cycle_list.go`:
- Around line 119-123: The queryParams map in Execute sets "page_size" as a
string causing a type mismatch with DryRun which uses an integer; update the
"page_size" entry in the queryParams map inside Execute (the map assigned to
variable queryParams in the Execute function) to use an integer (e.g., 100)
instead of the string "100" so both DryRun and Execute use the same numeric type
for page_size.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15ced817-767e-46e9-9093-624cec033596
📒 Files selected for processing (21)
.golangci.ymlshortcuts/common/runner.goshortcuts/okr/okr_cycle_detail.goshortcuts/okr/okr_cycle_detail_test.goshortcuts/okr/okr_cycle_list.goshortcuts/okr/okr_errors.goshortcuts/okr/okr_errors_test.goshortcuts/okr/okr_image_upload.goshortcuts/okr/okr_image_upload_test.goshortcuts/okr/okr_progress_create.goshortcuts/okr/okr_progress_delete.goshortcuts/okr/okr_progress_get.goshortcuts/okr/okr_progress_get_test.goshortcuts/okr/okr_progress_list.goshortcuts/okr/okr_progress_update.goshortcuts/whiteboard/whiteboard_errors.goshortcuts/whiteboard/whiteboard_errors_test.goshortcuts/whiteboard/whiteboard_query.goshortcuts/whiteboard/whiteboard_query_test.goshortcuts/whiteboard/whiteboard_update.goshortcuts/whiteboard/whiteboard_update_test.go
💤 Files with no reviewable changes (6)
- shortcuts/whiteboard/whiteboard_errors.go
- shortcuts/whiteboard/whiteboard_errors_test.go
- shortcuts/whiteboard/whiteboard_update_test.go
- shortcuts/whiteboard/whiteboard_query_test.go
- shortcuts/whiteboard/whiteboard_query.go
- shortcuts/whiteboard/whiteboard_update.go
🚧 Files skipped from review as they are similar to previous changes (14)
- shortcuts/okr/okr_progress_get.go
- shortcuts/okr/okr_progress_get_test.go
- shortcuts/okr/okr_image_upload_test.go
- shortcuts/okr/okr_cycle_detail_test.go
- shortcuts/okr/okr_errors_test.go
- shortcuts/okr/okr_errors.go
- .golangci.yml
- shortcuts/common/runner.go
- shortcuts/okr/okr_image_upload.go
- shortcuts/okr/okr_cycle_detail.go
- shortcuts/okr/okr_progress_list.go
- shortcuts/okr/okr_progress_create.go
- shortcuts/okr/okr_progress_delete.go
- shortcuts/okr/okr_progress_update.go
evandance
force-pushed
the
feat/errs-migrate-okr-whiteboard
branch
4 times, most recently
from
cb640a2 to
2366d71
Compare
The okr and whiteboard commands now report every failure as a typed error envelope. Invalid flags, malformed input, output-file conflicts, and API or transport failures alike carry a stable category, subtype, the offending flag or Lark error code, and a meaningful exit code — so scripts and agents can branch on the error shape instead of scraping message strings.
evandance
force-pushed
the
feat/errs-migrate-okr-whiteboard
branch
from
2366d71 to
81e48c7
Compare
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
Completes the cli-errors-refactor migration for the okr and whiteboard commands: every failure now leaves the command as a typed
errs.*envelope instead of a flat legacy string. Each error carries a stable category, subtype, the offending--flagor upstream Lark code, and a meaningful exit code, so scripts and agents can branch on the error shape rather than scraping messages.Changes
output.ErrValidation/common.FlagErrorf) →errs.NewValidationError(SubtypeInvalidArgument).WithParam("--flag").DoAPIJSON/ rawDoAPI) →runtime.CallAPITyped/runtime.ClassifyAPIResponse, with query params unified tomap[string]interface{}. Already-typed transport/auth errors pass through viaerrs.ProblemOfand are never downgraded.okrInputStatError,wbSaveError,wrapOkrNetworkErr/wrapWbNetworkErr), replacing the sharedcommon.Wrap*helpers that emitted legacy shapes.errs-typed-only/errs-no-bare-wrap/errs-no-legacy-helperpath-except lists so legacy shapes cannot regress.common.WrapSaveErrorhelper.Behavior notes (exit-code reclassification)
internal(exit 5, previously api/1) — they are response-shape bugs, not API business errors.network(exit 4).Test Plan
gofmt -l,go vet,go build ./...: cleango test ./shortcuts/okr/... ./shortcuts/whiteboard/... ./shortcuts/common/...: passgolangci-lint run(full, all three legacy-ban guards active on both domains): 0 issuesgolangci-lint run --new-from-rev=origin/main: 0 issuesdeadcode -test ./...: no new dead codego run -C lint . ..): 0 violationserrors.As/errs.ProblemOf, subtype, offending param or Lark code) survives command dispatch.Related Issues
Part of the cli-errors-refactor domain migration (follows the drive domain, #1205). No tracking issue.