feat(spec): allow a mount at the top level - #806
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRoot-level mounts are preserved in KDL specifications and support configurable discovery timing. Parsing resolves mounts lazily for unmatched non-flag words and eagerly for completion. Default-subcommand precedence, mount overrides, lookup-cache invalidation, and serialization are tested. ChangesRoot mount discovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SpecParser
participant CommandParser
participant RootMount
SpecParser->>CommandParser: provide root mount definitions
CommandParser->>CommandParser: check declared commands and flags
CommandParser->>RootMount: resolve mount for unmatched non-flag word
RootMount-->>CommandParser: return discovered command and flags
CommandParser-->>CommandParser: continue parsing with mounted command
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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
🤖 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 `@lib/src/parse.rs`:
- Around line 634-635: Update the default-subcommand descent after assigning
out.cmd to set mounts_resolved = true, preventing later unmatched words from
mounting the same subcommand again. Add coverage for a default subcommand with a
mount and positional argument, verifying the mount runs once and its output is
not duplicated.
- Around line 605-617: Update the root-mount resolution condition in the parsing
flow around mounts_resolved so it only resolves when input[idx] is a non-flag
word, preserving lazy command discovery for --help and unknown flags. Add a
regression test configuring mount run="exit 1" and verifying that parsing --help
does not execute the mount and still handles help normally.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ed916d45-0553-4760-a93b-e4086f568f63
📒 Files selected for processing (4)
docs/spec/reference/cmd.mdlib/src/parse.rslib/src/spec/cmd.rslib/src/spec/mod.rs
Greptile SummaryThe PR adds top-level mount parsing, serialization, and lazy command discovery while preserving eager discovery for completions.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (5): Last reviewed commit: "fix(parse): keep completion and executio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The spec accepted `mount` only inside a `cmd` block, so a CLI whose own commands are discovered rather than declared — one whose subcommands come from plugins — had no way to say so. The root is a command like any other, and now reads, writes, and resolves a mount like one. Resolved lazily: the root's mount runs only when a word matches nothing already declared. Eager resolution would spawn a process on every invocation, since nothing ever descends into the root, so declaring the commands you know about keeps the common path free. A test proves it by mounting `exit 1` and parsing a declared command anyway. Also fixes what would have made this silently not work. `find_subcommand` memoizes its lookup into a OnceLock, and `merge` adds subcommands without clearing it — so a name that arrived by mounting was not findable if anything had looked one up first. That was latent for subcommand mounts too, which worked only because mounting happened to precede the first lookup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Lazy discovery keyed off "this word matches no subcommand", and a flag matches no subcommand either — so `ex --help` spawned the mount command, which is both a side effect nobody asked for and the opposite of what the docs promise. Only a word that could name a command triggers discovery now. The default-subcommand path also descended without recording that it had already run the new command's mounts, so a later unmatched word could run them again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both findings fixed — and the Major one was a good catch that my own docs contradicted. A flag was triggering discovery. Lazy resolution keyed off "this word matches no subcommand", and The default-subcommand path descended without recording that it had already run the new command's mounts, so a later unmatched word could run them twice. Both are the same underlying mistake on my part: I keyed the decision on what didn't match rather than on what the token is. AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable. |
Lazy resolution broke the case a root mount exists for. A completion asks about `mycli <tab>`, where there is no word to trigger discovery with, so the mounted commands were never offered — the one thing the feature is for. Timing now depends on who is asking: a completion resolves up front because it has to offer every command, and a parse waits until a word matches nothing declared because it knows what it was given. Same split as the missing-value check: `parse_partial` is the lenient, complete view, `parse` is the strict, fast one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Another round, and the best finding yet: my laziness broke the case this feature exists for. A completion asks about Timing now depends on who is asking, which is the same split as the missing-value check in #807:
Flags never trigger it either way. Test: One ordering question I'd rather you decideA spec with both a root mount and a Nothing has both today, so it is theoretical — but the two orderings have real consequences:
I lean toward default-first, since a AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable. |
| *last = mounted.clone(); | ||
| } | ||
| out.cmd = mounted; | ||
| } |
There was a problem hiding this comment.
Eager mount before flag scan
Medium Severity
Root mounts now resolve at the start of every parse_partial call, before phase 1 scans tokens. Help generation uses parse_partial, so invocations like mycli --help can still spawn the mount command even though execution via parse skips discovery for flags and the spec docs say --help does not run the mount.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9eb998b. Configure here.
Instruction countsNothing was compared, and so nothing was gated. No series appears on both sides: either the base has no measurements recorded, or the two were measured on different runner classes, which are deliberately not comparable — counts shift between machine types by more than a real regression does. New, nothing to compare against: Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run. Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes.
|
Resolving a mount runs a process, so a spec with both a root mount and a `default_subcommand` would have spawned it for every word that is not a declared command — a subprocess per task invocation, for a CLI shaped like a task runner. The default now wins, since it already says what an unmatched word means and costs nothing to consult. A mount that should shadow it says so with `overrides_default=#true` and pays for discovery. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Made the ordering configurable, defaulting the way you leaned. You are right that mounting has a real cost — it runs a process. So a A mount that should shadow the fallback opts in: mount run="mycli plugin-commands" overrides_default=#trueTwo tests, one per direction, both proving it by side effect rather than by timing: the default-wins test mounts So the full timing story for a root mount is now:
AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1e79a08. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@lib/src/parse.rs`:
- Around line 621-625: Update parse_partial’s eager-mount path to collect the
declared root global flags before mounting and pass
mount_prefix_words(&prefix_flags) to mounted.mount instead of &[]. Preserve the
existing Phase 1 flag collection and add a completion regression test verifying
eager discovery selects commands for the requested global-flag context.
- Around line 642-653: Update the pre-default discovery logic around
default_catches_it and SpecCommand::mount so that, when a default_subcommand
exists and any root mount overrides it, only mounts with overrides_default are
executed during this attempt. Prevent non-overriding mounts from contributing
discovered commands that could shadow the default, while preserving current
behavior when no overriding mount is present. Add a test covering one overriding
mount and one non-overriding mount.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: cd94af57-0c19-4c32-82fe-a9aae6232bbe
📒 Files selected for processing (3)
docs/spec/reference/cmd.mdlib/src/parse.rslib/src/spec/mount.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/spec/reference/cmd.md
| if mount_timing == MountTiming::Eager && !out.cmd.mounts.is_empty() { | ||
| mounts_resolved = true; | ||
| let mut mounted = out.cmd.clone(); | ||
| mounted.mount(&[])?; | ||
| merge_subcommand_flags(&mut out.available_flags, gather_flags(&mounted), false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Forward root global flags during eager discovery.
parse_partial runs the root mount with &[] before Phase 1 collects prefix_flags. Normal lazy discovery forwards mount_prefix_words(&prefix_flags) at Line 652.
If a root mount uses a declared global flag to select its emitted commands, completion returns commands for the default context instead of the selected context. Collect and forward declared root global flags before eager mounting. Add a completion regression test.
🤖 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 `@lib/src/parse.rs` around lines 621 - 625, Update parse_partial’s eager-mount
path to collect the declared root global flags before mounting and pass
mount_prefix_words(&prefix_flags) to mounted.mount instead of &[]. Preserve the
existing Phase 1 flag collection and add a completion regression test verifying
eager discovery selects commands for the requested global-flag context.
| let default_catches_it = spec.default_subcommand.is_some() | ||
| && !out.cmd.mounts.iter().any(|m| m.overrides_default); | ||
| if !mounts_resolved | ||
| && !out.cmd.mounts.is_empty() | ||
| && !default_catches_it | ||
| && !input[idx].starts_with('-') | ||
| && out.cmd.find_subcommand(&input[idx]).is_none() | ||
| { | ||
| mounts_resolved = true; | ||
| let mut mounted = out.cmd.clone(); | ||
| mounted.mount(&mount_prefix_words(&prefix_flags))?; | ||
| merge_subcommand_flags(&mut out.available_flags, gather_flags(&mounted), false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run only overriding mounts before default fallback.
any(|m| m.overrides_default) enables discovery for all root mounts. SpecCommand::mount then executes every mount. A non-overriding mount can therefore run and let its discovered command shadow default_subcommand when another mount enables overrides_default.
When a default exists, execute only mounts with overrides_default during the pre-default discovery attempt. Add a test with one overriding mount and one non-overriding mount.
Proposed fix
let mut mounted = out.cmd.clone();
+if spec.default_subcommand.is_some() {
+ mounted.mounts.retain(|mount| mount.overrides_default);
+}
mounted.mount(&mount_prefix_words(&prefix_flags))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let default_catches_it = spec.default_subcommand.is_some() | |
| && !out.cmd.mounts.iter().any(|m| m.overrides_default); | |
| if !mounts_resolved | |
| && !out.cmd.mounts.is_empty() | |
| && !default_catches_it | |
| && !input[idx].starts_with('-') | |
| && out.cmd.find_subcommand(&input[idx]).is_none() | |
| { | |
| mounts_resolved = true; | |
| let mut mounted = out.cmd.clone(); | |
| mounted.mount(&mount_prefix_words(&prefix_flags))?; | |
| merge_subcommand_flags(&mut out.available_flags, gather_flags(&mounted), false); | |
| let default_catches_it = spec.default_subcommand.is_some() | |
| && !out.cmd.mounts.iter().any(|m| m.overrides_default); | |
| if !mounts_resolved | |
| && !out.cmd.mounts.is_empty() | |
| && !default_catches_it | |
| && !input[idx].starts_with('-') | |
| && out.cmd.find_subcommand(&input[idx]).is_none() | |
| { | |
| mounts_resolved = true; | |
| let mut mounted = out.cmd.clone(); | |
| if spec.default_subcommand.is_some() { | |
| mounted.mounts.retain(|mount| mount.overrides_default); | |
| } | |
| mounted.mount(&mount_prefix_words(&prefix_flags))?; | |
| merge_subcommand_flags(&mut out.available_flags, gather_flags(&mounted), false); |
🤖 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 `@lib/src/parse.rs` around lines 642 - 653, Update the pre-default discovery
logic around default_catches_it and SpecCommand::mount so that, when a
default_subcommand exists and any root mount overrides it, only mounts with
overrides_default are executed during this attempt. Prevent non-overriding
mounts from contributing discovered commands that could shadow the default,
while preserving current behavior when no overriding mount is present. Add a
test covering one overriding mount and one non-overriding mount.
With a `default_subcommand` and a root mount that does not say `overrides_default`, a parse skipped discovery while a completion still ran it — so the shell offered a discovered command that running it would hand to the default subcommand instead. Offering what execution will not route is worse than offering nothing. The gate now applies to both paths, which settles what the setting means: a root mount under a default subcommand contributes nothing anywhere unless it asks to outrank it. Also corrects a claim I made in this PR's own docs. Rendering help goes through `parse_partial`, so `--help` does resolve a root mount — and should, since help lists every command. "Flags never trigger it" was true of a parse and not of help, so the page now says which is which. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Two more findings fixed, one of which was a real UX bug I had not seen. Completion and execution disagreed. With a That settles what the setting means, which is cleaner than what I had: a root mount under a default subcommand contributes nothing anywhere unless it asks to outrank it. One test checks both halves at once by mounting I also had to correct this PR's own documentation. Rendering help goes through
The remaining threads above are re-posts from before these commits — the bots re-review the cumulative diff, so line numbers shift and fixed findings reappear. AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable. |
`main` is red because of a tripwire I wrote. #806 and #809 each added an ordinary corpus vector; each was green alone, because each saw only its own increment of a hardcoded count, and together they made 65 where the constant said 64. Counting was the mistake, not the number. A count asserts something nobody can check by reading it, and it collides whenever two changes touch the data it counts. What the assertion was actually for — noticing when a vector changes sides — is better served by a snapshot of which vectors are exempt and why: it fails as a reviewable diff naming the vector that moved, adding an ordinary vector does not touch it at all, and the list documents the post-binding boundary that the number obscured. The file lost two magic numbers, not one. `error_expectations_are_reachable` asserted that at least six error classes were exercised, which the per-vector checks already cover. Also drops `no_vector_has_an_unloadable_spec`, which duplicated `specs_are_valid` in reference.rs, and `out_of_scope_vectors_say_why`, whose reasons are now visible in the snapshot. Corpus well-formedness is checked in one place; this file is about the parser. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The mount page describes only the behavior of the first commit in #806 and never picked up what the later ones changed: it still claims flags never trigger discovery, which is untrue of completions and help, and `overrides_default` shipped with no documentation at all. My fault, and worth writing down how: the edits were made by string replacement against text prettier had already rewrapped and turned `*own*` into `_own_`, so both replacements silently matched nothing. I said in the pull request that the page had been corrected, and it had not. Diff verified this time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
) Cleaning up after myself. The mount documentation on `main` describes only the first commit of #806 and missed everything the later ones changed: - It claims **flags never trigger discovery**, which is untrue of completions and of rendering help — both resolve the mount up front, because both need the whole command list. - **`overrides_default` is undocumented entirely**, despite shipping. - Nothing says the default-subcommand gate applies to completions as well as parses, which is the part with a user-visible consequence. ## How it happened, since it is worth knowing Both docs edits in that PR were string replacements against text prettier had already reformatted — rewrapped lines, and `*own*` turned into `_own_`. The replacements matched nothing and silently did nothing, and I asserted in a PR comment that the page had been corrected. It had not. The lesson is mechanical: a `.replace()` that no-ops leaves no trace, so an edit made that way needs its diff checked rather than its exit code. This one is verified — `git diff --stat` shows 25 insertions, and the rendered diff is in the commit. ## What the page says now A table for when the root's mount runs, since "lazily" was never the whole truth: | asking | when it runs | | --- | --- | | a completion, or rendering help | up front — both need the whole command list | | a parse | only when a word matches nothing already declared | | a parse, for a flag | never | Plus `overrides_default`, and why it governs completions too: a completion offering a command that running would hand to the default subcommand instead is worse than not offering it. *AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable.* <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only change to the mount reference; no runtime or security-sensitive code is modified. > > **Overview** > Corrects the root `mount` docs that previously claimed discovery never runs for flags and omitted later behavior. > > Adds a table for **when a root mount runs**: up front for completions/help, lazily for unmatched parse words, and never for flags during parse. Also documents `overrides_default`, including that `default_subcommand` outranks discovery for both parses and completions unless a mount opts in. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f942f04. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
You asked for this in passing — it makes sense that users should be able to mount at the top level — and it turned out not to be a big issue.
What it does
mountwas accepted only inside acmdblock, so a CLI whose own commands are discovered rather than declared could not say so:The root is a command like any other, and now parses, serializes, and resolves a mount like one.
Resolved lazily, which matters here
A subcommand's mounts run when the parser descends into it. Nothing ever descends into the root, so an eager root mount would spawn a process on every invocation — including
mycli install, where the answer is already known.So the root's mount runs only when a word matches nothing declared. Declaring the commands you know about keeps the common path free, and only
mycli something-from-a-pluginpays for discovery.The test for that is my favourite part: it mounts
exit 1and then parses a declared command successfully. If resolution were eager, the parse would fail — no filesystem, no timing, no subprocess to observe.A latent bug this would have tripped over
find_subcommandmemoizes its lookup into aOnceLock, andmergeadds subcommands without clearing it. So a name that arrived by mounting was invisible if anything had already looked one up on that command.That was latent for subcommand mounts too — they work today only because mounting happens to precede the first lookup on the command being mounted into. Fixed in
merge, where the subcommands actually change, so it cannot depend on call order again.Three tests: a root mount discovering a subcommand, the laziness proof, and a write-and-read-back round trip.
AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable.
Note
Medium Risk
Changes core argument parsing and when mount subprocesses run; behavior is covered by new tests but affects every invocation path that hits an unknown subcommand word at the root.
Overview
Top-level
mountis now valid in usage specs (parse, round-trip serialize, and docs), so CLIs can declare static commands and discover the rest—e.g. plugin subcommands viamount run="mycli plugin-commands".Lazy vs eager discovery: Full parsing uses
MountTiming::WhenAWordIsUnknown, so the root mount runs only when the next token is a non-flag word that is not already a declared subcommand—avoiding a discovery subprocess on every invocation (mycli install,mycli --help).parse_partialstaysEagerso completions likemycli <tab>see mounted commands with nothing typed yet.Bug fix:
SpecCommand::mergenow resetssubcommand_lookupso subcommands added by mounting remain findable after any prior lookup memoization.Reviewed by Cursor Bugbot for commit 9eb998b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes