Skip to content

fix(cli): scope destructive typo recovery to each command - #25699

Closed
kambarakun wants to merge 2 commits into
stablyai:mainfrom
kambarakun:fix/orchestration-reset-destructive-suggestion
Closed

kambarakun wants to merge 2 commits into
stablyai:mainfrom
kambarakun:fix/orchestration-reset-destructive-suggestion

Conversation

@kambarakun

@kambarakun kambarakun commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

ELI5

Mistyped commands should not steer users toward resetting or deleting saved state. This protects reset suggestions and keeps a typo near one destructive command from unlocking a different one. Close misspellings still recover.

What Changed

  • Mark orchestration reset as destructive.
  • Check destructive intent against each command's own verb and aliases, so artifacts deset cannot unlock artifacts delete merely by resembling reset.
  • Keep all 16 behavioral regression cases in src/cli/command-suggestion.test.ts, covering suggestions, nextSteps, and near-miss recovery across isolated and real registries.
  • Combine duplicate imports in the orchestration spec to preserve its 300-line limit.

Why

Reset was missing its destructive flag. The guard also treated destructive intent globally: classifying reset made an unrelated deletion eligible. Applying the existing one-edit rule to each command fixes both problems without a special-case blacklist. Ranking thresholds, reset execution, and scope validation stay unchanged.

rest intentionally remains eligible because it is one edit from reset; matching any alias still permits recovery of that same command. Verbs absent from a command's own paths and aliases intentionally no longer unlock it (for example, environment remov no longer suggests environment rm).

Linked Issue

Fixes #25571

Visual Proof

Historical CLI captures are shown below with their original revisions. The rebuilt CLI at 26d65d1b0b61796e98d4bcced0d2528aeecfc4c0 reproduces the same After results; no graphical UI is involved.

node out/cli/index.js orchestration resume --json — baseline 10bea3a8faefc7adf8771f5bfd57a37435ccede4 to reviewed head 83858ef2dfca996f56af29dfc3591d85d49e54d9:

Before error.data:
{"suggestions":["orchestration reset"],"nextSteps":["Did you mean: orca orchestration reset"]}
After error.data:
{"suggestions":[],"nextSteps":[]}

node out/cli/index.js artifacts deset --json — initial head fcc2fcca01bffa1fef3e6b31cbb15503b9679469 to reviewed head 83858ef2dfca996f56af29dfc3591d85d49e54d9:

Before error.data:
{"suggestions":["artifacts delete","artifacts list"],"nextSteps":["Did you mean: orca artifacts delete, orca artifacts list"]}
After error.data:
{"suggestions":["artifacts list"],"nextSteps":["Did you mean: orca artifacts list"]}

Both commands exit 1 with invalid_argument / Unknown command. artifacts delet, orchestration rese/rest, worktree remov/delet, and orchestration chek retain recovery. reset --help is byte-identical.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated

Rebased onto 670c59d23a1d87f9ef3bd63b65f44673f16d28e2 after #25791 removed the old spec-text test file. Moved this PR's 12 reset-recovery cases alongside its four cross-command cases; no retired spec-text tests were restored. Both production files are byte-identical to 83858ef2dfca996f56af29dfc3591d85d49e54d9.

Revalidated 26d65d1b0b61796e98d4bcced0d2528aeecfc4c0 on macOS 26.5.2 arm64, Node 24.16.0, pnpm 12.8.1:

  • pnpm test src/cli/command-suggestion.test.ts: 31 passed, including all 16 added cases with no skips.
  • Negative controls run separately: removing only reset's flag caused 6 expected assertion failures; restoring the old global guard caused 2. Restored the original files, then all 31 passed again with a clean worktree.
  • pnpm test src/cli: 133 files; 1,635 passed, 4 skipped.
  • pnpm tc:cli, targeted oxlint / oxfmt --check for the three changed files, and pnpm run check:code-quality:changed -- 670c59d23a1d87f9ef3bd63b65f44673f16d28e2 passed.
  • CLI built with pnpm exec tsc -p config/tsconfig.cli.json --outDir out --composite false --incremental false and node config/scripts/verify-cli-bin.mjs --fix-executable --fix-package-json; no global installation hook.
  • Verified all 10 smoke inputs remain unregistered using the real registry and argument normalization before executing them. Checked exit codes, unknown-command errors, suggestions and nextSteps. Source inspection and CLI index tests confirm validation precedes runtime lookup.
  • reset --help passed and matched the historical capture byte-for-byte. No reset or deletion was executed.

Full repository lint/typecheck/unit suite, desktop build, Windows/Linux, and live SSH were not run.

AI Disclosure

OpenAI Codex implemented, tested, and rebased the change. Claude Opus 5.5 independently reviewed this conflict repair. The earlier CodeRabbit CLI 0.8.2 and Claude reviews apply to 83858ef2df, not the rebased head.

Review

Claude Opus 5.5 independently reviewed this rebase repair and found no actionable issues: all 16 added cases and 15 existing suggestion cases remain, no retired spec-text checks were restored, and the production files match the old head. The review used source and recorded evidence; it did not execute tests.

Historical review of 83858ef2df: CodeRabbit CLI reported zero findings; Claude found no blocking issue and noted the documented narrowing of undeclared synonym recovery. These earlier results are not presented as new-head GitHub review or CI approval.

The change affects suggestion policy, not execution authorization. It retains the length guard before edit-distance calculation. No execution handlers, platform branches, remote routing, agents, git-provider integration, mobile UI, persistence, or wire formats change.

Agent skill upstream boundary

  • Not applicable; no upstream skill content copied.

Notes

CLI recovery runs before runtime lookup. Local, SSH, and folder execution paths are unchanged.

X: @kambarakun

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after CLI output supplied; graphical screenshots are not applicable
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

Original merge base: 10bea3a8faefc7adf8771f5bfd57a37435ccede4. Pre-rebase head: 83858ef2dfca996f56af29dfc3591d85d49e54d9. Rebase base: 670c59d23a1d87f9ef3bd63b65f44673f16d28e2. Revalidated head: 26d65d1b0b61796e98d4bcced0d2528aeecfc4c0.

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4ea86c3d-36ff-4c09-a03e-5395d5cc2de1
📥 Commits

Reviewing files that changed from the base of the PR and between 83858ef2dfca996f56af29dfc3591d85d49e54d9 and 26d65d1.

📒 Files selected for processing (1)
  • src/cli/command-suggestion.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Destructive command suggestions are now checked against each command’s own paths or aliases. The orchestration reset command is marked destructive. Tests cover reset and artifact deletion suggestions with isolated and full CLI registries.

Priority: ⬇️ Low

Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 26d65

No concrete merge-blocking risk is identified for the command-suggestion change. Merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to fcc2f

The change suppresses misleading reset suggestions, but also makes some unrelated destructive commands eligible for recovery suggestions. Suggestions do not execute automatically, and command execution still requires a separate valid invocation.

Retained concerns

  • Low · security · inferred: Adding reset to the shared destructive-intent set makes unrelated destructive suggestions newly eligible. With the full registry, artifacts deset can now yield artifacts delete: deset is one edit from reset, while the deletion candidate is two edits from the complete input. This can steer a user or recovery agent toward an unintended deletion, although executing it requires a separate invocation and an artifact ID.
Security review details

Security Blast Radius

  • inferred — The changed recovery policy affects the complete CLI registry, not only orchestration. The concrete deletion example targets an artifact ID through the existing artifact deletion operation; the inspected CLI rejects remote-selection flags for this operation. Cross-account authorization was not independently established.

Security Findings and Attack Paths

  • inferred — A caller-controlled unknown verb near reset can newly place deletion guidance in suggestions and nextSteps. Harm requires a user or agent to follow that guidance and supply valid execution arguments; automatic execution or privilege escalation was not demonstrated.

Trust Boundaries and Controls

  • observed — The suggestion filter is advisory, not execution authorization. Reset execution independently rejects missing or conflicting scopes before invoking orchestration.reset; artifact deletion requires an ID. These controls limit the consequences of a misleading suggestion.

Hardening Proposals

  • proposed — Consider binding destructive near-match eligibility to each candidate's own verb and aliases, so classifying another command cannot broaden unrelated destructive recovery suggestions.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #25571 requires orchestration reset to be marked destructive so unrelated typos do not suggest it. src/cli/specs/orchestration.ts sets destructive: true. src/cli/command-suggestion.ts ch…
Out of Scope Changes check ✅ Passed The command-specific destructive-intent check and its regression tests directly support typo-recovery safety in issue #25571. The orchestration import change is formatting-only. The reviewed diff cont…
Title check ✅ Passed The title clearly and concisely describes the main change: destructive typo recovery is scoped to each command.
Description check ✅ Passed The description covers the required sections, links issue #25571, explains the behavior change, and reports tests and unrun checks. It also provides CLI before-and-after output where graphical proof d…
  • Fix all pre-merge checks with AI
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kambarakun kambarakun changed the title fix(orchestration): mark reset as destructive for typo recovery fix(cli): scope destructive typo recovery to each command Oct 5, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This review covered both commits and all four changed files.

  • orchestration reset marked destructive — src/cli/specs/orchestration.ts:295 sets destructive: true, so reset stops surfacing for unrelated verbs (resume, rerun, repl) while rese/rest still recover it.
  • Destructive intent scoped per command — src/cli/command-suggestion.ts:19 replaces the registry-wide destructiveVerbs/intendsDestruction pair with intendsDestructiveCommand, so a near-miss unlocks only the candidate whose own path or alias it resembles. artifacts deset no longer unlocks artifacts delete.
  • Regression coverage — new describe.each blocks exercise both the isolated and full registries in src/cli/command-suggestion.test.ts and src/cli/specs/orchestration.test.ts.
  • Import merge — src/cli/specs/orchestration.ts:1 folds the two ../args imports into one to stay under the 300-line cap.

The new guard is a strict subset of the old one (per-spec verbs ⊆ all destructive verbs), so it can only remove unlocks, never add them; the regression test for artifacts deset would fail against the pre-scoping code. pnpm test src/cli passes (141 files, 1658 tests).

ℹ️ Per-command scoping also narrows a few recovery paths

Binding the guard to each command's own verbs means a near-miss only recovers a candidate that shares its verb. environment remov no longer suggests environment rm (the spec's only verb is rm, with no remove alias), and a compound verb like project setup-del likewise falls outside the one-edit window. These were reachable before only through incidental cross-spec verb leakage, so this is a deliberate tradeoff the PR body already documents; noting it so the full set is on the record rather than only the one example given.

Technical details
# Recovery narrowing from per-command destructive scoping

## Affected sites
- `src/cli/command-suggestion.ts:53` — guard now binds the input token to the candidate spec's own `specPaths` final tokens.
- `src/cli/specs/environment.ts:55` — `environment rm` is the only verb; there is no `remove` alias.
- `src/cli/specs/project.ts:115` — `project setup-delete` is a compound verb; `setup-del` is more than one edit away.

## Required outcome
- None; informational. If that recovery is wanted back, registering `remove` as an alias on `environment rm` (matching `worktree rm` / `account rm`) restores it without weakening the guard.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Use the existing suggestion guard for orchestration reset. Keep near-miss reset recovery and non-destructive command suggestions unchanged.
Match destructive intent against each candidate command and its aliases. Prevent a reset near-miss from unlocking unrelated deletion suggestions, while retaining close-typo and alias recovery.
@kambarakun
kambarakun force-pushed the fix/orchestration-reset-destructive-suggestion branch from 83858ef to 26d65d1 Compare October 6, 2026 08:25
@kambarakun

Copy link
Copy Markdown
Contributor Author

Closing this PR as superseded by #26934, which has been merged and resolves #25571.

The follow-up builds on the reset destructive marker and candidate-specific guard approach from this PR, while preserving alias recovery and extending the protection to additional destructive commands.

Thanks for carrying this forward and for the attribution!

@kambarakun kambarakun closed this Oct 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: orchestration reset is not marked destructive, so CLI typo recovery suggests it

1 participant