docs(solutions): record why repair must precede capture

[marcusrbrown]

Writes up #1407 and #1519 for docs/solutions/. The durable lesson is an ordering rule: repair on restore must precede capture on save, or a transient failure gets encoded into durable state that the next run then trusts.

Why this one is worth writing down

The bug was a stable absorbing loop. One bootstrap timeout persisted state that made every later run fail identically, and the only known escape was manually purging every cache entry. What made it stable rather than self-correcting is a detail that is easy to get backwards: Actions save keys are unique per run while restore keys are prefixes matching the newest entry — so declining to save prevents new poisoning but leaves the poisoned entry newest. Declining forever is a stable state, not a recovery.

The most useful section is what didn't work

Seven plausible fixes, each refuted by evidence rather than argument:

• "Only persist cleanly-checkpointed state" — nothing had ever produced one, so this would have blocked every save in the repo's history.
• "Declining to save breaks the loop" — the key-shape detail above.
• "Wipe and save empty state" — destroys session history on every transient failure.
• "The log is provably empty at transport" — a late write from a still-live writer regrew it from 0 to 4152 bytes after the checkpoint reported success.
• "Route non-retryable failures into the clean-slate path" — an allowlist of retryable, negated, hands a full disk or a permissions fault to the branch that deletes history.

Including the regression the fix caused

Recorded deliberately, because it was introduced by the fix and found only in review. Once healthy saves stopped producing a local write-ahead log, an upload that only ever sends what exists stopped refreshing that object while the database beside it kept being overwritten at the same prefix. One reproduction reported malformed; the other silently replayed stale content and turned 3000 rows into 2000 with no error at all.

Claims stated at the strength the evidence supports

The "archiving opencode.db alone yields a consistent database" argument is conditional, not absolute. SQLite's default wal_autocheckpoint is 1000 pages, and I measured a live writer crossing it mutating the main file from 4096 to 860160 bytes with no explicit checkpoint. The doc says "much safer", not "safe" — which matters, since this doc is partly about the cost of claims nobody tested.

Placement

Overlap scored Moderate at highest against the four adjacent docs (3/5 on the cache-continuity and S3-restore docs, 2/5 on deadline-cleanup, 1/5 on ID-space-wrap), so this is a new doc cross-linked in both directions rather than folded into an existing one.

Validated: 392 links resolve, module taxonomy guard passes.

[fro-bot]

Verdict: PASS

Docs-only change adding one solution doc. I verified the doc's substantive claims against the code on this branch rather than reading it for prose quality — for docs/solutions/, accuracy against the current tree is the correctness criterion, since these docs are retrieved and acted on by later agents.

Claims checked, all accurate:

Doc claim	Verified against
WAL removed from both producers	DB_TRANSPORTABLE_BASENAMES: readonly string[] = [DB_MAIN_BASENAME] (packages/runtime/src/session/version.ts:51) and buildSaveCachePaths pushing only DB_MAIN_BASENAME (src/services/cache/paths.ts:130)
Success judged by log size, not pragma count	src/services/cache/checkpoint.ts:67,96,157
Probe reads one schema page, not PRAGMA integrity_check	db.prepare('SELECT count(*) FROM sqlite_master').get() (src/services/cache/integrity.ts:65)
Corruption classified positively	message.includes('not a database') || message.includes('malformed') (src/services/cache/sqlite-errors.ts:53)
Repair sits between restore and bootstrap, gated on 'hit'	src/harness/phases/cache-restore.ts:65-134
Probe runs only on nothing-to-checkpoint	same file, the else branch at :123-133
Source-specific legacy split (Actions pair checkpointed, object-store log deleted)	deleteDownloadedObjectStoreWal + surrounding rationale, src/services/cache/restore.ts:97-130
waitForServerQuiescence polls the port	packages/runtime/src/agent/server.ts:69, called from :206

Guards I ran locally: check-md-links reports 392 links across 319 files, OK (matches the PR body), and scripts/module-taxonomy.test.ts passes 4/4 — module: runtime is in MODULE_TAXONOMY. All four Related targets and all five referenced source paths exist. Frontmatter values (async_timing, service_object, logic_error, code_fix, critical) all reuse vocabulary already present in the corpus.

I want to call out the calibration in "Why This Works" specifically. Hedging the opencode.db-alone claim to "much safer, not safe" and grounding it in a measured wal_autocheckpoint crossing (4096 → 860160 bytes with no explicit checkpoint) is the right call — an unqualified version of that claim would have been the exact failure the doc's own Prevention section warns about. Same for recording the regression the fix caused, including the silent 3000→2000 row variant, which is the more valuable half because it has no second line of defense.

Blocking issues

None.

Non-blocking concerns

1. No backlinks added, though the PR body claims bidirectional cross-linking. The description says the doc is "cross-linked in both directions", but the diff is a single added file and no existing doc references it (grep -r "repair-before-capture" docs/ → no matches). This repo does follow a backlink convention: docs/solutions/workflow-issues/checks-report-clean-for-what-they-cannot-observe-2026-08-10.md:112 carries a forward link to the newer verify-behavior-not-signal-2026-08-23.md, added when that doc landed. The two highest-overlap neighbors (read-only-actions-cache-token-broke-session-continuity-2026-08-11.md, scored 3/5, and s3-restore-needs-listbucket-and-prefix-scope-2026-08-12.md) are where a reader is most likely to be standing when this doc becomes relevant. Not blocking — retrieval is primarily frontmatter/tag-driven, so discoverability doesn't collapse without them — but either add the two entries or drop the claim from the description.

2. related_components: background_job, database are novel values. Neither appears in any other doc under docs/solutions/. The field is unguarded free-form (existing values range from tooling to bare file paths), so nothing fails, but database in particular is a term the corpus will likely want again — worth being deliberate about it now rather than accumulating near-synonyms.

3. "The regression the fix introduced" doesn't name its own remedy. The section fully characterizes the orphaned-opencode.db-wal failure but leaves the fix implicit in two other sections (the DB_TRANSPORTABLE_BASENAMES removal under Solution, and the untrusted-log deletion under the legacy split). Since these docs get retrieved by section rather than read end-to-end, a reader landing there via a malformed search could reasonably conclude the hazard is still live. One sentence pointing forward would close it.

Missing tests

None required — no runtime code changed. The two guards that do gate this content both pass, and I ran them rather than taking the PR body's word for it: bun run scripts/check-md-links.ts (392 links) and vitest run scripts/module-taxonomy.test.ts (4 passed). markdownlint-cli2 surfaces no findings in the new file; the errors it reports are pre-existing in README.md, deploy/README.md, and docs/audits/.

Risk assessment (LOW)

Documentation-only: one added file, zero deletions, no source, config, or workflow changes. No public API surface, no security exposure, no regression path. Blast radius is limited to what a future agent reads — and the mitigation for that risk is claim accuracy, which I verified point by point above. The one place a doc like this could do real harm is by misdescribing current behavior and sending a future reader down a wrong path; it does not.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	33601295218
Cache	hit
Session	ses_f9f149f58ffekFzkuBG4I5am7R

[marcusrbrown]

All three actioned at 3038bbc3f.

The backlink finding was a fair hit. I wrote "cross-linked in both directions" in the description and only added forward links — a checkable claim that was wrong, in a PR whose entire subject is the cost of claims nobody checked. Added entries from the two highest-overlap neighbours, since those are where a reader is most likely to be standing when this becomes relevant: the cache-continuity doc (a save that failed and reported success, against one that succeeded and persisted the wrong thing) and the S3 restore-scope doc (the same restore-is-not-save asymmetry one layer down — capability there, atomicity here). Links now 392 → 394.

The regression section now names its own remedy. Your framing is the right one: these get retrieved by section, not read end to end, so a reader landing there from a malformed search could reasonably conclude the hazard was live. The fix was described in two other sections and never pointed at from the one describing the problem.

Dropped related_components entirely rather than picking better values. Both were novel to the corpus and the field is unguarded, so they would have seeded near-synonyms for no retrieval benefit — tags already carries sqlite and session-cache. If database earns a place later it should be introduced deliberately, not as a side effect of one doc.

Guards re-run: 394 links across 319 files, module taxonomy 4/4.

[fro-bot]

Verdict: CONDITIONAL

Re-review of 3038bbc against my dismissed PASS. All three earlier non-blocking items are genuinely resolved — I re-ran the guards rather than reading the commit message: check-md-links reports 394 links across 319 files (up from 392, matching the claim), scripts/module-taxonomy.test.ts passes 4/4, bun run lint is clean for this change (8 pre-existing warnings, all in packages/gateway test files). related_components is gone from the frontmatter. The regression section now closes itself with "This is fixed, not outstanding", and I verified that paragraph against the tree: DB_TRANSPORTABLE_BASENAMES: readonly string[] = [DB_MAIN_BASENAME] (packages/runtime/src/session/version.ts:51), buildSaveCachePaths pushing only DB_MAIN_BASENAME (src/services/cache/paths.ts:128), and deleteDownloadedObjectStoreWal running from a finally before anything opens the database (src/services/cache/restore.ts). Both statements in that paragraph hold.

The backlinks themselves are the problem. They resolve — which is all check-md-links can tell you — but both were written from the new doc's vantage point and pasted into the neighbour docs without flipping the deixis, so both now describe the two docs backwards.

Blocking issues

1. docs/solutions/integration-issues/read-only-actions-cache-token-broke-session-continuity-2026-08-11.md:142 — the backlink inverts both docs, and contradicts line 139 of its own list.

- [Repair on restore must precede capture on save](...) — the inverse failure in the same
  subsystem: there the save genuinely failed and reported success; here the save genuinely
  succeeded and persisted state that poisoned every later run.

The corpus convention is unambiguous and this file establishes it three lines earlier at :139: there = the linked doc, here = the doc you are reading.

- [A check reports clean...](...) — the same epistemic trap through a different mechanism:
  there, a check whose observable scope was narrower than its claimed scope;
  here, a write that failed and reported success.

So :139 says here = a write that failed and reported success, and :142 says here = the save genuinely succeeded, about the same document, in the same bulleted list. One of them is wrong, and it is :142 — this file is the failed-save-reported-success incident (saveCache() returning -1 under a read-only token, CACHE_SAVED set from an unchecked call). The new doc is the succeeded-save-persisted-poison incident.

The sentence is correct in the new doc at :115, where it originated. It needs the two clauses swapped when it lives on the other side:

— the inverse failure in the same subsystem: here the save genuinely failed and reported success; there the save genuinely succeeded and persisted state that poisoned every later run.

I am treating this as blocking rather than a nit for two reasons. It is retrieval-facing: an agent landing on the cache-continuity doc from a session-continuity search reads a one-line characterisation of the linked doc that is the exact opposite of its content, and the neighbouring bullet gives it a false consistency signal for the convention. And it is the failure mode this PR exists to document — a checkable claim, stated confidently, that nobody checked. The fix is a clause swap.

2. docs/solutions/security-issues/s3-restore-needs-listbucket-and-prefix-scope-2026-08-12.md:116 — same inversion.

- [Repair on restore must precede capture on save](...) — the same restore-is-not-save
  asymmetry one layer down: capability there, atomicity here, ...

From inside the S3 doc, there = the new doc = atomicity, and here = the S3 doc = capability (ListBucket, prefix scope). The line asserts the reverse. This one is a verbatim lift from your PR comment, where the vantage point was the new doc and the phrasing was correct; the frame changed and the wording did not. Swap to atomicity there, capability here.

Non-blocking concerns

1. "one layer down" in the same S3 line also points the wrong way. Written from the new doc it means "S3 is the layer below"; read from the S3 doc it now claims the new doc sits below the storage/IAM layer. Fixing the capability/atomicity swap without touching this leaves half a sentence still oriented the wrong way — one layer up reads correctly from the S3 side.

2. docs/solutions/logic-errors/repair-before-capture-sqlite-session-cache-loop-2026-09-02.md:38 — "Five plausible fixes" undercounts the seven that follow. The section lists Only persist cleanly-checkpointed state, Decline to save, Wipe the database, The write-ahead log is provably empty, Recreate a zero-byte log, Route non-retryable failures, and Hoist the untrusted-log deletion. The PR description repeats "Five". Harmless to a reader who scrolls, but this is the section the doc itself calls "the useful one", and it is the third countable claim in this PR that does not survive counting.

3. Heading case drift. ## The regression the fix introduced is sentence case against ## Why This Works / ## What Didn't Work in the same file. Cosmetic, and the corpus already has one-off headings, so purely optional.

Missing tests

None required — no runtime code changed, and no test should be added for this.

Worth naming what the guards structurally cannot see, since this PR's own thesis is about that gap: check-md-links proves a target resolves, not that the sentence describing it is true. Both defects above passed it. A directional-prose check is not worth building, but it does mean cross-link text is reviewed only by a human or an agent reading both documents — which is exactly what happened here, one round late.

Risk assessment (LOW)

Documentation-only: three files, zero deletions, no source, config, or workflow changes. No public API surface, no security exposure, no runtime regression path. Blast radius is a future retrieval reading an inverted one-line summary of an adjacent incident and forming a backwards model of which subsystem failed which way — bounded, since both docs state their own case correctly in their bodies, but it lands precisely on the retrieval path these backlinks were added to serve.

Swap the two clauses in each backlink and this is a PASS. The substance of the doc is unchanged from my earlier verification and remains accurate against the tree.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	33602012726
Cache	hit
Session	ses_f9f0c62d6ffehSznpQnhQBa3R6

[fro-bot]

Verdict: PASS

Re-review of 4012ffd against my CHANGES_REQUESTED on 3038bbc. Both blocking inversions are fixed, and I checked the corrected sentences by reading each backlink from inside the file that now hosts it rather than trusting the commit message.

Blocking issue 1 — resolved. read-only-actions-cache-token-broke-session-continuity-2026-08-11.md:142 now reads:

— the inverse failure in the same subsystem: here the save genuinely failed and reported success; there the save genuinely succeeded and persisted state that poisoned every later run.

here = the cache-continuity doc = the failed-save-reported-success incident (saveCache() returning -1, CACHE_SAVED set from an unchecked call). there = the new doc = the succeeded-save-persisted-poison incident. That is the right assignment, and it now agrees with :139 three lines above (here, a write that failed and reported success) instead of contradicting it inside the same bulleted list.

Blocking issue 2 — resolved. s3-restore-needs-listbucket-and-prefix-scope-2026-08-12.md:116 now reads capability here, atomicity there. From inside the S3 doc, here = capability (ListBucket, prefix scope) and there = the new doc = atomicity. Correct.

Guards re-run, not taken on faith: check-md-links reports 394 links across 319 files, OK; scripts/module-taxonomy.test.ts passes 4/4. The doc's substantive claims still hold against the tree — DB_TRANSPORTABLE_BASENAMES: readonly string[] = [DB_MAIN_BASENAME] (packages/runtime/src/session/version.ts:51), deleteDownloadedObjectStoreWal (src/services/cache/restore.ts:130) invoked at :225 before the database is opened, and waitForServerQuiescence (packages/runtime/src/agent/server.ts:69) called from :206. Nothing in this commit touched the doc body, and the point-by-point verification from my first review stands.

Blocking issues

None.

Non-blocking concerns

1. I withdraw the confident framing on "one layer down". I called this inverted in my last review; on re-reading it I do not think that holds up. Two readings are defensible — "down" toward IAM/storage as the infrastructure layer beneath cache orchestration (my reading), or "down" toward SQLite file-level atomicity as the layer closest to the bytes (plausibly yours). The clause swap resolved the actual contradiction; this one is a judgment call and I should not have asserted it as a defect. Author's call, no change needed.

2. repair-before-capture-sqlite-session-cache-loop-2026-09-02.md:38 — "Five plausible fixes" still undercounts the seven that follow. The bolded items are Only persist cleanly-checkpointed state (:40), Decline to save (:42), Wipe the database (:44), The write-ahead log is provably empty (:46), Recreate a zero-byte log (:56), Route non-retryable failures (:58), Hoist the untrusted-log deletion (:60). The PR description repeats "Five". Carried over unresolved from my last review, and still not blocking — a reader who scrolls sees all seven and loses nothing. Worth a two-character fix on the next touch of this file, since this is the section the doc calls "the useful one".

3. Heading case drift. ## The regression the fix introduced remains sentence case against ## Why This Works / ## What Didn't Work. Cosmetic; the corpus already has one-off headings.

Missing tests

None required — no runtime code changed, and no test should be added for this.

Restating the gap this PR's own thesis is about: check-md-links proves a target resolves, not that the sentence describing it is true. Both defects in the previous round passed it. A directional-prose check is not worth building, so cross-link text stays reviewed only by whoever reads both documents — which is what caught it, one round late, and is the honest limit of the guard set here.

Risk assessment (LOW)

Documentation-only: three files, zero deletions, no source, config, or workflow changes. No public API surface, no security exposure, no runtime regression path. The one real hazard for a doc like this is misdescribing current behavior and sending a future retrieval down a wrong path; the body was verified against the tree across all three rounds, and the two cross-link summaries that did exactly that are now correct.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	33603022688
Cache	hit
Session	ses_f9f0060d6ffeFOzErrrmuUMxlB

[marcusrbrown]

Count and heading fixed at 32ecbcd7c; corrected the PR description too, since it repeated the same error.

"Five" was wrong and had survived two reviews. Seven refuted approaches, and the two the count dropped are ones I would least want lost — the zero-byte-log recreation (which races the same live writer that made the checkpoint untrustworthy) and the restoreCache hoist (which would have deleted a locally-present log on a self-hosted runner with the object store disabled). Both were refused with reasons, in the section the doc calls the useful one.

On "one layer down" — appreciated, and I am leaving it. Your withdrawal is fair and I would not have argued it either way; the reading I intended was toward SQLite file-level atomicity as the layer nearest the bytes, which is one of the two you named. The clause swap was the actual defect.

Your closing point is the honest one to end on: check-md-links proves a target resolves, not that the sentence describing it is true. Both of the previous round's defects passed it cleanly. A directional-prose checker is not worth building, so cross-link text stays reviewed only by whoever reads both documents — which is what caught it, one round late. That limit is worth stating plainly rather than implying the guard set covers more than it does.

[fro-bot]

Verdict: PASS

Re-review of 32ecbcd against my dismissed PASS on 4012ffd. This commit closes the last two carried-over non-blocking items, and I verified both by counting and reading rather than trusting the commit message.

Count — fixed and correct. repair-before-capture-sqlite-session-cache-loop-2026-09-02.md:38 now reads "Seven plausible fixes". The bolded refuted approaches count to exactly seven (grep -c '^\*\*"' → 7): Only persist cleanly-checkpointed state (:40), Decline to save (:42), Wipe the database (:44), The write-ahead log is provably empty (:46), Recreate a zero-byte log (:56), Route non-retryable failures (:58), Hoist the untrusted-log deletion (:60). Your point in the comment stands — the two the old count dropped are the two with the most transferable reasoning, since both were refused for a reason that generalizes (racing the same live writer; a guard that returns before the try).

Heading case — fixed. ## The Regression the Fix Introduced is now title case, consistent with ## Why This Works and ## What Didn't Work across all eight headings in the file.

Directional prose — all four cross-link summaries re-read from inside their host file, and all four are now correct. This is where the previous round broke, so I checked every direction rather than only the two that were fixed:

Location	here	there	Correct
read-only-actions-cache...:142	failed save, reported success	succeeded save, persisted poison	yes — agrees with :139
s3-restore-needs-listbucket...:116	capability (ListBucket, prefix scope)	atomicity	yes
new doc :115	succeeded save, persisted the wrong thing	failed save, reported success	yes
new doc :116	source-specific legacy split	restore capability not inferable from save	yes

Guards re-run: check-md-links reports 394 links across 319 files, OK; scripts/module-taxonomy.test.ts passes 4/4 (module: runtime is in the taxonomy).

Doc body re-verified against the tree — nothing in this commit touched the substantive claims, but the file moved, so I re-checked the load-bearing ones rather than deferring to three rounds of prior verification:

Doc claim	Verified against
Log removed from both producers	DB_TRANSPORTABLE_BASENAMES: readonly string[] = [DB_MAIN_BASENAME] (packages/runtime/src/session/version.ts:51); buildSaveCachePaths pushes only DB_MAIN_BASENAME (src/services/cache/paths.ts:130)
Success judged by log size, not pragma count	src/services/cache/checkpoint.ts:68,96-98,157
Probe reads one schema page, not integrity_check	db.prepare('SELECT count(*) FROM sqlite_master').get() (src/services/cache/integrity.ts:65)
Corruption classified positively	message.includes('not a database') || message.includes('malformed') (src/services/cache/sqlite-errors.ts:53)
Repair between restore and bootstrap, gated on 'hit'	src/harness/phases/cache-restore.ts:65-67,101-107
Probe runs only on nothing-to-checkpoint	same file, :123-129
Object-store log deleted before the DB is opened	deleteDownloadedObjectStoreWal (src/services/cache/restore.ts:130), called at :225
waitForServerQuiescence polls the port	packages/runtime/src/agent/server.ts:69, called from :206

Every claim holds. The opencode.db-alone hedge in "Why This Works" remains stated at the strength the evidence supports ("much safer", not "safe", grounded in a measured wal_autocheckpoint crossing), which is the right calibration for a doc whose own Prevention section is about untested confident claims.

Blocking issues

None.

Non-blocking concerns

1. The PR description still lists five bullets under "Seven plausible fixes". You corrected the count in the description, but the bulleted list beneath it still enumerates five — the zero-byte-log recreation and the restoreCache hoist are named in your comment as the two most worth keeping, and they are correctly present in the doc at :56 and :60, but not in the description's list. A colon followed by exactly five bullets reads as the enumeration of the seven. This affects nothing that merges and is purely a description artifact; I mention it only because the merged artifact is correct and the description is the last place the old shape survives.

2. The two sides of the S3 cross-link describe the relationship in different vocabulary. s3-restore...:116 frames it as capability here, atomicity there; the new doc's :116 frames it as "restore capability cannot be inferred from the save path… the same asymmetry appears here as the source-specific legacy split". Both are accurate and neither is inverted — a retrieval landing on either gets a true summary. Noting it only as an observation about the corpus's cross-link style; no change warranted.

Missing tests

None required — no runtime code changed, and no test should be added for this.

The guard-set limit you and I converged on is worth restating once, since it is now the only thing standing between this corpus and another inverted backlink: check-md-links proves a target resolves, not that the sentence describing it is true. Both defects from round two passed it cleanly. Building a directional-prose checker is not worth it, so cross-link text remains reviewed only by whoever reads both documents in both directions — which is what I did above for all four, and is the honest description of the coverage here.

Risk assessment (LOW)

Documentation-only: three files, zero deletions, no source, config, or workflow changes. No public API surface, no security exposure, no runtime regression path. The one real hazard for a doc in docs/solutions/ is misdescribing current behavior and sending a future retrieval down a wrong path — the body was verified point-by-point against the tree in this round as well as the previous three, and the two cross-link summaries that previously did exactly that are correct and now internally consistent with the neighbouring bullets in their host files.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	33671111541
Cache	hit
Session	ses_f9c7ba1c5ffeP94PA4ng3T3DXw