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

src/core/config/action_attach.js

@@ -57,17 +57,19 @@ export function createAttachHandler(opts = {}) {
      * `ctx.clientDescriptors`, keep each descriptor whose owning `plugin` is
      * enabled in `ctx.config.plugins`, whose entry does not set
      * `attach.on_join: false` (read via `attach_policy.js`, the
-     * `backfill_policy.js` twin), and whose client the runtime registry has
-     * (`ctx.clients.getClient(name)` defined) so it never names a client
-     * `perform()` cannot reach. The owning plugin comes from the descriptor,
-     * not from `listClients()` (which omits it). Daemon-only by construction:
-     * a plain CLI boot has neither `clientDescriptors` nor `clients`, so the
-     * handler stays inert.
+     * `backfill_policy.js` twin), whose descriptor declares an `attachProbe`
+     * (so the effect is reversible, see below), and whose client the runtime
+     * registry has (`ctx.clients.getClient(name)` defined) so it never names a
+     * client `perform()` cannot reach. The owning plugin comes from the
+     * descriptor, not from `listClients()` (which omits it). Daemon-only by
+     * construction: a plain CLI boot has neither `clientDescriptors` nor
+     * `clients`, so the handler stays inert.
      *
      * @param {ActionContext} ctx
      * @returns {DesiredAction[]}
      * @ref LLP 0045#part-2--the-attach-handler-srccoreconfigaction_attachjs [implements] — desired() over clientDescriptors ∩ enabled plugins ∩ attach_policy, guarded on the runtime registry having the client
      * @ref LLP 0044#consent--join-implies-consent-default-on [constrained-by] — default-on; only `attach.on_join:false` in the locked central plugin entry opts out
+     * @ref LLP 0045#part-3--reverse-runs-from-disk-the-marker-is-a-self-describing-undo-record [constrained-by] — attach-eligibility requires the `attachProbe` reverse() replays; a probe-less client could attach but never be undone, orphaning its settings on a config-drop (#212)
      */
     desired(ctx) {
       const descriptors = ctx.clientDescriptors
@@ -92,6 +94,13 @@ export function createAttachHandler(opts = {}) {
         if (!entry || entry.enabled === false) continue
         // Default-on: only an explicit `on_join: false` opts out.
         if (readAttachPolicy(entry).onJoin === false) continue
+        // Attach-eligibility requires reverse-capability. reverse() undoes the
+        // on-disk settings by replaying the descriptor's `attachProbe` (Part 3);
+        // perform() needs no probe (it just calls the live adapter). A probe-less
+        // client would therefore attach and mark `done` but could never be
+        // reversed: on a config-drop the marker drops while the settings stay
+        // written, orphaning them. Never name a client we cannot also undo (#212).
+        if (!descriptor.attachProbe) continue
         // Never name a client the runtime registry can't reach.
         if (!clients.getClient(descriptor.name)) continue
         desired.push({
@@ -179,10 +188,19 @@ export function createAttachHandler(opts = {}) {
      * (`detachClientFromDisk`) — the same one `hyp detach` uses. It needs
      * `ctx.clientDescriptors` and the filesystem, **never** `ctx.clients`.
      *
+     * A descriptor with **no `attachProbe`** cannot be honestly reversed: the
+     * core undo returns `{ changed: false }` for "no probe" exactly as it does
+     * for "already clean", so a `done` here would silently drop the marker while
+     * the settings `attach()` wrote stay on disk, orphaned and invisible to a
+     * later detach. Treat a missing probe as a **failed** (retryable, visible)
+     * reverse instead. `desired()` already refuses to attach a probe-less client,
+     * so this only fires for a marker applied out-of-band (e.g. manual
+     * `hyp attach`, or a pre-fix marker).
+     *
      * @param {string} requestKey  The client name whose attach to reverse.
      * @param {ActionContext} ctx
      * @returns {Promise<ActionOutcome>}
-     * @ref LLP 0045#part-3--reverse-runs-from-disk-the-marker-is-a-self-describing-undo-record [implements] — reverse() invokes the single disk-driven core undo (detachClientFromDisk), not ctx.clients
+     * @ref LLP 0045#part-3--reverse-runs-from-disk-the-marker-is-a-self-describing-undo-record [implements] — reverse() invokes the single disk-driven core undo (detachClientFromDisk), not ctx.clients; a missing attachProbe is a failed reverse, not a no-op marker drop (#212)
      */
     async reverse(requestKey, ctx) {
       const descriptor = ctx.clientDescriptors?.get(requestKey)
@@ -191,6 +209,16 @@ export function createAttachHandler(opts = {}) {
         // goes away), so this is a real gap; keep the marker and retry.
         return { status: 'failed', reason: `no client descriptor for '${requestKey}' to reverse` }
       }
+      if (!descriptor.attachProbe) {
+        // No probe → the disk-driven undo can do nothing, but a marker exists
+        // (that is why reverse fired). Returning `done` would drop it while the
+        // settings stay written, orphaning them. Fail honestly so the marker
+        // stays visible and retryable rather than silently dropped (#212).
+        return {
+          status: 'failed',
+          reason: `client '${descriptor.name}' has no attach_probe; cannot reverse its on-disk settings - keeping the marker rather than orphaning them`,
+        }
+      }
 
       ctx.log.info('client_action.attach_reverse', {
         [Attr.COMPONENT]: 'action-attach',

test/core/action-attach.test.js

@@ -53,6 +53,19 @@ const CODEX_DESCRIPTOR = {
   attachProbe: { format: 'toml', settings_file: '.codex/config.toml', marker_header: '[model_providers.hypaware]' },
 }
 
+/**
+ * A client descriptor with **no `attachProbe`**. perform() can attach it (it
+ * only needs a live adapter), but the disk-driven reverse() has nothing to
+ * replay, so it must be excluded from attach-eligibility and, if a marker is
+ * ever applied out-of-band, treated as a failed (not no-op) reverse (#212).
+ * @type {ClientDescriptor}
+ */
+const PROBELESS_DESCRIPTOR = {
+  plugin: /** @type {any} */ ('@hypaware/probeless'),
+  name: 'probeless',
+  skillDir: 'skills/probeless',
+}
+
 /**
  * @param {ClientDescriptor[]} list
  * @returns {Map<string, ClientDescriptor>}
@@ -206,6 +219,20 @@ test('desired() does not fail open on a non-boolean on_join (treats it as opt-ou
   assert.deepEqual(stringFalse, [], 'on_join:"false" (string) must not attach')
 })
 
+test('desired() excludes a probe-less descriptor - attach-eligibility requires reverse-capability (#212)', () => {
+  const handler = createAttachHandler()
+  // Enabled plugin + registered client, but the descriptor declares no
+  // attachProbe. perform() could attach it, but reverse() (disk-driven) could
+  // never undo it, so it must never be named as an attach target, otherwise a
+  // later config-drop drops the marker while the settings stay written.
+  const desired = handler.desired(makeCtx({
+    plugins: [{ name: '@hypaware/probeless', enabled: true, config: {} }],
+    descriptors: descriptorMap([PROBELESS_DESCRIPTOR]),
+    clients: clientsWith({ probeless: attachRegistration('probeless') }),
+  }))
+  assert.deepEqual(desired, [], 'a probe-less client must never be named as an attach target')
+})
+
 test('desired() guards on the runtime registry actually having the client', () => {
   const handler = createAttachHandler()
   // Enabled plugin + descriptor, but the gateway registered no such client →
@@ -419,6 +446,25 @@ test('reverse() replays the real core undo from disk with no adapter loaded (fs
   }
 })
 
+test('reverse() of a probe-less descriptor fails - never silently drops the marker, orphaning settings (#212)', async () => {
+  // A marker applied out-of-band (manual `hyp attach`, or a pre-fix marker) for
+  // a probe-less client: the core undo returns { changed:false } for "no probe"
+  // exactly as it does for "already clean", so a `done` here would drop the
+  // marker while the settings stay on disk. reverse() must short-circuit on the
+  // missing probe and fail (retryable/visible), without consulting the undo.
+  let detachCalled = false
+  const handler = createAttachHandler({
+    detach: async () => { detachCalled = true; return { changed: false } },
+  })
+  const outcome = await reverseOf(handler)('probeless', makeCtx({
+    descriptors: descriptorMap([PROBELESS_DESCRIPTOR]),
+    clients: undefined,
+  }))
+  assert.equal(outcome.status, 'failed', 'a probe-less reverse is a failure, not a marker-dropping no-op')
+  assert.match(String(outcome.reason), /attach_probe/)
+  assert.equal(detachCalled, false, 'reverse must not pretend the disk undo ran')
+})
+
 test('reverse() returns failed (retry next pass) when the descriptor is gone from the catalog', async () => {
   const handler = createAttachHandler({
     detach: async () => { throw new Error('detach should not be called without a descriptor') },

test/core/action-reconciler.test.js

@@ -11,6 +11,7 @@ import {
   createActionReconciler,
   readClientActionStatus,
 } from '../../src/core/config/action_reconciler.js'
+import { createAttachHandler } from '../../src/core/config/action_attach.js'
 
 /**
  * @import {
@@ -19,6 +20,7 @@ import {
  *   ActionOutcome,
  *   DesiredAction,
  * } from '../../src/core/config/types.d.ts'
+ * @import { ClientDescriptor } from '../../src/core/types.js'
  */
 
 /** A quiet logger so tests don't spam stderr. */
@@ -50,6 +52,19 @@ function readMarkerFile(stateRoot) {
   return JSON.parse(fs.readFileSync(markerPath(stateRoot), 'utf8'))
 }
 
+/**
+ * A client descriptor with no `attachProbe`: perform() could attach it, but the
+ * disk-driven reverse() has nothing to replay, so a marker applied out-of-band
+ * for it must reverse as a `failed` (not a marker-dropping no-op). Mirrors the
+ * `PROBELESS_DESCRIPTOR` fixture in `action-attach.test.js` (#212).
+ * @type {ClientDescriptor}
+ */
+const PROBELESS_DESCRIPTOR = {
+  plugin: /** @type {any} */ ('@hypaware/probeless'),
+  name: 'probeless',
+  skillDir: 'skills/probeless',
+}
+
 /**
  * A run-once handler whose `perform` counts calls. `desired()` returns one
  * unit per configured request key.
@@ -350,6 +365,64 @@ test('a reversible handler undoes a previously-applied key the config no longer
   }
 })
 
+test('a failed reverse keeps the marker in the store (probe-less attach is never orphaned) (#212)', async () => {
+  const { tmp, stateRoot } = await makeFixture()
+  try {
+    // Seed a pre-existing applied `attach` marker for a probe-less client, as
+    // if it had been written out-of-band (a manual `hyp attach`, or a pre-fix
+    // marker). The real attach handler's reverse() short-circuits on the
+    // missing attachProbe and returns `failed` rather than a marker-dropping
+    // no-op, so the reconciler must KEEP the marker. Dropping it would orphan
+    // the on-disk settings: present, but invisible to a later detach.
+    const controlDir = path.join(stateRoot, 'config-control')
+    fs.mkdirSync(controlDir, { recursive: true })
+    fs.writeFileSync(
+      markerPath(stateRoot),
+      JSON.stringify(
+        { attach: { probeless: { status: 'done', request_key: 'probeless', at: '2026-06-25T00:00:00.000Z' } } },
+        null,
+        2
+      ) + '\n'
+    )
+
+    // Inject a detach spy so we can prove reverse() never even consulted the
+    // disk undo (it short-circuits on the missing probe).
+    let detachCalled = false
+    const handler = createAttachHandler({
+      detach: async () => {
+        detachCalled = true
+        return { changed: false }
+      },
+    })
+    const reconciler = createActionReconciler({ stateRoot, handlers: [handler], log: NOOP_LOG })
+
+    // desired() returns [] (no `clients` registry on this input), so `probeless`
+    // is no-longer-desired and the reverse gap fires for the seeded marker.
+    const report = await reconciler.reconcile({
+      ...INPUT,
+      clientDescriptors: new Map([[PROBELESS_DESCRIPTOR.name, PROBELESS_DESCRIPTOR]]),
+    })
+
+    // (a) the reverse failure is reported, not a `reversed`.
+    assert.deepEqual(
+      report.results.map((r) => [r.kind, r.requestKey, r.outcome]),
+      [['attach', 'probeless', 'failed']]
+    )
+    assert.match(String(report.results[0].reason), /attach_probe/)
+    assert.equal(detachCalled, false, 'a probe-less reverse must not pretend the disk undo ran')
+
+    // (b) the marker REMAINS in the store: never dropped, so the settings stay
+    // owned and a later detach can still find them.
+    assert.equal(readMarkerFile(stateRoot).attach.probeless.status, 'done')
+    assert.equal(
+      readClientActionStatus({ stateRoot }).byKind.attach.probeless.status,
+      'done'
+    )
+  } finally {
+    await fsp.rm(tmp, { recursive: true, force: true })
+  }
+})
+
 test('a run-once handler never reverses a no-longer-desired done marker', async () => {
   const { tmp, stateRoot } = await makeFixture()
   try {