help and kaish-tools described the same tool differently - #430
Merged
Conversation
Problem: kaish-extras reported that `help git` printed the worktree
node and stopped, never naming `list` as its verb or showing its
flags. The loop in `tool_help` (crates/kaish-help/src/topic.rs:126)
walked `schema.subcommands` one level and rendered each `sub.name`
without ever looking at `sub.subcommands`.
Evidence: `ToolSchema.subcommands` is a recursive `Vec<ToolSchema>`
carried over serde, so `tools --json` already expresses arbitrarily
deep grammars. The renderer disagreed with the structured surface it
was supposed to describe, and every OTHER tool renders fully, which
made the gap look like a complete document rather than a bug. This
matters most for wrapped commands (kaish_kernel::tools::wrapped),
0.17.0's headline feature, which can declare grammars deeper than one
level.
Wrote a failing test first: a two-level `ToolSchema` (git -> worktree
-> list, with a parameter on the leaf). It failed exactly as
predicted — content stopped at "worktree — Work with the
repository's working trees" and never named `list` or its parameter.
Decision: replace the one-level loop with `push_subcommand_roster`, a
recursive helper that renders one line per subcommand at ANY depth,
each carrying its FULL PATH ("worktree list — ...") rather than
adding an indent level per depth. kaish-extras parses this roster
structurally: lines at exactly two spaces, split on the " — "
separator (space, em-dash, space). A deeper indent or a different
separator would have broken their parser even while fixing the
missing verb. Added a three-level test (kj context session list) to
confirm depth is not capped at two, and a control test that a flat
tool (no subcommands) renders unchanged.
Rule now in force: a subcommand roster line is always the full path
from the tool name to that node, at a fixed two-space indent,
joined to its description with " — ", regardless of how deep the
grammar nests.
Surveyed the other two tool-listing surfaces named in review:
format_tool_list in crates/kaish-help/src/topic.rs (`help builtins`)
and format_tool_list in crates/kaish-kernel/src/tools/builtin/introspect.rs
(`kaish-tools` with no argument) are both flat one-line-per-tool
rosters that never attempted to walk subcommands, so they don't share
this defect. format_tool_detail in introspect.rs (`kaish-tools <name>`)
is a related but more severe gap: it doesn't render subcommands at
all, at any depth, not just one level. Left unchanged pending a
decision on scope.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Problem: while fixing help <tool>'s one-level subcommand gap, the coordinator asked whether format_tool_detail in crates/kaish-kernel/src/tools/builtin/introspect.rs shared the defect. It is worse: that function has zero occurrences of `subcommands` anywhere in the file. `kaish-tools <name>` renders a tool's params and operations and never names a subcommand at any depth, not even the top level `help` used to manage before this patch series. This matters more than the `help` gap because kaish-tools is the introspection surface an agent calls programmatically, not prose it reads. Wrote failing tests first, in the existing tools::builtin::introspect::tests module: a two-level grammar (git -> worktree -> list, parameter on the leaf) and a three-level one (kj -> context -> session -> list). Both failed exactly as predicted -- `kaish-tools git` returned only "git\nGit plumbing and porcelain\n\n", naming neither worktree nor list. Added a control test asserting a flat tool's output is byte-identical to today's, to guard against the fix touching output shape for tools that were already correct. Decision: rather than writing a second recursive walker, made push_subcommand_roster in crates/kaish-help/src/topic.rs `pub` and called it directly from format_tool_detail. kaish-kernel already depends on kaish-help (crate::help re-exports tool_help from it), so the crate boundary was already open; reusing the same function means `help <tool>` and `kaish-tools <name>` cannot drift into naming a tool's grammar two different ways. Kept format_tool_detail's own header/params/operations formatting untouched -- the control test pins that shape -- and inserted the shared roster between params and operations. Rule now in force: one subcommand renderer (push_subcommand_roster) backs every kaish surface that names a tool's subcommands, so a grammar change is visible to `help` and `kaish-tools` in lockstep. Surveyed further per the coordinator's question -- does format_tool_detail drop anything else tool_help renders. It does: schema.examples never renders (tool_help has an "Examples:" section), and a parameter's aliases (`-n` for `--max-count`) never render (tool_help's push_params adds "(also: ...)", format_tool_detail's inline loop does not). The disagreement also runs the other way: tool_help never renders schema.operations, which format_tool_detail does. Neither surface renders schema.aliases (command-level aliases like `ls` for `list`). Left all of these unchanged pending a scope decision -- this commit is subcommands only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the previous two commits. Sharing one recursion between
`help <tool>` and `kaish-tools <name>` is right -- two renderers would drift
into two spellings of a tool's grammar -- but the shared function was
published in the shape the recursion happened to have:
push_subcommand_roster(&mut output, "", &s.subcommands)
Both call sites passed `""`. `prefix` is the path accumulated during the
walk; there is no prefix a caller could correctly supply other than the empty
one, so the parameter existed only because the recursion needed it. kaish-help
is on crates.io, so that signature would have been the published API from
0.17.1 onward.
The public surface is now `subcommand_roster(subs) -> String`, which names
what a caller wants rather than how the walk carries state, and the recursion
stays private behind it. The caller still writes its own `Subcommands:`
header, because the two surfaces place it differently.
No behavior change: same roster, same two-space indent, same ` — ` separator
at every depth, same tests.
Problem: after fixing the subcommand-recursion gap, the coordinator asked whether kaish-tools <name> dropped anything else that help <tool> renders. It did, in both directions. tool_help (help <tool>) has an Examples: section and names a parameter's aliases (-n for --max-count); format_tool_detail (kaish-tools <name>) had neither. format_tool_detail names a tool's declared effects (Operations: ...); tool_help never rendered schema.operations at all. Neither surface named a tool's own command-level aliases (schema.aliases, e.g. ls for list) -- real, shipped data: schema_from_clap reflects it straight from clap, and kj's context/list subcommands already carry it. Wrote failing tests first. Two in crates/kaish-help/src/topic.rs (tool_help never named an operation or a command alias): rm — Remove files No parameters. and list — List sessions No parameters. Three more in crates/kaish-kernel/src/tools/builtin/introspect.rs (format_tool_detail never named an example, a parameter's alias, or a command alias) -- e.g. `kaish-tools head` on a param with alias -n rendered "lines : int (optional)\n Line count\n", no "(also: -n)". Decision: one implementation per element, called from both surfaces, same principle as the subcommand roster. Added four pub functions to kaish-help's topic module -- param_lines, examples_section, operations_line, command_aliases_line -- each named for the concept a caller wants (an owned string to append) rather than shaped by an internal walk. param_lines wraps the existing push_params so kaish- kernel can call it without holding the accumulator; push_params itself stays private and unchanged for tool_help's own use. Both format_tool_detail and tool_help now call all four, plus the already- shared subcommand_roster, so a field named on one surface cannot go unnamed on the other by omission. Kept the byte-identical control test passing on both surfaces for a flat tool (no subcommands, aliases, examples, or operations) -- switching format_tool_detail's inline parameter loop to param_lines turned out to produce the identical bytes for a required parameter with no alias, so the existing control needed no change; kaish-help's control was strengthened from a substring check to an exact-string assertion. Left the two surfaces' framing different on purpose: tool_help's header is "name — desc", format_tool_detail's is "name\ndesc" on two lines; tool_help blank-lines before each optional section, format_ tool_detail stays tight. Neither is an accident -- they're prose for an agent to read versus a fixed low-noise introspection format -- so only the *content* of each section was unified, not the spacing or header style around it. One judgment call, not fixed here: a subcommand's own command-level alias (kj's context list subcommand carries "ls") still doesn't appear in the subcommand roster line -- only a tool's top-level aliases were in scope, matching what was actually compared and reported. Adding alias text into a roster line's path column also risks the kaish-extras parser, which treats everything before " — " as the invocable path; that needs its own decision, not a silent addition here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The parity work added five public functions to kaish-help -- param_lines, examples_section, operations_line, command_aliases_line, subcommand_roster. kaish-help is on crates.io, so that is a surface an embedder can call and we now maintain, and the changelog filed the whole change under Fixed. The behavior entries were accurate; they described what a reader of `help` sees. They did not describe what a reader of the crate gets. An embedder scanning Fixed for a patch release has no reason to read further, which is exactly the reader who needed to know. Public is forced by the crate split rather than chosen -- kaish-kernel is across a crate boundary, and Rust has no visibility between "this crate" and "everyone." The granularity is chosen, though: the two surfaces frame a header and their section spacing differently on purpose, so one render-everything call could not serve both.
Merged
tobert
added a commit
that referenced
this pull request
Sep 2, 2026
Version bump and changelog stamp for v0.17.1, a patch release covering six PRs merged since v0.17.0: help/kaish-tools nested-subcommand recursion (#430), the wrapped-command allow_external_commands framing correction (#431), a changelog correction plus a new zero-padded date/time migration note (#432), nested verb groups for wrapped commands (#433), mount-point ancestor navigation when a backend also covers `/` (#435), and VfsRouter-shared path canonicalization closing a containment leak in `readlink -f`/`realpath` (#434). This bump also carries two documentation fixes surfaced by the release-gate review below rather than opening a separate PR for text-only changes: `docs/EMBEDDING.md` claimed `realpath` passes `allow_missing_final: true` and rechecks existence, when it actually passes `false` directly; and the canonicalize changelog entry overstated the default implementation as containment-checked, when containment is a property of `LocalFs`'s and `VfsRouter`'s overrides, not the shared default. Reviewed with kaibo (`consult`, cast `deepseek`) against the full `v0.17.0..HEAD` diff. Verdict: no undocumented semver breaks — the two new `canonicalize` trait methods are defaulted and every changed public type is either `#[non_exhaustive]` or privately fielded, so the patch framing holds. Two smaller findings from that review are real but scoped as code changes rather than release-blocking text, so they're queued as follow-up work rather than folded into this bump: a wrapped-command node can silently accept a no-op `json_output` declaration instead of being refused, and the new `canonicalize` default's symlink-hop cap has thin test coverage. Gates: `cargo test --all` (2231 passed), `cargo clippy --all --all-targets -- -D warnings` (clean), `cargo insta test --check` (no pending snapshots).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
help <tool>rendered one level of subcommands andkaish-tools <name>rendered none, so a two-level grammar's actual verbs were never named. kaish-extras hit it with a git tool that spells one verb as two words:An agent learns the node exists and cannot learn that
listis the verb under it, or that it takes flags. Every other verb rendered fully, so the document looked complete.ToolSchema.subcommandsis a recursiveVec<ToolSchema>with a plain derive and no depth cap, sotools --jsoncarried the leaf both rendered surfaces dropped. The structured and rendered surfaces disagreed about what exists, and an agent reads help far more often than it reads JSON.Both now recurse to any depth from one shared implementation. The roster stays flat — the full path on one line at a fixed two-space indent, never a deeper indent per level:
The shape is a contract, not an accident. kaish-extras parses this roster by column, so the two-space indent and the
—separator hold at every depth and a test asserts it.Chasing that turned up three more disagreements:
kaish-toolsdropped examples and parameter aliases,helpdropped declared effects, and neither rendered command-level aliases. Each now renders from one implementation, so a field named on one surface cannot go unnamed on the other by omission. The two surfaces still frame their header and section spacing differently, which is deliberate — prose to read versus a fixed introspection format — so only section content was unified.kaish-helpgains five public functions as a result. That is in the changelog under Added, since it is a published crate.Gates:
clippy -D warnings,cargo test --all(6525 passed, 0 failed),insta --check, rustdoc-D warnings.