fix(map): canonical map form in the directory hint, plus JSON and tests

[vlsi]

Why

The oversized-directory hint from #35 shipped, but it could not do its job.

It named whichever alias the caller entered through, so ast-bro digest <dir> --detail full printed a suggestion reading digest … --preset digest — nonsense after #37 made the two one command. JSON callers paid the same size for the same question and got no hint at all, because the --json branch returns before the text renderer ever reaches it. And an MCP client — the caller the hint was written for — heard nothing at all: the server has no stderr, so the branch that prints it was unreachable from tools/call. The behavior was documented nowhere: wiki/architecture.md covers #32, #33, #36 and #37, but not this one.

What

The suppression rule asks what the call renders, not how it was spelled. digest <dir> --include-private --include-fields --json produces byte for byte what map <dir> --detail names --json produces, yet only the second was hinted. It is now derived from the resolved axes — bare-name detail, nothing private, no fields, a cap of 50 or tighter — so an inflated digest call is hinted like the map call it equals. The hint is emitted once after both renderers rather than once per renderer, which is what keeps text and JSON from drifting apart again.

The suggestion reproduces the call it replaces. Every path rather than the first directory; the caller's --glob, --json, --compact; and a --max-members tighter than the suggested cap. It is built to survive the trip through a shell: values are quoted, the glob takes the attached --glob= form, and paths move behind a -- separator when one starts with a dash, since quoting alone leaves clap reading it as a flag.

The threshold is unchanged and now says what it measures — the payload the caller has to read, so the same directory crosses it under --json while staying quiet in text. A separate JSON threshold would silence genuinely oversized payloads.

The MCP server makes the same decision and words it for its own caller. map_hint_is_due is shared; the message is not. A shell command is the wrong thing to hand a client that speaks JSON-RPC, so the MCP text names the digest tool call that shrinks the answer. It rides the response the way #33's notes already do: a leading # hint: line on text, a hint field on JSON, since a prepended line would break the parser on the other end.

No change to the part of #35 that matters most: map <dir> is still answered as asked. The hint is a pointer beside the result, never a redirect.

How to verify

cargo test — the full suite. cli_ergonomics grew from 38 to 49 tests and mcp_e2e from 10 to 14; two of the new ones run the suggested command through /bin/sh and assert it exits 0.

By hand, on a build of this branch:

$ ast-bro map src                          # 277 KB  → hint names `ast-bro map src --preset digest …`
$ ast-bro digest src --detail full         #  98 KB  → same hint, `map` not `digest`
$ ast-bro digest src --include-private --include-fields --json
                                           #         → hinted: same bytes as the map call it equals
$ ast-bro map src --json --compact         #         → hint keeps `--json --compact`; stdout still parses
$ ast-bro map src wiki                     #         → hint names both roots
$ ast-bro map src --max-members 1          #         → the tighter cap survives into the suggestion
$ ast-bro map src/deps                     #  23 KB  → quiet, under the threshold
$ ast-bro map src/lib.rs                   #         → quiet, a single file has nothing smaller to suggest
$ ast-bro digest src                       #         → quiet, the digest answer would point at itself

Over MCP, tools/call on map with {"paths": ["src"]} returns a leading # hint: line, and the same call with "json": true returns a payload whose hint field carries the message.

Reviewed by a two-reviewer cross-review over three rounds; seven findings, all fixed.

Closes #35.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f8629424-1319-4f2d-ad1b-a4c60f6867fa

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[aeroxy]

Why

The oversized-directory hint from #35 shipped, but it could not do its job.

It named whichever alias the caller entered through, so ast-bro digest <dir> --detail full printed a suggestion reading digest … --preset digest — nonsense after #37 made the two one command. JSON callers paid the same size for the same question and got no hint at all, because the --json branch returns before the text renderer ever reaches it. And an MCP client — the caller the hint was written for — heard nothing at all: the server has no stderr, so the branch that prints it was unreachable from tools/call. The behavior was documented nowhere: wiki/architecture.md covers #32, #33, #36 and #37, but not this one.

What

The suppression rule asks what the call renders, not how it was spelled. digest <dir> --include-private --include-fields --json produces byte for byte what map <dir> --detail names --json produces, yet only the second was hinted. It is now derived from the resolved axes — bare-name detail, nothing private, no fields, a cap of 50 or tighter — so an inflated digest call is hinted like the map call it equals. The hint is emitted once after both renderers rather than once per renderer, which is what keeps text and JSON from drifting apart again.

The suggestion reproduces the call it replaces. Every path rather than the first directory; the caller's --glob, --json, --compact; and a --max-members tighter than the suggested cap. It is built to survive the trip through a shell: values are quoted, the glob takes the attached --glob= form, and paths move behind a -- separator when one starts with a dash, since quoting alone leaves clap reading it as a flag.

The threshold is unchanged and now says what it measures — the payload the caller has to read, so the same directory crosses it under --json while staying quiet in text. A separate JSON threshold would silence genuinely oversized payloads.

The MCP server makes the same decision and words it for its own caller. map_hint_is_due is shared; the message is not. A shell command is the wrong thing to hand a client that speaks JSON-RPC, so the MCP text names the digest tool call that shrinks the answer. It rides the response the way #33's notes already do: a leading # hint: line on text, a hint field on JSON, since a prepended line would break the parser on the other end.

No change to the part of #35 that matters most: map <dir> is still answered as asked. The hint is a pointer beside the result, never a redirect.

How to verify

cargo test — the full suite. cli_ergonomics grew from 38 to 49 tests and mcp_e2e from 10 to 14; two of the new ones run the suggested command through /bin/sh and assert it exits 0.

By hand, on a build of this branch:

$ ast-bro map src                          # 277 KB  → hint names `ast-bro map src --preset digest …`
$ ast-bro digest src --detail full         #  98 KB  → same hint, `map` not `digest`
$ ast-bro digest src --include-private --include-fields --json
                                           #         → hinted: same bytes as the map call it equals
$ ast-bro map src --json --compact         #         → hint keeps `--json --compact`; stdout still parses
$ ast-bro map src wiki                     #         → hint names both roots
$ ast-bro map src --max-members 1          #         → the tighter cap survives into the suggestion
$ ast-bro map src/deps                     #  23 KB  → quiet, under the threshold
$ ast-bro map src/lib.rs                   #         → quiet, a single file has nothing smaller to suggest
$ ast-bro digest src                       #         → quiet, the digest answer would point at itself

Over MCP, tools/call on map with {"paths": ["src"]} returns a leading # hint: line, and the same call with "json": true returns a payload whose hint field carries the message.

Reviewed by a two-reviewer cross-review over three rounds; seven findings, all fixed.

Closes #35.

🤖 Generated with Claude Code

Awesome!

Minor issues & questions

1. shell_quote slightly over-quotes safe characters

let safe = |c: char| c.is_ascii_alphanumeric() || "._/-+=:@,".contains(c);

~ and % are also POSIX-safe and commonly appear in paths (~/project, %20). Missing them means ~/foo gets quoted as '~/foo', which is harmless but slightly noisier than necessary. Not a blocker.

2. Integer division makes the KB display slightly misleading

output_len / 1024

At exactly 25_001 bytes (just past the threshold), the message reads 24 KB. Consider rounding or using div_ceil if you want the displayed KB to always be ≥ the threshold KB. Cosmetic only.

3. MCP hint hardcodes max_members 8 and doesn't preserve caller flags

The CLI hint carefully preserves --glob, --json, --compact, and tighter --max-members:

let cap = a.max_members.map_or(8, |m| m.min(8));

The MCP digest_hint() hardcodes the message:

"the `digest` tool with max_members 8, include_private and include_fields off..."

This is acceptable if the MCP caller (an agent) is expected to adapt, but it is a behavioral asymmetry between the two surfaces. If MCP ever gains a glob or compact argument, this message will become stale. Consider whether the MCP hint should also mention the caller's actual scope flags, or at least note this divergence in a comment.

4. with_hint() silently drops the hint on JSON parse failure

fn with_hint(json: String, hint: &Option<String>) -> String {
    let Some(hint) = hint else { return json; };
    match serde_json::from_str::<serde_json::Value>(&json) {
        Ok(mut doc) => { doc["hint"] = serde_json::json!(hint); ... }
        Err(_) => json,  // hint lost, no warning
    }
}

If render_json_map ever emits malformed JSON (unlikely), the hint disappears. Consider expect or debug_assert since this is an internal invariant, or at least document the silent fallback. Very low risk.

5. map_directory_hint uses raw args while already_digest uses resolved values

In run_map_digest:

let already_digest = matches!(detail, DetailLevel::Names) && !include_private && ...;
// ...
let hint = map_directory_hint(a, &paths, already_digest, rendered.len());

Inside map_directory_hint, a.max_members is read again to compute the suggested cap. This is correct today because max_members is resolved from a.max_members earlier in the same function, but if future refactoring changes how presets are applied, the two could drift. A small comment linking them would help future maintainers.

---

Suggested tweaks (optional)

// In shell_quote, add ~ and % to safe chars:
let safe = |c: char| c.is_ascii_alphanumeric() || "._/-+=:@,~%".contains(c);

// In map_directory_hint, consider a comment:
// `already_digest` above is computed from resolved axes; `a.max_members`
// here is the same source value after preset resolution.

[vlsi]

Thanks — all five are addressed, and I put the fixes through a two-round cross-review, which found seven more. Also rebased onto main, which had moved 12 commits and turned the branch conflicting.

1. shell_quote — added %, but deliberately kept ~ quoted. A path still spelled ~/project when it reaches this function is a directory literally named ~: a real tilde is expanded by the shell before the process starts, so emitting it bare would send the next shell to $HOME instead. Say the word if you read it differently.

2. kb_ceil — the displayed size rounds up, so 25 001 bytes no longer report 24 KB.

3. MCP hint — live rather than hypothetical: both MCP tools already accept glob and max_members. The hint now spells out the arguments to send, and it names the map tool rather than digest, because the digest schema has no no_attrs / no_lines to receive them — the same suggestion the CLI makes.

4. with_hint — debug_assert! on the parse failure; a release build still drops the hint rather than the answer.

5. Resolved vs. raw — both hint builders take a struct of already-resolved values, so the suggestion and the already_digest test cannot drift apart.

The cross-review's largest finding was that the trigger asked about the spelling rather than the answer: map alpha and map 'alpha/*.rs' render byte-identical output, and only the first was hinted. The trigger is now "more than one file was inspected", and the message reads multi-file answer since "directory" stopped being true of every case. Naming files explicitly stays quiet — that line is deliberate and pinned by a test. The suggestion also repeats a path that did not resolve (so the pasted command keeps the missing-path note), carries --no-attrs / --no-lines, and stays silent for a non-UTF-8 path, which removed a latent panic on the MCP side too.

Tests: +21 in cli_ergonomics, +5 in mcp_e2e; two of them run the suggested command through /bin/sh and assert it exits 0.

One question before merge: squash the three commits into one, or keep them as they are (hint fix / MCP surface / review fixes)? Happy either way.

[aeroxy]

@vlsi nice work on this. The shell-quoting story is airtight — the ~ reasoning is the right call, and running the suggested command through a real /bin/sh is the kind of assertion that actually holds. Moving the decision into map_hint_is_due so both surfaces share it is what keeps text and MCP from drifting apart again.

Two things, both in the same predicate.

1. A glob that matches a single directory silences the hint. src/lib.rs:1168

let inspects_several_files =
    |p: &PathBuf| p.is_dir() || path_glob::expand_existing(p).len() > 1;

This never inspects what the glob matched. expand_existing("sr*") returns one entry — the src directory — so len() > 1 is false, and p.is_dir() is false for the pattern string itself:

$ ast-bro map src   > /tmp/a    # 312423 bytes, hinted
$ ast-bro map 'sr*' > /tmp/b    # 312423 bytes, quiet
$ cmp /tmp/a /tmp/b && echo identical
identical

That's exactly the arbitrariness the doc comment above the function rules out ("map alpha and map 'alpha/*.rs' render byte for byte the same thing, so hinting one and not the other would be arbitrary"). The predicate is shared with src/mcp/tools.rs:581, so the MCP map / digest tools drop it too. The new test only covers a glob matching two directories, which is why this slipped through.

let inspects_several_files = |p: &PathBuf| {
    let m = path_glob::expand_existing(p);
    p.is_dir() || m.len() > 1 || m.iter().any(|e| e.is_dir())
};

2. A directory holding one file is hinted as a "multi-file answer". src/lib.rs:1295

Same predicate, other direction: p.is_dir() alone is enough, so a directory containing a single 50 KB file prints # hint: this was a multi-file answer (50 KB); ... while naming that file directly stays quiet. The message asserts something untrue, and the trigger is back to being decided by the spelling rather than by the answer — the property this PR is specifically about. results.len() is already in scope at the run_map_digest call site if you want to decide it from what actually rendered.

Everything else I poked at held up: already_digest is fixpoint-stable, so following any suggestion lands on a quiet call; the -- separator and attached --glob= form make the suggestion runnable as printed; the MCP suggestion's argument keys all exist in the map tool schema; with_hint operates on an object payload so doc["hint"] can't panic; and the && short-circuit keeps the extra expansion off the under-threshold path.

Happy to merge once those two are settled.

[aeroxy]

@vlsi both of mine are fixed, and files_inspected reading results.len() off the walk is the right shape for it — the predicate can't be spelled around now. Nice turnaround :)

Three more, all in the suggestion builder rather than the trigger.

1. The counted form can loop. src/lib.rs:1305

The spelled-out form is a fresh command, so it drops whatever the caller had and always settles. The counted form says add these flags to what you already typed — and when the caller has already set every axis the preset would change, adding them is a no-op:

$ ast-bro map <20 files> --detail names --include-private --include-fields --max-members 8
# hint: this was a multi-file answer (32 KB); adding `--preset digest --max-members 8` to the same 20 paths answers the same question in a fraction of the size

$ # append exactly that:
$ ast-bro map <20 files> --detail names --include-private --include-fields --max-members 8 --preset digest
# hint: this was a multi-file answer (32 KB); adding `--preset digest --max-members 8` to the same 20 paths answers the same question in a fraction of the size

Byte-identical 32302-byte output, byte-identical hint, forever. Explicit flags beat the preset on all four axes, so there is nothing left for the suggestion to change.

assert_suggestion_runs_and_settles can't catch it, and says so in its own assertion message — "this helper runs the whole command form; the counted form has none". So the one property the hint has to have is asserted only for the form that cannot violate it. Worth either making the counted phrasing prescriptive rather than additive ("running --preset digest --max-members 8 on the same N paths"), or teaching the helper to reconstruct the counted case from the original argv.

2. A newline in a path splits the hint across lines. src/lib.rs:1324

Reproduced with a directory literally named we<LF>ird. shell_quote wraps it correctly, but the newline stays inside the quotes, so the hint occupies two stderr lines and the first one ends:

# hint: this was a multi-file answer (28 KB); `ast-bro map '/…/nl/we

— an unbalanced single quote inside an unbalanced backtick. A line-oriented reader takes that as the whole suggestion, and pasting it hangs the shell waiting for the close. Same class as the backtick guard two lines up, and the repo's own helper is line-oriented (stderr.lines().find(...)), so the guard already has the right home: adding '\n' to the existing any(|p| p.contains(''))` sends it to the counted fallback.

3. Non-UTF-8 drops the hint rather than falling back. src/lib.rs:1318

collect::<Option<_>>()? propagates out of map_directory_hint, so at four paths or fewer a non-UTF-8 path loses the hint entirely. The inline comment is consistent with that, but the commit message says both unspellable cases "fall back to the counted form, which names no paths" — the backtick one does, this one doesn't. Since the counted form genuinely doesn't care about spelling, it should get the same return Some(counted) treatment.

Lowest severity of the three: APFS rejects the filename outright, so it's Linux-only and I couldn't reproduce it here — code reading only.

---

Everything else I went after held. The CLI and MCP really do share the decision, the suggestion carries the scope/display/format flags, the dash-prefixed -- separator and attached --glob= form survive a real shell, already_digest is computed from resolved axes on both surfaces, and with_hint can't panic on a non-object payload. 59 + 19 tests green, clippy clean apart from the pre-existing src/search/chunker.rs warning.

(1) is the only one I'd hold the merge for.

[vlsi]

All three fixed in e19212b, and a cross-review of that fix turned up three more, fixed in a7090b4. Rebased on main along the way.

1. The counted form looped. Both forms now describe a whole call — running \--preset digest --max-members 8` on the same N paths— so following either lands on the digest answer. Reproduced your case first:map src/**/*.rs --detail names --include-private --include-fields --max-members 8, 73 702 B, and following the suggestion now gives 23 186 B and silence. The test pins both readings the wording rules out: appending just the preset loops, and appending the fragment verbatim is a clap rejection, since --max-members` would repeat.

2. Newline in a path. Generalized rather than added to the list — the guard is now any control character, and the test is one table over a backtick, a newline and a tab.

3. Non-UTF-8 drops the hint. It falls back like the others now. It needed a unit test on the builder: APFS rejects the filename, and an argument naming nothing is a rejected call before the hint is built.

A Codex pass on top of that found the println! newline the size check never counted, so at exactly the threshold the emitted answer cleared a bar the predicate said it had not.

Then the cross-review found I had closed the class only halfway. The rule was "a string the hint interpolates must survive one backtick-delimited line", and I screened the paths but not --glob — the other caller-controlled string in the same span. So all three failure modes stayed reachable through it: map <dir> --glob '{*.rs,zz\zz}'` printed a hint carrying three backticks, and a newline glob split it across two stderr lines. The counted form broke too, which is exactly what the wiki had just claimed it could not do. The rule is now stated once and applied to every interpolated string; a path that fails it still falls back to the counted form, and a glob that fails it withholds the hint, because the counted form carries the glob as well and dropping it would prescribe a broader question.

One correction to my own commit message: I wrote that the negative tests sit on the threshold boundary. They do not — every threshold assertion in the suite clears the bar by 50 KB, so reverting the + 1 left all 60 green. There is now a fixture that pads a function name one character at a time (the renderer prints each name once, so a character is a byte) and pins both sides: 25 000 emitted bytes stay silent, 25 001 hint.

cli_ergonomics is at 62 tests. Each of the new ones was run against the code it replaces and does fail there — including the three above, which is what stopped the glob gap from being a fourth round.

Still happy to squash the five commits into one before merge if you prefer; say the word.

[aeroxy]

@vlsi all three are properly closed, and you found two I'd walked straight past — the glob being the other caller-controlled string in that same span is the one I should have caught, since screening the paths and leaving the glob open covers none of the three failure modes in practice. Widening the test from backtick to "any control character" is the better rule too; I'd have patched '\n' and left the tab. And catching the println! newline the size check never counted, on your own initiative, is the kind of one-byte thing that only ever gets found by someone actually holding the invariant in their head.

Verified the fixes at a7090b4 in a clean worktree: the counted form prescribes rather than adds, unspellable paths fall back instead of vanishing, and the glob withholds. 62 + 19 green.

One thing left, and it's small. The suggested cap 8 is a bare literal in two places — src/lib.rs:1302 and src/mcp/tools.rs:588, both map_or(8, |m| m.min(8)) — with nothing in defaults. I checked the drift rather than assuming it: changing only the MCP copy to 12 leaves all 81 tests green, because the assertions look for --preset digest and never the number.

Which is the same class you called out yourself one commit earlier, about the literal 50 — and the defaults build guard can't see a number living inside an expression. A HINT_MAX_MEMBERS in defaults plus one test reading it would close it for good.

And a nit, no action needed: with_hint (src/mcp/tools.rs:633) parses and re-serializes the entire payload to append one field, and by construction only runs when that payload is large — ~456 KB for map src --json here, twice in one response when a path is missing. Threading the hint through render_json_map would make it free. I did go looking for a format bug there and there isn't one: the MCP map tool has no compact option and renders pretty, so to_string_pretty is faithful.

---

Genuinely excellent work on this one — four rounds, and every round the fix got more general than the finding that prompted it. That's rarer than it sounds.

Enjoy the week off, and good luck with whatever you're off to do :) Nothing here is urgent, so leave it exactly as it is — pick it back up whenever you feel like it, or don't, and I'll take the cap literal myself if it starts bothering me before then. Thanks again.