Improve agent-facing error guidance for drive, markdown, and wiki#1779
Conversation
github-actions
Bot
added
domain/ccm
|
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:
📝 WalkthroughWalkthroughDrive push, pull, and sync now track abort state explicitly. Drive push also adds per-item hints, treats delete code 1061007 as ChangesDrive terminal failure handling and hints
Markdown and wiki token handling
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant drivePush as drivePush
participant driveClassifyBatchFailure as driveClassifyBatchFailure
participant DriveAPI as DriveAPI
DriveAPI-->>drivePush: upload_all error
drivePush->>driveClassifyBatchFailure: classify problem code
driveClassifyBatchFailure-->>drivePush: decision{Terminal, Hint, ErrorClass}
drivePush->>drivePush: store hint and terminal on failed item
DriveAPI-->>drivePush: DELETE error 1061007
drivePush->>drivePush: append already_deleted item and continue
sequenceDiagram
participant User as User
participant WikiNodeList as WikiNodeList
participant WikiAPI as WikiAPI
User->>WikiNodeList: --space-id and --parent-node-token
WikiNodeList->>WikiNodeList: readWikiNodeListSpec
WikiNodeList->>WikiAPI: fetchWikiNodes(space_id, parent_node_token)
WikiAPI-->>WikiNodeList: API error
WikiNodeList->>WikiNodeList: wikiNodeListProblem appends hint
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shortcuts/drive/drive_push.go (1)
590-638: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTerminal-ness duplicated across two independent switches.
driveClassifyBatchFailuresetsdecision.Terminalper-case (Line 625), anddriveTerminalBatchErrorClassindependently re-derives terminality from the stringErrorClass(Line 656) for use indrivePushHasTerminalFailure. Addingparent_node_missingrequires keeping both switches in sync by hand; a future new terminal class could easily update one and miss the other, silently makingaborted/loop-break behavior diverge from the per-itemTerminalflag returned bydrivePushFailedItem. This is a pre-existing pattern, not introduced by this diff, but it's growing with each new class.Consider deriving
driveTerminalBatchErrorClassfrom a single source of truth (e.g., look up terminality from a class→bool map thatdriveClassifyBatchFailurealso populates from), or storeTerminaldirectly indrivePushIteminstead of re-inferring it fromErrorClassstrings later.Also applies to: 654-661
🤖 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/drive/drive_push.go` around lines 590 - 638, `driveClassifyBatchFailure` and `driveTerminalBatchErrorClass` are duplicating terminal-state logic, so keep terminality in a single source of truth. Update the batching flow around `driveClassifyBatchFailure`, `driveTerminalBatchErrorClass`, and `drivePushHasTerminalFailure` so terminal-ness is derived consistently from one shared mapping or from the stored failure decision itself, rather than re-inferring it from `ErrorClass` strings in a separate switch. This will prevent future classes like `parent_node_missing` from getting out of sync between per-item failure handling and abort/loop-break behavior.
🤖 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 `@skills/lark-drive/references/lark-drive-push.md`:
- Around line 122-125: The delete_failed example is missing the structured
failure fields that the guidance now expects. Update the example near
drivePushFailedItem so the delete failure shape mirrors the other failure
entries by including the same structured fields used for failures, such as
error_class, phase, code, subtype, retryable, and any relevant hints, instead of
only showing error.
- Around line 114-116: The summary schema table is missing the new aborted field
even though the example already exposes summary.aborted. Update the schema
documentation in the lark-drive-push reference so the summary section includes
aborted alongside uploaded, skipped, failed, and deleted_remote, keeping the
table and example in sync.
---
Nitpick comments:
In `@shortcuts/drive/drive_push.go`:
- Around line 590-638: `driveClassifyBatchFailure` and
`driveTerminalBatchErrorClass` are duplicating terminal-state logic, so keep
terminality in a single source of truth. Update the batching flow around
`driveClassifyBatchFailure`, `driveTerminalBatchErrorClass`, and
`drivePushHasTerminalFailure` so terminal-ness is derived consistently from one
shared mapping or from the stored failure decision itself, rather than
re-inferring it from `ErrorClass` strings in a separate switch. This will
prevent future classes like `parent_node_missing` from getting out of sync
between per-item failure handling and abort/loop-break behavior.
🪄 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: 3d216c95-bda8-4e10-8d1a-8fd534652c2a
📒 Files selected for processing (3)
shortcuts/drive/drive_push.goshortcuts/drive/drive_push_test.goskills/lark-drive/references/lark-drive-push.md
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1779 +/- ##
==========================================
- Coverage 74.46% 74.40% -0.07%
==========================================
Files 855 857 +2
Lines 88568 88952 +384
==========================================
+ Hits 65954 66181 +227
- Misses 17543 17668 +125
- Partials 5071 5103 +32 ☔ 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@7a5f5289dc467e9ec36952bdd559e574f3a8fcf4🧩 Skill updatenpx skills add larksuite/cli#codex/drive-push-error-guidance -y -g |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
shortcuts/markdown/markdown_create.go (2)
182-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefix-based classifier duplicates similar token-shape logic elsewhere.
markdownKnownResourceTokenKindre-implements a heuristic that overlaps withlooksLikeWikiNodeTokenused inshortcuts/wiki/wiki_node_list.go. Consider centralizing token-kind detection (e.g. inshortcuts/common) so both the markdown and wiki commands share one source of truth for token prefixes, reducing drift risk if prefix conventions change.🤖 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_create.go` around lines 182 - 204, `markdownKnownResourceTokenKind` duplicates token-prefix detection logic that already exists in `looksLikeWikiNodeToken`, so the markdown and wiki paths can drift. Move the shared token-kind/prefix classification into a common helper (for example under shortcuts/common) and update both `markdownKnownResourceTokenKind` and `looksLikeWikiNodeToken` to call that shared logic so they use one source of truth for token prefixes.
137-180: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFolder/wiki token normalizers are weaker than the parallel wiki implementation.
Compared to
shortcuts/wiki/wiki_node_list.go'snormalizeWikiNodeListParentToken, these two functions:
- Don't reject stray URL fragments/paths (
strings.ContainsAny(token, "/?#")) for non-://inputs, so a malformed partial URL (e.g.wiki/wikcnXXXwithout a scheme) falls through to the prefix heuristic and may be sent to the API as-is, producing an opaque API error instead of a clear CLI validation message.- Don't call an equivalent of
validateOptionalResourceNameon the final token before returning it.Separately, several of the constructed messages (e.g. "Use --wiki-token for wiki nodes or pass a Drive folder URL/token.") bundle recovery guidance directly into the error message. Per coding guidelines, recovery guidance should live in
.WithHint(...), withparam/message limited to naming what failed.markdownValidationParamError's implementation isn't in this diff, so it's worth confirming it doesn't collapse hint text into the message field, for consistency withwiki_node_list.go's pattern (which mixes both styles in the same function — see companion comment there).♻️ Suggested hardening for the missing checks
func normalizeMarkdownFolderToken(token string) (string, error) { token = strings.TrimSpace(token) if strings.Contains(token, "://") { ref, ok := common.ParseResourceURL(token) ... return ref.Token, nil } + if strings.ContainsAny(token, "/?#") { + return "", markdownValidationParamError("--folder-token", "--folder-token must be a raw Drive folder token, not a partial URL or path") + } switch markdownKnownResourceTokenKind(token) { ... } return token, nil }Based on coding guidelines,
param field in errors should only name the user input that actually failed; recovery guidance goes in .WithHint(...).🤖 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_create.go` around lines 137 - 180, The token normalizers in normalizeMarkdownFolderToken and normalizeMarkdownWikiToken are too permissive compared with normalizeWikiNodeListParentToken, so malformed non-URL inputs with stray path/query/fragment characters can slip through and reach the API. Tighten both functions by rejecting tokens containing /, ?, or # before the token-kind heuristic, and by validating the final normalized token with the same optional-resource-name check used in the wiki implementation. Also revise markdownValidationParamError call sites so the error text only names the failed parameter/value, while recovery guidance is moved into a hint-style field rather than embedded in the main message.Source: Coding guidelines
shortcuts/wiki/wiki_helpers.go (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate of
appendMarkdownProblemHint.This is a byte-for-byte duplicate of
appendMarkdownProblemHintinshortcuts/markdown/helpers.go. Consider moving it to a shared location (e.g.shortcuts/common) so both packages call the same implementation.♻️ Suggested consolidation
-func appendWikiProblemHint(err error, hint string) error { - if strings.TrimSpace(hint) == "" { - return err - } - if p, ok := errs.ProblemOf(err); ok { - if strings.TrimSpace(p.Hint) != "" { - p.Hint = p.Hint + "\n" + hint - } else { - p.Hint = hint - } - } - return err -} +// use common.AppendProblemHint(err, hint) insteadAlso applies to: 31-43
🤖 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/wiki/wiki_helpers.go` at line 9, The helper in shortcuts/wiki/wiki_helpers.go is a byte-for-byte duplicate of appendMarkdownProblemHint in shortcuts/markdown/helpers.go, so consolidate the shared logic into a common package and have both call the same implementation. Move the implementation to a shared location such as shortcuts/common, update appendMarkdownProblemHint and its callers to use the shared function, and remove the duplicate copy from the wiki helpers while preserving the existing behavior.shortcuts/wiki/wiki_node_list.go (1)
151-172: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInconsistent Message/Hint separation for validation errors.
Within these two functions, some branches correctly put recovery guidance in
.WithHint(...)(e.g. the "not a numeric wiki space_id" and "not a wiki node token" cases), while sibling branches embed the same kind of guidance directly in the error Message (e.g. "not a URL or path; runlark-cli wiki +space-list --as user..." and "pass a raw wiki node token fromwiki +node-get..."/"Resolve the document URL withlark-cli wiki +node-get..."). Per coding guidelines,paramfield in errors should only name the user input that actually failed; recovery guidance goes in.WithHint(...). Aligning all branches to the pattern already used elsewhere in the same functions would make the resultingProblem.Message/Problem.Hintconsistent for downstream consumers/agents parsing structured fields.♻️ Example fix for one branch
if strings.Contains(spaceID, "://") || strings.ContainsAny(spaceID, "/?#") { return errs.NewValidationError(errs.SubtypeInvalidArgument, - "--space-id must be a numeric wiki space_id, not a URL or path; run `lark-cli wiki +space-list --as user` to discover space IDs", - ).WithParam("--space-id") + "--space-id must be a numeric wiki space_id, not a URL or path", + ).WithParam("--space-id").WithHint("Run `lark-cli wiki +space-list --as user` to discover space IDs.") }Based on coding guidelines,
param field in errors should only name the user input that actually failed; recovery guidance goes in .WithHint(...).Also applies to: 186-219
🤖 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/wiki/wiki_node_list.go` around lines 151 - 172, The validation errors in wiki node/space ID checks mix recovery guidance into the error message instead of keeping it in .WithHint(...), which makes Problem.Message/Problem.Hint inconsistent. Update validateWikiNodeListSpaceID and the related validation branches in the same file to keep the Message focused on the failed input and move any “run lark-cli ...” or “pass ...” guidance into WithHint, following the pattern already used by the numeric space ID and token validation paths.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/wiki/wiki_list_copy_test.go`:
- Around line 134-159: The two error-path tests for WikiNodeList currently
validate only error text, so update TestWikiNodeListRejectsNonNumericSpaceID and
TestWikiNodeListRejectsDocumentURLAsParentNodeToken to assert the structured
problem returned by mountAndRunWiki using errs.ProblemOf instead of
strings.Contains. Check the relevant category/subtype/param metadata and
preserve the cause chain, following the pattern used by
TestWikiNodeListProblemAddsActionableHint so the tests verify typed error
details rather than message substrings.
---
Nitpick comments:
In `@shortcuts/markdown/markdown_create.go`:
- Around line 182-204: `markdownKnownResourceTokenKind` duplicates token-prefix
detection logic that already exists in `looksLikeWikiNodeToken`, so the markdown
and wiki paths can drift. Move the shared token-kind/prefix classification into
a common helper (for example under shortcuts/common) and update both
`markdownKnownResourceTokenKind` and `looksLikeWikiNodeToken` to call that
shared logic so they use one source of truth for token prefixes.
- Around line 137-180: The token normalizers in normalizeMarkdownFolderToken and
normalizeMarkdownWikiToken are too permissive compared with
normalizeWikiNodeListParentToken, so malformed non-URL inputs with stray
path/query/fragment characters can slip through and reach the API. Tighten both
functions by rejecting tokens containing /, ?, or # before the token-kind
heuristic, and by validating the final normalized token with the same
optional-resource-name check used in the wiki implementation. Also revise
markdownValidationParamError call sites so the error text only names the failed
parameter/value, while recovery guidance is moved into a hint-style field rather
than embedded in the main message.
In `@shortcuts/wiki/wiki_helpers.go`:
- Line 9: The helper in shortcuts/wiki/wiki_helpers.go is a byte-for-byte
duplicate of appendMarkdownProblemHint in shortcuts/markdown/helpers.go, so
consolidate the shared logic into a common package and have both call the same
implementation. Move the implementation to a shared location such as
shortcuts/common, update appendMarkdownProblemHint and its callers to use the
shared function, and remove the duplicate copy from the wiki helpers while
preserving the existing behavior.
In `@shortcuts/wiki/wiki_node_list.go`:
- Around line 151-172: The validation errors in wiki node/space ID checks mix
recovery guidance into the error message instead of keeping it in
.WithHint(...), which makes Problem.Message/Problem.Hint inconsistent. Update
validateWikiNodeListSpaceID and the related validation branches in the same file
to keep the Message focused on the failed input and move any “run lark-cli ...”
or “pass ...” guidance into WithHint, following the pattern already used by the
numeric space ID and token validation paths.
🪄 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: ee959ed9-aa00-40c9-a6da-40bbcda739f6
📒 Files selected for processing (13)
internal/errclass/codemeta_drive.gointernal/errclass/codemeta_test.gointernal/errclass/codemeta_wiki.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_create.goshortcuts/markdown/markdown_test.goshortcuts/wiki/wiki_helpers.goshortcuts/wiki/wiki_list_copy_test.goshortcuts/wiki/wiki_node_list.goskills/lark-markdown/SKILL.mdskills/lark-markdown/references/lark-markdown-create.mdskills/lark-wiki/SKILL.mdskills/lark-wiki/references/lark-wiki-node-list.md
✅ Files skipped from review due to trivial changes (1)
- skills/lark-wiki/SKILL.md
fangshuyu-768
changed the title
ee5ea90 to
004f8a7
Compare
3 participants
Summary
markdown +createandwiki +node-listContext
Failure telemetry showed several repeated agent error patterns:
drive +push: missing parent folders and delete-remote already-deleted responses caused noisy hard failuresmarkdown +create: agents mixed folder/wiki/document tokens, retried quota/not-found/server failures without enough guidancewiki +node-list: agents passed wiki URLs, node tokens, document tokens, or stale page tokens where numericspace_id/ wikinode_tokenwere requiredThis PR turns those cases into clearer CLI behavior:
Tests
go test ./shortcuts/drive ./internal/errclassgo test ./shortcuts/drive ./internal/errclass ./internal/qualitygate/skillscan ./internal/skillcontentgo test ./shortcuts/wiki ./shortcuts/markdown ./internal/errclass ./internal/qualitygate/skillscan ./internal/skillcontentNotes
go test ./internal/qualitygate/... ./internal/skillcontent/... ./shortcuts/drive ./internal/errclassstill fails ininternal/qualitygate/cmd/manifest-exportbecause the existing command index test cannot finddrive file.comments create_v2; the affected packages above pass.Summary by CodeRabbit
drive push,drive pull, anddrive syncnow stop immediately on terminal errors and reportabortedin the summary.markdown +createand wiki/markdown targeting now accept supported folder/wiki URLs and normalize them automatically.1061007) as idempotent success (action: already_deleted) during--delete-remote.+node-listparsing, validation, and error hints (space IDs and parent-node tokens), including stale/invalid cases.