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

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

Manual `hyp detach` reversed a client's on-disk settings but left an
orphaned `status: "done"` attach marker in `client-actions.json`. The
action reconciler is level-triggered against that marker, so the next
`hyp join`'s forward gap short-circuits on `done` and never re-attaches:
detach-via-config-drop was rejoin-recoverable while detach-via-CLI was
not, and the two disagreed side by side in `hyp status`.

Fix: after a successful disk reversal, `detachClientViaCore` now retracts
the client's `attach` marker via a new `clearClientActionMarker` helper on
the reconciler module (which owns the marker store) — the same
`delete markers[requestKey]` the reconciler's `reverse()` does once its
disk undo succeeds. This keeps the single core undo's two call sites (CLI
detach and reconciler reverse) from drifting, honoring LLP 0045 §Part 3
(the marker is a self-describing undo record that must not outlive its own
effect being reversed). The retraction is best-effort and atomic
(tmp+rename, mode 0600), and runs even on a no-op reversal so a stale
marker over already-clean settings is still cleared.

Regression test drives the full join -> detach -> rejoin cycle end to end:
the real reconciler attaches (marker done + settings written), the real
`hyp detach` command reverses settings and retracts the marker, and a
second reconcile re-attaches while an un-detached sibling stays attached.
It fails on master at the marker-retract assertion and passes with the fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Detach marker retract: preserve the #212 probe-less invariant + cover the guard

Dual-review fixes on the #217 fix (PR #223).

Finding 2 (major): detachClientViaCore cleared the attach marker
unconditionally, even on `changed:false`. But `changed:false` is
overloaded: probe-HAVING means "already clean" (safe to clear); probe-LESS
means "cannot reverse, no probe to replay" (#212). The daemon reverse()
KEEPS the marker for a probe-less descriptor; the CLI was dropping it,
re-orphaning the settings the #212/#213 invariant protects. Gate the
clearClientActionMarker call on `descriptor.attachProbe` so the CLI mirrors
reverse(): clear for a probe-having client (changed:true or already-clean),
keep for a probe-less one. Correct the comment (it claimed it behaved
"exactly as reverse() does", which was wrong for the probe-less case).

Finding 3 (minor): unit-test clearClientActionMarker's untested branches -
missing file/bucket/key each return false and write no file; an emptied
bucket is dropped while a sibling `backfill` bucket and its keys survive.

Finding 4 (nit): cover the best-effort + changed:false semantics through the
real CLI detach path - a probe-having client with already-clean settings
still has its stale marker cleared; a probe-less client keeps its marker (the
finding-2 guard, mirroring reverse()); a throwing retraction does not fail
the detach. Mutation check: removing the `attachProbe` guard fails the
probe-less test.

Finding 5 (nit): replace a U+2014 em dash in test prose with a colon
(AGENTS.md bans em dashes; @ref glosses excepted).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: philcunliffe

src/core/cli/core_commands.js

@@ -22,6 +22,7 @@ import { discoverBundledPlugins } from '../runtime/bundled.js'
 import { isWithinDir } from '../runtime/contribution_names.js'
 import { buildPluginCatalog } from '../plugin_catalog.js'
 import { detachClientFromDisk } from '../config/client_detach_disk.js'
+import { clearClientActionMarker } from '../config/action_reconciler.js'
 import { configuredGatewayEndpoint } from '../config/gateway_endpoint.js'
 import { resolveClientSettingsPath } from '../daemon/client_settings_path.js'
 import { collectHypAwareStatus } from '../daemon/status.js'
@@ -3671,6 +3672,38 @@ async function detachClientViaCore({ name, descriptor, dryRun, json, ctx }) {
             changed: true,
           })
         }
+        // Retract the attach marker so the CLI undo and the marker store stay in
+        // sync, mirroring the reconciler's reverse() after its own disk undo,
+        // including reverse()'s probe-less exception. `changed:false` is
+        // overloaded: for a probe-HAVING descriptor it means "settings already
+        // clean" (safe to clear a stale marker over them); for a probe-LESS
+        // descriptor it means "cannot reverse, no probe to replay" (#212). In
+        // that probe-less case reverse() KEEPS the marker (records a failed
+        // reverse) rather than orphaning the settings attach() wrote, so we gate
+        // on `attachProbe` and do the same: only a probe-having client
+        // (changed:true OR already-clean) has its marker cleared; a probe-less
+        // one keeps it. Without this retraction a manual detach reverses the
+        // settings but leaves an orphaned `done` attach marker, and the next
+        // `hyp join`'s forward gap short-circuits on it and never re-attaches the
+        // client (#217). Best-effort: a marker we cannot retract is a status
+        // blemish, not a detach failure (the settings undo already landed).
+        // @ref LLP 0045#part-3--reverse-runs-from-disk-the-marker-is-a-self-describing-undo-record [implements] — manual detach retracts its attach marker via the one core undo's store (probe-less keeps it, like reverse()), so CLI and reconciler reverse cannot drift (#212/#217)
+        if (descriptor.attachProbe) {
+          try {
+            clearClientActionMarker({
+              stateRoot: readObservabilityEnv(ctx.env).stateDir,
+              kind: 'attach',
+              requestKey: name,
+            })
+          } catch (markerErr) {
+            getLogger('cmd-detach').warn('client.detach.marker_retract_failed', {
+              hyp_client: name,
+              hyp_plugin: descriptor.plugin,
+              error_kind: 'marker_retract_failed',
+              detail: markerErr instanceof Error ? markerErr.message : String(markerErr),
+            })
+          }
+        }
         writeCoreDetachOutput({ ctx, name, json, result })
       } catch (err) {
         span.setAttribute('status', 'failed')

src/core/config/action_reconciler.js

@@ -329,3 +329,46 @@ export function readClientActionStatus({ stateRoot }) {
   }
   return { byKind: store }
 }
+
+/**
+ * Retract a single client-action marker (`kind` + `requestKey`) from the store:
+ * the write counterpart to {@link readClientActionStatus}, callable from any
+ * process (the CLI is not the daemon, so it never constructs the reconciler).
+ *
+ * The manual `hyp detach` command calls it after a successful disk reversal so
+ * the CLI undo and the marker store stay in sync, doing the same
+ * `delete markers[requestKey]` the reconciler's `reverse()` does once its own
+ * disk undo succeeds. Without it a manual detach reverses the on-disk settings
+ * but leaves an orphaned `done` attach marker; the next join's forward gap then
+ * short-circuits on that stale marker and never re-attaches the client (#217).
+ * That is why detach-via-config-drop was rejoin-recoverable while detach-via-CLI
+ * was not: this retraction is what makes the single core undo's two call sites
+ * converge instead of drift.
+ *
+ * Atomic (tmp+rename, mode 0600), mirroring `writeStore`. A missing store, kind
+ * bucket, or key is a no-op returning `false`; an emptied bucket is dropped so a
+ * stale empty namespace never lingers, matching `reconcile()`'s own cleanup.
+ *
+ * @param {{ stateRoot: string, kind: string, requestKey: string, now?: () => number }} args
+ * @returns {boolean} whether a marker was found and the store rewritten
+ * @ref LLP 0045#part-3--reverse-runs-from-disk-the-marker-is-a-self-describing-undo-record [implements] — manual detach retracts its attach marker so the single core undo's two call sites (CLI + reconciler reverse) cannot drift; the marker never outlives its own effect being reversed (#217)
+ */
+export function clearClientActionMarker({ stateRoot, kind, requestKey, now = Date.now }) {
+  const controlDir = path.join(stateRoot, CONTROL_DIRNAME)
+  const markerPath = path.join(controlDir, CLIENT_ACTIONS_BASENAME)
+  const store = readMarkerStore(markerPath)
+  const markers = store[kind]
+  if (!markers || !(requestKey in markers)) return false
+  delete markers[requestKey]
+  // Drop an emptied bucket so a no-op namespace never lingers (mirrors reconcile).
+  if (Object.keys(markers).length > 0) {
+    store[kind] = markers
+  } else {
+    delete store[kind]
+  }
+  fs.mkdirSync(controlDir, { recursive: true, mode: 0o700 })
+  const tmp = `${markerPath}.tmp.${process.pid}.${now()}`
+  fs.writeFileSync(tmp, JSON.stringify(store, null, 2) + '\n', { mode: 0o600 })
+  fs.renameSync(tmp, markerPath)
+  return true
+}

test/core/action-reconciler.test.js

@@ -10,13 +10,15 @@ import path from 'node:path'
 import {
   createActionReconciler,
   readClientActionStatus,
+  clearClientActionMarker,
 } from '../../src/core/config/action_reconciler.js'
 import { createAttachHandler } from '../../src/core/config/action_attach.js'
 
 /**
  * @import {
  *   ActionContext,
  *   ActionHandler,
+ *   ActionMarkerStore,
  *   ActionOutcome,
  *   DesiredAction,
  * } from '../../src/core/config/types.d.ts'
@@ -444,3 +446,98 @@ test('a run-once handler never reverses a no-longer-desired done marker', async
     await fsp.rm(tmp, { recursive: true, force: true })
   }
 })
+
+/**
+ * Directly seed the client-actions marker store on disk (bypassing the
+ * reconciler) so `clearClientActionMarker`'s retraction branches can be
+ * exercised in isolation.
+ *
+ * @param {string} stateRoot
+ * @param {ActionMarkerStore} store
+ */
+function seedMarkerFile(stateRoot, store) {
+  const p = markerPath(stateRoot)
+  fs.mkdirSync(path.dirname(p), { recursive: true })
+  fs.writeFileSync(p, JSON.stringify(store, null, 2) + '\n')
+}
+
+test('clearClientActionMarker is a no-op (returns false, writes nothing) when the file, bucket, or key is missing', async () => {
+  const { tmp, stateRoot } = await makeFixture()
+  try {
+    // (a) Missing store file: nothing to retract, and none is created.
+    assert.equal(
+      clearClientActionMarker({ stateRoot, kind: 'attach', requestKey: 'claude' }),
+      false,
+      'missing marker file returns false'
+    )
+    assert.equal(
+      fs.existsSync(markerPath(stateRoot)),
+      false,
+      'no marker file is written for a missing store'
+    )
+
+    // (b) Missing bucket: an `attach` retraction over a store that only has a
+    //     `backfill` bucket leaves the file byte-for-byte unchanged.
+    seedMarkerFile(stateRoot, {
+      backfill: { '@hypaware/claude': { status: 'done', request_key: '@hypaware/claude' } },
+    })
+    const beforeBucket = fs.readFileSync(markerPath(stateRoot), 'utf8')
+    assert.equal(
+      clearClientActionMarker({ stateRoot, kind: 'attach', requestKey: 'claude' }),
+      false,
+      'missing bucket returns false'
+    )
+    assert.equal(
+      fs.readFileSync(markerPath(stateRoot), 'utf8'),
+      beforeBucket,
+      'missing bucket does not rewrite the file'
+    )
+
+    // (c) Missing key: the `attach` bucket exists but names a different client.
+    seedMarkerFile(stateRoot, {
+      attach: { codex: { status: 'done', request_key: 'codex' } },
+    })
+    const beforeKey = fs.readFileSync(markerPath(stateRoot), 'utf8')
+    assert.equal(
+      clearClientActionMarker({ stateRoot, kind: 'attach', requestKey: 'claude' }),
+      false,
+      'missing key returns false'
+    )
+    assert.equal(
+      fs.readFileSync(markerPath(stateRoot), 'utf8'),
+      beforeKey,
+      'missing key does not rewrite the file'
+    )
+  } finally {
+    await fsp.rm(tmp, { recursive: true, force: true })
+  }
+})
+
+test('clearClientActionMarker drops an emptied bucket but preserves sibling buckets and keys', async () => {
+  const { tmp, stateRoot } = await makeFixture()
+  try {
+    // `attach` holds a single key; `backfill` is a sibling bucket with two.
+    seedMarkerFile(stateRoot, {
+      attach: { claude: { status: 'done', request_key: 'claude' } },
+      backfill: {
+        '@hypaware/claude': { status: 'done', request_key: '@hypaware/claude' },
+        '@hypaware/codex': { status: 'done', request_key: '@hypaware/codex' },
+      },
+    })
+
+    assert.equal(
+      clearClientActionMarker({ stateRoot, kind: 'attach', requestKey: 'claude' }),
+      true,
+      'retracting the last attach key returns true'
+    )
+
+    const store = readMarkerFile(stateRoot)
+    // Emptied bucket dropped entirely, not left dangling as an empty `{}`.
+    assert.equal('attach' in store, false, 'the emptied attach bucket is dropped')
+    // The sibling bucket and both its keys survive untouched.
+    assert.equal(store.backfill['@hypaware/claude'].status, 'done', 'sibling backfill key survives')
+    assert.equal(store.backfill['@hypaware/codex'].status, 'done', 'sibling backfill key survives')
+  } finally {
+    await fsp.rm(tmp, { recursive: true, force: true })
+  }
+})