feat(markdown): emit typed error envelopes across the markdown domain#1347
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:
📝 WalkthroughWalkthroughStandardize error handling in shortcuts/markdown to use markdown-scoped errs types, add markdown error constructors and a download wrapper, update validations/read/save flows and tests, and include the markdown path in lint rules. ChangesMarkdown Error Handling Standardization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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@c09814eb8f1019ba313da53276194a2807d782ca🧩 Skill updatenpx skills add larksuite/cli#feat/errs-migrate-markdown -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1347 +/- ##
==========================================
+ Coverage 71.97% 71.99% +0.02%
==========================================
Files 696 697 +1
Lines 65891 65899 +8
==========================================
+ Hits 47424 47447 +23
+ Misses 14811 14798 -13
+ Partials 3656 3654 -2 ☔ 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.
🧹 Nitpick comments (1)
lint/errscontract/rule_no_legacy_envelope_literal.go (1)
24-24: ⚡ Quick winAdd a markdown-path regression test for this rule.
migratedEnvelopePathsnow includesshortcuts/markdown/, but there’s no explicitCheckNoLegacyEnvelopeLiteraltest asserting markdown is rejected. Adding one keeps this path-list contract from silently regressing.🤖 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_envelope_literal.go` at line 24, Add a regression test that ensures the new migratedEnvelopePaths entry "shortcuts/markdown/" is covered: update or add a test for CheckNoLegacyEnvelopeLiteral that constructs a sample markdown file path under "shortcuts/markdown/" and asserts the rule rejects it (i.e., returns a violation/error). Locate the test surrounding CheckNoLegacyEnvelopeLiteral and migratedEnvelopePaths, add a case with a markdown file path (e.g., ending in .md or appropriate test fixture) and assert the rule flags it so this path-list contract cannot silently regress.
🤖 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 `@lint/errscontract/rule_no_legacy_envelope_literal.go`:
- Line 24: Add a regression test that ensures the new migratedEnvelopePaths
entry "shortcuts/markdown/" is covered: update or add a test for
CheckNoLegacyEnvelopeLiteral that constructs a sample markdown file path under
"shortcuts/markdown/" and asserts the rule rejects it (i.e., returns a
violation/error). Locate the test surrounding CheckNoLegacyEnvelopeLiteral and
migratedEnvelopePaths, add a case with a markdown file path (e.g., ending in .md
or appropriate test fixture) and assert the rule flags it so this path-list
contract cannot silently regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aac81858-9660-4f6b-8245-a5ad4595303b
📒 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/markdown/helpers.goshortcuts/markdown/markdown_diff.goshortcuts/markdown/markdown_diff_test.goshortcuts/markdown/markdown_errors.goshortcuts/markdown/markdown_fetch.goshortcuts/markdown/markdown_overwrite.goshortcuts/markdown/markdown_patch.go
evandance
force-pushed
the
feat/errs-migrate-markdown
branch
2 times, most recently
from
31e4096 to
7938980
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/markdown/helpers.go (1)
410-416:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
--fileparam metadata consistent across upload modes.Line 415 correctly tags local-open failures with
--file, but the multipart counterpart still returns an untagged typed validation error. This makes large-file upload failures lose flag attribution compared to single-part uploads.Suggested fix
@@ fileReader, err := openReader() if err != nil { - return markdownUploadResult{}, common.WrapInputStatErrorTyped(err) + return markdownUploadResult{}, withMarkdownFileParam(common.WrapInputStatErrorTyped(err), "--file") }As per coding guidelines, user flag/arg validation failures should carry
WithParam("--flag"), so both file-open paths should retain--fileconsistently.🤖 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/markdown/helpers.go` around lines 410 - 416, The multipart upload path currently returns an untagged typed validation error on local file open, causing inconsistent flag attribution with single-part uploads; find the multipart upload function (e.g., uploadMarkdownFileMultipart or the multipart branch in uploadMarkdownFileAll) and change the openReader error return to wrap the error using withMarkdownFileParam(common.WrapInputStatErrorTyped(err), "--file") so both upload modes consistently attach the "--file" param metadata.Source: Coding guidelines
🤖 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/markdown/markdown_test.go`:
- Around line 861-872: Update requireMarkdownValidationParam to use
errs.ProblemOf instead of errors.As so tests assert the typed problem metadata
and cause preservation: call errs.ProblemOf(err) to obtain the problem and
underlying cause, verify the problem is non-nil, check problem.Category ==
"validation" (or the expected category), problem.Subtype matches the validation
subtype if applicable, and problem.Param == want, and also assert that ProblemOf
returns a non-nil cause when present to ensure cause preservation; keep the
helper signature requireMarkdownValidationParam(t *testing.T, err error, want
string) and fail via t.Fatalf with clear messages when any of these checks fail.
---
Outside diff comments:
In `@shortcuts/markdown/helpers.go`:
- Around line 410-416: The multipart upload path currently returns an untagged
typed validation error on local file open, causing inconsistent flag attribution
with single-part uploads; find the multipart upload function (e.g.,
uploadMarkdownFileMultipart or the multipart branch in uploadMarkdownFileAll)
and change the openReader error return to wrap the error using
withMarkdownFileParam(common.WrapInputStatErrorTyped(err), "--file") so both
upload modes consistently attach the "--file" param metadata.
🪄 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: d5761bb4-99cf-4c8a-9b49-cb796d2c0c0a
📒 Files selected for processing (12)
.golangci.ymllint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rules_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff.goshortcuts/markdown/markdown_diff_test.goshortcuts/markdown/markdown_errors.goshortcuts/markdown/markdown_fetch.goshortcuts/markdown/markdown_overwrite.goshortcuts/markdown/markdown_patch.goshortcuts/markdown/markdown_test.go
✅ Files skipped from review due to trivial changes (1)
- .golangci.yml
🚧 Files skipped from review as they are similar to previous changes (8)
- lint/errscontract/rule_no_legacy_envelope_literal.go
- lint/errscontract/rules_test.go
- shortcuts/markdown/markdown_errors.go
- shortcuts/markdown/markdown_diff_test.go
- shortcuts/markdown/markdown_patch.go
- shortcuts/markdown/markdown_overwrite.go
- lint/errscontract/rule_no_legacy_common_helper_call.go
- shortcuts/markdown/markdown_fetch.go
evandance
force-pushed
the
feat/errs-migrate-markdown
branch
from
7938980 to
2359bfd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/markdown/markdown_test.go (1)
861-881:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert cause preservation in file-read/open error-path tests.
requireMarkdownValidationParamonly validates category/subtype/param, so Line 893, Line 1339, and Line 1766 can pass even if.WithCause(err)gets dropped in the production path. The *_test guideline requires cause preservation checks for error paths.Suggested focused fix
-func requireMarkdownValidationParam(t *testing.T, err error, want string) { +func requireMarkdownValidationParam(t *testing.T, err error, want string, requireCause bool) { t.Helper() p, ok := errs.ProblemOf(err) if !ok { t.Fatalf("expected typed problem, got %T (%v)", err, err) } if p.Category != errs.CategoryValidation || p.Subtype != errs.SubtypeInvalidArgument { t.Fatalf("classification = %s/%s, want %s/%s", p.Category, p.Subtype, errs.CategoryValidation, errs.SubtypeInvalidArgument) } var ve *errs.ValidationError if !errors.As(err, &ve) { t.Fatalf("expected *errs.ValidationError, got %T (%v)", err, err) } if ve.Param != want { t.Fatalf("validation param = %q, want %q", ve.Param, want) } + if requireCause && ve.Cause == nil { + t.Fatalf("expected validation cause to be preserved") + } } @@ - requireMarkdownValidationParam(t, err, "--file") + requireMarkdownValidationParam(t, err, "--file", true) @@ - requireMarkdownValidationParam(t, err, "--file") + requireMarkdownValidationParam(t, err, "--file", true) @@ - requireMarkdownValidationParam(t, err, "--file") + requireMarkdownValidationParam(t, err, "--file", true)As per coding guidelines: "
**/*_test.go: Error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone."Also applies to: 893-894, 1312-1340, 1755-1767
🤖 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/markdown/markdown_test.go` around lines 861 - 881, The helper requireMarkdownValidationParam currently checks only ProblemOf and ValidationError.Param; update it to also assert that the original cause is preserved: after extracting ve via errors.As, ensure ve.Err (or the wrapped cause) is non-nil and that errors.Unwrap(err) (or errors.Is(err, ve.Err)) confirms the cause is present; use the existing symbols errs.ProblemOf, errs.ValidationError, errors.As, errors.Unwrap/errors.Is in the check so tests at the error-path sites will fail if .WithCause(err) is dropped.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@shortcuts/markdown/markdown_test.go`:
- Around line 861-881: The helper requireMarkdownValidationParam currently
checks only ProblemOf and ValidationError.Param; update it to also assert that
the original cause is preserved: after extracting ve via errors.As, ensure
ve.Err (or the wrapped cause) is non-nil and that errors.Unwrap(err) (or
errors.Is(err, ve.Err)) confirms the cause is present; use the existing symbols
errs.ProblemOf, errs.ValidationError, errors.As, errors.Unwrap/errors.Is in the
check so tests at the error-path sites will fail if .WithCause(err) is dropped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a0f26ab-cee4-45ed-bda3-c8f28cb4b27e
📒 Files selected for processing (12)
.golangci.ymllint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rules_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff.goshortcuts/markdown/markdown_diff_test.goshortcuts/markdown/markdown_errors.goshortcuts/markdown/markdown_fetch.goshortcuts/markdown/markdown_overwrite.goshortcuts/markdown/markdown_patch.goshortcuts/markdown/markdown_test.go
✅ Files skipped from review due to trivial changes (2)
- shortcuts/markdown/markdown_overwrite.go
- .golangci.yml
🚧 Files skipped from review as they are similar to previous changes (7)
- lint/errscontract/rule_no_legacy_envelope_literal.go
- lint/errscontract/rule_no_legacy_common_helper_call.go
- shortcuts/markdown/markdown_patch.go
- shortcuts/markdown/markdown_errors.go
- shortcuts/markdown/markdown_diff_test.go
- shortcuts/markdown/markdown_fetch.go
- shortcuts/markdown/markdown_diff.go
evandance
force-pushed
the
feat/errs-migrate-markdown
branch
from
2359bfd to
a4ef7cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/markdown/markdown_test.go`:
- Around line 861-881: requireMarkdownValidationParam currently only checks the
typed validation metadata but not that the original cause was preserved; change
its signature to requireMarkdownValidationParam(t *testing.T, err error, want
string, wantCause error) and inside after the existing checks assert that
errors.Is(err, wantCause) (or that errors.Unwrap chain contains wantCause) to
enforce the .WithCause(err) contract, then update the three file-open/read
failure tests that call requireMarkdownValidationParam to pass the expected
cause value.
🪄 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: a888da93-07f3-4875-a987-56f6922d900f
📒 Files selected for processing (12)
.golangci.ymllint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rules_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff.goshortcuts/markdown/markdown_diff_test.goshortcuts/markdown/markdown_errors.goshortcuts/markdown/markdown_fetch.goshortcuts/markdown/markdown_overwrite.goshortcuts/markdown/markdown_patch.goshortcuts/markdown/markdown_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- .golangci.yml
- shortcuts/markdown/markdown_overwrite.go
- shortcuts/markdown/markdown_errors.go
- lint/errscontract/rule_no_legacy_envelope_literal.go
- lint/errscontract/rule_no_legacy_common_helper_call.go
- shortcuts/markdown/markdown_diff_test.go
- lint/errscontract/rules_test.go
- shortcuts/markdown/markdown_fetch.go
- shortcuts/markdown/markdown_patch.go
- shortcuts/markdown/helpers.go
- shortcuts/markdown/markdown_diff.go
evandance
force-pushed
the
feat/errs-migrate-markdown
branch
3 times, most recently
from
cab8a51 to
04472c9
Compare
github-actions
Bot
added
size/L
Emit structured validation, API, network, file, and internal error envelopes for Markdown shortcuts so users and agents can recover from failed markdown workflows using stable type, subtype, param, and code fields. Add Markdown domain errscontract and golangci guards to prevent legacy envelope and common helper regressions.
evandance
force-pushed
the
feat/errs-migrate-markdown
branch
from
04472c9 to
c09814e
Compare
wittam-01
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 Markdown shortcut domain. Scope is limited to
shortcuts/markdown, Markdown error-contract guards, and Markdown guard tests.Changes
errs.*envelopes..golangci.ymlandlint/errscontractso legacy envelopes and common helpers cannot regress.Test Plan
gofmt -l <changed go files>produced no outputgo vet ./shortcuts/markdown/...go test ./shortcuts/markdown/...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/markdown/...go run -C lint . ..go build ./...Related Issues
Summary by CodeRabbit
Refactor
Tests
Chores