fix: make agent recovery and concealment reliable - #2189
Merged
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:
📝 WalkthroughWalkthroughThe change centralizes typed error presentation, adds device-code recovery hints, projects schema catalogs by command visibility, validates skill dependencies during composition, and resolves surface-aware skill references for runtime guidance. ChangesSurface-aware CLI behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 5
🧹 Nitpick comments (2)
tests/cli_e2e/docs/docs_update_dryrun_test.go (1)
250-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that stdout stays empty on this validate-stage failure.
The test verifies exit code 2 and the typed envelope on stderr. It does not verify that stdout stays empty. A regression that writes the error to stdout would still pass.
Stream separation is a stated repository rule: send JSON program data to stdout and send hints to stderr, and never mix the two streams. Add the assertion so this new error path pins that rule.
💚 Proposed addition
require.NoError(t, err) result.AssertExitCode(t, 2) + require.Empty(t, result.Stdout, "validate-stage failure must not write to stdout") require.Equal(t, "validation", gjson.Get(result.Stderr, "error.type").String(), result.Stderr)🤖 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 `@tests/cli_e2e/docs/docs_update_dryrun_test.go` around lines 250 - 259, Add an assertion in the validate-stage failure test around the existing result.AssertExitCode and stderr envelope checks to verify result.Stdout is empty. Keep the current stderr assertions unchanged and pin the stream-separation rule for this error path.Source: Coding guidelines
internal/skillpolicy/resolver_test.go (1)
364-372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the two new fail-closed manifest paths.
This PR introduces two abort conditions that no test exercises:
parseRequiredSkillsreturns"SKILL.md frontmatter is not closed"when an opening---has no closing---(internal/skillpolicy/dependencies.golines 58-60).parseRequiredSkillsrejects a dependency that failsisSkillName(internal/skillpolicy/dependencies.golines 77-79).Both surface through
scanSkillTreeas"skill %q has invalid metadata"and abort the build. The unclosed-frontmatter path applies to the host base tree, so it can break an existing overlay distribution. Add tests so a later relaxation of either rule fails.💚 Proposed tests
+func TestResolve_UnclosedFrontmatterFailsClosed(t *testing.T) { + base := skillFS(map[string]string{ + "lark-a/SKILL.md": "---\nmetadata:\n requires:\n skills: [\"lark-shared\"]\nbase a", + }) + _, err := resolveContent(base, []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{Allow: []string{"lark-a"}}, + }}) + if err == nil { + t.Fatal("unclosed frontmatter unexpectedly resolved") + } + for _, want := range []string{"lark-a", "invalid metadata"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not identify %q", err, want) + } + } +} + +func TestResolve_InvalidRequiredSkillNameFailsClosed(t *testing.T) { + base := skillFS(map[string]string{ + "lark-a/SKILL.md": "---\nmetadata:\n requires:\n skills: [\"../escape\"]\n---\nbase a", + }) + _, err := resolveContent(base, []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{Allow: []string{"lark-a"}}, + }}) + if err == nil { + t.Fatal("invalid required skill name unexpectedly resolved") + } + if !strings.Contains(err.Error(), "../escape") { + t.Errorf("error %q does not identify the invalid dependency", 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 `@internal/skillpolicy/resolver_test.go` around lines 364 - 372, Add resolver tests covering both fail-closed manifest paths through scanSkillTree: an unclosed frontmatter block in the host base tree and a dependency name rejected by isSkillName. Assert each causes resolution to abort with the existing “skill %q has invalid metadata” error, using the setup patterns from TestResolve_DoesNotInferDependenciesFromMarkdownLinks.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 `@cmd/error_presenter_test.go`:
- Around line 26-34: Strengthen the listed error-path tests to validate typed
metadata and cause preservation, not only rendered hints: in
cmd/error_presenter_test.go lines 26-34 assert errs.ProblemOf succeeds, then
verify the expected category and subtype and add a wrapped cause checked with
errors.Is; in internal/errclass/hint_gate_test.go lines 23-68 assert the
rendered permission error’s category and subtype and preserve a wrapped cause;
in cmd/root_test.go lines 546-562 assert the authentication subtype and
NeedAuthorizationError cause, lines 584-591 and 613-620 assert the
authentication category and subtype, and lines 644-655 assert category, subtype,
and the existing error chain. Use errors.As for Param only where the concrete
error type exposes it.
In `@internal/cmdutil/error_presenter_test.go`:
- Around line 16-58: Update
TestFactoryPresentErrorClonesAndPreservesPermissionMachineFields to attach a
sentinel cause to source and assert the presented error preserves it. Use
errs.ProblemOf on the presented error to verify the expected category, subtype,
and param metadata, while retaining the existing clone and field-preservation
checks.
In `@shortcuts/doc/v2_only_test.go`:
- Around line 42-56: Strengthen the typed validation assertions in
shortcuts/doc/v2_only_test.go:42-56 by checking problem.Category is
CategoryValidation and problem.Subtype is SubtypeInvalidArgument through
errs.ProblemOf, while retaining the existing errors.As assertion for
*errs.ValidationError and its --mode Param. Apply the same category and subtype
assertions in shortcuts/doc/docs_create_test.go:285-297 and assert its
validation Param is --markdown; in shortcuts/doc/docs_update_test.go:238-244,
assert the category, subtype, and --mode Param.
In `@tests/plugin_e2e/harness.go`:
- Around line 131-139: Replace the direct os.MkdirAll call in the fixture setup
loop with the corresponding internal/vfs directory-creation API, preserving the
existing path, permissions, error handling, and t.Fatalf behavior. Update any
required import or helper usage in the surrounding harness code without changing
unrelated filesystem operations.
In `@tests/plugin_e2e/skills_test.go`:
- Around line 272-282: Update the assertions in the affected test to parse the
documented JSON fields from res.stderr with gjson instead of checking raw
substrings. Assert the exact reason code field equals invalid_skills_overlay and
verify the affected-skills field contains exactly lark-doc and lark-shared,
while preserving the existing error.type and error.subtype assertions.
---
Nitpick comments:
In `@internal/skillpolicy/resolver_test.go`:
- Around line 364-372: Add resolver tests covering both fail-closed manifest
paths through scanSkillTree: an unclosed frontmatter block in the host base tree
and a dependency name rejected by isSkillName. Assert each causes resolution to
abort with the existing “skill %q has invalid metadata” error, using the setup
patterns from TestResolve_DoesNotInferDependenciesFromMarkdownLinks.
In `@tests/cli_e2e/docs/docs_update_dryrun_test.go`:
- Around line 250-259: Add an assertion in the validate-stage failure test
around the existing result.AssertExitCode and stderr envelope checks to verify
result.Stdout is empty. Keep the current stderr assertions unchanged and pin the
stream-separation rule for this error path.
🪄 Autofix
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 Plus
Run ID: f6972560-3a88-4608-a411-57bdb6b3e63d
📒 Files selected for processing (40)
cmd/build.gocmd/error_auth_hint.gocmd/error_presenter_test.gocmd/root.gocmd/root_test.gocmd/schema/schema.gocmd/schema/schema_test.goerrs/ERROR_CONTRACT.mdextension/platform/README.mdextension/platform/skillsoverlay.gointernal/cmdutil/error_presenter.gointernal/cmdutil/error_presenter_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_test.gointernal/errclass/classify.gointernal/errclass/hint_gate_test.gointernal/recovery/hint.gointernal/recovery/hint_test.gointernal/skillpolicy/dependencies.gointernal/skillpolicy/overlay.gointernal/skillpolicy/resolver.gointernal/skillpolicy/resolver_test.goshortcuts/common/runner.goshortcuts/common/skill_references.goshortcuts/doc/docs_create_test.goshortcuts/doc/docs_fetch_v2_test.goshortcuts/doc/docs_update_test.goshortcuts/doc/v2_only.goshortcuts/doc/v2_only_test.goshortcuts/task/tasklist_add_task.goshortcuts/task/tasklist_add_task_test.goshortcuts/task/tasklist_create.goshortcuts/task/tasklist_create_test.goshortcuts/vc/vc_notes.goshortcuts/vc/vc_notes_test.goskills/lark-doc/SKILL.mdtests/cli_e2e/docs/docs_update_dryrun_test.gotests/plugin_e2e/harness.gotests/plugin_e2e/restrict_test.gotests/plugin_e2e/skills_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@aa7f9e344011cdf7095c0f9155cb76384696ccd8🧩 Skill updatenpx skills add larksuite/cli#fix/agent-recovery-and-concealment -y -g |
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 `@internal/skillpolicy/resolver_test.go`:
- Around line 382-389: Strengthen both invalid-host-base error-path tests around
ErrInvalidHostBase to verify typed metadata, not just message text. Use
errs.ProblemOf(err) to assert the expected Category and Subtype, extract
*errs.ValidationError with errors.As to verify Param, and assert the expected
wrapped parse or validation cause with errors.Is or errors.As. Retain the
existing ErrInvalidHostBase sentinel and message assertions as supplementary
checks.
🪄 Autofix
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 Plus
Run ID: 28fe8693-037e-45ad-a4ce-8620bdd34228
📒 Files selected for processing (11)
cmd/error_presenter_test.gocmd/root_test.gointernal/cmdutil/error_presenter_test.gointernal/errclass/hint_gate_test.gointernal/skillpolicy/resolver_test.goshortcuts/doc/docs_create_test.goshortcuts/doc/docs_update_test.goshortcuts/doc/v2_only_test.gotests/cli_e2e/docs/docs_update_dryrun_test.gotests/plugin_e2e/harness.gotests/plugin_e2e/skills_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- shortcuts/doc/docs_update_test.go
- internal/errclass/hint_gate_test.go
- tests/plugin_e2e/skills_test.go
- tests/plugin_e2e/harness.go
- internal/cmdutil/error_presenter_test.go
- cmd/root_test.go
- shortcuts/doc/docs_create_test.go
- cmd/error_presenter_test.go
- tests/cli_e2e/docs/docs_update_dryrun_test.go
- shortcuts/doc/v2_only_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2189 +/- ##
==========================================
+ Coverage 75.92% 76.00% +0.07%
==========================================
Files 962 966 +4
Lines 102197 102541 +344
==========================================
+ Hits 77591 77933 +342
+ Misses 18716 18704 -12
- Partials 5890 5904 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 `@cmd/error_presenter_test.go`:
- Around line 106-123: Update the test around the presented permission error to
obtain problem metadata via errs.ProblemOf(rendered), and assert Category and
Subtype from that result instead of presented. Keep the concrete
*errs.PermissionError assertion for Hint, Message, Identity, and MissingScopes,
and preserve the existing errors.Is cause assertion.
🪄 Autofix
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 Plus
Run ID: 76c6708d-e217-4752-86ef-74e973f858cd
📒 Files selected for processing (3)
cmd/error_presenter_test.gocmd/skill_customization_test.gotests/plugin_e2e/skills_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/plugin_e2e/skills_test.go
7 tasks
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cmd/auth/check_test.go`:
- Around line 205-207: Isolate the test’s configuration state by setting
LARKSUITE_CLI_CONFIG_DIR to t.TempDir() with t.Setenv at the start of the test
containing the default suggestion assertion, before calling
cmdutil.TestFactory(t, cfg).
In `@shortcuts/common/mcp_client.go`:
- Around line 125-126: Replace the generic payload decoding in the
response-handling flow around json.Unmarshal with a typed MCP error envelope,
defining one projection function for each supported HTTP error shape. Decode
directly into the appropriate typed structure so numeric fields and error
properties are handled explicitly rather than through map[string]interface{}.
- Around line 134-145: Update the MCP error handling around
classifyMCPPayloadError and the hasBusinessError fallback so HTTP 401 responses
with unknown structured codes reach the existing AuthenticationError and
withMCPAuthenticationRecovery path before any generic APIError return. Preserve
classification for known business codes, and add coverage for both top-level and
JSON-RPC error payloads containing unknown codes.
🪄 Autofix
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 Plus
Run ID: 8a1fb4ff-d5bc-408e-b37e-94c2c680e454
📒 Files selected for processing (23)
cmd/auth/check.gocmd/auth/check_test.gocmd/auth/login.gocmd/auth/login_messages.gocmd/auth/login_messages_test.gocmd/auth/login_test.gocmd/error_presenter_test.gocmd/root_test.gocmd/skill_customization_test.gointernal/cmdutil/error_presenter.gointernal/errclass/classify.gointernal/errclass/classify_test.gointernal/recovery/hint.gointernal/recovery/hint_test.gointernal/skillpolicy/dependencies.gointernal/skillpolicy/resolver_test.goshortcuts/common/mcp_client.goshortcuts/common/mcp_client_test.goshortcuts/task/tasklist_create_test.goshortcuts/vc/vc_calendar_event_recovery_test.goshortcuts/vc/vc_notes.goshortcuts/vc/vc_recording.goskills/lark-shared/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (7)
- shortcuts/task/tasklist_create_test.go
- cmd/skill_customization_test.go
- internal/recovery/hint_test.go
- internal/cmdutil/error_presenter.go
- internal/recovery/hint.go
- cmd/error_presenter_test.go
- internal/skillpolicy/dependencies.go
7 tasks
evandance
force-pushed
the
fix/agent-recovery-and-concealment
branch
from
August 5, 2026 07:54
8704958 to
eb663cc
Compare
5 tasks
evandance
force-pushed
the
fix/agent-recovery-and-concealment
branch
from
August 5, 2026 10:36
eb663cc to
d524df1
Compare
evandance
force-pushed
the
fix/agent-recovery-and-concealment
branch
from
August 5, 2026 10:46
d524df1 to
aa7f9e3
Compare
liangshuo-1
approved these changes
Aug 5, 2026
sang-neo03
added a commit
that referenced
this pull request
Aug 5, 2026
Reverts the unattended-login change: auth login no longer decides whether to block from whether stdout is a terminal, and the --wait flag it added is gone. The owner asked for this to land as its own change rather than riding along with the token fixes. Reverted to the merge base, not to origin/main. login.go and login_test.go moved on main after this branch forked (#2189), and taking those files from origin/main would have pulled that work in under a revert commit. Merging main will bring it in on its own. This also undoes a mistake of mine: main already emits `"event": "device_authorization"` in the --no-wait payload. The reverted change had dropped that field while rewriting the block, and I "restored" it under a different name (authorization_requested), which would have broken any caller matching on the original value. Reverting the file restores the original name. Still in this PR: the corrupted / expired stored-token fixes, and bot auth for docs +search, base +title-resolve and vc +detail.
This was referenced Aug 6, 2026
zkh-bytedance
pushed a commit
that referenced
this pull request
Aug 6, 2026
Co-authored-by: TRAE CLI <traecli@bytedance.com>
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
Restore the agent-facing OAuth handoff detail lost when #1837 centralized typed permission recovery, and close command-surface projection gaps exposed by that change. In the affected typed-error,
auth check, MCP, and inline-result paths, user authorization now uses an explicit non-blocking two-turn device flow, while bot calls stay on bot/app/admin recovery.The one-command blocking root/classifier wording already existed before #1837. This PR therefore does two things deliberately: it restores the shortcut operational guidance lost during centralization, and it upgrades the older central recovery to an executable
--no-wait --json/--device-codeflow. Error, schema, docs, and skill behavior is derived from the final build-local surface without mutating producer errors.Changes
auth checkmissing-scope suggestions, start withauth login --scope "..." --no-wait --json(or--recommend --no-wait --jsononly when scope context is unavailable), presentverification_urlto the user and end the turn, then resume with--device-codeafter confirmation.permission_violations, rebuild user recovery from the current command's declared scopes instead of falling back to--recommend. Server-reported scopes retain precedence, andmissing_scopesremains server-owned rather than being synthesized.--profileoverride in both generated OAuth commands, including the start and--device-coderesume steps, with shell-safe argument quoting. The default profile-free output remains unchanged from this PR's existing two-turn flow.missing_scope,token_scope_insufficient,user_unauthorized, andpermission_deniedby the actual calling identity. Bot messages and hints stay on app, bot, tenant, resource, or admin remediation and never recommend user OAuth.token_invalidrecovery, while registered business codes retain their precise classification and non-401 unknown-code fallback remains unchanged.calendar_event_idanderrorfields remain intact.schema.--helpplus version-matched embedded skill references, honoring command concealment and skill remap/removal.metadata.requires.skillsdependencies afterSkillsOverlaycomposition, including UTF-8-BOM frontmatter, and declarelark-doc -> lark-shared. Incomplete composed distributions fail closed without wideningAllowor overridingRemove.validation/command_unavailablewire contract and expected consumer behavior.Scope and compatibility
type,subtype,code,missing_scopes,identity, andlog_id) remain unchanged. Human-facinghint/suggestionintentionally changes to the two-turn flow. Bot-only informational messages are corrected where the old text named the wrong actor. Affected VC partial results gain an additive projectedhintfield.auth check, MCP errors, and the identified inline result sinks. It does not claim a repository-wide rewrite of every pre-existing status note, helpTipsentry, or post-success business-authored login sentence; that family needs a separate semantics-aware sink audit and broader lint coverage.SkillsOverlaycomposition; this PR does not introduce a separate scan for no-overlay host bases.Test Plan
make unit-test(race-enabled)go vet ./...gofmt -l .(no output)go mod tidy(nogo.mod/go.sumchanges)--new-from-rev=origin/main(0 issues)tests/plugin_e2e, including external-wrapper schema concealment, remapped docs recovery, skill dependency guards, and concealed authorization fallbackRelated Issues