yuris/life-service
1
0
Fork 0
Code Issues Pull requests Projects Releases 6 Packages 1 Wiki Activity Actions

fix(cli): unify not_found exit code to 2 across commands #78

Merged
yuris merged 3 commits from feat/issue-49-exit-codes into main 2026-09-23 05:11:33 +00:00
Conversation 3 Commits 3 Files changed 8 +390 -22
yuris commented 2026-09-23 04:43:14 +00:00
Owner
Copy link

Motivation

Two slices shipped the same error class with two different CLI exit codes: life item get <unknown> exited 1 while life operation get <unknown> exited 2, both as HTTP 404 with error.code == "not_found". The two originating issue specs disagreed (docs/issues/009-… said "HTTP 404, CLI exit 1"; docs/issues/006-… said "If 404: exit 2").

docs/technical/cli-api.md defines exit 2 as "Invalid input/configuration" and exit 1 as "Other failure, including authentication/authorization". A caller naming an ID this service does not hold made an input error, not an infrastructure failure, so exit 2 is correct and life item get was the outlier. Issue #49 asks for one answer everywhere rather than a per-command patch.

Changes

  • src/api/mod.rs — ApiFailure::not_found now carries exit_codes::INVALID_INPUT instead of GENERAL_ERROR (HTTP status stays 404, retryable stays false). This is the single source every route builds the failure from.
  • src/client.rs — the shared CLI exit map (mapped_exit_code) moves not_found from the exit-1 group into the input-class group, so a not_found envelope exits 2 whatever HTTP status carried it.
  • src/cli.rs — the per-command not_found early return in operation_exit_code is deleted as redundant: life operation get's error envelope now derives exit 2 from the shared map, while the function's remaining job — the operation's own recorded state deciding the settled (0) and not-established (6) codes — is unchanged. life operation get is the only command in src/cli.rs that overrode a daemon error code; the other code comparison there (report_conflict_hint) only prints a stderr hint.
  • skills/life/references/errors.md — the not_found row now documents exit 2, and the parenthetical that existed only to describe life operation get as an exception is gone. The "Recovery by operation ID" section already said exit 2 for an unknown operation; it is now simply the general rule.

Routes audited (grep -rn "not_found" src/)

Every producer of a top-level not_found error code, and the exit code it now yields (all exit 2, all through the one shared map):

Route Producer Command reaching it
GET /v1/items/{id} src/api/items.rs (not_found, 4 branches: removal in history, non-removal record, no record, unreadable history) life item get
GET /v1/operations/{id} src/api/operations.rs life operation get
GET /v1/proposals/{id} src/api/proposals.rs life proposal get
POST /v1/proposals/{id}/apply src/api/proposals.rs life proposal apply
POST /v1/proposals/{id}/reject src/api/proposals.rs life proposal reject
POST /v1/tasks/{id}/{complete,reopen,cancel} src/api/lifecycle.rs life task complete|reopen|cancel
PATCH /v1/tasks/{id}, PATCH /v1/events/{id} src/api/update.rs life task update, life event update
POST /v1/plans (an intended_outcomes id no live task carries) src/api/plans.rs life plan
POST /v1/context, POST /v1/context/search (an item_id absent from the live calendar) src/api/context.rs life context add, life context search
Scoped-mutation preflight (a resource no longer holding the named component) src/api/mod.rs (ScopeFailure::Refused) any --scope mutation (life task update --scope …, life event update --scope …)
Any path the daemon does not route src/api/mod.rs router fallback (ErrorItem::new("not_found", …)) every command, when the daemon serves no such route

Two not_found strings that are not envelope surfaces, checked so the audit is not mistaken for a claim about them:

  • src/calendar/mod.rs — CalendarError::error_code() maps ItemNotFound/CollectionNotFound to the string "not_found", but that mapping is never called on a response path (both callers, live_read_failure in src/api/items.rs and the coverage classification in src/api/agenda.rs, turn calendar failures into calendar_unsupported/dependency_unavailable); its only caller is an in-file test. CalendarError::exit_code() is not called anywhere and still ends in _ => GENERAL_ERROR, which would be a second, contradictory mapping table if it were used.
  • src/hindsight/client.rs — HindsightError::NotFound reports the code "not_found", but it surfaces as a warning code (context_unavailable) or an outbox failure code, never as the envelope's error.code.

One exception, and why it is not an inconsistency

not_found as the error member of a partial envelope exits 6, not 2. The status is the semantic outcome and decides before the error code (client::exit_code_for), so POST /v1/replans — which records the reported interruption as a fact and then fails to resolve a named remaining_intended_outcomes id — answers HTTP 200 with status: "partial", error.code: "not_found", proposal: null and exits 6. Reading that as exit 2 would tell a caller "repair your input" when the durable fact has already been recorded and the proposal was simply not generated.

This is a pre-existing rule this change does not touch: partial always exits 6. The exception was found during this change's own route audit, before any review, and is recorded here rather than papered over by a claim of one uniform exit code. It is pinned at the route by tests/replan_tests.rs::the_fact_stands_when_the_named_outcomes_cannot_be_resolved (status partial + error.code == "not_found"), at the CLI by the new a_partial_envelope_carrying_not_found_still_exits_six, and for the general precedence rule by tests/cli_protocol_tests.rs::the_semantic_outcome_decides_before_the_http_status. Everywhere else — a terminal error-status envelope — not_found is exit 2.

Deliberate consequence: the two rows that are not per-command reads — POST /v1/plans, POST /v1/context*, the scoped preflight and the router fallback — also move from exit 1 to exit 2 when they answer not_found as a terminal error. That is the point of "the mapping is uniform", and exit 2 is the documented class for an input the caller must correct (including a route this daemon version does not serve).

Verification

Reproduced before the fix with the new end-to-end test against a real daemon over TCP (mock CalDAV backend + real journal), then re-run after:

  • Pre-fix, an_unknown_item_id_is_not_found_with_exit_two failed with left: 1, right: 2 — i.e. HTTP 404, error.code == "not_found", CLI exit 1 for life item get <unknown>.
  • Pre-fix, an_unknown_operation_id_is_not_found_with_exit_two passed — HTTP 404, error.code == "not_found", CLI exit 2 for life operation get <unknown>.
  • Post-fix both pass; the same test also asserts the envelope shape (schema_version: 1, status: "error", data: null, empty warnings, error.retryable == false) and that a present item still exits 0 through the same route.
  • The CLI-boundary state matrix for life operation get is now complete, each state observed through the real binary against a real daemon: applied → 0 (crash_recovery_tests.rs::operation_get_calls_the_operation_route_and_exits_zero_when_settled), needs_reconciliation → 6 (operation_get_exits_six_when_the_outcome_is_unresolved), failed → 0 (dual_agent_acceptance.rs), and — new here — a durably prepared row → 6 (operation_get_exits_six_for_a_durably_prepared_operation, which seeds the R05 crash point with the existing seed_prepared_create fixture, asserts nothing was dispatched, and asserts exit 6 with data.state == "prepared" and no error). The prepared case previously stopped at the HTTP envelope and never invoked the CLI, which left the mapping this change trimmed unguarded at the boundary an agent uses.
  • a_partial_envelope_carrying_not_found_still_exits_six observes the exception end-to-end: the real client binary against a daemon answering the partial + not_found shape exits 6 and keeps error.code == "not_found" in its envelope. (It is a multi-threaded test because it spawns the real binary while the canned daemon serves on the same runtime; a single-threaded runtime deadlocks the daemon and the client reports a 30 s timeout — observed while writing it, fixed by the runtime flavour, not by loosening an assertion.)

tests/issue49_exit_codes.rs (new) observes both commands at the boundary an agent uses — the route's HTTP status and error.code, and the CLI's exit code — rather than asserting the mapping table against itself.

Adjusted tests, with reasons:

  • tests/agenda_tasks_tests.rs — test_item_get_cli_end_to_end_against_daemon pinned the old contract (assert_eq!(…, exit_codes::GENERAL_ERROR) for an absent id, with the comment "Absent id maps to exit 1 (not_found)"). It now asserts INVALID_INPUT; this is the assertion the issue exists to change, not a weakened check — the assertion is still an exact exit-code comparison.
  • tests/crash_recovery_tests.rs — one test added (operation_get_exits_six_for_a_durably_prepared_operation); no existing assertion in that suite was changed.
  • tests/cli_protocol_tests.rs — the_exit_code_map_covers_every_code_the_service_can_emit keeps its totality assertion intact; its route-code row for not_found was updated from 1 to 2 so the table still states what the map does. No assertion was removed or relaxed.

Suite and linters, run in the worktree with the repo's devshell:

  • cargo fmt then cargo fmt --check — clean.
  • cargo test — 747 passed, 0 failed. Counting method: the summed N passed of the 43 test result lines the workspace emits (743 on main + 4 new tests: 3 in tests/issue49_exit_codes.rs, 1 in tests/crash_recovery_tests.rs).
  • cargo clippy --all-targets -- -D warnings — 0 errors.

Documentation

  • skills/life/references/errors.md is updated (exit column and the now-redundant exception note). tests/skill_package_tests.rs::skill_package_documents_the_exit_the_cli_really_returns_per_code re-derives each documented exit from the CLI's own map, and passes.
  • docs/technical/ is the specification and is deliberately not committed from this worktree (docs/ is excluded in .git/info/exclude; git ls-files docs is empty). No normative text was edited or needs to be: cli-api.md's exit table does not assign 404 to an exit, so this change does not contradict it — it resolves the silence the way issue #49 states. No docs/technical/verification.md scenario covers exit-code uniformity; the issue's acceptance criteria are the contract for this change.

Closes #49

## Motivation Two slices shipped the same error class with two different CLI exit codes: `life item get <unknown>` exited **1** while `life operation get <unknown>` exited **2**, both as HTTP 404 with `error.code == "not_found"`. The two originating issue specs disagreed (`docs/issues/009-…` said "HTTP 404, CLI exit 1"; `docs/issues/006-…` said "If 404: exit 2"). `docs/technical/cli-api.md` defines exit 2 as "Invalid input/configuration" and exit 1 as "Other failure, including authentication/authorization". A caller naming an ID this service does not hold made an input error, not an infrastructure failure, so exit 2 is correct and `life item get` was the outlier. Issue #49 asks for one answer everywhere rather than a per-command patch. ## Changes - `src/api/mod.rs` — `ApiFailure::not_found` now carries `exit_codes::INVALID_INPUT` instead of `GENERAL_ERROR` (HTTP status stays 404, `retryable` stays false). This is the single source every route builds the failure from. - `src/client.rs` — the shared CLI exit map (`mapped_exit_code`) moves `not_found` from the exit-1 group into the input-class group, so a `not_found` envelope exits 2 whatever HTTP status carried it. - `src/cli.rs` — the per-command `not_found` early return in `operation_exit_code` is deleted as redundant: `life operation get`'s error envelope now derives exit 2 from the shared map, while the function's remaining job — the operation's own recorded state deciding the settled (0) and not-established (6) codes — is unchanged. `life operation get` is the only command in `src/cli.rs` that overrode a daemon error code; the other code comparison there (`report_conflict_hint`) only prints a stderr hint. - `skills/life/references/errors.md` — the `not_found` row now documents exit 2, and the parenthetical that existed only to describe `life operation get` as an exception is gone. The "Recovery by operation ID" section already said exit 2 for an unknown operation; it is now simply the general rule. ### Routes audited (`grep -rn "not_found" src/`) Every producer of a top-level `not_found` error code, and the exit code it now yields (all exit **2**, all through the one shared map): | Route | Producer | Command reaching it | |---|---|---| | `GET /v1/items/{id}` | `src/api/items.rs` (`not_found`, 4 branches: removal in history, non-removal record, no record, unreadable history) | `life item get` | | `GET /v1/operations/{id}` | `src/api/operations.rs` | `life operation get` | | `GET /v1/proposals/{id}` | `src/api/proposals.rs` | `life proposal get` | | `POST /v1/proposals/{id}/apply` | `src/api/proposals.rs` | `life proposal apply` | | `POST /v1/proposals/{id}/reject` | `src/api/proposals.rs` | `life proposal reject` | | `POST /v1/tasks/{id}/{complete,reopen,cancel}` | `src/api/lifecycle.rs` | `life task complete\|reopen\|cancel` | | `PATCH /v1/tasks/{id}`, `PATCH /v1/events/{id}` | `src/api/update.rs` | `life task update`, `life event update` | | `POST /v1/plans` (an `intended_outcomes` id no live task carries) | `src/api/plans.rs` | `life plan` | | `POST /v1/context`, `POST /v1/context/search` (an `item_id` absent from the live calendar) | `src/api/context.rs` | `life context add`, `life context search` | | Scoped-mutation preflight (a resource no longer holding the named component) | `src/api/mod.rs` (`ScopeFailure::Refused`) | any `--scope` mutation (`life task update --scope …`, `life event update --scope …`) | | Any path the daemon does not route | `src/api/mod.rs` router fallback (`ErrorItem::new("not_found", …)`) | every command, when the daemon serves no such route | Two `not_found` strings that are **not** envelope surfaces, checked so the audit is not mistaken for a claim about them: - `src/calendar/mod.rs` — `CalendarError::error_code()` maps `ItemNotFound`/`CollectionNotFound` to the string `"not_found"`, but that mapping is never called on a response path (both callers, `live_read_failure` in `src/api/items.rs` and the coverage classification in `src/api/agenda.rs`, turn calendar failures into `calendar_unsupported`/`dependency_unavailable`); its only caller is an in-file test. `CalendarError::exit_code()` is not called anywhere and still ends in `_ => GENERAL_ERROR`, which would be a second, contradictory mapping table if it were used. - `src/hindsight/client.rs` — `HindsightError::NotFound` reports the code `"not_found"`, but it surfaces as a warning code (`context_unavailable`) or an outbox failure code, never as the envelope's `error.code`. ### One exception, and why it is not an inconsistency `not_found` as the error member of a **`partial`** envelope exits **6**, not 2. The status is the semantic outcome and decides before the error code (`client::exit_code_for`), so `POST /v1/replans` — which records the reported interruption as a fact and then fails to resolve a named `remaining_intended_outcomes` id — answers HTTP 200 with `status: "partial"`, `error.code: "not_found"`, `proposal: null` and exits 6. Reading that as exit 2 would tell a caller "repair your input" when the durable fact has already been recorded and the proposal was simply not generated. This is a pre-existing rule this change does not touch: `partial` always exits 6. The exception was found during this change's own route audit, before any review, and is recorded here rather than papered over by a claim of one uniform exit code. It is pinned at the route by `tests/replan_tests.rs::the_fact_stands_when_the_named_outcomes_cannot_be_resolved` (status `partial` + `error.code == "not_found"`), at the CLI by the new `a_partial_envelope_carrying_not_found_still_exits_six`, and for the general precedence rule by `tests/cli_protocol_tests.rs::the_semantic_outcome_decides_before_the_http_status`. Everywhere else — a terminal `error`-status envelope — `not_found` is exit 2. Deliberate consequence: the two rows that are not per-command reads — `POST /v1/plans`, `POST /v1/context*`, the scoped preflight and the router fallback — also move from exit 1 to exit 2 when they answer `not_found` as a terminal error. That is the point of "the mapping is uniform", and exit 2 is the documented class for an input the caller must correct (including a route this daemon version does not serve). ## Verification Reproduced before the fix with the new end-to-end test against a real daemon over TCP (mock CalDAV backend + real journal), then re-run after: - Pre-fix, `an_unknown_item_id_is_not_found_with_exit_two` failed with `left: 1, right: 2` — i.e. HTTP 404, `error.code == "not_found"`, CLI exit **1** for `life item get <unknown>`. - Pre-fix, `an_unknown_operation_id_is_not_found_with_exit_two` passed — HTTP 404, `error.code == "not_found"`, CLI exit **2** for `life operation get <unknown>`. - Post-fix both pass; the same test also asserts the envelope shape (`schema_version: 1`, `status: "error"`, `data: null`, empty `warnings`, `error.retryable == false`) and that a *present* item still exits 0 through the same route. - The CLI-boundary state matrix for `life operation get` is now complete, each state observed through the real binary against a real daemon: `applied` → 0 (`crash_recovery_tests.rs::operation_get_calls_the_operation_route_and_exits_zero_when_settled`), `needs_reconciliation` → 6 (`operation_get_exits_six_when_the_outcome_is_unresolved`), `failed` → 0 (`dual_agent_acceptance.rs`), and — new here — a durably `prepared` row → 6 (`operation_get_exits_six_for_a_durably_prepared_operation`, which seeds the R05 crash point with the existing `seed_prepared_create` fixture, asserts nothing was dispatched, and asserts exit 6 with `data.state == "prepared"` and no `error`). The `prepared` case previously stopped at the HTTP envelope and never invoked the CLI, which left the mapping this change trimmed unguarded at the boundary an agent uses. - `a_partial_envelope_carrying_not_found_still_exits_six` observes the exception end-to-end: the real client binary against a daemon answering the `partial` + `not_found` shape exits 6 and keeps `error.code == "not_found"` in its envelope. (It is a multi-threaded test because it spawns the real binary while the canned daemon serves on the same runtime; a single-threaded runtime deadlocks the daemon and the client reports a 30 s timeout — observed while writing it, fixed by the runtime flavour, not by loosening an assertion.) `tests/issue49_exit_codes.rs` (new) observes both commands at the boundary an agent uses — the route's HTTP status and `error.code`, and the CLI's exit code — rather than asserting the mapping table against itself. Adjusted tests, with reasons: - `tests/agenda_tasks_tests.rs` — `test_item_get_cli_end_to_end_against_daemon` pinned the old contract (`assert_eq!(…, exit_codes::GENERAL_ERROR)` for an absent id, with the comment "Absent id maps to exit 1 (not_found)"). It now asserts `INVALID_INPUT`; this is the assertion the issue exists to change, not a weakened check — the assertion is still an exact exit-code comparison. - `tests/crash_recovery_tests.rs` — one test added (`operation_get_exits_six_for_a_durably_prepared_operation`); no existing assertion in that suite was changed. - `tests/cli_protocol_tests.rs` — `the_exit_code_map_covers_every_code_the_service_can_emit` keeps its totality assertion intact; its route-code row for `not_found` was updated from 1 to 2 so the table still states what the map does. No assertion was removed or relaxed. Suite and linters, run in the worktree with the repo's devshell: - `cargo fmt` then `cargo fmt --check` — clean. - `cargo test` — 747 passed, 0 failed. Counting method: the summed `N passed` of the 43 `test result` lines the workspace emits (743 on `main` + 4 new tests: 3 in `tests/issue49_exit_codes.rs`, 1 in `tests/crash_recovery_tests.rs`). - `cargo clippy --all-targets -- -D warnings` — 0 errors. ## Documentation - `skills/life/references/errors.md` is updated (exit column and the now-redundant exception note). `tests/skill_package_tests.rs::skill_package_documents_the_exit_the_cli_really_returns_per_code` re-derives each documented exit from the CLI's own map, and passes. - `docs/technical/` is the specification and is deliberately not committed from this worktree (`docs/` is excluded in `.git/info/exclude`; `git ls-files docs` is empty). No normative text was edited or needs to be: `cli-api.md`'s exit table does not assign 404 to an exit, so this change does not contradict it — it resolves the silence the way issue #49 states. No `docs/technical/verification.md` scenario covers exit-code uniformity; the issue's acceptance criteria are the contract for this change. Closes #49 
An ID this service does not hold is caller input, not an infrastructure
failure: `cli-api.md` reserves exit 1 for "other failure, including
authentication/authorization". `ApiFailure::not_found` now carries
`INVALID_INPUT` and the shared exit map sends `not_found` to exit 2, so
every route that can answer it agrees.

The per-command `not_found` override in `life operation get` is removed as
redundant: the shared map derives the same code, while the operation's own
recorded state still decides the settled (0) and not-established (6) codes.
yuris changed title from fix(cli): unify not_found exit code to 2 across commands (#49) to fix(cli): unify not_found exit code to 2 across commands 2026-09-23 04:44:44 +00:00
yuris changed title from fix(cli): unify not_found exit code to 2 across commands to fix(cli): unify not_found exit code to 2 across commands (#49) 2026-09-23 04:55:46 +00:00 
A review of the audit found the one shape the uniformity claim must
exempt: `POST /v1/replans` records the interruption as a fact and then
fails to resolve a named remaining outcome, answering HTTP 200 with
`status: "partial"` and `error.code: "not_found"`. The status is the
semantic outcome and decides before the error code, so that envelope
exits 6, not 2 — a pre-existing rule this change does not touch.

The new test observes it through the real client binary, and the audit
section of the PR body now states the precedence instead of claiming one
exit code for every `not_found`.
yuris changed title from fix(cli): unify not_found exit code to 2 across commands (#49) to fix(cli): unify not_found exit code to 2 across commands 2026-09-23 04:56:39 +00:00
yuris commented 2026-09-23 05:04:49 +00:00
Author
Owner
Copy link

REQUEST CHANGES

Per-criterion review

Criterion Result Evidence
life item get <unknown-id> --json exits 2, remains HTTP 404, and returns error.code == "not_found" with the same envelope shape Met src/api/mod.rs:260-265, src/client.rs:91-101, and tests/issue49_exit_codes.rs::an_unknown_item_id_is_not_found_with_exit_two (:188) exercise the real router over TCP and the CLI dispatch path. tests/agenda_tasks_tests.rs::test_item_get_cli_end_to_end_against_daemon also keeps an exact exit-code assertion.
life operation get <unknown-id> --json still exits 2 Met tests/issue49_exit_codes.rs::an_unknown_operation_id_is_not_found_with_exit_two (:220) observes HTTP 404, the real envelope, and CLI exit 2. The redundant command-specific override is gone and the shared map supplies 2.
not_found is classified correctly on every affected terminal surface Met The producers in items, operations, proposals, lifecycle/update, plans, context, scoped preflight, and router fallback converge through ApiFailure::not_found and/or client::mapped_exit_code. Exit 2 is appropriate for plans/context and scoped preflight because the caller must correct or reread the named resource; the refusal occurs after authorization and is not an authorization failure. Exit 2 is also appropriate for an unrouted path: it is a client route/version mistake, whereas cli-api.md reserves exit 1 for authentication/authorization/other failures.
Semantic precedence is preserved for partial + not_found Met on final head 8f2a596 tests/replan_tests.rs::the_fact_stands_when_the_named_outcomes_cannot_be_resolved (:760) proves the real route emits HTTP 200, status: partial, and error.code: not_found. The follow-up tests/issue49_exit_codes.rs::a_partial_envelope_carrying_not_found_still_exits_six (:289) spawns the real life binary against a canned daemon and observes exit 6 plus the unchanged envelope. It is not a restatement of the mapping function. tests/cli_protocol_tests.rs::the_semantic_outcome_decides_before_the_http_status (:316) pins the general precedence rule. I independently found the same replan exception during review, but did not drive the follow-up commit; I reviewed the resulting final-head test and route citation.
No unrelated exit behavior changes Met The production diff changes only the not_found mapping and removes its redundant operation-specific override. The pre-existing partial-envelope rule still wins and exits 6.
life operation get state mapping remains pinned end to end for applied, failed, prepared, and needs_reconciliation Partially met Applied is covered by crash_recovery_tests.rs::operation_get_calls_the_operation_route_and_exits_zero_when_settled (:1210); needs-reconciliation by operation_get_exits_six_when_the_outcome_is_unresolved (:1242); failed is exercised by the real-client dual-agent acceptance route (dual_agent_acceptance.rs:827-918). The prepared-state check at crash_recovery_tests.rs:794-795 stops at the HTTP envelope and never invokes life operation get, so prepared → exit 6 is not pinned at the CLI boundary.
the_exit_code_map_covers_every_code_the_service_can_emit remains a real behavior table Met tests/cli_protocol_tests.rs:199-238 enumerates actual ApiFailure constructors and compares both the shared code map and envelope+HTTP behavior to each constructor's exit code; the route-code table keeps an exact not_found row at :288. No assertion was relaxed.
New regression test exercises the real path and would fail before the fix Met from code/diff The item test starts a real TCP daemon backed by the real router and journal, then invokes CLI dispatch; it does not assert a constant in isolation. On main, ApiFailure::not_found and the shared client map both selected exit 1, so its exact exit-2 assertion would fail while the operation test's old per-command override would still pass.
Skill references are current and drift is caught Met skills/life/references/errors.md:50 and :80 now state exit 2. commands.md did not document a conflicting not_found exit. tests/skill_package_tests.rs::skill_package_documents_the_exit_the_cli_really_returns_per_code (:572) parses the error table and checks each documented row through the real CLI exit mapper; it passed.

Findings

  • major — src/cli.rs:2634-2639 — prepared operation state lacks the required CLI-boundary regression test — after trimming the command-specific override, the suite covers the CLI outcomes for applied, failed, and needs-reconciliation, but the only prepared assertion stops at the API envelope. A regression in the life operation get handling of a durable prepared row therefore lacks the requested end-to-end guard. Correct looks like invoking the real CLI against an operation durably left in prepared and asserting exit 6 and data.state == "prepared".

Verification I ran on final head 8f2a596

  • nix develop --command cargo test — 746 passed, 0 failed, across 41 Cargo suite results (38 integration test targets plus lib, main, and doc tests).
  • nix develop --command cargo clippy --all-targets -- -D warnings — clean; 0 warnings/errors.
  • nix develop --command cargo fmt --check — clean; no output.

PR-body claims

  • The behavioral claims for terminal not_found, the partial-envelope exception, and the final test count reproduced.
  • The sentence “cargo test — 746 passed, 0 failed across 43 suites” overstates the suite count: my final-head run produced the same 746/0 test total but 41 Cargo suite results.
  • The quoted pre-fix observation (left: 1, right: 2) was not independently rerun on a pre-fix checkout because this review targeted the final head without modifying tracked files. The diff does substantiate the behavior: the new test exercises the real route/CLI path, and both pre-fix mappings selected 1.

Could not check

  • Only the exact textual output of the author's pre-fix run was not reproduced; the pre-fix behavior itself is established by the base implementation and the regression-test path described above.
REQUEST CHANGES ## Per-criterion review | Criterion | Result | Evidence | |---|---|---| | `life item get <unknown-id> --json` exits 2, remains HTTP 404, and returns `error.code == "not_found"` with the same envelope shape | Met | `src/api/mod.rs:260-265`, `src/client.rs:91-101`, and `tests/issue49_exit_codes.rs::an_unknown_item_id_is_not_found_with_exit_two` (`:188`) exercise the real router over TCP and the CLI dispatch path. `tests/agenda_tasks_tests.rs::test_item_get_cli_end_to_end_against_daemon` also keeps an exact exit-code assertion. | | `life operation get <unknown-id> --json` still exits 2 | Met | `tests/issue49_exit_codes.rs::an_unknown_operation_id_is_not_found_with_exit_two` (`:220`) observes HTTP 404, the real envelope, and CLI exit 2. The redundant command-specific override is gone and the shared map supplies 2. | | `not_found` is classified correctly on every affected terminal surface | Met | The producers in items, operations, proposals, lifecycle/update, plans, context, scoped preflight, and router fallback converge through `ApiFailure::not_found` and/or `client::mapped_exit_code`. Exit 2 is appropriate for plans/context and scoped preflight because the caller must correct or reread the named resource; the refusal occurs after authorization and is not an authorization failure. Exit 2 is also appropriate for an unrouted path: it is a client route/version mistake, whereas `cli-api.md` reserves exit 1 for authentication/authorization/other failures. | | Semantic precedence is preserved for `partial + not_found` | Met on final head `8f2a596` | `tests/replan_tests.rs::the_fact_stands_when_the_named_outcomes_cannot_be_resolved` (`:760`) proves the real route emits HTTP 200, `status: partial`, and `error.code: not_found`. The follow-up `tests/issue49_exit_codes.rs::a_partial_envelope_carrying_not_found_still_exits_six` (`:289`) spawns the real `life` binary against a canned daemon and observes exit 6 plus the unchanged envelope. It is not a restatement of the mapping function. `tests/cli_protocol_tests.rs::the_semantic_outcome_decides_before_the_http_status` (`:316`) pins the general precedence rule. I independently found the same replan exception during review, but did not drive the follow-up commit; I reviewed the resulting final-head test and route citation. | | No unrelated exit behavior changes | Met | The production diff changes only the `not_found` mapping and removes its redundant operation-specific override. The pre-existing partial-envelope rule still wins and exits 6. | | `life operation get` state mapping remains pinned end to end for `applied`, `failed`, `prepared`, and `needs_reconciliation` | **Partially met** | Applied is covered by `crash_recovery_tests.rs::operation_get_calls_the_operation_route_and_exits_zero_when_settled` (`:1210`); needs-reconciliation by `operation_get_exits_six_when_the_outcome_is_unresolved` (`:1242`); failed is exercised by the real-client dual-agent acceptance route (`dual_agent_acceptance.rs:827-918`). The prepared-state check at `crash_recovery_tests.rs:794-795` stops at the HTTP envelope and never invokes `life operation get`, so prepared → exit 6 is not pinned at the CLI boundary. | | `the_exit_code_map_covers_every_code_the_service_can_emit` remains a real behavior table | Met | `tests/cli_protocol_tests.rs:199-238` enumerates actual `ApiFailure` constructors and compares both the shared code map and envelope+HTTP behavior to each constructor's exit code; the route-code table keeps an exact `not_found` row at `:288`. No assertion was relaxed. | | New regression test exercises the real path and would fail before the fix | Met from code/diff | The item test starts a real TCP daemon backed by the real router and journal, then invokes CLI dispatch; it does not assert a constant in isolation. On `main`, `ApiFailure::not_found` and the shared client map both selected exit 1, so its exact exit-2 assertion would fail while the operation test's old per-command override would still pass. | | Skill references are current and drift is caught | Met | `skills/life/references/errors.md:50` and `:80` now state exit 2. `commands.md` did not document a conflicting `not_found` exit. `tests/skill_package_tests.rs::skill_package_documents_the_exit_the_cli_really_returns_per_code` (`:572`) parses the error table and checks each documented row through the real CLI exit mapper; it passed. | ## Findings - **major — `src/cli.rs:2634-2639` — prepared operation state lacks the required CLI-boundary regression test** — after trimming the command-specific override, the suite covers the CLI outcomes for applied, failed, and needs-reconciliation, but the only prepared assertion stops at the API envelope. A regression in the `life operation get` handling of a durable prepared row therefore lacks the requested end-to-end guard. Correct looks like invoking the real CLI against an operation durably left in `prepared` and asserting exit 6 and `data.state == "prepared"`. ## Verification I ran on final head `8f2a596` - `nix develop --command cargo test` — **746 passed, 0 failed**, across **41 Cargo suite results** (38 integration test targets plus lib, main, and doc tests). - `nix develop --command cargo clippy --all-targets -- -D warnings` — clean; 0 warnings/errors. - `nix develop --command cargo fmt --check` — clean; no output. ## PR-body claims - The behavioral claims for terminal `not_found`, the partial-envelope exception, and the final test count reproduced. - The sentence **“`cargo test` — 746 passed, 0 failed across 43 suites”** overstates the suite count: my final-head run produced the same 746/0 test total but 41 Cargo suite results. - The quoted pre-fix observation (`left: 1, right: 2`) was not independently rerun on a pre-fix checkout because this review targeted the final head without modifying tracked files. The diff does substantiate the behavior: the new test exercises the real route/CLI path, and both pre-fix mappings selected 1. ## Could not check - Only the exact textual output of the author's pre-fix run was not reproduced; the pre-fix behavior itself is established by the base implementation and the regression-test path described above.
Review finding: the CLI-boundary state matrix for `life operation get`
covered `applied` (0), `needs_reconciliation` (6) and `failed` (0), but
`prepared` stopped at the HTTP envelope and never invoked the CLI — the
one state the trimmed `operation_exit_code` still has to classify from a
durable row.

The new test seeds the R05 crash point with the existing
`seed_prepared_create` fixture, asserts nothing was dispatched, and runs
the real binary against the real daemon: exit 6, `status: partial`,
`data.state == "prepared"`, no error member.
yuris changed title from fix(cli): unify not_found exit code to 2 across commands to fix(cli): unify not_found exit code to 2 across commands (#49) 2026-09-23 05:08:42 +00:00
yuris commented 2026-09-23 05:08:54 +00:00
Author
Owner
Copy link

Review finding fixed at head 89551bf.

tests/crash_recovery_tests.rs::operation_get_exits_six_for_a_durably_prepared_operation closes the CLI-boundary gap: it seeds the R05 crash point with the existing seed_prepared_create fixture, asserts nothing was dispatched, and runs the real life operation get op-cli-prepared --json against the real daemon over TCP — exit 6, status: partial, data.state == "prepared", no error member. The state matrix is now pinned at the CLI for all four states: applied 0, failed 0, needs_reconciliation 6, prepared 6.

Counts, with the method stated in the PR body: 747 passed / 0 failed, summed over the 43 test result lines the workspace emits; cargo clippy --all-targets -- -D warnings clean; cargo fmt --check clean.

The partial + not_found precedence exception (replans → exit 6) was found during this change's own route audit, before review; the PR body now says so and cites the route-level, CLI-level and precedence-rule tests that pin it.

Review finding fixed at head 89551bf. `tests/crash_recovery_tests.rs::operation_get_exits_six_for_a_durably_prepared_operation` closes the CLI-boundary gap: it seeds the R05 crash point with the existing `seed_prepared_create` fixture, asserts nothing was dispatched, and runs the real `life operation get op-cli-prepared --json` against the real daemon over TCP — exit 6, `status: partial`, `data.state == "prepared"`, no `error` member. The state matrix is now pinned at the CLI for all four states: applied 0, failed 0, needs_reconciliation 6, prepared 6. Counts, with the method stated in the PR body: 747 passed / 0 failed, summed over the 43 `test result` lines the workspace emits; `cargo clippy --all-targets -- -D warnings` clean; `cargo fmt --check` clean. The `partial + not_found` precedence exception (replans → exit 6) was found during this change's own route audit, before review; the PR body now says so and cites the route-level, CLI-level and precedence-rule tests that pin it.
yuris commented 2026-09-23 05:11:10 +00:00
Author
Owner
Copy link

APPROVE

Re-verified final head 89551bf1acb5f9bf35cd3652dadc3bd4d2749f96.

The new tests/crash_recovery_tests.rs::operation_get_exits_six_for_a_durably_prepared_operation genuinely observes the CLI boundary: it seeds a durable prepared row with seed_prepared_create, confirms PUT == 0, starts the real daemon over TCP, launches the real life operation get op-cli-prepared --json binary, and asserts exit 6 plus status: partial, data.state: prepared, the operation identity/intent, and null error. A regression of the prepared mapping would fail this test. The four-state matrix is now complete: applied → 0, failed → 0, needs_reconciliation → 6, prepared → 6.

No earlier findings remain open. The terminal not_found mapping is correctly exit 2; the real replan route's partial + not_found exception remains correctly exit 6 and is pinned by both route and real-binary tests. The 43 count is now clear: 43 test result lines (41 test binaries plus lib/main/doc-test result lines), summing to 747 passed and 0 failed.

Verification on this head:

  • nix develop --command cargo test: 747 passed, 0 failed, 43 test result lines.
  • nix develop --command cargo clippy --all-targets -- -D warnings: clean.
  • nix develop --command cargo fmt --check: clean.
APPROVE Re-verified final head `89551bf1acb5f9bf35cd3652dadc3bd4d2749f96`. The new `tests/crash_recovery_tests.rs::operation_get_exits_six_for_a_durably_prepared_operation` genuinely observes the CLI boundary: it seeds a durable `prepared` row with `seed_prepared_create`, confirms `PUT == 0`, starts the real daemon over TCP, launches the real `life operation get op-cli-prepared --json` binary, and asserts exit 6 plus `status: partial`, `data.state: prepared`, the operation identity/intent, and null error. A regression of the prepared mapping would fail this test. The four-state matrix is now complete: applied → 0, failed → 0, needs_reconciliation → 6, prepared → 6. No earlier findings remain open. The terminal `not_found` mapping is correctly exit 2; the real replan route's `partial + not_found` exception remains correctly exit 6 and is pinned by both route and real-binary tests. The 43 count is now clear: 43 `test result` lines (41 test binaries plus lib/main/doc-test result lines), summing to 747 passed and 0 failed. Verification on this head: - `nix develop --command cargo test`: 747 passed, 0 failed, 43 `test result` lines. - `nix develop --command cargo clippy --all-targets -- -D warnings`: clean. - `nix develop --command cargo fmt --check`: clean.
yuris changed title from fix(cli): unify not_found exit code to 2 across commands (#49) to fix(cli): unify not_found exit code to 2 across commands 2026-09-23 05:11:21 +00:00
yuris merged commit 9241cc0b93 into main 2026-09-23 05:11:33 +00:00
yuris changed title from fix(cli): unify not_found exit code to 2 across commands to fix(cli): unify not_found exit code to 2 across commands 2026-09-23 06:25:44 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
yuris/life-service!78
No description provided.