fix(mcp): make list_commands browse and search the catalog - #395
Conversation
The test runs the list_commands handler against the real command tree. TestMain lets the test binary run as the jamf-cli child. The test requires valid JSON, every catalog command, the destructive mark on each command, and a result under maxChildOutputBytes. The test fails on this commit. The handler requests `commands -o json`, and capChildOutput cuts that output mid-string at 262144 bytes. Co-Authored-By: Claude Opus 5.5 <[email protected]>
list_commands requested `commands -o json` and returned it through runChild. That output is larger than maxChildOutputBytes, so capChildOutput cut it mid-string. The model got invalid JSON that held only part of the catalog, and no Protect, School or Security Cloud commands. CombinedOutput also put the stderr list hint into the result. listCommands now requests only command, description and destructive, as NDJSON. It reads stdout alone, and it puts stderr into the result only when the child fails. It does not call capChildOutput, because a cut catalog is invalid JSON. The catalog is fixed per build, so the test keeps it under the cap. The tool description, the `mcp` help text and agent_context.md now say which fields list_commands returns. They tell the model to use run_command with `<command> --help` for flags, and with `commands --select command,<field> -o ndjson` for other fields. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The whole catalog did not fit in one tool result. As NDJSON with three fields it is 210125 bytes. Claude Code 2.1.274 saves a result above its default 25,000-token limit to a file, and gives the model only an error line with the path. `commands` takes three new flags: --prefix <path>, --children and --search <words>. With no flags it prints the whole catalog, as before. queryCatalog, catalogChildren and searchCatalog use the same entry builder as collectCommands, so a row is the same in every view. list_commands takes two optional arguments, prefix and query, and runs `commands` with those flags. capCatalogLines keeps each result under maxListCommandsBytes, at a line boundary. When it drops rows, it adds a JSON line that counts them. TestQueryCatalog_ChildrenReachEveryCommandOnceUnderTheCeiling walks every --children level from the top. It requires each level to fit under maxListCommandsBytes, and every catalog command to appear exactly once. Co-Authored-By: Claude Opus 5.5 <[email protected]>
- catalogWordForms gives each search word its possible singular forms. The old fold made "patches" into "patche", so `--search patches` found 1 command where `--search patch` found 87. - A search with no letters or digits is refused. It matched every command. - A --children listing in table, CSV or plain output carries subcommands on every row, zero included. The first row decides the columns, and it is often a command with no count. - maxListCommandsBytes is 40 KiB. Claude Code 2.1.274 saves a text tool result longer than about 50,000 characters to a file, whatever its token count. A 64 KiB result was saved, and a 40 KiB result arrived inline. - The empty-result hint suits a browse or a search. The tool description says that some groups can run, and that search rows carry no flags. The unknown-prefix hint no longer names a CLI form. Co-Authored-By: Claude Opus 5.5 <[email protected]>
neilmartin83
left a comment
There was a problem hiding this comment.
Tip
✅ Merge-ready. Turns MCP list_commands into a browse/search table of contents backed by new commands --prefix/--children/--search flags, with a line-safe 40 KiB cap. No critical or important issues found.
See the collapsed section for 2 nice-to-have suggestions.
Rating: 5/5
- Clean: the size cap is measured rather than guessed, the level walk test pins every browse level under the ceiling, and the no-flag
commandsoutput is byte-compatible. The 2 suggestions below are optional. - Coverage: no specialist lane (
silent-failure-hunter,test-quality-reviewer,devil-advocate,usability-reviewer) has run on this PR. The rating reflects the orchestrator's own pass, not the whole panel.
Nice-to-have suggestions (2 items)
💡 NICE-TO-HAVE (1) (usability) — internal/commands/root.go:1334: --children --search combine silently into a search over the child rows only.
queryCatalog applies --search to whatever --children produced, so jamf-cli commands --children --search policy -o ndjson prints nothing and exits 0 (reproduced on this head), because no top-level row mentions "policy". The Long text lists the three flags as independent narrowings and says nothing about the combination. MCP never sends both, so this is CLI-only. Refuse the pair with a usage error, or document that --search then only matches the listed level.
💡 NICE-TO-HAVE (2) (usability) — internal/commands/root.go:1382: group rows in a browse listing carry "destructive":false and "flags":"".
A non-runnable group row goes through commandEntriesToMaps, which always writes destructive, and --select then fills the missing flags with "". So prefix "pro" answers {"command":"pro accounts",…,"destructive":false,"flags":"","subcommands":6}, a "not destructive" claim about a subtree that contains deletes. The tool description frames destructive as a property of commands, so a model is unlikely to act on it, but a row that is only a pointer should say only that. Emit destructive/flags only when isCatalogCommand(child).
Review coverage and scope
- Design: TOC-style browse/search instead of one dump; rejected alternatives (
maxResultSizeChars, product filter) are argued with numbers in the PR body and hold up - Correctness:
resolveCatalogPrefixalias/skip handling,catalogChildrenleaf fallback,newCommandEntryalias source (cmd.Parent()is equivalent to the old walked parent), table/CSVsubcommandscolumn,capCatalogLinesline-boundary cut and omitted count. Combination gap is (1) - Security: model text reaches the child only as
--prefix=/--search=values, still throughbuildChildArgsandchildEnv(dropsJAMF_CLI_ARGS); no new flag collides with a root persistent flag - Reliability: stdout/stderr split keeps hints out of the NDJSON; failure path carries stderr tail
- Test coverage: level walk (every command once, every level under ceiling), handler tests through a re-exec'd test binary, fake-child truncation/stderr/empty tests, argv-injection test
- Docs: CHANGELOG,
mcpLong,agent_context.md,jamf-investigateSKILL updated;platform-api-ga.mdjq pipelines unaffected (no-flag output unchanged) - [na] Performance: per-level
collectCommandsfor counts is O(tree) over ~1,766 entries - [na] Frontend, migrations, dependencies
Diff: 8 files, +870/−83, head fa6c92e2. High risk (size). Verified locally: built the binary and probed browse/search/prefix errors; go test ./internal/commands/ (catalog, MCP, positional, flag subsets) and go vet pass. Lanes run: none. Conventions: root CLAUDE.md plus 3 .claude/rules/*.md (credentials-and-auth, coding-style, classic-api), from the worktree. Prior reviews: one dismissed, empty-body review by @grahampugh on 73b9f721; nothing actionable.
What's done well
- ✅ The 50,000-character Claude Code limit was measured with a throwaway MCP server instead of assumed from the 25k-token doc figure, and the ceiling sits under it with headroom that
TestQueryCatalog_ChildrenReachEveryCommandOnceUnderTheCeilingenforces on every spec sync. - ✅ Extracting
newCommandEntry/catalogScope/skipInCatalogmeans a row is built one way in every view, and the walk test asserts each browsed row equals its full-catalog entry.
🤖 Generated by the pr-review:review skill v1.39.0 · reviewed head fa6c92e2
Why
The MCP
list_commandstool returned the whole command catalog in one result, and the AI could not use it.commands -o jsonthroughrunChild.capChildOutputcut that output mid-string at 262,144 bytes. The model got invalid JSON with no Protect, School or Security Cloud commands.CombinedOutputalso put the stderr list hint into the result.A catalog of this size is not a single tool result. So
list_commandsnow works like a table of contents. The first call lists the top level. Each next call opens one command path, or searches by words. Every browse level fits well under the limit, and a search result is cut at a line when it grows too large.These are the sizes on this head:
The Claude Code limit, measured
The Claude Code MCP docs name a default limit of 25,000 tokens. Claude Code 2.1.274 also applies a size limit that the docs do not name.
A throwaway MCP server returned a text result of an exact length, and
claude -p --model haikucalled it. The results:xxOutput too large (49KB)"a",The limit is therefore about 50,000 characters of raw text. JSON escapes do not count toward it.
maxListCommandsBytesis 40 KiB, below that limit.Scope
jamf-cli commandsflags.--prefix <path>lists the commands under one path. It accepts aliases.--childrenlists only the direct children of the prefix, or of the top level, with asubcommandscount.--search <words>keeps the commands whose path, description or aliases contain every word. A word matches the start of a word.catalogWordFormsgives each word its possible singular forms, so "policies", "patches" and "caches" match their singulars. A search with no letters or digits is refused, because it would match every command.commandsprints the whole catalog, as before. Thejqpipelines in the docs do not change.--childrenlisting in table, CSV or plain output carriessubcommandson every row, zero included. A table's columns come from its first row, and the first row is often a command with no count. JSON, NDJSON and YAML show the count only when it is above zero.queryCatalog,resolveCatalogPrefix,catalogChildrenandsearchCatalogininternal/commands/root.goimplement the flags.newCommandEntry,catalogScopeandskipInCatalogcome out ofcollectCommands, so a row is the same in every view.list_commandsarguments. Ininternal/commands/mcp.go, the tool takes two optional arguments,prefixandquery.listCommandsArgsbuilds thecommandscall. The model's text reaches the child only as a--flag=valuetoken.listCommandsreads stdout alone, and it puts stderr into the result only when the child fails. An empty result gets one JSON line with a hint that suits a browse or a search.capCatalogLineskeeps each result undermaxListCommandsBytesand cuts only between lines. When it drops rows, it ends with one JSON line that counts them.mcphelp text,agent_context.md,skills/skills/jamf-investigate/SKILL.mdandCHANGELOG.md.Tradeoffs
queryusually finds a command in one call._meta["anthropic/maxResultSizeChars"]annotation was rejected. It makes Claude Code show the whole catalog inline, which was tested. But every discovery call then carries the whole 210,125-byte catalog. The annotation only works in Claude Code, and the catalog grows with every spec sync.prorows alone are 165,328 bytes, as the command above shows.capCatalogLineskeeps the result under the ceiling and says how many rows it left out. Search rows carry no flags. The tool description tells the model to open a command withprefixto see its flags.TestEveryCommandEntryFieldReachesTheCatalogchanged. Its populator could not fill anint, and its failure message asks for that extension. It now setsreflect.Intfields, so the sweep also requires thesubcommandskey. The change was approved before it was made.Blast Radius
jamf-cli mcp serveget a differentlist_commandscontract: NDJSON rows, and two optional arguments. The old result was always cut and invalid, so no client parsed it.jamf-cli commandswithout the new flags is unchanged.internal/commandsgets aTestMain. It runsm.Run()unlessJAMF_CLI_TEST_RUN_AS_CLI=1. With that variable, the test binary runs the jamf-cli command tree as the child for the handler tests.Verification
Level walk.
TestQueryCatalog_ChildrenReachEveryCommandOnceUnderTheCeilingwalks every--childrenlevel from the top, in-process. It requires each level to fit undermaxListCommandsBytes, and every catalog command to appear exactly once and equal its full-catalog entry. Its log line:Search and table fixes. New tests cover plural folding, the refused search, the table and CSV count column, and the two empty-result hints. On the built binary:
Claude Code before the redesign. Claude Code 2.1.274 ran headless with a throwaway
--strict-mcp-configagainst commit73b9f721. The model got this in place of the catalog:Claude Code on lookup tasks. The same setup was given two tasks: find the command that deletes a Jamf Pro policy, and find the flags of the command that lists Jamf Protect computers. Every result arrived inline:
jamf-cli protect computers list --helplists only-h, --help, so the second answer is correct.Claude Code at full size. The query
amatches almost every command, so its result is cut at the ceiling.Output too large (69.9KB)and saved the result to a file.Suite and lint.
make testexited 0, withokfor all 35 packages.golangci-lint run ./internal/commands/...reported 0 issues.Unverified. Claude Desktop chat and Cursor document no result-size limit that could be found. No client could be driven from this session.
Review. A separate Opus review of
a2a25db5raised six points. This head fixes all six: the-esplural fold, the table count column, the full-size ceiling check, the punctuation-only search, four agent-facing texts, and the derived figures in this description.🤖 Generated with Claude Code