Manual hyp detach retracts its attach marker so rejoin re-attaches (#217)

[philcunliffe]

Problem

After a manual hyp detach <client>, a subsequent hyp join does not re-attach that client. It stays [configured, not attached] with no rejoin recovery, while a sibling client that was never detached remains attached — the divergence is visible side by side in hyp status.

Root cause (confirms the issue's diagnosis): manual hyp detach reverses the client's on-disk settings but leaves an orphaned status: "done" attach marker in client-actions.json. The action reconciler is level-triggered against that marker, so the next join's forward gap short-circuits on done and never calls perform(). The asymmetry: the daemon reconciler's reverse() deletes the marker (config-drop is rejoin-recoverable), but the manual CLI detach (detachClientViaCore → detachClientFromDisk) reversed disk only and never touched the marker store, so marker and disk drifted.

Fix

Make manual hyp detach retract the client's attach marker after a successful disk reversal, mirroring the daemon reverse() path so the two call sites of the single core undo cannot drift.

• New clearClientActionMarker({ stateRoot, kind, requestKey }) on action_reconciler.js (the module that owns the marker store) — the write counterpart to readClientActionStatus, atomic (tmp+rename, mode 0600), a no-op when the marker is absent.
• detachClientViaCore calls it after detachClientFromDisk succeeds. It runs even on a no-op reversal (changed: false) so a stale marker over already-clean settings is still cleared, and is best-effort: a marker it cannot retract is a status blemish, not a detach failure (the settings undo already landed).

This is consistent with LLP 0045 §Part 3 — the marker is a self-describing undo record for the one core undo shared by CLI detach and reconciler reverse(); a manual detach that reverses settings must retract the marker so state stays honest. It composes with auto-attach-on-join (no reintroduction of the #212 orphaned-settings or #213 reverse-marker classes) — the reconciler's own reverse path is untouched.

Test

test/core/detach-rejoin-recovery.test.js drives the full join → detach → rejoin cycle end to end: the real reconciler attaches claude + codex (marker done, settings written), the real hyp detach claude command reverses the settings and retracts the marker, and a second reconcile re-attaches claude while the un-detached codex stays attached. It fails on master at the marker-retract assertion (manual detach must retract the orphaned claude attach marker) and passes with the fix.

Gates

• npm test: 1670 pass, 0 fail, 1 pre-existing skip (adds 1 test)
• npm run build:types (tsc): clean
• npm run typecheck (includes test/): clean

Fixes #217

[philcunliffe]

Dual-agent review — request_changes

• Verdict: request_changes
• Risk class: medium
• Auto-merge advisory: 👎 thumbs down — verdict is request_changes; needs human-gated follow-up

Advisory only: no merge was attempted. Reviewed head 19316f7. The fix itself is validated correct (settings + marker now converge with the daemon reverse path); both majors are hardening/edge concerns, not defects in the happy path.

Risk capstone

Cross-reference: reviewer findings vs high-risk surfaces

Source	Finding (severity, evidence)	Intersects
Codex	major — concurrent RMW resurrects marker (action_reconciler.js:66,248,359,371)	Concurrency surface / Risks #1
Claude	major — probe-less no-op clears marker, re-orphans settings (core_commands.js:3685)	Risks #2
Claude	minor — missing unit coverage for no-op/emptied-bucket (action_reconciler.js:356-373)	Risks #3
Claude	nit — no coverage for best-effort/changed:false paths (core_commands.js:3684-3697)	Risks #3
Claude	nit — em dashes U+2014 added (test/core/detach-rejoin-recovery.test.js:26)	(style; out of high-risk surface)

Codex review

Fix Validations

Manual detach leaves stale attach marker, blocking rejoin

• Status: correct
• Evidence: src/core/cli/core_commands.js:3663, src/core/cli/core_commands.js:3685, src/core/config/action_reconciler.js:210, src/core/config/action_reconciler.js:212, test/core/detach-rejoin-recovery.test.js:194, test/core/detach-rejoin-recovery.test.js:211, test/core/detach-rejoin-recovery.test.js:223
• Assessment: The CLI now clears the same attach marker after successful disk detach that the reconciler reverse path already deletes after its own undo. The regression test covers join, manual detach, marker removal, and rejoin re-attach.

Findings

4) Concurrency, Ordering & State Safety

• Severity: major
• Confidence: medium
• Evidence: src/core/config/action_reconciler.js:101, src/core/config/action_reconciler.js:248, src/core/config/action_reconciler.js:66, src/core/config/action_reconciler.js:70, src/core/config/action_reconciler.js:359, src/core/config/action_reconciler.js:371, src/core/config/action_reconciler.js:372
• Why it matters: clearClientActionMarker() adds a second process that does read-modify-write over the whole marker file, so a daemon reconcile that read an older snapshot can later rewrite client-actions.json and resurrect the stale attach.claude marker this PR just cleared.
• Suggested fix: Put all marker-store writers behind the same cross-process lock or retrying compare-and-merge path, and use it from both writeStore() and clearClientActionMarker().

No Finding

1. Behavioral Correctness
2. Contract & Interface Fidelity
3. Change Impact / Blast Radius
4. Error Handling & Resilience
5. Security Surface
6. Resource Lifecycle & Cleanup
7. Release Safety
8. Test Evidence Quality
9. Architectural Consistency
10. Debuggability & Operability

Evidence Bundle

• Changed hot paths: hyp detach via runClientLifecycle() to detachClientViaCore(); config-control/client-actions.json marker read/write; action reconciler forward and reverse gaps.
• Impacted callers: src/core/cli/core_commands.js:3474, src/core/cli/core_commands.js:3493, src/core/cli/core_commands.js:3685; src/core/config/action_attach.js:107, src/core/config/action_attach.js:205.
• Impacted tests: test/core/detach-rejoin-recovery.test.js:161; existing reverse coverage at test/core/action-reconciler.test.js:330; status marker contract at test/core/status-client-actions.test.js:293.
• Unresolved uncertainty: I did not run the suite; this review is based on the supplied diff plus targeted caller and contract tracing.

Claude review

Claude review

CLI detach clears the attach marker on a probe-less no-op, re-orphaning settings (#212 asymmetry)

• Severity: major
• Confidence: 84
• Evidence: src/core/cli/core_commands.js:3685
• Why it matters: clearClientActionMarker runs unconditionally after detachClientFromDisk, explicitly "even on changed:false". For a probe-less descriptor detachClientFromDisk returns {changed:false} meaning "cannot reverse — no probe" (client_detach_disk.js:87), with the settings attach() wrote still on disk; the CLI conflates that with "already clean" and drops the marker — the exact invariant client attach: probe-less contributes.client can attach but reverse() silently no-ops, orphaning settings #212/Fix probe-less client attach/reverse asymmetry that orphans settings #213 hardened reverse() against (action_attach.js:212-221 returns failed and KEEPS the marker for a probe-less client). The PR comment claims it retracts "exactly as the reconciler's reverse() does," but reverse() deliberately does NOT retract in the probe-less case. Reachability is low today (no shipped client is probe-less; the reconciler never writes a probe-less marker post-client attach: probe-less contributes.client can attach but reverse() silently no-ops, orphaning settings #212, so it needs a stale/out-of-band marker), so this is a latent asymmetry rather than a live bug — but it is cheap to close and is precisely the invariant the team invested two prior issues in.
• Suggested fix: Mirror the reverse() guard: gate the clearClientActionMarker call on descriptor.attachProbe being present (skip retraction for a probe-less descriptor so an out-of-band marker survives, exactly as reverse() keeps it), and correct the "exactly as reverse() does" comment to note the probe-less exception.

Missing unit coverage for clearClientActionMarker no-op and emptied-bucket branches

• Severity: minor
• Confidence: 88
• Evidence: src/core/config/action_reconciler.js:356-373
• Why it matters: The only test (detach-rejoin-recovery.test.js) exercises solely the happy branch (delete one key; the bucket still holds codex so it never empties). The no-op return false guard (line 361) and the emptied-bucket deletion delete store[kind] (line 367) are never hit, so a regression that (e.g.) creates/rewrites the file on a missing key, or fails to drop the last-key bucket, would still pass CI.
• Suggested fix: Add a small unit test for clearClientActionMarker: missing file/bucket/key each return false and write no file; last-key removal drops the bucket while a sibling key is preserved; a truthy return rewrites atomically (mode 0600).

No coverage for the best-effort marker-retract failure and changed:false paths

• Severity: nit
• Confidence: 85
• Evidence: src/core/cli/core_commands.js:3684-3697
• Why it matters: The comment promises "a marker we cannot retract is a status blemish, not a detach failure" and "runs even on a no-op reversal (changed:false)", but nothing proves detach still returns 0 (and warns) when clearClientActionMarker throws, nor that the marker is cleared on an already-clean (changed:false) reversal.
• Suggested fix: Add a focused test where marker retraction throws (assert detach still succeeds with the warning emitted) and where settings are already clean (changed:false, assert the marker is still cleared).

New code adds em dashes (U+2014), banned by AGENTS.md

• Severity: nit
• Confidence: 82
• Evidence: test/core/detach-rejoin-recovery.test.js:26 (also the two diff-added @ref glosses at action_reconciler.js:354 and core_commands.js:3683)
• Why it matters: AGENTS.md:38 states "No em dashes (the U+2014 character) anywhere: code, comments, JSDoc, strings, or docs"; the diff adds 3. The two @ref glosses follow the em-dash @ref format that CLAUDE.md itself prescribes and the codebase uses pervasively (so they are defensible), leaving the test-file narrative JSDoc em dash as the clean, unexempted violation.
• Suggested fix: Replace the em dash(es) on the added test-prose line(s) with the punctuation the guidance prescribes (colon/comma/parentheses/sentence split); optionally reconcile the CLAUDE.md @ref format vs the AGENTS.md ban separately.

---

Reports: /Users/phil/workspace/hypaware/.git/worktrees/tmp.UZCN0Zueou/dual-review/pr-223

[philcunliffe]

🤖 neutral: reviewed (2 rounds), green, mergeable — held for your merge

Fixes #217 (manual hyp detach left an orphaned attach marker → rejoin never re-attached). Driven to held:

• Round 1 dual-review confirmed the convergence is correct (CLI detach now leaves the same state as the daemon reverse(): settings reversed AND marker retracted) and raised two majors, now resolved:
  1. client attach: probe-less contributes.client can attach but reverse() silently no-ops, orphaning settings #212 invariant preserved (fixed): the marker retraction now gates on descriptor.attachProbe. A probe-less detach returns changed:false meaning "cannot reverse" — so, exactly like reverse(), it now KEEPS the marker instead of dropping it (which would have orphaned the settings, re-opening the client attach: probe-less contributes.client can attach but reverse() silently no-ops, orphaning settings #212 class). Mutation-verified.
  2. Cross-process concurrency (deferred → client-actions marker store: lockless second writer (CLI detach) can race the daemon reconciler #224): the marker store gains a second writer (CLI detach) with no shared lock; a racing daemon reconcile could resurrect a cleared marker. The reviewer flagged this as follow-up-not-blocker; tracked as client-actions marker store: lockless second writer (CLI detach) can race the daemon reconciler #224.
• Added the missing-branch tests (clearClientActionMarker no-op / emptied-bucket), the best-effort + changed:false semantics tests, and removed an em-dash (AGENTS.md).
• State: head a61e0cd, MERGEABLE/CLEAN, CI green (1675 pass), tsc clean.

Merge is yours — neutral never merges. Held.