feat: add structured output to report and status built-ins - #306
Conversation
🟠 Maintainer review suggested — low confidenceThe automated review could not reach a fully supported conclusion. This review is advisory and does not block merging. |
|
The two red jobs — That is I can't re-run them myself (needs admin on this repo), and I'd rather not push an empty commit and re-trigger the whole matrix for a CDN blip. A re-run of just those two should clear it whenever someone gets a chance. |
Part of agentrhq#175. `validate`, `verify`, `doctor`, bare `skills`, `daemon status`, and `profile list` returned stable data internally but rejected `-f/--format` and printed hand-written text only. Each now routes through the shared `resolveOutputFormat` + `render` path introduced in agentrhq#190. `adapter status` already gained `-f` in agentrhq#190, so it needed no change here. Each command keeps its human-readable report as the `table` rendering, which remains the default, and returns the underlying result object under any other format. Following the `convention-audit` precedent, the human text is chosen on the raw format rather than the TTY-resolved one, so no existing implicit behavior changes. `daemon status` gains a machine-readable projection that mirrors the text rendering, reporting `{ running: false }` when no daemon is reachable. `profile list` returns one row per profile covering both connected profiles and saved-but-disconnected aliases, aligning it with the hosted profile-list row set. Daemon guidance for those two goes to stderr under structured formats so stdout stays parseable. Bare `skills` and `skills list` now share one renderer instead of duplicating the row set with a hardcoded format. Browser read commands are intentionally left for a follow-up: they go through the session/bridge layer and need separate care around the streaming and JSONL-follow exclusions the issue calls out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CVuD882dJuEwWKWBLaf7bs
228982c to
af84f96
Compare
|
Resolved — no re-run needed after all. I amended the commit (identical tree, That confirms it was the transient |
|
Thanks for the review! Quick check before this goes further — I'd left a scoping comment on #175 proposing to split this into two PRs (builtins here, browser reads separately), but it seems to have disappeared. Also saw an older comment on #175 mentioning it might already be spoken for via #190 — that one actually closed #172, a sibling issue, so I want to make sure #175 itself is still open for me to work on before I start phase 2. |
Part of #175 (tracking: #171). This is the first of the two PRs I proposed in #175 (comment) — the non-browser built-ins. Browser reads follow separately.
What
validate,verify,doctor, bareskills,daemon status, andprofile listall returned stable data internally but rejected-f/--formatand printed hand-written text only. Each now routes through the sharedresolveOutputFormat+renderpath from #190.adapter statusis on the issue's list but already gained-fin #190, so it needed no change here.Behavior
Each command keeps its human-readable report as the
tablerendering, which stays the default. Any other format returns the underlying result object.I followed the
convention-auditprecedent from #190 and branch on the raw format rather than the TTY-resolved one. I first tried resolving through the implicit non-TTY rule, but that movesprofile list's prose onto the structured path whenever stdout is a pipe, which breaks the guidance contract two existing tests encode (reports stale daemon instead of no profiles…,uses runtime profile wording…). Branching on the raw format means no existing implicit behavior changes and nothing here is breaking — the issue asks for these commands to stop rejecting-f, not to change what an unflagged invocation prints.Two commands needed a real data model rather than a straight serialization:
daemon statusgains a projection mirroring the text rendering:running,stale,pid,version,cliVersion,uptimeMs,runtimeConnected,runtimeName,runtimeVersion,profileRequired,profileDisconnected,profiles,memoryMB,port. It returns{ "running": false }when no daemon is reachable.profile listreturns one row per profile —contextId,alias,default,connected,runtimeVersion— covering both connected profiles and saved aliases that are not currently connected, which the prose surfaced but no caller could parse. That aligns it with the hosted profile-list row set.For those two, daemon guidance ("Daemon is not running…", stale-daemon hints) goes to stderr under structured formats so stdout stays parseable. It stays on stdout for
table.Bare
skillsandskills listnow share one renderer instead of duplicating the row set with a hardcoded format, which is the de-duplicating half of the issue's "or make it a true alias" option.Still open from my issue comment
Neither of these blocks review, and I'm happy to adjust:
skills— I shared the renderer rather than restructuring the Commander wiring into a literal alias. Say the word if you'd prefer the true alias.profile list— I kept the local field set (alias, default marker, runtime version) and matched hosted's format handling rather than adopting hosted's column set, since the local fields have no hosted equivalent.Testing
npm run typecheck,npm run build— cleannpm run check:hosted-contract— contract bytes unchangednpm run check:plugin-parity,check:package-bin,check:codex-plugin— passsrc/cli.test.tscovering JSON/YAML per command family, the stdout/stderr split, bare-skills/skills listequivalence, and-f xmlrejection (exit 2, before any side effects)profile list -f json 2>/dev/nullemits clean[]On the full suite I get 122 pre-existing failures on this Windows machine (symlink
EPERMand a build-test timeout). I confirmed that count is identical on a cleanmain— same 12 files — and that passing tests go 5450 → 5464, exactly the 14 added. No regressions; CI should be a cleaner signal than my local box.