fix(ops-mission-control): never publish the token or policy store over a failed read

[chenmingwei23]

1. What is the problem?

Two reads in ops_mission_control collapse every read failure to an empty document, and
both are the base of a whole-file rewrite:

Site	Lenient read	Mutations standing on it
backend/secrets.py	KeystoneFileBackend._read	put, delete
backend/policy_store.py	_read	set_ceiling, put

Each except clause names OSError, so a transient EACCES/EIO -- a scanner holding the
handle on Windows, a torn read, a permissions blip -- is indistinguishable from "the file is
not there yet". The mutation then rewrites the whole document from that empty base. The write
half is flawless; it just writes nothing.

Both lenient reads are correct where they are used as reads. get answering "not configured"
keeps the Settings page rendering and lets the fail-closed has_secrets check refuse the
provider; an unreadable ceiling resolving to observe with no act-rules is the most restrictive
answer available, not a permissive one. The defect is not the leniency -- it is reusing a lookup
answer as a mutation base.

Three distinct failures come out of that, and they are not variations of one thing.

Truncation of the credential store. put rewrites the whole file, so an empty base means
"delete every stored provider token". A provider token is not derivable from anything else on
the box, so this file is the only copy.

A failed revocation reported as completed. This is a separate harm class, not a corollary of
the truncation above. delete destroys nothing: on an unreadable store the lenient read reported
the provider absent, so it returned False and the audit recorded
secret_delete outcome=not_found. The operator's belief and the disk disagree, and the audit
log sides with the operator. They asked for a credential to be revoked and were told there was
nothing there to revoke, while a live, working token sits on disk.

A fail-open on the file that defines who is constrained. For policy_store the argument is a
security one, not a data-loss one. That single file holds every operator-only key -- the autonomy
ceiling (mode, autonomy_rules), the ledger sync remote, the Slack channel, the rotation
identity, the primary-instance flag -- and each is fenced onto the keystone floor precisely
because the agent must not be able to set it. Each also falls back to a value the constrained
party can influence. So one transient read failure reproduced, by accident, the exact bypass
the keystone exists to prevent.

The per-file locks already in place (_SecretLock, _PolicyLock) do not help: they serialize
writers and say nothing about a read that failed.

2. Why this issue matters to the user

None of the three reports itself, and they fail in different directions.

The truncation is loud, eventually. Every stored token vanishing means the operator goes back
to PagerDuty and Datadog to mint new credentials, and every poll fails closed until they do.
Disruptive, but it announces itself the next time something needs the token.

The false revocation is silent, and that makes it worse. A person who believes a credential is
revoked stops treating it as live: they stop rotating it, stop counting it in an access review,
and stop caring where it has been. Meanwhile the token still authenticates. There is no later
moment when this surfaces, because everything keeps working -- that is precisely the problem. Of
the three failures here this is the one to fix first, even though it loses no data.

The fail-open ceiling is an accident that looks exactly like an attack. effective = min(app_mode, rule_mode) is only a ceiling if the party it constrains cannot raise it. Losing
the file drops mode and autonomy_rules back to a re-derived default and the destination and
identity keys to absent -- and absent is how the off-shift refusal and the not_primary gate get
their inputs. The threat model this app documents at length is an agent writing those values; the
bug arrives at a similar place with no agent involved.

3. How our fix solves it

The chain from symptom to root cause: stored state disappears, or a revocation is falsely
reported <- a whole-file write publishes an empty document <- the mutation's base read answered
"empty" <- the base read cannot tell an absent file from an unreadable one, because one except
clause covers both.

The fix cuts the chain at the last link. Each module gains a private _read_for_update used
only as the base of a mutation, in which:

• a missing file still reads as empty -- nothing has been written yet, so empty is the truth,
  and the operator's first save must not become an error;
• a genuine read error propagates, so the mutation is abandoned rather than published over
  state nobody read;
• corruption keeps reading as empty, unchanged. A document that parsed to nothing usable has
  no stored token or ceiling left to lose by being replaced, which matches the lookup reads.

This is the idiom already in the tree, not a new one: mcp_quarantine._load_for_update,
config/loader.read_config_for_update, and most directly #7620 (8064a9bb5), which applied
exactly this shape to aws_control's library.py and shares.py.

secrets.py had multi-line normalization after the parse, so it was extracted into a shared
_coerce rather than duplicated -- which is what makes the two readers provably identical apart
from which failures answer "empty", the one thing that is supposed to differ.

The three routes that write these stores now refuse with a coded error rather than letting
the newly-propagating OSError land as aiohttp's default 500 (a plain-text body with no code
to branch on -- nothing in the middleware chain converts it, sel_audit_middleware logs and
re-raises). This follows _handle_rotation_arm's existing 503 cron_store_unreadable shape in
the same file, whose own comment states the rule: "escaping here becomes a 500".

• _handle_put_secret, _handle_delete_secret -> 503 secret_store_unwritable. The revocation
  route needs it most: the failure being replaced was not a 500 but
  {"ok": true, "removed": false}, and any 2xx there is the bug in section 1.
• _handle_put_settings -> 503 policy_store_unwritable / app_config_unwritable. This covers the
  three writes that reach policy_store.put through an intermediary rather than naming it:
  slack_out.set_settings (slack_out.py:134, :136) and ledger_sync.set_settings
  (ledger_sync.py:327, :329) own operator-only keys, so guarding only the call sites that spell
  policy_store left the Slack channel and the ledger remote -- the two keys an agent must not be
  able to redirect -- answering a plain 500.
• The partial-apply set goes to the audit log, not the refusal body. Phase 2 is a sequence of
  writes, so an earlier one may already have committed and a partial ceiling apply is a security
  state worth recording -- but the dashboard's own req helper
  (website/src/apps/ops-mission-control/api.ts:1077) reads only error from a non-2xx body and
  discards the rest, so a field there would have had no reader. The audit line has one, and mirrors
  the settings_put line the success path already writes.

One gap stated rather than papered over: set_top_level writes the app config, whose read still
collapses a failure to {}, so a transient config.json read failure truncates that file and
returns 200 without the helper ever seeing an OSError. The strict read closing it is in #7794; the
app_config_unwritable code here covers only the write-side failure that store can already raise
today. The helper docstring says so explicitly.

503 rather than 500 throughout because the condition is transient and retrying is the correct
client behaviour.

4. What tests we did

16 new tests. Five per store module plus six on the route layer.

• test_a_read_that_failed_never_truncates_* -- asserts the durable harm directly: the stored
  token, and the operator's other keystone keys, are still there after a failed-read mutation is
  attempted. Asserts on disk state, not on the exception.
• test_an_unreadable_store_refuses_the_save / ..._the_revocation, and the policy equivalents
  for set_ceiling and put -- the caller is told, rather than handed a silent no-op that reads
  as success. The revocation one exists separately because that half loses no data and still lied.
• test_a_missing_*_is_still_a_first_write -- negative control. Absent is the one failure where
  empty is the truth; the guard must not turn a first save into an error.
• test_a_corrupt_*_still_repairs_on_write -- negative control. Pins the existing corruption
  tolerance so the new guard cannot be mistaken for a licence to start failing on corruption.
• TestAStoreThatRefusesToWriteIsReportedNotCrashed -- six route tests: the coded 503 on a
  refused save (asserting the refusal does not echo the credential), the refused revocation
  (asserting removed is absent, so no removal verdict is claimed), the refused ceiling write, the
  refused destination write through slack_out.set_settings (the intermediary path the first
  pass missed), a check that the refusal body carries no field the dashboard discards, and a negative
  control that an ordinary settings write still returns 200.

Each unreadable-file fixture is scoped to the target path (read_text fails only for that one
file). A blanket failure would also break home resolution and the lock sidecars, and the test
would pass for the wrong reason.

Mutation-verified, six probes. Reverting secrets.put/delete or policy_store.set_ceiling
to the lenient reader reds exactly that module's loss guard plus its refusal, while all negative
controls stay green. Removing the revocation route's mapping reds its test with 500 != 503, and so
does removing the Slack destination guard. The secrets probe reproduces the loss literally: the
store goes from {"pagerduty": {"api_token": ...}} to {"datadog": {...}} -- the PagerDuty token
deleted by a read failure.

Gates: isort and flake8 clean on all 6 changed files. mypy reports 4 errors, all in
transcribe.py and providers/cloudwatch.py -- neither in this diff; they reproduce when those
two files are checked alone. App suite 937 passed, 44 skipped, 257 subtests (from 921 on main,
+16). Run with -o addopts="" to avoid the baked-in xdist parallelism.

5. Any other suggestions on the work

1. The same defect in store.py and providers/__init__.py ships as fix(ops-mission-control): never publish the incident index or app config over a failed read #7794, deliberately split
   rather than bundled. Those two are the mechanical half -- an incident index and a config
   file, no authorization decision -- and store.py alone carries six locked read-modify-writes,
   which is where the review surface of this change lives. Splitting on the risk boundary keeps a
   nit on the ops board's index from holding up the credential fix. That PR carries a caller-policy
   change too: making the index read strict meant dispatch.run_cycle's two maintenance passes
   raised out of the heartbeat, which its own ordering doctrine forbids.

2. Two further sites of this pattern remain, tracked in Three remaining lenient-read-feeding-whole-file-rewrite sites, and a ratchet to close the class #7789, one with durable harm
   (aws_control/backend/backup.py:78 -- read_state feeds _locked_state_update's whole-file
   rewrite, so a transient read failure drops the per-account nightly authorization bit and every
   run record). Six sites in two apps have now been fixed by hand in two days, so that issue also
   proposes an AST ratchet keyed on the write rather than the read -- most lenient reads in src/
   are legitimate lookup reads, so the write is the discriminator.

3. ops-mission-control: store-write failures answer a bare untyped 500, not the app's coded error #7790 remains open for the handlers this PR does not touch. The coded-error mapping added
   here covers the secret and settings routes. _handle_put_provider_config, the hygiene handler's
   prune_closed, and the fall-through in _handle_transition / _handle_decide_proposal still
   answer with a bare untyped 500; that is pre-existing and reachable on main today, since every
   one of those paths writes through atomic_write, which raises.

4. A non-UTF-8 file still escapes both lookup reads. read_text(encoding="utf-8") raises
   UnicodeDecodeError, a ValueError and not an OSError, so neither except catches it --
   meaning get_secret on a mojibake'd store raises rather than answering "not configured".
   Unchanged by this PR (it propagates before and after, and propagating is the safe direction for a
   mutation), and fix(aws-control): never publish the share or library ledger over a failed read #7620 left the same edge. config/loader is the module that names
   UnicodeDecodeError explicitly; these two reads arguably should too.

Sibling fix: #7620 (8064a9bb5), same defect class in aws_control.

Pattern harvest

Rule candidate: semgrep, plus a ratchet test for the half a pattern rule cannot see (tracked in #7789)
Pattern: a read that collapses every failure to an empty document, used as the base of a whole-file rewrite

Not a one-off, and the count is the evidence: six sites across two apps in two days -- #7620's
library.py and shares.py, this PR's two, #7794's two -- plus two still open in #7789, one of
them with durable harm. Three rounds of manual discovery for one mechanical shape says the class
needs retiring, not the instance.

Semgrep can match the lenient except (..., OSError, json.JSONDecodeError): return {} cheaply, but
that half alone is not the defect: ten sites in src/ match it and most are legitimate display or
lookup reads, so a rule keyed on the read would be almost entirely false positives. The
discriminator is the coupling -- the same function's result reaching a writer that replaces the
whole document -- which is a dataflow question rather than a syntactic one. That is why #7789
proposes keying the ratchet on the WRITE: for each module, find the lenient reader, then check
whether its return value flows into an atomic_write in the same file.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Strict read-for-update at the mutation base is the established in-tree idiom, correctly leaves lookup reads lenient, and the refusal routes follow an existing coded-503 shape.

Watch

• Description is stale on one point: it says the settings 503 "carr[ies] the keys that did apply" and that a test asserts "applied is present" — the shipped code sends applied to the audit log only, and test_a_refusal_body_carries_no_field_the_dashboard_discards pins its absence from the body. The shipped behavior is the better one; update the PR text so reviewers and future archaeology don't trust the wrong contract.

[DESIGN-REVIEWED] e0d8b82

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of e0d8b82ef0cac21dbacd94873dff44ce46b978b7 — 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 verified. The cited idioms exist (mcp_quarantine, config/loader, aws_control/shares.py), the _handle_rotation_arm precedent exists at routes.py:2079, and the cron store already refuses. The one thing the change leaves behind: store.py's incident index has the identical lenient-read-as-rewrite-base pattern, unfixed and undeclared. Final review:

First-Principles-Verdict: CONCERNS

Every item is aimed at the named cause, but the same root cause has an unfixed, undeclared sibling: the incident index in store.py.

What this change ships

Intent: stop a transient read failure from silently wiping the credential/policy stores or faking a revocation — a FIX.

1. A secret save on an unreadable store no longer deletes every stored token — justified
2. Revoking a credential on an unreadable store errors instead of claiming "already gone" — justified
3. A settings write on an unreadable policy file no longer drops every operator-only key — justified (keystone invariant)
4. Secret save/revoke routes answer coded 503 instead of aiohttp's plain 500 — justified (AGENTS.md code rule)
5. Settings PUT stops at the first refused write, answering coded 503 — justified
6. A refused settings write logs an audit failure line naming the keys already applied — justified
7. _coerce extraction keeping the two secret readers provably identical — justified, part of the fix
8. App-config read (set_top_level) still truncates on failed read — declared, deferred to companion PR

Watch

• Counted unfixed sibling. Grepped except (FileNotFoundError, OSError, json.JSONDecodeError) as an RMW base across this app: store.py:161 _read_index_unlocked feeds 6 locked whole-file rewrites (store.py:274,337,446,523,741,845), so one transient EACCES plus any mutation truncates every incident, approval and proposal. Same cause, same fix shape, not mentioned anywhere in the description — unlike the providers-config sibling, which is declared and deferred. Name it deferred or fix it.
• Description says the settings 503 is "carrying the keys that did apply"; the shipped body is {ok,error,code} (pinned by test_a_refusal_body_carries_no_field_the_dashboard_discards) with applied going to the audit log only. Update the description sentence.

[FIRST-PRINCIPLES-REVIEWED] e0d8b82

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] e0d8b82

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Both candidates fail the falsification bar.

Candidate 1 (ledger_sync mislabel as policy_store_unwritable): The branch-only write path is real — set_settings with only branch_name runs only set_top_level (config.json). But the "observable wrong outcome" collapses: the dashboard's req helper reads only error (the diff's own test_a_refusal_body_carries_no_field_the_dashboard_discards pins this), and error = str(exc), the true OSError message. The audit line logs "refused after {applied}" and never records the code. No consumer reads code and misbehaves, so (c) is not established in practice. Below 80 — drop.

Candidate 2 (blanket except OSError as retryable 503): stated in "could"/"any OSError bug"/edge-condition terms with no concrete input producing a concrete wrong outcome. Speculative robustness concern — drop.

No new Step-2 finding grounds at 80+: the _read_for_update split correctly distinguishes missing (→{}) from unreadable (→raise), the helper propagates non-OSError, and the secrets _coerce refactor is behavior-preserving.

No findings.

[OPUS-REVIEWED] e0d8b82

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

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

[chenmingwei23]

Round 1 dispositions -- all four lanes

Pushed 01cd4dbe4. Every finding is either fixed or answered below; nothing is deferred silently.

GPT 5.6

BLOCKING, test_providers.py:2012, cleanup registered too late -- FIXED. Correct, and I had the
better pattern in front of me: _HomeIsolated in test_policy_store.py carries a comment about this
exact hazard and uses addCleanup for it. I copied the sibling class in test_providers.py instead,
which uses tearDown, and inherited the defect. Now addCleanup(shutil.rmtree, ...) on the line
after mkdtemp, with the home restore moved to addCleanup as well -- registered second so LIFO
restores the variable before the directory is removed.

FINDING, method-local imports -- PARTIALLY FIXED, and I disagree with the rest. The stdlib
imports I added were straightforwardly wrong: contextlib is now at module scope in all four test
files and mock at module scope in test_security.py, with the local copies removed.

The app-module imports (policy_store, store, models, providers) I have left local, because
module scope would make my additions inconsistent with the files they live in. test_policy_store.py
imports policy_store inside the body of every one of its existing tests and never at module scope;
test_store_and_gate.py and test_dispatch.py do the same for store/models while keeping
rotation, routes at the top. Matching the surrounding file is the stronger rule here, and hoisting
one new class's imports while its neighbours keep theirs local would read as an oversight rather than
a convention. Happy to hoist them if the top-level-imports anchor is meant to apply repo-wide -- in
which case the existing tests want the same treatment, and that is its own change.

Design Review (Fable 5) -- CONCERNS

The heartbeat regression -- REAL, and fixed. I verified this rather than taking it on trust, and
it was worse than the summary suggested: expire_stale_proposals is at dispatch.py:584, which is
before resolve_shift and before the poll, so an unreadable index cost the entire cycle including
every claim -- where previously it cost nothing at all. sweep_stale at :724 sits immediately
before the Slack mirror, the notification bus and the SEL entry. And run_cycle's own comments state
the rule being broken at three separate points: the shared-repo pull is "never fatal ... never worth
losing a claim over", the mirror is "after the claim, so a Slack outage can never cost us a claim",
and the notifications come after both "so a bus fault can cost neither".

Both calls now log and continue with an empty result. That is the pre-fix outcome minus the silence,
and it does not weaken the guard: the mutation was abandoned before any write, so nothing was
published. The loss guard lives in the store; this is purely caller policy, which is how you framed
it.

I did not extend this to the claim path, and want to flag the boundary explicitly rather than
leave you to wonder whether I missed it. _claim_one stays unguarded because a compare-and-set has
no safe degraded answer -- None already means "another instance owns this signal" -- and because it
already aborted the loop on an atomic_write failure, so its behaviour is unchanged by this PR.
Three tests pin the new policy (TestAMaintenancePassCannotCostTheCycle), including the realistic
case where one read failure fails both passes at once; removing either guard reds them.

File the section-5 follow-ups -- DONE: #7789 and #7790.

First Principles (Fable 5) -- CONCERNS

The completeness claim was overstated -- CORRECTED, and you were right to grep it. Both sites
check out. aws_control/backend/backup.py:78 is the same defect with durable harm: read_state is
lenient and _locked_state_update is read-mutate-write_state over the whole file, under the same
sidecar-lock shape #7620 already fixed on the share ledger -- so one transient read failure drops the
per-account nightly bit and every run record. ledger_index.py:77 is the low-harm case you
described: _write_cursor(cursor | newly) is a union, so an empty base drops imported ids and the
next import re-checks them, and the docstring already reasons about degrading to "re-check
everything" deliberately.

Filed as #7789, with the durable one written up as the actionable half and the ratchet proposal
attached -- keyed on the write rather than the read, since most of the ten pattern matches in
src/ are legitimate display reads. The PR description now says these are the four sites in this
app that feed a whole-file rewrite, and carries an explicit correction of the earlier wording.

_config_path undeclared -- FIXED in the description. It is a path helper extracted for the same
reason as _coerce: two readers and the writer each spelled the same path expression inline. Now
listed in section 3 alongside the other two helpers.

Opus 4.8

No findings.

---

Full app suite 944 passed, 44 skipped, 257 subtests (+23 versus main). isort and flake8 clean
on all 10 changed files. mypy's 4 errors are in transcribe.py and providers/cloudwatch.py,
neither in this diff -- they reproduce when those two files are checked alone.

[chenmingwei23]

Re-scoped: this PR is now the two authorization-deciding stores only

Force-pushed 00b13d856. The store/config half moved out to #7794, so the round-1 review above
was against a wider diff than what is here now. Mapping, so no finding gets lost in the move:

Still in this PR (secrets.py, policy_store.py, routes.py):

• GPT's method-local-imports finding, for the part I accepted -- contextlib and mock are at
  module scope in test_security.py and test_policy_store.py. My pushback on the app-module
  imports stands for the same reason (every existing test in test_policy_store.py imports
  policy_store inside the body).
• First Principles' _config_path note is answered in section 3, and the completeness claim is
  corrected -- the description no longer says "the four remaining instances".

Moved to #7794:

• GPT's BLOCKING addCleanup finding (test_providers.py) -- fixed, and it travelled with the file.
• Design Review's heartbeat finding (dispatch.run_cycle) -- fixed, and it belongs there because the
  regression only exists once store.py's reader is strict. fix(ops-mission-control): never publish the incident index or app config over a failed read #7794's section 3 carries the full
  argument and the three tests.

New in this revision, and the reason for the re-push beyond the split: the handler question I
had explicitly deferred is now settled in this PR rather than tracked. _handle_put_secret,
_handle_delete_secret and _handle_put_settings map a refused write to the app's coded error
(503 secret_store_unwritable / policy_store_unwritable / app_config_unwritable), following
_handle_rotation_arm's existing shape in the same file. The settings route carries the keys that
did apply in the refusal body, because phase 2 is a sequence of writes and a partial ceiling
apply is a security state.

I had argued against this on the grounds that a bare 500 was already reachable on main via
atomic_write, which is true but is an argument about novelty, not about correctness -- and on the
revocation route specifically it misses the point: the failure being replaced there was never a
500, it was {"ok": true, "removed": false}. Four route tests cover it, including that the refusal
does not echo the credential and does not claim a removal verdict.

Also reframed in the description, because the two findings are not one finding. delete now has
its own place in the problem statement rather than reading as a corollary of put: it destroys
nothing, returns False, and audits secret_delete outcome=not_found -- a failed revocation
reported as completed, where the operator's belief and the disk disagree and the audit log sides
with the operator. And policy_store is stated as a security argument rather than a data-loss one:
every operator-only key in that one file falls back to a value the constrained party can influence,
so this was a fail-open on the file that defines who is constrained.

App suite 935 passed, 44 skipped, 257 subtests (+14 versus main). isort/flake8 clean on all
6 files. Five mutation probes: reverting either reader reds that module's loss guard and its
refusal; removing the revocation route's mapping reds its test with 500 != 503.