docs: describe every CLI argument and MCP property, with tests that keep them described (#39) - #49
docs: describe every CLI argument and MCP property, with tests that keep them described (#39)#49vlsi wants to merge 8 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
265c111 to
1a2ff3d
Compare
vlsi
changed the title
68a9e42 to
c33d93d
Compare
Great. One finding, and it's the interesting kind: the same defect, in a file this PR edits. Verified rather than assumedThe premise is real. Diffing The guard test genuinely bites. Deleted the "62 arguments across sixteen subcommands" is exact. Reproduced it by running the test at commit 1: 62 entries, 16 distinct subcommands. The descriptions are read off the code, as claimed. Spot-checked the ones that assert something a reader couldn't guess:
And "the only non-comment line the diff adds to Finding: the MCP surface has the same defect, and the new test can't see it
Those are the same flags commit 2 just documented on the CLI side — the The structural half matters more than the count. Same shape as the CLI test, over Not a finding
|
872a50e to
3a2df7c
Compare
|
Thanks — the MCP finding was right, and it turned out to have a second half. Your finding. All 21 blank properties now have descriptions, and each The second half. Three of the six The guard also demands Also in this push. History is restructured into five independent commits, each building and testing on its own — the earlier fix-up commits are folded into what they corrected. Rebased onto current Split out. The Not addressed, noted for whoever wants them: |
vlsi
changed the title
|
Reviewed the whole thing, @vlsi — this is a clean PR and I like the shape of it: documenting by hand would have left the next blank description to whoever happened to run
Everything else held up under direct testing:
Tests green here — Thanks for splitting the |
…eroxy#39) Issue aeroxy#39 asked for the `--no-*` flags to apply to `--json`, which they now do. Nothing outside the source said so: the five flags carried no clap doc comment at all, so `map --help` and `digest --help` printed them with an empty description, and neither the wiki nor the MCP tool schema mentioned that a projection reaches the payload. `--no-docs` and `--no-lines` name the payload keys they remove, since `docs_inside` and the byte offsets do not follow from the flag names; the other three do not, because "hide private declarations" is the whole story. `--json` names the `projected` object a stripped payload carries, and `--max-members` the `truncated` / `dropped_members` pair reporting its cut — the marker half of aeroxy#32, previously documented for the text renderer only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The aeroxy#39 flags were not the only ones shipping with an empty `--help` description; they were the ones someone noticed. 62 more across sixteen subcommands had no clap doc comment: every positional of `show`, `implements`, `deps`, `reverse-deps`, `cycles`, and `graph`, the whole flag set of `install` / `uninstall` / `status` / `hook`, and the `--json` / `--compact` / `--rebuild` triple on nine commands that had documented it elsewhere. Repeated flags reuse the wording the file already used for them, so the same flag reads the same way on every subcommand. Descriptions come from the code, not from the flag names: `--always` and `--min-lines` from `hook::decide`, `--force` from the two conflict branches in `installers::common` — which fail the install rather than skipping it, and only one of which carries a diff — and `--global` from `resolve_scope`, where it is the default rather than a distinct mode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Documenting the arguments by hand leaves the next empty description to whoever happens to run `--help`. This asks clap instead: walk the subcommand tree from `Cli::command()` and assert every argument has help text, and every subcommand an about line. Both walks recurse, so a nested subcommand is held to the rule its parent is. Whitespace-only counts as missing — clap prints the line either way and the reader learns nothing. A failure names the subcommand path and the argument, so the fix is to write the doc comment, not to extend a list here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The CLI half of this branch documented the arguments an agent reaches through `--help`. The MCP half reaches the same operations through `tools/list`, and 21 of its 109 input properties carried no description: `json` on thirteen tools, `rebuild` on six, and `include_private` / `include_fields` on digest. The two digest ones sat as untouched context lines in this branch's own hunk, two lines above a description it rewrote. Each `json` now names the schema its tool returns, read off the render path rather than assumed from the pattern: `show` is `ast-bro.show.v2`, `index` is `ast-bro.index-stats.v1`. Three of the six `rebuild` properties are removed instead of described. `CallersArgs`, `CalleesArgs`, and `TraceArgs` have no such field, and nothing sets `deny_unknown_fields`, so serde drops the key and `load_calls_graph` calls `get_or_init` regardless: `callers` with `rebuild: true` returns normal rows and no error. Describing it would have been worse than the blank it replaced — a blank gives an agent no grounds to believe anything. `rebuild` now appears only on the five tools that read it, matching `impact` and `context`, which never advertised it. `no_fields` and `include_fields` name every kind they act on, on both surfaces. `_map_eligible` matches `Field | Property | Event | Indexer`, so "field declarations" understated it: an agent mapping a C# or Kotlin file with `no_fields: true` loses every property and concludes the type has none — the silent-wrong-answer shape this branch is out to remove. The CLI's `--include-fields` said the same and is corrected with them, so the pair describes one set in one wording. An agent reading the schema is the audience least able to go and find the answer somewhere else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sibling CLI test walks `Cli::command()`, so it can never reach `src/mcp/tools.rs` — a hand-written JSON literal with no guard of any kind. Two surfaces documented separately need guarding separately, or the next blank description waits for whoever calls `tools/list`. Same shape as the CLI test, over `tools::list()`: every tool and every input property carries a non-blank description, and a failure names `<tool>.<property>`. `inputSchema.properties` is demanded rather than probed. `as_object()` answers `None` for a key that is absent and for one that is misspelled alike, so accepting `None` as "this tool takes no arguments" would let a typo drop a whole tool's properties past the check that exists to police them; a tool that genuinely takes none says so with an empty object. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3a2df7c to
06d04fe
Compare
|
Fixed, and it was in three places rather than two.
Folded into the MCP commit rather than appended, so the history stays five independent commits with no fix-up on top. Rebased onto
Checked that no hardcoded default crept back in during the resolution: 52 Live |
|
@vlsi thanks — the defaults centralisation is a real improvement over what I asked for, and I'd take I got the
|
The digest path tested `kind == Field` while the map path tested
`Field | Property | Event | Indexer`, so the same flag dropped different
members depending on which renderer answered:
$ ast-bro digest W.cs # C# type
Name [property] Changed [event] Go()
$ ast-bro digest W.cs --json # same flags, same file
Go
Reported by the maintainer on aeroxy#49, correcting his own earlier finding:
this branch had documented `--no-fields` as covering all four kinds,
which was true of `map --detail full` and false of `digest`. Rather than
narrow the wording to the weaker of the two behaviours, the two now share
one predicate.
`_member_visible` takes the two flags as plain booleans instead of an
options struct, which is what lets `MapOptions` and `DigestOptions` reach
the same answer — carrying the same pair of fields in separate structs is
how they came to disagree.
This changes what `digest` prints: a C# property or event, a Python
`@property`, are field-like, so the preset hides them and
`--include-fields` brings them back. README and the agent skill say so,
since the flag descriptions alone reach nobody reading either. Kotlin is
not among the examples on purpose — no `val` or `var` spelling produces a
field or property at all, so citing one would send a reader looking for
output that never appears.
Covered on all three routes to the predicate — text at each `--detail`
level, JSON, and MCP, the last in its own test because its option
plumbing is separate. `Indexer` is in the predicate but absent from the
fixtures: `csharp.rs` maps `indexer_declaration`, yet a C# `this[int i]`
yields no declaration at all, so a case would assert nothing.
The Python decorator test moves to `--include-fields`: a `@property` is
a field-like member, and that test is about the decorator reaching the
rendered modifier rather than about the projection.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both guards rejected correct code: every integer literal in an
`impl Default`, so `Self { hits: 0 }` on a counter was refused, and every
field whose name reads a constant elsewhere, so
`DepOptions { max_depth: 1, ..Default::default() }` — a deliberate
departure — was told to move into `defaults.rs`, which would be exactly
wrong. Both reported by the maintainer on aeroxy#49, and both reproduced by
injecting the shapes, since no committed line has ever tripped either
guard.
The first attempt inferred intent from the source text: exempt every
zero, and track brace frames to find `..Default::default()`. Cross-review
took both apart. The zero rule reopened the hole the guard exists for.
The brace scan counted braces inside comments and strings, so replaying
it over `src/` showed 11 files whose frame stack never balances and 24
lines already exempted blind, while the two `..Type::default()` spellings
in the tree went unrecognised. Neither inference was exercised by any
line in the tree, which is why only a replay could show it.
So the guards stop guessing and ask. A line that is not making the
mistake says why:
Self { hits: 0 } // defaults-ok: a counter's starting point
The reason is required, and the marker counts only in a comment the
compiler would see — a *value* spelling `"// defaults-ok: x"` exempts
nothing, because the comment split shares its string handling with the
rest of this module rather than scanning raw text.
The verdict is asserted as a composition, not predicate by predicate.
That distinction is what the second review round turned on: `is_exempt`
had a passing test while being unreachable, because a trailing comment
left the value unparseable and `is_literal_field` answered first — so
every commented line was silently exempt, `// TODO` as much as a stated
reason. `is_unmarked_literal` is what the guards call and what the table
covers, and the table counts the verdicts it observed rather than the
expectations it declared, so the coverage number measures the code
instead of the fixture.
Numbers are read the way this tree writes them: `budget: 8_000` is a
literal here, as `src/calls/trace.rs` and `src/lib.rs` already spell
them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Cycles.path`, `Graph.path` and `Callees.path` each named their default
in prose beside a `default_value` that already prints it, so `--help`
rendered the value twice:
[PATH] Repository root to scan (default: ".") [default: .]
That is the drift this branch closes everywhere else, reintroduced in
the docs half — spotted by the maintainer on aeroxy#49, who noted the existing
guard could not see it: it scans MCP `"description"` strings and never
clap doc comments. `impact --mode` said `(default)` with no value, which
this guard does not flag and which was removed here anyway, since clap
appends `[default: all]` beside it.
The rule differs between the two surfaces, so this is a second guard
rather than a widened one. An MCP description is a runtime `String` and
the repair is to interpolate the constant; a clap doc comment is an
attribute fixed at compile time and cannot interpolate anything, so
there the rule is to say nothing and let clap say it once.
Six shapes cost four rounds of cross-review, and each has a case:
- the doc block is read forwards, so a multi-line `#[arg(` cannot hide
a `default_value` that is not its first argument;
- an ordinary comment between the docs and the attribute does not end
the declaration;
- only clap's own attribute counts, matched at a word boundary, so
neither `#[my_macro(default_value = 5)]` nor `#[my_arg(…)]` is
mistaken for it while `#[cfg_attr(unix, arg(…))]` still is;
- commented-out text inside another attribute cannot forge that match,
because the compiler drops it before it means anything;
- a `[` inside a string does not latch the attribute scan open;
- and if some spelling this does not model latches it anyway, the scan
asserts rather than going quiet — a guard that checks nothing must
not report success, so the alarm has its own test.
A digit is a stated value only when the sentence ends on it, in the
spellings a default is actually written in: `8_000`, `200ms` and `4KB`
flag, while "the default 64-bit mode" and "the default 2 levels up" are
prose and pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Cycles.path`, `Graph.path` and `Callees.path` each named their default
in prose beside a `default_value` that already prints it, so `--help`
rendered the value twice:
[PATH] Repository root to scan (default: ".") [default: .]
That is the drift this branch closes everywhere else, reintroduced in
the docs half — spotted by the maintainer on aeroxy#49, who noted the existing
guard could not see it: it scans MCP `"description"` strings and never
clap doc comments. `impact --mode` said `(default)` with no value, which
this guard does not flag and which was removed here anyway, since clap
appends `[default: all]` beside it.
The rule differs between the two surfaces, so this is a second guard
rather than a widened one. An MCP description is a runtime `String` and
the repair is to interpolate the constant; a clap doc comment is an
attribute fixed at compile time and cannot interpolate anything, so
there the rule is to say nothing and let clap say it once.
Six shapes cost four rounds of cross-review, and each has a case:
- the doc block is read forwards, so a multi-line `#[arg(` cannot hide
a `default_value` that is not its first argument;
- an ordinary comment between the docs and the attribute does not end
the declaration;
- only clap's own attribute counts, matched at a word boundary, so
neither `#[my_macro(default_value = 5)]` nor `#[my_arg(…)]` is
mistaken for it while `#[cfg_attr(unix, arg(…))]` still is;
- commented-out text inside another attribute cannot forge that match,
because the compiler drops it before it means anything;
- a `[` inside a string does not latch the attribute scan open;
- and if some spelling this does not model latches it anyway, the scan
asserts rather than going quiet — a guard that checks nothing must
not report success, so the alarm has its own test.
A digit is a stated value only when the sentence ends on it, in the
spellings a default is actually written in: `8_000`, `200ms` and `4KB`
flag, while "the default 64-bit mode" and "the default 2 levels up" are
prose and pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
f21a88d to
8b4442a
Compare
|
All four points addressed, plus the You were right that I got
|
| Round | Found in my fix |
|---|---|
| 1 | both exemptions were dead code; the brace scan was already wrong on 11 files |
| 2 | the marker was unreachable — a trailing comment left the value unparseable, so // TODO bought the same silence as a stated reason |
| 3 | contains("arg(") matched my_arg(; budget: 8_000 walked past both guards; assert!(rejected >= 7) summed the table's expectations, so it could not fail |
| 4 | commented-out text could forge a clap attribute; the new bound had no test |
The round-2 one is the one worth naming: is_exempt had a green test while never being called. Testing the links instead of the chain is what hid it, so the guards now call one composed is_unmarked_literal, and the table asserts the verdict.
After that, @vlsi asked whether hard-coding // and /* was safe for other languages. It is scoped correctly — sources() yields *.rs and these helpers live inside #[cfg(test)], so the multi-language adapters in src/adapters/ share nothing with them — but checking it turned up a real gap in Rust's own syntax: block comments nest, and I matched the first */, so /* a /* b */ arg(default_value = 5) */ put the tail back outside the comment and forged the match again. Fixed by matching depth.
Every fix above is checked by reverting it and watching the test fail — including the mutation the reviewer named for the scan bound (moving the counter reset above the increment).
Verification
cargo test: 26 binaries green, on each of the eight commits in isolation. cargo clippy --all-targets clean apart from the pre-existing chunker.rs warning.
install --all --local writing into absent CLIs is #64 — reproduced there with the select_all detection bypass, untouched here as you offered.
|
@vlsi three commits for three findings, and #64 split out — thanks, that's exactly the shape I wanted. Confirmed fixed on the PR build — C# text and JSON now agree: $ ast-bro digest W.cs # property + event both gone
class W L1-6
Go()
$ ast-bro digest W.cs --json # kinds: class, methodTwo left, both reproduced.
|
2 participants
Why
The projection work for #39 landed in e50056e and works, but nothing outside the source said so. The five
--no-*flags carried no clap doc comment at all, somap --helpprinted them with an empty description:A caller hitting the oversized-payload problem the issue describes had no way to find the lever that fixes it.
Documenting those five by hand leaves the next empty description to whoever happens to run
--help, so this asks the code instead — and five was not the number. A guard overCli::command()found 62 more; the maintainer's review then pointed out that guard can never reachsrc/mcp/tools.rs, where 21 of 109 properties were blank. Two surfaces, documented separately, so guarded separately.What
Five commits, each building and testing on its own.
1.
docs:the #39 flags. Each--no-*gets a description.--no-docsand--no-linesadditionally name the payload keys they remove, sincedocs_insideand the byte offsets do not follow from the flag names; the other three don't, because "hide private declarations" is the whole story.--jsonnames theprojectedobject a stripped payload carries,--max-membersthetruncated/dropped_memberspair — the marker half of #32, previously documented for the text renderer only. Same facts inREADME.md,wiki/architecture.md,skills/ast-bro/SKILL.md, and the MCP schemas.2.
docs:the other 62 CLI arguments, across sixteen subcommands: every positional ofshow,implements,deps,reverse-deps,cycles,graph, the whole flag set ofinstall/uninstall/status/hook, and the--json/--compact/--rebuildtriple on nine commands that had documented it elsewhere. Repeated flags reuse the wording already in the file. Descriptions come from the code, not the flag names:--alwaysand--min-linesfromhook::decide,--forcefrom the two conflict branches ininstallers::common(which fail the install rather than skipping it, and only one of which carries a diff),--globalfromresolve_scope, where it is the default rather than a distinct mode.3.
test:the CLI guard. Walks the subcommand tree fromCli::command(); every argument needs help text, every subcommand an about line. Both walks recurse. Whitespace-only counts as missing.4.
docs:every MCP property, and three removed. The 21 blank properties are described, and eachjsonnames the schema its tool actually returns rather than the.v1the pattern suggests —showisast-bro.show.v2,indexisast-bro.index-stats.v1.Three of the six
rebuildproperties are removed instead of described.CallersArgs/CalleesArgs/TraceArgsdeclare no such field and nothing setsdeny_unknown_fields, so serde drops the key andload_calls_graphcallsget_or_initregardless:tools/call callers {rebuild: true}returnsisError: falseand normal rows. Describing an inert flag is worse than the blank it replaced — a blank gives an agent no grounds to believe anything.rebuildnow appears only on the five tools that read it, matchingimpactandcontext, which never advertised it. A client still sending the key gets the same result as before; only the false promise is gone.5.
test:the MCP guard. Same shape overtools::list().inputSchema.propertiesis demanded rather than probed:as_object()answersNonefor an absent key and a misspelled one alike, soif let Somewould let a typo drop a whole tool's properties past the check meant to police them.No behavior change beyond that one schema removal: the only non-comment lines added to
src/lib.rsare the test module.How to verify
cargo test --lib every_Removing any one doc comment turns it red and names the argument:
Live
tools/listis 19 tools, 106 properties, 0 undescribed, withrebuildondeps/reverse_deps/cycles/graph/indexonly. Full suite: 26 test binaries green, on each of the five commits in isolation.Split out
The
showdefect found while verifying — a markdown heading containing/is rejected as a mistyped path and never searched — is #55, with a minimal reproducer. Pre-existing, untouched here.Not addressed, noted for whoever wants them:
run'sjsonis the one of nineteen not naming its schema at the property level (the tool description does), andimpact/contextexpose norebuildover MCP despite having--rebuildon the CLI.🤖 Generated with Claude Code