Skip to content

chore(cli): declare and verify command output (CLI-2961) - #7606

Open
mdonnalley wants to merge 3 commits into
mikedonnalley/cli-2959-land-the-command-fact-registry-andfrom
mikedonnalley/cli-2961-declare-and-verify-command-output
Open

mdonnalley wants to merge 3 commits into
mikedonnalley/cli-2959-land-the-command-fact-registry-andfrom
mikedonnalley/cli-2961-declare-and-verify-command-output

Conversation

@mdonnalley

Copy link
Copy Markdown
Contributor

Summary

Declares a command's JSON result as a command fact and checks it in tests, without changing how handlers write output. branches delete is the first command to adopt it.

  • CommandOutputAnnotation (supabase/CommandOutput) stores a command's output schema; withOutput attaches it. Handlers keep writing through Output.
  • The registry gains the test and report-only check kinds. The output fact is registered as test-checked, and the catalog shows its JSON Schema.
  • mockOutput takes an optional contract. In json and stream-json it fails a result that doesn't match the schema (including undeclared keys) or a second result write. verifyCompletion also fails a handler that succeeds without writing a result, and leaves a failing handler's own error unchanged.
  • The docs spec publishes a declared schema as json_output_schema (draft 2020-12) and omits the field otherwise.
  • An adoption test requires output, target policy and permissions on every executable command. Output omissions are listed in OUTPUT_EXCEPTIONS, with logs and functions serve marked as streaming. An output exception can't excuse another missing fact, and an exception that no longer applies fails the test, so the list can only shrink.

Not in this PR: result ownership, write ordering, cleanup and compile-time enforcement stay with the deferred boundary work. Report-only production checks have no implementation yet.

Stacked on #7586.

Linked issue

CLI-2961

@mdonnalley
mdonnalley requested a review from a team as a code owner November 12, 2026 17:32
@mdonnalley
mdonnalley requested review from Coly010 and removed request for a team November 12, 2026 17:32

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI Review

Confirmed Claude's single finding: the declared output contract omits the target field added by production JSON output, while the mock hides this mismatch. Codex reported no findings. Verification used code inspection; tests were not run because dependencies are absent.

Findings

Severity Location Category Sources Claim
🟠 MAJOR apps/cli/src/commands/branches/delete/delete.output.ts:6 correctness claude The branches delete output contract omits target, although production JSON output adds it when a target is announced, including linked-parent resolution without explicit target flags. The published json_output_schema therefore disagrees with actual output. The new contract test misses this because mockOutput validates the payload without adding the announced target.

Stats

Claude findings: 1 · Codex findings: 0 · Confirmed: 1 · Refuted: 0 · Uncertain: 0


Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: auto · Workflow run

This review runs once per PR. A maintainer can request another with a /ai-review comment.

Comment on lines +6 to +10
export const branchesDeleteOutput = make(
Schema.Struct({
project_ref: Schema.String.annotate({ description: "Project ref of the deleted branch." }),
message: Schema.Literal("Deleted preview branch"),
}),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 MAJOR · correctness · source: claude

The branches delete output contract omits target, although production JSON output adds it when a target is announced, including linked-parent resolution without explicit target flags. The published json_output_schema therefore disagrees with actual output. The new contract test misses this because mockOutput validates the payload without adding the announced target.

Evidence: delete.output.ts:6-10 declares only project_ref and message. delete.handler.ts:34-37 resolves the parent target; command-internal/target/parent-scoped-project-ref.ts:36-46 announces it. shared/output/output.layer.ts:62-67 adds target, and :602-605/:649 routes JSON success through that merge. command-facts/command-output.ts:30-32 rejects undeclared keys. docs/docs-spec.ts:448-450 publishes the contract directly. tests/helpers/mocks.ts:419-422 validates only {...data, message}, despite tests/helpers/command-mocks.ts:1501-1505 providing the target cell.

Suggested fix: Include an optional target using the shared CommandTargetJson schema, and make mockOutput apply production's target merge before contract validation. Add coverage that validates output after announcing a linked target.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant