fix(memory): degrade undecodable preferences/projects to empty

[soroush5]

Problem / Motivation

A single non-UTF-8 byte in preferences.md crashes every preferences reader with a raw UnicodeDecodeError — verified: read_preferences() and add_preference() both blow up on b"# prefs\n\xff\xfe bad bytes\n".

Why it matters

Blast radius is the whole surface: the dashboard memory tab (500), get_context prompt assembly every turn, and — worst — the repair path itself, which reads before writing. A crash-mid-write, an editor encoding slip, or a synced file with one bad byte deadlocks the user out: the API cannot fix what the API cannot read.

What changed (motivation → approach → change)

The codebase already solved this exact case in _guarded_entry (decode fallback → warning + empty); the two small readers just never got it. read_preferences and its identical twin read_projects now catch UnicodeDecodeError, log a warning, and return empty — one bad byte degrades to "no preferences" instead of killing the tab, the turn, and the repair.

Tests

• test_non_utf8_preferences_degrade_to_empty: corrupt file reads as empty, and add_preference repairs instead of crashing (red without the fix, green with it).
• test_non_utf8_projects_degrade_to_empty: same for the twin.
• Full test_memory_cov80.py green.

Manual verification

N/A — unit coverage sufficient: crash and fallback both locked by automated tests against a tmp workspace. Same environment note as before: repo session conftest errors on this machine (pre-existing), suites run with --noconftest.

Fixes #8247

Pattern harvest

Rule candidate: strict UTF-8 reads of user-editable state files need a decode fallback — one bad byte should degrade, never crash every reader including the repair path.

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 2186415

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 2186415dae23d71ea21dca51bb30e2a4d6155801 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.

All evidence is gathered. The base's _guarded_entry (memory.py:770) degrades undecodable files to empty; the diff ships lossy decode instead — with an in-diff rationale — while the title and description still say "to empty". I also found two unfixed siblings in vector_memory.py and an in-diff duplication of the new helper. Final review:

First-Principles-Verdict: CONCERNS

Title and description promise "degrade to empty"; the diff ships lossy decode and argues empty would wipe content — the code is right, the framing is stale.

What this change ships

Intent: stop one bad byte in a memory file from crashing every reader, including the repair path — a FIX.

1. Corrupt preferences.md reads as its surviving content instead of crashing — justified, but contradicts the stated "empty"
2. Corrupt projects.md same — justified, same contradiction
3. append_history survives a corrupt today-file — undeclared sibling fix
4. Recent-history assembly survives a corrupt day — undeclared sibling fix
5. rebuild_index survives any corrupt memory file — undeclared sibling fix
6. New warning log on an undecodable memory file — justified
7. New private helper _read_text_lossy — 3 consumers, but not used at the 2 headline sites

Watch

• Description says "log a warning, and return empty — one bad byte degrades to 'no preferences'"; the diff returns raw.decode("utf-8", errors="replace") and its own comment says empty "would pass the compare-and-swap check… and let a consolidation overwrite" — the shipped semantics are the derived-correct ones; update the title/description, not the code.
• Point patch, 2 counted unfixed siblings: vector_memory.py:4149 and :4179 strict-read the same preferences.md/projects.md (grep: read_text(encoding="utf-8") on those paths) and still crash on the same byte — while vector_memory.py:4206 already reads history lossy in the same function.
• The module now holds two degrade policies for the same corrupt file: _guarded_entry (memory.py:840) returns empty, the readers return lossy — expect divergence questions.

Subtractions

• Delete the inline try/except bodies in read_preferences and read_projects and call _read_text_lossy — the inline copies are a second and third spelling of the helper, and their double read_text contradicts the helper's own single-read rationale.

[FIRST-PRINCIPLES-REVIEWED] 2186415

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

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

Design-Verdict: PASS

Verified crash with a real deadlock (repair reads before writing), fixed at the read boundary with lossy decode that deliberately preserves the CAS baseline — sound and proportionate.

Watch

• The description says the readers "return empty", but the diff ships lossy decode (errors="replace"), a deliberately different and better semantic (empty would pass the write_preferences CAS check and let a consolidation wipe surviving content). Update the PR text so the record matches what shipped — a reader of [Bug]: non-UTF-8 byte in preferences.md crashes every reader (dashboard tab, prompt assembly, and the repair path) #8247's resolution will otherwise expect empty-on-corruption.

Suggestions

• read_preferences/read_projects re-read the file on the lossy path, reopening the TOCTOU window the new _read_text_lossy helper's docstring explicitly closes ("costs no second I/O") — route both readers through the helper instead of duplicating the try/re-read inline.

[DESIGN-REVIEWED] 2186415

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

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

1 of 2 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/memory.py:325 -- Lossy decoding defeats stale-write protection
return self._preferences_file.read_text(encoding="utf-8", errors="replace")
Shift-JIS edit during consolidation -> distinct bytes decode identically -> CAS passes -> user edit is overwritten.
Anchor: residual/crash-data-loss-corruption
Fix: preserve and compare a byte-exact baseline under the write lock.

BLOCKING -- src/kiro_crew/memory.py:325 -- Decode fallback adds blocking I/O on the event loop
return self._preferences_file.read_text(encoding="utf-8", errors="replace")
Large file with an invalid trailing byte -> dashboard GET -> two synchronous full-file reads -> gateway loop stalls.
Anchor: no-blocking-call-on-event-loop
Fix: route preferences and projects through _read_text_lossy() on the initial read.

FINDING -- src/kiro_crew/memory.py:509 -- _read_text_lossy(path) also changes history append, context, and indexing outside the stated preferences/projects scope -> Fix: remove the history-path substitutions. (origin: validation)

[GPT-REVIEWED] 2186415
[BLOCK-MERGE] 2186415

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

Evidence gathered. Writing the ruling.

F2 — adjudicable

Anchored to no-blocking-call-on-event-loop, an AUTOSDE rule carrying blocking: true (AUTOSDE.yaml:91-92), which lists "large synchronous file IO" reachable from the loop as forbidden and records it as a repeated production-wedge cause. Confirmed the read runs inline on the gateway event loop: the dashboard GET calls mem.read_preferences() directly at dashboard/handlers/memory.py:129, unlike the PUT which offloads via asyncio.to_thread at :127. The diff adds a second synchronous read_text on the corrupt-byte path (memory.py read_preferences/read_projects fallbacks). The rule's flag is authoritative and outranks my weighing; the reviewer's fix (route through the single-read _read_text_lossy helper already in the diff) is cheap. → UPHOLD.

F1 — fenced (annotate-only)

Conditions: file must hold invalid UTF-8 (fallback only fires on UnicodeDecodeError); a consolidation must be in flight with expected_baseline from read_preferences() (history_consolidation.py:775); a concurrent edit must land in the window; and the CAS at memory.py:322 compares decoded strings (self.read_preferences() != expected_baseline). Because the compare is on decoded strings, any edit changing a decodable character alters the string and is caught; only byte differences that lossy-decode to the identical string (i.e. undecodable garbage bytes collapsing to the same U+FFFD sequence) can pass. Recovery: the consolidation write proceeds as it would have anyway; the only "lost" bytes are ones invisible after decode. No user-meaningful content is reachable for loss — the residual is garbage-byte identity a human would accept. → FLAG.

[ADJUDICATION] 2186415 total=1 uphold=1 downgrade=0
UPHOLD F2 src/kiro_crew/memory.py:325 reason=autosde-blocking-rule
[GPT-ADJUDICATED] 2186415

[ADJUDICATION-FENCED] 2186415 fenced=1 flagged=1
FLAG F1 src/kiro_crew/memory.py:325 -- CAS at memory.py:322 compares decoded strings, so any meaningful edit differs and is caught; only decode-invisible garbage bytes can pass, losing no user-meaningful content.
[GPT-ADJUDICATED-FENCED] 2186415

🏷️ Fenced finding(s) machine-flagged as likely edge case

The security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override recorded by a repository writer, who must independently verify a rationale before recording it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop. (This lane's comment deliberately carries no override command.)

• F1 src/kiro_crew/memory.py:325 — CAS at memory.py:322 compares decoded strings, so any meaningful edit differs and is caught; only decode-invisible garbage bytes can pass, losing no user-meaningful content.

[bolichen97]

Open PR relationship audit

This 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

• PR #8084 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8084: REBASE. Same theme, opposite and non-conflicting halves of it, disjoint code. Files: src/kiro_crew/memory.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
• This PR is OVERLAPPING with PR #7794. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8248: CONTINUE_DEVELOPMENT. Recently merged precedent in current main points the opposite way for reads that feed writers; PR 8248 should either follow it or state why the memory markdown store is the exception. Files: src/kiro_crew/apps/builtins/ops_mission_control/backend/store.py.
• This PR is OVERLAPPING with PR #8246. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8248: CONTINUE_DEVELOPMENT. Sibling hardening PR, not a duplicate: disjoint symbols and hunks, verified clean merge of the two heads, so both can land independently in either order. Files: src/kiro_crew/memory.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[soroush5]

Addressing the GPT BLOCK — reproduced it and you are right. Tracing the path: consolidation reads the baseline via read_preferences() (my "" fallback), merges, then write_preferences(merged, expected_baseline="") re-reads "" inside the lock, the CAS comparison passes, and the whole file is overwritten — the surviving valid bytes die with it.

Reworked to errors="replace" decoding: the baseline keeps every valid byte (only the bad byte becomes U+FFFD, deterministically, so CAS still matches on re-read), the dashboard/repair paths still never crash, and a new round-trip test pins that a consolidation computed from a corrupt file preserves the valid content. Single commit, pushed.

[soroush5]

Follow-up on both CONCERNS (now 2 commits, pushed):

• Design (empty baseline feeds writers): fixed as you suggested via the errors="replace" alternative — the baseline keeps every valid byte, only the bad byte is lost, and a round-trip test pins it. No quarantine file needed.
• First Principles (4 unfixed siblings): all covered. rebuild_index now reads prefs/projects through the fixed readers, and history reads in rebuild_index/append_history/_read_recent_history_uncached go through one new _read_text_lossy helper, with a test driving all three off the same corrupt daily file. Verified live: rebuild indexes, append preserves, recent history assembles.

[soroush5]

Noted on the relationship audit — no overlapping change on my side, nothing to coordinate.

[soroush5]

Taken on the BLOCKING finding — fair catch. _read_text_lossy now reads the bytes once and decodes strict first, re-decoding the same bytes lossy only on failure, so the corrupt path costs no second I/O (pushed, 24 green locally). On the FINDING about the three history/index sites: keeping them. The sibling coverage was added for the First Principles CONCERNS on the earlier commit, which explicitly counted those unfixed twins re-reading the very files this fixes — narrowing again would reopen that. Happy to split them into a separate rule if a maintainer prefers.

[bolichen97]

Closing in favour of #8378, which fixes the same defect (issue #8247) at the same call sites.

Both PRs make read_preferences / read_projects (and the same three sibling reads) decode with errors="replace" instead of raising UnicodeDecodeError, log a warning, and keep the surviving bytes so the read-before-write CAS baseline is not blanked. #8378 additionally hardens the reader against a planted symlink / hardlink / special file and is ~5 commits behind main versus ~270 here, so it is the one to carry forward.

One site this PR covers and #8378 currently skips: rebuild_index's history *.md glob is read lossy here. I have noted it on #8378 so it can be folded in there. Thanks for the fix -- it is landing, just via the other PR.

[soroush5]

Makes sense — I went through #8378 and it covers the same readers plus the link hardening, which mine does not have. Happy for this to land via that PR. Thanks for looking at it closely.