hyp leave's assetless-marker drop was never taught about refused markers (#627)

src/core/commands/central.js

@@ -494,18 +494,20 @@ 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.
-            try {
-              clearClientActionMarker({ stateRoot, kind: 'attach', requestKey: name })
-            } catch { /* best-effort: a stale failed marker is a status blemish */ }
-            continue
-          }
+          // Every marker leave holds goes to the real reversal, whatever status
+          // it carries. There used to be a shortcut here that skipped the
+          // reversal for a marker whose status "recorded nothing" (a `failed`
+          // one with no assets), and it read the status as a proxy for what is
+          // on disk: a marker that reached `done`, wrote the client's settings
+          // and installed no files, then re-`perform()`ed into a `failed` or
+          // `refused` rewrite, is indistinguishable from one that never applied
+          // anything, so the shortcut skipped a settings edit that was still
+          // there (#627 review round 2). The undo reads the client's own file
+          // instead of the marker's status, is idempotent, and reports
+          // `changed: false` when there is nothing to reverse, so running it
+          // over a marker that really did record nothing costs one stat.
+          // Leave's own no-op is quiet (`quietNoop`), which is what made the
+          // shortcut look worth having in the first place.
           const descriptor = descriptors.get(name)
           if (!descriptor) {
             // Plugin's gone, so we cannot replay its undo - do the best we can:
@@ -536,12 +538,19 @@ export async function runLeave(argv, ctx) {
           // visible and tells the user how to retry.
           // @ref LLP 0107#reversal [implements]: leave reverses org-installed
           //   assets through the same core detach the CLI verb uses
+          // @ref LLP 0045#part-3-reverse-runs-from-disk-the-marker-is-a-self-describing-undo-record [implements]: every marker leave holds takes this undo, which decides what to reverse from the client's own settings file rather than from the marker's status
           try {
             await detachClientViaCore({
               name,
               descriptor,
               dryRun: false,
               json: false,
+              // A sweep, not a request about this client: leave runs the undo
+              // for every marker on disk, so a client with nothing to reverse
+              // must not narrate a settings file the user may never have had.
+              // `hyp detach <client>` keeps saying it, because there the line
+              // is the answer to what the user asked.
+              quietNoop: true,
               ctx,
             })
           } catch (err) {

src/core/commands/clients.js

@@ -1017,18 +1017,28 @@ async function materializeAttachAssets({ name, descriptorMap, ctx, dryRun, json
  * lines can be the only surviving copy of what the undo left behind. The
  * asset-refusal stderr writes stay unconditional either way.
  *
+ * `quietNoop` is the narrower cut, for a caller that still wants the prose:
+ * it suppresses only the human "nothing to do" line, for the callers that
+ * reverse a whole set of markers rather than the one client a user named.
+ * `hyp leave` runs this over every attach marker on disk, and a client with
+ * nothing left to reverse must not narrate a settings file the user may never
+ * have had (#627). It changes nothing else, and never the `--json` payload,
+ * which carries `changed` for exactly this distinction. It is moot under
+ * `quiet`, which already withholds every stdout line this routine writes.
+ *
  * @param {{
  *   name: string,
  *   descriptor: ClientDescriptor | undefined,
  *   dryRun: boolean,
  *   json: boolean,
  *   quiet?: boolean,
+ *   quietNoop?: boolean,
  *   ctx: CommandRunContext,
  * }} args
  * @returns {Promise<DetachFromDiskResult | undefined>}
  * @ref LLP 0045#part-3-reverse-runs-from-disk-the-marker-is-a-self-describing-undo-record [implements]: manual detach is the disk-driven core undo, resolved via the clientDescriptor; one undo, shared with the reconciler reverse()
  */
-export async function detachClientViaCore({ name, descriptor, dryRun, json, quiet, ctx }) {
+export async function detachClientViaCore({ name, descriptor, dryRun, json, quiet, quietNoop, ctx }) {
   if (!descriptor) {
     throw new Error(`no client descriptor for '${name}'; cannot reverse its attach from disk`)
   }
@@ -1084,7 +1094,7 @@ export async function detachClientViaCore({ name, descriptor, dryRun, json, quie
             changed: true,
           })
         }
-        if (!quiet) writeCoreDetachOutput({ ctx, name, json, result })
+        if (!quiet) writeCoreDetachOutput({ ctx, name, json, quietNoop, result })
         const stateRoot = readObservabilityEnv(ctx.env).stateDir
 
         // Retract the attach marker so the CLI undo and the marker store stay in
@@ -1257,6 +1267,7 @@ export async function detachAllClientsFromDisk(ctx) {
  *   ctx: CommandRunContext,
  *   name: string,
  *   json: boolean,
+ *   quietNoop?: boolean,
  *   result: {
  *     changed: boolean,
  *     settingsPath?: string,
@@ -1267,7 +1278,7 @@ export async function detachAllClientsFromDisk(ctx) {
  *   },
  * }} args
  */
-function writeCoreDetachOutput({ ctx, name, json, result }) {
+function writeCoreDetachOutput({ ctx, name, json, quietNoop, result }) {
   const settingsPath = result.settingsPath
   if (json) {
     /** @type {Record<string, unknown>} */
@@ -1297,7 +1308,12 @@ function writeCoreDetachOutput({ ctx, name, json, result }) {
       ctx.stdout.write(`  Restored ${restoredPath} from the marker's malformed-block backup\n`)
     }
     if (result.warning !== undefined) ctx.stdout.write(`  warning: ${result.warning}\n`)
-  } else {
+  } else if (quietNoop !== true) {
+    // `changed: false` from a disk-driven undo means "this client's settings
+    // hold nothing of ours", which is an answer when the user named the client
+    // and noise when a sweep is walking every marker it can find (#627). Only
+    // the sweeps pass `quietNoop`; `hyp detach <client>` still reports, since
+    // this line is the whole of its output on an already-clean client.
     ctx.stdout.write(
       `No HypAware marker found${settingsPath !== undefined ? ` in ${settingsPath}` : ''}; nothing to do.\n`
     )

test/core/leave-command.test.js

@@ -142,7 +142,9 @@ test('leave reverses org-driven attaches and drops the forward identity', async
       {
         attach: {
           claude: { status: 'done', request_key: 'claude' },
-          // A failed marker never applied an effect: leave just drops it.
+          // A failed attach takes the same undo every other marker takes; it
+          // finds nothing in codex's settings, says nothing, and the marker
+          // goes with it.
           codex: { status: 'failed', request_key: 'codex', reason: 'boom', attempts: 1 },
         },
         // Backfill is run-once by design: its marker survives leave.
@@ -170,7 +172,7 @@ test('leave reverses org-driven attaches and drops the forward identity', async
   assert.equal('_hypaware' in settings, false)
   assert.equal(settings.env?.ANTHROPIC_BASE_URL, undefined)
 
-  // Attach markers are gone (done reversed, failed dropped); backfill stays.
+  // Attach markers are gone (both reversed, one of them a no-op); backfill stays.
   const markers = JSON.parse(await fs.readFile(path.join(controlDir, 'client-actions.json'), 'utf8'))
   assert.equal(markers.attach, undefined)
   assert.equal(markers.backfill['claude:default'].status, 'done')
@@ -407,6 +409,137 @@ test('leave removes the assets its attach marker records, and leaves manual copi
   assert.equal(markers.attach, undefined)
 })
 
+test('leave reverses a settings-only attach whose marker was later rewritten to refused', async () => {
+  // The case that decided #627's direction. A marker's status is not a record
+  // of what is on disk: an attach that wrote the client's settings and
+  // installed no files records `done` with no `installed_assets`, and a later
+  // re-perform() that refuses (LLP 0186) rewrites the status in place. Nothing
+  // carries the settings write forward, so the rewritten marker is
+  // byte-indistinguishable from one whose attach never applied anything -
+  // while the settings edit is still there. Leave therefore runs the
+  // disk-driven undo for every marker it holds and lets the client's own file
+  // decide, instead of reading the status as a proxy for it.
+  const { home, stateRoot, stdout, opts } = await makeDispatchOpts()
+  assert.equal(
+    await dispatch(['join', 'https://central.example', 'policy-token-1', '--no-daemon'], opts),
+    0
+  )
+
+  // The attach's whole effect: the managed block and the env it manages. No
+  // assets, which is the normal shape for a client that contributes none.
+  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: 'refused',
+            request_key: 'claude',
+            reason: 'settings.json is JSONC; refusing to modify',
+            at: '2026-08-04T00:00:00.000Z',
+          },
+        },
+      },
+      null,
+      2
+    ) + '\n'
+  )
+
+  const code = await dispatch(['leave'], opts)
+  assert.equal(code, 0, stdout.text())
+
+  // The settings the attach really wrote are reversed, not stranded behind a
+  // marker whose status claimed nothing had happened.
+  const settings = JSON.parse(await fs.readFile(settingsPath, 'utf8'))
+  assert.equal('_hypaware' in settings, false)
+  assert.equal(settings.env?.ANTHROPIC_BASE_URL, undefined)
+  // And the user is told, because this reversal changed something.
+  assert.match(stdout.text(), /Detached claude/)
+  const markers = JSON.parse(await fs.readFile(path.join(controlDir, 'client-actions.json'), 'utf8'))
+  assert.equal(markers.attach, undefined)
+})
+
+test('leave stays quiet about a marker whose client has nothing left to reverse', async () => {
+  // #627's actual symptom, fixed at the reporting layer rather than by
+  // skipping the undo. Leave sweeps every attach marker on disk, so it reaches
+  // clients that hold nothing of ours: a refusal never wrote the client's
+  // settings at all. The undo still runs (it is what proves the file is
+  // clean), it reports `changed: false`, and a sweep must not turn that into a
+  // line naming a settings file the user never had. `hyp detach codex` still
+  // says it: there the line is the answer to what was asked.
+  const { home, stateRoot, stdout, opts } = await makeDispatchOpts()
+  assert.equal(
+    await dispatch(['join', 'https://central.example', 'policy-token-1', '--no-daemon'], opts),
+    0
+  )
+
+  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',
+          },
+        },
+      },
+      null,
+      2
+    ) + '\n'
+  )
+
+  const code = await dispatch(['leave'], opts)
+  assert.equal(code, 0, stdout.text())
+
+  assert.doesNotMatch(stdout.text(), /No HypAware marker found/)
+  assert.doesNotMatch(stdout.text(), /\.codex/)
+  // The quiet has to come from the reversal finding nothing, not from codex
+  // having no descriptor to reverse through: that path prints its own line.
+  assert.doesNotMatch(stdout.text(), /'codex' plugin not installed/)
+  // A refusal writes no settings file, and probing for one must not create it.
+  await assert.rejects(fs.stat(path.join(home, '.codex', 'config.toml')))
+
+  // The neighbour is unaffected: an attach that did apply still reverses, and
+  // says so, and both markers are gone.
+  const settings = JSON.parse(await fs.readFile(settingsPath, 'utf8'))
+  assert.equal('_hypaware' in settings, false)
+  assert.match(stdout.text(), /Detached claude/)
+  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(