docs: correct concurrency protocol and credential runbook in the editable wiki plan

[marcusrbrown]

Three factual errors in the editable wiki path plan, each verified against primary sources, plus one requirement that contradicts the deployed system.

Concurrency status codes

The plan returned 409 when the branch head moved between validation and commit. That contradicts RFC 9110: a representation changing before commit is a lost validator, not a semantic conflict.

• Missing If-Match → 428
• Supplied but stale or nonmatching → 412
• Representation changed before commit → 412
• 409 only for genuine semantic conflict: page deleted between read and write, or identity migration (node_id moved)

The deeper fix is that the race shouldn't exist. If-Match requires strong comparison, and compare-and-write must be atomic — the Git Data API ref update with an expected parent SHA already provides that. A separate check-then-write sequence recreates exactly the race the validator prevents. Also pinned the edit-snapshot ETag as a strong validator, never W/"...", since weak validators are for cache equivalence and cannot prevent lost updates.

Credential leak response

Revoking a GitHub App private key does not invalidate already-issued installation tokens. The key signs the JWT used to mint tokens; killing it stops new minting while live bearer tokens keep working for up to an hour.

For a leak response that is the difference between contained and contained-in-an-hour. Added the missing step: explicitly revoke active installation tokens via DELETE /installation/token, or uninstall the installation.

Rotation ordering

One sequence was used for both routine rotation and leak response. They need opposite orderings:

• Routine: create new key → deploy consumers → verify minting → revoke old. GitHub permits up to 25 non-expiring keys per App, so overlap is the supported path and avoids a self-inflicted minting outage.
• Leak: revoke immediately and accept the interruption; containment outranks availability.

Session step-up

The plan required writes carry authentication issued within the last 30 minutes, described against the dashboard's 24-hour signed cookie. Both halves misdescribe the deployed system.

Production runs the dashboard in gateway mode, where the gateway operator session is the single auth authority and the dashboard's own cookie is never minted. There, the requirement is not implementable: OperatorSessionInfo is a frozen v1.0.0 contract carrying {operatorId, login, expiresAt}, and expiresAt is the sooner of absolute or idle expiry — so an idle-refreshed session is indistinguishable from a freshly authenticated one and auth age cannot be derived.

Dropped rather than approximated. The gateway's 30-minute idle expiry stands as the freshness bound, which is defensible because a wiki edit is a git commit: reversible, path-allowlisted, gate-validated, attributed, and restricted to a single allowlisted operator, with CSRF, origin binding, and rate limiting already covering session abuse.

Both rejected alternatives are recorded — bumping the frozen contract to add issuedAt, and deriving age from expiresAt minus the absolute TTL. The second is the more tempting and the worse: it hardcodes another repository's constant into a security check that would degrade silently if that constant ever changed.

Also

Reconciliation now classifies outcomes as succeeded / failed / indeterminate, and an indeterminate result is persisted and surfaced rather than silently reported as failure. A matching current state is not proof the write landed — another actor can produce identical content — which is what makes the commit correlation metadata load-bearing rather than belt-and-braces.

Documentation only. pnpm check-types, pnpm lint, and pnpm check:md-links pass; 2846 tests, 3 todo.

[fro-bot]

Four of the five corrections land clean. The HTTP semantics rework is right — If-Match does demand strong comparison (RFC 9110 §13.1.1), a strong ETag is the only validator that can prevent a lost update, and collapsing the branch-head race into 412 instead of 409 is the correct read. The 25-non-expiring-keys-per-App claim, the one-hour token lifetime, and the "key signs the JWT, it does not sign the token" distinction all check out. Splitting routine rotation from leak response into opposite orderings is the kind of correction that only surfaces when someone actually walks the runbook instead of reading it.

But the leak-response remedy you added to replace the wrong one is itself wrong in the same shape: it names a control that doesn't cover the threat it's placed under.

Verdict: CONDITIONAL

Blocking issues

docs/plans/2026-08-29-001-feat-editable-wiki-path-plan.md:348 and :364 — DELETE /installation/token cannot contain a leaked private key.

Both edits prescribe: revoke the key, then "explicitly revoke active installation tokens with DELETE /installation/token, or uninstall the App installation." That or presents two equivalent options. They are not equivalent, and the first one does nothing for this threat model.

DELETE /installation/token revokes the token presented in the Authorization header of that request — the caller's own token, and only that one. There is no enumerate-and-revoke-all endpoint. In the scenario the runbook is written for, the private key leaked; the adversary mints their own installation tokens, which the operator has never seen and cannot present. The API call revokes the writer's own token — good hygiene, zero containment.

You correctly identified that key revocation leaves live bearer tokens valid for up to an hour, then closed that window with a control that can't reach the tokens in question. The operator following this runbook believes they're contained and they are not. That's the exact failure class this PR set out to delete.

The only mechanism that invalidates all outstanding installation tokens is acting on the installation itself. Concretely:

• Prefer suspend over uninstall. A suspended installation's tokens fail immediately, and suspension is reversible without re-consenting permissions or re-establishing the installation ID that secrets.APPLICATION_PRIVATE_KEY consumers depend on. Uninstall is the same containment with a much worse restore path.
• Scope DELETE /installation/token to what it actually does: revoke the writer's own live token during a clean rotation. Move it out of the leak-containment sentence.
• Wire this to the escalation tier you already documented. The paragraph immediately below says installation-level action "takes the entire control plane dark — survey, promotion, reconcile, and every other autonomous loop," and warns that an operator reaching for the bigger hammer will otherwise still believe the narrower reassurance applies. That warning now describes the correct leak response. So the trailing reassurance — "Pulling the writer key pauses operator saves but does not interrupt autonomous survey writes" — is true for routine rotation and misleading for a key leak, because full leak containment does take everything dark. The risk-table row at :364 compresses both scenarios into one cell and attaches the routine-path reassurance to the leak path. Split it, or state the cost explicitly: a leaked writer key forces a choice between a one-hour uncontained window and a control-plane outage. That's the honest tradeoff, and it's the sentence an operator needs at 3am.

Non-blocking concerns

:121, :366 — the atomicity mechanism is described as an API parameter that doesn't exist. "the Git Data API ref update with an expected parent SHA" reads as if PATCH /repos/{owner}/{repo}/git/refs/{ref} accepts an expected-current-value field. It doesn't; it takes sha (the new value) and force. The compare-and-write property comes from force: false fast-forward-only enforcement, with the expected parent encoded in the commit object you built against the observed head. Same guarantee, but a plan being corrected specifically for factual precision shouldn't leave Unit 7b's implementer looking for a parameter that isn't in the schema. Worth naming the failure surface too: a non-fast-forward updateRef comes back 422, and that's what has to map to your 412.

:121 vs. the primitive Unit 1 extracts — the conflict-retry loop is a lost-update engine under the new contract. commitWikiChanges in scripts/wiki-ingest.ts retries updateRef conflicts against the new tip, and scripts/wiki-ingest.test.ts:1153 pins that behavior. For the autonomous survey path that's correct: rebuild against the new tip and land. For an operator write it's exactly the lost update the If-Match precondition exists to prevent — the retry rebases the operator's commit over a concurrent change instead of surfacing 412. Unit 1's description at :93 still advertises "Git Data API commit with conflict retry" as part of what the shared library ships. The plan needs one sentence saying the operator write path either uses a non-retrying variant or re-validates the edit snapshot on every retry attempt. Right now the concurrency contract and the reused primitive disagree, and the primitive wins at runtime.

Requirements trace holds. I checked the origin doc (docs/brainstorms/2026-08-29-editable-wiki-path-requirements.md:76): R7 asks for per-request session revalidation, origin/CSRF binding, a version precondition, and limits, plus fail-closed on expired/revoked sessions. It never asked for recent-auth step-up — that was the plan's own addition. Dropping it breaks no requirement, and the R1–R18 trace at :22-34 stays intact. The step-up removal is fully swept: no residual references anywhere in the repo, and the draft-persistence and test-scenario lines were updated in the same pass rather than left dangling. Good discipline.

No private disclosure. fro-bot/dashboard is a public entry in metadata/repos.yaml; the added DASHBOARD_GATEWAY_OPERATOR_SESSION_ENABLED flag name and the OperatorSessionInfo v1.0.0 shape are config surface, not secrets. Clean.

Missing tests

None required. Documentation-only change to a plan document with no executable surface. The plan's own test-scenario lists at :324 and :338 were updated in lockstep with the contract changes, which is the right place for this coverage to live until Units 7a/7b are implemented.

Risk assessment

MED. No code, no workflow, no dependency, no credential surface changes — the diff cannot break CI or production. The risk is entirely downstream: this document is the operational source of truth for a GitHub App key-leak response, and the corrected leak-containment step names a control that doesn't cover the threat. A plan that is wrong in a known way (the pre-PR 409/step-up text) is less dangerous than one that is wrong while presenting itself as verified against primary sources, because the second one stops getting checked. Fix the containment sentence and this is a clear improvement over what's on main.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/.github
Run ID	33574820851
Cache	hit
Session	ses_fa083a004ffe0PXwUvXj34bby5

[fro-bot]

feeaacc closes all three findings, and the blocking one is fixed at the root rather than patched at the sentence.

Verdict: PASS

Blocking issues

None.

The leak-containment fix (:348, :364) is correct now. DELETE /installation/token is demoted to what it actually is — "routine-rotation hygiene, not leak containment," with the reason stated inline: it revokes only the token in that request's own Authorization header and "cannot enumerate or revoke tokens minted by an adversary." Suspension is named as the containment step with the right justification for preferring it over uninstall: suspended-installation tokens fail immediately, and suspension preserves the installation ID that secrets.APPLICATION_PRIVATE_KEY consumers depend on.

The part I care most about is that you didn't just swap the API call. The trailing reassurance is now scoped — "only during routine rotation; that reassurance does not apply to leak containment" — and the tradeoff is stated in the operator's own terms: "a one-hour uncontained window or a control-plane outage." The old text let an operator carry the key-tier comfort into an installation-tier incident. That's gone from both the planning note and the risk row, which is the second half of the fix and the half that usually gets skipped.

Non-blocking concerns

:364 — the canary cannot run before unsuspending. The risk row ends: "Remove the old mount, deploy the replacement, and rerun canary/tip-identity checks before unsuspending." The canary is a real commit on a disposable branch authenticated with a Fro Bot App installation token (:126). While the installation is suspended every one of those tokens 403s, so the canary cannot pass — or even run — before the unsuspend. Correct order is unsuspend, then canary, then re-enable operator saves. The planning note at :348 already gets this right ("keep operator saves disabled until the canary and tip-identity checks pass" — no unsuspend dependency), so it's only the risk cell that inverted it.

Not blocking: the failure is loud and self-revealing. An operator hits 403 on the first canary call and works out the ordering in under a minute. It's an availability annoyance, not a silent false-containment belief like the one this PR just removed. Worth a one-word amend whenever this file is next touched.

Atomicity mechanism (:121, :366) — resolved and now implementable as written. "build the commit object against the observed head with parents: [observed head SHA], then call the Git Data API ref update with force: false; fast-forward-only enforcement supplies the compare-and-write property." That's the real mechanism, and the 422→412 mapping is spelled out at both sites. An implementer reading :366 can write the code without opening the API schema to look for a parameter that doesn't exist.

Retry contradiction (:93) — resolved. The Unit 1 candidate description now splits the semantics explicitly: conflict retry stays for autonomous survey writes, and "the operator write path must use a non-retrying variant or re-validate the edit snapshot on every retry attempt." That reconciles the concurrency contract with commitWikiChanges, which retries updateRef conflicts against the new tip (scripts/wiki-ingest.test.ts:1153). The contract and the primitive now agree instead of the primitive quietly winning at runtime.

Requirements trace and privacy posture unchanged from my prior pass — R1–R18 coverage intact, step-up removal fully swept, no private-repo disclosure.

Missing tests

None required. Documentation-only change to a plan document with no executable surface. The plan's test-scenario lists at :324 and :338 carry the contract coverage until Units 7a/7b are implemented, and they were updated in lockstep with the semantics changes rather than left to drift.

Risk assessment

LOW. Down from MED. No code, workflow, dependency, or credential surface touched, and the downstream risk that drove the earlier rating — a leak runbook prescribing a control that couldn't reach the threat — is gone. What's left is an ordering slip in a risk-table cell that fails loudly the first time anyone walks it. The plan on this branch is materially more accurate than what's on main: the HTTP semantics are RFC-correct and implementable, the credential runbook distinguishes routine rotation from leak containment with the right ordering and the right blast radius for each, and the concurrency contract no longer contradicts the primitive it reuses.

The runbook now says the uncomfortable thing out loud. A leaked writer key means choosing between an hour of exposure and taking the control plane dark — no third door, no reassuring footnote. Documents that name their own worst case are the ones that still work at 3am.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/.github
Run ID	33591962643
Cache	hit
Session	ses_fa083a004ffe0PXwUvXj34bby5

[fro-bot]

5fe397d fixes the ordering and does one better than the reorder I asked for — it carries the reason inline.

Verdict: PASS

Blocking issues

None.

Non-blocking concerns

Canary ordering (:364) — resolved. The risk row now reads "deploy the replacement, unsuspend, then rerun canary/tip-identity checks before re-enabling operator saves — the canary authenticates with an installation token, so it cannot run while the installation is suspended." Correct sequence, and the trailing clause means the next person to edit this cell knows why the order is load-bearing instead of rediscovering it against a 403. A reordered list decays under editing; a reordered list with its constraint attached doesn't.

One mirror gap, worth a note rather than a change request: :348 never names the unsuspend. The planning note prescribes suspension as the leak-containment step, then its recovery sentence says "Remove the old key from the infra secret mount and writer deployment, deploy the replacement, and keep operator saves disabled until the canary and tip-identity checks pass." That was complete before feeaacc, when suspension wasn't part of the sequence. Now that it is, the prose recovery path is silent on the step that undoes it — an operator working from :348 alone walks into the same 403 the risk row now warns about. The risk row carries the full corrected sequence, so the information exists in the document; it's a symmetry gap between the two places that describe the same runbook, not a wrong instruction. Fold the unsuspend into that sentence next time this file is open.

Everything from my prior two passes stays resolved. Leak containment correctly scoped to installation suspension with DELETE /installation/token demoted to rotation hygiene; routine-vs-leak reassurance split at both sites; compare-and-write described as parents: [observed head SHA] plus force: false with the 422→412 mapping; operator write path separated from the autonomous retry semantics at :93. R1–R18 trace intact, step-up removal fully swept, no private-repo disclosure. Diff is still a single file, +13/-13.

Missing tests

None required. Documentation-only change to a plan document with no executable surface. Contract coverage lives in the plan's own test-scenario lists at :324 and :338, which moved in lockstep with the semantics changes.

Risk assessment

LOW. No code, workflow, dependency, or credential surface touched. Three review passes have converged: the HTTP semantics are RFC-correct and implementable without opening an API schema to look for a parameter that doesn't exist, the credential runbook distinguishes routine rotation from leak containment with the right blast radius and the right ordering for each, and the concurrency contract no longer loses an argument with the primitive it reuses. What remains is one sentence that could say more, in a document that already says it elsewhere.

Three passes to get a runbook to survive being walked instead of read. That's the correct number when the alternative is finding out at 3am.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/.github
Run ID	33592599018
Cache	hit
Session	ses_fa083a004ffe0PXwUvXj34bby5

[fro-bot]

9be4b7f closes the mirror gap. Both places that describe the leak runbook now carry the same sequence and the same constraint.

Verdict: PASS

Blocking issues

None.

Non-blocking concerns

None.

:348 — resolved. The planning note's recovery sentence now reads "deploy the replacement, unsuspend the installation, and keep operator saves disabled until the canary and tip-identity checks pass — the canary authenticates with an installation token, so it cannot run before the unsuspend." That matches the risk row at :364 in both ordering and rationale. Two independent descriptions of the same runbook that agree on the sequence and on why the sequence is load-bearing — an operator entering the document from either the prose path or the risk table gets the same instruction, and neither one degrades into a bare list if someone edits it later.

Everything from the prior three passes stays resolved. Leak containment scoped to installation suspension with DELETE /installation/token correctly demoted to rotation hygiene; routine-vs-leak reassurance split at both sites; compare-and-write specified as parents: [observed head SHA] plus force: false with the 422→412 mapping; operator write path separated from autonomous retry semantics at :93. R1–R18 trace intact, step-up removal fully swept, no private-repo disclosure. Diff unchanged in shape: one file, +13/-13.

Missing tests

None required. Documentation-only change to a plan document with no executable surface. Contract coverage lives in the plan's own test-scenario lists at :324 and :338, which moved in lockstep with the semantics changes.

Risk assessment

LOW. No code, workflow, dependency, or credential surface touched. The document has converged: HTTP semantics that are RFC-correct and implementable without hunting for a parameter that doesn't exist, a credential runbook that separates routine rotation from leak containment with the right blast radius and ordering for each, and a concurrency contract that no longer loses an argument with the primitive it reuses. What's on this branch is strictly more accurate than what's on main, with no residual findings.

Four passes, and each one moved a real defect rather than polishing prose. The last three corrections all shared a shape — a control or a sequence that read fine but failed the moment someone walked it instead of reading it. That's the failure mode plans are built to hide, and this one no longer does.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/.github
Run ID	33593571870
Cache	hit
Session	ses_fa083a004ffe0PXwUvXj34bby5