Conversation
…#31-#37) Resolves the seven issues filed against 3.0.0, plus review follow-ups: - #31 callers under-reports on receiver shadowing: pass A now respects explicit receivers (self-like keywords only; type-qualified calls bind to the scoped qn), self-like binding prefers the caller's own scope, and edges the resolver could not attribute are surfaced in callers output (text section + JSON unattributed/_total/_truncated/_hidden) instead of silently dropped. PHP scope keywords are case-folded. - #33/#36 one error contract: stdout carries results only; exit 0 = query ran (empty answers included), 2 = query could not run as asked, 1 = internal failure; rejections print to stderr and emit an ast-bro.error.v1 envelope under --json (new src/cli_error.rs). Unknown flags exit 2 with a cross-subcommand hint instead of help-on-stdout. Empty/unresolved path lists are rejected before walking. - #32 truncation is never silent: callers/callees/reverse-deps/impact report true totals when --limit trims display, JSON carries total/truncated (+ frontier_truncated for depth-stopped walks, noted on stderr only when --depth was raised), map --max-members prints +N more and JSON dropped_members agrees with the text renderer. - #37 map and digest are one command: --detail names|signatures|full, one-polarity visibility flags, scope flags on both; digest is an alias for map --preset digest with explicit flags overriding the preset. - #35 map on a large directory hints at the digest preset (stderr, never a silent redirect). - #34 the Read-hook substitution leads with "nothing failed" so the map is read as the answer, not an error. MCP mirrors the CLI: trace rejections render a valid trace.v1 doc with found:false, callers folds hidden-unattributed counts into the response body, and the unattributed section shares the --limit budget everywhere. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR standardizes CLI errors and stdout/stderr behavior, adds structured JSON rejection envelopes, improves receiver-aware call-graph resolution and traversal reporting, adds plain-text chunking, and reports totals and truncation across analysis outputs. ChangesCLI and analysis behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
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 |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
PR Summary by QodoUnify CLI contracts and fix resolver and truncation reporting
AI Description
Diagram
High-Level Assessment
Files changed (27)
|
Code Review by Qodo
1. Hidden unattributed total wrong
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/impact.rs (1)
362-379: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftUnbounded reverse BFS per construction site is now quadratic-ish on wide types.
These walks previously stopped at
opts.limit; withusize::MAXeach construction site expands its full reverse-reachable set, and the implementor branch nests that inside a second loop (implementors × constructions × reachable set). For a type constructed in hundreds of places this is a large amount of repeated traversal whose results are then collapsed byconsiderinto one minimum-depth entry per qn.Reinstating a limit would bring back the wrong totals
#32fixes, so the fix is sharing work instead: track qns already resolved at a depth<= candidate depthand skip re-expanding them, or run one multi-source BFS seeded with all construction sources instead of one walk per site.Also applies to: 420-437
🤖 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 `@src/impact.rs` around lines 362 - 379, Optimize the reverse traversal around traverse::callers and its construction-site loop without restoring the opts.limit bound. Share traversal work across construction and implementor sources by tracking qns already resolved at an equal or shallower candidate depth, or by using a single multi-source BFS, while preserving consider’s minimum-depth results and correct totals.
🧹 Nitpick comments (6)
src/lib.rs (1)
845-861: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated envelope construction — add a JSON-only emit to
CliError.The
ast-bro.error.v1shape now lives in two places (CliError::emitand here), so a future field addition can silently drift on the parse-error path. Consider exposing aemit_json_only(&self)(orjson_envelope(&self) -> Value) onCliErrorand calling it here.♻️ Sketch
// src/cli_error.rs impl CliError { /// Envelope without the human text (for call sites that already /// printed a rendered message, e.g. clap parse errors). pub fn emit_json_only(&self) { eprintln!("{}", self.envelope()); } }if json_mode { let mut err = crate::cli_error::CliError::new(subcommand, kind, detail); if let Some(h) = hint { err = err.hint(h); } - // Human text already printed via clap above; emit only the envelope. - let mut doc = serde_json::json!({ ... }); - ... - eprintln!("{}", doc); + // Human text already printed via clap above; emit only the envelope. + err.emit_json_only(); }🤖 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 `@src/lib.rs` around lines 845 - 861, Move the JSON envelope construction into a reusable `CliError` method, such as `emit_json_only` backed by the existing envelope builder, and update the `json_mode` parse-error path in `src/lib.rs` to construct the error and call that method. Remove the duplicated `serde_json::json!` field assembly there while preserving the existing hint and JSON-only output behavior.tests/calls_e2e.rs (2)
1906-1941: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConditional assertion can no-op.
If the fixture ever yields
unattributed_total == shown, theunattributed_truncatedcheck is skipped and the test still passes. Assert the expected total (3) explicitly so the truncation flag is always exercised.🤖 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/calls_e2e.rs` around lines 1906 - 1941, Strengthen callers_limit_bounds_the_unattributed_section_too by asserting that unattributed_total equals the fixture’s expected three entries before checking truncation. Then assert unattributed_truncated is true unconditionally, ensuring the test always exercises the capped unattributed output.
1759-1763: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNegative assertion is spacing-dependent and can silently pass.
"Caller::getCtx (Exact)"hard-codes two spaces from the renderer's column padding. Any padding change makes the regression pass vacuously. Assert on JSON instead (target + confidence), as the other tests in this file do for exactly this reason.🤖 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/calls_e2e.rs` around lines 1759 - 1763, Update the assertion in the test around the existing output check to parse and inspect the emitted JSON, asserting the target and confidence for the explicit-receiver call rather than matching the renderer’s spacing-dependent text. Follow the JSON assertion pattern used by the other tests in the file, preserving the requirement that the local homonym is not classified as Exact.src/calls/traverse.rs (1)
166-174: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDepth-cap probe re-clones the edge list.
edges_atclones the whole adjacency vector (graph.forward/reverse.get(qn).cloned()), and the cap branch calls it a second time purely to test emptiness. On wide frontiers withusize::MAXlimits (the new unbounded walks incli.rs/impact.rs) this doubles the clone cost for every capped node. Cheaper: check non-emptiness without materializing, e.g. pass anhas_edgesclosure alongsideedges_at, or hoist the singleedges_at(&cur)result before the depth check and reuse it.🤖 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 `@src/calls/traverse.rs` around lines 166 - 174, The depth-cap branch in the queue traversal loop redundantly clones adjacency data through edges_at(&cur) solely to check whether edges remain. Restructure the traversal around edges_at so each node’s edge list is materialized at most once, reusing that result for the depth-cap frontier_truncated check and subsequent expansion while preserving existing walk behavior.src/core.rs (1)
725-759: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVisibility rule is now expressed twice.
The
visibleclosure re-implements the field/private gates already enforced by the early returns at Lines 681-687 (and again in_filter_decls). If one copy drifts,--max-memberswill count members the renderer never emits (or vice versa). Extracting a singlefn is_visible(d: &Declaration, opts: &MapOptions) -> booland using it in all three places keeps text and JSON caps in agreement.🤖 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 `@src/core.rs` around lines 725 - 759, Extract the field/private visibility predicate from the local visible closure into a shared is_visible(&Declaration, &MapOptions) helper. Replace the early-return checks near the declaration renderer, the _filter_decls logic, and the max-members counting closure with this helper so rendered output and JSON member caps use identical visibility rules.src/calls/cli.rs (1)
103-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
callershas no display-truncation stderr note;calleesdoes.
run_callees(Lines 248-254) prints# note: N callee(s) total; showing M …when--limitcuts the list, butrun_callersonly encodes that in the text header suffix / JSONtruncated. For an agent piping stdout, the two commands now behave differently for the same condition. Consider mirroring the note here.♻️ Suggested addition
let total = hits.len(); if hits.len() > limit { hits.truncate(limit); + eprintln!( + "# note: {} caller(s) total; showing {} (raise --limit to see the rest)", + total, + hits.len() + ); }🤖 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 `@src/calls/cli.rs` around lines 103 - 116, Update run_callers to emit the same stderr display-truncation note as run_callees when --limit reduces hits, using total and the post-truncation hits.len() to report the full and shown counts. Keep the existing frontier_truncated/depth note and header/JSON truncation behavior unchanged.
🤖 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 `@skills/ast-bro/SKILL.md`:
- Line 15: Update the Result caps documentation to state that the impact command
defaults to depth 2, while keeping callers and callees at depth 1. Ensure the
documented defaults match the depth declaration for Commands::Impact.
In `@src/calls/render.rs`:
- Around line 108-123: Update the caller header truncation check in the
rendering function containing total, shown, and header_line_suffixed so the
suffix compares the combined total against the combined shown count, rather than
calling trunc.header_suffix(shown) with mismatched callable-only truncation
data. Also inspect render_callers_json_extended and align its truncated and
total fields with the same type-group-inclusive counting semantics used by the
text output.
In `@src/calls/resolve.rs`:
- Around line 114-119: Update the scoped-receiver lookup in the surrounding
call-resolution logic to fall back to the receiver’s terminal segment when the
full namespace/path-qualified receiver does not match. Preserve the existing
full-receiver match first, then derive the last component from `recv`, build the
corresponding scoped name with `raw.bare_name`, and search `fp.defined` so
qualified receivers retain same-file Exact bindings.
In `@src/lib.rs`:
- Around line 1427-1435: Update the error-kind mapping in the surface error
handling match to map only SurfaceError::NoEntryPoint to PathNotFound; map
SurfaceError::Io alongside SurfaceError::Parse to IndexError, preserving the
existing BadOverride mapping to BadArgument and CliError construction.
In `@src/mcp/tools.rs`:
- Around line 827-834: Update the reverse-dependency flow around
crate::deps::traverse::reverse so traversal remains bounded by a.limit while
still producing the true total count. Change the traversal/API usage to return
the limited page and total count together, then remove the full-closure
materialization and post hoc hits.truncate call while preserving the existing
output behavior.
- Around line 27-33: Update the max_members property in both MCP schemas,
including the digest schema near its corresponding definition, with a minimum
constraint matching its usize deserialization: use 0 unless the implementation
explicitly disallows zero. Keep the existing integer type and descriptions
unchanged.
In `@wiki/calls.md`:
- Around line 164-168: The documentation in wiki/calls.md is out of sync with
the receiver_is_self_like behavior. Update the import-resolution description to
say it applies when the receiver is self-like, including self(), Self, crate,
super, this, and $this, rather than only when there is no explicit receiver;
also update the Pass B suppression list to include this and $this.
---
Outside diff comments:
In `@src/impact.rs`:
- Around line 362-379: Optimize the reverse traversal around traverse::callers
and its construction-site loop without restoring the opts.limit bound. Share
traversal work across construction and implementor sources by tracking qns
already resolved at an equal or shallower candidate depth, or by using a single
multi-source BFS, while preserving consider’s minimum-depth results and correct
totals.
---
Nitpick comments:
In `@src/calls/cli.rs`:
- Around line 103-116: Update run_callers to emit the same stderr
display-truncation note as run_callees when --limit reduces hits, using total
and the post-truncation hits.len() to report the full and shown counts. Keep the
existing frontier_truncated/depth note and header/JSON truncation behavior
unchanged.
In `@src/calls/traverse.rs`:
- Around line 166-174: The depth-cap branch in the queue traversal loop
redundantly clones adjacency data through edges_at(&cur) solely to check whether
edges remain. Restructure the traversal around edges_at so each node’s edge list
is materialized at most once, reusing that result for the depth-cap
frontier_truncated check and subsequent expansion while preserving existing walk
behavior.
In `@src/core.rs`:
- Around line 725-759: Extract the field/private visibility predicate from the
local visible closure into a shared is_visible(&Declaration, &MapOptions)
helper. Replace the early-return checks near the declaration renderer, the
_filter_decls logic, and the max-members counting closure with this helper so
rendered output and JSON member caps use identical visibility rules.
In `@src/lib.rs`:
- Around line 845-861: Move the JSON envelope construction into a reusable
`CliError` method, such as `emit_json_only` backed by the existing envelope
builder, and update the `json_mode` parse-error path in `src/lib.rs` to
construct the error and call that method. Remove the duplicated
`serde_json::json!` field assembly there while preserving the existing hint and
JSON-only output behavior.
In `@tests/calls_e2e.rs`:
- Around line 1906-1941: Strengthen
callers_limit_bounds_the_unattributed_section_too by asserting that
unattributed_total equals the fixture’s expected three entries before checking
truncation. Then assert unattributed_truncated is true unconditionally, ensuring
the test always exercises the capped unattributed output.
- Around line 1759-1763: Update the assertion in the test around the existing
output check to parse and inspect the emitted JSON, asserting the target and
confidence for the explicit-receiver call rather than matching the renderer’s
spacing-dependent text. Follow the JSON assertion pattern used by the other
tests in the file, preserving the requirement that the local homonym is not
classified as Exact.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 78ec2ac9-7e1c-4387-a4b8-8e138ed259fc
📒 Files selected for processing (27)
README.mdskills/ast-bro/SKILL.mdsrc/adapters/php.rssrc/calls/cli.rssrc/calls/mcp.rssrc/calls/render.rssrc/calls/resolve.rssrc/calls/trace.rssrc/calls/traverse.rssrc/cli_error.rssrc/context.rssrc/core.rssrc/deps/cli.rssrc/deps/render.rssrc/graph_cache/delta.rssrc/graph_cache/mod.rssrc/hook/decide.rssrc/impact.rssrc/lib.rssrc/mcp/tools.rstests/calls_e2e.rstests/cli_ergonomics.rstests/digest_format.rstests/show_markdown.rstests/surface_e2e.rswiki/architecture.mdwiki/calls.md
Correctness: - callees --depth 1 (one-hop path) now reports frontier_truncated when a resolved direct callee has outgoing edges, matching deeper walks. - callers <Type> --limit bounds type groups too: implementations and constructions share the display budget after callable hits; groups carry implementations_total / constructions_total, text section headers say "(showing N; ...)", and top-level total/truncated compare combined-shown vs combined-total in both text and JSON. - implements only accepts *type* declarations as proof the target exists; a function homonym no longer validates a 0-match answer. - map --detail names honors --no-attrs / --no-lines (DigestOptions grew both switches; wired through CLI and MCP). - --max-members counts raw declarations before overload collapse, so text "+N more" equals JSON dropped_members on overloaded APIs; free functions are never capped (matching JSON and the per-type contract). - Pass A scoped-receiver match handles namespace-qualified receivers (Foo\Greeter::m(), a::b::Type::m()) via separator normalization with a terminal-segment fallback. - surface: an Io error on an existing path exits 1 (index_error), not 2. Performance: - impact: one depth-monotonic multi-source reverse BFS (traverse::callers_multi) replaces a walk per construction site, which was quadratic-ish on widely-constructed types. Same results (consider still collapses to per-qn minimum depth). Polish: - CliError::envelope()/emit_json_only() — the ast-bro.error.v1 shape lives in one place; the clap parse-error path reuses it. - MCP schemas constrain max_members with minimum: 0. - SKILL.md: impact --depth default is 2; wiki/calls.md matches receiver_is_self_like and the self-like import gate. Rejected (with reasoning, not changed): bounding the reverse-deps walk by --limit again — the true total #32 requires needs the full BFS, which is O(V+E) over the in-memory file graph, the same cost class as `graph` and `cycles`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
--no-docs now drops docs, docs_inside, and doc_start_byte from ast-bro.map.v1; --no-lines drops start_line/end_line/start_byte/ end_byte; --no-attrs drops attrs. doc_start_byte rides with either group (a doc artifact and a byte offset). Omission is opt-in per call: with no projection flags the payload is serialized through the original path and stays byte-identical, so no schema bump. The MCP map tool inherits the behavior via the shared renderer. (--no-private/--no-fields filtering and truncated/dropped_members already reached JSON via the #37/#32 work; this closes the rest of the flag set vlsi measured against 3.0.0.) Measured on src/core.rs: 72.8KB -> 41.4KB with the full projection stack; doc-heavy packages shed considerably more. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
--no-private and the digest public-only view were no-ops for Rust:
every renderer filters on the literal "private", while the Rust adapter
emitted "" for un-pub items (its own comment said "Rust default is
private"). The adapter now emits the canonical string, with inherited
contexts exempted:
- private when the modifier is missing: module-scope fns/structs/enums/
traits/mods, inherent-impl methods, named + tuple struct fields,
extern-block items, non-#[macro_export] macros
- inherited ("" as before): trait method declarations, trait consts,
trait-impl methods (they cannot carry a modifier and take the trait's
reach), enum variants, impl-block containers
Also fixes two latent digest-renderer gaps this exposed: _flatten_types
and _flatten_free_functions never applied the visibility gate to types
or namespace recursion, so private types (and pub fns inside private
mods, e.g. #[cfg(test)] mod tests) leaked into the public-only text
view while the JSON filter dropped them. Text and JSON now agree.
Effect: `map --no-private` actually strips Rust private items;
`sb digest src/` on this repo shrinks 53.7KB -> 21.8KB and delivers the
documented "types and public methods"; map's default (and the Read-hook
substitution) is unchanged. Note for release notes: ast-bro.map.v1
payloads for Rust now carry "visibility": "private" where they carried
"" — a value change, not a shape change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/agentic_review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Code review by qodo was updated up to the latest commit 082e3cc |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mcp/tools.rs (1)
490-541: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
run_map's signatures/full branch silently returns empty text — misaligned with the CLI's issue#33fix.When
resultsis empty (e.g. paths contain nothing parseable), theelsebranch at Lines 533-540 builds an emptyoutand returnsCallResult::Text("")silently. The CLI's equivalent path inrun_map_digestexplicitly guards this case and emits"# 0 parseable file(s) in the given path(s)", precisely because a silent empty answer is indistinguishable from a broken call. Thenamesbranch here is safe only becauserender_digestitself has that fallback; the signatures/full branch has no such guard.🩹 Proposed fix
} else { + if results.is_empty() { + return CallResult::Text("# 0 parseable file(s) in the given path(s)\n".to_string()); + } let mut out = String::new(); for res in &results { out.push_str(&core::render_map(res, &opts)); out.push('\n'); } CallResult::Text(out) }🤖 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 `@src/mcp/tools.rs` around lines 490 - 541, Update the signatures/full branch of run_map to detect an empty results collection before rendering and return the same "# 0 parseable file(s) in the given path(s)" message used by run_map_digest. Preserve the existing rendering loop for non-empty results and leave the JSON and names branches unchanged.src/impact.rs (1)
351-371: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAmbiguous, hidden constructors still seed depth-2+ dependents.
ambiguous_hidden(line 358) gates thetest_callspush at line 360 but not thector_walk_seedspush at lines 368-369. So when a construction edge is ambiguous and--hide-ambiguous/!opts.include_ambiguousis set, its own entry is hidden, but callers of that constructor still get traversed viacallers_multiand reported as depth-2+ transitive dependents. The parallel implementor-construction path (lines 405-414) correctlycontinues on the same condition before seeding — this path doesn't.🐛 Proposed fix
// Callers of the constructors are depth-2+ dependents — seeded // into the shared walk below. - if opts.depth > 1 { + // Skip ambiguous, hidden constructors so their callers don't + // leak into depth-2+ output either (matches the + // implementor-construction path below). + if opts.depth > 1 && !ambiguous_hidden { ctor_walk_seeds.push((e.source.clone(), 1)); }🤖 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 `@src/impact.rs` around lines 351 - 371, Update the constructor loop around collect_type_callers so ambiguous edges hidden by !opts.include_ambiguous are skipped before adding ctor_walk_seeds. Match the parallel implementor-construction path’s continue behavior, preventing hidden constructors and their depth-2+ dependents from entering the traversal while preserving existing handling for visible edges.
🧹 Nitpick comments (2)
skills/ast-bro/SKILL.md (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the new fenced blocks.
markdownlint flags MD040 on both new fences; the surrounding examples in this file use the same bare form, so
bashkeeps them consistent and highlighted.♻️ Proposed fix
- ``` + ```bash sb digest src/ sb digest src/ --glob '*.java' --max-members 8 ```As per static analysis hints (MD040, fenced-code-language).
Also applies to: 40-42
🤖 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 `@skills/ast-bro/SKILL.md` at line 34, Add the bash language identifier to both newly introduced fenced code blocks in SKILL.md, including the block containing the sb digest examples and the additional block referenced by the comment, while preserving their existing commands and formatting.Source: Linters/SAST tools
src/core.rs (1)
729-763: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
is_typepredicate between text and JSON capping paths.
matches!(decl.kind, Class | Struct | Interface | Record | Enum)here is repeated verbatim in_filter_decls(Line 1430). Both drive the same--max-memberscap and are covered by a test asserting parity between the text+N moreand JSONdropped_members. Extracting a singlefn _is_capped_type(kind: DeclarationKind) -> boolhelper removes the risk of the two capping paths silently diverging if the type-kind list changes.♻️ Proposed refactor
+fn _is_capped_type(kind: DeclarationKind) -> bool { + use DeclarationKind::*; + matches!(kind, Class | Struct | Interface | Record | Enum) +} + fn _render_decl(decl: &Declaration, opts: &MapOptions, indent: usize, out: &mut Vec<String>) { ... - let is_type = matches!(decl.kind, Class | Struct | Interface | Record | Enum); + let is_type = _is_capped_type(decl.kind);And in
_filter_decls:- let is_type = matches!(d.kind, Class | Struct | Interface | Record | Enum); + let is_type = _is_capped_type(d.kind);🤖 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 `@src/core.rs` around lines 729 - 763, Extract the repeated type-kind predicate into a shared _is_capped_type helper accepting DeclarationKind and returning whether the kind is Class, Struct, Interface, Record, or Enum. Replace the local is_type check in the text rendering path and the equivalent predicate in _filter_decls, preserving identical --max-members behavior and parity between text and JSON outputs.
🤖 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 `@skills/ast-bro/SKILL.md`:
- Line 15: Remove `context` from the `--depth` defaults list in the result-caps
documentation. Keep `context` documented with its `--budget` option and internal
depth-2 behavior, while retaining the `--depth` defaults for commands that
expose that flag.
- Line 11: Update the `--json` schema example in the command-output
documentation to use the emitted schema identifier `ast-bro.outline.v1` for
`map`/`digest`, replacing the nonexistent `ast-bro.map.v1`; keep the surrounding
stability and `--compact` behavior unchanged.
In `@src/lib.rs`:
- Around line 1159-1171: Update the read_to_string error handling in the squeeze
path: keep directory inputs as CliErrorKind::BadArgument, but classify other
read failures as the existing index_error/exit-1 error kind used for IO
failures, matching the surface handler’s behavior. Preserve the current
path-specific diagnostic details and JSON exit handling.
In `@wiki/calls.md`:
- Line 182: Update the traversal reference in the ambiguous-edge guidance to use
the exported entrypoint `src/deps/traverse.rs::forward` instead of the internal
`forward_bfs` primitive, leaving the surrounding dependency-closure instructions
unchanged.
---
Outside diff comments:
In `@src/impact.rs`:
- Around line 351-371: Update the constructor loop around collect_type_callers
so ambiguous edges hidden by !opts.include_ambiguous are skipped before adding
ctor_walk_seeds. Match the parallel implementor-construction path’s continue
behavior, preventing hidden constructors and their depth-2+ dependents from
entering the traversal while preserving existing handling for visible edges.
In `@src/mcp/tools.rs`:
- Around line 490-541: Update the signatures/full branch of run_map to detect an
empty results collection before rendering and return the same "# 0 parseable
file(s) in the given path(s)" message used by run_map_digest. Preserve the
existing rendering loop for non-empty results and leave the JSON and names
branches unchanged.
---
Nitpick comments:
In `@skills/ast-bro/SKILL.md`:
- Line 34: Add the bash language identifier to both newly introduced fenced code
blocks in SKILL.md, including the block containing the sb digest examples and
the additional block referenced by the comment, while preserving their existing
commands and formatting.
In `@src/core.rs`:
- Around line 729-763: Extract the repeated type-kind predicate into a shared
_is_capped_type helper accepting DeclarationKind and returning whether the kind
is Class, Struct, Interface, Record, or Enum. Replace the local is_type check in
the text rendering path and the equivalent predicate in _filter_decls,
preserving identical --max-members behavior and parity between text and JSON
outputs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b55affa0-bafc-4487-b5e0-423621a22837
📒 Files selected for processing (31)
README.mdskills/ast-bro/SKILL.mdsrc/adapters/php.rssrc/adapters/rust.rssrc/calls/cli.rssrc/calls/mcp.rssrc/calls/render.rssrc/calls/resolve.rssrc/calls/trace.rssrc/calls/traverse.rssrc/cli_error.rssrc/context.rssrc/core.rssrc/deps/cli.rssrc/deps/render.rssrc/graph_cache/delta.rssrc/graph_cache/mod.rssrc/hook/decide.rssrc/impact.rssrc/lib.rssrc/mcp/tools.rssrc/search/chunker.rstests/calls_e2e.rstests/cli_ergonomics.rstests/digest_format.rstests/show_markdown.rstests/surface_e2e.rswiki/architecture.mdwiki/calls.mdwiki/file-filtering.mdwiki/search.md
- squeeze: a read failure on an existing file exits 1 (index_error), matching the surface handler's Io mapping; a directory argument stays exit 2 (bad_argument). - impact: constructions hidden by --hide-ambiguous no longer seed the transitive walk — same rule the implementor path already applied, so depth-2+ dependents of a hidden edge can't leak into the report. - MCP map: zero parseable files returns the CLI's "# 0 parseable file(s) in the given path(s)" message instead of an empty string. - core: extract _is_capped_type — one shared definition of which kinds --max-members caps, used by both the text renderer and the JSON filter so the two can't drift. - SKILL.md: drop `context` from the --depth defaults list (it exposes no --depth; it walks to depth 2 internally under --budget). - wiki/calls.md: pass C references src/deps/traverse.rs::forward (the actual exported entrypoint), not forward_bfs. Rejected with reasoning: renaming ast-bro.map.v1 to "ast-bro.outline.v1" (the latter does not exist — the binary emits ast-bro.map.v1, verified live) and bash-tagging two fenced blocks (all 18 indented example blocks in SKILL.md share the untagged style; partial tagging would be inconsistent). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(self) - search/chunker: a line holding only spaces/tabs is still a blank line. The paragraph splitter consumes horizontal whitespace between line terminators (committing only when another terminator follows, so indentation opening a non-blank line stays put). Split points still land right after an ASCII \n — UTF-8 boundary and byte-exact coverage unchanged. LF + CRLF regression tests added. - calls/resolve: drop the terminal-segment fallback for qualified receivers. `other::Type::method()` names another scope's Type; when the complete normalized receiver path doesn't match a local qn the edge defers to pass B/C (Inferred via dep closure, or honestly Ambiguous) instead of binding Exact to a same-file homonym. A PHP self-referencing FQN now resolves Inferred rather than Exact. - adapters/rust: `pub(self)` / `pub(in self)` normalize to "private" — they grant no reach beyond the module. Wider restricted forms (pub(crate), pub(super), pub(in path)) deliberately stay visible in the public-only view, matching how C# internal and Java package-private have always been treated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit ae310db |
|
/agentic_review |
MCP map with an all-missing path list returned "# 0 parseable file(s)"
(or a valid-looking {"files": []} under json) with isError:false — the
exact confusion #33 fixed on the CLI, where walk_and_parse silently
drops missing inputs. The server has no stderr and no exit codes, so
the distinction must ride the error channel.
New resolve_paths_for_mcp (the MCP-side counterpart of require_paths)
returns Err for an empty list or a list where nothing resolves, and a
partial-miss note otherwise. Wired into map, digest, implements, and
run (which keeps its bare default "." and validates only explicit
paths). Text responses prepend the partial-miss note; JSON responses
stay pure. Existing paths with no parseable files remain a successful
empty answer.
Adds tests/mcp_e2e.rs — the first e2e coverage of the MCP server over
stdio: missing-path text + json reject with isError, empty-but-existing
dirs stay success, partial misses render with the note.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit ae310db |
- calls/resolve: when several lexical scopes in one file declare the same `Type::method` suffix, pass A no longer takes the first suffix match. A unique match still binds Exact; multiple matches bind only a candidate whose scope encloses the caller (innermost wins — what the unqualified path means in source). A caller outside every declaring scope defers to pass B/C, preserving candidates as Ambiguous. - core/_filter_decls: apply the parent's --max-members cap to eligible direct children BEFORE recursing, so a capped subtree that the ancestor cap removes contributes nothing to dropped_members. JSON now equals the sum of the text renderer's "+N more" lines even with nested capped types (previously the removed subtree's internal drops were counted first and never rolled back). Eligibility lives in one _map_eligible predicate shared by the parent filter and the pre-cap child count. Regression tests: mod-a/mod-b Foo::method scope binding (Exact to the enclosing scope; no arbitrary Exact from outside), and a nested capped class removed by an ancestor cap (dropped_members == Σ text "+N"). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/agentic_review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Code review by qodo was updated up to the latest commit f11285a |
- adapters/rust: replace the `inherited: bool` parameter with a two-variant Vis enum (Owned | Inherited) so call sites name the semantics instead of passing a bare bool; same for _const_or_static_to_field. - tests: pin the restricted-visibility design down — pub(self) / pub(in self) are hidden by --no-private, pub(crate) / pub(super) / pub(in crate) stay visible (the C# internal / Java package-private analogy). Extend the self-like receiver regression with a variable named `static` alongside the existing `parent` case. - tests/mcp_e2e: bound the server wait at 30s (thread + recv_timeout, no new deps) and capture stderr into panic messages instead of nulling it. Rejected from the review round: the "+N parsing is vacuous" claim (the test asserts equality with a fixture-guaranteed 2, so a parse failure reads 0 != 2 and fails loudly) and "update downstream prompt consumers" (sb prompt is SKILL.md with frontmatter stripped — the contract ships in the prompt by construction). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
tests/calls_e2e.rs (1)
1928-1941: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test passes vacuously if the fixture stops yielding unattributed sites.
With
shown == 0andunattributed_total == 0every assertion holds, so a regression that drops unattributed reporting entirely would go unnoticed. Anchor the fixture expectation first.💚 Proposed tightening
let shown = doc["unattributed"].as_array().unwrap().len(); let total = doc["unattributed_total"].as_u64().unwrap() as usize; + assert!( + total >= 2, + "fixture must produce unattributed sites for this test to mean anything: {out}" + ); assert!( shown <= 1, "unattributed section must respect the remaining --limit budget: {out}" );🤖 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/calls_e2e.rs` around lines 1928 - 1941, Strengthen the test around the callers JSON response by asserting the fixture produces at least one unattributed site before validating limit and truncation behavior. Update the assertions in the test using doc["unattributed_total"] so a zero total fails explicitly, while preserving the existing shown-count and truncation checks.src/core.rs (1)
750-754: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
visibleduplicates_map_eligibleminus the Heading/CodeBlock rule.
_is_capped_typewas extracted precisely so the two renderers can't drift; the eligibility predicate is the other half of that contract and is still forked. Consider a shared predicate (taking the field/private/docs flags) used by both_render_decland_map_eligible.🤖 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 `@src/core.rs` around lines 750 - 754, The visible predicate in the declaration rendering flow duplicates eligibility logic from _map_eligible and can drift. Extract or reuse a shared predicate, including field, private, and documentation-related flags as needed, then update both visible and _map_eligible to call it while preserving _map_eligible’s Heading/CodeBlock exclusion.tests/digest_format.rs (1)
488-547: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSubstring assertions on the whole digest can false-negative via the temp path.
!digest.contains("Hidden")/"gain"also match the rendered directory and file names. Vanishingly unlikely withtempdir(), but scoping the negative checks to declaration lines (orp-stripped output) makes the test independent of the environment.🤖 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/digest_format.rs` around lines 488 - 547, Update rust_missing_pub_means_private_except_inherited_contexts so negative assertions inspect declaration content rather than the entire digest, avoiding matches from the temporary directory or file path. Strip the input path from rendered output or scope checks to relevant declaration lines, while preserving the existing visibility assertions.src/calls/render.rs (1)
76-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
section_suffixhere.This inlines the same string the new helper produces.
♻️ Proposed tidy-up
- let suffix = if total > edges.len() { - format!(" (showing {}; raise --limit to see the rest)", edges.len()) - } else { - String::new() - }; + let suffix = section_suffix(total, edges.len());🤖 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 `@src/calls/render.rs` around lines 76 - 80, Update the suffix construction in the rendering flow to reuse the existing section_suffix helper instead of duplicating its formatted string and conditional logic. Preserve the current total-versus-edges behavior and pass the appropriate values to section_suffix.src/calls/mcp.rs (1)
68-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winText-mode MCP responses drop the frontier signal.
frontier_truncatedreaches JSON consumers but the text branch has no equivalent — the CLI compensates with a stderr note, which MCP lacks. Consider prepending it alongsidehidden_noteso a depth-capped walk isn't read as exhaustive.♻️ Suggested addition
} else { format!( - "{}{}", + "{}{}{}", hidden_note, + if frontier_truncated && depth > 1 { + format!( + "# note: depth {} reached with unexplored edges beyond it; raise depth to walk further\n", + depth + ) + } else { + String::new() + }, render::render_callers_text(target, &hits, &unattributed, unattributed_total, &trunc) ) }🤖 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 `@src/calls/mcp.rs` around lines 68 - 89, Update the non-JSON branch around render_callers_text to include a text representation of frontier_truncated alongside hidden_note before the rendered callers output, ensuring MCP text responses indicate when the depth-capped walk is incomplete while preserving the existing JSON behavior.src/calls/traverse.rs (1)
219-227: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
frontier_truncatedcan fire when nothing new is reachable.
edges_at(&cur)being non-empty doesn't mean unexplored nodes remain — all targets may already be inseen/reported. The stderr hint ("raise--depthto walk further") then appears on walks that are actually complete. Checking for at least one unseen target would tighten it, at the cost of threadingseeninto the check.🤖 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 `@src/calls/traverse.rs` around lines 219 - 227, Update the depth-cap handling in the traversal loop to set frontier_truncated only when edges_at(&cur) contains at least one target not already present in seen or reported. Thread the existing reachability state into this check, preserving the current depth-limit behavior and avoiding the truncation hint when all targets were already discovered.src/lib.rs (1)
882-955: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePath-resolution loop is duplicated between
require_pathsandresolve_paths_for_mcp.Both walk
paths, split intoexisting/missingwith identicalexpand_existingsemantics; only the reporting differs. Extracting the split keeps CLI and MCP from drifting on what "resolved" means.♻️ Suggested extraction
fn split_existing(paths: &[PathBuf]) -> (Vec<PathBuf>, Vec<String>) { let mut existing = Vec::new(); let mut missing = Vec::new(); for p in paths { if path_glob::expand_existing(p).is_empty() { missing.push(p.display().to_string()); } else { existing.push(p.clone()); } } (existing, missing) }🤖 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 `@src/lib.rs` around lines 882 - 955, Extract the shared path partitioning logic from require_paths and resolve_paths_for_mcp into a helper such as split_existing(paths), returning existing paths and missing display strings. Update both callers to use this helper while preserving their distinct CLI and MCP error/reporting behavior.tests/cli_ergonomics.rs (1)
139-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese two assertions are coupled to this repo's own source shape.
implements CliErrorflips to a non-empty match the moment anythingimplsCliError(e.g. aDisplay/Fromimpl), and thestdout.len() > 25_000threshold depends onsrc/staying above the hint cutoff. A small fixture directory / a type that is intentionally never implemented would keep both tests asserting the contract rather than the repo's current contents.Also applies to: 272-283
🤖 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_ergonomics.rs` around lines 139 - 146, Update the affected tests in tests/cli_ergonomics.rs to use dedicated fixture sources instead of the repository’s src/ tree and CliError. Define a fixture type that has no implementations and keep the fixture contents intentionally sized to exercise the intended hint-cutoff behavior, so the assertions remain stable as production code changes.
🤖 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 `@skills/ast-bro/SKILL.md`:
- Line 34: Update both shell command fences in the documentation to use the bash
language tag, changing each opening fence to ```bash while preserving the
commands and closing fences.
In `@src/lib.rs`:
- Around line 789-795: Update the rejection-path argument collection in
src/lib.rs to use std::env::args_os() and convert arguments with
to_string_lossy() before collecting them as Vec<String>. Preserve the existing
json_mode and subcommand detection so non-UTF-8 arguments still follow the
exit-2 handling contract without panicking.
In `@src/mcp/tools.rs`:
- Around line 573-577: Preserve and surface the partial-miss note in the digest
response: update the result-building logic around resolve_paths_for_mcp and
walk_and_parse to use path_note instead of discarding it, prefixing non-JSON
output with the note consistently with run_map’s note handling.
In `@wiki/architecture.md`:
- Line 39: Update the exit-code documentation to clarify that exit code 0
indicates successful execution but may contain partial results, such as path
misses; require consumers to inspect stderr notes before treating output as
exhaustive. Apply this contract correction at wiki/architecture.md lines 39-39
and README.md lines 453-453, replacing wording that claims every exit-0 result
is complete.
---
Nitpick comments:
In `@src/calls/mcp.rs`:
- Around line 68-89: Update the non-JSON branch around render_callers_text to
include a text representation of frontier_truncated alongside hidden_note before
the rendered callers output, ensuring MCP text responses indicate when the
depth-capped walk is incomplete while preserving the existing JSON behavior.
In `@src/calls/render.rs`:
- Around line 76-80: Update the suffix construction in the rendering flow to
reuse the existing section_suffix helper instead of duplicating its formatted
string and conditional logic. Preserve the current total-versus-edges behavior
and pass the appropriate values to section_suffix.
In `@src/calls/traverse.rs`:
- Around line 219-227: Update the depth-cap handling in the traversal loop to
set frontier_truncated only when edges_at(&cur) contains at least one target not
already present in seen or reported. Thread the existing reachability state into
this check, preserving the current depth-limit behavior and avoiding the
truncation hint when all targets were already discovered.
In `@src/core.rs`:
- Around line 750-754: The visible predicate in the declaration rendering flow
duplicates eligibility logic from _map_eligible and can drift. Extract or reuse
a shared predicate, including field, private, and documentation-related flags as
needed, then update both visible and _map_eligible to call it while preserving
_map_eligible’s Heading/CodeBlock exclusion.
In `@src/lib.rs`:
- Around line 882-955: Extract the shared path partitioning logic from
require_paths and resolve_paths_for_mcp into a helper such as
split_existing(paths), returning existing paths and missing display strings.
Update both callers to use this helper while preserving their distinct CLI and
MCP error/reporting behavior.
In `@tests/calls_e2e.rs`:
- Around line 1928-1941: Strengthen the test around the callers JSON response by
asserting the fixture produces at least one unattributed site before validating
limit and truncation behavior. Update the assertions in the test using
doc["unattributed_total"] so a zero total fails explicitly, while preserving the
existing shown-count and truncation checks.
In `@tests/cli_ergonomics.rs`:
- Around line 139-146: Update the affected tests in tests/cli_ergonomics.rs to
use dedicated fixture sources instead of the repository’s src/ tree and
CliError. Define a fixture type that has no implementations and keep the fixture
contents intentionally sized to exercise the intended hint-cutoff behavior, so
the assertions remain stable as production code changes.
In `@tests/digest_format.rs`:
- Around line 488-547: Update
rust_missing_pub_means_private_except_inherited_contexts so negative assertions
inspect declaration content rather than the entire digest, avoiding matches from
the temporary directory or file path. Strip the input path from rendered output
or scope checks to relevant declaration lines, while preserving the existing
visibility assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5848e320-043a-4d49-b9a7-a2e456ca991a
📒 Files selected for processing (32)
README.mdskills/ast-bro/SKILL.mdsrc/adapters/php.rssrc/adapters/rust.rssrc/calls/cli.rssrc/calls/mcp.rssrc/calls/render.rssrc/calls/resolve.rssrc/calls/trace.rssrc/calls/traverse.rssrc/cli_error.rssrc/context.rssrc/core.rssrc/deps/cli.rssrc/deps/render.rssrc/graph_cache/delta.rssrc/graph_cache/mod.rssrc/hook/decide.rssrc/impact.rssrc/lib.rssrc/mcp/tools.rssrc/search/chunker.rstests/calls_e2e.rstests/cli_ergonomics.rstests/digest_format.rstests/mcp_e2e.rstests/show_markdown.rstests/surface_e2e.rswiki/architecture.mdwiki/calls.mdwiki/file-filtering.mdwiki/search.md
… JSON - calls/resolve: crate::/self::/super:: receiver prefixes never match qns textually (qns start with the repo-relative file path). Strip the prefixes and resolve the remainder against the caller's scope chain by anchored equality — self:: at the caller's enclosing scope (innermost first), super:: one level up the same chain, crate:: at the file segment. No terminal-segment fallback; a miss still defers to pass B/C. Previously all three forms fell through and could stay Ambiguous despite naming their target explicitly. - core/_map_eligible: markdown Heading/CodeBlock declarations are the structure of the file (their docs are empty), not doc comments — stop deleting them from JSON when include_docs is off. The detail-level work had made this catastrophic: digest README.md --json returned 0 declarations against 29 headings + 23 code blocks in text. The JSON projection still strips documentation fields; the declarations stay. Tests: crate/self/super forms each bind Exact to the enclosing scope's Foo past a same-file decoy; markdown JSON now carries all 52 declarations with docs keys stripped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…st rigor Inline findings: - lib.rs: the clap rejection path collects args via args_os() + to_string_lossy(), so a non-UTF-8 argument exits 2 per the contract instead of panicking std::env::args(). - MCP digest/implements: partial-miss path notes are prepended to non-JSON responses instead of discarded (matching map). - docs: exit-0 wording in wiki/architecture.md and README no longer claims completeness — qualifications arrive as stderr notes; read them before treating output as exhaustive. - SKILL.md: all 18 bare opening fences tagged ```bash (uniformly, resolving the earlier partial-tagging objection). Nitpicks: - MCP callers text carries the frontier note when an explicitly deepened walk was cut (same depth>1 gating as the CLI). - render: unattributed_section reuses section_suffix. - traverse: frontier_truncated fires only when something genuinely unexplored lies beyond the cap — back-edges to visited nodes no longer produce a raise---depth hint that would deliver nothing. - core: _render_decl's early-return and member-visibility closure both call _map_eligible — one projection predicate for text and JSON. - lib.rs: split_existing extracts the path partition shared by require_paths and resolve_paths_for_mcp. - tests: unattributed-budget test asserts total >= 2 — which exposed the fixture only ever produced one unattributed site (the rest resolved Inferred via the dep closure), so the truncation branch had never run; rebuilt with three genuinely ambiguous sites. implements zero-impls test is fixture-local. Rust visibility test strips the temp path before negative assertions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit b02278b |
The stripping loop collapsed crate::/self::/super:: into one boolean and walked the caller's scope chain upward taking the first anchored match — "search upward until found", not the per-prefix semantics claimed. An ancestor decoy proved it: super::Foo::method() from inside `deep` bound deep's own Foo instead of the parent's. Each prefix now picks its own anchor: - crate::P — one anchored trial at the file segment, no walk; an intermediate scope declaring the same path cannot shadow the root. - super::^n P — the walk skips n levels past the caller's own scope before the first trial, so a caller-level decoy cannot shadow the parent. - self::P — walk from the enclosing scope, as before. Documented limitation (previously overstated): a method's qn carries a type segment indistinguishable from a module, so super:: may start one level early for methods and self:: keeps walking upward — anchored equality keeps any hit real, and every case is no worse than before; free functions are exact. Regression tests pin both reviewer scenarios: the super:: ancestor decoy and the crate:: intermediate-scope decoy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- adapters/rust: a trait impl inherits its trait's reach. During impl distribution (the one place the impl and the same-scope trait decl are both in hand), methods implementing a locally-declared private trait are marked private, so `impl Hidden for Loud` no longer leaks its methods into public-only map/digest output just because the type is pub. Traits declared elsewhere (other files, std) keep inherited visibility — their modifier is unknowable from this file. - MCP json partial misses: resolve_paths_for_mcp returns the raw missing list; text responses keep the "# note: path not found" line (partial_note), and JSON responses inject the unresolved originals as a missing_paths field (with_missing_paths) so a partial payload can never read as covering inputs it never saw. Applied to map, digest, and implements; no-op (byte-identical payload) when everything resolved. Tests: private-trait impl hidden in digest / shown in map default; mcp_e2e drives map + digest with one existing + one missing path under json:true and asserts missing_paths, the rendered file, and no phantom field on clean requests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vlsi
left a comment
vlsi
left a comment
There was a problem hiding this comment.
Reviewed at 72a53b5. I built the branch and ran the binary against two real repositories: this one, and pgjdbc (~300k lines of Java) where I had an existing API-naming task to drive it end to end. cargo test --release --no-fail-fast passes except deps_e2e::go_module_prefix_strips_correctly, which is red on main too.
The error contract holds up on real code. I re-measured every case in the diagnostics table I keep for this tool — map /nope.java, surface /nope, show <file> NoSuchSymbol, implements NoSuchSymbol <path>, callers NoSuchSymbol, deps /nope.java, map with no paths, and an unknown flag — and all eight now behave identically: exit 2, empty stdout, message on stderr. The table collapsed into one sentence, which is the point of #33/#36. The map/digest merge also came through clean: map --preset digest --json is byte-identical to digest --json, and --max-members text/JSON parity matched exactly on four files I checked.
Two things I would fix before merge, plus three smaller ones.
Blocking — the unattributed section is unusable on common method names. Details inline on src/calls/traverse.rs. On pgjdbc, callers PgConnection.close returns 0 resolved callers and 1124 unattributed; the first three are connection.close(), os.close() on an OutputStream, and lo.close() on a LargeObject. In this repository, callers CliError.new returns 1 resolved and 1085 unattributed — every Vec::new() and String::new() in the tree.
implements <TARGET> now rejects a call that --help says is valid. Details inline on src/lib.rs.
For the record, on precise targets the section does exactly what #31 promised, and that is worth keeping. On pgjdbc, callers CodecContextBuilder.registry reports 2 resolved (Inferred) plus 5 unattributed against 8 hits for git grep '\.registry(', the eighth being the declaration. CodecContextBuilder.prefersJavaTime reports 0 plus 1, and grep finds exactly one real call site. Fluent chains through an interface-typed receiver still do not resolve, but they no longer vanish silently. The fix is to gate the section on whether the name discriminates, not to remove it.
Three smaller items I did not anchor inline:
digest --jsondropsdocsfromast-bro.map.v1without a schema bump. Measured onsrc/core.rs: 18docskeys and 55,586 bytes onmain, 0 keys and 16,675 bytes here. The README documents the behavior, but the caller never opted in — the preset did — and the schema constant is unchanged, so a consumer indexingdocsbreaks with no signal to guard on. A release-note line would cover it; av2would be safer.callees <Type>did not get the--limitfix thatcallers <Type>got in 45312b0. Ancestor groups are neither bounded by--limitnor counted intotal, sotruncatedinast-bro.callees.v1describes onlymatches. Inheritance chains are short, so this is a consistency gap rather than a volume problem.callers,impact, andreverse-depsnow walk withusize::MAXto compute a true total. At the default--depth 1that costs nothing; at--depth 5on a large repository it is the entire reverse cone, where the old code stopped at 200 hits. The trade-off looks right, but--limitno longer bounds work, and the docs still read as though it does.
vlsi
left a comment
vlsi
left a comment
There was a problem hiding this comment.
One more, found while re-measuring the cap reporting on real repositories.
The unresolved-call-site section (#31) keyed only on the callee name, and candidate sets are keyed on the bare name too, so every `Vec::new()` in the tree carried `CliError::new` among its candidates: `callers CliError.new` reported 1 resolved caller and 1020 "possible" ones, `callers PgConnection.close` on pgjdbc 0 and 1124. A number that size reads as a rename cost while describing other symbols entirely — the opposite of what the section is for. Three narrowings, none of them type inference: - receiver check: a receiver naming a project type other than the target's enclosing type meant that other type (pass A's rule, one layer later). Locals, parameters and external types stay in, or the real `connection.close()` sites would go with the noise. - discrimination gate: `name_declarers` counts the project symbols sharing the target's terminal name; past 3 the rows are withheld for a one-line count that states the reason. JSON carries `unattributed_declarers` and `unattributed_suppressed`, so MCP stops shipping 199 unattributable rows into an agent's context. - own cap of min(--limit, 25), ordered by ambiguity breadth ascending, with the receiver on every row. Sharing --limit gave the worst-resolved targets the most screen. Also from the review round: - `implements <TARGET>` declared PATHS optional in clap while rejecting the call at runtime, and blamed a shell substitution for a missing argument. Required in clap now, and the error envelope names the argument instead of quoting clap's first line. - the `crate::` anchor fired only when the caller's own file declared a matching path — precisely the case where anchoring there is wrong. Gated on the caller being the crate root. (Pass C can still prefer a same-file decoy; it no longer claims Exact.) - `callees` printed the post-cap count in its stdout header while stderr and JSON reported the true total, and `--hide-external` left header, note and `total` counting three different populations. One filter, applied before counting, in both CLI and MCP. - `callees <Type>` ancestor groups now count toward `total` and share the display budget, as `callers <Type>` already did. - `run`, `search` and `find-related` didn't follow the error contract the README claims for every subcommand: unresolved paths now exit 2 with empty stdout and an `ast-bro.error.v1` envelope, and `find-related --json` stops answering "nothing is similar" for a location the index never saw. `run`'s exit 1 on no matches is grep convention and is now documented as such. - `map --json` payloads with keys removed carry `projected: {docs, line_numbers, attributes}`, so a consumer of `digest --json` has a signal to guard on rather than an absence to infer. - `--limit` help and docs now say it caps display, not the walk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`apply_delta_to_calls` step 9 re-resolved bare edges by assigning `symbol_table[name]` — the *unfiltered* global match list — as the edge's candidate set, and it did so for every bare edge in the graph, not just the ones the delta touched. A cold build gets there through pass C, which filters candidates to what the caller's file can reach through the dep graph, so the two paths disagreed and one edit to an unrelated file changed answers everywhere: on ast-bro itself, `callers CliError.new` reported 1023 unresolved sites cold and 1073 after appending a comment to src/adapters/sql.rs, because every `Vec::new()` in the tree had just been handed `CliError::new` as a candidate the dep filter had ruled out. Pass C's decision is now one function, `resolve::disambiguate`, used by both the cold build and the updater, with the per-file dep closure memoized (`ClosureCache`) since a sweep re-asks for it repeatedly. Pass B's rule — single global match, no receiver — still runs first, unchanged. Parity across 15 symbols × callers/callees on this repo is now byte-identical between a partial update and a cold build of the same content. The regression test asserts it on a fixture where the dep filter leaves two of three homonyms, which is the shape that diverged; it fails against the old code. Closes the "pass C is not re-run in the partial-update path" known gap in wiki/calls.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/agentic_review |
| let unattributed = render::Unattributed { | ||
| edges: &unattributed, | ||
| total: unattributed_total, | ||
| hidden: unattributed_hidden, | ||
| declarers, |
There was a problem hiding this comment.
1. Hidden unattributed total wrong 🐞 Bug ≡ Correctness
run_callers computes unattributed_total after clearing unresolved call sites when ambiguous edges are hidden, so JSON/text can report unattributed_total: 0 even when `unattributed_hidden > 0. This breaks the documented Unattributed` contract (“true total behind them”) and misleads consumers about how many unresolved call sites existed.
Agent Prompt
### Issue description
When `--hide-ambiguous` (CLI) / `include_ambiguous:false` (MCP) hides unresolved call sites, the code clears the `unattributed` vector and *then* sets `unattributed_total = unattributed.len()`. This makes `Unattributed.total` exclude hidden sites, contradicting the renderer contract that `total` is the true pre-filter count.
### Issue Context
`src/calls/render.rs` documents `Unattributed.total` as the “true total behind them” and `hidden` as the count dropped by `--hide-ambiguous`. Both the CLI and MCP implementations currently lose the pre-hide total.
### Fix Focus Areas
- src/calls/cli.rs[162-191]
- src/calls/mcp.rs[50-80]
### What to change
- Capture `unattributed_total` **before** any hiding/truncation, e.g. immediately after computing/filtering `unattributed`.
- If you want `total` to always include hidden rows, set `unattributed_total` to the pre-hide length (or compute it as `unattributed.len() + unattributed_hidden` after clearing, ensuring it matches the intended semantics).
- Keep `edges` as the displayed sample (possibly empty when hidden), but keep `total` as the true count so JSON fields like `unattributed_total` remain accurate.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit ee39be9 |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/mcp/tools.rs (1)
896-903: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftReverse traversal is still unbounded while
limitonly trims the display.
reverse(..., usize::MAX, ...)materializes the full reachable importer set beforehits.truncate(a.limit), solimitno longer bounds traversal or memory on large repos. Same concern as previously raised; if the true-total requirement makes a bounded page impractical, a counting-only walk that doesn't retain everyDepHitwould preserve both properties.🤖 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 `@src/mcp/tools.rs` around lines 896 - 903, Update the reverse traversal around crate::deps::traverse::reverse so limit bounds retained results and memory instead of traversing with usize::MAX and truncating afterward. Preserve the true total by using a counting-only traversal or equivalent approach that counts all reachable importers without materializing every DepHit, while returning only the first a.limit hits.
🧹 Nitpick comments (6)
tests/show_markdown.rs (1)
49-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that rejected
showcalls keep stdout empty.This test pins the exit code and stderr, but not the no-stdout portion of the CLI error contract.
Proposed test addition
assert_eq!(out.status.code(), Some(2), "substring must not match a code symbol"); + assert!( + out.stdout.is_empty(), + "rejected show call wrote to stdout:\n{}", + String::from_utf8_lossy(&out.stdout) + ); let stderr = String::from_utf8(out.stderr).expect("utf8");🤖 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/show_markdown.rs` around lines 49 - 58, Update the rejected show-command test around Command::new(bin()) to assert that out.stdout is empty, while preserving the existing exit-code and stderr assertions. This should verify the CLI error contract for substring-matching failures without changing production behavior.tests/digest_format.rs (1)
614-620: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSubstring overlap makes the failure message misleading.
"in_self_only"contains"self_only", so a regression that leaks onlyin_self_onlyfails theself_onlyassertion first and points at the wrong item. Matching on the rendered token (e.g.self_only()) disambiguates.🤖 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/digest_format.rs` around lines 614 - 620, Update the hidden-item assertions in the no-private output test to match each rendered token distinctly, such as using the complete `self_only()` and `in_self_only()` forms, so substring overlap cannot report the wrong item; keep the shown-item visibility assertions unchanged.tests/calls_e2e.rs (1)
2511-2593: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider pinning the cache-invalidation trigger rather than relying on mtime resolution.
The delta path is entered because
src/unrelated.rsis rewritten immediately after the cold build; on filesystems with coarse mtime granularity the edit can land in the same tick and the re-query may silently take the "nothing changed" path, making the parity assertions pass vacuously. Asserting that the incremental run actually saw a delta (or nudging mtime explicitly) would keep the test honest.🤖 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/calls_e2e.rs` around lines 2511 - 2593, Make incremental_update_matches_a_cold_build_of_the_same_content explicitly trigger cache invalidation after rewriting unrelated.rs, rather than relying on filesystem mtime resolution. Nudge the file’s modification timestamp to a later value using the test’s available filesystem/time utilities, or assert the existing delta-path indicator if the query exposes one, before issuing the incremental query. Keep the parity assertions unchanged.tests/cli_ergonomics.rs (1)
276-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese two tests encode the repo's own directory sizes.
src/staying above 25 KB is safe, butsrc/hook/staying below the threshold is a property of current sources, so ordinary growth will flipmap_on_small_input_stays_quietinto a failure unrelated to the hint logic. Atempfilefixture (one tiny file vs. many generated ones) pins both sides of the threshold to the 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 `@tests/cli_ergonomics.rs` around lines 276 - 296, Replace the source-directory-dependent coverage in map_on_large_directory_hints_at_digest_preset and map_on_small_input_stays_quiet with tempfile-based fixtures: create a deliberately small input below the threshold and a generated directory above it, then invoke map using those paths. Preserve the assertions for exit status, stdout size where applicable, and digest-preset hints only on stderr for the large fixture.src/lib.rs (1)
806-810: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSubcommand detection can name a flag value instead of the subcommand.
find(|a| !a.starts_with('-'))picks the first non-dash token, which for a global-flag-with-value form (e.g.--some-value foo bogus-sub) is the flag's value, not the subcommand. Envelopecommandand the`{}` has no `{}` flagdetail then name the wrong thing. Usinge.get(ContextKind::InvalidSubcommand)/ clap'susagecontext, or matching the token againstCli::command().get_subcommands(), keeps the field honest.🤖 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 `@src/lib.rs` around lines 806 - 810, Update subcommand detection in the raw argument parsing around the subcommand variable so flag values are not treated as subcommands. Match tokens against the actual subcommands exposed by Cli::command() (or use clap’s InvalidSubcommand/usage context), and retain the existing ast-bro fallback when no valid subcommand is found.README.md (1)
444-445: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stale JSON schema table now that rejections are covered.
mapanddigestboth documentast-bro.map.v1elsewhere, and rejections are described in the paragraph below, so the standalone table with outdatedast-bro.outline.v1entries is misleading.🤖 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 `@README.md` around lines 444 - 445, Remove the stale JSON schema table entry from README.md, including the outdated ast-bro.outline.v1/ast-bro.error.v1 documentation. Keep the existing ast-bro.map.v1 references for map and digest and the paragraph describing rejected calls unchanged.
🤖 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/graph_cache/delta.rs`:
- Around line 258-286: The single-candidate promotion in the delta update loop
should match cold-build confidence semantics. In the `calls.forward` iteration,
change the `Confidence` assigned alongside `CallTarget::Resolved` for a unique
global symbol-table match to the same exact-confidence value used by Pass B;
leave the receiver check and delegated `resolve::disambiguate` path unchanged.
In `@src/mcp/tools.rs`:
- Around line 1024-1027: Update the run tool’s resolve_paths_for_mcp handling to
retain both resolved and missing paths instead of discarding the missing list.
Prefix text responses with partial_note(&missing), and wrap JSON responses with
with_missing_paths(..., &missing), while preserving the existing error handling
for resolution failures.
In `@wiki/calls.md`:
- Around line 313-315: Update the “Bare-name re-resolution” documentation to
describe single-match, no-receiver hits as promoting to Resolved/Exact, matching
Pass B’s documented result and confidence semantics; leave the disambiguation
behavior and parity explanation unchanged.
---
Duplicate comments:
In `@src/mcp/tools.rs`:
- Around line 896-903: Update the reverse traversal around
crate::deps::traverse::reverse so limit bounds retained results and memory
instead of traversing with usize::MAX and truncating afterward. Preserve the
true total by using a counting-only traversal or equivalent approach that counts
all reachable importers without materializing every DepHit, while returning only
the first a.limit hits.
---
Nitpick comments:
In `@README.md`:
- Around line 444-445: Remove the stale JSON schema table entry from README.md,
including the outdated ast-bro.outline.v1/ast-bro.error.v1 documentation. Keep
the existing ast-bro.map.v1 references for map and digest and the paragraph
describing rejected calls unchanged.
In `@src/lib.rs`:
- Around line 806-810: Update subcommand detection in the raw argument parsing
around the subcommand variable so flag values are not treated as subcommands.
Match tokens against the actual subcommands exposed by Cli::command() (or use
clap’s InvalidSubcommand/usage context), and retain the existing ast-bro
fallback when no valid subcommand is found.
In `@tests/calls_e2e.rs`:
- Around line 2511-2593: Make
incremental_update_matches_a_cold_build_of_the_same_content explicitly trigger
cache invalidation after rewriting unrelated.rs, rather than relying on
filesystem mtime resolution. Nudge the file’s modification timestamp to a later
value using the test’s available filesystem/time utilities, or assert the
existing delta-path indicator if the query exposes one, before issuing the
incremental query. Keep the parity assertions unchanged.
In `@tests/cli_ergonomics.rs`:
- Around line 276-296: Replace the source-directory-dependent coverage in
map_on_large_directory_hints_at_digest_preset and map_on_small_input_stays_quiet
with tempfile-based fixtures: create a deliberately small input below the
threshold and a generated directory above it, then invoke map using those paths.
Preserve the assertions for exit status, stdout size where applicable, and
digest-preset hints only on stderr for the large fixture.
In `@tests/digest_format.rs`:
- Around line 614-620: Update the hidden-item assertions in the no-private
output test to match each rendered token distinctly, such as using the complete
`self_only()` and `in_self_only()` forms, so substring overlap cannot report the
wrong item; keep the shown-item visibility assertions unchanged.
In `@tests/show_markdown.rs`:
- Around line 49-58: Update the rejected show-command test around
Command::new(bin()) to assert that out.stdout is empty, while preserving the
existing exit-code and stderr assertions. This should verify the CLI error
contract for substring-matching failures without changing production behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fdfd21be-6f6c-45d3-82cb-266e683c9cf4
📒 Files selected for processing (33)
README.mdskills/ast-bro/SKILL.mdsrc/adapters/php.rssrc/adapters/rust.rssrc/calls/cli.rssrc/calls/mcp.rssrc/calls/render.rssrc/calls/resolve.rssrc/calls/trace.rssrc/calls/traverse.rssrc/cli_error.rssrc/context.rssrc/core.rssrc/deps/cli.rssrc/deps/render.rssrc/graph_cache/delta.rssrc/graph_cache/mod.rssrc/hook/decide.rssrc/impact.rssrc/lib.rssrc/mcp/tools.rssrc/search/chunker.rssrc/search/cli.rstests/calls_e2e.rstests/cli_ergonomics.rstests/digest_format.rstests/mcp_e2e.rstests/show_markdown.rstests/surface_e2e.rswiki/architecture.mdwiki/calls.mdwiki/file-filtering.mdwiki/search.md
| match crate::resolve_paths_for_mcp("run", &a.paths) { | ||
| Ok((paths, _)) => paths, | ||
| Err(e) => return CallResult::Error(e), | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
run discards its unresolved paths silently.
Every other tool in this file now reports partial misses via partial_note / with_missing_paths; here the missing half is dropped, so a caller passing one good and one bad path gets a clean-looking match list that never mentions the path it didn't search. MCP has no stderr, so the signal has to ride the payload.
🔧 Keep the missing list and surface it
- match crate::resolve_paths_for_mcp("run", &a.paths) {
- Ok((paths, _)) => paths,
- Err(e) => return CallResult::Error(e),
- }
+ match crate::resolve_paths_for_mcp("run", &a.paths) {
+ Ok((paths, miss)) => {
+ missing = miss;
+ paths
+ }
+ Err(e) => return CallResult::Error(e),
+ }Then prefix the text output with partial_note(&missing) and wrap the JSON returns in with_missing_paths(..., &missing).
🤖 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 `@src/mcp/tools.rs` around lines 1024 - 1027, Update the run tool’s
resolve_paths_for_mcp handling to retain both resolved and missing paths instead
of discarding the missing list. Prefix text responses with
partial_note(&missing), and wrap JSON responses with with_missing_paths(...,
&missing), while preserving the existing error handling for resolution failures.
| 7. Bare-name re-resolution: for every `Bare` edge, look up its name in the (now updated) symbol table. Single-match, no-receiver hits promote to `Resolved/Inferred` (pass B's rule, receiver suppression included); everything else goes through `resolve::disambiguate` — the *same* function pass C uses — so candidates are filtered to what the caller's file can reach through the dep graph, with a memoized per-file closure. Picks up two cases the partial path would otherwise miss: edges demoted in step 6 whose target moved to a different file, and pre-existing `Bare` edges in unchanged files that finally have a target because a *new* file in this delta defines it. | ||
|
|
||
| The invariant is parity — a partial update must produce the graph a cold build of the same content produces. This step used to assign the *unfiltered* symbol-table entry as an edge's candidate set, which broke that: it ran over the whole graph, so one edit to an unrelated file gave every bare edge in the project candidates the dep filter had ruled out. Measured on ast-bro itself, `callers CliError.new` reported 1023 unresolved sites cold and 1073 after appending a comment to `src/adapters/sql.rs` — every `Vec::new()` in the tree newly counted as a possible caller. Reusing pass C's own decision function is what keeps the two paths honest; `tests/calls_e2e.rs::incremental_update_matches_a_cold_build_of_the_same_content` pins it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Exact for the reused single-candidate Pass B result.
Line 313 calls this “pass B's rule,” but Pass B documents a single candidate as Resolved/Exact. Calling it Inferred contradicts the stated cold-build parity and misstates confidence semantics.
Proposed documentation fix
-7. Bare-name re-resolution: for every `Bare` edge, look up its name in the (now updated) symbol table. Single-match, no-receiver hits promote to `Resolved/Inferred` (pass B's rule, receiver suppression included);
+7. Bare-name re-resolution: for every `Bare` edge, look up its name in the (now updated) symbol table. Single-match, no-receiver hits promote to `Resolved/Exact` (pass B's rule, receiver suppression included);📝 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.
| 7. Bare-name re-resolution: for every `Bare` edge, look up its name in the (now updated) symbol table. Single-match, no-receiver hits promote to `Resolved/Inferred` (pass B's rule, receiver suppression included); everything else goes through `resolve::disambiguate` — the *same* function pass C uses — so candidates are filtered to what the caller's file can reach through the dep graph, with a memoized per-file closure. Picks up two cases the partial path would otherwise miss: edges demoted in step 6 whose target moved to a different file, and pre-existing `Bare` edges in unchanged files that finally have a target because a *new* file in this delta defines it. | |
| The invariant is parity — a partial update must produce the graph a cold build of the same content produces. This step used to assign the *unfiltered* symbol-table entry as an edge's candidate set, which broke that: it ran over the whole graph, so one edit to an unrelated file gave every bare edge in the project candidates the dep filter had ruled out. Measured on ast-bro itself, `callers CliError.new` reported 1023 unresolved sites cold and 1073 after appending a comment to `src/adapters/sql.rs` — every `Vec::new()` in the tree newly counted as a possible caller. Reusing pass C's own decision function is what keeps the two paths honest; `tests/calls_e2e.rs::incremental_update_matches_a_cold_build_of_the_same_content` pins it. | |
| 7. Bare-name re-resolution: for every `Bare` edge, look up its name in the (now updated) symbol table. Single-match, no-receiver hits promote to `Resolved/Exact` (pass B's rule, receiver suppression included); everything else goes through `resolve::disambiguate` — the *same* function pass C uses — so candidates are filtered to what the caller's file can reach through the dep graph, with a memoized per-file closure. Picks up two cases the partial path would otherwise miss: edges demoted in step 6 whose target moved to a different file, and pre-existing `Bare` edges in unchanged files that finally have a target because a *new* file in this delta defines it. | |
| The invariant is parity — a partial update must produce the graph a cold build of the same content produces. This step used to assign the *unfiltered* symbol-table entry as an edge's candidate set, which broke that: it ran over the whole graph, so one edit to an unrelated file gave every bare edge in the project candidates the dep filter had ruled out. Measured on ast-bro itself, `callers CliError.new` reported 1023 unresolved sites cold and 1073 after appending a comment to `src/adapters/sql.rs` — every `Vec::new()` in the tree newly counted as a possible caller. Reusing pass C's own decision function is what keeps the two paths honest; `tests/calls_e2e.rs::incremental_update_matches_a_cold_build_of_the_same_content` pins it. |
🤖 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 `@wiki/calls.md` around lines 313 - 315, Update the “Bare-name re-resolution”
documentation to describe single-match, no-receiver hits as promoting to
Resolved/Exact, matching Pass B’s documented result and confidence semantics;
leave the disambiguation behavior and parity explanation unchanged.
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 5 seconds. |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/calls/cli.rs (1)
261-297: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
totalconsistent across--depth.
depth <= 1extendscallees_one_hop, which returns every direct call-site edge, whiledepth >= 2uses BFS deduping by resolved target qn. That makes# N callee(s)change meaning acrossdepth 1→2/3and can make JSONtotalandshowndiverge unexpectedly for duplicate call sites to the same callee. Either extend the traversal contract for depth 1 to report total resolved targets or count it in the same reported-target terms.🤖 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 `@src/calls/cli.rs` around lines 261 - 297, Make the depth <= 1 path use the same deduplicated resolved-target counting semantics as the depth >= 2 path, while preserving all direct call-site edges for display. Update the total calculation around callees_one_hop and all_edges so duplicate call sites to one resolved callee contribute once, keeping JSON total and shown consistent across depths.
🧹 Nitpick comments (14)
src/impact.rs (1)
476-483: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the truncation-suffix formatting.
The
"showing {} — raise --limit to see the rest"phrasing is now hand-built in three places in this file (here, Line 686-695, Line 777-785) plussrc/deps/render.rs. A single helper (e.g.fn count_suffix(total: usize, shown: usize) -> String) keeps the agent-facing wording from drifting between sections.🤖 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 `@src/impact.rs` around lines 476 - 483, Extract the repeated truncation suffix formatting into a shared helper such as count_suffix(total: usize, shown: usize), then replace the hand-built “showing … raise --limit …” wording in this formatting block and the corresponding sections around the other affected-count messages, including the dependency renderer. Preserve the existing wording and values while centralizing its construction.tests/digest_format.rs (4)
437-445: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNegative assertions here don't strip the temp path.
rust_missing_pub_means_private_except_inherited_contexts(Line 526) deliberately replaces the fixture path before asserting absence, because the rendered header carries the random temp path. Applying the same strip here keeps these two assertions from depending on temp-dir naming.🤖 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/digest_format.rs` around lines 437 - 445, Update the test around the `without` result and its `--no-attrs`/`--no-lines` assertions to remove the temporary fixture path before checking for absent content, following the path-normalization approach used by `rust_missing_pub_means_private_except_inherited_contexts`. Keep the existing assertions and messages unchanged after normalization.
448-486: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also pinning the
projectedmetadata block.
render_json_map(src/core.rs:1573-1583) adds aprojectedobject whenever anything is stripped, and omits it otherwise. That's the field consumers guard on, but no assertion covers it here.🤖 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/digest_format.rs` around lines 448 - 486, Extend the json_honors_projection_flags test to assert the projected metadata block: the default payload must omit projected, while calls using --no-docs, --no-lines, or --no-attrs must include it.
533-535: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
gaindoesn't test visibility here.
digestdefaults toinclude_fields: false(DigestOptions::default(), src/core.rs:293-302), so the private field is hidden because fields are suppressed, not because it's non-pub. Themap --no-privatecheck at Line 539-546 is the one that actually exercises field visibility; consider droppinggainfrom this list or adding--include-fieldsto make the claim real.🤖 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/digest_format.rs` around lines 533 - 535, The visibility assertion in the digest test incorrectly includes gain while fields are disabled by default. Remove gain from the hidden-name list, or update the digest invocation to enable fields so the assertion actually verifies its non-public visibility; keep the existing map --no-private coverage unchanged.
412-418: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the absence of a cap note too.
contains("a()")proves the names survive but not that no cap fired. Addingassert!(!text.contains("+"))-style negative check (or matching"more member(s)") pins the actual claim of the 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 `@tests/digest_format.rs` around lines 412 - 418, Strengthen the assertions in the map detail test around run by verifying the output does not contain the cap-indicator text (such as the “more member(s)” note) after confirming a(), b(), and c() are present. Use the formatter’s actual cap-note wording rather than a broad symbol check.src/mcp/tools.rs (1)
640-647: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated partial-miss note prefixing across MCP tools.
partial_notereturns the note but each tool hand-rolls the samematch &path_note { Some(n) => format!("{}\n{}", n, out), None => out }prefixing, so text-mode note formatting can drift per tool. Add one helper (e.g.fn with_note(note: &Option<String>, out: String) -> String) next topartial_noteand use it everywhere.
src/mcp/tools.rs#L640-L647: replace thematch &path_noteblock inrun_digestwith the shared helper.src/mcp/tools.rs#L782-L785: replace thematch &path_noteblock inrun_implementswith the shared helper (and reuse it forrun_map'snoteclosure at Line 539-542).🤖 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 `@src/mcp/tools.rs` around lines 640 - 647, The partial-miss note prefixing is duplicated across MCP tools and should use one shared formatter. In src/mcp/tools.rs lines 640-647, replace run_digest’s path_note match with a helper such as with_note; in lines 782-785, make the same change in run_implements; also reuse the helper in run_map’s note closure at lines 539-542. Define the helper next to partial_note and preserve the existing note-before-output formatting.src/lib.rs (1)
538-546: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
map/digesthelp still advertisesPATHSas optional while the runtime rejects an empty list.
implementswas maderequired = trueso--helpand the runtime agree;MapArgs.pathsdeliberately stays optional to surface theno_inputenvelope. Both behaviours are defensible, butmap --helpprinting[PATHS]...whileast-bro mapexits 2 is the same mismatch that was fixed forimplements. Consider documenting the intent in the doc comment (orvalue_name) so the divergence is explicit.🤖 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 `@src/lib.rs` around lines 538 - 546, Update the MapArgs paths argument documentation or value_name configuration to explicitly state that paths may be omitted and empty input produces the no_input envelope, while preserving the existing runtime validation and optional argument behavior for both map and digest.src/calls/resolve.rs (2)
349-356: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
file.contains("/bin/")matches more than crate roots.Any path with a
bin/component (tools/bin/helper.rs,vendor/bin/x.rs) is treated as a crate root, so thecrate::arm can fire in a file that isn't one. The blast radius is small — anchored equality still requires the caller's own file to declare the fullcrate::P::namepath — but the check is cheap to tighten to Cargo's actual layout.♻️ Suggested tightening
fn is_crate_root(file: &str) -> bool { let name = file.rsplit('/').next().unwrap_or(file); - matches!(name, "lib.rs" | "main.rs") || file.contains("/bin/") + matches!(name, "lib.rs" | "main.rs") + || file.contains("src/bin/") + || file.starts_with("bin/") }🤖 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 `@src/calls/resolve.rs` around lines 349 - 356, Update is_crate_root so directory-based detection only recognizes Cargo binary crate roots directly under the repository’s src/bin directory, rather than any path containing a bin component. Preserve the existing lib.rs and main.rs checks, and keep paths such as tools/bin/helper.rs and vendor/bin/x.rs from being classified as crate roots.
358-377: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePython's
clsisn't in the self-like set.
cls.helper()inside a@classmethodnames the enclosing scope exactly likeself.helper(), so it currently misses same-file/import binding and falls through to pass B/C. Conservative (no wrongExact), but a cheap addition if Python classmethods matter.🤖 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 `@src/calls/resolve.rs` around lines 358 - 377, Add Python’s "cls" receiver to receiver_is_self_like so cls.helper() in classmethods follows the same enclosing-scope promotion path as self-like receivers. Update the function’s explanatory comment to document Python classmethod usage while preserving all existing receiver behavior.tests/cli_ergonomics.rs (2)
60-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the envelope parser.
This seven-line find-and-parse block is repeated verbatim seven times (Lines 60-64, 112-116, 210-214, 342-346, 366-370, 394-398, 426-430).
♻️ Suggested helper
+fn envelope(stderr: &str) -> serde_json::Value { + let line = stderr + .lines() + .find(|l| l.starts_with('{')) + .unwrap_or_else(|| panic!("json envelope on stderr:\n{stderr}")); + serde_json::from_str(line).expect("valid json") +}Each site then becomes
let doc = envelope(&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_ergonomics.rs` around lines 60 - 64, Extract the repeated stderr JSON lookup and deserialization into an `envelope` helper in the test module. Update the seven call sites in `tests/cli_ergonomics.rs` to use `let doc = envelope(&stderr);`, preserving the existing behavior of selecting the first line beginning with `{` and failing on missing or invalid JSON.
276-296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese two tests assert on the repo's own source size.
stdout.len() > 25_000forsrc/and "no hint" forsrc/hook/both break as the tree grows or shrinks, with a failure that says nothing about the hint logic.implements_existing_type_with_zero_impls_exits_0(Line 140-150) already went tempdir-local for this reason; a generated fixture (one large file above the threshold, one small dir below) would make both assertions self-contained.🤖 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_ergonomics.rs` around lines 276 - 296, Update map_on_large_directory_hints_at_digest_preset and map_on_small_input_stays_quiet to use a temporary, generated fixture rather than repo source paths and size assumptions. Create one directory containing a file that reliably exceeds the threshold and another small directory below it, then invoke run with those fixture paths while preserving the existing exit-code, stdout, and stderr hint assertions.src/adapters/rust.rs (1)
419-424: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOne boolean trap survives the fix.
The
Visdoc at Lines 1106-1108 introduces the enum to avoid unlabeled booleans, butis_method: boolsits right beside it —_function_to_decl(&item, src, true, Vis::Inherited)still reads as a puzzle at the call site. A second small enum (or derivingis_methodfromVis/caller context) would finish the job.🤖 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 `@src/adapters/rust.rs` around lines 419 - 424, The _function_to_decl signature still uses the unlabeled is_method boolean, leaving call sites ambiguous. Replace it with a small descriptive enum or derive the method status from existing Vis/caller context, then update all _function_to_decl callers to use the clearer representation while preserving declaration behavior.src/calls/mcp.rs (1)
46-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis unattributed policy is now duplicated in the CLI path.
The hide-ambiguous clear, the
unattributed_totalordering, the declarer gate againstMAX_DECLARERS_TO_LIST, and theUNATTRIBUTED_SAMPLE.min(limit)cap all appear verbatim insrc/calls/cli.rsrun_callers. Since the whole point of these rules is that both surfaces answer identically, a shared helper returning the builtrender::Unattributed(plus the hidden count for the caller to channel to stderr or response text) would keep them from drifting.🤖 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 `@src/calls/mcp.rs` around lines 46 - 84, Extract the duplicated unattributed-callers policy from the MCP flow and CLI run_callers into a shared helper that constructs render::Unattributed and returns the hidden count separately. Reuse this helper in both paths, preserving the include_ambiguous clearing, declarer gate using MAX_DECLARERS_TO_LIST, and UNATTRIBUTED_SAMPLE.min(limit) cap while leaving each surface’s stderr or response-note handling intact.tests/calls_e2e.rs (1)
1759-1763: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the JSON assertion for this negative.
"Caller::getCtx (Exact)"hard-codes the two-space gap inrender_callees_text's format string, so any rendering tweak turns this into a vacuous pass on the exact regression it guards. Later tests in this file (e.g. Lines 2070-2081) already assert on--jsonconfidence+targetinstead.🤖 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/calls_e2e.rs` around lines 1759 - 1763, The negative assertion in the relevant end-to-end test is coupled to render_callees_text’s spacing. Replace the raw-output contains check with a --json assertion that verifies the local homonym does not produce an entry combining target Caller::getCtx with Exact confidence, following the existing JSON assertion pattern later in the file.
🤖 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 `@README.md`:
- Line 454: Update the README’s “Machine-readable rejections” statement to
account for the `impact --mode <bogus>` exception, or modify the
`Commands::Impact` handler to route invalid modes through `CliError` with
`ErrorKind::BadArgument` so `--json` emits the `ast-bro.error.v1` envelope.
Ensure the documented claim matches the implemented behavior.
In `@src/calls/mcp.rs`:
- Around line 196-198: The MCP JSON callees path in the json branch of the calls
handler omits the promised truncation metadata. Compute the frontier-truncated
state in the existing depth <= 1 and depth > 1 traversal branches using the same
logic as run_callees, then pass it through render_callees_json (and its callers)
so the payload includes total and frontier_truncated while preserving the
existing schema, target, depth, and matches fields.
In `@src/calls/traverse.rs`:
- Around line 104-142: Update callers_multi’s traversal around the seed queue
and while-loop to process entries by total-depth layers rather than relying on
FIFO ordering across mixed base depths. Ensure all nodes at depth d are expanded
before any depth d+1 entries, so first visits and predicate reports use minimum
total depth and entries beyond max_total_depth are excluded consistently.
In `@src/lib.rs`:
- Around line 1315-1331: Extend the target type check in the implements handling
block to recognize delegate declarations by including "delegate" alongside the
existing class, struct, interface, record, and enum kinds. Keep the
SymbolNotFound behavior unchanged for targets that still do not exist.
In `@src/mcp/tools.rs`:
- Around line 786-789: The output header in the find-implementations flow should
reflect the computed transitive setting instead of always claiming transitive
results. Update the format construction near find_implementations to include the
“incl. transitive” wording only when transitive is enabled, while preserving the
existing match count and target text.
In `@tests/calls_e2e.rs`:
- Around line 2576-2592: Extend the parity assertions in the test around the
existing total-count key loop to explicitly compare per-edge confidence for
matched caller edges between incremental and cold results. Include
unchanged-edge and newly resolved-edge cases so the comparison detects
differences between preserved original confidence and Inferred confidence, while
retaining the existing count-field checks.
In `@tests/cli_ergonomics.rs`:
- Around line 197-203: Update map_preset_digest_json_matches_digest_json to
retain and assert successful exit statuses for both run invocations, and verify
each stdout is non-empty before comparing them. Keep the existing byte-identical
output assertion after these checks.
In `@tests/digest_format.rs`:
- Around line 615-620: Rename the overlapping fixture function identifiers in
the digest-format test to non-overlapping names, including the
`self_only`/`in_self_only` pair and `crate_wide`/`in_crate_wide` pair. Update
the corresponding positive and negative assertion lists so each visibility case
is checked independently.
---
Outside diff comments:
In `@src/calls/cli.rs`:
- Around line 261-297: Make the depth <= 1 path use the same deduplicated
resolved-target counting semantics as the depth >= 2 path, while preserving all
direct call-site edges for display. Update the total calculation around
callees_one_hop and all_edges so duplicate call sites to one resolved callee
contribute once, keeping JSON total and shown consistent across depths.
---
Nitpick comments:
In `@src/adapters/rust.rs`:
- Around line 419-424: The _function_to_decl signature still uses the unlabeled
is_method boolean, leaving call sites ambiguous. Replace it with a small
descriptive enum or derive the method status from existing Vis/caller context,
then update all _function_to_decl callers to use the clearer representation
while preserving declaration behavior.
In `@src/calls/mcp.rs`:
- Around line 46-84: Extract the duplicated unattributed-callers policy from the
MCP flow and CLI run_callers into a shared helper that constructs
render::Unattributed and returns the hidden count separately. Reuse this helper
in both paths, preserving the include_ambiguous clearing, declarer gate using
MAX_DECLARERS_TO_LIST, and UNATTRIBUTED_SAMPLE.min(limit) cap while leaving each
surface’s stderr or response-note handling intact.
In `@src/calls/resolve.rs`:
- Around line 349-356: Update is_crate_root so directory-based detection only
recognizes Cargo binary crate roots directly under the repository’s src/bin
directory, rather than any path containing a bin component. Preserve the
existing lib.rs and main.rs checks, and keep paths such as tools/bin/helper.rs
and vendor/bin/x.rs from being classified as crate roots.
- Around line 358-377: Add Python’s "cls" receiver to receiver_is_self_like so
cls.helper() in classmethods follows the same enclosing-scope promotion path as
self-like receivers. Update the function’s explanatory comment to document
Python classmethod usage while preserving all existing receiver behavior.
In `@src/impact.rs`:
- Around line 476-483: Extract the repeated truncation suffix formatting into a
shared helper such as count_suffix(total: usize, shown: usize), then replace the
hand-built “showing … raise --limit …” wording in this formatting block and the
corresponding sections around the other affected-count messages, including the
dependency renderer. Preserve the existing wording and values while centralizing
its construction.
In `@src/lib.rs`:
- Around line 538-546: Update the MapArgs paths argument documentation or
value_name configuration to explicitly state that paths may be omitted and empty
input produces the no_input envelope, while preserving the existing runtime
validation and optional argument behavior for both map and digest.
In `@src/mcp/tools.rs`:
- Around line 640-647: The partial-miss note prefixing is duplicated across MCP
tools and should use one shared formatter. In src/mcp/tools.rs lines 640-647,
replace run_digest’s path_note match with a helper such as with_note; in lines
782-785, make the same change in run_implements; also reuse the helper in
run_map’s note closure at lines 539-542. Define the helper next to partial_note
and preserve the existing note-before-output formatting.
In `@tests/calls_e2e.rs`:
- Around line 1759-1763: The negative assertion in the relevant end-to-end test
is coupled to render_callees_text’s spacing. Replace the raw-output contains
check with a --json assertion that verifies the local homonym does not produce
an entry combining target Caller::getCtx with Exact confidence, following the
existing JSON assertion pattern later in the file.
In `@tests/cli_ergonomics.rs`:
- Around line 60-64: Extract the repeated stderr JSON lookup and deserialization
into an `envelope` helper in the test module. Update the seven call sites in
`tests/cli_ergonomics.rs` to use `let doc = envelope(&stderr);`, preserving the
existing behavior of selecting the first line beginning with `{` and failing on
missing or invalid JSON.
- Around line 276-296: Update map_on_large_directory_hints_at_digest_preset and
map_on_small_input_stays_quiet to use a temporary, generated fixture rather than
repo source paths and size assumptions. Create one directory containing a file
that reliably exceeds the threshold and another small directory below it, then
invoke run with those fixture paths while preserving the existing exit-code,
stdout, and stderr hint assertions.
In `@tests/digest_format.rs`:
- Around line 437-445: Update the test around the `without` result and its
`--no-attrs`/`--no-lines` assertions to remove the temporary fixture path before
checking for absent content, following the path-normalization approach used by
`rust_missing_pub_means_private_except_inherited_contexts`. Keep the existing
assertions and messages unchanged after normalization.
- Around line 448-486: Extend the json_honors_projection_flags test to assert
the projected metadata block: the default payload must omit projected, while
calls using --no-docs, --no-lines, or --no-attrs must include it.
- Around line 533-535: The visibility assertion in the digest test incorrectly
includes gain while fields are disabled by default. Remove gain from the
hidden-name list, or update the digest invocation to enable fields so the
assertion actually verifies its non-public visibility; keep the existing map
--no-private coverage unchanged.
- Around line 412-418: Strengthen the assertions in the map detail test around
run by verifying the output does not contain the cap-indicator text (such as the
“more member(s)” note) after confirming a(), b(), and c() are present. Use the
formatter’s actual cap-note wording rather than a broad symbol check.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fdfd21be-6f6c-45d3-82cb-266e683c9cf4
📒 Files selected for processing (33)
README.mdskills/ast-bro/SKILL.mdsrc/adapters/php.rssrc/adapters/rust.rssrc/calls/cli.rssrc/calls/mcp.rssrc/calls/render.rssrc/calls/resolve.rssrc/calls/trace.rssrc/calls/traverse.rssrc/cli_error.rssrc/context.rssrc/core.rssrc/deps/cli.rssrc/deps/render.rssrc/graph_cache/delta.rssrc/graph_cache/mod.rssrc/hook/decide.rssrc/impact.rssrc/lib.rssrc/mcp/tools.rssrc/search/chunker.rssrc/search/cli.rstests/calls_e2e.rstests/cli_ergonomics.rstests/digest_format.rstests/mcp_e2e.rstests/show_markdown.rstests/surface_e2e.rswiki/architecture.mdwiki/calls.mdwiki/file-filtering.mdwiki/search.md
| let target_exists = results.iter().any(|r| { | ||
| crate::core::find_symbols(r, target).iter().any(|m| { | ||
| matches!( | ||
| m.kind.as_str(), | ||
| "class" | "struct" | "interface" | "record" | "enum" | ||
| ) | ||
| }) | ||
| }); | ||
| if !target_exists { | ||
| crate::cli_error::CliError::new( | ||
| "implements", | ||
| crate::cli_error::ErrorKind::SymbolNotFound, | ||
| format!("no type named '{}' in the given path(s)", target), | ||
| ) | ||
| .hint("A 0-match answer is only reported for types that exist. Check the spelling or widen the search path.") | ||
| .exit(*json); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Which DeclarationKind values do adapters assign to type declarations?
rg -nP --type=rust -C2 'kind:\s*DeclarationKind::(Class|Struct|Interface|Record|Enum|Delegate|Namespace)' src/adapters | head -60Repository: aeroxy/ast-bro
Length of output: 2766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DeclarationKind definitions =="
rg -n "pub enum DeclarationKind|enum DeclarationKind|enum class DeclarationKind|DeclarationKind::[A-Z_" src -g '*.rs' -g '*.ts' -g '*.js' | head -80
echo
echo "== adapter declaration construction sites =="
for f in $(git ls-files src/adapters | sort); do
echo "--- $f"
rg -n "Declaration\\s*{|kind:\\s*DeclarationKind::" "$f" -C 1 | head -120
done
echo
echo "== implements type check implementation =="
sed -n '1280,1340p' src/lib.rs | cat -nRepository: aeroxy/ast-bro
Length of output: 444
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all DeclarationKind enum members =="
rg -n "enum DeclarationKind" src || true
rg -n "declare_enum!\\(DeclarationKind|EnumDecl!" src || true
echo
echo "== declaration-visitor helper symbols =="
rg -n "parse_?declaration|visit_?declaration|DeclarationKind|Declaration\\s*=" src/adapters src/core -g '*.rs' | head -200
echo
echo "== adapter function signatures and relevant node types =="
rg -n "fn \\w+\\(|const .*DEC|kind: DeclarationKind::" src/adapters -C 2 | head -220Repository: aeroxy/ast-bro
Length of output: 13289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DeclarationKind definitions =="
rg -n "pub enum DeclarationKind|enum DeclarationKind|EnumDecl|declare_enum" src -g '*.rs' | head -120
echo
echo "== declaration creation kind assignments =="
python3 - <<'PY'
import re, subprocess, pathlib
# simple scanner because exact syntax may vary
for path in pathlib.Path('src').rglob('*'):
if path.suffix not in {'.rs'}:
continue
try:
text = path.read_text()
except UnicodeDecodeError:
continue
for m in re.finditer(r'kind:\s*DeclarationKind::([A-Za-z0-9_]+)', text):
start = max(0, text.rfind('\n', 0, m.start())-200)
snippet = text[m.start():text.find('\n}', m.end())+2]
print(f"{path}:{m.start()+1}: {m.group(1)} :: {snippet.splitlines()[0].strip()}")
PY
echo
echo "== implementation allowlist =="
sed -n '1300,1335p' src/lib.rs | cat -nRepository: aeroxy/ast-bro
Length of output: 7474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DeclarationKind definitions/imports =="
rg -n "pub enum DeclarationKind|enum DeclarationKind|EnumDecl|declare_enum|DeclarationKind" src/core src/adapters src/lib.rs -g '*.rs' | head -200
echo
echo "== declaration construction snippets by file =="
for f in $(git ls-files 'src/adapters/*.rs' | sort); do
echo "==$f"
rg -n "kind:\s*DeclarationKind::" "$f" -C 2
done
echo
echo "== type-filter logic =="
sed -n '1312,1332p' src/lib.rs | cat -nRepository: aeroxy/ast-bro
Length of output: 13208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== core.rs relevant definitions =="
sed -n '1,80p' src/core.rs | cat -n
echo
echo "== find_symbols implementation =="
rg -n "fn find_symbols|pub fn find_symbols|find_symbols" src -g '*.rs' -C 8 | head -200
echo
echo "== find_implementations implementation =="
rg -n "fn find_implementations|pub fn find_implementations|find_implementations" src -g '*.rs' -C 12 | head -240
echo
echo "== TypeScript delegate/namespace target declaration construction snippets =="
rg -n "delegate\\b|namespace\\b|interface\\b|type\\b" src/adapters/typescript.rs -C 2 | sed -n '1,220p'Repository: aeroxy/ast-bro
Length of output: 17989
Include declared type aliases in the implements target check.
DeclarationKind::Delegate emits kind: "delegate" in several adapters, and existing adapter-level code treats delegates as targetable type declarations. Since the current check only recognizes class|struct|interface|record|enum, implements X can exit 2 instead of returning 0 matches when X is a declared alias/delegate that exists but has no implementations.
🤖 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 `@src/lib.rs` around lines 1315 - 1331, Extend the target type check in the
implements handling block to recognize delegate declarations by including
"delegate" alongside the existing class, struct, interface, record, and enum
kinds. Keep the SymbolNotFound behavior unchanged for targets that still do not
exist.
| out.push_str(&format!( | ||
| "# {} match(es) for '{}' (incl. transitive):\n", | ||
| matches.len(), a.target | ||
| ); | ||
| )); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Header claims transitive results even when direct: true.
transitive is computed at Line 773 and honored by find_implementations, but the text header hard-codes (incl. transitive). An agent asking for direct subtypes gets output labelled as including transitive ones.
🔧 Proposed fix
out.push_str(&format!(
- "# {} match(es) for '{}' (incl. transitive):\n",
- matches.len(), a.target
+ "# {} match(es) for '{}'{}:\n",
+ matches.len(),
+ a.target,
+ if transitive { " (incl. transitive)" } else { " (direct only)" }
));📝 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.
| out.push_str(&format!( | |
| "# {} match(es) for '{}' (incl. transitive):\n", | |
| matches.len(), a.target | |
| ); | |
| )); | |
| out.push_str(&format!( | |
| "# {} match(es) for '{}'{}:\n", | |
| matches.len(), | |
| a.target, | |
| if transitive { " (incl. transitive)" } else { " (direct only)" } | |
| )); |
🤖 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 `@src/mcp/tools.rs` around lines 786 - 789, The output header in the
find-implementations flow should reflect the computed transitive setting instead
of always claiming transitive results. Update the format construction near
find_implementations to include the “incl. transitive” wording only when
transitive is enabled, while preserving the existing match count and target
text.
| for key in [ | ||
| "total", | ||
| "unattributed_total", | ||
| "unattributed_declarers", | ||
| "unattributed_suppressed", | ||
| ] { | ||
| assert_eq!( | ||
| incremental[key], cold_again[key], | ||
| "{key} diverged between a partial update and a cold build of the same content:\n\ | ||
| incremental={incremental}\ncold={cold_again}" | ||
| ); | ||
| assert_eq!( | ||
| cold[key], cold_again[key], | ||
| "{key} changed across an edit that touched no relevant file:\n\ | ||
| before={cold}\nafter={cold_again}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# What does the delta detector compare — mtime, size, or hash?
rg -nP --type=rust -C4 '\b(mtime|modified|len\(\)|hash)\b' src/search/cache.rs src/graph_cache/ | head -80Repository: aeroxy/ast-bro
Length of output: 4940
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== Locate relevant files ==\n'
git ls-files | rg 'tests/calls_e2e\.rs|src/graph_cache/delta\.rs|src/search/cache\.rs' || true
printf '\n== calls_e2e parity test section ==\n'
sed -n '2510,2595p' tests/calls_e2e.rs | cat -n
printf '\n== delta.rs confidence mentions and relevant functions ==\n'
rg -n -C 5 'Inferred|Exact|confidence|Partial|total|unattributed' src/graph_cache/delta.rs | head -240
printf '\n== search cache rebuild path flags ==\n'
rg -n -C 4 'PartialUpdate|Rebuild|requires_rebuild|rebuild|compute_delta|mtime_only|modified' src tests -g '*.rs' | head -320Repository: aeroxy/ast-bro
Length of output: 22928
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== locate callers graph json definitions ==\n'
rg -n -C 4 'struct .*Graph|confidenc|confidence|unattributed|total.*matches|Output|QueryResult|match.*json|serde_json::to|rebuild' src/calls src/cmd src -g '*.rs' | head -400
printf '\n== callers output builders ==\n'
rg -n -C 8 'fn calls|command calls|callers|build.*calls|calls\(' src -g '*.rs'Repository: aeroxy/ast-bro
Length of output: 22018
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== callers json payload field names ==\n'
sed -n '620,710p' src/calls/render.rs | cat -n
printf '\n== exact callers output builder invocations ==\n'
rg -n -C 4 'render_callers_json|render_callers_json_extended|Unattributed|json_unattributed|json_unattributed_fields|unattributed' src/calls src/cli src -g '*.rs' | head -320Repository: aeroxy/ast-bro
Length of output: 21573
Check the relevant delta-confidence comparison explicitly.
The current parity keys omit per-edge confidence, and delta.rs preserves unchanged-file edges with their original confidence while newly resolved ones may get Inferred; this test can pass even when incremental/cold disagree. Compare matched caller edges’ confidence alongside the count fields.
🤖 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/calls_e2e.rs` around lines 2576 - 2592, Extend the parity assertions in
the test around the existing total-count key loop to explicitly compare per-edge
confidence for matched caller edges between incremental and cold results.
Include unchanged-edge and newly resolved-edge cases so the comparison detects
differences between preserved original confidence and Inferred confidence, while
retaining the existing count-field checks.
| #[test] | ||
| fn map_preset_digest_json_matches_digest_json() { | ||
| // #37: `digest` is exactly `map --preset digest`. | ||
| let (_, a, _) = run(&["map", "src/core.rs", "--preset", "digest", "--json", "--compact"]); | ||
| let (_, b, _) = run(&["digest", "src/core.rs", "--json", "--compact"]); | ||
| assert_eq!(a, b, "digest alias must be byte-identical to the preset"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This equality passes vacuously if both commands start failing.
Both exit codes are discarded, so two empty stdouts compare equal. Assert success and non-empty output before comparing.
💚 Proposed fix
- let (_, a, _) = run(&["map", "src/core.rs", "--preset", "digest", "--json", "--compact"]);
- let (_, b, _) = run(&["digest", "src/core.rs", "--json", "--compact"]);
+ let (ca, a, ea) = run(&["map", "src/core.rs", "--preset", "digest", "--json", "--compact"]);
+ let (cb, b, _) = run(&["digest", "src/core.rs", "--json", "--compact"]);
+ assert_eq!(ca, Some(0), "stderr:\n{ea}");
+ assert_eq!(cb, Some(0));
+ assert!(!a.is_empty(), "empty payloads compare equal for the wrong reason");
assert_eq!(a, b, "digest alias must be byte-identical to the preset");📝 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.
| #[test] | |
| fn map_preset_digest_json_matches_digest_json() { | |
| // #37: `digest` is exactly `map --preset digest`. | |
| let (_, a, _) = run(&["map", "src/core.rs", "--preset", "digest", "--json", "--compact"]); | |
| let (_, b, _) = run(&["digest", "src/core.rs", "--json", "--compact"]); | |
| assert_eq!(a, b, "digest alias must be byte-identical to the preset"); | |
| } | |
| #[test] | |
| fn map_preset_digest_json_matches_digest_json() { | |
| // `#37`: `digest` is exactly `map --preset digest`. | |
| let (ca, a, ea) = run(&["map", "src/core.rs", "--preset", "digest", "--json", "--compact"]); | |
| let (cb, b, _) = run(&["digest", "src/core.rs", "--json", "--compact"]); | |
| assert_eq!(ca, Some(0), "stderr:\n{ea}"); | |
| assert_eq!(cb, Some(0)); | |
| assert!(!a.is_empty(), "empty payloads compare equal for the wrong reason"); | |
| assert_eq!(a, b, "digest alias must be byte-identical to the preset"); | |
| } |
🤖 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_ergonomics.rs` around lines 197 - 203, Update
map_preset_digest_json_matches_digest_json to retain and assert successful exit
statuses for both run invocations, and verify each stdout is non-empty before
comparing them. Keep the existing byte-identical output assertion after these
checks.
| for hidden in ["self_only", "in_self_only"] { | ||
| assert!(!out.contains(hidden), "`{hidden}` is private-equivalent:\n{out}"); | ||
| } | ||
| for shown in ["crate_wide", "parent_wide", "in_crate_wide", "open"] { | ||
| assert!(out.contains(shown), "`{shown}` must stay visible:\n{out}"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Overlapping fixture names make these assertions unable to fail independently.
"crate_wide" is a substring of "in_crate_wide", so the positive assertion for pub(crate) fn crate_wide passes as long as in_crate_wide renders — a regression that hides only pub(crate) would go undetected. Same overlap between self_only and in_self_only on the negative side. Rename the fixture functions to non-overlapping identifiers (e.g. crate_wide / in_crate_form).
🔧 Proposed fixture rename
- pub(self) fn self_only() {}\n\
- pub(in self) fn in_self_only() {}\n\
+ pub(self) fn self_only() {}\n\
+ pub(in self) fn scoped_self_form() {}\n\
pub(crate) fn crate_wide() {}\n\
pub(super) fn parent_wide() {}\n\
- pub(in crate) fn in_crate_wide() {}\n\
+ pub(in crate) fn scoped_crate_form() {}\n\and update the two assertion lists accordingly.
🤖 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/digest_format.rs` around lines 615 - 620, Rename the overlapping
fixture function identifiers in the digest-format test to non-overlapping names,
including the `self_only`/`in_self_only` pair and `crate_wide`/`in_crate_wide`
pair. Update the corresponding positive and negative assertion lists so each
visibility case is checked independently.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/calls/render.rs (1)
747-756: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake MCP callees emit the same
callees.v1truncation fields as the CLI.
src/calls/mcp.rs::run_callees_textusesrender_callees_json(...), while the CLI usesrender_callees_json_extended(...); only the extended payload includestotal,truncated, andfrontier_truncatedforast-bro.callees.v1. A consumer guarding ontruncatedgets a different surface for the same schema version. Thread equivalent truncation state through both callee JSON paths like the caller JSON paths do.🤖 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 `@src/calls/render.rs` around lines 747 - 756, Update the MCP callee flow in run_callees_text and its render_callees_json path to carry the same truncation state used by render_callees_json_extended and the caller JSON paths. Ensure ast-bro.callees.v1 always emits total, truncated, and frontier_truncated consistently with the CLI, preserving the existing calculations for shown results and frontier truncation.
♻️ Duplicate comments (3)
src/lib.rs (1)
1315-1331: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winType-declaration allowlist still omits
delegate(and other adapter type kinds).
DeclarationKind::Delegaterenders as"delegate"(src/core.rs:54-76), soimplements SomeDelegateon an existing declared delegate exits 2 (symbol_not_found) instead of returning a legitimate 0-match answer.🔧 Proposed fix
matches!( m.kind.as_str(), - "class" | "struct" | "interface" | "record" | "enum" + "class" | "struct" | "interface" | "record" | "enum" | "delegate" )🤖 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 `@src/lib.rs` around lines 1315 - 1331, Update the target type-kind allowlist in the `target_exists` check to include `"delegate"` and any other declaration kinds rendered by `DeclarationKind` that represent valid type declarations, so existing delegates proceed to the legitimate 0-match result instead of `SymbolNotFound`.src/calls/traverse.rs (1)
104-142: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe "sorted seeds + FIFO ⇒ depth-monotonic" invariant only holds while seed base depths differ by at most 1.
With seeds
(A,1)and(B,5), poppingApushes its callers at depth 2 behindB(5), so a node reachable at total depth 2 fromAcan be first visited (and reported, or dropped abovemax_total_depth) fromB's deeper path.impactcurrently only seeds depths 1 and 2, so this is latent rather than live — but the comment states the invariant unconditionally, and any future seed at depth 3+ silently breaks min-depth reporting. Either expand one total-depth layer at a time (BTreeMap<usize, Vec<Qn>>), or state the ≤1-spread precondition and debug-assert it.🤖 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 `@src/calls/traverse.rs` around lines 104 - 142, Fix the traversal ordering in the function containing the sorted seeds and FIFO queue: ensure nodes are expanded in globally nondecreasing total depth even when seed depths differ by more than one, such as by processing depth buckets with a BTreeMap. Preserve minimum-depth first visitation, predicate-based reporting, and max_total_depth handling; do not rely on the current unconditional sorted-seeds/FIFO invariant.README.md (1)
454-454: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
impact --mode <bogus>is still an exception to this claim.The
Commands::Impacthandler rejects an unknown--modewith a bare# note:on stderr plusstd::process::exit(2)— noast-bro.error.v1envelope — so a consumer keying onschema == "ast-bro.error.v1"sees nothing. Route it throughCliError/ErrorKind::BadArgumentor narrow the wording here.🤖 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 `@README.md` at line 454, Update the Commands::Impact handling for an invalid --mode value so it reports through CliError using ErrorKind::BadArgument and emits the documented ast-bro.error.v1 envelope under --json, instead of printing a bare note and exiting directly; alternatively narrow the README claim to exclude this case.
🧹 Nitpick comments (4)
src/lib.rs (1)
790-883: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueSubcommand attribution in the envelope is a positional guess.
raw.iter().find(|a| !a.starts_with('-'))picks the first non-flag token, which for a bad value (ast-bro callers foo --depth abc) yields the subcommand correctly, but for a global-flag-first invocation of an unknown subcommand it reports whatever token appears first — including a bare value. Clap already knows the matched subcommand path for most error kinds viaContextKind::InvalidSubcommand/Error::cmd; using the raw args only as a fallback would keepcommandinast-bro.error.v1trustworthy for consumers keying on it.🤖 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 `@src/lib.rs` around lines 790 - 883, Update subcommand attribution in exit_with_parse_error to use clap’s parsed command context, preferring ContextKind::InvalidSubcommand or the command associated with Error::cmd when available. Retain raw-argument scanning only as a fallback, and avoid treating bare values as the command for global-flag-first unknown-subcommand errors; ensure the ast-bro.error.v1 envelope’s command remains the actual subcommand.src/calls/resolve.rs (2)
115-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider extracting this receiver-resolution closure into a named function.
The
and_thenclosure now spans ~125 lines, declares a localenum SelfRel, and holds three distinct resolution strategies (crate-anchored, self/super walk, suffix match with caller-scope tie-break). Lifting it tofn resolve_scoped_receiver(recv, bare_name, caller, defined) -> Option<Qn>would make each strategy independently testable and keep thefor raw in fp.raw_edgesloop readable.🤖 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 `@src/calls/resolve.rs` around lines 115 - 241, Extract the receiver-resolution `and_then` closure into a named `resolve_scoped_receiver` function accepting the receiver, bare name, caller, and defined QNs. Move the `SelfRel` enum and crate-anchored, self/super-walk, and suffix tie-break strategies into that function without changing their behavior, then call it from the raw-edge loop so the loop remains concise and each strategy is independently testable.
353-356: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
is_crate_root's/bin/check is broader than "crate root".
file.contains("/bin/")matches any path with abincomponent (e.g.tools/bin/helper.rs,crates/x/src/bin/y.rsis intended butscripts/bin/gen.rsis not). Restricting it tosrc/bin/would match the Cargo layout the doc comment describes. The downstream anchored-equality check limits the blast radius, so this is a tightening rather than a live bug.♻️ Proposed tightening
- matches!(name, "lib.rs" | "main.rs") || file.contains("/bin/") + matches!(name, "lib.rs" | "main.rs") || file.contains("src/bin/")🤖 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 `@src/calls/resolve.rs` around lines 353 - 356, Update is_crate_root so the binary crate-root condition matches only paths containing the Cargo-style /src/bin/ component, while preserving the existing lib.rs and main.rs checks.src/core.rs (1)
1440-1447: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit:
from_ref(..).pop()recursion allocates aVecper child.Extracting the single-declaration case into a
_filter_one(d, opts, dropped) -> Option<Declaration>helper (with_filter_declsmapping over it) removes the per-child allocation and reads more directly.🤖 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 `@src/core.rs` around lines 1440 - 1447, Refactor the recursion around _filter_decls to add a _filter_one helper returning Option<Declaration> for a single declaration, then have _filter_decls map over _filter_one. Replace the per-child std::slice::from_ref(c) and pop() call in the children construction with direct _filter_one usage, preserving the existing filtering and depth-capping behavior.
🤖 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/impact.rs`:
- Around line 606-612: Apply the existing --limit display-capping behavior to
the forward “→ calls” section constructed here and the “→ imports” section near
the referenced code: preserve the uncapped count in total, limit the entries
shown, and set truncated based on whether entries were omitted, matching the
implementation used by the capped dependent/transitive/reverse-import sections.
In `@tests/mcp_e2e.rs`:
- Line 73: Replace the Unix-specific `/nope/...` missing-path fixtures in the
MCP requests and related assertions with a platform-independent nonexistent name
such as `gone.rs`; update all corresponding `missing` and `missing_paths`
expectations in the affected test cases to match.
---
Outside diff comments:
In `@src/calls/render.rs`:
- Around line 747-756: Update the MCP callee flow in run_callees_text and its
render_callees_json path to carry the same truncation state used by
render_callees_json_extended and the caller JSON paths. Ensure
ast-bro.callees.v1 always emits total, truncated, and frontier_truncated
consistently with the CLI, preserving the existing calculations for shown
results and frontier truncation.
---
Duplicate comments:
In `@README.md`:
- Line 454: Update the Commands::Impact handling for an invalid --mode value so
it reports through CliError using ErrorKind::BadArgument and emits the
documented ast-bro.error.v1 envelope under --json, instead of printing a bare
note and exiting directly; alternatively narrow the README claim to exclude this
case.
In `@src/calls/traverse.rs`:
- Around line 104-142: Fix the traversal ordering in the function containing the
sorted seeds and FIFO queue: ensure nodes are expanded in globally nondecreasing
total depth even when seed depths differ by more than one, such as by processing
depth buckets with a BTreeMap. Preserve minimum-depth first visitation,
predicate-based reporting, and max_total_depth handling; do not rely on the
current unconditional sorted-seeds/FIFO invariant.
In `@src/lib.rs`:
- Around line 1315-1331: Update the target type-kind allowlist in the
`target_exists` check to include `"delegate"` and any other declaration kinds
rendered by `DeclarationKind` that represent valid type declarations, so
existing delegates proceed to the legitimate 0-match result instead of
`SymbolNotFound`.
---
Nitpick comments:
In `@src/calls/resolve.rs`:
- Around line 115-241: Extract the receiver-resolution `and_then` closure into a
named `resolve_scoped_receiver` function accepting the receiver, bare name,
caller, and defined QNs. Move the `SelfRel` enum and crate-anchored,
self/super-walk, and suffix tie-break strategies into that function without
changing their behavior, then call it from the raw-edge loop so the loop remains
concise and each strategy is independently testable.
- Around line 353-356: Update is_crate_root so the binary crate-root condition
matches only paths containing the Cargo-style /src/bin/ component, while
preserving the existing lib.rs and main.rs checks.
In `@src/core.rs`:
- Around line 1440-1447: Refactor the recursion around _filter_decls to add a
_filter_one helper returning Option<Declaration> for a single declaration, then
have _filter_decls map over _filter_one. Replace the per-child
std::slice::from_ref(c) and pop() call in the children construction with direct
_filter_one usage, preserving the existing filtering and depth-capping behavior.
In `@src/lib.rs`:
- Around line 790-883: Update subcommand attribution in exit_with_parse_error to
use clap’s parsed command context, preferring ContextKind::InvalidSubcommand or
the command associated with Error::cmd when available. Retain raw-argument
scanning only as a fallback, and avoid treating bare values as the command for
global-flag-first unknown-subcommand errors; ensure the ast-bro.error.v1
envelope’s command remains the actual subcommand.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fdfd21be-6f6c-45d3-82cb-266e683c9cf4
📒 Files selected for processing (33)
README.mdskills/ast-bro/SKILL.mdsrc/adapters/php.rssrc/adapters/rust.rssrc/calls/cli.rssrc/calls/mcp.rssrc/calls/render.rssrc/calls/resolve.rssrc/calls/trace.rssrc/calls/traverse.rssrc/cli_error.rssrc/context.rssrc/core.rssrc/deps/cli.rssrc/deps/render.rssrc/graph_cache/delta.rssrc/graph_cache/mod.rssrc/hook/decide.rssrc/impact.rssrc/lib.rssrc/mcp/tools.rssrc/search/chunker.rssrc/search/cli.rstests/calls_e2e.rstests/cli_ergonomics.rstests/digest_format.rstests/mcp_e2e.rstests/show_markdown.rstests/surface_e2e.rswiki/architecture.mdwiki/calls.mdwiki/file-filtering.mdwiki/search.md
| ImpactSection { | ||
| title: format!("→ calls ({})", entries.len()), | ||
| total: entries.len(), | ||
| truncated: false, | ||
| entries, | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Forward sections hard-code truncated: false and never apply --limit.
--limit's help text says "Cap how many results are displayed per section", and the dependents/transitive/reverse-import sections now honour it, but → calls here and → imports (Lines 736-739) emit every entry with total: entries.len(), truncated: false. On a fan-out-heavy symbol that section alone can dwarf the capped ones. Either cap these two the same way, or narrow the flag's help text to the sections it actually bounds.
🤖 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 `@src/impact.rs` around lines 606 - 612, Apply the existing --limit
display-capping behavior to the forward “→ calls” section constructed here and
the “→ imports” section near the referenced code: preserve the uncapped count in
total, limit the entries shown, and set truncated based on whether entries were
omitted, matching the implementation used by the capped
dependent/transitive/reverse-import sections.
| let resp = call_tool( | ||
| tmp.path(), | ||
| "map", | ||
| serde_json::json!({"paths": ["/nope/missing-xyz.rs"]}), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'Files:'
git ls-files | rg '(^|/)tests/mcp_e2e\.rs$|mcp' || true
echo
echo 'Target excerpt:'
if [ -f tests/mcp_e2e.rs ]; then
nl -ba tests/mcp_e2e.rs | sed -n '55,170p'
else
echo "tests/mcp_e2e.rs not found"
fi
echo
echo 'Search for missing_paths and /nope:'
rg -n "missing_paths|/nope|gone\.rs|slash|paths" tests/mcp_e2e.rs tests src .github 2>/dev/null || trueRepository: aeroxy/ast-bro
Length of output: 321
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'Target excerpt with cat:'
cat -n tests/mcp_e2e.rs | sed -n '60,175p'
echo
echo 'Search for missing_paths and /nope:'
rg -n "missing_paths|/nope|gone\.rs|slash|paths" tests/mcp_e2e.rs src || true
echo
echo 'Read relevant MCP implementation snippets:'
for f in src/search/mcp.rs src/calls/mcp.rs src/mcp/tools.rs src/mcp/protocol.rs src/mcp/mod.rs; do
if [ -f "$f" ]; then
echo "--- $f"
cat -n "$f" | sed -n '1,260p'
fi
doneRepository: aeroxy/ast-bro
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'Relevant MCP implementation snippets:'
sed -n '460,555p' src/mcp/tools.rs
sed -n '585,635p' src/mcp/tools.rs
sed -n '968,1008p' src/lib.rs
sed -n '70,90p' src/project_root.rs
sed -n '160,190p' src/project_root.rs
sed -n '150,180p' src/project_root.rs
echo
echo 'Read-only behavioral probe for split_existing / missing_PATHs logic:'
python3 - <<'PY'
from pathlib import Path
import tempfile
for root in [Path(tempfile.gettempdir()), Path("C:\\temp")]:
root.mkdir(exist_ok=True)
for name in ["lib.rs", "gone.rs", "nope/gone.rs", "/nope/gone.rs"]:
p = root / name
missing = not p.exists()
print(f"{root!s:20} {name!s:18} exists={not missing}")
PYRepository: aeroxy/ast-bro
Length of output: 11085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'path_glob expand_existing implementation:'
rg -n "pub fn expand_existing|fn expand_existing|expand_existing" src tests -A 120 -B 20
echo
echo 'Relevant PathBuf display cases in tests:'
python3 - <<'PY'
import subprocess, json
code = r'''
use std::path::PathBuf;
fn main() {
for s in ["gone.rs", "nope/gone.rs", "/nope/gone.rs"] {
let p = PathBuf::from(&s);
println!("{:?}: {}", s, p.display());
}
}
'''
with open('/tmp/path_display_probe.rs', 'w') as f:
f.write(code)
subprocess.run(['rustc', '/tmp/path_display_probe.rs', '-o', '/tmp/path_display_probe'])
subprocess.check_call(['/tmp/path_display_probe'])
PYRepository: aeroxy/ast-bro
Length of output: 16044
Avoid Unix-specific missing-path fixtures.
The /nope/... inputs are always treated as missing, and the text assertions/assertions rely on the input being displayed with forward slashes. Use a platform-independent nonexistent path name such as gone.rs in the MCP requests and corresponding missing/missing_paths assertions.
Also applies to: 89, 121-127, 138, 155-163.
🤖 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/mcp_e2e.rs` at line 73, Replace the Unix-specific `/nope/...`
missing-path fixtures in the MCP requests and related assertions with a
platform-independent nonexistent name such as `gone.rs`; update all
corresponding `missing` and `missing_paths` expectations in the affected test
cases to match.
2 participants
Resolves the seven issues filed against 3.0.0, plus review follow-ups:
digestintomap: one command, orthogonal flags, named presets #37 map and digest are one command: --detail names|signatures|full, one-polarity visibility flags, scope flags on both; digest is an alias for map --preset digest with explicit flags overriding the preset.MCP mirrors the CLI: trace rejections render a valid trace.v1 doc with found:false, callers folds hidden-unattributed counts into the response body, and the unattributed section shares the --limit budget everywhere.
Summary by CodeRabbit
--jsonincludesast-bro.error.v1; consistent exit codes (including 3 for cycles).callersadds unattributed call-site reporting.map/digestshare unified behavior (member caps, projections); chunking now supports TOML and PowerShell; MCP tools/schema updated.