refactor(output): land the command-output module and adapter wiring - #325
Merged
Conversation
Deepen json-output.ts (renamed to command-output.ts) into the owner of
the JSON/text output contract:
- resolveOutputFormat is the one --json/--format precedence rule; the
commander adapter's private telemetry copy is deleted and the
onCommandResolved event now uses the shared rule (json-only commands
report "json").
- createCommandOutput builds the per-invocation CommandOutput handle
({format, emit, emitJson}) handed to handlers via the new
CliCommandContext. Commands that declare an output mode get strict
--format validation before parseInput and the shared option
definitions attached by the adapter; undeclared commands get an inert
text handle, so nothing changes for existing commands.
- CliCommandDefinition gains an optional output: "standard" | "json-only"
field (no command sets it yet; migration follows in the next PR).
- jsonOutputOptions is renamed to outputFormatOptions everywhere; the
schemaVersion envelope logic is unchanged and now also backs
CommandOutput.emitJson.
No behavior change: no command declares an output mode yet, and the
telemetry rule relocation evaluates identically for every existing
command.
WalkthroughIntroduces a shared Sequence Diagram(s)sequenceDiagram
participant CLI
participant CommanderCliAdapter
participant CommandOutput
participant CommandHandler
participant Writer
CLI->>CommanderCliAdapter: execute command with output flags
CommanderCliAdapter->>CommandOutput: resolve and validate format
CommanderCliAdapter->>CommandHandler: invoke with output handle
CommandHandler->>CommandOutput: emit result
CommandOutput->>Writer: write text or JSON payload
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
This was referenced Jul 27, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/application/commands/command-output.ts (1)
93-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated json-only/format-precedence ternary across two files. Both sites independently reimplement
mode/definition.output === "json-only" ? "json" : resolveOutputFormat(optionValues); extracting a single shared helper avoids the two copies drifting if the json-only rule changes.
src/application/commands/command-output.ts#L93-L109: export a helper (e.g.resolveDeclaredOutputFormat(optionValues, mode)) encapsulating this ternary, and use it as the final return ofresolveStrictOutputFormat.src/adapters/commander/commander-cli-adapter.ts#L284-L306: call the same exported helper for theoutputFormattelemetry field instead of re-deriving it inline.♻️ Proposed refactor
+export function resolveDeclaredOutputFormat( + optionValues: OutputOptionValues, + mode: CliCommandOutputMode, +): "json" | "text" { + return mode === "json-only" ? "json" : resolveOutputFormat(optionValues); +} + function resolveStrictOutputFormat( optionValues: OutputOptionValues, mode: CliCommandOutputMode, ): "json" | "text" { const format = optionValues.format; if ( format !== undefined && !outputFormatValues.includes(format as (typeof outputFormatValues)[number]) ) { throw new CliUserError("errors.shared.invalidFormat", 2, { value: String(format), }); } - return mode === "json-only" ? "json" : resolveOutputFormat(optionValues); + return resolveDeclaredOutputFormat(optionValues, mode); }- outputFormat: definition.output === "json-only" - ? "json" - : resolveOutputFormat(optionValues), + outputFormat: definition.output === undefined + ? resolveOutputFormat(optionValues) + : resolveDeclaredOutputFormat(optionValues, definition.output),🤖 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/application/commands/command-output.ts` around lines 93 - 109, Extract the shared json-only/output-format precedence logic from resolveStrictOutputFormat into an exported helper such as resolveDeclaredOutputFormat(optionValues, mode), and use it for the function’s final return. In src/application/commands/command-output.ts lines 93-109, retain the existing strict validation and delegate only the format resolution; in src/adapters/commander/commander-cli-adapter.ts lines 284-306, replace the inline derivation of the outputFormat telemetry field with this helper.Source: Coding guidelines
🤖 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/application/commands/completion.test.ts`:
- Line 3: The test fixtures must satisfy the CliCommandContext contract by
providing output directly instead of using a double cast. In
src/application/commands/completion.test.ts lines 3-3 and 40-40, import
createCommandOutput, add it to the completion context fixture, and remove the as
unknown as CliCommandContext cast. In
src/application/commands/file/cleanup.test.ts lines 1-1, 31-31, 58-58, 85-85,
and 114-114, import createCommandOutput and provide an output handle for each
cleanup invocation.
In `@src/application/commands/skills/recommend/suppression-command.ts`:
- Line 58: Align both command schemas with the shared output format options: in
src/application/commands/skills/recommend/suppression-command.ts at lines 58-58,
widen suppressionFormatValues and its schema to accept "text" alongside "json";
in src/application/commands/skills/uninstall.ts at lines 99-99, widen
SkillsUninstallInput.format to accept "text" as well.
---
Nitpick comments:
In `@src/application/commands/command-output.ts`:
- Around line 93-109: Extract the shared json-only/output-format precedence
logic from resolveStrictOutputFormat into an exported helper such as
resolveDeclaredOutputFormat(optionValues, mode), and use it for the function’s
final return. In src/application/commands/command-output.ts lines 93-109, retain
the existing strict validation and delegate only the format resolution; in
src/adapters/commander/commander-cli-adapter.ts lines 284-306, replace the
inline derivation of the outputFormat telemetry field with this helper.
🪄 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
Run ID: 5c1a3b02-839d-4835-b18e-41f43e12207b
📒 Files selected for processing (44)
src/adapters/commander/commander-cli-adapter.test.tssrc/adapters/commander/commander-cli-adapter.tssrc/application/commands/auth/index.cli.test.tssrc/application/commands/auth/status.tssrc/application/commands/check-update.tssrc/application/commands/command-output.test.tssrc/application/commands/command-output.tssrc/application/commands/completion.test.tssrc/application/commands/connector/apps.tssrc/application/commands/connector/proxy.tssrc/application/commands/connector/run.tssrc/application/commands/connector/schema.tssrc/application/commands/connector/search.tssrc/application/commands/file/cleanup.test.tssrc/application/commands/file/cleanup.tssrc/application/commands/file/download.test.tssrc/application/commands/file/list.tssrc/application/commands/file/upload.tssrc/application/commands/info.cli.test.tssrc/application/commands/info.tssrc/application/commands/json-output.test.tssrc/application/commands/json-output.tssrc/application/commands/llm/config.tssrc/application/commands/llm/json.tssrc/application/commands/search.tssrc/application/commands/skills/check-update.tssrc/application/commands/skills/install.tssrc/application/commands/skills/list.tssrc/application/commands/skills/recommend/plan.tssrc/application/commands/skills/recommend/suppression-command.tssrc/application/commands/skills/repair.tssrc/application/commands/skills/search.test.tssrc/application/commands/skills/search.tssrc/application/commands/skills/sync.tssrc/application/commands/skills/uninstall.tssrc/application/commands/skills/update.tssrc/application/commands/team/current.tssrc/application/commands/team/list.tssrc/application/commands/variables/create.tssrc/application/commands/variables/delete.tssrc/application/commands/variables/get.tssrc/application/commands/variables/list.tssrc/application/commands/version.tssrc/application/contracts/cli.ts
💤 Files with no reviewable changes (2)
- src/application/commands/json-output.test.ts
- src/application/commands/json-output.ts
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.
Part 1/3 of the output-contract deepening (candidate 08 of the architecture review; spec in
.scratch/output-contract/spec.md, local-only).What
Deepens the shallow
json-output.ts(an option array + a JSON.stringify writer) intocommand-output.ts, the single owner of the JSON/text output contract:resolveOutputFormatis now the one--json/--formatprecedence rule. The commander adapter's private telemetry copy is deleted; theonCommandResolvedevent imports the shared rule (with ajson-onlyternary so JSON-only commands report"json").createCommandOutputbuilds the per-invocationCommandOutputhandle ({format, emit, emitJson}) that handlers receive via the newCliCommandContext(CliExecutionContext+output). Commands that declare an output mode get strict--formatvalidation beforeparseInputand the shared option definitions attached by the adapter. Undeclared commands get an inert text handle — nothing changes for them.CliCommandDefinitiongainsoutput?: "standard" | "json-only". No command sets it yet; migration follows in the stacked PR.jsonOutputOptionsis renamed tooutputFormatOptionseverywhere (same array, honest name — the contract covers text too).Behavior
No behavior change. No command declares an output mode yet, and the telemetry-rule relocation evaluates identically for every existing command and flag combination.
Tests
Table-driven module tests over the new seam (lenient precedence, strict rejection with the offending value, inert handle, json-only pinning, emit routing, schemaVersion envelope). Three new adapter tests cover option attachment, pre-handler strict rejection, and the json-only telemetry event. Handler-invoking test fabrication sites gained the
outputmember (compiler-verified set).Stack