diff --git a/src/core/config/action_attach.js b/src/core/config/action_attach.js index 0667a56..35759cf 100644 --- a/src/core/config/action_attach.js +++ b/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} - * @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', diff --git a/test/core/action-attach.test.js b/test/core/action-attach.test.js index 6758970..47712e1 100644 --- a/test/core/action-attach.test.js +++ b/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} @@ -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') }, diff --git a/test/core/action-reconciler.test.js b/test/core/action-reconciler.test.js index be03f3a..4c69016 100644 --- a/test/core/action-reconciler.test.js +++ b/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 {