fix(mochi): refuse a corrupt pin store instead of rewriting it (#8088)

[iamwhatever]

Problem / Motivation

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 of pinned-files.json. A corrupt pin store was therefore silently replaced by whatever the in-memory list held.

Two writers mutate this file — the gateway service and the MCP server's pin_file / unpin_file — and the MCP side had the worse half of the same bug: it 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 had just refused to touch, so both now go through one reader.

Same lenient-read-feeding-whole-file-rewrite class as #7620, #7788, #7794 and #7805.

Why it matters

What is lost is exactly the rows another process wrote that this one never loaded, and the unparseable file was their only remaining copy — so the loss is silent and unrecoverable. On the MCP path the whole store goes. The pin list also drives a poller that stamps updatedAt, so a background tick could trigger the rewrite with no user action at all.

The user-visible half was just as quiet: mark-seen has no failure return, so the route reported {"ok": true} while the store was being replaced.

What changed (motivation → approach → change)

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 three shapes that all reach the same whole-file rewrite:

• a parse failure — the truncated bytes still hold most of the records verbatim, and only a person can recover them;
• a read error the filesystem raised (a transient EACCES/EIO, a scanner holding the handle on Windows) — the store is still there, this process just could not see it this instant;
• valid JSON whose root is not an object, or whose pins is not a list — no parse failure at all, so left uncaught it would be the quietest loss of the three.

A fourth refusal covers an undecodable byte, and decoding is strict here where load and the Node original use errors="replace". That 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 written in its place. Raised by GPT 5.6 review (span=617a8d5961a6) and fixed in this PR.

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 two HTTP handlers map it to 500 {"code": "pins_corrupt"}. Returning {"ok": false} would read as "no such pin", and for mark-seen as outright success. 500 rather than 503: corruption does not clear on retry, a person has to repair the file, and the bytes it still holds are exactly why the mutation refused. The exception text is not echoed — pin labels and paths are agent-authored and are redacted on the way out of the GET handler.

_process_watch_event is the one path that swallows the refusal and logs instead. 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. Every path a user drove raises, so the operator still learns the store needs repair.

The startup load path is deliberately untouched: there a corrupt file is replaced on purpose and preserved as a .bak.<now_ms> sidecar first, which is a decision with a recovery copy rather than a silent loss under someone else's write.

Refuse-only rather than #7789's sidecar-preserve, which the issue asked to weigh: this file already has the sidecar exactly where a replacement is intended, and the update path's whole problem is that no replacement was intended at all. Extending the sidecar there would mint a .bak on every refused mutation of the same unchanged bytes. This is one more data point for #7789, not a decision taken on its behalf.

Tests

test/test_mochi_pinned_files_cov80.py, 13 new tests across three classes.

• TestReadPinsForUpdate — absence returns None; a healthy file returns its list; unparseable bytes, a non-UTF-8 byte outside a string, a non-UTF-8 byte inside a JSON string, a wrong-shaped root, and an unreadable file each refuse; the service's reload leaves _pins untouched when refusing.
• TestCorruptStoreIsNotOverwrittenOnMutation — add_pin, remove_pin, mark_seen, a debounced watch event, and both MCP pin tools all leave the corrupt bytes intact and broadcast nothing; a mutation preserves a non-UTF-8 byte in a label; a missing store still lets the first pin land; the MCP tools still work on a healthy store.
• TestRefusalIsTheNamedType — each mutator raises PinsCorruptError.

The mutation cases assert on the file's bytes, not on the exception type, so they fail on the buggy code for the reason that regressed rather than because a new symbol is missing; the type is pinned separately.

Red-before, with these tests run against pristine origin/main source — 13 failed / 27 passed, each on a behavioural assertion:

path	behaviour on main
add_pin	rewrote the corrupt file from its in-memory list
remove_pin	rewrote it, landing "pins": []
mark_seen	rewrote it
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

All 13 pass on this branch.

Manual verification

N/A — no UI change. The behaviour is a refusal on a corrupt on-disk file, which the red-before table above exercises end to end through the public mutators, the tick loop and both MCP tools.

Local gates: 610 tests green (all 17 test_mochi_*.py plus test_black_gate_contract.py), black, isort, flake8, mypy (23 files), and the 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 (lenient read feeding a whole-file rewrite: #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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

A real data-loss class fixed at its actual chokepoint — one strict reader shared by both writers, under the lock they already share — with the alternative (sidecar-preserve) explicitly weighed.

[DESIGN-REVIEWED] d6c2c00

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] d6c2c00

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

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of d6c2c00268ba126904a7d13d3c8458b85b97017b — 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 checks are done — I have what I need for the verdict.

First-Principles-Verdict: CONCERNS

The pin fix is real and earns its surface, but the same lenient-read→whole-file-rewrite sits unfixed three times in the very file this PR edits.

What this change ships

Intent: stop a corrupt pin store from being silently destroyed by any pin mutation, from either writing process — a FIX.

1. Dashboard unpin/mark-seen no longer silently rewrite a corrupt pin store — justified
2. Agent pin_file/unpin_file no longer zero a corrupt store — justified
3. A non-UTF-8 byte in a pin label is refused instead of mangled on disk — justified
4. Unpin/mark-seen now answer 500 pins_corrupt instead of ok — justified (AGENTS.md code rule)
5. Background tick skips the stamp and logs instead of rewriting — justified
6. New public read_pins_for_update / PinsCorruptError — 3 and 4 counted consumers, justified
7. MCP pin matching now skips malformed rows (_pin_path) — rides along, duplicate of _entry_path
8. MCP writes drop unknown root keys ({"version": 1, "pins": …}) — undeclared; matches _persist, harmless
9. _read_json in mcp_server.py left with zero callers — dead code

Watch

Point patch with counted siblings in the same file: grepped read_queue|read_watchlist feeding a rewrite — mcp_server.py:460 and :560 (read_queue(...) or _empty_queue → write_queue_atomic, no backup: a corrupt queue is zeroed) and :585 (read_watchlist → write_atomic; its errors="replace" read at watchlist_file.py:210 is exactly the lossy-decode-feeding-a-writer pattern the PR's own harvest names). Accepted-and-deferred at most — the description names the class but not these three.

Subtractions

• Delete _read_json (mcp_server.py:666) — this PR removed its last two callers; grep count: 0 remaining.
• Drop _pin_path (mcp_server.py:659) — byte-identical second spelling of _entry_path (pinned_files_service.py:184), a module mcp_server already imports three names from; use that one.

[FIRST-PRINCIPLES-REVIEWED] d6c2c00

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/apps/builtins/mochi/pinned_files_service.py:121 -- "Three shapes refuse" and the .bak claim contradict load, which accepts invalid UTF-8 inside strings without backup -> Fix: document four refusals and remove the backup claim.
[GPT-REVIEWED] d6c2c00

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

[iamwhatever]

• span=617a8d5961a6 — Invalid UTF-8 inside JSON strings is silently rewritten — fixed in d6c2c0026.

Legitimate, and the sharpest instance of the very loss this PR exists to prevent. Reproduced before changing anything, against the reviewed commit:

original: b'{"version": 1, "pins": [{"path": "/a", "label": "caf\xe9 notes"}]}'
after add_pin: b'... "label": "caf\\ufffd notes" ...'
0xe9 present before: True   after: False

Invalid byte in a quoted label -> mutation parses U+FFFD -> whole-file rewrite irreversibly replaces the original bytes.
Fix: Decode strictly and convert UnicodeDecodeError to PinsCorruptError.

The reasoning I had written into the docstring was wrong, not merely incomplete: I claimed the lenient decode was harmless because U+FFFD would fail in json.loads. That holds only when the bad byte falls outside a JSON string. Inside a quoted value the document parses cleanly, so no parse-failure clause anywhere downstream could have caught it — which makes it the one corruption shape that reaches the whole-file rewrite with nothing raised and nothing logged.

Applied exactly as suggested: read_pins_for_update now reads bytes and decodes strictly, with a UnicodeDecodeError arm placed before the OSError arm — UnicodeDecodeError is a ValueError, not an OSError, so it would otherwise escape unwrapped and slip past every caller's PinsCorruptError clause.

load deliberately keeps errors="replace": it performs no write, and a file it accepts is preserved as a .bak.<now_ms> sidecar first. A mangled label can therefore still reach memory at startup, but it can no longer reach disk, because every write path re-reads through the strict reader and refuses.

Two tests, both proven red against the pre-fix revision of this branch (2 failed / 1 passed): test_a_non_utf8_byte_INSIDE_a_json_string_refuses covers the reader, and test_a_mutation_preserves_a_non_utf8_byte_in_a_label asserts on the raw bytes through add_pin, so it fails on the old code for the reason that regressed rather than on a missing symbol.

The PR body's UTF-8 claim was corrected in the same push, and the generalisable form is recorded under ## Pattern harvest: a lossy decode is a repair, and a repair is only safe on a path that does not persist what it read.

[chenmingwei23]

Tier 1 auto-approve: fix (4 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: maps the pin store's corruption refusal to a coded HTTP 500 instead of zeroing the file on rewrite (#8088).