feat(slides): emit typed error envelopes across the slides domain#1349
Merged
Conversation
evandance
added
size/M
|
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 ChangesSlides shortcuts errs migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
shortcuts/slides/slides_replace_slide.go (6)
60-60: ⚡ Quick winConsider adding
.WithParam("--slide-id")for consistency.Other validation errors in the migration attach the related flag via
.WithParam()(e.g., slides_create.go:48, slides_media_upload.go:92). This helps structured error consumers identify which parameter failed.Suggested enhancement
-return errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide-id cannot be empty") +return errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide-id cannot be empty").WithParam("--slide-id")🤖 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/slides/slides_replace_slide.go` at line 60, The validation error returned uses errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide-id cannot be empty") but is missing the structured parameter name; update the return to chain .WithParam("--slide-id") on the error (i.e., locate the call to errs.NewValidationError in slides_replace_slide.go and append .WithParam("--slide-id") to the returned error) so it matches other validations like slides_create.go and slides_media_upload.go.
207-207: ⚡ Quick winConsider adding
.WithParam("--parts")for consistency.This validation error should attach the parameter like other similar errors in the migration.
Suggested enhancement
-return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts invalid JSON, must be an array of objects: %v", err) +return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts invalid JSON, must be an array of objects: %v", err).WithParam("--parts")🤖 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/slides/slides_replace_slide.go` at line 207, The validation error created by errs.NewValidationError in slides_replace_slide.go should attach the parameter for consistency; update the return that currently calls errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts invalid JSON, must be an array of objects: %v", err) to chain .WithParam("--parts") on the error before returning so the parameter name is included (locate the NewValidationError call in the ReplaceSlide/slide replacement handling and add .WithParam("--parts")).
203-203: ⚡ Quick winConsider adding
.WithParam("--parts")for consistency.Similar to the
--slide-idvalidation and other migrations in this PR, attaching the parameter helps structured error consumers.Suggested enhancement
-return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts cannot be empty") +return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts cannot be empty").WithParam("--parts")🤖 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/slides/slides_replace_slide.go` at line 203, The validation error return in slides_replace_slide.go uses errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts cannot be empty") but omits the parameter metadata; update the return to attach the parameter by chaining .WithParam("--parts") on the error (i.e., call errs.NewValidationError(...).WithParam("--parts")) so it matches the other validations like the --slide-id case and provides structured param info for consumers.
215-243: ⚡ Quick winConsider adding
.WithParam("--parts")to all field type validation errors.These five type-assertion errors (action, replacement, block_id, insertion, insert_before_block_id) should attach the parameter for consistency with other validation errors in this migration.
Suggested enhancement
if v, ok := m["action"]; ok { s, ok := v.(string) if !ok { - return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].action must be a string", i) + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].action must be a string", i).WithParam("--parts") } p.Action = s }Apply the same pattern to the remaining four field validations (replacement, block_id, insertion, insert_before_block_id).
🤖 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/slides/slides_replace_slide.go` around lines 215 - 243, The validation error calls in slides_replace_slide.go for the parts parsing (fields action, replacement, block_id, insertion, insert_before_block_id) should include the parameter context; update each errs.NewValidationError invocation in the parts-parsing logic (the block that assigns to p.Action, p.Replacement, p.BlockID, p.Insertion, p.InsertBeforeBlockID) to chain .WithParam("--parts") so every type-assertion error attaches the "--parts" parameter consistently.
330-330: ⚡ Quick winConsider adding
.WithParam("--parts")for consistency.This error wraps a replacement XML parsing failure and should attach the parameter like other --parts validations.
Suggested enhancement
-return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].replacement: %v", i, err).WithCause(err) +return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].replacement: %v", i, err).WithCause(err).WithParam("--parts")🤖 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/slides/slides_replace_slide.go` at line 330, The validation error returned from errs.NewValidationError for the replacement XML parse should include the parameter name for consistency; modify the error construction at the return (the call that currently does errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].replacement: %v", i, err).WithCause(err)) to also call .WithParam("--parts") (e.g., chain .WithParam("--parts") before/after .WithCause(err)) so the error includes the parameter metadata while preserving the original cause.
283-309: ⚡ Quick winConsider adding
.WithParam("--parts")to all validation errors in this function.For consistency with the migration pattern, all eight validation errors in
validateReplacePartsshould attach the parameter.Suggested enhancement
Add
.WithParam("--parts")to each of the eighterrs.NewValidationErrorcalls in this function (lines 283, 286, 292, 295, 299, 305, 307, 309).Example for line 283:
-return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts must contain at least 1 item") +return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts must contain at least 1 item").WithParam("--parts")🤖 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/slides/slides_replace_slide.go` around lines 283 - 309, In validateReplaceParts, update every errs.NewValidationError return to chain .WithParam("--parts") so all eight validation errors include the parameter context; locate the NewValidationError calls inside validateReplaceParts (the ones checking len(parts), maxReplaceParts, and each switch case for p.Action including block_replace, block_insert, str_replace, empty action, and default) and append .WithParam("--parts") to each returned error.shortcuts/slides/helpers.go (1)
105-105: ⚡ Quick winConsider using an internal error for this defensive programming case.
The
ref.Kindfield is set only byparsePresentationRef(lines 44, 47, 57), which returns exclusively"slides"or"wiki". Thisdefaultcase should never execute during normal operation—it's a defensive guard against programming errors (e.g., a future code change that adds a new kind without updating this function).A validation error implies user input is wrong, but the user never directly sets
ref.Kind. An internal error would more accurately reflect that this represents a programming bug rather than invalid user input.Suggested improvement
- return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "unsupported presentation ref kind %q", ref.Kind) + return "", errs.NewInternalError(errs.SubtypeSDKError, "unsupported presentation ref kind %q (programming error)", ref.Kind)🤖 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/slides/helpers.go` at line 105, The default branch returning a validation error for unsupported presentation ref kind should be an internal/programming error because ref.Kind is set only by parsePresentationRef (which yields "slides" or "wiki"); change the error created in that return (the one referencing ref.Kind) from errs.NewValidationError(errs.SubtypeInvalidArgument, ...) to the appropriate internal error constructor (e.g., errs.NewInternalError or the codebase's internal error helper) so this impossible/default case is reported as an internal/programming error rather than user input validation failure.
🤖 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/slides/helpers.go`:
- Line 105: The default branch returning a validation error for unsupported
presentation ref kind should be an internal/programming error because ref.Kind
is set only by parsePresentationRef (which yields "slides" or "wiki"); change
the error created in that return (the one referencing ref.Kind) from
errs.NewValidationError(errs.SubtypeInvalidArgument, ...) to the appropriate
internal error constructor (e.g., errs.NewInternalError or the codebase's
internal error helper) so this impossible/default case is reported as an
internal/programming error rather than user input validation failure.
In `@shortcuts/slides/slides_replace_slide.go`:
- Line 60: The validation error returned uses
errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide-id cannot be
empty") but is missing the structured parameter name; update the return to chain
.WithParam("--slide-id") on the error (i.e., locate the call to
errs.NewValidationError in slides_replace_slide.go and append
.WithParam("--slide-id") to the returned error) so it matches other validations
like slides_create.go and slides_media_upload.go.
- Line 207: The validation error created by errs.NewValidationError in
slides_replace_slide.go should attach the parameter for consistency; update the
return that currently calls errs.NewValidationError(errs.SubtypeInvalidArgument,
"--parts invalid JSON, must be an array of objects: %v", err) to chain
.WithParam("--parts") on the error before returning so the parameter name is
included (locate the NewValidationError call in the ReplaceSlide/slide
replacement handling and add .WithParam("--parts")).
- Line 203: The validation error return in slides_replace_slide.go uses
errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts cannot be empty")
but omits the parameter metadata; update the return to attach the parameter by
chaining .WithParam("--parts") on the error (i.e., call
errs.NewValidationError(...).WithParam("--parts")) so it matches the other
validations like the --slide-id case and provides structured param info for
consumers.
- Around line 215-243: The validation error calls in slides_replace_slide.go for
the parts parsing (fields action, replacement, block_id, insertion,
insert_before_block_id) should include the parameter context; update each
errs.NewValidationError invocation in the parts-parsing logic (the block that
assigns to p.Action, p.Replacement, p.BlockID, p.Insertion,
p.InsertBeforeBlockID) to chain .WithParam("--parts") so every type-assertion
error attaches the "--parts" parameter consistently.
- Line 330: The validation error returned from errs.NewValidationError for the
replacement XML parse should include the parameter name for consistency; modify
the error construction at the return (the call that currently does
errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].replacement:
%v", i, err).WithCause(err)) to also call .WithParam("--parts") (e.g., chain
.WithParam("--parts") before/after .WithCause(err)) so the error includes the
parameter metadata while preserving the original cause.
- Around line 283-309: In validateReplaceParts, update every
errs.NewValidationError return to chain .WithParam("--parts") so all eight
validation errors include the parameter context; locate the NewValidationError
calls inside validateReplaceParts (the ones checking len(parts),
maxReplaceParts, and each switch case for p.Action including block_replace,
block_insert, str_replace, empty action, and default) and append
.WithParam("--parts") to each returned error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3143418a-7fbf-4e56-9e2c-497dd7b0b082
📒 Files selected for processing (11)
.golangci.ymllint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rules_test.goshortcuts/slides/helpers.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goshortcuts/slides/slides_errors.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1349 +/- ##
==========================================
+ Coverage 71.96% 71.97% +0.01%
==========================================
Files 695 696 +1
Lines 65879 65891 +12
==========================================
+ Hits 47408 47424 +16
+ Misses 14814 14811 -3
+ Partials 3657 3656 -1 ☔ 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@ae8aa84fa18e188a1d703b8b4ba18cbec50cb632🧩 Skill updatenpx skills add larksuite/cli#feat/errs-migrate-slides -y -g |
evandance
force-pushed
the
feat/errs-migrate-slides
branch
from
3e0b96c to
5c16948
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shortcuts/slides/slides_replace_slide_test.go (1)
724-730: 💤 Low valueConsider adding an explicit nil check for consistency.
The test correctly uses
errors.Asto unwrap the*errs.ValidationErrorand check itsParamfield. However, for consistency with the established pattern in this file (lines 284-286, 343-345, 631-633, 670-672), consider adding an explicit nil check before theerrors.Ascall:if err == nil { t.Fatalf("expected validation error for %s", tt.name) }This would make the failure message more descriptive when the test accidentally passes without producing an error.
🤖 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/slides/slides_replace_slide_test.go` around lines 724 - 730, Add an explicit nil check before unwrapping the error: ensure that when testing the ReplaceSlide error path you verify err != nil (e.g., check err == nil and call t.Fatalf with a message referencing tt.name) before calling errors.As; update the block that declares var ve *errs.ValidationError and uses errors.As(err, &ve) to first fail if err is nil, then proceed to errors.As and the subsequent Param assertion (referencing variables err, ve, tt.wantParam, tt.name).
🤖 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 `@lint/errscontract/rule_no_legacy_common_helper_call.go`:
- Around line 18-20: The entries "cmd/event/", "events/",
"internal/event/consume/", and "shortcuts/event/" were added to the scope in
lint/errscontract/rule_no_legacy_common_helper_call.go but there are no actual
code changes under those paths; update the PR to either (A) clarify in the PR
description that this change is intentional and is a linter-scope/config-only
update (explain why the rule should now cover event paths), or (B) remove those
added prefixes from rule_no_legacy_common_helper_call.go if they were added
unintentionally, ensuring the file's scope matches the intended affected code.
In `@shortcuts/slides/slides_media_upload_test.go`:
- Around line 220-227: The test currently asserts the error is a
*errs.ValidationError and that ve.Param == "--file" but must also assert the
subtype and that the underlying cause was preserved; update the assertion block
(where ve is checked) to verify ve.Subtype == errs.SubtypeInvalidArgument and
that ve.Cause (or the wrapped cause accessible on errs.ValidationError) matches
the original stat error returned by slidesInputStatError (i.e., use errors.Is or
compare the cause returned from slidesInputStatError().WithCause(err) to ensure
WithCause(err) was preserved).
---
Nitpick comments:
In `@shortcuts/slides/slides_replace_slide_test.go`:
- Around line 724-730: Add an explicit nil check before unwrapping the error:
ensure that when testing the ReplaceSlide error path you verify err != nil
(e.g., check err == nil and call t.Fatalf with a message referencing tt.name)
before calling errors.As; update the block that declares var ve
*errs.ValidationError and uses errors.As(err, &ve) to first fail if err is nil,
then proceed to errors.As and the subsequent Param assertion (referencing
variables err, ve, tt.wantParam, tt.name).
🪄 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: 940aa31e-f633-4f5e-bca6-28b8ce972f73
📒 Files selected for processing (13)
.golangci.ymllint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rules_test.goshortcuts/slides/helpers.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goshortcuts/slides/slides_errors.goshortcuts/slides/slides_errors_test.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_media_upload_test.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_test.go
✅ Files skipped from review due to trivial changes (1)
- .golangci.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- lint/errscontract/rules_test.go
- shortcuts/slides/slides_errors.go
- shortcuts/slides/slides_media_upload.go
- shortcuts/slides/slides_create.go
- shortcuts/slides/helpers.go
- shortcuts/slides/slides_replace_slide.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: 2
🧹 Nitpick comments (1)
shortcuts/slides/slides_replace_slide_test.go (1)
724-730: 💤 Low valueConsider adding an explicit nil check for consistency.
The test correctly uses
errors.Asto unwrap the*errs.ValidationErrorand check itsParamfield. However, for consistency with the established pattern in this file (lines 284-286, 343-345, 631-633, 670-672), consider adding an explicit nil check before theerrors.Ascall:if err == nil { t.Fatalf("expected validation error for %s", tt.name) }This would make the failure message more descriptive when the test accidentally passes without producing an error.
🤖 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/slides/slides_replace_slide_test.go` around lines 724 - 730, Add an explicit nil check before unwrapping the error: ensure that when testing the ReplaceSlide error path you verify err != nil (e.g., check err == nil and call t.Fatalf with a message referencing tt.name) before calling errors.As; update the block that declares var ve *errs.ValidationError and uses errors.As(err, &ve) to first fail if err is nil, then proceed to errors.As and the subsequent Param assertion (referencing variables err, ve, tt.wantParam, tt.name).
🤖 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 `@lint/errscontract/rule_no_legacy_common_helper_call.go`:
- Around line 18-20: The entries "cmd/event/", "events/",
"internal/event/consume/", and "shortcuts/event/" were added to the scope in
lint/errscontract/rule_no_legacy_common_helper_call.go but there are no actual
code changes under those paths; update the PR to either (A) clarify in the PR
description that this change is intentional and is a linter-scope/config-only
update (explain why the rule should now cover event paths), or (B) remove those
added prefixes from rule_no_legacy_common_helper_call.go if they were added
unintentionally, ensuring the file's scope matches the intended affected code.
In `@shortcuts/slides/slides_media_upload_test.go`:
- Around line 220-227: The test currently asserts the error is a
*errs.ValidationError and that ve.Param == "--file" but must also assert the
subtype and that the underlying cause was preserved; update the assertion block
(where ve is checked) to verify ve.Subtype == errs.SubtypeInvalidArgument and
that ve.Cause (or the wrapped cause accessible on errs.ValidationError) matches
the original stat error returned by slidesInputStatError (i.e., use errors.Is or
compare the cause returned from slidesInputStatError().WithCause(err) to ensure
WithCause(err) was preserved).
---
Nitpick comments:
In `@shortcuts/slides/slides_replace_slide_test.go`:
- Around line 724-730: Add an explicit nil check before unwrapping the error:
ensure that when testing the ReplaceSlide error path you verify err != nil
(e.g., check err == nil and call t.Fatalf with a message referencing tt.name)
before calling errors.As; update the block that declares var ve
*errs.ValidationError and uses errors.As(err, &ve) to first fail if err is nil,
then proceed to errors.As and the subsequent Param assertion (referencing
variables err, ve, tt.wantParam, tt.name).
🪄 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: 940aa31e-f633-4f5e-bca6-28b8ce972f73
📒 Files selected for processing (13)
.golangci.ymllint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rules_test.goshortcuts/slides/helpers.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goshortcuts/slides/slides_errors.goshortcuts/slides/slides_errors_test.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_media_upload_test.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_test.go
✅ Files skipped from review due to trivial changes (1)
- .golangci.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- lint/errscontract/rules_test.go
- shortcuts/slides/slides_errors.go
- shortcuts/slides/slides_media_upload.go
- shortcuts/slides/slides_create.go
- shortcuts/slides/helpers.go
- shortcuts/slides/slides_replace_slide.go
🛑 Comments failed to post (2)
lint/errscontract/rule_no_legacy_common_helper_call.go (1)
18-20:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check if this PR includes event-domain changes beyond linter config # Check for modified files under event-related paths git diff --name-only origin/main | grep -E '^(cmd/event/|events/|internal/event/consume/|shortcuts/event/)' || echo "No event-domain file changes found"Repository: larksuite/cli
Length of output: 92
Adjust scope rationale for added event path prefixes in lint rule (non-code change only)
git diffshows no file changes undercmd/event/,events/,internal/event/consume/, orshortcuts/event/; the newcmd/event/,events/,internal/event/consume/,shortcuts/event/entries inlint/errscontract/rule_no_legacy_common_helper_call.go(lines 18-20) appear to be linter-scope configuration only. If the PR is intended to be slides-only, clarify in the PR description why this lint rule should cover event paths (or remove the prefixes if unintended).🤖 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 `@lint/errscontract/rule_no_legacy_common_helper_call.go` around lines 18 - 20, The entries "cmd/event/", "events/", "internal/event/consume/", and "shortcuts/event/" were added to the scope in lint/errscontract/rule_no_legacy_common_helper_call.go but there are no actual code changes under those paths; update the PR to either (A) clarify in the PR description that this change is intentional and is a linter-scope/config-only update (explain why the rule should now cover event paths), or (B) remove those added prefixes from rule_no_legacy_common_helper_call.go if they were added unintentionally, ensuring the file's scope matches the intended affected code.shortcuts/slides/slides_media_upload_test.go (1)
220-227: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Complete the typed error assertion per guidelines.
The typed error check correctly verifies
Param == "--file", but per coding guidelines, error-path tests must also assert subtype and cause preservation. The upstreamslidesInputStatErrorhelper returnserrs.SubtypeInvalidArgumentand wraps the underlying error via.WithCause(err).✅ Proposed additions to complete the migration
Add subtype and cause-preservation checks:
var ve *errs.ValidationError if !errors.As(err, &ve) { t.Fatalf("err = %v, want *errs.ValidationError", err) } if ve.Param != "--file" { t.Fatalf("Param = %q, want --file", ve.Param) } + if ve.Subtype != errs.SubtypeInvalidArgument { + t.Fatalf("Subtype = %v, want SubtypeInvalidArgument", ve.Subtype) + } + if !errors.Is(err, os.ErrNotExist) && !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("error should wrap fs.ErrNotExist for cause preservation, got: %v", err) + }📝 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.var ve *errs.ValidationError if !errors.As(err, &ve) { t.Fatalf("err = %v, want *errs.ValidationError", err) } if ve.Param != "--file" { t.Fatalf("Param = %q, want --file", ve.Param) } if ve.Subtype != errs.SubtypeInvalidArgument { t.Fatalf("Subtype = %v, want SubtypeInvalidArgument", ve.Subtype) } if !errors.Is(err, os.ErrNotExist) && !errors.Is(err, fs.ErrNotExist) { t.Fatalf("error should wrap fs.ErrNotExist for cause preservation, got: %v", 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/slides/slides_media_upload_test.go` around lines 220 - 227, The test currently asserts the error is a *errs.ValidationError and that ve.Param == "--file" but must also assert the subtype and that the underlying cause was preserved; update the assertion block (where ve is checked) to verify ve.Subtype == errs.SubtypeInvalidArgument and that ve.Cause (or the wrapped cause accessible on errs.ValidationError) matches the original stat error returned by slidesInputStatError (i.e., use errors.Is or compare the cause returned from slidesInputStatError().WithCause(err) to ensure WithCause(err) was preserved).Source: Coding guidelines
evandance
force-pushed
the
feat/errs-migrate-slides
branch
2 times, most recently
from
e3aae12 to
4ae2809
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/slides/slides_replace_slide.go`:
- Around line 207-208: The validation error currently formats the JSON decode
error into the message and loses the typed cause; update the return so it does
not embed "%v" with err but instead preserves the original error by calling
.WithCause(err) on the errs.NewValidationError result (keep the human message
like "--parts invalid JSON, must be an array of objects" and retain the existing
.WithParam("--parts")). Specifically modify the expression using
errs.NewValidationError(...) and .WithParam to also chain .WithCause(err) so
errors.Is/unwrapping still work.
- Around line 329-331: The current code always wraps errors from ensureXMLRootID
in a new errs.NewValidationError, which can strip already-typed errs.* metadata;
change the error handling in the replacement parsing loop so that if err is
already a typed errs error you return it unchanged (e.g. type-assert or use the
package's exported typed error interface/marker like errs.Error /
errs.TypedError), otherwise wrap non-typed errors with errs.NewValidationError
as before; check and update the branch around ensureXMLRootID (the block that
currently calls errs.NewValidationError("--parts[%d].replacement: %v", i, err))
to first detect typed errs and return err directly, only constructing a new
validation error for non-errs errors.
🪄 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: 7bc8f222-df65-44b4-b515-567ac2a36290
📒 Files selected for processing (13)
.golangci.ymllint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rules_test.goshortcuts/slides/helpers.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goshortcuts/slides/slides_errors.goshortcuts/slides/slides_errors_test.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_media_upload_test.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_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 (11)
- lint/errscontract/rule_no_legacy_envelope_literal.go
- lint/errscontract/rules_test.go
- .golangci.yml
- shortcuts/slides/slides_media_upload.go
- shortcuts/slides/slides_media_upload_test.go
- shortcuts/slides/slides_errors_test.go
- shortcuts/slides/slides_errors.go
- shortcuts/slides/helpers.go
- shortcuts/slides/slides_create.go
- shortcuts/slides/slides_create_test.go
- shortcuts/slides/slides_replace_slide_test.go
evandance
force-pushed
the
feat/errs-migrate-slides
branch
from
4ae2809 to
dd0221c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/slides/slides_errors.go`:
- Line 49: Replace the misclassified subtype in the errs.NewInternalError call:
change the second argument from errs.SubtypeSDKError to errs.SubtypeUnknown so
the fallback unclassified errors are correctly labeled, and ensure the returned
error still chains the original error via .WithCause(err) (you may retain
.WithHint(hint) if the hint is applicable); specifically update the
errs.NewInternalError(...) invocation that currently references
errs.SubtypeSDKError to use errs.SubtypeUnknown and keep .WithCause(err).
In `@shortcuts/slides/slides_replace_slide_test.go`:
- Around line 724-730: Replace the concrete-type cast using errors.As with the
typed-envelope helper errs.ProblemOf: call p := errs.ProblemOf(err), assert p !=
nil and p.Subtype matches the expected validation subtype, then assert p.Param
equals tt.wantParam (instead of casting to *errs.ValidationError and checking
ve.Param). Use the existing tt.wantParam and the error variable err to drive
these assertions and remove the errors.As path.
🪄 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: 59a96c38-2733-475a-9b2f-310729becd05
📒 Files selected for processing (13)
.golangci.ymllint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rules_test.goshortcuts/slides/helpers.goshortcuts/slides/slides_create.goshortcuts/slides/slides_create_test.goshortcuts/slides/slides_errors.goshortcuts/slides/slides_errors_test.goshortcuts/slides/slides_media_upload.goshortcuts/slides/slides_media_upload_test.goshortcuts/slides/slides_replace_slide.goshortcuts/slides/slides_replace_slide_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 (10)
- lint/errscontract/rule_no_legacy_envelope_literal.go
- shortcuts/slides/slides_media_upload_test.go
- .golangci.yml
- shortcuts/slides/slides_media_upload.go
- lint/errscontract/rules_test.go
- shortcuts/slides/helpers.go
- shortcuts/slides/slides_errors_test.go
- shortcuts/slides/slides_replace_slide.go
- shortcuts/slides/slides_create.go
- shortcuts/slides/slides_create_test.go
evandance
force-pushed
the
feat/errs-migrate-slides
branch
from
dd0221c to
bc3e83b
Compare
ViperCai
previously approved these changes
Jun 10, 2026
Emit structured validation, API, network, file, and internal error envelopes for Slides shortcuts so users and agents can recover from failed presentation workflows using stable type, subtype, param, and code fields. Add Slides domain errscontract and golangci guards to prevent legacy envelope and common helper regressions.
evandance
force-pushed
the
feat/errs-migrate-slides
branch
from
bc3e83b to
ae8aa84
Compare
ViperCai
approved these changes
Jun 10, 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
Migrates typed error envelopes across the Slides shortcut domain. Scope is limited to
shortcuts/slides, Slides error-contract guards, and Slides guard tests.Changes
errs.*envelopes.--presentation,--slides,--file,--slide-id, and--partsvalidation..golangci.ymlandlint/errscontractso legacy envelopes and common helpers cannot regress.Test Plan
gofmt -l <changed go files>produced no outputgo vet ./shortcuts/slides/...go test ./shortcuts/slides/...go test -C lint ./...go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/main ./shortcuts/slides/...go run -C lint . ..go build ./...Related Issues
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores