Fix /buddy off not syncing across concurrent dreb instances

[m-aebrer]

Closes #302

Make the buddy hidden flag the authoritative, live source of truth so that /buddy off (and re-enable via /buddy) converges across all concurrently-running dreb instances within one interaction cycle — instead of only taking effect at the next process start.

PR-248 fixed the TUI startup mount gate, but the flag is never re-read during a live session, so a second concurrent instance keeps showing/reacting with the buddy. The reporter runs multiple long-lived instances, which is exactly the condition that surfaces this.

Implementation plan posted as a comment below.

[m-aebrer]

Implementation Plan

Problem analysis

/buddy off writes "hidden": true to the shared ~/.dreb/agent/buddy.json, but the flag is only ever read at three sites, and only one of them works:

1. TUI startup — mountExistingBuddyIfVisible() gates the visual mount on hidden. This is the PR-248 fix and it is correct.
2. Telegram controller creation — start() sets enabled = !hidden once.
3. BuddyController.reset() — intended to re-sync, but does getState() ?? load(), so the stale in-memory state always wins and the fresh disk read never runs.

Crucially, no running process ever re-reads hidden during a live session — there is no watcher, timer, or re-load(). Each process caches buddy state at init. The reporter runs multiple concurrent dreb instances and keeps long-lived sessions (rarely restarting), so:

• The startup gate (site 1) almost never fires for her.
• Turning the buddy off in instance A leaves instance B (which mounted while visible) showing/reacting with the buddy indefinitely.
• Only deleting buddy.json fixes it, because that affects every instance's next start.

This is the same root cause as the Telegram reproduction (TUI /buddy off didn't stop Telegram reactions until /buddy off was run there too).

Deliverable

Make the persisted hidden flag the authoritative, live source of truth so that /buddy off (and re-enable via /buddy) converges across all concurrent instances within one interaction cycle — no restart, no manual file deletion.

Approach (event-driven re-sync — no watchers/timers)

Re-read the hidden flag from disk at the natural, low-frequency sync points that already exist, and react to transitions. Disk reads stay off the hot path (they happen at turn granularity, not per-token/per-tool).

1. BuddyManager: add a cheap fresh-read accessor, e.g. isHidden(): boolean that does a loadStored() and returns stored?.hidden ?? false (and reports absence so a deleted file can be treated as "gone").
2. BuddyController:
   • Add private syncHiddenState() that reads manager.isHidden() fresh, updates this.enabled = !hidden, and — on a visibility transition — invokes a new optional callback onVisibilityChange?(visible: boolean).
   • Call syncHiddenState() at: agent_end in handleEvent(), processUserMessage(), and inside reset().
   • Fix reset() to consult the fresh disk read rather than stale getState().
3. TUI (interactive-mode.ts): implement onVisibilityChange — false → removeBuddy(), true → load + mountBuddy(). This makes another instance's /buddy off unmount the widget on the next user turn / response, and re-enable re-mount it.
4. Telegram (handlers/buddy.ts): reaction suppression already flows from enabled; provide onVisibilityChange (no-op is acceptable since Telegram has no persistent widget) to satisfy the interface.

onVisibilityChange is optional on BuddyCallbacks to avoid breaking the interface contract.

Files to create or modify

• packages/coding-agent/src/core/buddy/buddy-manager.ts — add isHidden() fresh-read accessor.
• packages/coding-agent/src/core/buddy/buddy-controller.ts — syncHiddenState(), call at sync points, fix reset(), add optional onVisibilityChange to BuddyCallbacks.
• packages/coding-agent/src/modes/interactive/interactive-mode.ts — wire onVisibilityChange to mount/remove.
• packages/telegram/src/handlers/buddy.ts — provide onVisibilityChange.
• packages/coding-agent/test/buddy-controller.test.ts — cross-instance regression tests.
• packages/coding-agent/docs/buddy.md and root README.md / packages/coding-agent/README.md — update the /buddy off description (it now syncs across concurrent instances, not just on restart).

Testing approach

Mandatory regression coverage in buddy-controller.test.ts (uses the existing writeStoredBuddy / createTestController helpers against a temp agent dir):

• Cross-instance off: controller A runs /buddy off (writes hidden:true); controller B is already started/enabled. After a sync trigger (handleEvent agent_end, processUserMessage, or reset), assert B.enabled === false, onVisibilityChange(false) fired once, and canReact() / triggerReaction() no-op.
• Cross-instance re-enable: flip hidden:false externally → after sync, enabled === true, onVisibilityChange(true) fired.
• Transition-only firing: onVisibilityChange fires on change, not on every sync.
• reset() reads fresh disk (extends the existing "keep buddy disabled after reset" test to a cross-instance variant).
• Deleted file edge: buddy.json removed externally → isHidden()/sync treats buddy as gone and fires onVisibilityChange(false).
• BuddyManager.isHidden() unit test (true / false / missing-file).

Risks and open questions

• Idle convergence: event-driven sync converges on the next interaction, not instantly while a process sits idle. Considered a file watcher (fs.watch on buddy.json) for instant convergence but rejected for now — it adds watcher lifecycle + cross-platform flakiness for a cosmetic feature, and "updates on next interaction" is acceptable. Flag for review if instant idle un-mount is desired.
• Disk read frequency: bounded to turn-level events; negligible. Will avoid adding the read to tool_execution_end (the frequent event) — reactions there are error-only and already throttled.
• Interface change: onVisibilityChange kept optional so no external BuddyCallbacks consumer breaks.

---

Plan created by mach6

[github-actions]

Vitest coverage

Metric	Covered	Total	Coverage
Statements	18179	34422	52.81%
Branches	9871	21325	46.28%
Functions	2859	5401	52.93%
Lines	16061	30618	52.45%

View full coverage run

[m-aebrer]

Progress Update

Implemented the plan. The buddy hidden flag is now the live, authoritative source of truth — each instance re-reads the shared buddy.json at turn boundaries instead of caching it for the process lifetime, so /buddy off (and re-enable) converges across concurrently-running instances.

Changes

• buddy-manager.ts — added isHidden(), a fresh-from-disk accessor that bypasses the in-memory cache.
• buddy-controller.ts — added syncHiddenState() (re-reads disk, converges enabled, fires onVisibilityChange only on a transition), wired into agent_end, processUserMessage, and reset(); fixed reset() to load() fresh instead of preferring stale getState(); added the optional onVisibilityChange callback and a public refreshVisibility() sync point; kept lastVisible authoritative across in-process commands.
• interactive-mode.ts — TUI mounts/unmounts the widget via onVisibilityChange, and calls refreshVisibility() on each user message so another instance's /buddy off removes the widget as the user resumes.
• telegram/handlers/buddy.ts — explicit no-op onVisibilityChange (reaction suppression already flows from enabled; Telegram has no widget).
• docs/buddy.md — corrected the /buddy off description and added a "Persistence & multiple instances" section (documents the next-interaction, not-idle, convergence tradeoff).

Tests — 6 cross-instance regression tests (stop reacting on agent_end / on user message, re-enable propagation, transition-only firing, reset() picks up an externally-written flag, deleted-file → unmount) plus 4 isHidden() unit tests. Updated one pre-existing test whose artificial enabled = false (disk still visible) state is now correctly overridden by disk-authoritative sync.

Verification — biome clean, full build clean, entire suite green (pre-commit hook: 3974 passed). Maps to acceptance criteria 1–6 in the scope definition.

Commit: abec3bd

---

Progress tracked by mach6

[m-aebrer]

Code Review

Critical

None.

Important

1. onVisibilityChange callback runs unguarded — a throw can silently drop the user's message and skip post-turn rendering (error-auditor, confidence 84)
buddy-controller.ts:562 invokes this.callbacks.onVisibilityChange?.(visible) with no try/catch. In the TUI this callback runs arbitrary host rendering (new BuddyComponent, mountBuddy/removeBuddy → renderWidgets, ui.requestRender). Two unprotected trigger paths:

• User-message path: refreshVisibility() → syncHiddenState() is the first line of onSubmit (interactive-mode.ts:2465). A throw aborts everything below it in the same handler — including the actual session.prompt(text) submission. The editor invokes onSubmit fire-and-forget (no await/catch), so this becomes an unhandled rejection and a silently swallowed user message: user presses Enter, prompt never reaches the agent, no error shown.
• agent_end path: throw at interactive-mode.ts:2723 skips the post-turn refreshDailyCost() and ui.requestRender() (2725-2726), also as a swallowed rejection.

A cosmetic companion should never be able to take down message submission. Recommend wrapping the onVisibilityChange invocation (or the whole syncHiddenState body) in try/catch with a debug log.

Suggestions

2. lastVisible is committed before load()/mount succeeds — a failed post-transition read can latch an invisible-but-active buddy (error-auditor, confidence 80)
buddy-controller.ts:554 sets this.lastVisible = visible before this.manager.load() (558) and the host mount (562). If another instance hatches the buddy and this instance's load() returns null (parse failure / truncated concurrent read), no widget mounts — but enabled and lastVisible are now true, so subsequent turns short-circuit at the equality check and onVisibilityChange never fires again. Result: a buddy that reacts/consumes Ollama calls but stays permanently invisible until a manual /buddy off+/buddy. Suggest committing lastVisible only after the refresh/mount actually succeeds.

3. Non-atomic writeFileSync + new per-turn reads → concurrent-write race can make the buddy spuriously vanish (error-auditor, confidence 82)
buddy-manager.ts:167 (saveStored) writes with a bare writeFileSync (truncate-then-write, no temp+rename). This PR adds loadStored() reads at every turn boundary across concurrent instances. A reader hitting the write window reads a truncated file → JSON.parse throws → loadStored returns null → syncHiddenState reads it as "no buddy" → unmount + disable, re-mounting next clean turn. This is exactly the multi-instance scenario the PR targets. Also: syncHiddenState() issues two separate reads back-to-back (hasStoredBuddy() then isHidden()), so it can observe an inconsistent exists/hidden pair. Suggest making saveStored atomic (temp file + renameSync) and collapsing the two reads into one.

4. refreshVisibility() — the TUI's actual sync point — is never directly tested (test-reviewer, confidence 88)
The cross-instance tests drive convergence through processUserMessage, handleEvent('agent_end'), and reset(), but the docstring at buddy-controller.ts:564-571 explicitly states the TUI does not route through processUserMessage — it calls refreshVisibility() instead. That makes refreshVisibility() the real integration point for the primary frontend, yet there's zero direct test (no reference anywhere in the suite). If its delegation breaks, the whole TUI cross-instance fix silently stops working while every existing test stays green.

5. Re-enable path doesn't verify the in-memory buddy state is refreshed from disk (test-reviewer, confidence 85)
On a transition to visible, syncHiddenState() calls this.manager.load() (line 558) specifically so a buddy rerolled/changed elsewhere is picked up — the substance of acceptance criterion 2. The re-enable test only asserts enabled === true and onVisibilityChange(true); deleting the load() call would leave both passing. Suggest rerolling/rewriting buddy.json in instance A (different name), re-enabling, then asserting instance B's loaded state shows the new name.

6. Same-instance no-double-fire after local /buddy off is untested (test-reviewer, confidence 82)
handleCommand("off") defensively sets lastVisible = false (line 415) so the same instance's next syncHiddenState() doesn't re-fire onVisibilityChange. No test covers this — the cross-instance tests assert the callback only on the observing instance B; instance A's spy is created but never asserted. Suggest: one instance with a spy, start(), handleCommand("off"), then agent_end, assert the callback fired at most once.

7. refreshVisibility() is a pure one-line alias of syncHiddenState() (simplifier, confidence 80)
buddy-controller.ts:572-574 forwards verbatim to the private syncHiddenState() (line 547). The only reason for the split is the public/private boundary (internal callers use syncHiddenState, the TUI uses refreshVisibility). Could collapse into a single public method with all callers using one name. Judgment call — leave as-is if the public/private naming boundary is intentional.

8. Three identical "make visible" blocks in handleCommand could be one helper (simplifier, confidence 82)
The exact three statements (enabled = true; manager.setHidden(false); lastVisible = true) appear verbatim in the reroll, "already showing", and "load existing" cases (buddy-controller.ts:396-398, 429-431, 438-440). Extracting a private markVisible() helper preserves behavior exactly. Note: the hatch case (449-450) intentionally omits setHidden(false) and must stay separate.

Strengths

• State-machine correctness verified (code-reviewer): every combination of in-process commands (off/show/reroll/hatch set lastVisible directly and skip onVisibilityChange, mounting via the command result) and sync-driven transitions was traced — no double transitions, no spurious unmounts. enabled is reconciled to disk on every sync (self-healing), not just on transitions.
• All six acceptance criteria met (completeness-checker): cross-instance off/re-enable, fresh-disk reset(), deleted-file unmount, Telegram reaction suppression, and the doc correction are all wired and tested. Criterion 6's "Docs/READMEs" plural was investigated — the restart-implying language lived only in docs/buddy.md; neither README contained it, so no further doc edits were required.
• Disk reads are synchronous within a single syncHiddenState() call, so no JS-level interleaving is possible; the only TOCTOU is external and self-heals.
• enabled and lastVisible are correctly kept separate (simplifier confirmed) — merging would break transition edge-detection.
• Good test coverage of isHidden() (happy/hidden/no-buddy/cross-instance) and the core cross-instance off/re-enable/transition/reset/deleted-file cases.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[m-aebrer]

Review Assessment

Independent verification of the review findings against actual source.

Classifications

Finding	Classification	Reasoning
1: Unguarded onVisibilityChange can silently swallow a user prompt	genuine (high)	Full chain verified: onVisibilityChange?.(visible) at buddy-controller.ts:562 has no try/catch; the TUI callback runs mountBuddy/removeBuddy/requestRender. refreshVisibility() is line 2465 of the async onSubmit, before any session.prompt(). The editor calls onSubmit(result) fire-and-forget (editor.ts:1256, no await/catch) after clearing the text. A sync throw rejects onSubmit, skips session.prompt, message gone with no feedback. Low probability, severe blast radius for a cosmetic widget.
2: lastVisible committed before load()/mount can latch invisible-but-active buddy	genuine (low) — subsumed by finding 3	visible uses the same loadStored() validation as load(), so on the visible branch load() can only return null if the file is truncated/deleted between the two reads — exactly the finding-3 race. The "never retry" latch is real but only triggered by that race and self-heals on next hatch/reroll. Fixing finding 3 removes the trigger.
3: Non-atomic writeFileSync → concurrent-write race makes buddy vanish	genuine (low-medium)	Confirmed saveStored does bare writeFileSync (buddy-manager.ts:167, O_TRUNC). A reader hitting the window gets a partial parse → null → spurious one-turn unmount/remount flicker. Writes infrequent, reads per-turn, so low-frequency — but the repo already has temp+rename (performance-tracker.ts:188-190), making the fix cheap and idiomatic.
4: refreshVisibility() — the TUI's real sync point — never directly tested	genuine (coverage gap)	Grep confirms zero test references. Body is a one-line delegate to the well-tested syncHiddenState, so indirectly covered, but the actual TUI entry point has no direct test.
5: Re-enable path doesn't assert in-memory state refreshed from disk	genuine (coverage gap)	The re-enable test asserts only enabled + onVisibilityChange(true), never the buddy identity. The manager.load() at line 558 could be deleted and all tests still pass — a buddy rerolled in another instance wouldn't be caught.
6: Same-instance no-double-fire after local /buddy off untested	genuine (coverage gap, low)	handleCommand("off") sets lastVisible=false (line 415) to suppress a re-fire; transition tests only cover cross-instance (A→B) firing, none assert instance A doesn't re-fire its own callback on the next sync.
7: refreshVisibility() is a one-line alias of syncHiddenState()	nitpick	Deliberate public wrapper around a private method with a doc comment explaining why (hosts that bypass processUserMessage). Reasonable API boundary; collapsing gains nothing.
8: Three identical "make visible" blocks could be a markVisible() helper	nitpick	Blocks at 396-398/429-431/438-440 are byte-identical; hatch case (449-451) correctly excluded (omits setHidden(false)). A DRY helper is fine but purely stylistic.

Tally: 6 genuine (1 high, 2 medium-ish, 3 coverage gaps), 2 nitpicks, 0 false positives, 0 deferred.

Action Plan

1. (Critical, blocks merge) Guard the visibility callback so a buddy widget throw can never abort message submission. Wrap onVisibilityChange?.(visible) at buddy-controller.ts:562 (or the refreshVisibility() call at interactive-mode.ts:2465) in try/catch with a log.debug. Add a regression test where onVisibilityChange throws and assert the sync path doesn't propagate. (Loud-but-contained: log the buddy error, never swallow the user's prompt.)

2. (Should-fix) Make saveStored atomic — temp file + renameSync, mirroring performance-tracker.ts:188-190. Eliminates the truncated-read race (finding 3) and removes the finding-2 latch trigger in one change.

3. (Ship with PR — do not defer) Close the three coverage gaps, per the repo completeness rule (new testable code, package already has buddy tests):

   • Re-enable refreshes B's in-memory buddy identity from disk (guards manager.load() at line 558) — finding 5.
   • Direct refreshVisibility() test covering a cross-instance transition — finding 4.
   • Instance running its own /buddy off does not re-fire its own onVisibilityChange on next sync — finding 6.

No tracking issue needed — items 1–3 all land in this PR.

Skip: findings 7 and 8 (nitpicks). Finding 2 needs no separate work once item 2 lands.

---

Assessment by mach6

[m-aebrer]

Progress Update

Addressed all five genuine findings from the review assessment (nitpicks 7 and 8 intentionally skipped).

Production fixes

• Finding 1 (critical) — buddy-controller.ts syncHiddenState(): wrapped the onVisibilityChange invocation in try/catch with log.debug (matching the existing triggerReaction/handleNameCall error pattern). A host widget throw can no longer abort the user's message submission via the fire-and-forget onSubmit path — local state still converges to disk before the callback runs.
• Finding 3 — buddy-manager.ts saveStored(): now writes atomically via a temp file (.buddy-<pid>-<ts>.json.tmp) + renameSync, mirroring the existing performance-tracker.ts pattern, with best-effort cleanup on failure. Eliminates the truncated-read race introduced by per-turn reads (and removes the finding-2 latch trigger).

Test coverage gaps closed (buddy-controller.test.ts)

• Finding 4 — direct refreshVisibility() convergence test (the TUI's actual sync point, previously only covered indirectly).
• Finding 5 — re-enable test now asserts instance B's in-memory buddy identity is refreshed from disk (Testbud → Zorp), guarding the manager.load() on the visible transition.
• Finding 6 — same-instance /buddy off no-double-fire test.
• Plus a regression test that a throwing onVisibilityChange does not propagate and state still converges.

Verification — biome clean; full build clean; full suite green (pre-commit hook: 3963 passed, 0 failed). Buddy suite specifically: 151 tests pass.

Commit: 9c43225

---

Progress tracked by mach6

[m-aebrer]

Code Review (round 2 — post-fix state)

Fresh review of the current state after the round-1 fixes landed in 9c43225. The two round-1 production fixes (guarded onVisibilityChange, atomic saveStored) and the three round-1 test-gap closures were independently re-verified as correct and complete. This round surfaces 5 new findings — all at the concurrency/coverage boundaries the controller-level tests can't reach.

Critical

None.

Important

1. InteractiveMode's onVisibilityChange callback — the actual TUI mount/unmount — is untested (test-reviewer, confidence 88)
interactive-mode.ts:324-335 (the callback) and :2465 (the refreshVisibility() wiring) are the entire user-visible payload of this PR for the TUI, yet every onVisibilityChange assertion in the suite is against a bare vi.fn() at the controller level. A regression that swapped the visible/hidden branches, dropped removeBuddy(), omitted requestRender(), or removed the ?? manager.load() fallback (so a buddy hatched elsewhere mounts with null state) would pass the entire suite. Directly testable via the existing interactive-mode-status.test.ts precedent (prototype.<method>.call(fakeThis) with mount/remove/requestRender spies).

2. Cross-process lost-update race can silently clobber a concurrent /buddy off (error-auditor, confidence 82)
The atomic-rename fix makes each individual write atomic but does not serialize the read-modify-write sequences across processes. setHidden (buddy-manager.ts:481-486) and setOllamaModel (453-458) both do loadStored() → mutate one field → saveStored(whole object). If instance B is mid-/buddy model while instance A runs /buddy off, B's later write clobbers A's hidden:true with its stale hidden:false — silently reverting the off on disk, and other instances re-mount on their next sync. This is last-writer-wins on the very file the PR newly designates as the shared cross-instance source of truth. Window is microseconds (synchronous read→write span), so low-probability, but genuine. Mitigation: re-read-merge under a lock, or merge only the changed field at write time.

Suggestions

3. Atomic-write fix only tests the happy-path no-temp case, not failure-atomicity (test-reviewer, confidence 82)
"setHidden saves complete JSON and leaves no temp files behind" verifies the success path, but the point of temp+rename — that a failed write never corrupts the existing buddy.json — is unasserted, and the finally rmSync cleanup on a throwing writeFileSync/renameSync is wholly untested. A regression that wrote directly to the final path (reintroducing torn writes) would still pass. Sketch: mock fs to throw, pre-seed valid buddy.json, call setHidden, assert original content unchanged and no .buddy-*.tmp remains.

4. Two-read TOCTOU window inside syncHiddenState is untested (test-reviewer, confidence 80)
syncHiddenState reads disk twice: hasStoredBuddy() then isHidden(). The "externally deleted" test deletes before the call, so both reads see the same absent state — it never exercises file-present-then-deleted between the two reads, which computes visible=true then load() returns null and fires onVisibilityChange(true) with no backing state. Deterministically testable by mocking the manager methods to return an inconsistent present-then-gone sequence.

5. Orphaned temp files accumulate on hard crash between write and rename (error-auditor, confidence 80)
saveStored's finally cleanup only runs for in-process throws. A SIGKILL/power-loss/OOM in the window between writeFileSync and renameSync leaves a uniquely-named .buddy-<pid>-<ts>.json.tmp behind permanently; nothing ever reclaims them. No correctness impact (only buddy.json is read), purely unbounded cruft in getAgentDir(). A startup sweep of stale .buddy-*.json.tmp would bound it.

Strengths

• State machine verified correct (code-reviewer, no findings): no double-firing (transition guard + lastVisible set before early-return), clean separation of in-process commands (set lastVisible directly, render via command result) vs sync-driven transitions (fire onVisibilityChange), enabled reconciled to disk every sync, correct agent_end ordering (sync before reaction gate).
• Both round-1 fixes re-verified correct (error-auditor): guarded callback runs state mutations before the callback so a throw can't half-converge; atomic write is same-dir (no EXDEV), whole-or-throws, distinct-pid temp names, correct sentinel cleanup.
• All 6 acceptance criteria met, PR-248 startup gate not regressed (completeness-checker): mountExistingBuddyIfVisible() unchanged; both READMEs checked for stale restart-implying language (none found); 151/151 buddy tests pass, build clean.
• No simplification findings (simplifier): atomic-write sentinel pattern, transition guard, and Telegram documented no-op are all idiomatic minimal forms.
• Strong controller-level concurrency coverage: instances A/B, transition-only firing, throw-resilience with log assertion, deletion, reset convergence, identity refresh (Testbud→Zorp).

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[m-aebrer]

Review Assessment (round 2)

Independent verification of the round-2 review findings against actual source.

Classifications

Finding	Classification	Reasoning
1: TUI onVisibilityChange callback untested	genuine (medium)	Confirmed: the callback is an inline arrow closure in the constructor call (interactive-mode.ts:327-336) doing mountBuddy/removeBuddy/requestRender with getState() ?? load() fallback — zero TUI-level coverage. It's the literal payload that makes /buddy off unmount in another instance. The prototype.<method>.call(fakeThis) precedent is real and used for the sibling mountExistingBuddyIfVisible, but applies only after extracting the closure to a named method. Severity medium not high — logic is simple and controller-side firing is exhaustively tested.
2: Cross-process lost-update write race	deferred	Confirmed real: setHidden (481-486) and setOllamaModel (453-458) do full-object read-modify-write, last-writer-wins. But the write pattern is pre-existing — this PR adds per-turn reads, not new writes. Triggered only by two manual commands colliding in a microsecond synchronous window. Out of scope for this PR; track separately, not a merge blocker.
3: Atomic-write failure-atomicity untested	genuine (medium)	Confirmed: the only test ("leaves no temp files behind", buddy-manager.test.ts:189) is pure happy-path. The temp+rename+finally cleanup was added by this PR (9c43225), yet the cleanup-on-throw and original-file-intact properties — the entire reason the code exists — are unasserted. Testable by mocking fs to throw.
4: Two-read TOCTOU in syncHiddenState untested	nitpick	The realistic deletion case is tested (buddy-controller.test.ts:499). Only the sub-microsecond delete-between-the-two-reads interleaving is uncovered, and it self-corrects on the next sync. Better addressed by collapsing the two loadStored() reads into one than by a contrived test.
5: Orphaned .tmp on SIGKILL, no sweep	nitpick	Confirmed no sweep exists, but this is the standard accepted tradeoff of write-temp-then-rename. A SIGKILL in the microsecond write→rename window orphans a tiny cosmetic file. Not a merge blocker.

Tally: 2 genuine (both medium, both test gaps on new code), 1 deferred (pre-existing race), 2 nitpicks, 0 false positives.

Action Plan

Both genuine findings are test gaps on code newly added by this PR, so per the repo completeness rule (test gaps don't auto-defer; the package already has buddy test infra) they ship with the PR.

1. (Highest priority) Finding 3 — add failure-atomicity test for saveStored (buddy-manager.test.ts, near line 189). Mock fs.writeFileSync (and separately renameSync) to throw, drive through setHidden/setOllamaModel, assert the finally cleanup leaves no .buddy-*.tmp and a pre-existing buddy.json is unchanged. Smallest change, exercises the most consequential new logic.

2. Finding 1 — cover the cross-instance visibility callback at the TUI level (interactive-mode.ts:327-336 + interactive-mode-status.test.ts). Extract the inline onVisibilityChange closure into a named method (e.g. syncBuddyWidget(visible)) so the established prototype.<method>.call(fakeThis) precedent applies, then test: visible → mountBuddy (incl. the getState() ?? load() fallback), hidden → removeBuddy, and requestRender fires in both branches. Mirrors the existing mountExistingBuddyIfVisible test.

Deferred (track, do not block merge):

• Finding 2 — pre-existing full-object lost-update race in setHidden/setOllamaModel, not introduced by this PR. Optional follow-up issue: switch to re-read-and-merge under the temp+rename. If combined with the Finding 4 single-read simplification, both races close cheaply — but that's out of this PR's scope.

Skip (nitpicks): Findings 4 and 5. Finding 4 is optionally worth a one-line simplification (collapse the two loadStored() reads in syncHiddenState to one) if already touching that function, but not required for merge.

---

Assessment by mach6

[m-aebrer]

Progress Update

Addressed the two genuine findings from the round-2 review assessment (finding 2 deferred as a pre-existing race; findings 4 and 5 are nitpicks, intentionally skipped).

Finding 1 — TUI visibility-sync callback now covered

• interactive-mode.ts: extracted the inline onVisibilityChange closure into a private syncBuddyWidget(visible) method (pure refactor, identical behavior); the constructor now delegates to it.
• interactive-mode-status.test.ts: 4 new tests via the established prototype.<method>.call(fakeThis) precedent — visible + current state mounts & renders; visible + missing state falls back to manager.load(); visible + no state doesn't mount but still renders; hidden removes the widget & renders. A regression that swapped branches, dropped removeBuddy(), omitted requestRender(), or removed the ?? load() fallback now fails.

Finding 3 — atomic-save failure-atomicity now covered

• buddy-manager.test.ts: added a passthrough vi.mock("fs") (only renameSync overridable) and a regression test that forces renameSync to throw, then asserts setHidden propagates, the pre-existing buddy.json is byte-identical (uncorrupted), and no .buddy-*.tmp is left behind. This pins the safety property the temp+rename change exists to provide.

No production logic changed beyond the extract-method refactor.

Verification — npm run build clean; biome clean; full suite green (pre-commit hook: 3960 passed, 0 failed). Targeted: buddy-manager 49, buddy-controller 103, interactive-mode-status 29 — all pass.

Deferred (tracked, not blocking): finding 2 — pre-existing full-object lost-update race in setHidden/setOllamaModel (not introduced by this PR).

Commit: 22b96c2

---

Progress tracked by mach6