fix(update): fail closed when approval nonce persists

[leonlaiyc]

Problem / Motivation

The host-local update approval nonce is documented as single-use, but consume() delegated deletion to a best-effort cleanup helper. If unlinking the nonce file failed, the helper swallowed the error and consume() still returned the approved request, leaving the same nonce on disk and replayable.

Why it matters

The nonce is the authority boundary between dashboard arming and host-local approval. Accepting an approval without durably consuming that nonce breaks the at-most-once guarantee precisely when the filesystem reports that it could not enforce it.

What changed (motivation → approach → change)

Approval consumption now uses a strict removal path that raises StepUpError on any unlink failure. General cleanup remains best-effort, so expired-state cleanup keeps its existing behavior while the security-sensitive transition fails closed. The update architecture RFC now states that removal must succeed before approval is accepted.

Review hardening: consume() runs its read → validate → remove as one critical section under a new module mutex (_PENDING_MUTEX), and arm()'s atomic swap, the expiry cleanup, and clear_pending() take the same lock. This closes a window the GPT review lane flagged: an approve that validated request A could unlink a fresh request B a concurrent arm swapped in — accepting the stale approval while silently destroying the new one. Reading never writes by default: read_pending() removes an expired file only on the explicit clear_expired=True opt-in, which only gateway callers use (under the mutex, serialized against arm). The one out-of-process reader (kirocrew update approve) uses the no-write default, so the lock covers every writer that exists. A stale file that lingers grants nothing (every reader checks expiry) and the next arm replaces it.

Tests

• Added a deterministic test that forces the nonce unlink to raise PermissionError, verifies approval is rejected, and verifies the request remains pending.
• Red-before: the new test failed with DID NOT RAISE StepUpError on current main.
• Added a two-thread regression test for the serialization: a consume paused inside its read-validate-remove window while a second thread re-arms. Red-before: without the mutex it fails with the re-armed request destroyed (verified by mutation probe); with the mutex the re-armed nonce survives.
• Added a cross-process-reader test: a default read_pending() on an expired file reports absent and leaves the file untouched; cleanup happens only via the explicit clear_expired=True gateway opt-in.
• test/test_update_stepup.py: 28 passed.
• isort, flake8, and mypy (src/kiro_crew/) pass on the final tree.

Manual verification

N/A — the unit test directly controls the filesystem failure at the consumption boundary and the existing endpoint tests cover StepUpError response handling.

Related Issues

no linked issue: found through direct source inspection; no matching open issue or pull request.

Pattern harvest

Rule candidate: review-prompt
Pattern: single-use credentials must fail closed unless durable consumption succeeds.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: This PR has been inactive for 7+ days with its readiness gate stuck. I've assessed the blockers and they appear resolvable — I'll push fixes directly to this branch as a co-author.

Assessment: All CI checks are green on head 33afaf4 and the PR is mergeable, but all five Stage-2 fork review lanes (GPT/Opus/Design/First Principles/UX) skipped off the original CI completion and never restamp on reruns, leaving PR Readiness pending on 7 checks. Fix plan: rebase-only push onto current main to mint a new head, which re-fires CI and the review lanes, then drive any resulting findings to green.

If you'd prefer I don't touch this PR, add the pr-no-autofix label.

[bolichen97]

Rebased onto main 9c2b4b67 by a maintainer as part of the 2026-09-08 open-PR audit. Old head 33afaf4d9, new head 2332c89b0.

Clean rebase: no conflicts in any of the three files. The diff is byte-identical to the original (35 insertions, 2 deletions), including the RFC §1 sentence, which main's #7532 rewrite of §5 did not touch.

Gates run locally on the rebased head, scoped to the changed files: black --check clean, isort --check-only clean, flake8 clean, and pytest test/test_update_stepup.py = 26 passed.

@leonlaiyc please review the rebase. Note that a maintainer push makes the maintainer the last pusher, so under this repo's last-push rule a second approver is needed before merge. Reply here if anything looks wrong.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: Drive-to-green status on head 2332c89b0.

What changed by this drive:

• PR body hygiene only: removed the leftover ## Contribution License Agreement placeholder section, and rewrote the Related Issues line to the no linked issue: convention so the intentional no-close reads as deliberate.

What did not change:

• No code push from this drive. A maintainer (@bolichen97) rebased the branch onto main 9c2b4b67 at 15:25 UTC — patch-id verified identical to the rebase this drive had prepared locally (cc04dc32), so that head was adopted rather than overwritten. Local gates on the adopted head: isort / flake8 / mypy clean, test/test_update_stepup.py 26 passed.

The maintainer push minted the new head, which re-dispatched CI and all five Stage-2 review lanes (the sole triage blocker — lanes stuck skipping off the Sep-1 workflow_run). Now polling to green.

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of 4faa01c1f6f993dd6c1098796c607cf76a2fcb9f via the fork AI-review pipeline — 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.

I've verified the key claims against the base tree: on base, consume() delegates removal to clear_pending(), which swallows OSError (update_stepup.py:194-197), so the fail-closed test would indeed fail on base. Both arm and consume run as asyncio.to_thread executor threads in one gateway process (updates.py:2313, updates.py:2429), so the TOCTOU window the mutex closes is real and readable from base. Grepping src/ for writers of the nonce file: all three (arm, expired-clear, consume) are gateway-side; the only out-of-process consumer is the read-only CLI at cli_server.py:1691. The RFC update in the same commit satisfies the docs invariant.

First-Principles-Verdict: PASS

Only unreadable claim: the mutation-probe "red-before" on the serialization test — confirm the test actually fails with _PENDING_MUTEX removed.

What this change ships

Inventory (5 items) — 5 justified

Intent: make consuming the host-local update-approval nonce genuinely at-most-once — fail closed when the file can't be removed, and atomic against a concurrent re-arm. This is a FIX (the new fail-closed test demonstrably fails on base, where clear_pending() swallows OSError at update_stepup.py:194-197).

1. Approving an update now errors when the nonce file cannot be deleted, instead of proceeding with the nonce still replayable — justified
2. An in-flight approval can no longer silently destroy a request a concurrent re-arm just wrote (arm/approve/cleanup serialized in the gateway) — justified
3. Reading the armed request no longer deletes an expired file by default; cleanup is a gateway-only clear_expired=True opt-in (2 counted True-callers: updates.py arm-status, consume; the one out-of-process reader at cli_server.py:1691 keeps the no-write default) — justified
4. The update RFC now states removal-must-succeed and the serialization — justified
5. Three regression tests pin fail-closed, the serialization, and the no-write default read — justified

Writer count grepped (read_pending|clear_pending|update_stepup\.(arm|consume) over src/): every writer is gateway-side, so the in-process lock covers all of them — the module comment's premise holds.

[FIRST-PRINCIPLES-REVIEWED] 4faa01c

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 4faa01c1f6f993dd6c1098796c607cf76a2fcb9f via the fork AI-review pipeline; updated in place on each push.

Review details

The candidate list contains no candidates, and I've independently verified the change. This is a concurrency fix: it serializes the nonce read-validate-remove under a reentrant module mutex, makes expiry cleanup an explicit opt-in so the out-of-process CLI reader can never unlink a fresh request, and makes the consume unlink fail-closed.

I confirmed:

• The RLock is correctly reentrant across the nested consume → read_pending → clear_pending calls; a plain Lock would self-deadlock.
• Every writer to the final path (arm's os.replace, clear_pending, _consume_pending_file via consume) is now serialized; arm's unique request-id-keyed temp write outside the lock cannot collide.
• The only cross-process reader (CLI, default clear_expired=False) never writes, so an in-process lock genuinely covers every writer.
• Serialized consume calls cannot double-trigger: a second concurrent approve sees the file already gone via read_pending returning None before reaching the fail-closed unlink, so unlink()-without-missing_ok never spuriously raises.
• The keyword-only signature change breaks no caller (cli_server uses the default, the status handler is updated in-diff).

No behavioral defect, security hole, or blocking-rule violation is groundable on the changed lines.

No findings.

[OPUS-REVIEWED] 4faa01c

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 4faa01c1f6f993dd6c1098796c607cf76a2fcb9f via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The consume path, both callers, and the approve endpoint's StepUpError handling all check out against the base tree: the only writers to the nonce file live in the gateway process, the one out-of-process reader (the CLI) gets the no-write default, and the RFC is updated in the same change. The fail-closed unlink sits in the module that owns the nonce, with a documented failure story (approval rejected, request stays pending, TTL still bounds it).

Design-Verdict: PASS

Fail-closed consumption and an in-process mutex land at the exact layer that owns the nonce, matching the real writer topology — sound and proportionate.

[DESIGN-REVIEWED] 4faa01c

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 4faa01c1f6f993dd6c1098796c607cf76a2fcb9f via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 4faa01c

[NicholasRBowers]

• fixed span=f22124132eca — Current-path unlink can consume a replacement nonce
  self-added: no
  mechanism: module RLock (_PENDING_MUTEX) serializing every nonce-file read-validate-remove against arm's atomic swap

Fixed in e24762d. consume() now runs read → constant-time validate → unlink as ONE critical section under a module mutex, and every sibling writer takes the same lock: arm()'s os.replace swap, read_pending()'s expiry clear, and clear_pending(). The request consume removes is therefore always the request it validated — a concurrent arm's fresh nonce B can no longer be unlinked by an approve that validated A, on any interleave.
The fix covers the whole branch table of nonce-file writers, not just the flagged site: (1) arm swap — serialized; (2) consume read-validate-remove — one section; (3) read_pending expiry clear — same mutex, so an expired-A read cannot clear a fresh B; (4) clear_pending — same mutex. All writers live in update_stepup.py and run as executor threads in one process, so a process-local lock closes the class.
Regression test test_in_flight_consume_cannot_unlink_a_concurrent_rearm drives the exact two-thread interleave (consume paused inside its window, concurrent re-arm) and FAILS on the pre-fix code with the re-armed request destroyed (verified by mutation probe), passes with the mutex.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: Pushed e24762def fixing the GPT security finding on 2332c89b0 (span f22124132eca).

What changed:

• src/kiro_crew/platform/update_stepup.py: added a module threading.RLock (_PENDING_MUTEX). consume() now performs its read → validate → remove as one critical section; arm()'s os.replace swap, read_pending()'s expiry clear, and clear_pending() all take the same mutex. This closes the TOCTOU the reviewer flagged (an approve that validated request A could unlink a fresh request B a concurrent arm swapped in) and the sibling window in the expiry path.
• test/test_update_stepup.py: new regression test test_in_flight_consume_cannot_unlink_a_concurrent_rearm drives the exact two-thread interleave; verified red on the pre-fix code (re-armed nonce destroyed) and green with the lock.
• docs/request-for-change/rfc-update-architecture.md: the touched paragraph now also records the serialization guarantee.

Local gates: isort / flake8 / mypy clean, test/test_update_stepup.py 27 passed. Leon's authorship preserved; Kiro Crew co-author trailer added. Old head's in-flight runs were cancelled before the push.

[NicholasRBowers]

• fixed span=f22124132eca — Cross-process expiry cleanup can delete a fresh approval
  self-added: yes
  mechanism: read_pending(clear_expired=...) keyword — cross-process callers opt out of the expiry unlink

Fixed in 7739e27 with the finding's own first remedy: expiry cleanup is removed from the cross-process path. read_pending() takes clear_expired (default True); the CLI's kirocrew update approve passes clear_expired=False, so the out-of-process reader never writes at all — it can no longer unlink a fresh request a gateway re-arm swapped in. Cleanup of an expired file happens only inside the gateway, under _PENDING_MUTEX, where it is serialized against arm. A stale file that lingers when only the CLI observes it grants nothing (every reader checks expiry; consume fails closed) and the next arm replaces it via os.replace.
This span is at x2 (round-1: in-process consume/arm race, fixed with the mutex; this round: the cross-process sibling of the same class). With this change the lock covers every writer that exists and no writer runs outside it, so the class — an unserialized nonce-file write racing arm — has no remaining instance in any process.
Regression test test_cross_process_reader_never_deletes_the_file locks the contract: an expired read with clear_expired=False reports absent and leaves the file untouched (red on pre-fix code).

[NicholasRBowers]

• fixed span=5317da8b3256 — drop the inner with _PENDING_MUTEX: in _consume_pending_file
  self-added: yes

Subtraction taken in 7739e27: the inner re-acquisition is removed. _consume_pending_file()'s only production caller is consume(), which already holds the re-entrant lock around its whole read-validate-remove section; the docstring now states that contract so a future second caller knows the locking is the caller's job.

[NicholasRBowers]

• rebutted span=54677a16f5ab — rides along: serialization is a second fix in the same PR
  self-added: yes

Keeping both fixes in one PR is deliberate and declared. The serialization was demanded by this PR's own review: the GPT lane blocked the fail-closed unlink without it (security-class, adjudication-upheld), so shipping the title's fix alone would ship a PR its own review had condemned, and splitting it out would leave main carrying the adjudicated race between the two merges. The item's own observations — base-verifiable harm, red-before tests, zero new public surface, a "What changed" paragraph naming the mechanism — are the declaration working as intended. A retrospective-equivalent ruling on this class was recorded on the prior head; this ruling covers findings that ask the serialization to ship separately from the fail-closed unlink, wherever they land.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: Pushed 7739e272e (round 3) fixing the GPT security blocker on 82cd360d3.

What changed:

• src/kiro_crew/platform/update_stepup.py: read_pending() gains clear_expired (default True). The expiry unlink now runs only when the caller opts in — which every in-gateway caller does, under _PENDING_MUTEX. Also took the First Principles subtraction: dropped the redundant inner lock in _consume_pending_file() (its only caller, consume(), already holds the re-entrant lock; documented in the docstring).
• src/kiro_crew/cli_server.py: kirocrew update approve calls read_pending(clear_expired=False) — the cross-process reader never writes, so it can no longer delete a fresh request a gateway re-arm swapped in (the flagged residual is closed, not merely documented).
• test/test_update_stepup.py: new test_cross_process_reader_never_deletes_the_file (red on pre-fix code); suite now 28 passed.
• PR body rewritten to describe the final design.

Housekeeping: deleted one of my earlier disposition comments whose span (36a44d4b5692) no longer resolves after the #9318 re-stamp rebuilt the review comment — it was blocking the disposition gate. Gates green (isort/flake8/mypy, 28 passed). Leon's authorship preserved.

[NicholasRBowers]

• fixed span=00859f067f65 — Worker thread can outlive fixture isolation
  self-added: yes

Fixed in 4f96f5f with the finding's own remedy plus one hardening beyond it. consume() now runs inside try/finally: the finally sets in_window and joins the worker, so the thread is released and reaped INSIDE the fixture's scope on every path, including a consume() that raises before the window opens. The worker also checks wait()'s return value and arms only when the window genuinely opened — a timed-out wait arms nothing, so no code path can write into the operator's real data home after teardown reverts KIROCREW_HOME. This ruling covers findings that require the test's worker thread to be signaled and joined on exception paths.

[NicholasRBowers]

• fixed span=c4615150a7bb — invert the read_pending default to clear_expired=False
  self-added: yes
  mechanism: no-write-by-default read contract — expiry cleanup is an explicit gateway opt-in

Suggestion taken in 4f96f5f. read_pending() defaults to clear_expired=False: reading never writes unless a caller explicitly opts in. The two gateway callers (consume(), the /api/update/arm status handler) pass clear_expired=True under the mutex; the CLI uses the bare default and cannot write. A future out-of-process caller now inherits the safe behavior silently instead of the dangerous one.

[NicholasRBowers]

• rebutted span=6a4fb2306a04 — rides along: the serialization is a second defect fixed in the same PR
  self-added: yes

The item's own text concedes "it needs no change, only this tag" — the tag is hereby recorded. Keeping both fixes in one PR is deliberate: this PR's own GPT lane blocked the fail-closed unlink without the serialization (security-class, adjudication-upheld on head 2332c89), so splitting it out would leave main carrying an adjudicated race between two merges. Declared in the body's "What changed"; red-before tests cover both defects. This is the third same-class rides-along item (spans a8259b16d0b7, 54677a16f5ab, 6a4fb2306a04 — a fresh span id each re-review); this ruling covers the class: findings that ask the serialization to ship separately from the fail-closed unlink, wherever they land.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: Pushed 4f96f5f5f (round 4) answering the three findings on 7739e272e.

What changed:

• test/test_update_stepup.py: the two-thread regression test now releases and joins its worker in a finally block, and the worker arms only when wait() genuinely opened the window — no exception path can leave a thread alive past fixture teardown to write into the real data home (the GPT security blocker).
• src/kiro_crew/platform/update_stepup.py: took the Design suggestion — read_pending() now defaults to clear_expired=False (reading never writes); the gateway's two callers (consume(), the arm-status handler in src/kiro_crew/dashboard/handlers/updates.py) explicitly opt in under the mutex, and the CLI's cross-process read uses the safe default.
• Body updated to describe the final no-write-by-default contract.

Gates green (isort/flake8/mypy, 28 passed). Dispositions posted: GPT 00859f067f65 fixed, Design c4615150a7bb fixed, First Principles 6a4fb2306a04 rebutted (third same-class rides-along tag; class-level ruling recorded).

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: Pushed 4faa01c1f (formatting only). CI's black gate flagged src/kiro_crew/dashboard/handlers/updates.py — the arm-status handler edit wasn't black-formatted. Ran black --target-version py310 on that file (a 3-line lambda collapse, no behavior change). The gate's other candidate, cli_server.py, is pre-existing baseline code untouched by this PR's hunk and was left alone. All local gates green (black/isort/flake8/mypy, 28 tests passed). No review findings this round — GPT and Opus both settled clean on 4f96f5f5f.

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: Review-ready at head 4faa01c1f: PR Readiness passed, 68/68 checks green, mergeable, all five AI review lanes clean, zero unresolved threads, every concern dispositioned. Five fix rounds total: (1) _PENDING_MUTEX serialization closing the adjudicated consume/re-arm race (red-proven two-thread test); (2) declared the mechanism + corrected the overclaiming comment; (3) read_pending(clear_expired=...) so the cross-process CLI reader never writes (red-proven no-write test) + dropped a redundant inner lock; (4) finally-joined the test worker + inverted the default to no-write-by-default; (5) black formatting. Remaining gate: human maintainer review (last-push rule needs approvers other than the pushers). Auto-merge NOT armed.

[bolichen97]

Maintainer review (agent-assisted, on behalf of @bolichen97), head 4faa01c1f.

Verified: consume() in src/kiro_crew/platform/update_stepup.py:180-190 now raises StepUpError on unlink failure instead of the old best-effort clear_pending(), and dashboard/handlers/updates.py:2430-2432 turns that into a 403 approve_refused with nothing swallowed. Read-validate-remove runs as one critical section under _PENDING_MUTEX, so the stale-nonce, unreadable-file and check/consume race paths all fail closed. Negative tests cover each path. No findings.