Fix probe-less client attach/reverse asymmetry that orphans settings

[philcunliffe]

Root cause

A contributes.client descriptor with no attach_probe could be auto-attached on join and marked done, but the disk-driven reverse path needs the probe to undo the on-disk settings. The reverse read the missing probe as a clean no-op and dropped the action marker, leaving the client's settings orphaned with no record an attach ever happened.

The two sides of the contract had diverged:

• src/core/config/action_attach.js desired() named a client as an attach target on enabled plugin + on_join≠false + runtime registry reachable, without gating on descriptor.attachProbe. perform() only needs a live adapter, so a probe-less client could attach and mark done.
• reverse() is disk-driven (LLP 0045 Part 3): it replays the descriptor's attachProbe to strip the managed settings. detachClientFromDisk() opens with if (!probe) return { changed: false } — and { changed: false } is overloaded: it means both "no probe (can't reverse)" and "already clean (nothing to do)". The old reverse() always returned done, so on a config-drop the marker dropped while the settings stayed written.

Net: attach can succeed without a probe, but reverse needs it to undo → orphaned settings, invisible to a later detach.

Fix (both halves of the contract — issue's proposals 1 and 2)

1. desired() requires attachProbe. A descriptor with no probe is skipped, so a probe-less client is never named as an attach target — attach-eligibility now requires reverse-capability.
2. reverse() is honest about the gap. A missing attachProbe for an already-applied marker is treated as a failed (retryable, visible) reverse rather than a marker-dropping done. This keeps any out-of-band marker (manual hyp attach, or a pre-fix marker) recoverable instead of orphaning its settings.

Added @ref LLP 0045#part-3… annotations on desired() ([constrained-by]) and reverse() explaining why the probe is required, since Part 3 already establishes reverse depends on the attachProbe.

Regression test

test/core/action-attach.test.js, two new tests (a probe-less PROBELESS_DESCRIPTOR fixture):

• desired() excludes a probe-less descriptor … — failed on master (desired() named probeless), passes here ([]).
• reverse() of a probe-less descriptor fails … — failed on master ('done' !== 'failed'), passes here (failed, reason mentions attach_probe, undo not consulted).

Verified failing→passing by running the two tests against the unfixed origin/master source (both ✖) then the fixed source (both ✔).

Both shipped adapters (claude/codex) declare a probe, so their behavior is unchanged — this closes the latent gap #179's converged review flagged. Full suite green: 1564 pass, 1 skipped, 0 fail; npm run lint clean.

Fixes #212

[philcunliffe]

Dual-agent review — approve

• Verdict: approve
• Risk class: low
• Auto-merge advisory: 👎 thumbs down — requires human merge gate

Advisory only: no merge was attempted.

Risk capstone

Cross-reference: reviewer findings vs high-risk surfaces

Source	Finding (severity, evidence)	Intersects
Codex	Test Evidence Quality - no reconciler-level regression that the applied marker survives (minor, action_reconciler.js:211,222)	Direct callers (reconciler keep-marker-on-failed path)
Claude	Added lines introduce U+2014 em dashes, banned by branch Code Style (minor, action_attach.js:219, test titles :222,:449)	Targets (changed files)

Codex review

Fix Validations

Probe-less descriptor auto-attach

• Status: correct
• Evidence: src/core/config/action_attach.js:91, src/core/config/action_attach.js:103, test/core/action-attach.test.js:222
• Assessment: desired() now excludes descriptors without attachProbe before naming an attach target, which closes the auto-attach half of the asymmetry.

Probe-less reverse dropping an applied marker

• Status: correct
• Evidence: src/core/config/action_attach.js:212, src/core/config/action_attach.js:217, src/core/config/action_reconciler.js:211, src/core/config/action_reconciler.js:222, test/core/action-attach.test.js:449
• Assessment: The new failed outcome is the right signal. The reconciler deletes markers only on done; failed reverse paths are logged and left in place for retry.

Existing handler check

• Status: correct
• Evidence: src/core/config/client_detach_disk.js:85, src/core/config/client_detach_disk.js:87, src/core/config/action_attach.js:250
• Assessment: Existing disk detach could not have fixed this by itself: no probe returned { changed: false }, and reverse() treated all no-op detach results as successful marker drops.

Manual probe-less attach recovery

• Status: incomplete
• Evidence: src/core/cli/core_commands.js:3578, src/core/cli/core_commands.js:3585, src/core/config/client_detach_disk.js:85, src/core/config/client_detach_disk.js:87
• Assessment: The daemon action path is fixed, but manual hyp attach still dispatches to the live adapter without an attachProbe gate, while manual detach still no-ops without a probe. If manual probe-less attach recovery is part of the intended claim, it is not covered by this diff.

Findings

9) Test Evidence Quality

• Severity: minor
• Confidence: medium
• Evidence: test/core/action-attach.test.js:459, test/core/action-attach.test.js:463, src/core/config/action_reconciler.js:211, src/core/config/action_reconciler.js:222
• Why it matters: The new regression verifies reverse() returns failed, but the PR’s key safety claim is that the reconciler keeps the applied marker, and that marker deletion logic lives in a separate caller.
• Suggested fix: Add a reconciler-level regression with a pre-existing attach marker for a probe-less descriptor, then assert reconcile reports reverse failure and the marker remains in the action store.

No Finding

• 1. Behavioral Correctness
• 2. Contract & Interface Fidelity
• 3. Change Impact / Blast Radius
• 4. Concurrency, Ordering & State Safety
• 5. Error Handling & Resilience
• 6. Security Surface
• 7. Resource Lifecycle & Cleanup
• 8. Release Safety
• 10. Architectural Consistency
• 11. Debuggability & Operability

Evidence Bundle

• Changed hot paths: attachHandler.desired() probe gate, attachHandler.reverse() missing-probe failure, direct handler regression tests.
• Impacted callers: daemon registers attachHandler via DEFAULT_ACTION_HANDLERS at src/core/daemon/runtime.js:65; reconciler invokes handler reverse at src/core/config/action_reconciler.js:210; CLI detach shares detachClientFromDisk at src/core/cli/core_commands.js:3654.
• Impacted tests: test/core/action-attach.test.js:222, test/core/action-attach.test.js:449, existing reverse coverage at test/core/action-attach.test.js:389 and test/core/action-attach.test.js:414.
• Unresolved uncertainty: I did not run the suite; this review used the provided diff plus targeted caller and contract traces.

Claude review

Claude review

Added lines introduce U+2014 em dashes (banned by repo Code Style)

• Severity: minor
• Confidence: 85
• Evidence: src/core/config/action_attach.js:219 (runtime reason string); also :61,:72,:101,:194,:203; test/core/action-attach.test.js:222,:449 (test titles), :59,:226,:454
• Why it matters: The branch's own Code Style (CLAUDE.md / AGENTS.md line 38: "No em dashes (the U+2014 character) anywhere: code, comments, JSDoc, strings, or docs") forbids U+2014, and the repo lint (scripts/check-syntax.js, not eslint) does not flag it, so green CI is no defense; these added lines each contain an em dash, most notably the user-visible returned reason at action_attach.js:219 and the two test titles. (Note: the file already uses em dashes pervasively pre-existing, so the PR follows local precedent.)
• Suggested fix: Replace each added U+2014 with the punctuation the rule prescribes - - in the runtime reason string and test titles, and a comma/colon/parenthetical or sentence split in comments and JSDoc.

---

Reports: /Users/phil/workspace/hypaware/.git/worktrees/dual-review-pr-213/dual-review/pr-213

[philcunliffe]

🧭 Decision map — where to spend your attention

Companion to the dual-review verdict. This casts no verdict — it points at the 2 forks where the author made a real choice, so you can skim the rest.

Scanned: ~7 hunks across 2 files. Most is mechanical: expanded JSDoc / inline-comment prose and @ref annotation edits in action_attach.js, plus the PROBELESS_DESCRIPTOR fixture and helper wiring in action-attach.test.js. The behavior change is two guard lines. The decisions worth your eyes, in order:

1. Probe-less reverse() returns failed, not done · unhappy-path policy

src/core/config/action_attach.js:212

if (!descriptor.attachProbe) {
  return {
    status: 'failed',
    reason: `client '${descriptor.name}' has no attach_probe; cannot reverse its on-disk settings ...`,
  }
}

• Decision: short-circuit to failed before the disk undo, so the reconciler keeps the marker.
• Alternative not taken: fall through to detachClientFromDisk, which returns { changed:false } for "no probe" and maps to done — the exact pre-fix path that dropped the marker and orphaned the settings (client attach: probe-less contributes.client can attach but reverse() silently no-ops, orphaning settings #212). Dropping the marker and scrubbing settings was also unavailable (no probe = no recorded location to scrub).
• Check: is "fail visibly and retry every pass" the right steady state for a marker that can never become reversible? The reconciler has no retry cap/backoff (action_reconciler.js:222-235), so a legacy / out-of-band probe-less marker re-logs reverse_failed indefinitely. Confirm that beats a terminal state.

2. Attach-eligibility now requires reverse-capability · default-on policy

src/core/config/action_attach.js:103

if (!descriptor.attachProbe) continue

• Decision: a probe-less client is never named as an attach target, even when its plugin is enabled and default-on. "May we attach?" is coupled to "can we undo?".
• Alternative not taken: keep attaching probe-less clients and rely on reverse to clean up — impossible without a probe, which is the root of client attach: probe-less contributes.client can attach but reverse() silently no-ops, orphaning settings #212.
• Check: that no client meant to auto-attach lacks a probe (today both claude and codex declare attach_probe, so the excluded set is empty — this narrows nothing real now, but it is the guard a future probe-less client would silently hit).

---

Honorable mentions (real but lower-stakes): action_attach.js:217-219 — the failed outcome has no retry cap, so a structurally probe-less marker is an indefinite retry+log loop (a pre-existing reconciler trait now reachable via this branch).

_{Generated by /decision-map. Advisory — directs attention, casts no verdict.}

[philcunliffe]

🤖 neutral: reviewed, green, mergeable — held for human merge gate

Fixes #212 (probe-less contributes.client could auto-attach and mark done, but reverse() silently no-op'd, orphaning client settings). Driven through the reconcilePR rungs to held:

• Review (dual-agent, Codex + Claude): approve · risk low · 0 blocker/major. Production logic (desired() now refuses a probe-less descriptor; reverse() treats a missing probe on an applied marker as a failed — retryable, visible — reverse instead of a marker-dropping no-op) reviewed clean and is unchanged since. Both shipping clients (claude, codex) declare attach_probe, so the new desired() exclusion attaches no real client — no regression.
• 2 minor findings → fixed:
  1. (codex) Added a reconciler-level regression (action-reconciler.test.js) that seeds a pre-existing probe-less attach marker, runs reconcile(), and asserts the reverse reports failed, the disk-undo is never called, and the marker survives — directly testing the marker-survival guarantee. Mutation-verified (deleting the reverse() guard fails exactly this test).
  2. (claude) Stripped em dashes the diff added (runtime reason string, test titles, prose) per the repo's no-em-dash style rule, which CI's syntax check doesn't catch.
• 1 mechanical convention correction (neutral): the minor-fix pass over-applied the no-em-dash rule to the two @ref gloss separators ([relation] — gloss → [relation]: gloss). The em dash is structural @ref syntax (ref-check's documented format; 123:0 the dominant separator across src/), not the prose the rule targets — restored to —. No non-@ref em dash remains on any added line.

State: MERGEABLE / CLEAN, CI green (lint + test + typecheck, full suite 1565 pass / 0 fail), reviewed head 4385b37.

Out of scope (pre-existing, not this diff — noted for follow-up, not fixed here):

• Manual hyp attach / hyp detach CLI path is still ungated for probe-less clients (this PR closes the daemon-action path only).
• A legacy/out-of-band probe-less marker now takes the reverse()→failed branch, which the reconciler re-logs each pass with no cap/backoff (pre-existing reconciler trait; desired() now prevents new probe-less markers).

Merge is a human act — neutral never merges. Held for your call.