feat(cli): make defaultOnResult the default for buildCliCommands [OS-605]

[galligan]

Summary

buildCliCommands now uses defaultOnResult by default, so handler results are automatically output based on CLI flags (--output, --json, --jsonl). Previously, success values were silently discarded unless consumers explicitly passed onResult: defaultOnResult — a pattern that was non-obvious enough that consumers kept writing their own withOutput() wrappers instead.

Fixes https://linear.app/outfitter/issue/OS-605/surface-layer-output-handling-make-buildclicommands-handle-output-by

What changed

packages/cli/src/actions.ts:

• buildCliCommands defaults onResult to defaultOnResult instead of undefined
• onResult: null explicitly opts out (silently discards success values, only throws errors)
• Updated TSDoc to document the new default and opt-out

apps/outfitter/src/cli.ts:

• Added onResult: null since outfitter's actions handle their own output internally

The principle

Handlers return data. Surfaces present it. This was already the intended design — defaultOnResult existed and worked. This change makes it the default rather than an opt-in that nobody discovers.

Follow-up needed

Scaffold templates in plugins/outfitter/shared/templates/ still show handlers calling output() directly. These should be updated to reflect the new pattern (handlers return Result, surface handles output). Filed as a follow-up — not blocking since onResult: null provides the escape hatch for existing consumers.

Test plan

• All 910 CLI tests pass
• Typecheck passes (@outfitter/cli, outfitter)
• Outfitter app opts out with onResult: null — no double-output

🤘🏻 In-collaboration-with: Claude Code

[linear]

OS-605 Surface-layer output handling — make `buildCliCommands` handle output by default

[galligan]

• fix(ci): prevent spurious CI Summary failures after force-push [OS-616] #813
• feat(cli): make defaultOnResult the default for buildCliCommands [OS-605] #812 👈 (View in Graphite)
• feat(cli): derive variadic CLI flags from z.array() types [OS-615] #810
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[greptile-apps]

Greptile Summary

This PR makes defaultOnResult the automatic default for buildCliCommands, so handler results are output based on CLI flags without any extra wiring. It also introduces onResult: null as the explicit opt-out. The apps/outfitter CLI is updated to opt out since its actions still handle output directly.

Key changes:

• buildCliCommands resolves onResult: null → undefined and onResult: undefined → defaultOnResult before delegating to internal helpers — clean and correct
• Changeset is correctly tagged as major since any consumer of buildCliCommands that previously relied on success values being silently discarded (no onResult) will now get unexpected output
• The defaultOnResult TSDoc was not updated and still describes the old explicit-pass pattern, which now conflicts with the updated buildCliCommands docs
• The updated BuildCliCommandsOptions.onResult TSDoc dropped the important warning that custom callbacks are solely responsible for error handling — a behavioral contract that hasn't changed in runAction

Confidence Score: 4/5

Safe to merge — the runtime logic is correct and the breaking change is properly versioned; only documentation gaps remain.

The null→undefined mapping is correct, all three call-sites in buildCliCommands use the resolved onResult, and the opt-out in apps/outfitter/src/cli.ts is applied correctly. The only concerns are documentation: the defaultOnResult TSDoc is stale and the interface TSDoc omits the critical note that custom callbacks must handle errors themselves.

packages/cli/src/actions.ts — specifically the defaultOnResult TSDoc (lines 514–525) and the BuildCliCommandsOptions.onResult field doc (lines 48–57).

Important Files Changed

Filename	Overview
packages/cli/src/actions.ts	Core change: buildCliCommands now resolves onResult to defaultOnResult when not provided, converting null to undefined for internal runAction calls. Logic is correct, but the updated TSDoc omits the critical note that custom onResult callbacks are fully responsible for error handling.
apps/outfitter/src/cli.ts	Correctly opts out of the new default by passing onResult: null, with a clear inline comment. No issues.
.changeset/cli-default-on-result.md	Correctly marks the change as major for @outfitter/cli since it alters default runtime behavior for all existing consumers of buildCliCommands.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["buildCliCommands(source, options)"] --> B{options.onResult}
    B -- "=== null" --> C["onResult = undefined\nopt-out: silently discard success"]
    B -- "=== undefined not set" --> D["onResult = defaultOnResult\nNEW DEFAULT"]
    B -- "custom function" --> E["onResult = custom fn\ncaller responsible for errors"]

    C --> F["runAction with onResult"]
    D --> F
    E --> F

    F --> G{onResult truthy}
    G -- "Yes" --> H["await onResult and return"]
    G -- "No undefined" --> I{result.isErr}
    I -- "Yes" --> J["throw result.error"]
    I -- "No" --> K["return success discarded"]

    H --> L["defaultOnResult:\nthrow if err else output value"]

Loading

Comments Outside Diff (2)

1. packages/cli/src/actions.ts, line 48-57 (link)

   Custom onResult error-handling responsibility no longer documented

   The previous TSDoc explicitly warned: "When provided, handler errors are not auto-thrown — the callback is responsible for error handling." This is still the actual runtime behavior (runAction lines 304–311 return immediately after calling onResult, bypassing the auto-throw), but the new docs only describe the default and the null opt-out — the custom-callback case is completely silent.

   A consumer who provides a custom onResult that doesn't call throw ctx.result.error for error results will silently swallow errors. Since defaultOnResult does throw, users extending it may not realize they need to replicate that behavior. Consider adding a note like:

2. packages/cli/src/actions.ts, line 514-525 (link)

   defaultOnResult TSDoc is now stale

   The function's description says "Pass this to buildCliCommands({ onResult: defaultOnResult }) to get automatic output…" and the @example block shows exactly that pattern. However, the whole point of this PR is that you no longer need to pass it explicitly — it's the new default. This directly contradicts the updated buildCliCommands example, which removed the explicit onResult: defaultOnResult call.

   The TSDoc and example should be updated to reflect the new role of defaultOnResult:

   typescript

   • // Extend the default with extra logging
   • const commands = buildCliCommands(registry, {
   • onResult: async (ctx) => {

   • logger.info("action completed", { id: ctx.action.id });
     

   • await defaultOnResult(ctx);
     

   • },
   • });

   */

Prompt To Fix All With AI

This is a comment left during a code review.
Path: packages/cli/src/actions.ts
Line: 48-57

Comment:
**Custom `onResult` error-handling responsibility no longer documented**

The previous TSDoc explicitly warned: *"When provided, handler errors are not auto-thrown — the callback is responsible for error handling."* This is still the actual runtime behavior (`runAction` lines 304–311 return immediately after calling `onResult`, bypassing the auto-throw), but the new docs only describe the default and the `null` opt-out — the custom-callback case is completely silent.

A consumer who provides a custom `onResult` that doesn't call `throw ctx.result.error` for error results will silently swallow errors. Since `defaultOnResult` does throw, users extending it may not realize they need to replicate that behavior. Consider adding a note like:

```suggestion
  /**
   * Called after each handler returns with `Result.ok` or `Result.err`.
   *
   * Defaults to {@link defaultOnResult}, which outputs success values
   * based on CLI flags (`--output`, `--json`, `--jsonl`) and throws errors.
   * Pass `null` to disable and silently discard success values.
   *
   * When a custom function is provided, it is **fully responsible for error
   * handling** — errors are not auto-thrown. The callback receives the raw
   * `Result` and must throw (or otherwise surface) errors itself.
   */
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: packages/cli/src/actions.ts
Line: 514-525

Comment:
**`defaultOnResult` TSDoc is now stale**

The function's description says "Pass this to `buildCliCommands({ onResult: defaultOnResult })` to get automatic output…" and the `@example` block shows exactly that pattern. However, the whole point of this PR is that you no longer need to pass it explicitly — it's the new default. This directly contradicts the updated `buildCliCommands` example, which removed the explicit `onResult: defaultOnResult` call.

The TSDoc and example should be updated to reflect the new role of `defaultOnResult`:

```suggestion
/**
 * Default `onResult` callback that outputs success values and throws errors.
 *
 * This is automatically used by {@link buildCliCommands} unless overridden.
 * Import and reference it directly when composing a custom callback that
 * partially delegates to the default behavior.
 *
 * @example
 * ```typescript
 * // Extend the default with extra logging
 * const commands = buildCliCommands(registry, {
 *   onResult: async (ctx) => {
 *     logger.info("action completed", { id: ctx.action.id });
 *     await defaultOnResult(ctx);
 *   },
 * });
 * ```
 */
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (10): Last reviewed commit: "chore: add changeset" | Re-trigger Greptile}

[galligan]

@greptileai review

[galligan]

Merge activity

• Mar 26, 11:48 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Mar 26, 11:49 PM UTC: Graphite rebased this pull request as part of a merge.
• Mar 26, 11:52 PM UTC: Graphite rebased this pull request as part of a merge.
• Mar 26, 11:53 PM UTC: @galligan merged this pull request with Graphite.