feat(wiki-writer): add the gated GitHub write path and operation ledger

[marcusrbrown]

Unit 2b — the writer can now commit to fro-bot/.github's data branch. This is the first code in this repository that holds GitHub write authority.

Atomicity

PATCH /git/refs/{ref} has no expected-current-value parameter, so the compare-and-write comes from two things together: the commit object is built with parents: [input.parentSha] against the observed head, and the ref update passes force: false so only a fast-forward is accepted. GitHub returns 422 on a non-fast-forward, which maps to a caller-visible 412.

There is no retry loop. The reference implementation upstream — commitWikiChanges in scripts/wiki-ingest.ts — catches a conflict, re-reads head, rebuilds the tree against the new tip, and retries. That is correct for autonomous survey writes, which just need to land. For an operator write it is exactly the lost update the precondition exists to prevent: it would silently rebase the operator's content over someone else's concurrent change. A conflict returns 412 and the draft is preserved.

Proven by forcing the ref update: the test fails, and passes again once force: false is restored.

Reconciliation

Every commit carries a Fro-Operation-Id trailer, and an intent record is persisted before the write: operation ID, target, expected parent, content digest.

When the outcome is ambiguous, reconciliation requires all three signals — matching content digest, matching trailer, and the expected parent in the commit's ancestry. Matching content alone is not proof the write landed, because another actor can produce identical bytes. That case stays indeterminate.

Outcomes are succeeded, failed, or indeterminate. An indeterminate result is persisted and surfaced for resolution, never reported as failure and discarded, and never blindly retried. An ambiguous external write is a state to resolve, not an exception to swallow.

Proven by relaxing reconciliation to content-only: the test fails by reporting success.

Bounds and gates

The allowed repository, the data ref, and the wiki/corrections path allowlist are hardcoded and checked before any GitHub access. An installation token with contents:write cannot be scoped to a single branch and path, so this allowlist is the only thing bounding the credential.

The gate-contract checker from #425 is now wired in and runs before the ledger intent is created — it provided no live protection until this PR. Gate rejections surface only spans present in the submitted content, which the operator can already read in their own editor. Writer-internal failures collapse to one generic response plus a correlation ID.

Ledger and retention

node:sqlite with bounded count and age pruning. Retention will not delete unresolved indeterminate records — those are precisely the ones someone needs in order to resolve an ambiguous write. Proven by allowing their deletion: the test fails.

Credential boundary

The App private key is loaded from a file path, never an environment value, matching the HMAC secret posture from Unit 1.

The boundary test walks every .ts/.tsx file under src/ and web/src/ — 98 today — and fails if any references the write client, the key path constant, its environment variable, or imports anything from wiki-writer/. The file list is enumerated from the filesystem rather than hardcoded, and the test asserts the walk found a plausible number of files so a glob that silently matches nothing cannot pass.

Its first version read two hand-picked files. That version would have passed unchanged if someone added a new file under src/ importing the write client, which is an unfalsifiable guard on the one boundary that keeps a write-capable key out of the web process. Both trees are now proven covered by planting a violation in each and confirming the failure names the offending file.

Not in this PR

Dashboard routes, the writer client on the dashboard side, browser drafts, UI, image publication, and the Compose/network/secret wiring in marcusrbrown/infra. No live canary was run — that belongs to deployment.

pnpm check-types, pnpm lint, pnpm test pass: 2051 root tests across 38 files, 1038 web tests across 25.

[fro-bot]

Verdict: CONDITIONAL

Reviewed wiki-writer/src/{contract,github-data-client,operation-ledger,retention,server,write-operation}.ts and their tests directly against the diff (e8e51b5..HEAD). The credential-handling and precondition/atomicity core is careful and well-proven (private key loaded only from a file path, non-fast-forward force: false mapped to 412, gate-finding redaction bounded to text already present in the submitted content, and the credential-boundary walk enumerating src/+web/src/ at runtime rather than a hardcoded list). No secret-leak or auth-bypass found. The blocking items below are correctness/reliability gaps in the new write/ledger machinery itself, not the security boundary.

Blocking issues

1. Missing runtime dependency declarations for the isolated service — wiki-writer/src/github-data-client.ts:3-4 imports @octokit/auth-app and @octokit/core, but wiki-writer/package.json (lines 10-12) declares only @fro-bot/wiki-write-core. This resolves today solely because pnpm-workspace.yaml sets shamefullyHoist: true, flattening the whole monorepo's deps into the root node_modules. AGENTS.md and this PR's own description say wiki-writer is "a separately deployed" service (Compose/network/secret wiring lives in marcusrbrown/infra, not this repo). If that deployment does an isolated install scoped to this workspace member (e.g. pnpm --filter @fro-bot/wiki-writer deploy, the standard pnpm pattern for shipping just one service's dependency footprint — which is exactly what "separately deployed" implies), the octokit packages would be absent and the service would crash on startup trying to authenticate as the GitHub App, on the very write path this PR adds. Fix: add "@octokit/auth-app": "8.3.0" and "@octokit/core": "7.0.8" (matching the root pins) to wiki-writer/package.json dependencies.

2. Ledger race on duplicate operationId submission throws uncaught — in write-operation.ts, the idempotency check options.ledger.get(request.operationId) (line 108) and the later options.ledger.begin(...) (line 151) are separated by multiple await points (getSnapshot, gates.run). Two concurrent requests carrying the same operationId (e.g. a client retry racing the original after a perceived timeout — exactly the scenario idempotent operation IDs exist to protect) can both observe "no existing record," and the second ledger.begin() call throws 'operation intent already exists' (operation-ledger.ts:93). That throw is not wrapped in any try/catch in execute(), propagates out of writeOperation.execute() in server.ts:71 (also uncaught there), and is only caught by handleNodeRequest's generic .catch() (server.ts:90), which returns a bare 500 with no body — bypassing the "generic response plus a correlation ID" contract the PR description itself states for writer-internal failures. Fix: on a unique-constraint race, re-read the now-existing record and return resultFromExisting() instead of letting the insert error surface.

3. Crash-interrupted (pending) operations are misreported as failed with no reconciliation attempt — resultFromExisting() (write-operation.ts:284-288) special-cases only succeeded and indeterminate; a record still in pending state — meaning the writer crashed anywhere after ledger.begin() but before any completeLedger() call, across the blob/tree/commit/ref-update sequence — falls through to return {state: 'failed', ...} on resubmission, without checking whether the commit actually landed on GitHub. This is the same class of ambiguity reconcile() already exists to resolve for ref-update failures, but the crash-recovery path never calls it. A caller that trusts the false "failed" signal and retries with a fresh operationId can push a duplicate commit for content that already landed — the exact "lost update"/duplicate-write class this PR is designed to prevent. Fix: run the same reconciliation check against GitHub state for a resubmitted pending record before defaulting to failed.

Non-blocking concerns

• github-data-client.ts getSnapshot() issues one getBlob call per tree entry sequentially (lines 135-144) — an N+1 pattern that adds real latency and API-quota cost to every single write as the wiki grows. Worth batching with Promise.all at minimum.
• write-operation.ts:120-121 rejects any request whose path isn't already in existingFiles (existingContent === undefined → invalid-request), so this path can only edit pre-existing pages/corrections, never create a new file — including the very first write to knowledge/corrections.yaml if it doesn't yet exist on the data branch. createSharedWikiWriteGates.run (line 217-219) has explicit handling for a corrections file that doesn't exist yet, which is unreachable given the earlier check. If pre-seeding is guaranteed out-of-band, fine, but the mismatch between the gate's defensiveness and the operation's rejection is worth a comment or a confirming test.
• server.ts:245 casts the wire-level WikiWriteRequest (contract.ts's corrections?: readonly Record<string, unknown>[]) straight to write-operation.ts's RecordCorrectionInput[] via as unknown as, with no runtime shape validation at the boundary. Malformed correction objects are currently caught indirectly by the try/catch around gates.run(), so it isn't exploitable today, but the type safety here is nominal only.

Missing tests

• No test exercises the ledger race described in blocking issue #2 (two execute() calls for the same operationId overlapping between the ledger.get check and the ledger.begin insert).
• No test exercises resubmission of an operationId whose ledger record is still pending (only succeeded/indeterminate resubmission paths are covered in write-operation.test.ts).
• No test covers a write targeting a path absent from the snapshot (new-page creation), to document whether rejection is intentional.
• Nothing in CI would have caught issue #1 (missing package.json dependency entries) — pnpm check-types type-checks wiki-writer/**/*.ts via the root tsconfig.json (include: ["**/*.ts"]), but tsc resolves modules from the hoisted root node_modules regardless of what wiki-writer/package.json declares, so it can't detect this gap. Worth a lightweight manifest-vs-imports check, or at minimum a docs/solutions/ note on the shamefullyHoist blind spot for isolated service deployments.

Risk assessment (MED)

The write path's security-critical properties — private key never touching an env var or the dashboard process, generic redacted responses for writer-internal failures, non-fast-forward always mapped to 412 without a silent-rebase retry loop, and the credential-boundary test walking the real filesystem rather than a hardcoded file list — are all solid and specifically proven with adversarial tests (forced force:true, relaxed content-only reconciliation, planted import violations). Nothing here leaks secrets or opens an auth bypass. The risk is operational: a packaging gap that could break the isolated deployment this feature is explicitly built around, plus two edge cases (concurrent duplicate submission, crash mid-write) in the exact ledger/reconciliation machinery this PR introduces to make writes safe against exactly those conditions. None compromise the security boundary, but they undermine the idempotency/crash-recovery/deployability guarantees that are this PR's stated purpose, so I'm requesting changes rather than passing with notes.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/dashboard
Run ID	33707245162
Cache	hit
Session	ses_f9aee490effeYF2By7KTmEEgSN

[fro-bot]

Verdict: PASS

Re-review after the push (commits 831107d..6648c54) that followed fro-bot's prior CHANGES_REQUESTED review. I read the current diff directly (fd0b78d..6648c54), re-verified the three prior blocking findings against the code as it stands today, and ran the actual gates rather than trusting the PR description: pnpm --filter @fro-bot/wiki-writer exec vitest run (10 files, 70 tests, all passing), pnpm check-types, and pnpm exec eslint wiki-writer/src wiki-writer/test — all clean.

All three previously blocking issues are now fixed:

1. Missing runtime dependencies — wiki-writer/package.json now declares @octokit/auth-app@8.3.0 and @octokit/core@7.0.8, matching the root pins exactly. wiki-writer/test/runtime-dependencies.test.ts is new: it AST-walks every import in wiki-writer/src/ and asserts each external package name is present in package.json dependencies, so this class of gap fails CI going forward rather than only surfacing at isolated-deploy time.
2. Ledger race on duplicate operationId — write-operation.ts now tracks in-flight executions via an inFlight Map of deferred promises alongside the ledger.begin() call, and the catch around begin() recognizes isDuplicateIntentError and returns the in-flight promise (same-process race) or re-reads via resultFromExisting() (cross-process race) instead of letting the insert error propagate to a bare 500. Covered by a new test that forces two execute() calls to race past the async gap using a manually-resolved deferred (write-operation.test.ts:134-156), confirming createCommit is called exactly once and both callers observe the same result.
3. Crash-interrupted pending resubmission misreported as failed — resultFromExisting() now branches on record.state === 'pending' and runs the same three-signal reconcile() (digest + trailer + ancestry) used for ref-update ambiguity, only falling back to indeterminate (never a false failed) when reconciliation can't confirm the write landed. Two new tests cover both outcomes (write-operation.test.ts:158-200).

The non-blocking items from the prior review are also resolved: getSnapshot() now reads blobs through an 8-way concurrent worker pool (github-data-client.ts:137-156) instead of sequential N+1 calls, and the as unknown as cast at the request boundary is gone — contract.ts now has real runtime type guards (isRecordCorrectionInput, isCorrectionSpan, isCorrectionAttribution) validating corrections shape before they reach write-operation.ts.

Security posture is unchanged from the prior pass and remains solid: private key loaded only from a file path, force: false mapped to 412 with no silent-rebase retry, gate-finding redaction bounded to text already in the submitted content, and the credential-boundary test walks the real src/+web/src/ filesystem (now with a sourceFiles.length > 90 sanity floor) rather than a hardcoded file list.

Blocking issues

None.

Non-blocking concerns

• write-operation.ts:124 still rejects any write to a path absent from existingFiles (only knowledge/corrections.yaml is exempted for bootstrap). This is very likely intentional — the writer edits existing pages, doesn't create new wiki pages — but there's still no test documenting/asserting this rejection path, so a future refactor could silently change the behavior without a red test. Low priority given the narrow allowlisted path space.

Missing tests

• No test exercises a write request whose path is in the allowlist but absent from the current snapshot (new-page creation attempt), to lock in the current rejection behavior as intentional rather than incidental.

Risk assessment (LOW)

The write path's security-critical properties are unchanged and solid (private key file-path-only, non-fast-forward always 412, redaction bounded, credential boundary enumerated from the real filesystem). The three correctness/reliability gaps that justified the prior CONDITIONAL — the deployment packaging gap, the concurrent-duplicate-submission race, and the crash-mid-write false-failure — are each fixed with a matching regression test, and I confirmed the fixes by reading the code and running the tests/type-check/lint myself rather than taking the diff at face value. Remaining concern is a documentation-level test gap on new-page rejection, not a functional risk.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/dashboard
Run ID	33920283036
Cache	hit
Session	ses_f91b80a9effeMnDeCXVSvetMp7

[fro-bot]

Verdict: PASS

Re-review after the latest push (c543394 on top of 6648c54, the commit fro-bot's prior APPROVED review already validated). Diffed 6648c54..HEAD directly rather than trusting the description — the only change is one new test in wiki-writer/test/write-operation.test.ts (+14 lines, no other file touched):

it('rejects creating a wiki page that is absent from the snapshot', ...)

This closes the exact gap flagged as a non-blocking concern / missing test in the prior review: write-operation.ts:124's existingContent === undefined && request.path !== 'knowledge/corrections.yaml' rejection was previously unlocked-in behavior. I read the test against the current execute() implementation and confirmed it exercises the real path: snapshot() only populates knowledge/wiki/topics/example.md, the new test submits knowledge/wiki/topics/new-page.md, and asserts {state: 'rejected', reason: 'invalid-request'} plus createBlob never called and no ledger row created (ledger.get(...) returns undefined) — correctly proving the check fires before ledger.begin() at line 158, not after.

Verified independently rather than taking the diff at face value:

• pnpm --filter @fro-bot/wiki-writer exec vitest run — 10 files, 71 tests passing (was 70; the new test is the only addition and it passes).
• pnpm check-types — clean.
• pnpm exec eslint wiki-writer/src wiki-writer/test — clean.

No security-relevant code changed. Security posture is unchanged from the prior PASS pass (private key file-path-only, non-fast-forward always mapped to 412, gate-finding redaction bounded to submitted content, credential-boundary walk over the real filesystem with a sanity floor).

Blocking issues

None.

Non-blocking concerns

None.

Missing tests

None — this push adds exactly the test the prior review identified as missing.

Risk assessment (LOW)

This is a test-only change locking in already-shipped, already-reviewed behavior. No production code changed since the prior APPROVED review. The addition is verified correct against the current implementation and the full test/type-check/lint suite passes.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/dashboard
Run ID	33920780671
Cache	hit
Session	ses_f9fb681bbffeMbK9syxEidtDUZ