From 2015d8add7c1d16a5beb67a0802241eccd96707d Mon Sep 17 00:00:00 2001 From: test Date: Tue, 4 Aug 2026 21:13:32 +0000 Subject: [PATCH 1/7] Unpicking an attached client is named, not silently left broken (#604) Re-running `hyp init` and unchecking a previously attached client regenerates a config without that client's adapter while the client's own settings still route through the HypAware gateway. The finale only attaches, and the action reconciler's reverse lane undoes only config-named (org/central) keys, so a wizard attach on the local layer is never reversed: capture stops and the client's requests can fail. Picking only OpenTelemetry composes no gateway at all, and every still-attached client points at a dead port after the finale's restart. The finale now names the clients it left stranded (attach marker on disk, not picked, enabled by neither the written config nor the central layer) and prints the `hyp detach --client ` that clears each. It does not detach: rewriting a client's settings file is not what unchecking a picker row asked for, and that call is the maintainer's to make (LLP 0185). `hyp status` gains the matching `client_attached_not_configured` diagnostic as the after-the-fact backstop, gated to solo hosts because on a joined host that shape is a reconciler pass that has not run yet. The org/central attach markers are untouched throughout. Co-Authored-By: Claude --- ...unpicked-client-stays-attached.decision.md | 105 ++++++++++++++ src/core/cli/types.d.ts | 8 ++ src/core/cli/walkthrough.js | 103 +++++++++++++- src/core/daemon/status.js | 18 +++ src/core/daemon/types.d.ts | 1 + ...tus-client-attached-not-configured.test.js | 88 ++++++++++++ test/core/status-first-sync-hold.test.js | 6 +- ...kthrough-unpicked-attached-clients.test.js | 130 ++++++++++++++++++ 8 files changed, 457 insertions(+), 2 deletions(-) create mode 100644 llp/0185-unpicked-client-stays-attached.decision.md create mode 100644 test/core/status-client-attached-not-configured.test.js create mode 100644 test/core/walkthrough-unpicked-attached-clients.test.js diff --git a/llp/0185-unpicked-client-stays-attached.decision.md b/llp/0185-unpicked-client-stays-attached.decision.md new file mode 100644 index 00000000..ab4e714d --- /dev/null +++ b/llp/0185-unpicked-client-stays-attached.decision.md @@ -0,0 +1,105 @@ +# LLP 0185: An unpicked client is named, not detached + +**Type:** Decision +**Status:** Accepted +**Systems:** Onboarding, CLI, Clients +**Author:** Brendan / Claude +**Date:** 2026-08-04 +**Related:** LLP 0045 (the one disk-driven undo), LLP 0129 (Reconfigure re-enters the picker), LLP 0086 (the existing attach drift diagnostic), LLP 0180 (the finale's attach lane), LLP 0031 (central layer) + +> Re-running `hyp init` and unchecking a client the previous run attached +> regenerates a config without that client's adapter, while the client's own +> settings still route through the HypAware gateway. Capture stops, and the +> requests themselves can fail. Nothing in the finale or the reconciler +> reverses that attach. + +## Context {#context} + +The wizard finale attaches picked clients and nothing else +([LLP 0180](./0180-finale-attaches-openclaw.decision.md)). The action +reconciler's reverse lane undoes **config-named** action keys, which is the +org/central lane: a solo machine's wizard-finale attach has no marker there +and is never reversed by it. A solo machine has been able to re-enter the +picker through Reconfigure since +[LLP 0129](./0129-init-wizard-fork.decision.md#returning-gate), so the state +is reachable on master. + +Two shapes, worst first: + +- Unchecking one of several clients drops that client's adapter and its + gateway upstream. The client still points at the gateway, which now has no + upstream matching it. +- Re-picking only OpenTelemetry composes no `@hypaware/ai-gateway` at all. + After the finale's daemon restart every still-attached client points at a + dead port. + +Neither shape is announced. `hyp status`'s only client drift check is the +port-mismatch one ([LLP 0086](./0086-attach-tracks-ephemeral-port.decision.md)), +which is scoped to clients that *are* configured. + +## Options considered {#options} + +1. **Detach in the finale.** Probe the previously attached clients the new + effective config no longer enables and run the one core undo + ([LLP 0045 §Part 3](./0045-client-attach.design.md)) on each. +2. **Warn in the finale.** Name each stranded client and the `hyp detach + --client ` that clears it. +3. **Warn in `hyp status`.** An attached-but-not-configured diagnostic, so a + run that has already closed still surfaces the state. + +## Decision {#decision} + +**The finale names what it left attached and +stops there (option 2, plus option 3 as the backstop).** After the config +write and before the daemon restart, the finale lists every client whose +settings still carry a HypAware attach marker, that this run did not pick, +and that neither the written config nor the central layer enables. It prints +the clients, the consequence, and one `hyp detach --client ` line each. + +Option 1 is deliberately **not** taken. Rewriting a client's settings file is +destructive and out of proportion to a menu confirm: unchecking a row in a +picker is not an instruction to edit `~/.codex/config.toml`. It also +overloads the picker's meaning, which up to now has only ever *added*. This +decision does not close option 1; it declines to make it on the user's behalf +inside a wizard step, and the warning is what makes choosing it later a +deliberate change rather than a bug fix. + +The warning **surfaces the breakage, it does not prevent it.** A user who +ignores it is in exactly the state the issue describes. That is the honest +cost of not detaching, and it is why the status backstop below exists. + +**The org/central lane is untouched.** A client whose +adapter the central layer names is never counted as stranded: its attach is +the reconciler's, forward and reverse +([LLP 0031](./0031-layered-config.decision.md)). The finale reads the central +layer read-only for the plugin names it declares, and nothing else. + +**`hyp status` warns on attached-but-not- +configured, on solo hosts only.** The diagnostic +(`client_attached_not_configured`) is the mirror of `client_attach_missing` +and repairs with `hyp detach --client `. On a joined host the same +shape is a reconciler pass that has not run yet, not a state the operator +should undo by hand, so it is gated on the absence of a central layer, the +same gate the client-action defaults already use. The known gap: a *local +layer* addition stranded on a managed host is not diagnosed by status. The +finale still warns about it at the time, which is where the state is created. + +## Consequences {#consequences} + +- `FinaleSummary` gains `attachedNotConfigured?: string[]`, so the run's + result carries what was printed. Optional, so a scripted finale runner (the + wizard's injectable seam, tests) need not synthesize it. +- The finale performs one extra descriptor-driven settings probe per unpicked + client, and one read of the central layer. Both are reads; a failure of + either degrades to "nothing stranded", which can only cost a missing + warning, never a wrong undo. +- `StatusDiagnosticKind` gains `client_attached_not_configured`. +- Option 1 remains available and is now cheap: the detection it needs is the + function this decision added, and the undo it needs already exists. + +## References + +- Issue #604; #603 covers the config-side blindness of the same flow +- `src/core/cli/walkthrough.js` (`findAttachedNotConfiguredClients`, the + finale warning), `src/core/daemon/status.js` (the diagnostic) +- LLP 0045, LLP 0129, LLP 0086, LLP 0180, LLP 0031 diff --git a/src/core/cli/types.d.ts b/src/core/cli/types.d.ts index cad52511..465846b8 100644 --- a/src/core/cli/types.d.ts +++ b/src/core/cli/types.d.ts @@ -243,6 +243,14 @@ export interface FinaleSummary { daemonRestart: { skipped: boolean; dryRun: boolean; ok: boolean } /** Per-provider onboarding backfill outcomes (empty when none ran). */ backfill: BackfillFinaleResult[] + /** + * Clients still carrying a HypAware attach marker that this run's config no + * longer enables: the state an unchecked-on-re-run client is left in. The + * finale names them and stops there (LLP 0185); the detach stays the user's + * to run. Optional so a scripted finale runner (tests, the wizard's + * injectable seam) need not synthesize it. + */ + attachedNotConfigured?: string[] } export interface PickerWalkthroughResult { diff --git a/src/core/cli/walkthrough.js b/src/core/cli/walkthrough.js index 9c206827..2c740e3f 100644 --- a/src/core/cli/walkthrough.js +++ b/src/core/cli/walkthrough.js @@ -5,8 +5,10 @@ import path from 'node:path' import readline from 'node:readline/promises' import { Attr, getLogger, withSpan } from '../observability/index.js' -import { defaultConfigPath, prepareLocalConfigWrite } from '../config/schema.js' +import { defaultConfigPath, loadConfigFile, prepareLocalConfigWrite } from '../config/schema.js' +import { resolveCentralLayerPath } from '../config/apply.js' import { DEFAULT_GATEWAY_ENDPOINT, configuredGatewayEndpoint } from '../config/gateway_endpoint.js' +import { probeClientAttachFromDescriptor } from '../daemon/status.js' import { readObservabilityEnv } from '../observability/env.js' import { discoverBundledPlugins } from '../runtime/bundled.js' import { materializeClientAssets } from '../runtime/client_assets.js' @@ -1011,6 +1013,24 @@ export async function runPickerFinale(args) { summary, }) + // Re-running the picker regenerates the config from the picks alone, so a + // client the previous run attached and this run left unchecked keeps routing + // through the gateway that no longer collects it. Nothing here undoes that: + // the finale only attaches, and the reconciler's reverse lane covers only the + // config-named org/central keys, never a wizard attach on the local layer. + // Naming the stranded clients before the restart is what keeps the breakage + // visible; the undo stays the user's to run. + // @ref LLP 0185#warn-do-not-detach [implements]: the finale names what it left attached and stops there + summary.attachedNotConfigured = await findAttachedNotConfiguredClients({ + clientsPicked, + config, + env, + homeDir, + }) + if (summary.attachedNotConfigured.length > 0) { + writeAttachedNotConfiguredWarning({ clients: summary.attachedNotConfigured, stdout, dryRun }) + } + if (!finale.skipDaemon && !finale.skipDaemonRestart && !dryRun) { try { const { restartServiceDaemon } = await import('../daemon/install.js') @@ -1029,6 +1049,87 @@ export async function runPickerFinale(args) { return summary } +/** + * The plugin names the org's central layer declares, read-only and + * best-effort. A centrally named client is attached and reversed by the + * reconciler, not by the wizard, so it must never be counted as stranded by + * the local layer's picks. Any failure to read the layer degrades to "no + * central plugins", which can only cost an extra warning, never a wrong undo. + * + * @ref LLP 0031#central-layer-is-sacrosanct [constrained-by]: the central layer is read here, never written or interpreted beyond its plugin names + * @param {NodeJS.ProcessEnv} env + * @returns {Promise>} + */ +async function readCentralPluginNames(env) { + try { + const { stateDir } = readObservabilityEnv(env) + const centralPath = resolveCentralLayerPath({ stateRoot: stateDir }) + if (!centralPath) return new Set() + const loaded = await loadConfigFile(centralPath) + if (!loaded.ok) return new Set() + return new Set((loaded.config.plugins ?? []).map((entry) => entry.name)) + } catch { + return new Set() + } +} + +/** + * The clients this run leaves stranded: their settings still carry a HypAware + * attach marker, they are not among this run's picks, and neither the config + * the finale just wrote nor the org's central layer enables their adapter. The + * marker is read through the same descriptor-driven probe `hyp status` uses, so + * "still attached" means exactly what the status surface means by it. + * + * @param {{ + * clientsPicked: string[], + * config: HypAwareV2Config, + * env: NodeJS.ProcessEnv, + * homeDir: string, + * }} args + * @returns {Promise} stranded client names, in catalog order + * @ref LLP 0045#part-3-reverse-runs-from-disk-the-marker-is-a-self-describing-undo-record [constrained-by]: the on-disk marker is the only evidence of a prior attach, so it is what the check reads + */ +export async function findAttachedNotConfiguredClients({ clientsPicked, config, env, homeDir }) { + if (!homeDir) return [] + const picked = new Set(clientsPicked) + const configured = new Set((config.plugins ?? []).map((entry) => entry.name)) + for (const name of await readCentralPluginNames(env)) configured.add(name) + + const descriptors = await buildWalkthroughClientDescriptorMap() + /** @type {string[]} */ + const stranded = [] + for (const [clientName, descriptor] of descriptors) { + if (picked.has(clientName) || configured.has(descriptor.plugin)) continue + if (!descriptor.attachProbe) continue + const probe = await probeClientAttachFromDescriptor({ descriptor, homeDir, env }) + if (probe.attached) stranded.push(clientName) + } + return stranded +} + +/** + * Name the stranded clients and the one command that clears each. The wizard + * says what it found and what to run; it does not run it, because rewriting a + * client's settings is not something a menu confirm asked for. + * + * A dry run carries the same tag the rest of the finale uses: the clients + * really are attached, but the config that strands them was not written. + * + * @param {{ + * clients: string[], + * stdout: NodeJS.WritableStream | { write(chunk: string): unknown }, + * dryRun: boolean, + * }} args + */ +function writeAttachedNotConfiguredWarning({ clients, stdout, dryRun }) { + stdout.write('\n') + stdout.write(`${dryRun ? '(dry-run) ' : ''}Still attached, no longer collected: ${clients.join(', ')}\n`) + stdout.write('These tools still send their requests through the HypAware gateway,\n') + stdout.write('but this setup no longer collects them, so their requests can start\n') + stdout.write('failing. Point each one back at its provider with:\n') + for (const client of clients) stdout.write(` hyp detach --client ${client}\n`) +} + /** * Run the onboarding backfill step. For each picked client that has a * registered backfill provider (intersection of `clientsPicked` and diff --git a/src/core/daemon/status.js b/src/core/daemon/status.js index 44774c67..d57c34c9 100644 --- a/src/core/daemon/status.js +++ b/src/core/daemon/status.js @@ -638,6 +638,24 @@ export async function collectHypAwareStatus(opts = {}) { message: `${clientName} is attached at port ${probe.port} but the gateway is now bound to port ${liveGatewayPort} - run 'hyp attach --client ${clientName}' to re-point it`, repair: [`hyp attach --client ${clientName}`], }) + } else if (!configured && probe.attached && !hasCentral) { + // The mirror image of `client_attach_missing`: the marker is on disk but + // nothing enables the adapter, so this client still routes through a + // gateway that no longer collects it (and, with no gateway configured at + // all, through a dead port). Re-running the picker and unchecking a + // previously picked client is the way in; the wizard warns at the time, + // and this is the after-the-fact backstop for a run already closed. + // + // Solo hosts only. On a joined host a config-named client's attach is the + // reconciler's to reverse, so the same shape there is a pass it has not + // run yet, not a state the operator should undo by hand. + // @ref LLP 0185#status-backstop [implements]: attached-but-not-configured is a warning on solo hosts, and the reconciler's business on managed ones + diagnostics.push({ + severity: 'warning', + kind: 'client_attached_not_configured', + message: `${clientName} settings still point at the HypAware gateway but '${descriptor.plugin}' is not enabled - its requests are no longer collected and can fail; run 'hyp detach --client ${clientName}' to unhook it`, + repair: [`hyp detach --client ${clientName}`], + }) } } diff --git a/src/core/daemon/types.d.ts b/src/core/daemon/types.d.ts index f354ce9e..05c62d2d 100644 --- a/src/core/daemon/types.d.ts +++ b/src/core/daemon/types.d.ts @@ -97,6 +97,7 @@ export type StatusDiagnosticKind = | 'daemon_loaded_no_pid' | 'client_attach_missing' | 'client_attach_stale' + | 'client_attached_not_configured' | 'gateway_port_fallback' | 'recent_errors' | 'remote_config_rolled_back' diff --git a/test/core/status-client-attached-not-configured.test.js b/test/core/status-client-attached-not-configured.test.js new file mode 100644 index 00000000..8adc3563 --- /dev/null +++ b/test/core/status-client-attached-not-configured.test.js @@ -0,0 +1,88 @@ +// @ts-check + +import test from 'node:test' +import assert from 'node:assert/strict' +import fs from 'node:fs/promises' +import os from 'node:os' +import path from 'node:path' + +import { collectHypAwareStatus } from '../../src/core/daemon/status.js' +import { defaultConfigPath } from '../../src/core/config/schema.js' +import { centralSeedPath } from '../../src/core/config/apply.js' + +// The mirror image of `client_attach_missing`: a client whose settings still +// carry the attach marker while nothing enables its adapter (issue #604). The +// wizard warns when it creates that state; this is the after-the-fact backstop. +// @ref LLP 0185#status-backstop [tests]: + +async function makeHome() { + const hypHome = await fs.mkdtemp(path.join(os.tmpdir(), 'hyp-status-stranded-')) + await fs.mkdir(path.join(hypHome, 'hypaware'), { recursive: true }) + return hypHome +} + +/** @param {string} hypHome */ +function env(hypHome) { + return { ...process.env, HYP_HOME: hypHome, HYP_CONFIG: '' } +} + +/** + * A home whose `~/.codex/config.toml` carries the managed block a previous + * attach wrote: the marker the codex `attach_probe` reads back. + */ +async function homeWithAttachedCodex() { + const home = await fs.mkdtemp(path.join(os.tmpdir(), 'hyp-status-stranded-home-')) + await fs.mkdir(path.join(home, '.codex'), { recursive: true }) + await fs.writeFile( + path.join(home, '.codex', 'config.toml'), + '# BEGIN hypaware\n[model_providers.hypaware]\nname = "hypaware"\n# END hypaware\n', + 'utf8' + ) + return home +} + +test('a client attached with no plugin enabling it is a warning naming the detach', async () => { + const hypHome = await makeHome() + const homeDir = await homeWithAttachedCodex() + await fs.writeFile(defaultConfigPath(hypHome), JSON.stringify({ version: 2, plugins: [] }) + '\n') + + const report = await collectHypAwareStatus({ env: env(hypHome), homeDir }) + + const found = report.diagnostics.find((d) => d.kind === 'client_attached_not_configured') + assert.ok(found, JSON.stringify(report.diagnostics, null, 2)) + assert.equal(found.severity, 'warning') + assert.match(found.message, /codex/) + assert.deepEqual(found.repair, ['hyp detach --client codex']) +}) + +test('a configured attached client draws no stranded diagnostic', async () => { + const hypHome = await makeHome() + const homeDir = await homeWithAttachedCodex() + await fs.writeFile(defaultConfigPath(hypHome), JSON.stringify({ + version: 2, + plugins: [{ name: '@hypaware/ai-gateway' }, { name: '@hypaware/codex' }], + }) + '\n') + + const report = await collectHypAwareStatus({ env: env(hypHome), homeDir }) + + assert.equal(report.diagnostics.some((d) => d.kind === 'client_attached_not_configured'), false) +}) + +// The org/central attach markers are the reconciler's to reverse, so a managed +// host's config-drop is a pass it has not run yet, never an operator's detach. +test('a managed host leaves the reverse lane to the reconciler and stays quiet', async () => { + const hypHome = await makeHome() + const homeDir = await homeWithAttachedCodex() + const seedPath = centralSeedPath(path.join(hypHome, 'hypaware')) + await fs.mkdir(path.dirname(seedPath), { recursive: true }) + await fs.writeFile(seedPath, JSON.stringify({ + version: 2, + plugins: [{ name: '@hypaware/central' }], + }) + '\n') + await fs.writeFile(defaultConfigPath(hypHome), JSON.stringify({ version: 2, plugins: [] }) + '\n') + + const report = await collectHypAwareStatus({ env: env(hypHome), homeDir }) + + assert.equal(report.layered?.hasCentral, true) + assert.equal(report.diagnostics.some((d) => d.kind === 'client_attached_not_configured'), false) +}) diff --git a/test/core/status-first-sync-hold.test.js b/test/core/status-first-sync-hold.test.js index 8325a37e..9f080de4 100644 --- a/test/core/status-first-sync-hold.test.js +++ b/test/core/status-first-sync-hold.test.js @@ -103,7 +103,11 @@ test('a corrupt marker fails open (absent, no diagnostic, no degrade) - LLP 0101 await fs.mkdir(path.dirname(markerPath), { recursive: true }) await fs.writeFile(markerPath, '{ not valid json', 'utf8') - const report = await collectHypAwareStatus({ env: env(hypHome) }) + // An empty `homeDir`: this assertion is that the *diagnostics list* is + // untouched, so the client probes must read a home with no attach markers. + // Inheriting the runner's real `$HOME` made the assertion depend on whether + // whoever ran the suite happens to have HypAware attached. + const report = await collectHypAwareStatus({ env: env(hypHome), homeDir: hypHome }) assert.equal(report.firstSyncHoldDeadline, null) assert.equal(report.overall, 'healthy', 'a corrupt marker is timing-only; it never degrades overall (fail-open)') assert.deepEqual(report.diagnostics, [], 'a corrupt hold marker mints no diagnostic - it is timing-only, not a privacy signal') diff --git a/test/core/walkthrough-unpicked-attached-clients.test.js b/test/core/walkthrough-unpicked-attached-clients.test.js new file mode 100644 index 00000000..ad4f7e94 --- /dev/null +++ b/test/core/walkthrough-unpicked-attached-clients.test.js @@ -0,0 +1,130 @@ +// @ts-check + +import test from 'node:test' +import assert from 'node:assert/strict' +import fs from 'node:fs/promises' +import os from 'node:os' +import path from 'node:path' + +import { runPickerWalkthrough } from '../../src/core/cli/walkthrough.js' + +// Re-running the picker and unchecking a client the previous run attached +// leaves that client's settings pointing at the gateway while the regenerated +// config no longer collects it (issue #604). The finale cannot silently undo +// the attach, so it must at least name the stranded client and the detach +// command that clears it. + +function makeBuf() { + let value = '' + return { + write(/** @type {string} */ chunk) { + value += String(chunk) + return true + }, + text() { + return value + }, + } +} + +/** @param {string} prefix */ +async function tmpEnv(prefix) { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), prefix)) + return { HOME: tmp, HYP_HOME: path.join(tmp, '.hyp') } +} + +/** + * Write the on-disk marker a previous run's `codex` attach left behind: the + * managed block its `attach_probe` (`toml` / `[model_providers.hypaware]`) + * reads back. + * + * @param {string} home + */ +async function writeCodexAttachMarker(home) { + const dir = path.join(home, '.codex') + await fs.mkdir(dir, { recursive: true }) + await fs.writeFile( + path.join(dir, 'config.toml'), + [ + 'model_provider = "hypaware"', + '', + '# BEGIN hypaware', + '[model_providers.hypaware]', + 'name = "hypaware"', + 'base_url = "http://127.0.0.1:4319/openai/v1"', + '# END hypaware', + '', + ].join('\n'), + 'utf8' + ) +} + +/** Gateway capability stub with no adapters: the attach lane is not the subject. */ +function gatewayCapability() { + return /** @type {any} */ ({ + has: () => false, + require: () => ({ getClient: () => undefined, localEndpoint: () => 'http://127.0.0.1:4319' }), + }) +} + +test('the finale names a still-attached client the new config no longer collects', async () => { + const env = await tmpEnv('hypaware-unpicked-attached-') + await writeCodexAttachMarker(env.HOME) + const stdout = makeBuf() + const stderr = makeBuf() + + const result = await runPickerWalkthrough({ + capabilities: gatewayCapability(), + stdout, + stderr, + env, + picks: { sources: ['claude'], exportChoice: 'keep-local', retentionDays: 30 }, + finale: { skipDaemon: true }, + }) + + assert.equal(result.exitCode, 0) + assert.deepEqual(result.finale?.attachedNotConfigured, ['codex']) + const out = stdout.text() + assert.match(out, /codex/, out) + assert.match(out, /hyp detach --client codex/, out) +}) + +test('a picked client that stays configured draws no stranded-attach warning', async () => { + const env = await tmpEnv('hypaware-unpicked-attached-ok-') + await writeCodexAttachMarker(env.HOME) + const stdout = makeBuf() + const stderr = makeBuf() + + const result = await runPickerWalkthrough({ + capabilities: gatewayCapability(), + stdout, + stderr, + env, + picks: { sources: ['codex'], exportChoice: 'keep-local', retentionDays: 30 }, + finale: { skipDaemon: true }, + }) + + assert.equal(result.exitCode, 0) + assert.deepEqual(result.finale?.attachedNotConfigured, []) + assert.doesNotMatch(stdout.text(), /hyp detach --client/, stdout.text()) + assert.doesNotMatch(stderr.text(), /hyp detach --client/, stderr.text()) +}) + +test('an unattached client the picker skipped is not warned about', async () => { + const env = await tmpEnv('hypaware-unpicked-unattached-') + const stdout = makeBuf() + const stderr = makeBuf() + + const result = await runPickerWalkthrough({ + capabilities: gatewayCapability(), + stdout, + stderr, + env, + picks: { sources: ['claude'], exportChoice: 'keep-local', retentionDays: 30 }, + finale: { skipDaemon: true }, + }) + + assert.equal(result.exitCode, 0) + assert.deepEqual(result.finale?.attachedNotConfigured, []) + assert.doesNotMatch(stdout.text(), /hyp detach --client/, stdout.text()) +}) From a70dc445144160e22fcac954381401e68f1b54ca Mon Sep 17 00:00:00 2001 From: test Date: Tue, 4 Aug 2026 22:07:16 +0000 Subject: [PATCH 2/7] Review fixes: the stranded-attach check reads intent, not accidents (#604) Three defects in the #604 fix, found in neutral review. An unreadable local config emptied the active-plugin set for a reason that has nothing to do with what the operator enabled, and the new `client_attached_not_configured` branch read that as "nothing enables this adapter": `hyp status` answered a parse failure with a `hyp detach` for every attached client, stacked on top of the `config_unreadable` error that is the actual repair. The branch now stands down when the local layer is present but did not parse. `findAttachedNotConfiguredClients` counted a plugin entry with `enabled: false` as configured, while `hyp status` builds the same set from `activePlugins`, which skips it. A switched-off adapter collects exactly as little as an absent one, so the two surfaces disagreed about the same file. LLP 0185 records both gates, and the forward-refs the corpus convention wants land on the docs it extends: LLP 0180 (the finale's attach lane) and LLP 0086 (the client drift diagnostic family). The status test's `@ref` carried an empty gloss; it now says what it tests. Tests: an unreadable local config draws no detach advice; a disabled plugin entry strands its client; and the central-layer suppression LLP 0185 #scope promises for the finale, which had no coverage on the walkthrough side. Both new gates fail without the src change. npm test: 3386 tests / 3385 pass / 0 fail / 1 skipped. npm run typecheck clean. Smokes status_diagnostics and walkthrough_picker_to_first_query ok. Co-Authored-By: Claude --- ...6-attach-tracks-ephemeral-port.decision.md | 2 +- llp/0180-finale-attaches-openclaw.decision.md | 1 + ...unpicked-client-stays-attached.decision.md | 18 +++++-- src/core/cli/walkthrough.js | 9 +++- src/core/daemon/status.js | 15 +++++- ...tus-client-attached-not-configured.test.js | 21 +++++++- ...kthrough-unpicked-attached-clients.test.js | 52 ++++++++++++++++++- 7 files changed, 109 insertions(+), 9 deletions(-) diff --git a/llp/0086-attach-tracks-ephemeral-port.decision.md b/llp/0086-attach-tracks-ephemeral-port.decision.md index 92a926e4..e2d87e56 100644 --- a/llp/0086-attach-tracks-ephemeral-port.decision.md +++ b/llp/0086-attach-tracks-ephemeral-port.decision.md @@ -6,7 +6,7 @@ **Generated-by:** neutral **Date:** 2026-07-07 **Related:** LLP 0016, LLP 0036, LLP 0041, LLP 0044, LLP 0045 -**Extended-by:** LLP 0114 (the default listen port is now fixed; the ephemeral port and this tracking machinery remain as the fallback) +**Extended-by:** LLP 0114 (the default listen port is now fixed; the ephemeral port and this tracking machinery remain as the fallback), [LLP 0185](./0185-unpicked-client-stays-attached.decision.md) (the client drift family gains `client_attached_not_configured`, the mirror of `client_attach_missing`, for a marker no configured adapter accounts for) > [Issue #277](https://github.com/hyparam/hypaware/issues/277) (a real user > report): the daemon's gateway binds an **ephemeral** port by default diff --git a/llp/0180-finale-attaches-openclaw.decision.md b/llp/0180-finale-attaches-openclaw.decision.md index a11f7c5d..f22752b7 100644 --- a/llp/0180-finale-attaches-openclaw.decision.md +++ b/llp/0180-finale-attaches-openclaw.decision.md @@ -6,6 +6,7 @@ **Author:** Brendan / Claude **Date:** 2026-08-03 **Related:** LLP 0177 (the issue this resolves), LLP 0169 (OpenClaw attach surface), LLP 0174 (manual-path sibling; its non-goal), LLP 0170 (sweep consent stance), LLP 0130 (manifest-sourced picker), LLP 0011 (init finale), LLP 0135 (wizard orchestration), LLP 0115 (Claude Desktop is not an attach-on-join client) +**Extended-by:** [LLP 0185](./0185-unpicked-client-stays-attached.decision.md) (the finale still only attaches; a re-run that leaves a previously attached client unpicked now names it and the detach that clears it) > Picking OpenClaw in `hyp init` enables the adapter and then drops it > from the finale: no attach, no import, a `client_attach_missing` limbo diff --git a/llp/0185-unpicked-client-stays-attached.decision.md b/llp/0185-unpicked-client-stays-attached.decision.md index ab4e714d..dac38db1 100644 --- a/llp/0185-unpicked-client-stays-attached.decision.md +++ b/llp/0185-unpicked-client-stays-attached.decision.md @@ -80,9 +80,21 @@ configured, on solo hosts only.** The diagnostic and repairs with `hyp detach --client `. On a joined host the same shape is a reconciler pass that has not run yet, not a state the operator should undo by hand, so it is gated on the absence of a central layer, the -same gate the client-action defaults already use. The known gap: a *local -layer* addition stranded on a managed host is not diagnosed by status. The -finale still warns about it at the time, which is where the state is created. +same gate the client-action defaults already use. It is gated a second time +on the local layer having *parsed*: a config file that is present but +unreadable empties the active-plugin set for a reason that says nothing about +what the operator enabled, and answering a parse failure with a detach for +every attached client would be reading an accident as intent, stacked on top +of the `config_unreadable` finding that is the real repair. The known gap: a +*local layer* addition stranded on a managed host is not diagnosed by status. +The finale still warns about it at the time, which is where the state is +created. + +**"Not configured" means what +`hyp status` already means by it.** Both surfaces read a plugin entry with +`enabled: false` as inactive, so a switched-off adapter strands its client +exactly as an absent one does. The finale and the diagnostic must agree about +the same config file; a set built two ways would eventually disagree. ## Consequences {#consequences} diff --git a/src/core/cli/walkthrough.js b/src/core/cli/walkthrough.js index 2c740e3f..6c692617 100644 --- a/src/core/cli/walkthrough.js +++ b/src/core/cli/walkthrough.js @@ -1092,7 +1092,14 @@ async function readCentralPluginNames(env) { export async function findAttachedNotConfiguredClients({ clientsPicked, config, env, homeDir }) { if (!homeDir) return [] const picked = new Set(clientsPicked) - const configured = new Set((config.plugins ?? []).map((entry) => entry.name)) + // `enabled: false` is what `hyp status` reads as "not active" when it builds + // the same set, and an entry left in the file with the switch off collects + // exactly as little as an absent one. Reading it as configured here would + // make the two surfaces disagree about the same config. + // @ref LLP 0185#not-configured-means-not-active [implements]: a disabled plugin entry strands its client exactly as an absent one does + const configured = new Set( + (config.plugins ?? []).filter((entry) => entry.enabled !== false).map((entry) => entry.name) + ) for (const name of await readCentralPluginNames(env)) configured.add(name) const descriptors = await buildWalkthroughClientDescriptorMap() diff --git a/src/core/daemon/status.js b/src/core/daemon/status.js index d57c34c9..465953a7 100644 --- a/src/core/daemon/status.js +++ b/src/core/daemon/status.js @@ -301,6 +301,14 @@ export async function collectHypAwareStatus(opts = {}) { // outage. `configExists` tracks whether *anything* is configured. const configExists = config !== null + // A local layer that is present but does not parse: `activePlugins` is then + // empty (or central-only) because the file could not be read, not because + // the operator disabled anything. Any diagnostic whose repair is "your + // config no longer names this" would be reading a parse failure as intent, + // so the attached-but-not-configured check below stands down here and lets + // `config_unreadable` / `config_local_unreadable` own the run. + const localConfigUnreadable = !localLoaded.ok && localLoaded.errorKind !== 'config_missing' + // Validate the *effective* (merged + pruned) config: that is what runs. // After pruning, any error left is the central layer's own (apply-time's // concern); a local entry that lost the merge shows in `layered.drops`, @@ -638,7 +646,7 @@ export async function collectHypAwareStatus(opts = {}) { message: `${clientName} is attached at port ${probe.port} but the gateway is now bound to port ${liveGatewayPort} - run 'hyp attach --client ${clientName}' to re-point it`, repair: [`hyp attach --client ${clientName}`], }) - } else if (!configured && probe.attached && !hasCentral) { + } else if (!configured && probe.attached && !hasCentral && !localConfigUnreadable) { // The mirror image of `client_attach_missing`: the marker is on disk but // nothing enables the adapter, so this client still routes through a // gateway that no longer collects it (and, with no gateway configured at @@ -648,7 +656,10 @@ export async function collectHypAwareStatus(opts = {}) { // // Solo hosts only. On a joined host a config-named client's attach is the // reconciler's to reverse, so the same shape there is a pass it has not - // run yet, not a state the operator should undo by hand. + // run yet, not a state the operator should undo by hand. And only when + // the local layer actually parsed: an unreadable file says nothing about + // what the operator enabled, so "not configured" would be a guess, and + // detaching on a guess is the one irreversible thing here. // @ref LLP 0185#status-backstop [implements]: attached-but-not-configured is a warning on solo hosts, and the reconciler's business on managed ones diagnostics.push({ severity: 'warning', diff --git a/test/core/status-client-attached-not-configured.test.js b/test/core/status-client-attached-not-configured.test.js index 8adc3563..38b4f740 100644 --- a/test/core/status-client-attached-not-configured.test.js +++ b/test/core/status-client-attached-not-configured.test.js @@ -13,7 +13,7 @@ import { centralSeedPath } from '../../src/core/config/apply.js' // The mirror image of `client_attach_missing`: a client whose settings still // carry the attach marker while nothing enables its adapter (issue #604). The // wizard warns when it creates that state; this is the after-the-fact backstop. -// @ref LLP 0185#status-backstop [tests]: +// @ref LLP 0185#status-backstop [tests]: the diagnostic fires on a solo host, stays quiet on a joined one, and never reads an unparseable local layer as intent async function makeHome() { const hypHome = await fs.mkdtemp(path.join(os.tmpdir(), 'hyp-status-stranded-')) @@ -86,3 +86,22 @@ test('a managed host leaves the reverse lane to the reconciler and stays quiet', assert.equal(report.layered?.hasCentral, true) assert.equal(report.diagnostics.some((d) => d.kind === 'client_attached_not_configured'), false) }) + +// A local layer that does not parse leaves the active-plugin set empty for a +// reason that has nothing to do with what the operator enabled. Reading that +// as "not configured" would hand back a detach for every attached client, on +// top of the config error that is the actual repair. +test('an unreadable local config is not read as an instruction to detach', async () => { + const hypHome = await makeHome() + const homeDir = await homeWithAttachedCodex() + await fs.writeFile(defaultConfigPath(hypHome), '{ not valid json', 'utf8') + + const report = await collectHypAwareStatus({ env: env(hypHome), homeDir }) + + assert.equal(report.diagnostics.some((d) => d.kind === 'config_unreadable'), true) + assert.equal( + report.diagnostics.some((d) => d.kind === 'client_attached_not_configured'), + false, + JSON.stringify(report.diagnostics, null, 2) + ) +}) diff --git a/test/core/walkthrough-unpicked-attached-clients.test.js b/test/core/walkthrough-unpicked-attached-clients.test.js index ad4f7e94..ff6ddcea 100644 --- a/test/core/walkthrough-unpicked-attached-clients.test.js +++ b/test/core/walkthrough-unpicked-attached-clients.test.js @@ -6,7 +6,9 @@ import fs from 'node:fs/promises' import os from 'node:os' import path from 'node:path' -import { runPickerWalkthrough } from '../../src/core/cli/walkthrough.js' +import { findAttachedNotConfiguredClients, runPickerWalkthrough } from '../../src/core/cli/walkthrough.js' +import { centralSeedPath } from '../../src/core/config/apply.js' +import { readObservabilityEnv } from '../../src/core/observability/env.js' // Re-running the picker and unchecking a client the previous run attached // leaves that client's settings pointing at the gateway while the regenerated @@ -128,3 +130,51 @@ test('an unattached client the picker skipped is not warned about', async () => assert.deepEqual(result.finale?.attachedNotConfigured, []) assert.doesNotMatch(stdout.text(), /hyp detach --client/, stdout.text()) }) + +// The org lane is the reconciler's, forward and reverse: an adapter the central +// layer names is attached on its behalf, so the wizard's own picks must never +// be read as a reason to hand the operator a detach for it. +// @ref LLP 0185#scope [tests]: a centrally named adapter is never counted stranded by the local layer's picks +test('a client the central layer names is not stranded by an unpicking run', async () => { + const env = await tmpEnv('hypaware-unpicked-central-') + await writeCodexAttachMarker(env.HOME) + const seedPath = centralSeedPath(readObservabilityEnv(env).stateDir) + await fs.mkdir(path.dirname(seedPath), { recursive: true }) + await fs.writeFile( + seedPath, + JSON.stringify({ version: 2, plugins: [{ name: '@hypaware/codex' }] }) + '\n', + 'utf8' + ) + const stdout = makeBuf() + const stderr = makeBuf() + + const result = await runPickerWalkthrough({ + capabilities: gatewayCapability(), + stdout, + stderr, + env, + picks: { sources: ['claude'], exportChoice: 'keep-local', retentionDays: 30 }, + finale: { skipDaemon: true }, + }) + + assert.equal(result.exitCode, 0) + assert.deepEqual(result.finale?.attachedNotConfigured, []) + assert.doesNotMatch(stdout.text(), /hyp detach --client/, stdout.text()) +}) + +// `hyp status` reads `enabled: false` as inactive when it builds the same set, +// and a switched-off entry collects exactly as little as an absent one. The two +// surfaces must not disagree about the same config. +test('a plugin entry left in the config with enabled false does not count as configured', async () => { + const env = await tmpEnv('hypaware-unpicked-disabled-') + await writeCodexAttachMarker(env.HOME) + + const stranded = await findAttachedNotConfiguredClients({ + clientsPicked: ['claude'], + config: { version: 2, plugins: [{ name: '@hypaware/codex', enabled: false }] }, + env, + homeDir: env.HOME, + }) + + assert.deepEqual(stranded, ['codex']) +}) From d6eaa7d284ad26a392879f4e73288787d280107d Mon Sep 17 00:00:00 2001 From: test Date: Tue, 4 Aug 2026 22:47:28 +0000 Subject: [PATCH 3/7] Review round 2: pin the config_missing carve-out, keep the central read unfiltered (#604) The status gate stands down on a local layer that is present but does not parse, and deliberately does not stand down when there is no config at all: nothing collects then, so a marker still on disk really is stranded. That boundary (`errorKind !== 'config_missing'`, not `!ok`) had no test, so widening the gate would have dropped the case silently. Adds one. `readCentralPluginNames` is deliberately not filtered by `enabled`, unlike the local set the caller builds: LLP 0185 #scope settles the org lane on the name alone, and filtering would hand the operator a detach for an org-owned client. Says so at the read, and scopes #not-configured-means-not-active to the local layer so the two sections cannot be read against each other. Co-Authored-By: Claude --- ...5-unpicked-client-stays-attached.decision.md | 13 +++++++++---- src/core/cli/walkthrough.js | 6 ++++++ ...tatus-client-attached-not-configured.test.js | 17 +++++++++++++++++ 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/llp/0185-unpicked-client-stays-attached.decision.md b/llp/0185-unpicked-client-stays-attached.decision.md index dac38db1..d70898bc 100644 --- a/llp/0185-unpicked-client-stays-attached.decision.md +++ b/llp/0185-unpicked-client-stays-attached.decision.md @@ -91,10 +91,15 @@ The finale still warns about it at the time, which is where the state is created. **"Not configured" means what -`hyp status` already means by it.** Both surfaces read a plugin entry with -`enabled: false` as inactive, so a switched-off adapter strands its client -exactly as an absent one does. The finale and the diagnostic must agree about -the same config file; a set built two ways would eventually disagree. +`hyp status` already means by it.** Both surfaces read a **local layer** plugin +entry with `enabled: false` as inactive, so a switched-off adapter strands its +client exactly as an absent one does. The finale and the diagnostic must agree +about the same config file; a set built two ways would eventually disagree. +This is about the local layer only: the org lane is settled by +[§scope](#scope) above, which is stronger. A client the central layer *names* +is never counted as stranded whatever its `enabled` flag says, because +reversing that attach is the reconciler's job either way, and the finale is +not the surface that gets to second-guess it. ## Consequences {#consequences} diff --git a/src/core/cli/walkthrough.js b/src/core/cli/walkthrough.js index 6c692617..263dfd48 100644 --- a/src/core/cli/walkthrough.js +++ b/src/core/cli/walkthrough.js @@ -1056,6 +1056,12 @@ export async function runPickerFinale(args) { * the local layer's picks. Any failure to read the layer degrades to "no * central plugins", which can only cost an extra warning, never a wrong undo. * + * Deliberately *not* filtered by `enabled`, unlike the local set the caller + * builds: what matters here is that the org named the plugin at all, because a + * name is what makes the attach the reconciler's to reverse. Adding the filter + * would hand the operator a detach for an org-owned client. + * + * @ref LLP 0185#scope [constrained-by]: a centrally named adapter is never counted stranded, whatever its enabled flag says * @ref LLP 0031#central-layer-is-sacrosanct [constrained-by]: the central layer is read here, never written or interpreted beyond its plugin names * @param {NodeJS.ProcessEnv} env * @returns {Promise>} diff --git a/test/core/status-client-attached-not-configured.test.js b/test/core/status-client-attached-not-configured.test.js index 38b4f740..2137097b 100644 --- a/test/core/status-client-attached-not-configured.test.js +++ b/test/core/status-client-attached-not-configured.test.js @@ -105,3 +105,20 @@ test('an unreadable local config is not read as an instruction to detach', async JSON.stringify(report.diagnostics, null, 2) ) }) + +// The other side of that gate, and the reason it is `errorKind !== +// 'config_missing'` rather than `!ok`: no config at all is not an accident the +// diagnostic has to read around. Nothing collects, so a marker still on disk +// really is stranded and the detach really is the repair. Widening the gate to +// every unreadable load would drop this case silently. +test('a missing config still names the client its marker strands', async () => { + const hypHome = await makeHome() + const homeDir = await homeWithAttachedCodex() + + const report = await collectHypAwareStatus({ env: env(hypHome), homeDir }) + + assert.equal(report.diagnostics.some((d) => d.kind === 'config_missing'), true) + const found = report.diagnostics.find((d) => d.kind === 'client_attached_not_configured') + assert.ok(found, JSON.stringify(report.diagnostics, null, 2)) + assert.deepEqual(found.repair, ['hyp detach --client codex']) +}) From ce11b6729941e56f08662f5fb41caf58bf6ff7dc Mon Sep 17 00:00:00 2001 From: test Date: Wed, 5 Aug 2026 00:22:09 +0000 Subject: [PATCH 4/7] The stranded-attach warning repeats where the run ends (#614) The finale names the clients this run left attached but no longer collects, before the daemon restart (LLP 0185). `runInitWizard` then writes the run summary, a first look that is roughly sixty lines of real query output, and on the team path the privacy narration, none of which pause, so on a real terminal the warning is gone by the time setup ends. It is buried worst where it is the only signal there is: on a managed host `hyp status`'s mirror diagnostic is gated to hosts with no central layer. The wizard now repeats it, short, after the first look and before the privacy narration. The repeat is the caller's rather than the shared `writeWalkthroughRunSummary`'s, because only the caller knows whether anything came between: `runPickerWalkthrough` writes a short summary and stops, so it keeps its single print instead of showing the same paragraph twice within a few lines of itself. LLP 0188 records the print contract and carries the forward-ref on LLP 0185. Co-Authored-By: Claude --- ...unpicked-client-stays-attached.decision.md | 7 ++ ...ach-warning-repeats-at-the-end.decision.md | 99 +++++++++++++++++++ src/core/cli/walkthrough.js | 32 ++++++ src/core/cli/wizard/index.js | 27 ++++- test/core/cli/wizard/index.test.js | 71 +++++++++++++ ...kthrough-unpicked-attached-clients.test.js | 7 ++ 6 files changed, 241 insertions(+), 2 deletions(-) create mode 100644 llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md diff --git a/llp/0185-unpicked-client-stays-attached.decision.md b/llp/0185-unpicked-client-stays-attached.decision.md index d70898bc..7486c705 100644 --- a/llp/0185-unpicked-client-stays-attached.decision.md +++ b/llp/0185-unpicked-client-stays-attached.decision.md @@ -6,6 +6,7 @@ **Author:** Brendan / Claude **Date:** 2026-08-04 **Related:** LLP 0045 (the one disk-driven undo), LLP 0129 (Reconfigure re-enters the picker), LLP 0086 (the existing attach drift diagnostic), LLP 0180 (the finale's attach lane), LLP 0031 (central layer) +**Extended-by:** LLP 0188 (the finale's print stays where §warn-do-not-detach put it; the wizard, whose closing sequence buries it, repeats it short at the end of the run) > Re-running `hyp init` and unchecking a client the previous run attached > regenerates a config without that client's adapter, while the client's own @@ -90,6 +91,12 @@ of the `config_unreadable` finding that is the real repair. The known gap: a The finale still warns about it at the time, which is where the state is created. +> **Extended-by [LLP 0188](./0188-stranded-attach-warning-repeats-at-the-end.decision.md):** +> the finale's print is where this decision put it, but it is not the last +> thing `hyp init` writes. The wizard repeats it, short, after its first look +> and before the privacy narration; `runPickerWalkthrough`, which writes +> nothing substantial after the finale, still prints it once. + **"Not configured" means what `hyp status` already means by it.** Both surfaces read a **local layer** plugin entry with `enabled: false` as inactive, so a switched-off adapter strands its diff --git a/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md b/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md new file mode 100644 index 00000000..4ce16190 --- /dev/null +++ b/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md @@ -0,0 +1,99 @@ +# LLP 0188: The stranded-attach warning repeats where the run ends + +**Type:** Decision +**Status:** Accepted +**Systems:** Onboarding, CLI, Clients +**Author:** Brendan / Claude +**Date:** 2026-08-05 +**Extends:** LLP 0185 (an unpicked client is named, not detached) +**Related:** LLP 0135 (the wizard's closing sequence), LLP 0100/0101 (the privacy narration), LLP 0086 (the attach drift diagnostic) + +> [LLP 0185](./0185-unpicked-client-stays-attached.decision.md) put the +> stranded-attach warning in the finale, before the daemon restart. It is +> still there. But `hyp init` writes about seventy more lines after the finale +> returns, so on a real terminal the warning is not on screen when the run +> ends, and the user who does not scroll back never reads it. + +## Context {#context} + +`runPickerFinale` prints the warning and returns. What happens next depends on +which entry point called it: + +- `runPickerWalkthrough` writes its short run summary and stops. The warning + is within a handful of lines of the last thing on screen. +- `runInitWizard` writes the run summary, then `runWizardFirstLook` (roughly + sixty lines of real query output, by its own design in + [LLP 0135 §first-look](./0135-install-experience-overhaul.design.md)), then + on the team path the privacy narration. Nothing pauses between them. + +So the same print is prominent on one path and buried on the other. It is +buried worst exactly where it is needed most: on a managed host, +[LLP 0185 §status-backstop](./0185-unpicked-client-stays-attached.decision.md#status-backstop) +gates the `client_attached_not_configured` diagnostic to hosts with no central +layer, so the finale's print is the *only* signal a wizard-attached client the +reconfigure run left unpicked ever gets. A joined host has no backstop to +scroll back to. + +The obvious repair, echoing `finaleSummary.attachedNotConfigured` from +`writeWalkthroughRunSummary`, is wrong twice over. That function is shared by +both entry points, so `runPickerWalkthrough` would print the same warning +twice within a few lines of itself, and the wizard would still bury the echo +under the first look, which is the whole complaint. + +## Decision {#decision} + +**The finale's print stays where it is, and the +caller that buries it repeats it at its own end.** LLP 0185's placement is +unchanged: the finale still names the stranded clients after the config write +and before the daemon restart. `runInitWizard` additionally writes a short +repeat after the first look and before the privacy narration, naming the same +clients and the same `hyp detach --client ` lines. + +The repeat is the *caller's*, not the shared run summary's, because only the +caller knows whether anything came between. `runPickerWalkthrough` prints +nothing substantial after the finale, so it does not repeat, and no path +prints the warning twice on one screen. + +**The repeat is conditional on the closing sequence having +run.** The wizard emits it only when the first look ran (attended, +uncancelled, not a dry run) or the pathway is `team` (the privacy narration +follows). A scripted `--yes` or `--dry-run` wizard run prints nothing between +the finale and its summary, so it stays on the single finale print and its +output is unchanged, matching the attended-only rule +([LLP 0131](./0131-configure-phase.decision.md)). + +**The repeat is shorter than the original.** The first +print explains the consequence in full; repeating that paragraph verbatim +reads as a bug rather than a reminder. The repeat carries only what a user +acts on: the client names and one `hyp detach --client ` line each. Both +prints share the dry-run tag rule the rest of the finale uses. + +**The privacy narration is still the last +words on the team path.** The repeat goes before it, not after +([LLP 0135 §first-look](./0135-install-experience-overhaul.design.md), +LLP 0100/0101). Seven lines of narration is a reminder that survives on one +screen; sixty lines of query output is not, and that is the difference this +decision turns on. + +## Consequences {#consequences} + +- `writeAttachedNotConfiguredReminder` is exported from + `src/core/cli/walkthrough.js` beside the finale's own + `writeAttachedNotConfiguredWarning`, which stays private to the finale. +- `writeWalkthroughRunSummary` is untouched. It still reports only what the + finale *did*, per + [LLP 0135](./0135-install-experience-overhaul.design.md); a warning is not + an action taken. +- A stranded attach on an attended wizard run is printed twice in one run, in + two places, deliberately. That is the cost of the finale's placement being + correct for the restart and wrong for the end of the run. +- The managed-host gap LLP 0185 recorded is narrowed, not closed: status still + says nothing on a joined host, but the run that creates the state now ends + on the warning instead of burying it. + +## References + +- Issue #614 (deferred review finding from PR #608), PR #608 +- `src/core/cli/walkthrough.js` (`writeAttachedNotConfiguredReminder`), + `src/core/cli/wizard/index.js` (the closing sequence) +- LLP 0185, LLP 0135, LLP 0131, LLP 0100/0101 diff --git a/src/core/cli/walkthrough.js b/src/core/cli/walkthrough.js index 263dfd48..74b0384c 100644 --- a/src/core/cli/walkthrough.js +++ b/src/core/cli/walkthrough.js @@ -1143,6 +1143,38 @@ function writeAttachedNotConfiguredWarning({ clients, stdout, dryRun }) { for (const client of clients) stdout.write(` hyp detach --client ${client}\n`) } +/** + * The closing repeat of the stranded-attach warning, for an entry point whose + * finale is not the last thing it writes. + * + * The finale's own warning stays exactly where LLP 0185 put it, before the + * daemon restart that is the point of no return. This is a second, compact + * print for the caller that keeps writing afterwards: the wizard follows the + * finale with a run summary, a first look that is roughly sixty lines of real + * query output, and on the team path a privacy narration, none of which pause, + * so on a real terminal the original warning is scrolled away by the time the + * run ends. It matters most on a managed host, where the finale's print is the + * only signal there is because `hyp status`'s mirror diagnostic is gated to + * hosts with no central layer (LLP 0185 #status-backstop). + * + * Only a caller that printed something substantial in between calls this. + * `runPickerWalkthrough` writes a short run summary and stops, so it keeps the + * single finale print and never repeats it onto the same screen. + * + * @ref LLP 0188#repeat-at-the-end [implements]: the repeat belongs to the caller whose own output buried the first print + * @param {{ + * clients: string[], + * stdout: NodeJS.WritableStream | { write(chunk: string): unknown }, + * dryRun: boolean, + * }} args + */ +export function writeAttachedNotConfiguredReminder({ clients, stdout, dryRun }) { + stdout.write('\n') + stdout.write(`${dryRun ? '(dry-run) ' : ''}Still attached, no longer collected: ${clients.join(', ')}\n`) + stdout.write('Their requests can start failing until you run:\n') + for (const client of clients) stdout.write(` hyp detach --client ${client}\n`) +} + /** * Run the onboarding backfill step. For each picked client that has a * registered backfill provider (intersection of `clientsPicked` and diff --git a/src/core/cli/wizard/index.js b/src/core/cli/wizard/index.js index 690923d1..c52b234a 100644 --- a/src/core/cli/wizard/index.js +++ b/src/core/cli/wizard/index.js @@ -19,7 +19,12 @@ import { discoverBundledPlugins } from '../../runtime/bundled.js' import { buildPluginCatalog } from '../../plugin_catalog.js' import { collectHypAwareStatus } from '../../daemon/status.js' import { formatFirstSyncDeadline, readFirstSyncDeadline } from '../../usage-policy/first_sync_hold.js' -import { LOCAL_INSTALL_RETENTION_DAYS, runPickerFinale, writeWalkthroughRunSummary } from '../walkthrough.js' +import { + LOCAL_INSTALL_RETENTION_DAYS, + runPickerFinale, + writeAttachedNotConfiguredReminder, + writeWalkthroughRunSummary, +} from '../walkthrough.js' import { LOGIN_ORG_SELECTION_MESSAGE } from '../remote_commands.js' import { useColor } from '../stdio.js' import { evaluateReturningGate, runWizardFork } from './fork.js' @@ -202,7 +207,8 @@ export async function runInitWizard(opts) { // still have to type. Attended and non-dry-run only: a scripted `--yes` // install gets no extra output, and a dry run has no writes to look at. // @ref LLP 0135#first-look [implements]: placed after the finale (backfill has landed) and before the privacy narration, which stays the last words - if (interactive && !cancelled && opts.finale?.dryRun !== true) { + const firstLookRan = interactive && !cancelled && opts.finale?.dryRun !== true + if (firstLookRan) { const notices = firstLookNoticeSink(opts.stderr) await runWizardFirstLook({ runner: opts.firstLook ?? firstLookRunnerFromCtx(opts.ctx, notices), @@ -216,6 +222,23 @@ export async function runInitWizard(opts) { notices.close() } + // The finale already named these, before the daemon restart that strands + // them (LLP 0185 #warn-do-not-detach). That print is no longer on screen by + // the time this run ends: the summary, then the first look's ~60 lines, then + // the narration below all follow it without a pause. Repeat it here, short, + // and only when this closing sequence actually wrote something, so the + // direct `runPickerWalkthrough` entry point (whose summary follows the + // finale with nothing in between) keeps its single print. + // @ref LLP 0188#repeat-at-the-end [implements]: the wizard repeats what its own closing output buried + const stranded = finaleSummary?.attachedNotConfigured ?? [] + if (stranded.length > 0 && (firstLookRan || pathway === 'team')) { + writeAttachedNotConfiguredReminder({ + clients: stranded, + stdout: opts.stdout, + dryRun: opts.finale?.dryRun === true, + }) + } + // The wizard's last words on the team pathway: when the first upload // happens and that nothing has shipped yet (LLP 0100/0101, narration // only - the hold itself was written by the join lane's login). diff --git a/test/core/cli/wizard/index.test.js b/test/core/cli/wizard/index.test.js index bfa7bc13..05c4a1cb 100644 --- a/test/core/cli/wizard/index.test.js +++ b/test/core/cli/wizard/index.test.js @@ -358,6 +358,77 @@ test('runInitWizard: a non-interactive or dry run skips the first look', async ( assert.equal(dry.seen.length, 0) }) +// --- the stranded-attach warning's closing repeat (LLP 0188) --- + +/** + * A finale summary that reports clients this run left attached but no longer + * collects. The finale itself printed the full warning before the daemon + * restart; this is what it hands back for the closing repeat to read. + * + * @param {string[]} clients + */ +function strandedFinale(clients) { + return /** @type {any} */ ({ + daemonInstall: { skipped: true, dryRun: false }, + globalInstall: { skipped: true, installed: false }, + attach: [], + skillsInstalled: [], + agentsInstalled: [], + daemonRestart: { skipped: true, dryRun: false, ok: false }, + backfill: [], + attachedNotConfigured: clients, + }) +} + +// The finale names the stranded clients before the daemon restart (LLP 0185 +// #warn-do-not-detach) and then the wizard writes the run summary, the first +// look's ~60 lines, and the privacy narration on top of it, so by the time an +// attended run ends the warning has scrolled away. On a managed host it is +// the only signal there is, because `hyp status`'s mirror diagnostic is gated +// to hosts with no central layer. +// @ref LLP 0188#repeat-at-the-end [tests]: the wizard repeats what its own closing output buried +test('runInitWizard: an attended run repeats the stranded-attach warning after the first look', async () => { + const home = await tmpHome() + await writeFirstSyncHoldMarker({ stateDir: path.join(home, '.hyp', 'hypaware') }) + const stub = firstLookStub( + [{ provider: 'anthropic', model: 'claude-opus-5', input_tokens: 400, cached_tokens: 4000, output_tokens: 40 }], + [{ date: '2026-07-24', sessions: 3, input_tokens: 400, cached_tokens: 4000, output_tokens: 40 }] + ) + const { opts, stdout } = wizardOpts(home, { + fork: async () => 'team', + firstLook: stub.runner, + finaleRunner: async () => strandedFinale(['codex']), + }) + await runInitWizard(opts) + const text = stdout.text() + + // The names and the one command that clears each, not a bare mention. + assert.match(text, /Still attached, no longer collected: codex/, text) + assert.match(text, /hyp detach --client codex/, text) + // Past the block that buried the finale's own print. + assert.ok(text.indexOf('First look') >= 0, text) + assert.ok(text.indexOf('hyp detach --client codex') > text.indexOf('First look'), text) + // And still ahead of the privacy narration, which stays the last words. + assert.ok( + text.indexOf('hyp detach --client codex') < text.indexOf('Nothing has been uploaded yet'), + text + ) +}) + +// The repeat exists because the wizard's closing sequence buries the finale's +// print. A scripted run writes nothing between the two, so repeating there +// would be the double-print on one screen the shared run summary would have +// caused. Its output stays byte-identical to what the finale alone produced. +// @ref LLP 0188#when [tests]: no closing sequence, no repeat +test('runInitWizard: a scripted run does not repeat the stranded-attach warning', async () => { + const { opts, stdout } = wizardOpts(await tmpHome(), { + picks: { sources: ['claude'], exportChoice: 'local-parquet', retentionDays: 30 }, + finaleRunner: async () => strandedFinale(['codex']), + }) + await runInitWizard(opts) + assert.doesNotMatch(stdout.text(), /hyp detach --client/, stdout.text()) +}) + test('runInitWizard: team pathway with a live first-sync hold narrates the deadline', async () => { const home = await tmpHome() const stateDir = path.join(home, '.hyp', 'hypaware') diff --git a/test/core/walkthrough-unpicked-attached-clients.test.js b/test/core/walkthrough-unpicked-attached-clients.test.js index ff6ddcea..8674ca9b 100644 --- a/test/core/walkthrough-unpicked-attached-clients.test.js +++ b/test/core/walkthrough-unpicked-attached-clients.test.js @@ -89,6 +89,13 @@ test('the finale names a still-attached client the new config no longer collects const out = stdout.text() assert.match(out, /codex/, out) assert.match(out, /hyp detach --client codex/, out) + // Exactly once. This entry point writes only its short run summary after the + // finale, so the warning is still on screen when the run ends and repeating + // it (the shape an echo from the shared `writeWalkthroughRunSummary` would + // take) would print the same paragraph twice within a few lines of itself. + // @ref LLP 0188#repeat-at-the-end [tests]: the repeat belongs to the caller that buried the first print, and this one does not + assert.equal(out.match(/hyp detach --client codex/g)?.length, 1, out) + assert.equal(out.match(/Still attached, no longer collected/g)?.length, 1, out) }) test('a picked client that stays configured draws no stranded-attach warning', async () => { From e21952ae6ec5f0a7f1219b157635db167df624a5 Mon Sep 17 00:00:00 2001 From: test Date: Wed, 5 Aug 2026 01:01:35 +0000 Subject: [PATCH 5/7] Review round 1: the closing repeat fires only where something buried it The gate was `firstLookRan || pathway === 'team'`. A `pathway` is only ever resolved on an interactive run, so a team run that is neither cancelled nor a dry run already has `firstLookRan`; the disjunct added exactly the runs where the first look did *not* run. Those are the runs that wrote nothing between the finale and here, so on a team run cancelled at the backfill consent the repeat landed three lines under the finale's own full warning, which is the same double print the shared run summary was rejected for. The gate is now `firstLookRan` alone. LLP 0188 #when says so and says why, and a new test pins the cancelled team run (fails on the old gate). Co-Authored-By: Claude --- ...ach-warning-repeats-at-the-end.decision.md | 21 +++++++++++++------ src/core/cli/wizard/index.js | 11 +++++++++- test/core/cli/wizard/index.test.js | 20 ++++++++++++++++++ 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md b/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md index 4ce16190..f58f4111 100644 --- a/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md +++ b/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md @@ -55,12 +55,21 @@ nothing substantial after the finale, so it does not repeat, and no path prints the warning twice on one screen. **The repeat is conditional on the closing sequence having -run.** The wizard emits it only when the first look ran (attended, -uncancelled, not a dry run) or the pathway is `team` (the privacy narration -follows). A scripted `--yes` or `--dry-run` wizard run prints nothing between -the finale and its summary, so it stays on the single finale print and its -output is unchanged, matching the attended-only rule -([LLP 0131](./0131-configure-phase.decision.md)). +run.** The wizard emits it only when the first look ran: attended, +uncancelled, and not a dry run. That single condition covers the team pathway +as well, because a pathway is only ever resolved on an interactive run, so a +team run that is neither cancelled nor a dry run has already run the first +look. Widening the condition to "or the pathway is `team`" on the theory that +the privacy narration follows would admit only the runs where the first look +did *not* run, and those are exactly the runs that wrote nothing between the +finale and here: the repeat would land a few lines under the print it repeats, +which is the same double print the shared run summary was rejected for. + +So a scripted `--yes` or `--dry-run` wizard run stays on the single finale +print and its output is unchanged, matching the attended-only rule +([LLP 0131](./0131-configure-phase.decision.md)), and so does an interactive +run cancelled at the backfill consent, whose summary is all that follows the +finale. **The repeat is shorter than the original.** The first print explains the consequence in full; repeating that paragraph verbatim diff --git a/src/core/cli/wizard/index.js b/src/core/cli/wizard/index.js index c52b234a..c3e6b931 100644 --- a/src/core/cli/wizard/index.js +++ b/src/core/cli/wizard/index.js @@ -229,9 +229,18 @@ export async function runInitWizard(opts) { // and only when this closing sequence actually wrote something, so the // direct `runPickerWalkthrough` entry point (whose summary follows the // finale with nothing in between) keeps its single print. + // + // `firstLookRan` is the whole condition, the team pathway included: a + // `pathway` is only ever resolved on an interactive run, so a team run that + // is neither cancelled nor a dry run has already run the first look. Adding + // `|| pathway === 'team'` would widen this to exactly the runs where the + // first look did *not* run (cancelled at the backfill consent, or a dry + // run), and those wrote nothing between the finale and here, so the repeat + // would land a few lines under the print it repeats. + // @ref LLP 0188#when [constrained-by]: no closing sequence, no repeat // @ref LLP 0188#repeat-at-the-end [implements]: the wizard repeats what its own closing output buried const stranded = finaleSummary?.attachedNotConfigured ?? [] - if (stranded.length > 0 && (firstLookRan || pathway === 'team')) { + if (stranded.length > 0 && firstLookRan) { writeAttachedNotConfiguredReminder({ clients: stranded, stdout: opts.stdout, diff --git a/test/core/cli/wizard/index.test.js b/test/core/cli/wizard/index.test.js index 05c4a1cb..4656139b 100644 --- a/test/core/cli/wizard/index.test.js +++ b/test/core/cli/wizard/index.test.js @@ -429,6 +429,26 @@ test('runInitWizard: a scripted run does not repeat the stranded-attach warning' assert.doesNotMatch(stdout.text(), /hyp detach --client/, stdout.text()) }) +// A cancel at the backfill consent skips the first look, so the run summary is +// the only thing between the finale's own warning (which the finale prints +// before its restart block, cancelled or not) and the end of the run. The team +// pathway is not on its own a reason to repeat: a pathway is only resolved on +// an interactive run, so an uncancelled non-dry team run has already run the +// first look, and the runs a `pathway === 'team'` clause would add are exactly +// the ones with nothing in between. +// @ref LLP 0188#when [tests]: a cancelled team run buried nothing, so it does not repeat +test('runInitWizard: a run cancelled at the finale does not repeat the stranded-attach warning', async () => { + const home = await tmpHome() + await writeFirstSyncHoldMarker({ stateDir: path.join(home, '.hyp', 'hypaware') }) + const { opts, stdout } = wizardOpts(home, { + fork: async () => 'team', + finaleRunner: async () => ({ ...strandedFinale(['codex']), cancelled: true }), + }) + const result = await runInitWizard(opts) + assert.equal(result.cancelled, true) + assert.doesNotMatch(stdout.text(), /hyp detach --client/, stdout.text()) +}) + test('runInitWizard: team pathway with a live first-sync hold narrates the deadline', async () => { const home = await tmpHome() const stateDir = path.join(home, '.hyp', 'hypaware') From e1bfbc3c7ee6b702c7fc12af11a6947f795956aa Mon Sep 17 00:00:00 2001 From: test Date: Wed, 5 Aug 2026 01:42:42 +0000 Subject: [PATCH 6/7] Review round 2: the repeat gates on the first look printing, not attempting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `firstLookRan` says the closing block was attempted. It is documented to degrade to a silent skip rather than fail a finished install (LLP 0135 #first-look), so an unregistered `ai_gateway_messages` dataset, an unreadable cache, or a render that throws leaves an attended run that wrote nothing between the finale's warning and the closing repeat. Probed against the real orchestrator with no overview dataset: stdout was the config-write line and the repeat, with the finale's own six-line warning directly above it in production. That is the same-screen double print LLP 0188 says no path produces, reached through the first look's designed degradation instead of through the pathway disjunct round 1 removed. Gate on the result: `runWizardFirstLook` already returns `shown`, which is exactly "the block reached the screen". LLP 0188 §when restated to match (the condition is the closing sequence having *written* something), and the `**Extends:**` header folded into `Related:`, which is how the corpus's other 167 docs name what they extend. Co-Authored-By: Claude --- ...ach-warning-repeats-at-the-end.decision.md | 29 +++++++++++------- src/core/cli/wizard/index.js | 30 ++++++++++++------- test/core/cli/wizard/index.test.js | 24 +++++++++++++++ 3 files changed, 62 insertions(+), 21 deletions(-) diff --git a/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md b/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md index f58f4111..7e1459e1 100644 --- a/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md +++ b/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md @@ -5,8 +5,7 @@ **Systems:** Onboarding, CLI, Clients **Author:** Brendan / Claude **Date:** 2026-08-05 -**Extends:** LLP 0185 (an unpicked client is named, not detached) -**Related:** LLP 0135 (the wizard's closing sequence), LLP 0100/0101 (the privacy narration), LLP 0086 (the attach drift diagnostic) +**Related:** LLP 0185 (an unpicked client is named, not detached; this doc is its `Extended-by`), LLP 0135 (the wizard's closing sequence), LLP 0100/0101 (the privacy narration), LLP 0086 (the attach drift diagnostic) > [LLP 0185](./0185-unpicked-client-stays-attached.decision.md) put the > stranded-attach warning in the finale, before the daemon restart. It is @@ -55,15 +54,23 @@ nothing substantial after the finale, so it does not repeat, and no path prints the warning twice on one screen. **The repeat is conditional on the closing sequence having -run.** The wizard emits it only when the first look ran: attended, -uncancelled, and not a dry run. That single condition covers the team pathway -as well, because a pathway is only ever resolved on an interactive run, so a -team run that is neither cancelled nor a dry run has already run the first -look. Widening the condition to "or the pathway is `team`" on the theory that -the privacy narration follows would admit only the runs where the first look -did *not* run, and those are exactly the runs that wrote nothing between the -finale and here: the repeat would land a few lines under the print it repeats, -which is the same double print the shared run summary was rejected for. +written something.** The wizard emits it only when the first look actually +reached the screen. That is a narrower test than "the first look ran", and +deliberately so: the step is documented to degrade to a silent skip rather +than fail a finished install +([LLP 0135 §first-look](./0135-install-experience-overhaul.design.md)), so an +unregistered dataset, an unreadable cache, or a render that throws leaves an +attended run that attempted the block and printed none of it. Nothing buried +the finale's print on such a run, so nothing is repeated. + +That single condition covers the team pathway as well, because a pathway is +only ever resolved on an interactive run, so a team run that is neither +cancelled nor a dry run has already run the first look. Widening the condition +to "or the pathway is `team`" on the theory that the privacy narration follows +would admit only the runs where the first look did *not* run, and those are +exactly the runs that wrote nothing between the finale and here: the repeat +would land a few lines under the print it repeats, which is the same double +print the shared run summary was rejected for. So a scripted `--yes` or `--dry-run` wizard run stays on the single finale print and its output is unchanged, matching the attended-only rule diff --git a/src/core/cli/wizard/index.js b/src/core/cli/wizard/index.js index c3e6b931..fea3ecb0 100644 --- a/src/core/cli/wizard/index.js +++ b/src/core/cli/wizard/index.js @@ -208,13 +208,21 @@ export async function runInitWizard(opts) { // install gets no extra output, and a dry run has no writes to look at. // @ref LLP 0135#first-look [implements]: placed after the finale (backfill has landed) and before the privacy narration, which stays the last words const firstLookRan = interactive && !cancelled && opts.finale?.dryRun !== true + // Whether the block actually reached the screen, which is not the same + // question: the step is documented to degrade to a silent skip rather than + // fail a finished install (LLP 0135 #first-look), so an unregistered + // dataset, an unreadable cache, or a render that throws all leave + // `firstLookRan` true and stdout untouched. The closing repeat below reads + // this, not the gate, because what it needs to know is what was written. + let firstLookShown = false if (firstLookRan) { const notices = firstLookNoticeSink(opts.stderr) - await runWizardFirstLook({ + const look = await runWizardFirstLook({ runner: opts.firstLook ?? firstLookRunnerFromCtx(opts.ctx, notices), stdout: opts.stdout, color: useColor(opts.stdout, opts.env), }) + firstLookShown = look.shown // The abandoned queries from an expired deadline keep running and can // still resolve with a withheld-row report. Close the sink so that // report cannot land after the privacy narration below, which is @@ -230,17 +238,19 @@ export async function runInitWizard(opts) { // direct `runPickerWalkthrough` entry point (whose summary follows the // finale with nothing in between) keeps its single print. // - // `firstLookRan` is the whole condition, the team pathway included: a - // `pathway` is only ever resolved on an interactive run, so a team run that - // is neither cancelled nor a dry run has already run the first look. Adding - // `|| pathway === 'team'` would widen this to exactly the runs where the - // first look did *not* run (cancelled at the backfill consent, or a dry - // run), and those wrote nothing between the finale and here, so the repeat - // would land a few lines under the print it repeats. - // @ref LLP 0188#when [constrained-by]: no closing sequence, no repeat + // `firstLookShown` is the whole condition, the team pathway included. It is + // read rather than `firstLookRan` because a first look that skipped itself + // wrote nothing, and rather than `pathway`, because a `pathway` is only ever + // resolved on an interactive run: a team run that is neither cancelled nor a + // dry run has already run the first look, so `|| pathway === 'team'` would + // widen this to exactly the runs where the first look did *not* run + // (cancelled at the backfill consent, or a dry run). Every run either + // condition would add wrote nothing between the finale and here, so the + // repeat would land a few lines under the print it repeats. + // @ref LLP 0188#when [constrained-by]: nothing written in between, no repeat // @ref LLP 0188#repeat-at-the-end [implements]: the wizard repeats what its own closing output buried const stranded = finaleSummary?.attachedNotConfigured ?? [] - if (stranded.length > 0 && firstLookRan) { + if (stranded.length > 0 && firstLookShown) { writeAttachedNotConfiguredReminder({ clients: stranded, stdout: opts.stdout, diff --git a/test/core/cli/wizard/index.test.js b/test/core/cli/wizard/index.test.js index 4656139b..e7f959d9 100644 --- a/test/core/cli/wizard/index.test.js +++ b/test/core/cli/wizard/index.test.js @@ -449,6 +449,30 @@ test('runInitWizard: a run cancelled at the finale does not repeat the stranded- assert.doesNotMatch(stdout.text(), /hyp detach --client/, stdout.text()) }) +// The first look is documented to degrade to a silent skip rather than fail a +// finished install (LLP 0135 #first-look): an unregistered dataset, an +// unreadable cache, or a render that throws all leave an attended run that +// attempted the block and printed none of it. The gate that admits the repeat +// is therefore the block having reached the screen, not the run having tried: +// on a skip the finale's own print is still the last thing above the summary, +// and repeating under it would be the same-screen double print. +// @ref LLP 0188#when [tests]: a first look that printed nothing buried nothing +test('runInitWizard: an attended run whose first look skips itself does not repeat the stranded-attach warning', async () => { + const home = await tmpHome() + await writeFirstSyncHoldMarker({ stateDir: path.join(home, '.hyp', 'hypaware') }) + const { opts, stdout } = wizardOpts(home, { + fork: async () => 'team', + // The shape `firstLookRunnerFromCtx` yields when the overview dataset is + // not registered: the step returns `{ shown: false }` without writing. + firstLook: { hasDataset: () => false, async run() { return { columns: [], rows: [] } } }, + finaleRunner: async () => strandedFinale(['codex']), + }) + await runInitWizard(opts) + const text = stdout.text() + assert.doesNotMatch(text, /First look/, text) + assert.doesNotMatch(text, /hyp detach --client/, text) +}) + test('runInitWizard: team pathway with a live first-sync hold narrates the deadline', async () => { const home = await tmpHome() const stateDir = path.join(home, '.hyp', 'hypaware') From d50d688a29fb874b2390bfe97c8f64716585dee4 Mon Sep 17 00:00:00 2001 From: test Date: Fri, 14 Aug 2026 19:48:23 +0000 Subject: [PATCH 7/7] The closing repeat gates on measured output, not on `shown` (#614) Maintainer decision on the fix shape: measure "wrote something" directly. `runWizardFirstLook` now writes through a counter and returns `wrote` beside `shown`, so the step reports for itself whether it put text on the screen. The wizard's closing stranded-attach repeat gates on that. `shown` answers "did the block render", which is not the question the gate asks. The `slow` skip renders no block and still writes two lines saying so, and those lines bury the finale's warning exactly as a full render would: on a managed host, where LLP 0185 #status-backstop gates `hyp status`'s mirror diagnostic off, that print was the only signal the user was ever going to get. Inferring the gate from a neighbouring fact has now broken on the no-dataset, error and slow paths in turn; a measured value closes the class, because a skip reason added later is counted by the same writable. - `FirstLookResult` = `FirstLookOutcome & { wrote: boolean }`, attached at a single exit so no branch can omit it. - New orchestrator test: an attended run whose first look skips slowly still repeats the warning. It fails on the `shown` gate (`not ok 22`, `# fail 1`) and passes on the measured one. `firstLookBudgetMs` is a test seam so it does not wait out the real 8s budget. - The first-look unit tests now pin `wrote` per branch: true for the render, the partial render and the slow skip, false for no-dataset and the error skips. - The comment claiming "a first look that skipped itself wrote nothing" is replaced by the real contract, and LLP 0188 #when is restated to say the condition is measured output. `npm test`: 3413 tests, 3412 pass, 0 fail, 1 skipped. `npm run typecheck`: clean. Co-Authored-By: Claude --- ...ach-warning-repeats-at-the-end.decision.md | 36 +++++++++--- src/core/cli/wizard/first_look.js | 35 ++++++++++- src/core/cli/wizard/index.js | 43 ++++++++------ src/core/cli/wizard/types.d.ts | 20 ++++++- test/core/cli/wizard/first-look.test.js | 23 +++++--- test/core/cli/wizard/index.test.js | 58 ++++++++++++++++--- 6 files changed, 168 insertions(+), 47 deletions(-) diff --git a/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md b/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md index 7e1459e1..b6d25ecc 100644 --- a/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md +++ b/llp/0188-stranded-attach-warning-repeats-at-the-end.decision.md @@ -54,18 +54,33 @@ nothing substantial after the finale, so it does not repeat, and no path prints the warning twice on one screen. **The repeat is conditional on the closing sequence having -written something.** The wizard emits it only when the first look actually -reached the screen. That is a narrower test than "the first look ran", and -deliberately so: the step is documented to degrade to a silent skip rather -than fail a finished install -([LLP 0135 §first-look](./0135-install-experience-overhaul.design.md)), so an -unregistered dataset, an unreadable cache, or a render that throws leaves an -attended run that attempted the block and printed none of it. Nothing buried -the finale's print on such a run, so nothing is repeated. +written something, and that fact is measured rather than inferred.** +`runWizardFirstLook` reports `wrote` beside `shown`, counted at the writable +the step writes through, and the wizard gates on `wrote`. + +The two are different questions, and every wrong version of this gate has been +an attempt to answer the first with the second. "The first look ran" is too +wide: the step is documented to degrade rather than fail a finished install +([LLP 0135 §first-look](./0135-install-experience-overhaul.design.md)), and an +unregistered dataset, an unreadable cache or a render that throws leave an +attended run that attempted the block and printed none of it, so nothing +buried the finale's print. "The block rendered" is too narrow: an expired +deadline with nothing renderable prints two lines saying the look was skipped +and reports `shown: false`, and those lines bury the finale's print exactly as +a full render would. On a managed host that is the run where the repeat +matters most, because §status-backstop of +[LLP 0185](./0185-unpicked-client-stays-attached.decision.md#status-backstop) +gates the mirror diagnostic off there. + +Measuring closes the class rather than the instance. A skip reason added later +is counted by the same writable, so it carries its own answer instead of +requiring this gate to be revisited, and a caller that wants "does the user +have their numbers" still reads `shown`. That single condition covers the team pathway as well, because a pathway is only ever resolved on an interactive run, so a team run that is neither -cancelled nor a dry run has already run the first look. Widening the condition +cancelled nor a dry run has already run the first look and repeats on what it +wrote. Widening the condition to "or the pathway is `team`" on the theory that the privacy narration follows would admit only the runs where the first look did *not* run, and those are exactly the runs that wrote nothing between the finale and here: the repeat @@ -96,6 +111,9 @@ decision turns on. - `writeAttachedNotConfiguredReminder` is exported from `src/core/cli/walkthrough.js` beside the finale's own `writeAttachedNotConfiguredWarning`, which stays private to the finale. +- `FirstLookResult` carries `wrote` as well as `shown`, so the first look + answers "did this put text on the screen" for itself. It is the step's fact + to report, not the orchestrator's to deduce. - `writeWalkthroughRunSummary` is untouched. It still reports only what the finale *did*, per [LLP 0135](./0135-install-experience-overhaul.design.md); a warning is not diff --git a/src/core/cli/wizard/first_look.js b/src/core/cli/wizard/first_look.js index 55f58bf2..e595436b 100644 --- a/src/core/cli/wizard/first_look.js +++ b/src/core/cli/wizard/first_look.js @@ -9,7 +9,7 @@ * This module owns only the wizard's half of the contract: when the step * runs, and that it can never fail a finished install. * - * @import { FirstLookResult } from '../../../../src/core/cli/wizard/types.js' + * @import { FirstLookOutcome, FirstLookResult } from '../../../../src/core/cli/wizard/types.js' * @import { OverviewNotice, OverviewQueryRunner } from '../../../../src/core/query/types.js' */ @@ -113,7 +113,18 @@ async function withDeadline(promise, ms) { * materialized) degrades to a skipped step, because setup itself already * succeeded by the time this runs. * + * Returns `wrote` alongside `shown`, and it is *measured*: the writable the + * body sees is a counter in front of the caller's, so every branch, present + * or future, reports whether it put text on the screen. The two questions + * are not the same one. `shown` is "did the block render", and a caller that + * needs "did this push what came before it out of view" cannot infer that + * from `shown`: the `slow` skip renders no block and still writes two lines + * saying so. The wizard's closing repeat asks the second question + * (LLP 0188 #when), and inferring it from `shown` is what broke across the + * no-dataset, error and slow branches in turn. + * * @ref LLP 0135#first-look [implements]: setup ends on the user's own rows, and never fails on them + * @ref LLP 0188#when [implements]: the caller needs "wrote something", so measure it here rather than let the caller guess * * @param {{ * runner?: OverviewQueryRunner | undefined, @@ -123,8 +134,23 @@ async function withDeadline(promise, ms) { * }} args * @returns {Promise} */ -export async function runWizardFirstLook({ runner, stdout, color = false, budgetMs = FIRST_LOOK_BUDGET_MS }) { - return withSpan( +export async function runWizardFirstLook({ runner, stdout: target, color = false, budgetMs = FIRST_LOOK_BUDGET_MS }) { + /** @type {boolean} */ + let wrote = false + // The only `stdout` in scope below, so a branch cannot write without being + // counted. Set before delegating rather than after: a `write` that throws + // part-way (EPIPE on a closed pipe) may have emitted, and the safe error + // is an extra reminder, not a lost one. + /** @type {{ write(chunk: string): unknown }} */ + const stdout = { + /** @param {string} chunk */ + write(chunk) { + wrote = true + return target.write(chunk) + }, + } + /** @type {FirstLookOutcome} */ + const outcome = await withSpan( 'wizard.first_look', { [Attr.COMPONENT]: 'wizard', @@ -230,4 +256,7 @@ export async function runWizardFirstLook({ runner, stdout, color = false, budget }, { component: 'wizard' } ) + // One exit, so `wrote` is attached to whatever the body decided rather + // than restated per branch. + return { ...outcome, wrote } } diff --git a/src/core/cli/wizard/index.js b/src/core/cli/wizard/index.js index ec5e103f..d857082f 100644 --- a/src/core/cli/wizard/index.js +++ b/src/core/cli/wizard/index.js @@ -218,21 +218,25 @@ export async function runInitWizard(opts) { // install gets no extra output, and a dry run has no writes to look at. // @ref LLP 0135#first-look [implements]: placed after the finale (backfill has landed) and before the privacy narration, which stays the last words const firstLookRan = interactive && !cancelled && opts.finale?.dryRun !== true - // Whether the block actually reached the screen, which is not the same - // question: the step is documented to degrade to a silent skip rather than - // fail a finished install (LLP 0135 #first-look), so an unregistered - // dataset, an unreadable cache, or a render that throws all leave - // `firstLookRan` true and stdout untouched. The closing repeat below reads - // this, not the gate, because what it needs to know is what was written. - let firstLookShown = false + // Whether the step put text on the screen, which is neither "it ran" nor + // "the block rendered". The step is documented to degrade rather than fail + // a finished install (LLP 0135 #first-look), and it degrades in two + // different ways: an unregistered dataset, an unreadable cache or a render + // that throws leave `firstLookRan` true and stdout untouched, while an + // expired deadline with nothing renderable writes two lines saying so and + // still reports `shown: false`. `runWizardFirstLook` measures the writes, + // so this is the fact itself rather than an inference from which branch it + // took, and a branch added later reports itself without a change here. + let firstLookWrote = false if (firstLookRan) { const notices = firstLookNoticeSink(opts.stderr) const look = await runWizardFirstLook({ runner: opts.firstLook ?? firstLookRunnerFromCtx(opts.ctx, notices), stdout: opts.stdout, color: useColor(opts.stdout, opts.env), + ...(opts.firstLookBudgetMs !== undefined ? { budgetMs: opts.firstLookBudgetMs } : {}), }) - firstLookShown = look.shown + firstLookWrote = look.wrote // The abandoned queries from an expired deadline keep running and can // still resolve with a withheld-row report. Close the sink so that // report cannot land after the privacy narration below, which is @@ -248,19 +252,22 @@ export async function runInitWizard(opts) { // direct `runPickerWalkthrough` entry point (whose summary follows the // finale with nothing in between) keeps its single print. // - // `firstLookShown` is the whole condition, the team pathway included. It is - // read rather than `firstLookRan` because a first look that skipped itself - // wrote nothing, and rather than `pathway`, because a `pathway` is only ever - // resolved on an interactive run: a team run that is neither cancelled nor a - // dry run has already run the first look, so `|| pathway === 'team'` would - // widen this to exactly the runs where the first look did *not* run - // (cancelled at the backfill consent, or a dry run). Every run either - // condition would add wrote nothing between the finale and here, so the - // repeat would land a few lines under the print it repeats. + // `firstLookWrote` is the whole condition, the team pathway included. It is + // read rather than `firstLookRan` because a first look that wrote nothing + // buried nothing, rather than `look.shown` because a skip that explains + // itself on stdout (the expired deadline) buried the finale's print exactly + // as a full render would, and rather than `pathway` because a `pathway` is + // only ever resolved on an interactive run: a team run that is neither + // cancelled nor a dry run has already run the first look, so + // `|| pathway === 'team'` would widen this to exactly the runs where the + // first look did *not* run (cancelled at the backfill consent, or a dry + // run). Every run either of those conditions would add wrote nothing + // between the finale and here, so the repeat would land a few lines under + // the print it repeats. // @ref LLP 0188#when [constrained-by]: nothing written in between, no repeat // @ref LLP 0188#repeat-at-the-end [implements]: the wizard repeats what its own closing output buried const stranded = finaleSummary?.attachedNotConfigured ?? [] - if (stranded.length > 0 && firstLookShown) { + if (stranded.length > 0 && firstLookWrote) { writeAttachedNotConfiguredReminder({ clients: stranded, stdout: opts.stdout, diff --git a/src/core/cli/wizard/types.d.ts b/src/core/cli/wizard/types.d.ts index 2f1b08b2..eee858de 100644 --- a/src/core/cli/wizard/types.d.ts +++ b/src/core/cli/wizard/types.d.ts @@ -301,15 +301,25 @@ export interface RunWizardPickOptions { } /** - * What the first look did. `shown: false` is a normal outcome, not a + * What the first look decided. `shown: false` is a normal outcome, not a * failure: `no-dataset` when no gateway source was picked, `error` when the * query itself failed, `slow` when summarizing the cache would have * outlasted the step's budget (setup had already succeeded in every case). */ -export type FirstLookResult = +export type FirstLookOutcome = | { shown: true; providerRows: number; dayRows: number; partial?: true } | { shown: false; reason: 'no-dataset' | 'error' | 'slow' } +/** + * The outcome plus whether the step wrote anything to stdout, which is a + * different question and is measured rather than derived from `shown`: the + * `slow` skip renders no block and still writes two lines saying so, while + * `no-dataset` and `error` write nothing. A caller asking "did this push + * earlier output out of view" reads `wrote`; a caller asking "does the user + * now have their numbers" reads `shown` (LLP 0188 #when). + */ +export type FirstLookResult = FirstLookOutcome & { wrote: boolean } + /** * Options for `runInitWizard`, the fork -> join -> pick -> configure -> * privacy -> finale orchestrator (LLP 0135 #orchestration). Non-interactive @@ -358,6 +368,12 @@ export interface RunInitWizardOptions { * built from `ctx`; the step is skipped when neither is available. */ firstLook?: OverviewQueryRunner + /** + * Override the first look's deadline (tests). Defaults to the step's own + * budget; a test driving the slow-skip branch through the orchestrator + * would otherwise have to wait out the real one. + */ + firstLookBudgetMs?: number /** Phase overrides (tests). */ gate?: (opts: EvaluateReturningGateOptions) => Promise fork?: (opts: RunWizardForkOptions) => Promise diff --git a/test/core/cli/wizard/first-look.test.js b/test/core/cli/wizard/first-look.test.js index ebf68933..8dcbde5e 100644 --- a/test/core/cli/wizard/first-look.test.js +++ b/test/core/cli/wizard/first-look.test.js @@ -63,7 +63,7 @@ test('runWizardFirstLook: writes every section, names the repeat command, report seen, [OVERVIEW_PROBE_SQL, SECTION_SQL.models, SECTION_SQL.daily, SECTION_SQL.repos, SECTION_SQL.tools] ) - assert.deepEqual(result, { shown: true, providerRows: 1, dayRows: 1 }) + assert.deepEqual(result, { shown: true, providerRows: 1, dayRows: 1, wrote: true }) const text = stdout.text() assert.match(text, /First look at what HypAware has recorded/) assert.match(text, /claude-opus-5/) @@ -96,6 +96,7 @@ test('runWizardFirstLook: an expired deadline keeps the sections that finished', // What completed is shown rather than thrown away. assert.equal(result.shown, true) assert.equal(result.partial, true) + assert.equal(result.wrote, true) assert.match(text, /First look at what HypAware has recorded/) assert.match(text, /claude-opus-5/) assert.match(text, /2026-07-24/) @@ -121,7 +122,10 @@ test('runWizardFirstLook: a slow cache skips within budget and says what to run' }), }, }) - assert.deepEqual(result, { shown: false, reason: 'slow' }) + // The block did not render, and the step still put two lines on the + // screen. Both facts are reported, because callers ask both questions + // (LLP 0188 #when). + assert.deepEqual(result, { shown: false, reason: 'slow', wrote: true }) // Setup moved on rather than waiting out the query. assert.ok(Date.now() - started < 2000) assert.match(stdout.text(), /Skipped the first look/) @@ -142,6 +146,7 @@ test('runWizardFirstLook: a cache inside the budget still renders', async () => }, }) assert.equal(result.shown, true) + assert.equal(result.wrote, true) assert.match(stdout.text(), /claude-opus-5/) assert.ok(!stdout.text().includes('Skipped the first look')) }) @@ -152,7 +157,7 @@ test('runWizardFirstLook: an unregistered dataset skips silently', async () => { stdout, runner: { hasDataset: () => false, async run() { throw new Error('must not run') } }, }) - assert.deepEqual(result, { shown: false, reason: 'no-dataset' }) + assert.deepEqual(result, { shown: false, reason: 'no-dataset', wrote: false }) assert.equal(stdout.text(), '') }) @@ -162,7 +167,7 @@ test('runWizardFirstLook: a query failure degrades to a skipped step, not a thro stdout, runner: { hasDataset: () => true, async run() { throw new Error('cache unreadable') } }, }) - assert.deepEqual(result, { shown: false, reason: 'error' }) + assert.deepEqual(result, { shown: false, reason: 'error', wrote: false }) assert.equal(stdout.text(), '') }) @@ -192,7 +197,10 @@ test('runWizardFirstLook: a synchronous write failure cannot escape and fail a f }, }, }) - assert.deepEqual(result, { shown: false, reason: 'error' }) + // `wrote` is true here: the write was attempted and a partial chunk may + // have landed before the throw. It is counted before the delegation for + // exactly that reason, and the failure direction is the safe one. + assert.deepEqual(result, { shown: false, reason: 'error', wrote: true }) }) test('runWizardFirstLook: a render failure is contained too', async () => { @@ -211,7 +219,8 @@ test('runWizardFirstLook: a render failure is contained too', async () => { }, }, }) - assert.deepEqual(result, { shown: false, reason: 'error' }) + assert.deepEqual(result, { shown: false, reason: 'error', wrote: false }) + assert.equal(stdout.text(), '') }) test('firstLookNoticeSink: discloses withheld rows, drops the freshness line', async () => { @@ -239,6 +248,6 @@ test('firstLookNoticeSink: a closed sink drops a late disclosure', async () => { test('runWizardFirstLook: no runner (no query registry) skips', async () => { const stdout = makeBuf() const result = await runWizardFirstLook({ stdout }) - assert.deepEqual(result, { shown: false, reason: 'no-dataset' }) + assert.deepEqual(result, { shown: false, reason: 'no-dataset', wrote: false }) assert.equal(stdout.text(), '') }) diff --git a/test/core/cli/wizard/index.test.js b/test/core/cli/wizard/index.test.js index 25115218..2be9d038 100644 --- a/test/core/cli/wizard/index.test.js +++ b/test/core/cli/wizard/index.test.js @@ -484,13 +484,13 @@ test('runInitWizard: a run cancelled at the finale does not repeat the stranded- assert.doesNotMatch(stdout.text(), /hyp detach --client/, stdout.text()) }) -// The first look is documented to degrade to a silent skip rather than fail a -// finished install (LLP 0135 #first-look): an unregistered dataset, an -// unreadable cache, or a render that throws all leave an attended run that -// attempted the block and printed none of it. The gate that admits the repeat -// is therefore the block having reached the screen, not the run having tried: -// on a skip the finale's own print is still the last thing above the summary, -// and repeating under it would be the same-screen double print. +// The first look is documented to degrade rather than fail a finished install +// (LLP 0135 #first-look): an unregistered dataset, an unreadable cache, or a +// render that throws all leave an attended run that attempted the block and +// printed none of it. The gate that admits the repeat is therefore what the +// step wrote, not what it attempted: on a silent skip the finale's own print +// is still the last thing above the summary, and repeating under it would be +// the same-screen double print. // @ref LLP 0188#when [tests]: a first look that printed nothing buried nothing test('runInitWizard: an attended run whose first look skips itself does not repeat the stranded-attach warning', async () => { const home = await tmpHome() @@ -498,7 +498,7 @@ test('runInitWizard: an attended run whose first look skips itself does not repe const { opts, stdout } = wizardOpts(home, { fork: async () => 'team', // The shape `firstLookRunnerFromCtx` yields when the overview dataset is - // not registered: the step returns `{ shown: false }` without writing. + // not registered: the step returns `{ wrote: false }` without writing. firstLook: { hasDataset: () => false, async run() { return { columns: [], rows: [] } } }, finaleRunner: async () => strandedFinale(['codex']), }) @@ -508,6 +508,48 @@ test('runInitWizard: an attended run whose first look skips itself does not repe assert.doesNotMatch(text, /hyp detach --client/, text) }) +// The skip that is not silent, and the reason the gate measures writes rather +// than reading `shown`. When the deadline expires with nothing renderable +// (`reason: 'slow'`, the branch `FIRST_LOOK_BUDGET_MS` exists for: a +// pathological day, a disk that stalls), the block does not render and two +// lines saying so do land on stdout. Those lines, plus the run summary and +// the privacy narration, bury the finale's own warning exactly as a full +// render would, so this run must repeat it. A gate reading `shown` drops the +// repeat here, which on a managed host is the only signal there is: LLP 0185 +// #status-backstop gates `hyp status`'s mirror diagnostic off on a joined +// machine. +// @ref LLP 0188#when [tests]: a skip that still wrote buried the finale's print, so it repeats +test('runInitWizard: an attended run whose first look skips slowly still repeats the stranded-attach warning', async () => { + const home = await tmpHome() + await writeFirstSyncHoldMarker({ stateDir: path.join(home, '.hyp', 'hypaware') }) + const { opts, stdout } = wizardOpts(home, { + fork: async () => 'team', + firstLook: { + hasDataset: () => true, + // Far longer than the budget below, so no section ever lands. `unref` + // so the abandoned query does not hold the test runner open. + run: () => new Promise((resolve) => { + setTimeout(() => resolve({ columns: [], rows: [] }), 5000).unref() + }), + }, + firstLookBudgetMs: 40, + finaleRunner: async () => strandedFinale(['codex']), + }) + await runInitWizard(opts) + const text = stdout.text() + // The block itself never rendered. + assert.match(text, /Skipped the first look/, text) + assert.doesNotMatch(text, /First look at what HypAware has recorded/, text) + // The repeat still ran, under what the skip wrote and ahead of the privacy + // narration, which stays the last words. + assert.match(text, /Still attached, no longer collected: codex/, text) + assert.ok(text.indexOf('hyp detach --client codex') > text.indexOf('Skipped the first look'), text) + assert.ok( + text.indexOf('hyp detach --client codex') < text.indexOf('Nothing has been uploaded yet'), + text + ) +}) + test('runInitWizard: team pathway with a live first-sync hold narrates the deadline', async () => { const home = await tmpHome() const stateDir = path.join(home, '.hyp', 'hypaware')