diff --git a/src/core/commands/central.js b/src/core/commands/central.js index 3c05b266..ef100918 100644 --- a/src/core/commands/central.js +++ b/src/core/commands/central.js @@ -12,6 +12,7 @@ import { validateConfig } from '../config/validate.js' import { atomicWriteJson } from '../util/fs_atomic.js' import { clearClientActionMarker, + markerRecordsNoEffect, readClientActionStatus, readInstalledAssets, } from '../config/action_reconciler.js' @@ -432,16 +433,25 @@ export async function runLeave(argv, ctx) { for (const name of attachedNames) { const marker = attachMarkers[name] const installedAssets = readInstalledAssets(marker) - if (!marker || (marker.status === 'failed' && installedAssets.length === 0)) { - // A failed marker that recorded nothing applied no effect; just - // drop it, mirroring the reconciler's own reverse pass. A failed - // marker CAN still name assets: an attach that went `done` and then - // re-`perform()`ed unsuccessfully is rewritten `failed` with its - // `installed_assets` carried forward, and those copies are still on - // disk. Such a marker falls through to the normal reversal below. + // A marker that recorded nothing applied no effect; just drop it, + // through the same `markerRecordsNoEffect` test the reconciler's own + // reverse pass uses, so the two gates cannot drift over a marker + // state only one of them was taught about. That is not hypothetical: + // this gate asked `status === 'failed'` and was left behind when + // LLP 0186 added the terminal `refused` state, even though a refusal + // records strictly less than a failure does (it stops before touching + // the client's settings at all). + // + // A `failed` or `refused` marker CAN still name assets: an attach that + // went `done` and then re-`perform()`ed unsuccessfully is rewritten + // with its `installed_assets` carried forward, and those copies are + // still on disk. Such a marker falls through to the normal reversal + // below, whichever of the two states it now carries. + // @ref LLP 0186#how-the-reconciler-distinguishes-it-from-done [implements]: leave's assetless-drop reads refused the way the reverse gap does + if (markerRecordsNoEffect(marker)) { try { clearClientActionMarker({ stateRoot, kind: 'attach', requestKey: name }) - } catch { /* best-effort: a stale failed marker is a status blemish */ } + } catch { /* best-effort: a stale marker that undoes nothing is a status blemish */ } continue } const descriptor = descriptors.get(name) diff --git a/src/core/config/action_reconciler.js b/src/core/config/action_reconciler.js index 71dca809..0a03e68a 100644 --- a/src/core/config/action_reconciler.js +++ b/src/core/config/action_reconciler.js @@ -312,11 +312,10 @@ export function createActionReconciler(opts) { // @ref LLP 0138#marker-undo [implements]: a marker is never dropped // over an effect it recorded, whichever status it carries // @ref LLP 0186#how-the-reconciler-distinguishes-it-from-done [implements]: the reverse gap treats refused the way it treats failed - if ( - !marker || - ((marker.status === 'failed' || marker.status === 'refused') && - readInstalledAssets(marker).length === 0) - ) { + // + // The test itself is {@link markerRecordsNoEffect}, shared with + // `hyp leave`'s parallel gate rather than spelled out twice. + if (markerRecordsNoEffect(marker)) { delete markers[requestKey] mutated = true continue @@ -502,6 +501,56 @@ export function readInstalledAssets(marker) { return raw.filter((dest) => typeof dest === 'string' && dest.length > 0) } +/** + * Whether a marker records no on-disk effect at all: the question every gate + * that drops markers actually asks before choosing between a plain drop and a + * real reversal. Lives beside {@link readInstalledAssets} for the same reason + * that accessor does: the droppers (this reconciler's reverse gap and + * `hyp leave`) are not all handlers, and two gates deciding "nothing to undo" + * for themselves are two chances to disagree about it. + * + * Two independent records both have to say nothing happened: + * + * - **A status that never wrote the handler's effect.** `failed` and `refused` + * are the whole of that set: a `failed` `perform()` tried and did not land, + * and a refusal stops *before* touching the client's settings (LLP 0186). + * `done` and `applied` are their opposite by definition, and this is why + * "carries no assets" cannot be the whole test on its own: a `done` attach + * that copied no files still owns the settings edit it wrote. + * - **No `installed_assets`.** A marker that went `done` and was later + * rewritten `failed` or `refused` carries the earlier attach's file list + * forward, and nothing else on disk names those copies. + * + * A status this function does not recognize counts as recording an effect, so + * a marker state added later routes to the real reversal until someone decides + * otherwise, rather than being silently dropped by a gate nobody remembered to + * update. A missing marker records nothing by construction. + * + * **Known incomplete, in the one direction those two records do not cover.** + * A marker that reached `done` and was later rewritten `failed` or `refused` + * carries the earlier attach's *asset* list forward, but nothing carries its + * *settings* write forward: no field on the rewritten marker distinguishes it + * from one whose first `perform()` applied nothing at all. So an attach that + * landed a settings write while installing no assets (a client contributing + * none, a copy that failed, any pre-LLP-0138 marker) and then re-`perform()`ed + * into a failure or a refusal reads as "recorded nothing" here and is dropped + * over settings that are still on disk. Closing it means teaching the rewrite + * to record the effect it overwrites, a marker-schema question LLP 0138 has + * not settled, rather than changing this test. + * + * @param {ActionMarker} [marker] + * @returns {boolean} + * @ref LLP 0138#marker-undo [implements]: a marker is never dropped over an + * effect it recorded, so the drop test is "recorded nothing", not a status name + * @ref LLP 0186#how-the-reconciler-distinguishes-it-from-done [implements]: a + * refusal wrote nothing of its own, exactly as a failure did not + */ +export function markerRecordsNoEffect(marker) { + if (!marker) return true + if (marker.status !== 'failed' && marker.status !== 'refused') return false + return readInstalledAssets(marker).length === 0 +} + /** * Read-only view of the client-action markers for `hyp status`: usable * from any process (the CLI is not the daemon), so it never constructs the diff --git a/test/core/action-reconciler.test.js b/test/core/action-reconciler.test.js index ed0c0808..adbe54e8 100644 --- a/test/core/action-reconciler.test.js +++ b/test/core/action-reconciler.test.js @@ -9,6 +9,7 @@ import path from 'node:path' import { createActionReconciler, + markerRecordsNoEffect, readClientActionStatus, clearClientActionMarker, rearmRefusedActionMarker, @@ -880,3 +881,80 @@ test('the reverse gap drops an assetless refused marker and reverses one that re await fsp.rm(tmp, { recursive: true, force: true }) } }) + +test('markerRecordsNoEffect: an unrecognized status records an effect, so a new marker state routes to the real reversal', () => { + // The gates' shared drop test is an ALLOWLIST with a safe default, not a + // denylist. Both callers (the reverse gap and `hyp leave`) hand every marker + // they might drop to this one predicate, so the direction it defaults in is + // the whole anti-drift property: a marker state added after this predicate + // was written costs a pointless disk probe (the #627 symptom) instead of a + // silent drop over an effect nobody has taught it to recognize yet. The + // reverse-gap and leave tests both exercise the two known no-effect states; + // nothing but this pins which way the unknown ones fall. + // @ref LLP 0138#marker-undo [tests]: an unrecognized status is never grounds + // to drop a marker, because only the marker could name what it recorded + for (const status of ['applied', 'reversing', '', 'FAILED', 'Refused']) { + assert.equal( + markerRecordsNoEffect( + /** @type {any} */ ({ status, request_key: 'k' }) + ), + false, + `status '${status}' must count as recording an effect` + ) + } + // A marker with no status at all (a hand edit, a truncated write) is an + // unrecognized status too, not an absent marker. + assert.equal(markerRecordsNoEffect(/** @type {any} */ ({ request_key: 'k' })), false) +}) + +test('markerRecordsNoEffect: both halves are required, and the asset half reads through readInstalledAssets', () => { + // No marker records nothing by construction. + assert.equal(markerRecordsNoEffect(undefined), true) + + // The two states that never wrote the handler's effect, with nothing carried. + assert.equal(markerRecordsNoEffect({ status: 'failed', request_key: 'k' }), true) + assert.equal(markerRecordsNoEffect({ status: 'refused', request_key: 'k' }), true) + + // The status half alone is not enough: a `done` attach that copied no files + // still owns the settings edit it wrote. + assert.equal(markerRecordsNoEffect({ status: 'done', request_key: 'k' }), false) + + // The asset half alone is not enough either: a marker rewritten from `done` + // carries the earlier attach's file list forward, and nothing else on disk + // names those copies. + assert.equal( + markerRecordsNoEffect({ status: 'failed', request_key: 'k', installed_assets: ['/a'] }), + false + ) + assert.equal( + markerRecordsNoEffect({ status: 'refused', request_key: 'k', installed_assets: ['/a'] }), + false + ) + + // The store is a JSON file a hand edit can malform. An `installed_assets` + // that holds no usable path names nothing, so it must not pin a marker open + // forever: the same defensive read every other dropper makes. + assert.equal( + markerRecordsNoEffect({ status: 'failed', request_key: 'k', installed_assets: [] }), + true + ) + assert.equal( + markerRecordsNoEffect( + /** @type {any} */ ({ status: 'refused', request_key: 'k', installed_assets: 'nope' }) + ), + true + ) + assert.equal( + markerRecordsNoEffect( + /** @type {any} */ ({ status: 'failed', request_key: 'k', installed_assets: ['', null, 7] }) + ), + true + ) + // ... but one usable path among the junk still counts. + assert.equal( + markerRecordsNoEffect( + /** @type {any} */ ({ status: 'failed', request_key: 'k', installed_assets: [null, '/a'] }) + ), + false + ) +}) diff --git a/test/core/leave-command.test.js b/test/core/leave-command.test.js index d4feb456..9f0b447a 100644 --- a/test/core/leave-command.test.js +++ b/test/core/leave-command.test.js @@ -354,6 +354,133 @@ test('leave removes the assets its attach marker records, and leaves manual copi assert.equal(markers.attach, undefined) }) +test('leave drops an assetless refused attach marker the way it drops an assetless failed one', async () => { + // LLP 0186 added `refused` beside `failed` as a marker state that records no + // effect of its own: a refusal stops before touching the client's settings. + // The reconciler's reverse gap was taught that; leave's parallel gate still + // asked `status === 'failed'`, so a refused marker fell through to a disk + // reversal with nothing to reverse - and said so out loud, telling the user + // about settings that were never written. + const { home, stateRoot, stdout, opts } = await makeDispatchOpts() + assert.equal( + await dispatch(['join', 'https://central.example', 'policy-token-1', '--no-daemon'], opts), + 0 + ) + + // One store, three states. Only claude was ever applied; codex refused and + // openclaw failed, so neither wrote a settings file at all. + const settingsPath = path.join(home, '.claude', 'settings.json') + await fs.mkdir(path.dirname(settingsPath), { recursive: true }) + await fs.writeFile( + settingsPath, + JSON.stringify( + { + env: { ANTHROPIC_BASE_URL: 'http://127.0.0.1:4388' }, + _hypaware: { managed: { env: { ANTHROPIC_BASE_URL: 'http://127.0.0.1:4388' }, hooks: [] } }, + }, + null, + 2 + ) + '\n' + ) + const controlDir = path.join(stateRoot, 'config-control') + await fs.writeFile( + path.join(controlDir, 'client-actions.json'), + JSON.stringify( + { + attach: { + claude: { status: 'done', request_key: 'claude' }, + codex: { + status: 'refused', + request_key: 'codex', + reason: 'model_providers.hypaware already exists and was not written by HypAware', + at: '2026-08-04T00:00:00.000Z', + }, + openclaw: { status: 'failed', request_key: 'openclaw', reason: 'boom', attempts: 1 }, + }, + }, + null, + 2 + ) + '\n' + ) + + const code = await dispatch(['leave'], opts) + assert.equal(code, 0, stdout.text()) + + // The refused marker never reached the disk reversal, so leave never reported + // on a settings file the refusal did not write. This is the regression: the + // pre-fix gate let `codex` through to `detachClientViaCore`, whose no-op + // result prints exactly this line. + assert.doesNotMatch(stdout.text(), /No HypAware marker found/) + assert.doesNotMatch(stdout.text(), /\.codex/) + + // A refusal writes no settings file, and leave must not create one probing. + await assert.rejects(fs.stat(path.join(home, '.codex', 'config.toml'))) + + // The neighbours are untouched by the change: `done` still reverses on disk, + // and an assetless `failed` is still dropped. All three markers are gone. + const settings = JSON.parse(await fs.readFile(settingsPath, 'utf8')) + assert.equal('_hypaware' in settings, false) + assert.equal(settings.env?.ANTHROPIC_BASE_URL, undefined) + const markers = JSON.parse(await fs.readFile(path.join(controlDir, 'client-actions.json'), 'utf8')) + assert.equal(markers.attach, undefined) +}) + +test('leave still reverses a refused attach marker that carries installed assets', async () => { + // The other half of the same test: `refused` is not a licence to drop. An + // attach that went `done`, then re-`perform()`ed into a refusal, carries the + // earlier run's `installed_assets` forward, and the marker is the only thing + // on disk naming those copies (LLP 0138 #marker-undo). It has to take the + // real reversal, exactly as an asset-bearing `failed` marker does. + const { home, stateRoot, opts } = await makeDispatchOpts() + + const settingsPath = path.join(home, '.claude', 'settings.json') + await fs.mkdir(path.dirname(settingsPath), { recursive: true }) + await fs.writeFile( + settingsPath, + JSON.stringify( + { + env: { ANTHROPIC_BASE_URL: 'http://127.0.0.1:4388' }, + _hypaware: { managed: { env: { ANTHROPIC_BASE_URL: 'http://127.0.0.1:4388' }, hooks: [] } }, + }, + null, + 2 + ) + '\n' + ) + const orgSkill = path.join(home, '.claude', 'skills', 'hypaware-privacy') + await fs.mkdir(orgSkill, { recursive: true }) + await fs.writeFile(path.join(orgSkill, 'SKILL.md'), 'org\n', 'utf8') + + const controlDir = path.join(stateRoot, 'config-control') + await fs.mkdir(controlDir, { recursive: true }) + await fs.writeFile( + path.join(controlDir, 'client-actions.json'), + JSON.stringify( + { + attach: { + claude: { + status: 'refused', + request_key: 'claude', + reason: 'settings file is JSONC; refusing to modify', + installed_assets: [orgSkill], + }, + }, + }, + null, + 2 + ) + '\n' + ) + + await dispatch(['leave'], opts) + + // Routed to the real reversal: the carried asset is gone and the settings the + // earlier successful attach wrote are reversed, not stranded by a drop. + await assert.rejects(fs.stat(orgSkill)) + const settings = JSON.parse(await fs.readFile(settingsPath, 'utf8')) + assert.equal('_hypaware' in settings, false) + const markers = JSON.parse(await fs.readFile(path.join(controlDir, 'client-actions.json'), 'utf8')) + assert.equal(markers.attach, undefined) +}) + test('leave self-heals an org attach whose plugin is gone: drops the marker, warns, stays clean', async () => { const { stateRoot, stdout, opts } = await makeDispatchOpts() assert.equal(