fix(cli): honest exit codes — stalled runs fail, --cwd validated, lookup misses exit 1

[plombeer31]

What

Exit codes that scripts and CI can trust. The CLI now separates usage errors from
operational failures:

exit	meaning
0	success
1	operational failure — invoked correctly, the work did not succeed (lookup misses included)
2	usage error — unknown flag/subcommand, missing required argument, argument of the wrong kind. Nothing was attempted

run, skill and the top-level dispatcher implement this. The split is documented on
CommandDescriptor.run in src/cli/index.ts, including the commands that do not
follow it, so a caller is not misled: config, serve, trace, task, models and
import predate the split and return 1 for usage errors too; trace replay returns 2
for "stable-prefix drift detected" (a diff-style result code); tui passes a relaunched
child's status straight through, so any code is possible; repl is a scaffold and always
returns 0. Widening the split to those is a separate change.

⚠️ Breaking

Reference point is pre-PR. Every previously-failing case was already non-zero, so scripts
testing if ! atomic-agent … are unaffected by the skill rows; scripts comparing against
a literal 1/2 need updating. The two run rows turn previously-silent successes into
failures — that is the point of the PR.

command / situation	before	after
run — turn exhausts agent.maxSteps (session stalled)	0	1
bare atomic-agent (TUI) — same stalled turn	0	1
run --cwd <nonexistent>	0 (booted, ENOENT'd every tool)	2
run --cwd <file> / --working-dir <file>	0	2
skill show <missing>	2	1
skill uninstall <missing>	2	1
skill install already installed	2	1
skill install <owner/repo> hub failure	2	1
skill install @owner/slug ClawHub failure	2	1
skill browse / search when every source failed and nothing was found	0	1
skill <unknown-subcommand>	1	2
skill install with no source	1	2
skill show|uninstall|enable|disable with no <name>	1	2
skill browse --source with no value	1	2
skill search with an empty query	1	2
skill tap add|remove with no repo, or a malformed owner/repo	1	2
skill tap <unknown-verb>	1	2

Known in-repo consumers. The eval harnesses gate on atomic-agent run exiting 0 —
eval/harness/run-case.ts:104 (the actual pass/fail gate), eval/cases.eval.ts:74,
eval/harness/append-jsonl-row.ts:109 (failure-reason telemetry), and the three
eval-memory/experiments/*/runner.ts. A scenario that exhausts the step budget now flags
as a subprocess failure where it previously scored as a normal run. Arguably correct — a
stalled turn is degraded — but it will move eval bookkeeping.

Why skill.ts moved in both directions

The original version of this PR claimed "2 means you invoked this wrong across the CLI"
while skill.ts did the opposite: missing-argument usage errors returned 1 and
operational failures returned 2. Fixing only skill show split it further —
show <missing> exiting 1 while uninstall <missing> exited 2 for the same class of error.
The file is now internally consistent, and the false blanket claim is replaced by the
documented contract above.

The stalled rule also lived in two places: run-agent.ts was updated and
chat-orchestrator.ts (the TUI, i.e. bare atomic-agent) was not, so the default command
still exited 0 on the exact condition this PR is named for. Both now read
isFailedSessionStatus from src/session/session-state.ts, so they cannot drift again.
cancelled deliberately stays a success — the operator asked to stop.

Testing

• npm run lint (tsc) clean, verified against the branch as pushed to origin.
• npx vitest run src/cli src/session — 100 passed.
• Full repo suite — 3996 passed / 8 failed, against 3982 / 7 on the same branch before
  these commits. The extra failure is llm-health-poller, a pre-existing flake reproduced
  at baseline (~40%, passes 12/12 solo); no new failure.
• New src/cli/run-agent.test.ts (5 tests) drives the real agent loop, tool registry
  and status transitions to step-budget exhaustion with only the LLM stubbed — the stalled
  assertion checks "status": "stalled" in the run summary, not a mock.
• src/session/session-exit-status.test.ts pins the shared predicate across every member
  of SessionStatus, so a future status cannot silently default to "success".
• Every new test verified to fail without its fix (reverting the production hunk and
  re-running): stalled → expected +0 to be 1; --cwd guard → both path tests fail;
  uninstall/show → expected 2 to be 1; usage codes → expected 1 to be 2;
  browse/search → expected 0 to be 1.

Not done here

• The six legacy commands still conflate 1. Documented rather than changed, to keep the
  breaking surface deliberate.
• skill enable <not-installed> returns 0 (disable at least warns).
• A cancelled session exits 0 in both run and the TUI. Probably intended, but a
  SIGINT-killed CI run exiting 0 is arguably the same class of dishonesty.

[sosidudku1]

The direction is right: a stalled run and a bad --cwd should not exit 0. Three things to sort before merge:

1. No tests for the new contract. A PR titled "honest exit codes" changes the return code in three places (stalled -> 1, missing --cwd -> 2, skill show miss -> 1) and adds no test for any of them. Exit codes are exactly the kind of contract that needs pinning, since scripts and CI read $?. Please add cases for each.

2. This is a breaking change and is not flagged as one. A one-shot echo "goal" | atomic-agent run that hits agent.maxSteps used to exit 0 and now exits 1; a bad --cwd used to run and exit 0 and now exits 2. Both are correct on the merits, but anyone parsing $? today will see new behavior, so it should be called out in the description (and the release notes when it ships).

3. The unification is half done. The PR justifies skill show 2 -> 1 by saying "2 means a malformed invocation" across the CLI, but skill uninstall <missing> (skill.ts:209-210) is the same lookup-miss category and still returns 2. After this PR skill show foo exits 1 while skill uninstall foo exits 2 for the same kind of error. Either bring uninstall in line too, or narrow the claim.

The changes themselves are small and correct once tests and the breaking-change note land.

[plombeer31]

All three addressed, and point 3 turned out to run deeper than the uninstall line.

3 first, since it drove the rest. You were right that uninstall was left behind — but
auditing the file showed skill.ts used the opposite convention from the one this PR
claimed: missing-argument usage errors returned 1, operational failures returned 2. So
"2 means you invoked this wrong" was false in the very file the PR edits, and fixing only
show had split the lookup-miss case in two. I took your first option — skill.ts is now
internally consistent (17 return sites), and the blanket claim is replaced by a documented
contract on CommandDescriptor.run that also names the commands which do not follow it:
the six legacy commands, trace replay (2 = drift detected), tui (passes a relaunched
child's status through), and repl. I deliberately did not widen the split to those six —
that would ship untested behaviour changes in files this PR never touched.

A bug the audit surfaced, worth its own mention. The stalled rule lived in two places.
run-agent.ts got fixed; chat-orchestrator.ts did not — and that is bare atomic-agent,
the default command, which was still exiting 0 on exactly the condition this PR is named
for. Both now read isFailedSessionStatus from src/session/session-state.ts, so they
cannot drift apart again. cancelled stays 0 on purpose (the operator asked to stop).

While there I also fixed skill browse/search returning 0 when every source errored and
nothing was found — reporting total failure as success, in the file being unified.

1. Tests. Every changed code is pinned, and each was verified to fail without its fix by
reverting the production hunk and re-running:

test	without the fix
stalled run → 1	expected +0 to be 1
--cwd <missing> / --working-dir <file> → 2	both fail
skill uninstall <missing> → 1	expected 2 to be 1
skill show <missing> → 1	expected 2 to be 1
usage errors → 2 (unknown subcommand, missing args, bad tap repo, valueless --source, empty query)	expected 1 to be 2
browse/search all-sources-failed → 1	expected 0 to be 1

The stalled test is not a mock: src/cli/run-agent.test.ts wraps only the bootstrap module
to stub llamaComplete, then drives the real loop, real tool registry and real status
transitions to step-budget exhaustion, asserting "status": "stalled" in the run summary
before the exit code. Runs in ~280 ms.

2. Breaking change. Flagged in the description with a full before/after table (17 rows).
It also names the in-repo consumers I found: the eval harnesses gate on run exiting 0 —
eval/harness/run-case.ts:104 is the actual pass/fail gate, plus eval/cases.eval.ts:74,
eval/harness/append-jsonl-row.ts:109 and the three eval-memory/experiments/*/runner.ts.
Scenarios that stall will now flag as subprocess failures rather than scoring as normal
runs. Correct on the merits, but it will move eval bookkeeping, so better said out loud.

Verification. tsc clean against the branch as pushed. vitest run src/cli src/session
— 100 passed. Full repo suite 3996 passed / 8 failed vs 3982 / 7 on the same branch before
these commits; the one extra is llm-health-poller, a pre-existing flake I reproduced at
baseline (~40%, 12/12 solo).

Left alone and listed under "Not done here": the six legacy commands, skill enable <not-installed> returning 0, and cancelled exiting 0 in both entry points.