From aa31fc738b93d4ef2138518a020c026517544340 Mon Sep 17 00:00:00 2001 From: test Date: Wed, 5 Aug 2026 04:23:12 +0000 Subject: [PATCH 1/3] hyp leave's assetless-marker drop was never taught about refused markers (#627) LLP 0186 added `refused` as a terminal marker state that, like `failed`, records no effect of its own: an attach refusal stops before it touches the client's settings. The reconciler's reverse gap was taught to treat the two alike. `hyp leave`'s parallel gate was not, and still asked `marker.status === 'failed'`, so an assetless `refused` marker fell through to the full `detachClientViaCore` reversal instead of being dropped. The fallthrough is not data loss, but it is not silent either: the disk probe finds nothing to reverse and prints "No HypAware marker found in ; nothing to do." during `hyp leave`, naming a settings file the refusal never wrote. Both gates now share one predicate, `markerRecordsNoEffect`, beside `readInstalledAssets` in action_reconciler.js. It asks the property the gates actually care about (this marker records no on-disk effect), which needs both a status that never wrote one and an empty `installed_assets`: a `done` attach that copied no files still owns the settings edit it wrote, so "carries no assets" alone would be the wrong test. An unrecognized status counts as recording an effect, so a marker state added later routes to the real reversal instead of being dropped by a gate nobody updated. The reconciler's own behaviour is unchanged; its inline condition is replaced by the identical shared call. Co-Authored-By: Claude --- src/core/commands/central.js | 26 ++++-- src/core/config/action_reconciler.js | 47 ++++++++-- test/core/leave-command.test.js | 127 +++++++++++++++++++++++++++ 3 files changed, 187 insertions(+), 13 deletions(-) diff --git a/src/core/commands/central.js b/src/core/commands/central.js index 3c05b266..d5481ac4 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 {@link 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..e26cb5c3 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,44 @@ 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. + * + * @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/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( From d898a88e4af9c65568e5ff708d6cfc4773569738 Mon Sep 17 00:00:00 2001 From: test Date: Wed, 5 Aug 2026 05:04:40 +0000 Subject: [PATCH 2/3] Pin markerRecordsNoEffect's defaulting direction, the property the shared predicate exists for The predicate's two halves are both already covered: the reverse-gap and `hyp leave` tests fail if the asset check goes away, and the `done` cases fail if the status check goes away. Its third property was not covered at all: mutating the status test from an allowlist ("failed or refused, else an effect") to a denylist ("done or applied, else no effect") left the whole suite green. That defaulting is the anti-drift property the shared predicate was introduced for. Inverted, a fifth marker state added later is silently dropped by both gates instead of routed to the real reversal, which is the data-loss shape of the #627 bug rather than its harmless one. Pin it, plus the truth table for a marker whose `installed_assets` is malformed rather than absent. Co-Authored-By: Claude --- test/core/action-reconciler.test.js | 78 +++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) 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 + ) +}) From 7b598df3826a0b8e80138e15c82d0ebda5efc601 Mon Sep 17 00:00:00 2001 From: neutral Date: Wed, 5 Aug 2026 06:11:02 +0000 Subject: [PATCH 3/3] markerRecordsNoEffect names the case its two records do not cover A marker rewritten from `done` to `failed` or `refused` carries the earlier attach's asset list forward but nothing carries its settings write forward, so an attach that landed a settings write while installing no assets reads as "recorded nothing" and is dropped over settings still on disk. The predicate's doc claimed the two records were sufficient. Say what they miss instead, and where the fix belongs. Co-Authored-By: Claude --- src/core/commands/central.js | 4 ++-- src/core/config/action_reconciler.js | 12 ++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/core/commands/central.js b/src/core/commands/central.js index d5481ac4..ef100918 100644 --- a/src/core/commands/central.js +++ b/src/core/commands/central.js @@ -434,8 +434,8 @@ export async function runLeave(argv, ctx) { const marker = attachMarkers[name] const installedAssets = readInstalledAssets(marker) // A marker that recorded nothing applied no effect; just drop it, - // through the same {@link markerRecordsNoEffect} test the reconciler's - // own reverse pass uses, so the two gates cannot drift over a marker + // 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 diff --git a/src/core/config/action_reconciler.js b/src/core/config/action_reconciler.js index e26cb5c3..0a03e68a 100644 --- a/src/core/config/action_reconciler.js +++ b/src/core/config/action_reconciler.js @@ -526,6 +526,18 @@ export function readInstalledAssets(marker) { * 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