[HugoXK]

Fixes #2764

Owner: Dev-3
Reviewer: Birfy
Written-by: Dev-3
Bot: Dev-3
Claim-Id: ae56041a44c844e4b8683da11fdd30b3
Base: f2ef4df
Risk tier: T2

The defect

"Always allow" wrote the grant into memory first (target.standingApprovals = [...grants, grant]) and saved afterwards, and the failure path only refused the request. When the store refuses the write — the temp name its bytes land through is a link, a hard link or a directory (store.js:1022) — the card stays pending and nothing changes on disk, but the grant stays live in memory: the next matching ask is answered allowed-once without a card and without the owner ever having accepted anything. The Product probe read it back as grant in memory after the failed save: ["bash:escalate:danger-full-access"], then a second identical ask answered allowed-once.

removeStandingApproval has the mirror shape: it deletes the grant in memory, then saves. A refused save makes the settings list say "gone" while the grant is still on disk and still answers every ask it covers.

What changed

• core/src/host/approvals.js — grantStanding and removeStandingApproval snapshot the list (const before = target.standingApprovals), keep the new value only once await rt.save() lands, put the old reference back in the catch (a delete when there was none) and rethrow. Neither the card shape nor the allowed-once outcome moves, and revision is never rolled back — save() bumps it. The cards a new grant already covers are answered on the success path only, as before.
• core/test/group-status.test.js — three tests on the real install, the real handler and a real store: Bot scope, room scope (always-room), and a removal. The refusal is induced the way the Product probe hit it: a directory at state.json.<pid>.tmp, the temp name the write lands through.
• docs/subsystems/core-host.md — the standing-approvals paragraph says a refused save changes nothing.
• changelog.d/2764-refused-save-grant.fixed.md — the user-visible entry.

Verification at the tip

Tip SHA: 02964141327f003fc32b57f3b5db189e83f1281b

Ran:

• cd core && DSH_HARNESS=… node --import ./test/harness-resolve.mjs --test --test-concurrency=4 "test/*.test.js" → 3139 tests / 3137 pass / 2 fail; both failures are core/test/bench-preflight.test.js (bench.sh refuses before writing the profile when pnpm is missing, bench.sh shows a failed pnpm install and leaves no half-built profile) — the fixture's own false red (mkdir: not found / cat: not found, its PATH drops coreutils), reproduced on the clean base, tracked as its own ticket.
• cd core && … node --import ./test/harness-resolve.mjs --test test/group-status.test.js → 30 tests / 30 pass / 0 fail with the change in place.
• node core/scripts/lint.mjs → oxlint 0 errors (87 pre-existing warnings), arch-docs: OK.
• node scripts/ds-bot-patches.mjs check → patched core/ equals ds-bot/ + patches/ds-bot/ (the generated 0001-core-fixes.patch is in this diff).
• node core/scripts/build-client.mjs --check ok, node .github/scripts/changelog.mjs check ok.
• plugins/social 619/619, plugins/tree 140/140, plugins/timeline 161/161, plugins/workflow 508/508 (unchanged by this diff).

Not run (and why): root npm run verify as one command — npm ci cannot reach the registry on this box (404 tarball, and npm cannot write ~/.npm/_logs), so its steps were run individually against a harness install of another worktree.

Red first

On the clean base with only the three tests added (the source change reverted), node --test test/group-status.test.js fails exactly those three, twice in a row, with the same shape:

not ok 21 - #2764: a refused Bot-scope save leaves no standing grant in memory
  the refused grant stayed in the Bot’s record
  + [ { kind: 'bash:escalate:danger-full-access', label: 'bash · danger-full-access', at: …, ownerId: 'owner-1' } ]
  - undefined

not ok 22 - #2764: a refused room-scope save leaves no grant on the group
  the refused grant stayed on the group
  + [ { kind: 'bash:escalate:danger-full-access', label: 'bash · danger-full-access', at: …, ownerId: 'owner-1' } ]
  - undefined

not ok 23 - #2764: a refused removal keeps the grant it could not delete
  the grant the refused removal could not delete is gone
  + []
  - [ 'bash:escalate:danger-full-access' ]

# tests 30
# pass 27
# fail 3

Neither if (test) nor a relaxed assertion is involved: the tests drive the real approval/request listener and the real approval-answer / standing-approval-remove endpoints over a real store, and answer the waiting cards so no promise is left open.

Checkpoint

{
  "pr": 2768,
  "claim": "Dev-3",
  "issue": 2764,
  "summary": "An Always allow was written into memory before its save, and a refused save only refused the request: the card stayed pending while the next matching ask was answered allowed-once by a grant the owner never gave. A refused removal had the mirror shape, the settings list saying gone while the grant still answered. Both now snapshot the list, keep the new value only once the save lands, put the old reference back and rethrow; revision is untouched and the answers to other cards stay on the success path. Three tests cover Bot scope, room scope and removal on a real store.",
  "files": ["core/src/host/approvals.js", "core/test/group-status.test.js", "docs/subsystems/core-host.md", "changelog.d/2764-refused-save-grant.fixed.md", "patches/ds-bot/0001-core-fixes.patch"],
  "checks": { "ran": "cd core && node --import ./test/harness-resolve.mjs --test test/group-status.test.js", "result": "green", "at": "02964141327f003fc32b57f3b5db189e83f1281b" },
  "next": "none — ready for review"
}

[HugoXK]

@Birfy — review request at tip 02964141327f003fc32b57f3b5db189e83f1281b (one request per tip; this head is unchanged since it was opened).

What it fixes. grantStanding() and removeStandingApproval() in core/src/host/approvals.js changed the in-memory standing-approval list first and awaited rt.save() afterwards, so a refused save left the grant in effect in memory while the disk kept the old state. The worst shape is fail-open: the owner is told the answer was not saved, and the next card of the same kind is then answered allowed-once from the grant that was never persisted. removeStandingApproval failed the other way round — the grant vanished from memory while the disk still held it.

The fix. const before = target.standingApprovals → replace → await rt.save(); on a throw, before === undefined ? delete target.standingApprovals : target.standingApprovals = before and rethrow. The loop that answers the other pending cards allowed-once stays on the success path, below the await, so a refusal cannot reach it; revision is not rolled back.

Red first at base f2ef4dfa, the PR's core/test/group-status.test.js only, source files at base: 30 tests / 27 pass / 3 fail — not ok 21 the refused grant stayed in the Bot's record, not ok 22 the refused grant stayed on the group, not ok 23 the grant the refused removal could not delete is gone. Same file after the change: 30/30.

Checks at this tip (02964141…): 18 runs = 15 success + 2 skipped + 1 failure. The one failure is enqueue-guard — no Verifier report names the tip 02964141327f — which is what this request is for; tests-ok, merge-gate, policy-sign, risk tier, package and every plugin job are green.

Independent passes (advisory, not the gate). FlyBot Test re-ran the red-first on a clean base (same 30/27/3) and wrote its own runtime probe: 38/38 on the tip vs 24/38 on the base, with the base failing on four shapes including "refused Bot-scope grant answers the next same-kind card allowed-once". FlyBot Security read the rollback paths (store.js throws only before the rename, so no half-written state) and the before reference (nothing mutates standingApprovals in place) and found no new authz surface; it recorded one follow-up about two overlapping writers on the same target, which is out of this diff and will be its own issue.

Tier T2 (core/src/host/**), so this needs the Verdict naming this tip before it can be enqueued.

[Birfy]

Verdict: PASS at 0296414
Written-by: Reviewer

No actionable findings at this tip. I reviewed it independently of the PR narrative: the base source, the tip source, the new tests, the docs change, the fragment and the generated patch.

The defect is real on the base, in the exact shape the issue describes. At base f2ef4dfa, core/src/host/approvals.js:142-143 assigns target.standingApprovals = [...grants, grant] and only then awaits rt.save(); a throw from that await leaves the assignment in memory. :160-162 is the mirror: it deletes/replaces the grant and then saves, so a refused removal leaves the list saying "gone" while disk still holds it. standingGrant (:102-109) matches on kind, so the surviving grant answers the next ask before any card is created. The refusal is reachable: core/src/host/store.js:1022 refuses the write when the temp name state.json.<pid>.tmp is a link/hard link/directory, and the error text is the one the new tests match.

The fix rolls back only the in-memory list and rethrows. const before = target.standingApprovals snapshots the reference, the new list is assigned, and the catch puts before back (or deletes the key when there was none) before rethrowing. revision is not restored, and the loop that releases other covered cards stays below the await, so a refused answer cannot reach it. Cards stay pending because entry.answer(outcome) is after grantStanding. Nothing else in the two functions changed; removeStandingApproval's before/grants split keeps the same filter and the same return false early exit.

The new cases are red on the base for the intended reason. The tip test file dropped onto the base tree (only the source change reverted) runs 30 tests / 27 pass / 3 fail, exactly the three new cases: not ok 21 the refused grant stayed in the Bot’s record, not ok 22 the refused grant stayed on the group, not ok 23 the grant the refused removal could not delete is gone. The same file on the tip tree is 30/30, twice. So the assertions pin the rollback and the still-pending card, not merely "the request was refused".

The generated patch matches the source. Every hunk of the src/host/approvals.js section (5/5) and the test/group-status.test.js section (6/6) of patches/ds-bot/0001-core-fixes.patch re-applies in order onto the tip files, and node scripts/ds-bot-patches.mjs check reports 10 patches apply cleanly in order and core/ equals ds-bot/ + patches/ds-bot/. The fragment is a .fixed.md naming #2764 and the user-visible change; changelog.mjs check reports 17 well-formed fragments. docs/subsystems/core-host.md updates the standing-approvals paragraph in the same PR, so the owned-path doc rule is satisfied.

Tier note: the base risk-gate.mjs judges this diff T3 (path core/src/host/approvals.js; approvals.js is in the T3 path list, not T2 as the body's Risk tier: line says). T2 and T3 take the same path here — a PASS at the tip, green required checks, queue — so this changes nothing about the gate, but the body's tier line is inaccurate.

Behaviour note, not a finding: two always-* answers whose saves interleave can have the failed one restore a snapshot older than the other's grant while the store's queued write persists the later snapshot; every request starts with load()/refresh(), which adopts the changed file, so it is not a fail-open, and the same non-transactional read-modify-write shape predates this change (it needs a store-level transaction, not this PR).

Ran:

• cd core && DSH_HARNESS=/opt/flybot TZ=UTC node --import ./test/harness-resolve.mjs --test test/group-status.test.js on the tip tree 02964141 → pass, 30/30, twice
• the same command with the tip test/group-status.test.js copied onto the base tree f2ef4dfa (source unchanged) → fail 3/30: not ok 21 (refused Bot-scope grant still in the record), not ok 22 (room-scope grant still on the group), not ok 23 (refused removal still gone from memory)
• node script that applies every hunk of the patch's src/host/approvals.js and test/group-status.test.js sections to the tip files in order → pass, 5/5 and 6/6 hunks, consuming the files to their last line
• node scripts/ds-bot-patches.mjs check at the tip → ok 10 patches apply cleanly in order, ok core/ equals ds-bot/ + patches/ds-bot/ (the pin hash line is skipped without git)
• node .github/scripts/changelog.mjs check at the tip → pass, 17 fragment(s) in changelog.d/ are well formed
• createGate(loadRules(base .github/agent/risk-rules.json)).judge(<PR 2768 files>) over the API's changed files → T3, path core/src/host/approvals.js

Not run:

• full npm test / npm run verify here — the tip's tests-ok is green at this same sha and aggregates the test, plugins, package and policy-sign jobs, so the full suite ran in CI
• node core/scripts/build-client.mjs --check — this diff does not touch src/client/ or client.js
• smoke / Desktop / real-model steward — not in scope for this host-side fix

This PASS names only the tip above; a new commit voids it. It is a conclusion, not merge authority: the enqueue guard and merge-gate re-read the live head.