approvals: a refused save over two overlapping grants rolls back a stale snapshot, so memory and state.json disagree #2782

Implementation: FlyBot (ours) — PR follows within 24h. Do not pick up without a pickup-ok comment on this issue.

Written-by: FlyBot Lead
Claimed-by: FlyBot Lead
Claim-Id: 1b72bc46b1fe4eb4b877cf3b33a49ecf
Branch: dev/3-approvals-serialization
Base: 7b7f6c4
At: 2026-11-10T05:24:54Z

Category: defect — approvals.js rolls a refused save back by restoring a whole-table snapshot taken before the write. With two writes in flight for the same target the snapshot is already stale, so a refused save resurrects a grant the other writer had just removed — or drops one it had just landed. #2764/#2768 fixed the single-writer case (a grant whose own save was refused must not stay in memory); this is the overlapping case that fix cannot reach.

Risk tier: T2. core/src/host/**.

What happens

core/src/host/approvals.js — the grant path snapshots the old list, replaces it, awaits the save, and puts the snapshot back on refusal:

:146   const before = target.standingApprovals
:147   target.standingApprovals = [...grants, grant]
:149   await rt.save()
:151   if (before === undefined) delete target.standingApprovals
:152   else target.standingApprovals = before

removeStandingApproval has the same shape: :168 snapshot, :173 replace, :177 await rt.save(), :179/:180 restore.

core/src/host/store.js explains why that snapshot can be stale: :1014 queued += 1, :1017 const snapshot = JSON.stringify(state, null, 2) (taken synchronously at call time), :1018 const run = writing.then(async () => { (the writes are serialized, the callers are not), :1052 writing = run.catch(() => {}). The write-path helper mutate() at :1076-1080 is await refresh() → await change() → await save() with no mutex, so it is not a fix either.

Sequence that breaks: A and B both await rt.load(); both read before = target.standingApprovals; A replaces and awaits; B's replacement was built from the pre-A list. If B's save is refused (broken write chain, lost lock, temp name that is not a plain file — all reachable, :1022/:1028), B restores its stale before: A's grant is on disk and gone from memory, or a grant that was never written comes back. Memory and state.json then disagree until the next refresh() — and the settings card shows whichever of the two it read.

Delta rollback (remove only grant, re-add only the removed one) is not the fix either: it deletes a grant the other writer already saved.

Reproduction (red first, measured on 6cdffcaf, twice, same shape)

# tests 1
# pass 0
# fail 1
not ok 1 - a refused save rolls back only its own grant, not the one another writer landed
  f1-approvals-rollback.test.mjs:77

Two overlapping always-bot grants for the same Bot, the second save refused on purpose (real refusal from the store, temp name is a directory). Actual memory value after both settle: ["bash:escalate:danger-full-access"] while state.json holds no standing approval — the grant the second writer never landed is live in memory, on the strength of the first writer's snapshot.

Expected

A refused save rolls back only its own change: a grant another writer landed stays, a grant another writer removed stays removed, and memory equals state.json once both writes settle. Correct direction is per-target serialization — one promise chain shared by grantStanding and removeStandingApproval, so the snapshot is always taken inside the chain and never stale.

Change scope (XS)

• core/src/host/approvals.js — one per-target chain, entered by grantStanding (:132-160) and removeStandingApproval (:164-184); no behaviour change when writes do not overlap (byte-identical state).
• core/test/approvals-serialization.test.js (new) — the two readings below; the bench/ probe is not committed.
• patches/ds-bot/0001-core-fixes.patch mirrors approvals.js (+++ b/src/host/approvals.js) — generate + check required after the change.
• changelog.d/ fragment.

Acceptance (red first)

1. Two overlapping always-bot grants for the same Bot, the second save refused → after both settle, memory equals state.json: no grant nobody gave, none lost. On 6cdffcaf: 1 fail / 0 pass.
2. The same two in the other order (refused first), and a removeStandingApproval overlapping a grantStanding of a different kind (both must survive) → same equality. Under the serialization sketch (bench/f1-serialization.patch): 1/1 and 1/1 green, existing group-status / authz / approval-* 91/91 green.

Dedupe (2026-11-10)

1. GitHub repo:ThinkFlowLab/FlyBot standingApprovals → #2764 (closed, fixed by #2768) is the single-writer case (a grant whose own save was refused must not stay in memory). This issue is the overlapping-writer case that fix cannot reach. approvals rollback → #2768 (closed, same fix), #608 / #544 (origin of standing approvals).
2. Local ledger: the F1 finding from the FlyBot Security pass; no open issue or PR on the overlapping-writer face.

Slot: no PR opens until a merge frees a slot (team cap). Issue + claim + red test only until then; the branch is held, not opened.

Written-by: FlyBot Lead
Bot: FlyBot Dev-3
Found by: FlyBot Security (F1)

Second red direction (gated reading, measured on 6cdffcaf, twice)

The same overlap can go the other way, and that one reaches the file: with A's save refused and B's succeeding, the gated probe bench/f1-rollback-overlap.test.mjs:105 — assert.deepEqual(onDisk(host.home), [KIND_2], 'a refused grant reached the file') — is red on the base with the file holding A+B. Reason: store.js:1017 takes the snapshot at call time, before B's change is in state, so A's queued write carries B's grant to disk while only memory is rolled back — a grant the owner never had the save accepted for, on disk.

So the two readings are the two halves of one defect and the per-target chain fixes both: the critical section has to cover the snapshot as well as the save, which is exactly what serialization does and what a delta rollback cannot.

Red first as committed (dev/3-approvals-serialization, tip 3494ca03d36df75a78a23e2072ce9c2c3e63228e)

Single commit, one file core/test/approvals-serialization.test.js, parent 7b7f6c47. On the live main, twice, same shape — # tests 3 / # pass 0 / # fail 3:

1. overlapping grants: a refused save leaves no grant live that the file does not hold — memory ["bash:escalate:danger-full-access"], state.json [] (the refused grant comes back on the other writer's stale snapshot), sameAfterRefusal at :77.
2. overlapping grants in the other order: memory still equals the file — memory ["pwsh"], state.json [].
3. a refused removal: a grant landed in the same window is not dropped — memory ["bash:escalate:danger-full-access"], expected ["bash:escalate:danger-full-access","pwsh"] (a refused removal takes the grant the same window landed).

Under the serialization sketch (not committed): same file 3/3 twice, and 13 existing files group-status / authz / approval-* / standing-approvals at 118/118.

node scripts/ds-bot-patches.mjs check is red on this tip for one expected reason — core/test/approvals-serialization.test.js: committed but not produced by the patches. The mirror is generated in the fix commit, after #2781 releases the generate lane; the PR opens only after that, with check/canonical green on its own tip.