fix(ops-mission-control): coded 503 on refused ledger writes (#7790)

[CrysisDeu]

Summary

Closes #7790.

The issue lists seven mutating ops-mission-control handlers whose store-write failures escaped as aiohttp's bare plain-text 500. Six of them (put/delete secret, settings, provider config, transition, decide_proposal) were already covered on main by #7788/#7794 with per-store coded refusals and tests. This PR closes the remaining class: the three ledger mutating routes.

Write half (the issue as filed). POST /ledger, DELETE /ledger, and POST /ledger/hygiene all rewrite ledger.jsonl through atomic_write under the ledger lock, which raises OSError on a refused write. Each now answers the shape _handle_rotation_arm established: {ok: false, error, code: "ledger_store_unwritable"}, status 503 (transient, retry is correct). The hygiene route additionally skips index/prune/push when the rewrite was refused — publishing a ledger the dedupe pass never committed to is worse than deferring to the next cron run — and audits the refusal as a failure.

Read half (found in pre-push review, GPT 5.6 — the worse one). ledger.read_entries collapses a failed open to [], and every locked read-modify-rewrite in ledger.py started from it. So a transient EACCES at hygiene time silently truncated the whole shared ledger, DELETE answered a coded 404 about an entry still on disk, and upsert re-created instead of merging. The five mutation paths (upsert, record_use, record_miss, remove, hygiene) now start from a new read_entries_for_update() that propagates OSError; the lenient read_entries remains for read-only callers (match, stats, GET routes) and delegates to it. Both record_* callers in dispatch.py already tolerate OSError (record_miss is wrapped; record_use propagates into the claim path's existing degrade-to-claim-without-matches arm).

Slug note: the issue text suggested incident_store_unwritable for the hygiene route, but the store that fails there is the ledger, not the incident index — the per-store naming rule the same suggestion establishes gives ledger_store_unwritable. The incident-index writes on transition/decide already answer dispatch_index_* codes via _store_read_refusal since #7794.

Changes

• src/kiro_crew/apps/builtins/ops_mission_control/backend/routes.py: coded 503 arms on the three ledger routes; hygiene 503 carries changed: false (the SOP branches on it) and the docstring's fault-tolerance paragraph now states the deliberate exception.
• src/kiro_crew/apps/builtins/ops_mission_control/backend/ledger.py: read_entries_for_update() (strict) extracted from read_entries() (lenient, now delegating); five mutation call sites switched.
• Tests: refused write → coded 503 for all three routes (hygiene one also pins push-is-skipped and the failure audit); failed underlying read on DELETE → 503, not 404; ledger-level pin that a failed mutation read refuses before any rewrite and the file survives intact.

Testing

• Local gates: isort, flake8, mypy (clean), diff-scoped black/brand/scrub gates. Test suites run in CI per operator instruction (no local test runs on this host).
• Pre-push review: GPT 5.6 lane ×2 (one PASS; one HIGH — the lenient mutation read — fixed above) and Opus lane (PASS; two advisories applied: docstring drift, changed: false).

No UI changes, no screenshots required.

Pattern harvest

Rule candidate: review checklist / semgrep
Pattern: a lenient reader that collapses a failed read to an empty default ([]/{}) feeding a locked read-modify-REWRITE — the rewrite publishes the collapsed default and silently truncates the store. Third recurrence in this app alone (app-config set_top_level, the incident index, now the ledger). Detectable shape: except OSError: return [] (or .get(..., {}) on a read) whose callers pass the result to a whole-file write (_write_all, atomic_write). Mutation paths must use a strict read that refuses; lenient defaults are for read-only callers.

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 05571af5e23d891543c0d31b8de7a04458c349e5 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

The truncation fault this PR names survives on one path: ledger_sync.resolve_conflict still rewrites from the lenient read — and then pushes.

Watch

• resolve_conflict() (ledger_sync.py:682-684) is a locked read_entries() → _write_all() rewrite outside ledger.py, so it dodges the new rule ("every locked read → mutate → _write_all in this module must start here"). A failed open there still collapses to [], _write_all([]) truncates the shared ledger, and the surrounding pull/push flow then commits and publishes it — the exact outcome the hygiene handler now refuses to push ("publishing a ledger the dedupe pass never committed to is worse"). Switch it to read_entries_for_update and let its existing OSError handling in pull/push absorb the escape.
• The strict/lenient split is enforced only by docstring convention, and resolve_conflict (per its own docstring, a caller "added afterwards" that missed the lock rule too) shows out-of-module mutators are the recurring blind spot. The short, obvious name (read_entries) is the dangerous one for a mutator; consider a test that greps for _write_all callers starting from the lenient read, mirroring how the lock rule was pinned.

[DESIGN-REVIEWED] 05571af

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 05571af5e23d891543c0d31b8de7a04458c349e5 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/apps/builtins/ops_mission_control/tests/test_routes.py:3457 -- New function-local imports here and in test_store_and_gate.py:1434 violate top-level-imports -> Fix: use existing routes/mock bindings and a dotted ledger_sync patch target.
[GPT-REVIEWED] 05571af

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 05571af5e23d891543c0d31b8de7a04458c349e5: <one-sentence reason>

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 05571af5e23d891543c0d31b8de7a04458c349e5 — this comment is updated in place on each push.

Review details

All three mutation route calls (upsert:2341, hygiene:2425, remove:2529) are exactly the ones the diff wraps in try/except OSError → coded 503. The non-route mutation callers (attach_ledger_matches→record_use, and _record_verification_misses→record_miss) are each guarded by their callers. The lenient read_entries preserves its old []-on-failure contract for read-only callers. No mutation path can now crash or truncate on a transient read fault.

No findings.

[OPUS-REVIEWED] 05571af

Verdict parsed from the review's SHA-scoped output markers for commit 05571af5e23d891543c0d31b8de7a04458c349e5.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 05571af5e23d891543c0d31b8de7a04458c349e5: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 05571af5e23d891543c0d31b8de7a04458c349e5 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims in the description verify against the repo: the _handle_rotation_arm/secret-route precedent shape exists (routes.py:2187, 2207), the changed field has a real consumer (the SOP branch, routes.py:2483-2486, plus the sibling 409 arm at routes.py:2418), record_miss is wrapped (dispatch.py:538-540) and record_use propagates into the claim path's existing except OSError arm (dispatch.py:717), and the strict-read posture mirrors store._read_index_for_update (store.py:334). One genuine finding survived: counting _write_all call sites gives six — five in ledger.py now on the strict read, and one in ledger_sync.resolve_conflict (ledger_sync.py:682-684) still starting from the lenient read_entries(), which is the exact truncation shape this PR names as its root cause.

First-Principles-Verdict: CONCERNS

The fix is cause-level and every item is declared — but ledger_sync.resolve_conflict is a sixth lenient-read→_write_all rewrite this change leaves able to truncate.

What this change ships

Intent: stop refused ledger reads/writes from escaping as bare 500s or silently truncating the shared ledger — a FIX (issue #7790).

1. POST /ledger refused write answers coded 503, not plain-text 500 — justified
2. DELETE /ledger refused write answers coded 503 — justified
3. Hygiene refused rewrite answers coded 503 and skips index/prune/push — justified
4. Hygiene refusal now audited as a failure — justified
5. Hygiene 503 carries changed: false — justified (consumer: routes.py:2483-2486)
6. Failed mutation read now refuses instead of truncating/answering 404 — justified, cause-level
7. Read-only callers keep the lenient empty-on-failure read — justified
8. Route docstring names the fault-tolerance exception — justified

Watch

• One unfixed sibling of the named root cause. Grepped _write_all( across the backend: 6 call sites; 5 (ledger.py:364, 504, 546, 563, 747) now start from read_entries_for_update, but resolve_conflict (ledger_sync.py:682-684) holds the ledger lock, calls lenient read_entries(), and rewrites via _write_all — a failed open collapses to [] and the rewrite truncates the ledger, exactly the defect described. The new docstring's rule is scoped "in this module", which is how this out-of-module caller escapes it. The subtraction is one call-site switch to read_entries_for_update (its caller sync_safely already tolerates raised errors per its "never raises" wrapper pattern — verify that arm when switching).

[FIRST-PRINCIPLES-REVIEWED] 05571af

[NicholasRBowers]

Tier 1 auto-approve: fix (5 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix with a clear root cause — refused ops-mission-control ledger writes (OSError from the locked read-modify-write) previously escaped as bare 500s and a lenient read could truncate the ledger; now a propagating read-for-update plus coded 503 (ledger_store_unwritable) on POST/DELETE/hygiene routes, error-path only, no auth/gate/trust-boundary change.