fix(apps): refuse a corrupt document in the four update readers instead of replacing it (#7805) - #8084
Conversation
github-actions
Bot
added
the
readiness: checking
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Refuse-corrupt-instead-of-rewrite is the correct root-cause fix, applied uniformly with the display/mutation asymmetry, caller audit, and fail-closed authority read all deliberate. [DESIGN-REVIEWED] 9b25015 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All mechanical checks are done. Composing the review. First-Principles-Verdict: CONCERNS Everything earns its place except What this change shipsIntent: stop four read-modify-write stores from silently destroying a corrupt-but-recoverable file — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 9b25015 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've verified the candidate's mechanics and the surrounding contracts directly. Candidate 1 (secrets.py:594, null-valued provider slips the strict check). The behavior claim is correct: Step 2 review of the diff surfaced no new grounded defect. The refusal wiring is consistent: No findings. [OPUS-REVIEWED] 9b25015 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
github-actions
Bot
added
readiness: action required
15cd57f to
8d47e7f
Compare
github-actions
Bot
added
readiness: checking
github-actions
Bot
added
readiness: action required
8d47e7f to
26ca390
Compare
github-actions
Bot
added
readiness: checking
|
Disposition: FIXED — span=9a3cae9e78a7
|
github-actions
Bot
added
readiness: action required
## Problem
`PinnedFilesService._reload_pins_from_disk` caught `(FileNotFoundError,
json.JSONDecodeError)` and returned with `_pins` unchanged, and all four of its
callers then `_persist()` a whole-file rewrite. A corrupt `pinned-files.json`
was therefore silently replaced by whatever the in-memory list held, discarding
the rows another process wrote that this one never loaded -- the unparseable
file was their only remaining copy.
The MCP server is the second writer of the same file and had the worse half of
the same bug: `_tool_pin_file` / `_tool_unpin_file` read through `_read_json`
with a `{"version": 1, "pins": []}` default, so a corrupt store was ZEROED
rather than merely reverted to one process's view. Fixing only the service
would have left that writer free to destroy what the service just refused to
touch, so both go through one reader.
Same lenient-read-feeding-whole-file-rewrite class as #7620, #7788, #7794 and
#7805.
## What changed
`read_pins_for_update(file_path)` is the shared update-path reader. It returns
the stored list, `None` when the file is absent (the one case where an empty
base is true, so a first pin on a fresh install still lands), and raises the
new public `PinsCorruptError` on the four shapes that all reach the same
whole-file rewrite: a parse failure, a read error the filesystem raised
(transient EACCES/EIO -- the store is still there), valid JSON whose root is
not an object or whose `pins` is not a list (no parse failure at all), and an
undecodable byte.
That last one decodes STRICTLY where `load` and the Node original use
`errors="replace"`. The leniency only surfaces as a parse failure when the bad
byte falls OUTSIDE a JSON string; inside a quoted value -- a pin label, or a
path on a filesystem that does not enforce UTF-8 -- it becomes U+FFFD,
`json.loads` SUCCEEDS, and the whole-file rewrite persists the mangled text.
Proven directly: a store holding `"label": "caf\xe9 notes"` came back from a
mutation with byte `0xe9` gone and `\ufffd` in its place. Raised by GPT 5.6
review (span=617a8d5961a6).
`load` keeps the lenient decode: it does not write, and a file it accepts is
preserved as a `.bak.<now_ms>` sidecar first. A mangled label can still reach
memory at startup; it can no longer reach disk, because every write path
re-reads through this reader.
The refusal is a named public type rather than the bare `json.JSONDecodeError`
the aws-control readers raise in #8084: that app reached for the plain type
only because the named one lived in another app, which does not apply to a type
declared in the module both writers already import. Modelled on
ops-mission-control's `CorruptDocumentError`.
`add_pin` / `remove_pin` / `mark_seen` propagate it, and the HTTP handlers map
it to `500 {"code": "pins_corrupt"}`. Returning `{"ok": false}` would read as
"no such pin", and for mark-seen -- which has no failure return -- the old
behaviour reported success while the store was being replaced. The exception
text is not echoed: pin labels and paths are agent-authored and redacted on the
way out of the GET handler.
`_process_watch_event` is the one path that swallows the refusal. It fires from
the owner's tick loop, where an escaping error would take down every other tick
(stats, watchlist, presence) over a file it merely wanted to stamp; dropping
the stamp is the same degradation a failed `_persist` already takes there. The
paths a user drove raise, so the operator still learns the store needs repair.
Refuse-only was chosen over extending the sidecar to the update path (#7789's
shape) because this file already has the sidecar where a replacement is
intended, and the update path's whole problem is that no replacement was
intended at all.
## Tests
`test/test_mochi_pinned_files_cov80.py`, 15 new tests. The mutation cases
assert on the FILE's bytes rather than the exception type, so they fail on the
buggy code for the reason that regressed; `TestRefusalIsTheNamedType` pins the
type separately.
Red-before, against pristine `origin/main` source -- 13 failed / 27 passed,
each on a behavioural assertion:
- `add_pin` rewrote the corrupt file from its in-memory list
- `remove_pin` rewrote it, landing `"pins": []`
- `mark_seen` rewrote it
- a debounced watch event rewrote it from the tick loop
- `_tool_pin_file` returned `{"ok": true, "pins": 1}` and zeroed it
- `_tool_unpin_file` zeroed it
The two UTF-8 tests were separately proven against the pre-strict-decode
revision of this branch -- 2 failed / 1 passed, the mutation case on the raw
bytes.
Gates: 612 tests green (all 17 `test_mochi_*` files plus the black-gate
contract), black, isort, flake8, mypy (23 files), sync-io-in-async gate.
## Pattern harvest
Rule candidate: semgrep
Pattern: `read_text(errors="replace")` -- or any lossy decode -- on a read whose
value is written back to the same file.
A lossy decode is a repair, and a repair is only safe on a path that does not
persist what it read. On a read-modify-write path the substituted character is
written over the original bytes, so the loss is silent and irreversible -- and,
uniquely among the corruption shapes, it produces VALID JSON, so no
parse-failure clause downstream can catch it. The same file legitimately keeps
the lenient decode on its display/startup path, which is why the rule has to
key on the read reaching a writer rather than on the decode call alone.
This is the fifth instance of the enclosing class (#7620, #7788, #7794, #7805,
this PR), and the decode variant is the one the earlier four did not cover --
they raised on `UnicodeDecodeError` because they decoded strictly to begin with.
Closes #8088
d9e5873 to
3263496
Compare
github-actions
Bot
added
readiness: checking
#8092) ## Problem `PinnedFilesService._reload_pins_from_disk` caught `(FileNotFoundError, json.JSONDecodeError)` and returned with `_pins` unchanged, and all four of its callers then `_persist()` a whole-file rewrite. A corrupt `pinned-files.json` was therefore silently replaced by whatever the in-memory list held, discarding the rows another process wrote that this one never loaded -- the unparseable file was their only remaining copy. The MCP server is the second writer of the same file and had the worse half of the same bug: `_tool_pin_file` / `_tool_unpin_file` read through `_read_json` with a `{"version": 1, "pins": []}` default, so a corrupt store was ZEROED rather than merely reverted to one process's view. Fixing only the service would have left that writer free to destroy what the service just refused to touch, so both go through one reader. Same lenient-read-feeding-whole-file-rewrite class as #7620, #7788, #7794 and #7805. ## What changed `read_pins_for_update(file_path)` is the shared update-path reader. It returns the stored list, `None` when the file is absent (the one case where an empty base is true, so a first pin on a fresh install still lands), and raises the new public `PinsCorruptError` on the four shapes that all reach the same whole-file rewrite: a parse failure, a read error the filesystem raised (transient EACCES/EIO -- the store is still there), valid JSON whose root is not an object or whose `pins` is not a list (no parse failure at all), and an undecodable byte. That last one decodes STRICTLY where `load` and the Node original use `errors="replace"`. The leniency only surfaces as a parse failure when the bad byte falls OUTSIDE a JSON string; inside a quoted value -- a pin label, or a path on a filesystem that does not enforce UTF-8 -- it becomes U+FFFD, `json.loads` SUCCEEDS, and the whole-file rewrite persists the mangled text. Proven directly: a store holding `"label": "caf\xe9 notes"` came back from a mutation with byte `0xe9` gone and `\ufffd` in its place. Raised by GPT 5.6 review (span=617a8d5961a6). `load` keeps the lenient decode: it does not write, and a file it accepts is preserved as a `.bak.<now_ms>` sidecar first. A mangled label can still reach memory at startup; it can no longer reach disk, because every write path re-reads through this reader. The refusal is a named public type rather than the bare `json.JSONDecodeError` the aws-control readers raise in #8084: that app reached for the plain type only because the named one lived in another app, which does not apply to a type declared in the module both writers already import. Modelled on ops-mission-control's `CorruptDocumentError`. `add_pin` / `remove_pin` / `mark_seen` propagate it, and the HTTP handlers map it to `500 {"code": "pins_corrupt"}`. Returning `{"ok": false}` would read as "no such pin", and for mark-seen -- which has no failure return -- the old behaviour reported success while the store was being replaced. The exception text is not echoed: pin labels and paths are agent-authored and redacted on the way out of the GET handler. `_process_watch_event` is the one path that swallows the refusal. It fires from the owner's tick loop, where an escaping error would take down every other tick (stats, watchlist, presence) over a file it merely wanted to stamp; dropping the stamp is the same degradation a failed `_persist` already takes there. The paths a user drove raise, so the operator still learns the store needs repair. Refuse-only was chosen over extending the sidecar to the update path (#7789's shape) because this file already has the sidecar where a replacement is intended, and the update path's whole problem is that no replacement was intended at all. ## Tests `test/test_mochi_pinned_files_cov80.py`, 15 new tests. The mutation cases assert on the FILE's bytes rather than the exception type, so they fail on the buggy code for the reason that regressed; `TestRefusalIsTheNamedType` pins the type separately. Red-before, against pristine `origin/main` source -- 13 failed / 27 passed, each on a behavioural assertion: - `add_pin` rewrote the corrupt file from its in-memory list - `remove_pin` rewrote it, landing `"pins": []` - `mark_seen` rewrote it - a debounced watch event rewrote it from the tick loop - `_tool_pin_file` returned `{"ok": true, "pins": 1}` and zeroed it - `_tool_unpin_file` zeroed it The two UTF-8 tests were separately proven against the pre-strict-decode revision of this branch -- 2 failed / 1 passed, the mutation case on the raw bytes. Gates: 612 tests green (all 17 `test_mochi_*` files plus the black-gate contract), black, isort, flake8, mypy (23 files), sync-io-in-async gate. ## Pattern harvest Rule candidate: semgrep Pattern: `read_text(errors="replace")` -- or any lossy decode -- on a read whose value is written back to the same file. A lossy decode is a repair, and a repair is only safe on a path that does not persist what it read. On a read-modify-write path the substituted character is written over the original bytes, so the loss is silent and irreversible -- and, uniquely among the corruption shapes, it produces VALID JSON, so no parse-failure clause downstream can catch it. The same file legitimately keeps the lenient decode on its display/startup path, which is why the rule has to key on the read reaching a writer rather than on the decode call alone. This is the fifth instance of the enclosing class (#7620, #7788, #7794, #7805, this PR), and the decode variant is the one the earlier four did not cover -- they raised on `UnicodeDecodeError` because they decoded strictly to begin with. Closes #8088 Co-authored-by: Joe Guo <zejiangg@amazon.com>
github-actions
Bot
added
the
merge conflict
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
…ad of replacing it The four merged *_for_update readers -- shares._load_for_update, library._read_ledger_for_update, KeystoneFileBackend._read_for_update and policy_store._read_for_update -- caught json.JSONDecodeError alongside FileNotFoundError and returned an empty container, so a corrupt or truncated file was silently treated as empty and then rewritten whole, destroying bytes a person could still have recovered by hand. secrets.py is the copy that matters most: it is the only copy of every provider token on the box. Each reader now lets corruption propagate, mirroring the #7794 decision for the incident index and app config: only a MISSING file reads as empty. The non-UTF-8 byte stream (UnicodeDecodeError, a ValueError that is NOT a JSONDecodeError) and the valid-JSON-wrong-root shape are folded into the same refusal, and the secrets coercion refuses anything that would not survive a read-write cycle. The ops-mission-control pair raises the named CorruptDocumentError; the aws_control pair raises plain json.JSONDecodeError because the named type lives in another app and these readers pass rows through verbatim. Every caller of the four mutation paths was audited for the JSONDecodeError-under-ValueError trap the issue names: the library push and remove routes gained a corruption arm AHEAD of their tolerant ValueError arms, the share mint and forget routes answer a coded 500 instead of a bare one (a minted-but-unrecorded URL is withheld), the reconcile caller degrades to not-reconciled and keeps rendering, and the secret save/revocation routes answer the shared _store_read_refusal mapper's coded 500. Display and lookup reads stay lenient -- that asymmetry is the point -- and now log when they degrade for any reason other than an absent file. Closes #7805 Co-authored-by: Kiro Crew <noreply@kirodotdev.github.io>
3263496 to
9b25015
Compare
github-actions
Bot
added
readiness: checking
|
[operator: bolichen97] Rebased onto current Sole conflict was in All other 15 files auto-merged clean. Local gates on the rebased head: black-baseline, isort, flake8 7.1.0, mypy 1.14.1 (1279 files) green; targeted tests |
github-actions
Bot
added
readiness: passed
The |
It is a real behavior change, and it is declared — it is not a rider but the core of the widened-door fix this PR was driven to close. The PR description names it explicitly: "The one exception, |
chenmingwei23
left a comment
chenmingwei23
left a comment
There was a problem hiding this comment.
Approving: PR Readiness green (the repo's only required check), no failing lanes, MERGEABLE.
github-actions
Bot
removed
the
readiness: passed
… read All three mutations rewrite the whole ledger from what they read, and they stood on the lenient display reader: a transient EACCES/EIO or a truncated document read as empty and was then published back -- record_install over every other app's refcount, and classify_and_clean_for_uninstall as an outright wipe of the only record that an installed dependency is still referenced. The sidecar lock serializes writers and says nothing about a read that failed. _read_ledger_for_update is the mutation base: only a MISSING file reads as empty; an unreadable or corrupt document (and a root that parses but is not an object) propagates and the mutation is abandoned. UnicodeDecodeError is folded into the refusal: it is a ValueError but not a json.JSONDecodeError, so unwrapped it slips past corruption clauses. The display reader stays lenient -- and gains the object-root check it lacked, so a non-object ledger degrades to empty with a loud log instead of crashing every lookup with AttributeError. Because the strict read fires after the uninstall's irreversible steps, the uninstall handler pre-flights it before anything destructive and returns a handled dependency_ledger_unreadable refusal instead of stranding a half-uninstalled app whose retry reruns teardown -- the same shape as its trust-grant precondition, offloaded to the executor. A keep_dependencies purge never touches the ledger and skips the pre-flight. Mirrors the merged kirodotdev#8084 readers (kirodotdev#7805 class).
… read All three mutations rewrite the whole ledger from what they read, and they stood on the lenient display reader: a transient EACCES/EIO or a truncated document read as empty and was then published back -- record_install over every other app's refcount, and classify_and_clean_for_uninstall as an outright wipe of the only record that an installed dependency is still referenced. The sidecar lock serializes writers and says nothing about a read that failed. _read_ledger_for_update is the mutation base: only a MISSING file reads as empty; an unreadable or corrupt document (and a root that parses but is not an object) propagates and the mutation is abandoned. UnicodeDecodeError is folded into the refusal: it is a ValueError but not a json.JSONDecodeError, so unwrapped it slips past corruption clauses. The display reader stays lenient -- and gains the object-root check it lacked, so a non-object ledger degrades to empty with a loud log instead of crashing every lookup with AttributeError. Each flow surfaces the refusal in its own contract instead of tearing through it. The uninstall handler pre-flights the read before anything destructive and returns a handled dependency_ledger_unreadable refusal -- the same shape as its trust-grant precondition, offloaded to the executor; a keep_dependencies purge never touches the ledger and skips it. The install resolver reports a refused record as a failed dependency (the install did happen; a retry after the ledger is repaired re-records it), and the dep-cleanup loop logs and continues so one unreadable read cannot strand the rest of the uninstall. Mirrors the merged kirodotdev#8084 readers (kirodotdev#7805 class).
… read All three mutations rewrite the whole ledger from what they read, and they stood on the lenient display reader: a transient EACCES/EIO or a truncated document read as empty and was then published back -- record_install over every other app's refcount, and classify_and_clean_for_uninstall as an outright wipe of the only record that an installed dependency is still referenced. The sidecar lock serializes writers and says nothing about a read that failed. _read_ledger_for_update is the mutation base: only a MISSING file reads as empty; an unreadable or corrupt document (and a root that parses but is not an object) propagates and the mutation is abandoned. UnicodeDecodeError is folded into the refusal: it is a ValueError but not a json.JSONDecodeError, so unwrapped it slips past corruption clauses. The display reader stays lenient -- and gains the object-root check it lacked, so a non-object ledger degrades to empty with a loud log instead of crashing every lookup with AttributeError. Each flow surfaces the refusal in its own contract instead of tearing through it. The uninstall handler pre-flights the read before anything destructive and returns a handled dependency_ledger_unreadable refusal -- the same shape as its trust-grant precondition, offloaded to the executor; a keep_dependencies purge never touches the ledger and skips it. The install resolver reports a refused record as a failed dependency (the install did happen; a retry after the ledger is repaired re-records it), and the dep-cleanup loop logs and continues so one unreadable read cannot strand the rest of the uninstall. Mirrors the merged kirodotdev#8084 readers (kirodotdev#7805 class).
… read All three mutations rewrite the whole ledger from what they read, and they stood on the lenient display reader: a transient EACCES/EIO or a truncated document read as empty and was then published back -- record_install over every other app's refcount, and classify_and_clean_for_uninstall as an outright wipe of the only record that an installed dependency is still referenced. The sidecar lock serializes writers and says nothing about a read that failed. _read_ledger_for_update is the mutation base: only a MISSING file reads as empty; an unreadable or corrupt document (and a root that parses but is not an object) propagates and the mutation is abandoned. UnicodeDecodeError is folded into the refusal: it is a ValueError but not a json.JSONDecodeError, so unwrapped it slips past corruption clauses. The display reader stays lenient -- and gains the object-root check it lacked, so a non-object ledger degrades to empty with a loud log instead of crashing every lookup with AttributeError. Each flow surfaces the refusal in its own contract instead of tearing through it. The uninstall handler pre-flights the read before anything destructive and returns a handled dependency_ledger_unreadable refusal -- the same shape as its trust-grant precondition, offloaded to the executor; a keep_dependencies purge never touches the ledger and skips it. The install resolver reports a refused record as a failed dependency (the install did happen; a retry after the ledger is repaired re-records it), and the dep-cleanup loop logs and continues so one unreadable read cannot strand the rest of the uninstall. Mirrors the merged kirodotdev#8084 readers (kirodotdev#7805 class).
3 participants
Summary
Closes #7805.
The four merged
*_for_updateread-modify-write readers —shares._load_for_update,library._read_ledger_for_update,KeystoneFileBackend._read_for_update(the provider credential store), andpolicy_store._read_for_update(the keystone policy file) — caughtjson.JSONDecodeErroralongsideFileNotFoundErrorand returned an empty container, so a corrupt or truncated file was silently read as empty and then rewritten whole, destroying bytes a person could still have recovered by hand. This mirrors the #7794 decision for the incident index and app config: only a MISSING file reads as empty; corruption refuses.Per reader:
UnicodeDecodeErroris aValueErrorbut NOT aJSONDecodeError, so unwrapped it slips past every corruption clause), and refuse valid JSON whose root has the wrong type (which parses without raising, so normalizing destroys a document nobody could read).secrets.pyadditionally refuses any entry the coercion would lose (deserialize → re-serialize → refuse if anything on disk did not survive), and its refusals carry NO document content: fixed messages, emptydoc, and a fully severed exception chain (__cause__/__context__bothNone), because here the raw file text IS the credential store. The parser's real line/column are folded into the message text.shares.pyalso checks per ROW thatexpiresAtis readable, because both mutations pipe the reader's return through_prune, whose damage path silently drops rows it cannot read a stamp from — the same coercion loss, one call later. The deliberate expiry drop (parseable stamp in the past) remains retention.CorruptDocumentError; the aws_control pair raises plainjson.JSONDecodeError(the named type lives in another app; apps do not import each other).Every caller of the four mutation paths was audited for the
JSONDecodeError-under-ValueErrortrap the issue names:except ValueErrorarms (without it, corruption reads asnot_pushable/invalid_slug— a 400 blaming the client for a store that needs repair).share_ledger_corruptvia a shared helper.reconciled: falsewith a repair-wordedremoteErrorand keeps rendering (rows come from the lenient display read)._store_read_refusalmapper's coded 500secret_store_corrupt; the policy-store writes were already wired through_settings_write_or_refuse's corruption arm.Display/lookup reads stay lenient — that asymmetry is the point — and LOG when they degrade for any reason other than an absent file, mirroring #7794's display reads. Per-call logging with no dedup matches the merged precedent (
store._read_index_unlocked). All four also now tolerateUnicodeDecodeError(previously it escaped them) — safe because every value read leniently now has a restrictive default. The one exception,primary_instance(defaults True, so a degraded read GRANTS ledger-prune authority — the corrupt file becoming the key that unlocks destroying shared knowledge), no longer reads through the lenient path at all:rotation.is_primarynow uses the new strictpolicy_store.read_authority, which reuses the update reader's corruption doors and answers False when the file cannot be read. Two GPT lane rounds drove this: round 1 flagged the widened door, round 2 correctly rejected scoping the pre-existing doors out as out-of-scope.Deliberately untouched, same as the issue scopes it:
_update_ledger's per-ACCOUNT scalar reset (replaces one account's unusable entry while carrying every other row forward; the sidecar-preserve alternative is tracked in #7789). Two further lenient-read-feeding-rewrite siblings named by the First Principles lane are tracked:backup.pyas item 1 of #7789,mochi/pinned_files_service.pyas #8088. The stale "siblings need a follow-up" rationale instore.pyandtest_store_and_gate.pyis updated to record that #7805 landed it.Review
Two pre-push blind review lanes (GPT 5.6, Opus). Both blocking findings fixed at root:
{"<token>": "scalar"}) andraise ... from exckept the full credential document reachable via__cause__.doc. Fixed: position-only message; chain severed by raising outside the handler; adversarial token-shaped-key test asserts the whole chain._prunedrops rows onKeyError/ValueError/TypeError), while the reader's docstring claimed the case could not happen. Fixed: strict per-row check; docstring corrected; regression test reproduces the reviewer's measured 3-row loss.Accepted advisories: stale #7794 docstrings updated;
nosemgrepannotation on the new fixed-literal warning (sibling precedent in-file);CorruptDocumentErrorpinned in policy tests; display-readUnicodeDecodeErrortolerance now tested per module. Not changed: per-call degradation logging (matches merged #7794 precedent); the route test'sassertNotIn(token, …)belt assertion stays alongside the discriminating doc-bytes assertion.Tests
test/test_aws_control_app.py,test/test_aws_control_routes.py,test/test_aws_control_library.py, and the fullops_mission_controlsuite.doc, or exception chain, token-shaped-key adversarial case).library_ledger_corruptnotnot_pushable; remove answers it notinvalid_slug; share mint withholds the URL; library list survives a corrupt ledger and reports repair.Pattern harvest
Rule candidate: a
*_for_updatereader's strictness must cover every transform between the read and the write (root type, row shape, coercion round-trip), not just the parse — a lenient normalizer downstream of a strict reader reintroduces the same silent loss one call later.Rule candidate: an exception raised from a credential store must carry no document content anywhere reachable — message,
doc,__cause__, or__context__; raise outside theexceptblock to sever the chain.Class: lenient-read-feeding-whole-file-rewrite (#7620, #7788, #7794, #7805; remainder tracked in #7789).