fix(cli,protocols): registry⇄MATRIX drift guard (#130) + roll out "did you mean?" flag errors (#111) - #157
Merged
Merged
Conversation
…d you mean?" flag errors (#111) Two mechanical, no-CHR Dry July items bundled per the umbrella's micro-item PR-bundling rule (#152). #130 protocols/index.ts registry drift (bug): - native-api: add missing `transfer` capability (transfer/native-api is CHR-passed) - ssh: implemented false -> true (execute/terminal/transfer all CHR-passed) - mndp: implemented false -> true (discover/mndp is CHR-passed) - add test/unit/protocol-registry.test.ts: derives the expectation from the docs/MATRIX.md grid (every CHR-passed cell implies implemented:true + the matching capability), so it fails if the registry under-claims again. MATRIX stays the only status surface. Module header states the contract. - Grounded: the only runtime `.implemented` consumer (retrieve) is gated behind a retrieve-capability check ssh/mndp never pass, so these are descriptive-only corrections with no runtime path change. #111 roll shared unknownFlagError("did you mean?") into the parsers still throwing bare errors — transfer, terminal, btest (client+server), devices, discover, mcp — matching the api parser wired in PR #110. Adds per-parser near-miss coverage in cli-errors.test.ts via in-process runCli. Verification: bun run lint && bun run test && bun run build (unit-tier; no CHR). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR standardizes unknown-flag error handling across six CLI parsers (btest, devices, discover, mcp, terminal, transfer) by using a shared unknownFlagError helper, and fixes protocol registry drift by correcting implemented status and capabilities for ssh, mndp, and native-api, with new tests for both. ChangesShared unknownFlagError rollout across CLI parsers
Protocol registry drift fix against MATRIX
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Hardens centrs’ internal consistency by (1) preventing protocol registry drift from the MATRIX status grid and (2) standardizing CLI unknown-flag UX across commands via the shared unknownFlagError helper.
Changes:
- Added a unit drift-guard that parses
docs/MATRIX.mdand asserts the protocol registry never under-claims vsCHR-passedcells. - Aligned
src/protocols/index.tswith MATRIX reality (native-apitransfer,sshimplemented,mndpimplemented) and documented the contract. - Rolled out shared “Did you mean …?” unknown-flag errors across remaining CLI parsers and added near-miss coverage.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
test/unit/protocol-registry.test.ts |
New MATRIX→registry drift guard + regression anchors for the #130 fixes. |
test/unit/cli-errors.test.ts |
Adds coverage asserting “Did you mean?” suggestions and non-stack error output for unknown flags. |
src/protocols/index.ts |
Updates registry capabilities/implemented flags to match MATRIX; documents asserted contract. |
src/cli/transfer.ts |
Uses shared unknownFlagError for unknown flags. |
src/cli/terminal.ts |
Uses shared unknownFlagError for unknown flags. |
src/cli/mcp.ts |
Uses shared unknownFlagError for unknown flags. |
src/cli/discover.ts |
Uses shared unknownFlagError for unknown flags. |
src/cli/devices.ts |
Uses shared unknownFlagError for unknown flags. |
src/cli/btest.ts |
Uses shared unknownFlagError for unknown flags (client + server). |
Comment on lines
58
to
+61
| const arg = args[index]; | ||
| if (arg === undefined) { | ||
| continue; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Keeping the if (arg === undefined) continue; guard: it exists to satisfy TypeScript noUncheckedIndexedAccess (indexed access on args[index] is string | undefined) and it intentionally mirrors the identical idiom already used by the sibling parsers — discover.ts:91, devices.ts:193, transfer.ts:334. args is never sparse here, so the branch is inert in practice; if it ever did fire it skips one stray token rather than dereferencing undefined. Making mcp fail-loud while the other three continue would be a one-off inconsistency, so I'm leaving it consistent with the established pattern.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/cli/btest.ts`:
- Line 280: The unknown-flag handling in btest is using shared
btestCommand.options, which can produce suggestions from the wrong mode. Update
the unknownFlagError calls in the btest command flow to pass only the active
mode’s options (client vs server), using the existing btest mode handling around
btestCommand and the relevant client/server option lists so suggestions stay
actionable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 462652fd-34c2-4e45-b1f7-3db55fbd8f8d
📒 Files selected for processing (9)
src/cli/btest.tssrc/cli/devices.tssrc/cli/discover.tssrc/cli/mcp.tssrc/cli/terminal.tssrc/cli/transfer.tssrc/protocols/index.tstest/unit/cli-errors.test.tstest/unit/protocol-registry.test.ts
…ver mode CodeRabbit review on #157: btest's single `btestCommand.options` list documents both subcommands, so a typo in one mode could suggest a flag that only exists in the other (e.g. `btest server --protcol` hinting the client-only `--protocol`). Add `btestOptionsFor(mode)` to filter by the `(client)`/`(server)` marker (shared flags carry no marker → kept for both; a dropped marker degrades to the old behavior, never a crash) and pass the scoped list to unknownFlagError at both throw sites. Adds a regression test asserting cross-mode isolation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.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
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.
Pull Request
Two mechanical, no-CHR Dry July items (#152) bundled per the umbrella's sanctioned micro-item PR-bundling rule. Both touch the same
src/cli+src/protocolsself-consistency seam and are unit-tier only.Links
docs/MATRIX.md(protocol grid);commands/AGENTS.md→ "Did you mean?" UX contractimplemented: false, native-api missingtransfercapability #130, Closes Roll out shared unknownFlagError ("did you mean?") to all CLI parsers #111Change type
What changed
#130 — protocol registry ⇄ MATRIX drift (bug)
src/protocols/index.tshad drifted fromdocs/MATRIX.md:native-apitransfertransferaddedtransfer / native-api=CHR-passedsshimplemented: falseimplemented: trueCHR-passedmndpimplemented: falseimplemented: truediscover / mndp=CHR-passedRecurrence guard:
test/unit/protocol-registry.test.tsparses the MATRIX grid and asserts everyCHR-passedcell implies its protocol isimplemented: trueand lists the mapped capability — the registry can never silently under-claim against MATRIX again (MATRIX stays the only status surface; the test derives from it). The module header states the contract.#111 — roll out shared
unknownFlagError("did you mean?")PR #110 added
unknownFlagErrorand wired it intoapi.retrieveandexecutewere already converted before this PR (src/cli/retrieve.ts:376,src/cli/execute.ts:257); this PR finishes the remaining six parsers —transfer,terminal,btest(client + server),devices,discover,mcp— so all parsers give the same Levenshtein closest-match + alias guidance with structuredcontext: { flag, suggestions }. btest's hints are scoped to the active client/server subcommand (review follow-up). Per-parser near-miss coverage added incli-errors.test.tsvia in-processrunCli.Notes
bun run lint && bun run test && bun run build— 965 unit tests pass, build clean, cspell/typecheck green. All CI checks (incl. CodeQL, CHR smoke, CodeRabbit) green..implemented(resolveViainretrieve.ts) is reached only after aretrieve-capability check thatssh/mndpnever pass;snmp(the soleretrieve-capable unimplemented protocol) is untouched. native-apitransferis descriptive (transfer/native-api is alreadyCHR-passed, so no capability gate enforced it). Unit-tier only; no CHR integration run required (both issues' Verification sections specify unit-tier).🤖 Generated with Claude Code