fix(roster): keep the empty-write guard armed after a refusal

[gpechieu]

Symptom

A non-empty roster.json was flattened to an empty one after a renderer reload. Live sequence (2026-08-16): the first empty mirror write of the run was correctly refused ([roster] refused to overwrite a non-empty roster with an empty one, declined backup written at 20:38:47) — but a reload re-sent the same empty snapshot and the second write went through, flattening the file two minutes later.

Root cause

The empty-guard set this.wrote = true on the declined branch, so a refusal disarmed the guard: the very next empty write of the run was treated as a legitimate deletion.

Fix

wrote stays false on a refusal and only arms when a NON-empty write lands — i.e. once the renderer has proven it actually holds a roster. After that, an empty write is a real deletion (refusing those would make deletion impossible). The comment block is updated with the real-world case.

Validation

• New regression test: repeated empty writes stay refused; a non-empty write disarms the guard; a later empty write is honored as deletion.
• npm run typecheck clean; npm run test:focused 131/131 pass.

Found running the app as a daily driver for a multi-agent office.

🤖 Generated with Claude Code

[chaitanyagiri]

TL;DR

Thanks @gpechieu — this is a correct, well-scoped one-line fix with a test that genuinely earns its place. I'd merge it.

What it does: RosterStore protects against a known dangerous sequence — a packaged build boots with empty localStorage (different origin), the store mirrors zero agents to disk, and a real roster.json gets flattened. The guard refuses the first empty write of a run when the file on disk is non-empty. The bug: the refusal branch also set this.wrote = true, so refusing once disarmed the guard. A renderer reload re-sent the same empty snapshot and the second write sailed through — which is exactly what happened to you live on 2026-08-16, two minutes apart. The fix keeps wrote false on a refusal; it arms only when a genuinely non-empty write lands.

Verdict: no malicious code, no NSFW, no prompt injection, no dependency changes. Typecheck green, 150/150 tests pass, and I independently confirmed the regression is real. One follow-up worth raising separately about backup retention — not a blocker for this PR.

---

What I did

Checked out the branch, ran typecheck and the full test suite, then re-introduced the one line you deleted (this.wrote = true; on the declined branch) and re-ran your new test to confirm it actually fails against the old behaviour rather than merely passing against the new one.

The state machine, before and after

                  BEFORE (broken)                       AFTER (this PR)

   run start   wrote=false                          wrote=false
       │                                                 │
   empty write  → REFUSED, wrote:=true  ◄── bug      → REFUSED, wrote stays false
       │                                                 │
   reload re-sends the same empty snapshot               │
       │                                                 │
   empty write  → wrote is true → ALLOWED            → REFUSED again ✅
       │              💥 roster flattened                │
       ▼                                                 ▼
                                                 non-empty write → wrote:=true
                                                          │
                                                 empty write → ALLOWED
                                                    (a real deletion) ✅

The invariant you've landed on is the right one: wrote should mean "the renderer has proven it holds a roster", not "we have seen a write". That reading also keeps deletion possible, which the original comment was rightly worried about.

What I tested

Check	Result
npm run typecheck (node + web)	✅ exit 0
node --test test/*.test.cjs	✅ 150/150
test/roster.test.cjs alone	✅ 12/12
Regression genuinely caught	✅ I restored this.wrote = true; on the declined branch and re-ran: not ok 6 - a refusal does not disarm the guard: repeated empty writes stay refused → 11/12. The test fails against the old code and passes against the new, which is what a regression test is supposed to do
Bug still present on main	✅ confirmed — main's roster.ts still sets this.wrote = true in the declined branch
Mergeability	✅ MERGEABLE, no conflicts, despite the branch being 40 commits behind
Malware / obfuscation / network / new deps	✅ none — one deleted line, one comment rewrite, one test
NSFW / profanity / prompt injection	✅ none

The test itself is well constructed: it uses a second storeAt(home) to simulate a fresh run against an existing file (rather than poking at private state), asserts the roster survives both refusals, and then proves the guard still disarms properly so deletion isn't broken. That last assertion is the one most people would forget, and it's the one that stops this fix from turning into a different bug.

One follow-up, separate from this PR

Because the guard no longer disarms on refusal, this.backup(home, p, 'declined') now runs on every refused empty write instead of only the first. That's correct behaviour, and each backup is small — but it points at something worth knowing: RosterStore.backup() has no retention policy at all. It stamps a new file and never prunes.

That isn't theoretical. On the machine I'm reviewing from:

roster-backups/   6,413 files   153 MB

…all from ordinary -write backups, zero declined. So the unbounded growth is pre-existing and this PR barely moves the needle — but a reload loop that repeatedly re-sends an empty snapshot is now a second source, and it'd be sensible to cap the directory (keep the last N, or prune beyond a few days) before that becomes a support ticket. Happy to open that as its own issue rather than widening this PR.

Small notes

• The branch is 40 commits behind main but still merges cleanly, so no rebase is strictly required. Worth pulling anyway so your test runs against the current 258-test suite rather than the 150 it sees today.
• test/roster.test.cjs is already in test:focused, so the new case is covered by the focused run too. Nothing to wire up.
• The rewritten comment block is genuinely good — it records the live date and the two-minute gap, which is the kind of detail that stops someone "simplifying" this back into the bug in six months.

Questions

@gpechieu — one design question, not a change request: the guard is per-RosterStore-instance, so it resets whenever a new store is constructed. Your test relies on that (storeAt(home) twice = two "runs"). Is a renderer reload guaranteed to reuse the same main-process store instance, or can a reload ever produce a fresh one? If it can, the guard re-arms on reload, which is the safe direction — but it'd be worth a sentence in the comment either way.

@chaitanyagiri — want me to open the backup-retention issue (6,413 files / 153 MB with no pruning), or would you rather fold a cap into this PR? My recommendation is a separate issue: this fix is small and correct, and I'd rather it lands as-is than grows.

[gpechieu]

Thanks for the review — and for re-introducing the deleted line to prove the regression test actually bites. That's the check I most wanted a second pair of eyes on.

Your design question, verified against the code: the store is a main-process singleton — src/main/index.ts constructs it once at module scope (const roster = new RosterStore(() => readConfig().harnessHome)). A renderer reload talks to the same instance over IPC, so the guard does not re-arm on reload; its state has the main process's lifetime, and only a fresh app launch re-arms it. That's exactly the shape of the 2026-08-16 incident: one run, two empty writes minutes apart (the reload re-sent the snapshot, the process never restarted). Since the safe-direction question deserved an answer in the code rather than in a PR thread, I've added a sentence to the RosterStore class comment documenting the instance lifetime.

Branch updated: rebased onto current main per your suggestion, so the suite now runs at full width — npm run typecheck clean, 259/259 tests pass locally (main's 258 + the new regression case). No functional change beyond the comment.

On backup retention: +1 to a separate issue, and here's a second datapoint for it: my daily-driver harnessHome sits at 579 files / 2.9 MB after ~2 weeks — nowhere near your 6,413 / 153 MB, but the growth curve is the same and it only points one way. A keep-last-N plus max-age prune on RosterStore.backup() seems right, and I'd be happy to pick that up once this lands, if you open the issue.

[chaitanyagiri]

Shipped in v0.4.6.

Your commit ed8bc84f is in the release. The empty write guard stays armed after a refusal, and the case you added to test/roster.test.cjs ships with it.

This closes rather than shows as merged because the commit went in through the 0.4.6 release branch instead of this PR's branch. The work is yours and it is in the build.

https://github.com/chaitanyagiri/munder-difflin/releases/tag/v0.4.6

Sorry it sat open this long after landing. Thank you.