From f3c951cd09381c2f415453f210de2f55810940ff Mon Sep 17 00:00:00 2001 From: test Date: Wed, 19 Aug 2026 04:07:45 +0000 Subject: [PATCH 1/4] Attach and status residuals: bound the macOS probes, stop the contradictory enablement line, reach both attach tails (#887) Three residuals on the attach and status paths, each reproduced by a test that failed before the change. 1. `hyp status` could hang forever. `collectProxyTrust` shells out twice on darwin with a CA on disk (`security verify-cert`, `launchctl getenv`), and `runServiceCommand` only ever settled on the child's `close`: a probe that never returned was not a caught error, it was a command that printed nothing and never exited. macOS trust evaluation can reach the network for revocation, so an offline or captive-portal host is the trigger. `runServiceCommand` gains an opt-in `timeoutMs` that kills the child, detaches its handles so the caller can still exit, and rejects; the two read-only probes pass it, so the trust line reads unknown instead of holding up the report. Mutating commands (`launchctl bootstrap`, `security add-trusted-cert` behind its password dialog) pass nothing and keep the unbounded wait they have always had. 2. A partial enable no longer contradicts itself. When the accept path's config write landed but the restart, wait, or in-process activation step did not, the prompt printed an accurate report ("the config change already persists ... re-running resumes from the new state") and the caller then printed `enablement.message`, computed before the write: "not enabled ... add @hypaware/claude to and run 'hyp daemon restart'". LLP 0174 #prompt says each step reports its own failure, so the prompt now says it reported one and the caller stays quiet. 3. The daemon-managed "already attached at the live port" branch reaches both tails below `client.attach()`. It used to `continue` right after materializing assets, so on the install shape an explicit `hyp attach` is most often run on, LLP 0186's re-arm (the only re-arm a `refused` marker gets) never happened, and LLP 0174 step 4's backfill offer, which the accept path exists to reach, was never made. The re-arm moves into a shared helper so neither success exit can silently lose it again. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/commands/clients.js | 138 +++++-- src/core/daemon/service_ops.js | 35 +- src/core/daemon/status.js | 15 +- src/core/tls/darwin_trust.js | 10 +- test/core/attach-daemon-managed-tails.test.js | 381 ++++++++++++++++++ test/core/attach-enable-resume.test.js | 46 ++- test/core/service-command-timeout.test.js | 188 +++++++++ 7 files changed, 760 insertions(+), 53 deletions(-) create mode 100644 test/core/attach-daemon-managed-tails.test.js create mode 100644 test/core/service-command-timeout.test.js diff --git a/src/core/commands/clients.js b/src/core/commands/clients.js index 3148b3873..66dd19c02 100644 --- a/src/core/commands/clients.js +++ b/src/core/commands/clients.js @@ -204,7 +204,17 @@ async function runClientLifecycle(action, argv, ctx) { error: message, }) + '\n' ) - } else { + } else if (!promptResult.reported) { + // The prompt already said what happened, in terms of the state it + // left behind ("the config change already persists ... re-running + // resumes from the new state"). `enablement.message` was computed + // before that write and reads "not enabled ... add to + // ": printing it under the first line contradicts it and + // instructs an edit that has already been made. The guided message + // is for the paths that never wrote anything. + // @ref LLP 0174#prompt [implements]: each step reports its own + // failure, so a step that reported one is not re-reported as the + // pre-write refusal ctx.stderr.write(`error: ${message}\n`) } }, @@ -288,7 +298,9 @@ async function runClientLifecycle(action, argv, ctx) { } if (!client) { if (enablement.state !== 'unknown') { - reportAttachEnablement({ name, enablement, parsed, ctx }) + // Same rule as the capability gate above: a prompt that already + // reported its own step's failure owns the message. + if (!promptResult.reported) reportAttachEnablement({ name, enablement, parsed, ctx }) exitCode = 1 continue } @@ -397,6 +409,28 @@ async function runClientLifecycle(action, argv, ctx) { // @ref LLP 0107#every-attach [implements]: every attach path // materializes, including the one with nothing left to wire await materializeAttachAssets({ name, descriptorMap, ctx, dryRun: false, json: parsed.json }) + // The two tails below `client.attach()` are reached from here + // too, because this branch is a successful attach: it is the + // exit an explicit `hyp attach ` takes on a + // daemon-managed install, the shape an operator most often runs + // it on. Skipping them made this the one path where the re-arm + // LLP 0186 specifies never happens and where LLP 0174's step-4 + // offer, the whole point of the accept path that just ran, is + // never made. + // + // Reached with the settings already at the live port rather than + // freshly written, which changes nothing either tail depends on: + // the re-arm's precondition is a successful attach (idempotent + // over its own output by construction, so "already correct" is + // the same fact as "just written"), and the offer's is that this + // invocation enabled the adapter. + // @ref LLP 0186#re-arm-explicit-hyp-attach-re-run-only [implements]: the explicit re-run re-arms, including on the daemon-managed install where the settings need no rewrite + rearmRefusedAttachMarker({ name, ctx, dryRun: false }) + // @ref LLP 0174#prompt [implements]: step 4's backfill consent + // follows the accept path to whichever attach exit it reaches + if (activatedViaPrompt) { + await maybeBackfillAfterEnable({ name, ctx }) + } continue } if (liveEndpoint) { @@ -456,44 +490,7 @@ async function runClientLifecycle(action, argv, ctx) { dryRun: parsed.dryRun, json: parsed.json, }) - // A successful manual attach is the only re-arm a `refused` marker gets - // in this pass: after it, the next reconcile pass must stop - // short-circuiting on the marker and re-`perform()` the request key. - // - // Scoped to a `refused` marker, and skipped on `--dry-run`, on purpose. - // A `done` marker is the only record naming the files an org-driven - // attach installed, so clearing it would strand them past any later - // `hyp detach`, which reads exactly this marker to know what to remove - // (LLP 0138#marker-undo). A `failed` marker needs no help: nothing - // short-circuits it, so the next pass already retries it. And a dry run - // must leave the marker store exactly as it found it, the same way the - // detach path returns before its own clear under `--dry-run`. - // - // The re-arm itself is a drop only when the marker records no - // `installed_assets`. One that carries them is the same undo record a - // `done` marker is (a refusal on a re-`perform()` carries the earlier - // successful attach's copies forward), so it is rewritten to `failed` - // rather than dropped: same re-arm, record intact. That branch lives in - // `rearmRefusedActionMarker` beside the store it rewrites. - // - // Best-effort: a marker-store I/O failure must never fail the attach that - // just succeeded. - // @ref LLP 0186#re-arm-explicit-hyp-attach-re-run-only [implements]: an explicit hyp attach re-arms a refused marker, and only that; the reconciler never re-arms one on its own - if (parsed.dryRun !== true) { - try { - rearmRefusedActionMarker({ - stateRoot: readObservabilityEnv(ctx.env).stateDir, - kind: 'attach', - requestKey: name, - }) - } catch (markerErr) { - getLogger('cmd-attach').warn('client.attach.marker_retract_failed', { - hyp_client: name, - error_kind: 'marker_retract_failed', - detail: markerErr instanceof Error ? markerErr.message : String(markerErr), - }) - } - } + rearmRefusedAttachMarker({ name, ctx, dryRun: parsed.dryRun === true }) // Attach wires a client into HypAware, and its registered skills and // subagents are part of that wiring: manual attach skipping them was the // inconsistency, not the norm (the wizard has always treated @@ -525,6 +522,55 @@ async function runClientLifecycle(action, argv, ctx) { return exitCode } +/** + * Re-arm a `refused` attach marker after a successful manual attach, the only + * re-arm one gets in this pass: after it, the next reconcile pass must stop + * short-circuiting on the marker and re-`perform()` the request key. + * + * Scoped to a `refused` marker, and skipped on `--dry-run`, on purpose. A + * `done` marker is the only record naming the files an org-driven attach + * installed, so clearing it would strand them past any later `hyp detach`, + * which reads exactly this marker to know what to remove (LLP 0138#marker-undo). + * A `failed` marker needs no help: nothing short-circuits it, so the next pass + * already retries it. And a dry run must leave the marker store exactly as it + * found it, the same way the detach path returns before its own clear under + * `--dry-run`. + * + * The re-arm itself is a drop only when the marker records no + * `installed_assets`. One that carries them is the same undo record a `done` + * marker is (a refusal on a re-`perform()` carries the earlier successful + * attach's copies forward), so it is rewritten to `failed` rather than + * dropped: same re-arm, record intact. That branch lives in + * `rearmRefusedActionMarker` beside the store it rewrites. + * + * Best-effort: a marker-store I/O failure must never fail the attach that just + * succeeded. + * + * A function rather than an inline block because attach has two success + * exits, the freshly-wired one and the daemon-managed already-current one, and + * the second silently had no re-arm at all while this lived in the first. + * + * @ref LLP 0186#re-arm-explicit-hyp-attach-re-run-only [implements]: an explicit hyp attach re-arms a refused marker, and only that; the reconciler never re-arms one on its own + * @param {{ name: string, ctx: CommandRunContext, dryRun: boolean }} args + * @returns {void} + */ +function rearmRefusedAttachMarker({ name, ctx, dryRun }) { + if (dryRun) return + try { + rearmRefusedActionMarker({ + stateRoot: readObservabilityEnv(ctx.env).stateDir, + kind: 'attach', + requestKey: name, + }) + } catch (markerErr) { + getLogger('cmd-attach').warn('client.attach.marker_retract_failed', { + hyp_client: name, + error_kind: 'marker_retract_failed', + detail: markerErr instanceof Error ? markerErr.message : String(markerErr), + }) + } +} + /** * Resolve *why* `name` cannot be attached right now, so the failure names the * enablement layer instead of dead-ending on "unknown client". @@ -658,6 +704,12 @@ function reportAttachEnablement({ name, enablement, parsed, ctx }) { * of them reach {@link enableClientAdapter}, so there is no write, no backup, * and no restart to undo. * + * `reported` distinguishes the two ways this can answer "not activated". + * Every early return and the decline are the caller's own refusal to report, + * unchanged. The two failures below `enableClientAdapter` are not: each has + * already written a message describing the state it actually left on disk, so + * the caller's pre-write guided error would contradict it. + * * @ref LLP 0174#bootstrap-floor [implements]: no local config file at all * skips the prompt outright and falls through to the caller's existing * `not_enabled` refusal (which already names `hyp init`) rather than asking @@ -672,7 +724,7 @@ function reportAttachEnablement({ name, enablement, parsed, ctx }) { * parsed: { client: string, json: boolean, dryRun: boolean }, * enablement: { state: 'unknown' } | { state: 'not_enabled' | 'disabled_central', errorKind: string, message: string }, * }} args - * @returns {Promise<{ activated: boolean }>} + * @returns {Promise<{ activated: boolean, reported?: boolean }>} */ async function maybeInteractiveEnableAttach({ name, ctx, parsed, enablement }) { // `disabled_central` never reaches this prompt (LLP 0174 #detection): a @@ -756,7 +808,7 @@ async function maybeInteractiveEnableAttach({ name, ctx, parsed, enablement }) { }) if (!result.ok) { reportEnableFailure({ name, result, ctx }) - return { activated: false } + return { activated: false, reported: true } } // The write and (if a daemon is installed) the restart already landed; what @@ -779,7 +831,7 @@ async function maybeInteractiveEnableAttach({ name, ctx, parsed, enablement }) { status: 'failed', [Attr.ERROR_KIND]: 'activation_incomplete', }) - return { activated: false } + return { activated: false, reported: true } } getLogger('cmd-attach').info('client.attach.enable_prompt', { diff --git a/src/core/daemon/service_ops.js b/src/core/daemon/service_ops.js index 5b3fd319c..be2593140 100644 --- a/src/core/daemon/service_ops.js +++ b/src/core/daemon/service_ops.js @@ -31,6 +31,30 @@ export class ServiceOpError extends Error { } } +/** + * Error raised when a bounded service command passed its deadline and was + * killed. A distinct name so a caller can tell "the probe did not answer" + * apart from "the probe answered no": only the second is a fact about the + * machine. + */ +export class ServiceCommandTimeoutError extends ServiceOpError { + /** + * @param {string} message + */ + constructor(message) { + super(message) + this.name = 'ServiceCommandTimeoutError' + } +} + +/** + * How long a read-only status probe waits for a service command before it + * gives up. Long enough for a cold `security verify-cert` (keychain open, + * trust evaluation) on a healthy machine, short enough that the offline or + * captive-portal host it exists for still gets a report. + */ +export const SERVICE_PROBE_TIMEOUT_MS = 5000 + /** * Error raised when the test-runner guard refuses to spawn a service * manager. A distinct name so a caller (or a test) can tell "we declined @@ -141,7 +165,8 @@ function serviceManagerSpawnRefusal(bin, args) { * killed and the promise rejects with {@link ServiceCommandTimeoutError}; * `SIGKILL` rather than `SIGTERM` because the case worth bounding is a * process blocked on a GUI keychain prompt, which is exactly the state that - * ignores a polite signal. + * ignores a polite signal. Only a bounded caller is ever killed, so that + * signal can never interrupt a half-applied mutation. * * @param {string} bin * @param {string[]} args @@ -165,7 +190,15 @@ export function runServiceCommand(bin, args, opts = {}) { if (timeoutMs !== undefined) { timer = setTimeout(function() { timedOut = true + // Settle on the deadline itself, not on the kill's own `close`. A + // child that leaves a grandchild holding the inherited pipe never + // emits one, and waiting for it would reinstate the unbounded wait + // this timer exists to remove. Detaching the three handles is what + // lets the calling process exit while such a child winds down. proc.kill('SIGKILL') + proc.stdout.destroy() + proc.stderr.destroy() + proc.unref() reject(new ServiceCommandTimeoutError( `'${[bin, ...args].join(' ')}' did not finish within ${timeoutMs}ms and was killed` )) diff --git a/src/core/daemon/status.js b/src/core/daemon/status.js index 0246b07df..b5a99b093 100644 --- a/src/core/daemon/status.js +++ b/src/core/daemon/status.js @@ -1957,12 +1957,19 @@ const TRUST_PROBE_TIMEOUT_MS = 5_000 * "non-macOS platforms skip this entirely"), and with no CA on disk proxy * mode was never on, so there is nothing to be trusted or untrusted. * - * The two probes shell out, so each is caught independently: a probe that + * The two probes shell out, so each is settled independently: a probe that * could not run reports `null` (unknown), never `false`, because "the * dialog was cancelled" and "`security` did not run" are different answers - * and only the first is actionable. The fingerprint is computed locally from - * the DER and is `[0-9A-F:]` by construction, and probe stderr is deliberately - * not surfaced, so neither needs bounding. + * and only the first is actionable. "Did not run" covers one case a try/catch + * cannot reach on its own: a probe that never returns. Both probes therefore + * spawn on a deadline (`TRUST_PROBE_TIMEOUT_MS`) and reject when it passes, + * and they are started concurrently so the worst case is one deadline rather + * than the sum of both. An offline or captive-portal host, where macOS trust + * evaluation can sit on a revocation fetch indefinitely, then still gets a + * rendered report with these lines unknown instead of a `hyp status` that + * never prints. The fingerprint is computed locally from the DER and is + * `[0-9A-F:]` by construction, and probe stderr is deliberately not surfaced, + * so neither of those needs sanitizing. * * The permitted host set travels with the fingerprint because the grant is * wider than any one install uses: the CA is constrained to the whole static diff --git a/src/core/tls/darwin_trust.js b/src/core/tls/darwin_trust.js index 40abc2ae5..24fcd36a0 100644 --- a/src/core/tls/darwin_trust.js +++ b/src/core/tls/darwin_trust.js @@ -53,9 +53,13 @@ export function loginKeychainPath(homeDir = os.homedir()) { * * `timeoutMs` bounds the spawn for callers that cannot afford to block on it. * Silent is the expectation, not a guarantee: a locked login keychain can put - * `security` in front of a GUI prompt, and a caller nobody is watching (`hyp - * status`) would then wait on an answer forever. Left unset the wait is - * unbounded, which is what an interactive attach wants. + * `security` in front of a GUI prompt, and macOS trust evaluation can reach + * the network for revocation, so on an offline or captive-portal host this is + * not a slow command but one that may never return. A caller nobody is + * watching (`hyp status`) would then wait on an answer forever, and reads the + * rejection as unknown instead. Left unset the wait is unbounded, which is + * what an interactive attach wants. + * @ref LLP 0237#consequences [constrained-by]: hyp status has to be able to state the trust line, so the probe behind it must be able to give up * * @param {object} args * @param {string} args.certPath diff --git a/test/core/attach-daemon-managed-tails.test.js b/test/core/attach-daemon-managed-tails.test.js new file mode 100644 index 000000000..835b59d72 --- /dev/null +++ b/test/core/attach-daemon-managed-tails.test.js @@ -0,0 +1,381 @@ +// @ts-check + +import assert from 'node:assert/strict' +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import path from 'node:path' +import process from 'node:process' +import { PassThrough } from 'node:stream' +import test from 'node:test' + +import { runAttach } from '../../src/core/commands/clients.js' +import { readClientActionStatus } from '../../src/core/config/action_reconciler.js' + +/** + * The daemon-managed "already attached at the live port" branch is a success + * exit of `hyp attach`, and it is the one an operator on a default install + * actually reaches: the gateway is not bound in this CLI process, no `listen` + * is configured, and the marker in the client's settings already names the + * daemon's live port. It used to `continue` straight after materializing + * assets, so it was the one attach path that reached neither tail below + * `client.attach()`: + * + * - the `refused`-marker re-arm, which LLP 0186 makes the ONLY re-arm a + * refused marker gets, and whose sole trigger is exactly this explicit + * re-run; + * - LLP 0174 step 4's backfill consent, the offer the accept path exists to + * reach, which the accept branch had just earned by enabling the adapter in + * this same invocation. + * + * @import { CommandRunContext } from '../../hypaware-plugin-kernel-types.js' + * + * @ref LLP 0186#re-arm-explicit-hyp-attach-re-run-only [tests]: the explicit + * re-run re-arms a refused marker on the daemon-managed install too + * @ref LLP 0174#prompt [tests]: step 4's backfill consent is reached from the + * attach exit the accept path actually lands on here + */ + +/** + * @param {{ onWrite?: (chunk: unknown) => void }} [opts] + * @returns {{ write(chunk: unknown): boolean, text(): string }} + */ +function makeBuf(opts = {}) { + let value = '' + return { + write(chunk) { + value += String(chunk) + opts.onWrite?.(chunk) + return true + }, + text() { + return value + }, + } +} + +/** + * A TTY stdin that feeds queued answers, the first pre-buffered and each later + * one on demand. Same shape (and same reason) as + * test/core/attach-enable-backfill.test.js's fixture: an answer written before + * its own question's `readline.Interface` exists can be eaten by the previous + * one. + * + * @param {string[]} answers + * @returns {{ stream: PassThrough, feedNext(): void }} + */ +function makeAnswerStdin(answers) { + const stream = new PassThrough() + Object.defineProperty(stream, 'isTTY', { value: true }) + const queue = [...answers] + const first = queue.shift() + if (first !== undefined) stream.write(`${first}\n`) + return { + stream, + feedNext() { + const next = queue.shift() + if (next !== undefined) stream.write(`${next}\n`) + }, + } +} + +/** @param {string} home */ +function stateRoot(home) { + return path.join(home, '.hyp', 'hypaware') +} + +/** @param {string} home */ +function writeLocalConfig(home) { + const configPath = path.join(home, '.hyp', 'hypaware-config.json') + mkdirSync(path.dirname(configPath), { recursive: true }) + writeFileSync(configPath, JSON.stringify({ version: 2, plugins: [] })) +} + +/** + * Seed the daemon run dir with a live pid file (this process, guaranteed + * alive) and a status.json naming the gateway's bound port, plus the client + * settings marker already at that same port. Together these are the + * "daemon-managed, already current" state. + * + * @param {string} home + * @param {number} port + */ +function seedDaemonManagedAttach(home, port) { + const runDir = path.join(stateRoot(home), 'run') + mkdirSync(runDir, { recursive: true }) + writeFileSync( + path.join(runDir, 'hypaware.pid'), + JSON.stringify({ pid: process.pid, runId: 'test-run', mode: 'foreground' }) + ) + writeFileSync( + path.join(runDir, 'status.json'), + JSON.stringify({ + state: 'healthy', + pid: process.pid, + startedAt: new Date().toISOString(), + uptimeMs: 0, + runId: 'test-run', + mode: 'foreground', + sources: [ + { + name: 'ai-gateway', + plugin: '@hypaware/ai-gateway', + state: 'started', + details: { host: '127.0.0.1', port, upstreams: ['anthropic'] }, + }, + ], + sinks: [], + }) + ) + mkdirSync(path.join(home, '.claude'), { recursive: true }) + writeFileSync( + path.join(home, '.claude', 'settings.json'), + JSON.stringify({ _hypaware: { version: '2.0.0', port } }) + ) +} + +/** + * Seed a `refused` attach marker, as `action_attach.js`'s `perform()` would + * have recorded a permanent precondition refusal (LLP 0186): no `attempts`. + * + * @param {string} home + */ +function seedRefusedMarker(home) { + const dir = path.join(stateRoot(home), 'config-control') + mkdirSync(dir, { recursive: true }) + writeFileSync( + path.join(dir, 'client-actions.json'), + JSON.stringify({ + attach: { + claude: { + status: 'refused', + request_key: 'claude', + reason: '~/.claude/settings.json appears to be JSONC; refuse to modify', + at: '2026-08-01T00:00:00.000Z', + }, + }, + }) + '\n' + ) +} + +/** + * A minimal `BackfillContribution`-shaped provider that yields nothing, so + * only the fact that it ran matters. + * + * @param {string} name + * @param {() => void} onRun + */ +function makeProvider(name, onRun) { + return { + name, + plugin: `@hypaware/${name}`, + datasets: ['ai_gateway_messages'], + async *run() { + onRun() + }, + } +} + +/** + * A daemon-managed context: the gateway capability is present (or becomes + * present via the enable prompt) but unbound in this process, so + * `localEndpoint()` throws and attach falls through to the status.json port + * discovery. `preEnabled` picks which of the two shapes this is: an adapter + * already enabled coming in, or one the enable prompt turns on here. + * + * @param {{ + * home: string, + * preEnabled: boolean, + * answers?: string[], + * backfillProvider?: ReturnType, + * }} opts + */ +function makeDaemonManagedCtx({ home, preEnabled, answers = [], backfillProvider }) { + /** @type {string[]} */ + const registered = preEnabled ? ['claude'] : [] + /** @type {{ name: string }[]} */ + const plugins = [] + /** @type {string[]} */ + const attachCalls = [] + const gateway = { + localEndpoint() { + throw new Error('ai-gateway: localEndpoint() called before the gateway started') + }, + /** @param {string} name */ + getClient(name) { + if (!registered.includes(name)) return undefined + return { + name, + async attach() { + attachCalls.push(name) + }, + } + }, + listClients() { + return registered.map((name) => ({ name })) + }, + } + const stdin = makeAnswerStdin(answers) + const stdout = makeBuf({ + onWrite: (chunk) => { + if (String(chunk).includes('[Y/n]: ')) stdin.feedNext() + }, + }) + const stderr = makeBuf({ + onWrite: (chunk) => { + // The LLP 0244 proxy-migration question sits between the enable prompt + // and the backfill consent; decline it directly so the queued answers + // keep meaning [enable, backfill]. + if (String(chunk).includes('Switch this install to proxy mode now? [y/N] ')) { + stdin.stream.write('n\n') + } + }, + }) + const ctx = /** @type {CommandRunContext} */ (/** @type {any} */ ({ + stdout, + stderr, + stdin: stdin.stream, + cwd: home, + env: { HOME: home, HYP_HOME: path.join(home, '.hyp') }, + config: { version: 2 }, + storage: { cacheRoot: home }, + query: {}, + backfillMaterializers: { get: () => undefined }, + backfills: { + /** @param {string} name */ + get: (name) => (backfillProvider && backfillProvider.name === name ? backfillProvider : undefined), + list: () => (backfillProvider ? [backfillProvider] : []), + }, + plugins, + capabilities: { + /** @param {string} id */ + has: (id) => (id === 'hypaware.ai-gateway' ? registered.length > 0 : false), + require: () => gateway, + }, + /** @param {string[]} names */ + activatePluginClosure: async (names) => { + for (const name of names) { + if (!plugins.some((p) => p.name === name)) plugins.push({ name }) + } + if (names.includes('@hypaware/claude')) registered.push('claude') + return { activated: names, failed: [] } + }, + })) + return { ctx, stdout, stderr, attachCalls } +} + +/** @param {(home: string) => Promise | void} fn */ +async function withTempHome(fn) { + const dir = mkdtempSync(path.join(tmpdir(), 'hyp-attach-daemon-tails-')) + try { + await fn(dir) + } finally { + rmSync(dir, { recursive: true, force: true }) + } +} + +test('an explicit attach on a daemon-managed install re-arms a refused marker', async () => { + await withTempHome(async (home) => { + writeLocalConfig(home) + seedDaemonManagedAttach(home, 55555) + seedRefusedMarker(home) + + const { ctx, stdout, stderr, attachCalls } = makeDaemonManagedCtx({ home, preEnabled: true }) + const code = await runAttach(['claude'], ctx) + assert.equal(code, 0, stderr.text()) + + // The branch under test, not some other exit: nothing was re-wired. + assert.match(stdout.text(), /already attached/) + assert.deepEqual(attachCalls, [], 'a marker already at the live port is a settings no-op') + + assert.equal( + readClientActionStatus({ stateRoot: stateRoot(home) }).byKind.attach?.claude, + undefined, + 'the explicit re-run is the only re-arm a refused marker gets, and this is the install shape it is run on' + ) + }) +}) + +test('a done marker still survives that same daemon-managed attach', async () => { + await withTempHome(async (home) => { + writeLocalConfig(home) + seedDaemonManagedAttach(home, 55555) + const dir = path.join(stateRoot(home), 'config-control') + mkdirSync(dir, { recursive: true }) + const installed = [path.join(home, '.claude', 'skills', 'org-helper')] + writeFileSync( + path.join(dir, 'client-actions.json'), + JSON.stringify({ + attach: { + claude: { + status: 'done', + request_key: 'claude', + at: '2026-07-01T00:00:00.000Z', + installed_assets: installed, + }, + }, + }) + '\n' + ) + + const { ctx, stderr } = makeDaemonManagedCtx({ home, preEnabled: true }) + assert.equal(await runAttach(['claude'], ctx), 0, stderr.text()) + + // The re-arm is scoped to `refused`: a `done` marker is the only record + // naming the files an org-driven attach installed (LLP 0138#marker-undo). + const marker = readClientActionStatus({ stateRoot: stateRoot(home) }) + .byKind.attach?.claude + assert.equal(marker?.status, 'done') + assert.deepEqual(marker?.installed_assets, installed) + }) +}) + +test('the post-enable backfill offer is reached on the daemon-managed already-attached exit', async () => { + await withTempHome(async (home) => { + writeLocalConfig(home) + // The concrete state: the config lost the Claude adapter, but the settings + // marker still names the daemon's current port. The enable prompt turns + // the adapter on, the probe then reports `alreadyCurrent`, and step 4's + // offer must still be made for the client this invocation just enabled. + seedDaemonManagedAttach(home, 55555) + let ran = false + const provider = makeProvider('claude', () => { ran = true }) + const { ctx, stdout, stderr } = makeDaemonManagedCtx({ + home, + preEnabled: false, + answers: ['y', 'y'], + backfillProvider: provider, + }) + const code = await runAttach(['claude'], ctx) + assert.equal(code, 0, stderr.text()) + + assert.match(stdout.text(), /already attached/, 'the exit under test is the daemon-managed one') + assert.equal(ran, true, 'the accept path must reach its own backfill offer on this exit too') + assert.match(stdout.text(), /backfill claude: ok/) + + // The write really did land, so the enable half of the flow is genuine. + const written = JSON.parse(readFileSync(path.join(home, '.hyp', 'hypaware-config.json'), 'utf8')) + assert.ok( + written.plugins.some((/** @type {{ name: string }} */ p) => p.name === '@hypaware/claude'), + 'the enable prompt wrote the adapter entry' + ) + }) +}) + +test('an adapter that was already enabled reaches no backfill offer on that exit', async () => { + await withTempHome(async (home) => { + writeLocalConfig(home) + seedDaemonManagedAttach(home, 55555) + let ran = false + const provider = makeProvider('claude', () => { ran = true }) + // No enable prompt ran, so `activatedViaPrompt` is false and step 4 stays + // confined to the accept branch it is scoped to. + const { ctx, stdout, stderr } = makeDaemonManagedCtx({ + home, + preEnabled: true, + backfillProvider: provider, + }) + assert.equal(await runAttach(['claude'], ctx), 0, stderr.text()) + + assert.equal(ran, false) + assert.doesNotMatch(stdout.text(), /backfill claude:/) + }) +}) diff --git a/test/core/attach-enable-resume.test.js b/test/core/attach-enable-resume.test.js index 1f9964b8a..06866ceff 100644 --- a/test/core/attach-enable-resume.test.js +++ b/test/core/attach-enable-resume.test.js @@ -31,6 +31,9 @@ import { installFakeDaemonService } from '../helpers/daemon_service_fixture.js' // @ref LLP 0174#prompt [tests]: a config write that already landed means the // next invocation never re-prompts, it falls through to the registered/ // endpoint-unreachable path +// @ref LLP 0174#prompt [tests]: "each step reports its own failure" means one +// report, so the pre-write guided error is not printed under a step report +// that already described the state the write left behind /** @returns {{ write(chunk: unknown): boolean, text(): string }} */ function makeBuf() { @@ -82,9 +85,12 @@ function backupsIn(dir) { * pre-loaded with `answer`, and an `activatePluginClosure` stub mimicking * real in-process activation. * - * @param {{ home: string, answer?: string }} opts + * `activationFails` models the second partial-failure exit: the config write + * lands, but this process's own kernel never brings the plugin up. + * + * @param {{ home: string, answer?: string, activationFails?: boolean }} opts */ -function makeNotEnabledCtx({ home, answer }) { +function makeNotEnabledCtx({ home, answer, activationFails }) { /** @type {string[]} */ const registered = [] /** @type {{ name: string }[]} */ @@ -119,6 +125,7 @@ function makeNotEnabledCtx({ home, answer }) { }, /** @param {string[]} names */ activatePluginClosure: async (names) => { + if (activationFails) return { activated: [], failed: names } for (const name of names) { if (!plugins.some((p) => p.name === name)) plugins.push({ name }) } @@ -209,6 +216,41 @@ test('accept, write succeeds, restart fails: names the restart step, the backup // Exactly one backup: enableClientAdapter ran (and failed) exactly once, // not retried within this invocation. assert.equal(backupsIn(path.dirname(localConfigPath(home))).length, 1) + + // And nothing contradicts it. The caller's guided refusal was computed + // before the write and says the adapter "is not enabled ... add + // @hypaware/claude to and run 'hyp daemon restart'": under the + // line above, it denies the write that just landed and instructs an edit + // already made. One failure, one report. + assert.doesNotMatch(message, /error: the claude adapter is not enabled on this install/) + assert.doesNotMatch(message, /enable it with 'hyp init'/) + assert.equal( + (message.match(/error: /g) ?? []).length, + 1, + `expected exactly one error line, got:\n${message}` + ) + }) +}) + +test('accept, write succeeds, in-process activation fails: the activation report is not contradicted either', async () => { + await withTempHome(async (home) => { + writeLocalConfig(home) + // The other partial-failure exit below `enableClientAdapter`: the write + // (and, with no daemon marker on disk, no restart at all) succeeded, and + // this process could not activate what the config now names. + const { ctx, stderr } = makeNotEnabledCtx({ home, answer: 'y', activationFails: true }) + const code = await runAttach(['claude'], ctx) + assert.equal(code, 1) + + const message = stderr.text() + assert.match(message, /could not activate it in this process/) + assert.match(message, /re-run 'hyp attach claude' to finish/) + assert.doesNotMatch(message, /error: the claude adapter is not enabled on this install/) + assert.equal( + (message.match(/error: /g) ?? []).length, + 1, + `expected exactly one error line, got:\n${message}` + ) }) }) diff --git a/test/core/service-command-timeout.test.js b/test/core/service-command-timeout.test.js new file mode 100644 index 000000000..2e3fefdb4 --- /dev/null +++ b/test/core/service-command-timeout.test.js @@ -0,0 +1,188 @@ +// @ts-check + +import assert from 'node:assert/strict' +import { spawnSync } from 'node:child_process' +import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import path from 'node:path' +import process from 'node:process' +import test from 'node:test' +import { fileURLToPath, pathToFileURL } from 'node:url' + +// `hyp status` on darwin with a CA on disk shells out twice: `security +// verify-cert` and `launchctl getenv`. Neither had a deadline, and +// `runServiceCommand` only ever settled on the child's `close`, so a probe +// that never returned was not a caught error, it was a `hyp status` that +// printed nothing and never exited. macOS trust evaluation can reach the +// network for revocation, so an offline or captive-portal host on a +// proxy-mode install is the realistic trigger. +// +// Both facts under test are about a real spawned child that really does not +// return, so both are proved in a child process: the guard of LLP 0181 refuses +// every spawn inside the test runner, and a fake child would prove nothing +// about killing a real one. The scripts below are named `.mjs` and run with no +// `--test`, with `NODE_TEST_CONTEXT` deleted, so the guard is legitimately +// inactive in them rather than switched off. +// +// @ref LLP 0237#consequences [tests]: the trust line is reported, or reported +// unknown, but never at the cost of the report +// @ref LLP 0181#the-guard [constrained-by]: the spawn seam refuses inside the +// test runner, so a real hanging child has to be driven from outside it + +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..') + +/** @param {string} rel */ +function moduleUrl(rel) { + return pathToFileURL(path.join(REPO_ROOT, rel)).href +} + +/** @param {(dir: string) => void} fn */ +function withTempDir(fn) { + const dir = mkdtempSync(path.join(tmpdir(), 'hyp-svc-timeout-')) + try { + fn(dir) + } finally { + rmSync(dir, { recursive: true, force: true }) + } +} + +/** + * Write an executable shell stub that never returns, the shape of the failure + * this file exists for. `sleep` is a grandchild holding the pipe the wrapper + * inherited, so a kill of the stub alone does not close it: exactly the case + * that makes waiting for `close` after the kill the wrong move. + * + * @param {string} binDir + * @param {string} name + */ +function writeHangingStub(binDir, name) { + const stub = path.join(binDir, name) + writeFileSync(stub, '#!/bin/sh\nsleep 600\n') + chmodSync(stub, 0o755) + return stub +} + +/** + * Run a script outside the test runner and report how it went. `spawnSync`'s + * own timeout is the backstop that turns "the code under test hangs" into a + * failed assertion instead of a wedged suite. + * + * @param {string} script + * @param {{ pathPrefix?: string, timeout?: number }} [opts] + */ +function runOutsideTestRunner(script, opts = {}) { + /** @type {NodeJS.ProcessEnv} */ + const env = { ...process.env } + delete env.NODE_TEST_CONTEXT + delete env.HYP_ALLOW_REAL_SERVICE_MANAGER + if (opts.pathPrefix) env.PATH = `${opts.pathPrefix}${path.delimiter}${env.PATH ?? ''}` + const run = spawnSync(process.execPath, [script], { + cwd: REPO_ROOT, + encoding: 'utf8', + env, + timeout: opts.timeout ?? 60_000, + }) + return { + status: run.status, + signal: run.signal, + stdout: run.stdout ?? '', + output: `${run.stdout ?? ''}${run.stderr ?? ''}`, + } +} + +test('a service command that never returns is killed at its deadline, and the caller still exits', () => { + withTempDir((dir) => { + const binDir = path.join(dir, 'bin') + mkdirSync(binDir, { recursive: true }) + const hanging = writeHangingStub(binDir, 'hangs') + + const script = path.join(dir, 'timeout-probe.mjs') + writeFileSync(script, [ + `import { runServiceCommand } from ${JSON.stringify(moduleUrl('src/core/daemon/service_ops.js'))}`, + 'const startedAt = Date.now()', + 'let name = null', + 'try {', + ` await runServiceCommand(${JSON.stringify(hanging)}, [], { timeoutMs: 250 })`, + ' name = "resolved"', + '} catch (err) {', + ' name = err instanceof Error ? err.name : String(err)', + '}', + 'process.stdout.write(JSON.stringify({ name, elapsedMs: Date.now() - startedAt }))', + // No process.exit: the run only ends here if the killed child and its + // pipes stopped holding the event loop open, which is half the fix. + '', + ].join('\n')) + + const run = runOutsideTestRunner(script, { timeout: 20_000 }) + assert.equal( + run.signal, + null, + 'the probe never returned and nothing bounded it: hyp status would hang exactly like this', + ) + assert.equal(run.status, 0, run.output) + const result = JSON.parse(run.stdout) + assert.equal(result.name, 'ServiceCommandTimeoutError', run.output) + assert.ok(result.elapsedMs < 10_000, `expected the deadline to settle it, waited ${result.elapsedMs}ms`) + }) +}) + +// The end-to-end shape of the report: real `collectHypAwareStatus`, real +// probes, real spawn, against a `security` that never answers. The trust line +// reads unknown, and every other line is still rendered. +test('hyp status still renders when the darwin trust probe never returns', () => { + withTempDir((dir) => { + const binDir = path.join(dir, 'bin') + mkdirSync(binDir, { recursive: true }) + writeHangingStub(binDir, 'security') + // Only the trust probe hangs: `launchctl` answers at once, so its own line + // stays a real answer and the assertion below is about the hanging one. + const launchctl = path.join(binDir, 'launchctl') + writeFileSync(launchctl, '#!/bin/sh\nexit 1\n') + chmodSync(launchctl, 0o755) + + const script = path.join(dir, 'status-probe.mjs') + writeFileSync(script, [ + "import fs from 'node:fs'", + "import path from 'node:path'", + `import { collectHypAwareStatus } from ${JSON.stringify(moduleUrl('src/core/daemon/status.js'))}`, + `import { ensureLocalCa } from ${JSON.stringify(moduleUrl('src/core/tls/ca.js'))}`, + `import { defaultConfigPath } from ${JSON.stringify(moduleUrl('src/core/config/schema.js'))}`, + `const hypHome = ${JSON.stringify(path.join(dir, 'home'))}`, + "const stateRoot = path.join(hypHome, 'hypaware')", + "fs.mkdirSync(path.join(stateRoot, 'run'), { recursive: true })", + 'fs.writeFileSync(defaultConfigPath(hypHome), JSON.stringify({ version: 2, plugins: [] }) + "\\n")', + "const ca = await ensureLocalCa({ stateRoot, hosts: ['api.anthropic.com'] })", + 'const startedAt = Date.now()', + 'const report = await collectHypAwareStatus({', + " env: { ...process.env, HYP_HOME: hypHome, HYP_CONFIG: '' },", + " platform: 'darwin',", + ' isLaunchAgentInstalled: () => false,', + ' isSystemdUnitInstalled: () => false,', + '})', + 'process.stdout.write(JSON.stringify({', + ' elapsedMs: Date.now() - startedAt,', + ' fingerprintMatches: report.proxyTrust?.caFingerprint === ca.fingerprint,', + ' trusted: report.proxyTrust?.trusted ?? null,', + ' launchdEnvSet: report.proxyTrust?.launchdEnvSet ?? null,', + ' overall: report.overall,', + '}))', + // The report is what is under test, so end the run once it is printed + // rather than make this case depend on every handle the collector opens. + 'process.exit(0)', + '', + ].join('\n')) + + const run = runOutsideTestRunner(script, { pathPrefix: binDir, timeout: 60_000 }) + assert.equal( + run.signal, + null, + 'hyp status hung on an unbounded macOS trust probe instead of rendering a report', + ) + assert.equal(run.status, 0, run.output) + const result = JSON.parse(run.stdout) + assert.equal(result.trusted, null, 'a probe that never answered is unknown, never "not trusted"') + assert.equal(result.launchdEnvSet, false, 'the probe that did answer still reports its answer') + assert.equal(result.fingerprintMatches, true, 'the rest of the proxy-trust line is still rendered') + assert.equal(result.overall, 'healthy', 'an unknown trust line is not an outage') + }) +}) From 38606bea89392970c8719c881482bb5574105717 Mon Sep 17 00:00:00 2001 From: test Date: Wed, 19 Aug 2026 04:58:01 +0000 Subject: [PATCH 2/4] Review round: a failed config write keeps the guided remedy (#900) The `reported` bit added in be2b4e33 suppressed the caller's pre-write guided error for every `enableClientAdapter` failure, including `failedStep === 'write'`. That shape wrote nothing, so the guided error does not contradict it - and it is the only line naming the config path and the manual remedy. On an unwritable config the user was left with "the config write failed (...); nothing changed" and no next step. Scope `reported` to the failures below the write (`restart` / `wait`, and the incomplete in-process activation), which are the ones whose own report describes state that now exists on disk. Pinned by a new case in test/core/attach-enable-resume.test.js that fails on be2b4e33. Also corrects the daemon-managed re-arm comment: with no live endpoint to compare against, "already correct is the same fact as just written" is not true, so the comment now rests on what LLP 0186 actually scopes the re-arm to (the explicit manual re-run), not on a write having happened. Co-Authored-By: Claude Opus 5 (1M context) --- src/core/commands/clients.js | 42 ++++++++++++++++++-------- test/core/attach-enable-resume.test.js | 36 +++++++++++++++++++++- 2 files changed, 65 insertions(+), 13 deletions(-) diff --git a/src/core/commands/clients.js b/src/core/commands/clients.js index 66dd19c02..bcb2ca136 100644 --- a/src/core/commands/clients.js +++ b/src/core/commands/clients.js @@ -211,7 +211,9 @@ async function runClientLifecycle(action, argv, ctx) { // before that write and reads "not enabled ... add to // ": printing it under the first line contradicts it and // instructs an edit that has already been made. The guided message - // is for the paths that never wrote anything. + // is for the paths that never wrote anything, which includes a + // write that itself failed: there the guided remedy is still true + // and is the only line naming the config path. // @ref LLP 0174#prompt [implements]: each step reports its own // failure, so a step that reported one is not re-reported as the // pre-write refusal @@ -418,12 +420,17 @@ async function runClientLifecycle(action, argv, ctx) { // offer, the whole point of the accept path that just ran, is // never made. // - // Reached with the settings already at the live port rather than - // freshly written, which changes nothing either tail depends on: - // the re-arm's precondition is a successful attach (idempotent - // over its own output by construction, so "already correct" is - // the same fact as "just written"), and the offer's is that this - // invocation enabled the adapter. + // Reached with the settings already in place rather than freshly + // written, which changes nothing either tail depends on: the + // re-arm's precondition is an explicit `hyp attach` that + // succeeded (LLP 0186 scopes it to the manual re-run, not to a + // write having happened), and the offer's is that this + // invocation enabled the adapter. With a live endpoint the + // marker was validated against it, so "already correct" is the + // same fact as "just written"; with no live endpoint to compare + // against, a present marker is a no-op success by the pre-#277 + // rule above, and this is still the explicit re-run the re-arm + // is scoped to. // @ref LLP 0186#re-arm-explicit-hyp-attach-re-run-only [implements]: the explicit re-run re-arms, including on the daemon-managed install where the settings need no rewrite rearmRefusedAttachMarker({ name, ctx, dryRun: false }) // @ref LLP 0174#prompt [implements]: step 4's backfill consent @@ -705,10 +712,13 @@ function reportAttachEnablement({ name, enablement, parsed, ctx }) { * and no restart to undo. * * `reported` distinguishes the two ways this can answer "not activated". - * Every early return and the decline are the caller's own refusal to report, - * unchanged. The two failures below `enableClientAdapter` are not: each has - * already written a message describing the state it actually left on disk, so - * the caller's pre-write guided error would contradict it. + * Every early return, the decline, and a failed config write leave the disk + * exactly as the caller's pre-write guided error describes it, so that error + * is still the right thing to print: `reported` stays false. The failures + * *after* the write landed (a failed `restart`/`wait` step, and an + * incomplete in-process activation) have already described the state they + * left on disk, which that same guided error would deny: `reported` is true + * there, and the caller prints nothing more. * * @ref LLP 0174#bootstrap-floor [implements]: no local config file at all * skips the prompt outright and falls through to the caller's existing @@ -808,7 +818,15 @@ async function maybeInteractiveEnableAttach({ name, ctx, parsed, enablement }) { }) if (!result.ok) { reportEnableFailure({ name, result, ctx }) - return { activated: false, reported: true } + // Only a failure *below* the write owns the caller's message. Its report + // describes state that now exists on disk ("the config change already + // persists"), which the caller's pre-write guided error would deny. A + // failed write is the other shape {@link reportEnableFailure} documents: + // it changed nothing, so that guided error is still true, and it is the + // only line that names the config path and the manual remedy. Suppressing + // it there would leave "the config write failed; nothing changed" with no + // next step. + return { activated: false, reported: (result.failedStep ?? 'write') !== 'write' } } // The write and (if a daemon is installed) the restart already landed; what diff --git a/test/core/attach-enable-resume.test.js b/test/core/attach-enable-resume.test.js index 06866ceff..4732aaf46 100644 --- a/test/core/attach-enable-resume.test.js +++ b/test/core/attach-enable-resume.test.js @@ -1,7 +1,7 @@ // @ts-check import assert from 'node:assert/strict' -import { mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'node:fs' +import { chmodSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import path from 'node:path' import { PassThrough } from 'node:stream' @@ -254,6 +254,40 @@ test('accept, write succeeds, in-process activation fails: the activation report }) }) +test('accept, the write itself fails: the guided remedy still prints, because nothing was written to contradict', async () => { + await withTempHome(async (home) => { + writeLocalConfig(home) + // The third exit below `enableClientAdapter`, and the one that is NOT a + // partial failure: an unwritable config dir means neither the `.bak-` + // backup nor the rewrite can land, so the run ends exactly where it + // started. `reportEnableFailure`'s own message for this shape says only + // that the write failed and that nothing changed; it names no path and no + // next step, so the caller's pre-write guided error is the only line that + // does, and it is still true. Suppressing it here would be the reverse of + // the contradiction the two cases above pin. + const configDir = path.dirname(localConfigPath(home)) + chmodSync(configDir, 0o500) + try { + const { ctx, stderr } = makeNotEnabledCtx({ home, answer: 'y' }) + const code = await runAttach(['claude'], ctx) + assert.equal(code, 1) + + const message = stderr.text() + assert.match(message, /the config write failed/) + assert.match(message, /nothing changed/) + assert.match(message, /the claude adapter is not enabled on this install/) + assert.match(message, /add @hypaware\/claude to /) + } finally { + chmodSync(configDir, 0o700) + } + + // And the disk really is untouched: no entry, no backup. + const written = JSON.parse(readFileSync(localConfigPath(home), 'utf8')) + assert.deepEqual(written.plugins, []) + assert.equal(backupsIn(path.dirname(localConfigPath(home))).length, 0) + }) +}) + test('second invocation after a partial failure: config already carries the entry, so attach never re-prompts', async () => { await withTempHome(async (home) => { // Simulates the state left behind by the failed first run: the local From 926e4a31a8ff55691c802a403f07d33a22c7f225 Mon Sep 17 00:00:00 2001 From: test Date: Wed, 19 Aug 2026 06:32:52 +0000 Subject: [PATCH 3/4] Review round: re-arm before the asset tail, keep the machine record, probe in parallel (#900) Three review findings on the daemon-managed attach exit and the bounded macOS status probes, plus the LLP the first of them was missing. - The daemon-managed exit materialized assets *before* re-arming. The two tails are independent, so the order only shows on failure, and there it matters one way: `materializeAttachAssets` swallows a per-copy failure but not its plan read, prune pass, or asset digest, and a throw there lands in the loop's outer catch. With the re-arm second, that failure left the `refused` marker short-circuiting the reconciler forever, after exactly the explicit re-run LLP 0186 makes its only trigger. Both exits now re-arm first, the order the freshly-wired exit already used. - `reportAttachEnablement` was skipped whole when the enable prompt had already reported. It also emits the `client.attach.adapter_inactive` warn and the `--json` payload, neither of which any prompt writes, so a failed attach could leave no machine record at all. It takes a `quiet` bit now that drops only the human line, matching the capability gate above it, which has always suppressed only its `stderr.write`. - `collectProxyTrust` ran its two bounded probes in sequence, so the wedged host this bound exists for waited two deadlines to render. They read unrelated state and neither reads the other's answer, so they start together under `Promise.allSettled`, which keeps the per-probe independence the catches gave. New LLP 0266 settles what the first item assumed and LLP 0186 does not state: the re-arm fires at every success exit of an explicit `hyp attach`, including the one that calls no adapter `attach()` hook, and runs ahead of the asset tail. LLP 0186 keeps its text and gains the forward-ref. `test/core/attach-daemon-managed-tails.test.js` gains the ordering case: an asset tail that throws after the re-arm still leaves the marker cleared. It fails with the tails in the old order. npm test: 4492 pass, 0 fail, 1 skipped. npm run typecheck: clean. Co-Authored-By: Claude Opus 5 (1M context) --- llp/0186-reconciler-refused-marker.design.md | 2 +- ...s-at-both-attach-success-exits.decision.md | 118 ++++++++++++++++++ src/core/commands/clients.js | 57 ++++++--- src/core/daemon/status.js | 26 ++-- test/core/attach-daemon-managed-tails.test.js | 44 ++++++- 5 files changed, 213 insertions(+), 34 deletions(-) create mode 100644 llp/0295-rearm-fires-at-both-attach-success-exits.decision.md diff --git a/llp/0186-reconciler-refused-marker.design.md b/llp/0186-reconciler-refused-marker.design.md index 11cfce5ec..022eea3d3 100644 --- a/llp/0186-reconciler-refused-marker.design.md +++ b/llp/0186-reconciler-refused-marker.design.md @@ -5,7 +5,7 @@ **Systems:** Config, Daemon **Generated-by:** neutral **Related:** LLP 0036, LLP 0041, LLP 0086, LLP 0109, LLP 0184 -**Extended-by:** LLP 0250 (#how-the-reconciler-distinguishes-it-from-done: the reverse gap's drop also reads a `prior_done` bit, so a settings-only attach rewritten to `refused` is reversed rather than dropped) +**Extended-by:** LLP 0250 (#how-the-reconciler-distinguishes-it-from-done: the reverse gap's drop also reads a `prior_done` bit, so a settings-only attach rewritten to `refused` is reversed rather than dropped), LLP 0295 (#re-arm-explicit-hyp-attach-re-run-only: the re-arm fires at every success exit of the explicit re-run, including the daemon-managed one that calls no adapter `attach()`) > [LLP 0184](./0184-reconciler-retries-permanent-failures.issue.md) reports > that the action reconciler ([LLP 0036](./0036-central-config-driven-client-actions.decision.md) diff --git a/llp/0295-rearm-fires-at-both-attach-success-exits.decision.md b/llp/0295-rearm-fires-at-both-attach-success-exits.decision.md new file mode 100644 index 000000000..f333b8fc6 --- /dev/null +++ b/llp/0295-rearm-fires-at-both-attach-success-exits.decision.md @@ -0,0 +1,118 @@ +# LLP 0295: The refused re-arm fires at both `hyp client attach` success exits + +**Type:** Decision +**Status:** Accepted +**Systems:** Config, Daemon +**Generated-by:** neutral +**Date:** 2026-08-19 +**Related:** LLP 0086, LLP 0107, LLP 0138, LLP 0174, LLP 0184, LLP 0186, LLP 0250 + +> Extends [LLP 0186](./0186-reconciler-refused-marker.design.md) +> `#re-arm-explicit-hyp-attach-re-run-only`. LLP 0186 is Active and does not +> change: this document does not touch its rule (re-arm is the explicit +> `hyp client attach` re-run only, and only for a `refused` marker), it settles which +> *exits* of that command count as the re-run, because `hyp client attach` grew a +> second success exit after LLP 0186 was written. Raised as residual 3 of +> [hyparam/hypaware#887](https://github.com/hyparam/hypaware/issues/887) and +> as a review finding on +> [#900](https://github.com/hyparam/hypaware/pull/900). + +## Context + +LLP 0186 `#re-arm-explicit-hyp-attach-re-run-only` names the manual re-run as +the one and only trigger that clears a terminal `refused` marker, and locates +the call by the shape the command path had at the time: + +> after `registration.attach(...)` (or the equivalent adapter call the command +> path uses) resolves without throwing, and the run was not a `--dry-run` + +When that was written, `runClientLifecycle('attach', ...)` had exactly one +success exit, the one that calls the adapter's `attach()` hook, so "after +`attach()` resolves" and "this explicit re-run succeeded" named the same +moment. They no longer do. The daemon-managed install grew a second success +exit: the client is already attached at the live port, so there is nothing to +rewrite, the command reports success and returns without calling `attach()` +at all. + +That exit is the one an operator on a daemon-managed install most often +reaches, and it is precisely the exit a user reaches after fixing by hand the +precondition the reconciler refused on. Reading LLP 0186's sentence as a +mechanical requirement (the re-arm is attached to the `attach()` call, not to +the command) makes the re-arm unreachable on exactly the path that needs it, +for a marker whose entire design is that nothing else will ever clear it. + +## Decision + + + +### Both success exits + +**The re-arm fires at every success exit of an explicit, non-`--dry-run` +`hyp client attach `, including the daemon-managed exit that calls no +adapter `attach()` hook.** Everything LLP 0186 settled about the re-arm is +unchanged and carries to the new exit verbatim: it is gated on the marker's +prior status being `refused` (a `done` marker is never cleared, LLP 0138 +`#marker-undo`); a `refused` marker carrying `installed_assets` is rewritten +to `failed` rather than dropped; `--dry-run` re-arms nothing; and no +automatic, `isCurrent`-style re-arm is introduced here either (still the +follow-up candidate LLP 0186 names and does not build). + +Reaching the re-arm with the settings already correct rather than freshly +written satisfies the same precondition. LLP 0186's own reasoning for why a +manual attach may re-arm is that the adapter's write is idempotent over its +previous output, so the manual write and the reconciler's next `perform()` +"briefly do the same work twice, which is free". An adapter that is +idempotent over its own output makes "already correct" and "just written" the +same fact about disk; the daemon-managed exit is the case where the first +write already happened, not a case where no write is required. + +### Ordered ahead of the asset tail + +**At both exits the re-arm runs before `materializeAttachAssets`.** The two +tails are independent, so their order is only visible on failure, and there it +matters in one direction only. `materializeAttachAssets` swallows a per-copy +failure, but not everything it does is guarded: the plan read, the prune pass, +and the digest of an installed asset can all throw, and a throw there reaches +the command loop's outer catch. With the re-arm second, an asset-tail failure +would leave the `refused` marker short-circuiting the reconciler forever, +after exactly the explicit re-run that is its only trigger. With the re-arm +first, that failure costs the assets and nothing else. It cannot cost the +assets in the other direction, because the re-arm logs and swallows its own +marker error (`client.attach.marker_retract_failed`) rather than throwing. + +## Consequences + +- `src/core/commands/clients.js` calls one shared `rearmRefusedAttachMarker` + helper from both success exits, so a future third exit is a call site to + add rather than logic to re-derive. +- LLP 0186's `#re-arm-explicit-hyp-attach-re-run-only` keeps its text; its + `Extended-by:` line names this document. +- No change to `action_reconciler.js`, to `ActionMarkerStatus`, or to any + handler: this settles a call site, not the marker seam. + +## Test strategy + +Extends LLP 0186's own "Re-arm" cases to the second exit, in +`test/core/attach-daemon-managed-tails.test.js`: + +- A `refused` marker is re-armed when `hyp client attach` takes the daemon-managed + already-attached exit. +- A `done` marker and its `installed_assets` still survive that exit + untouched (LLP 0138 `#marker-undo` holds at both exits). +- The re-arm still fires when the asset tail after it fails, proving the + ordering above. + +## References + +- [LLP 0186](./0186-reconciler-refused-marker.design.md): the `refused` + marker state and the re-arm rule this document scopes to both exits +- [LLP 0138](./0138-client-assets-one-install.decision.md): `#marker-undo`, + why a `done` marker is never collateral of the re-arm +- [LLP 0107](./0107-skills-ride-attach.decision.md): the asset tail + the re-arm is now ordered ahead of +- [LLP 0174](./0174-attach-prompts-to-enable.design.md): the backfill offer, + the other tail this same exit was skipping +- [LLP 0250](./0250-marker-records-the-effect-it-overwrites.decision.md): the + other extension of LLP 0186's marker rules +- `src/core/commands/clients.js`: both success exits and the shared + `rearmRefusedAttachMarker` helper diff --git a/src/core/commands/clients.js b/src/core/commands/clients.js index bcb2ca136..3e2010b50 100644 --- a/src/core/commands/clients.js +++ b/src/core/commands/clients.js @@ -301,8 +301,13 @@ async function runClientLifecycle(action, argv, ctx) { if (!client) { if (enablement.state !== 'unknown') { // Same rule as the capability gate above: a prompt that already - // reported its own step's failure owns the message. - if (!promptResult.reported) reportAttachEnablement({ name, enablement, parsed, ctx }) + // reported its own step's failure owns the *human* message. Only + // that line is suppressed, exactly as the capability gate + // suppresses only its `stderr.write`: the structured warn and the + // `--json` payload are the machine record of a failed attach and + // no prompt writes either, so dropping them here would lose the + // failure from the log and from a scripted caller's output. + reportAttachEnablement({ name, enablement, parsed, ctx, quiet: promptResult.reported }) exitCode = 1 continue } @@ -401,17 +406,7 @@ async function runClientLifecycle(action, argv, ctx) { `the daemon manages attach for this install, so only its assets are refreshed.\n` ) } - // The settings are already wired, but attach means settings *and* - // assets, and this branch is the one an operator on a - // daemon-managed install actually reaches. Short-circuiting past - // the materialization below would make `hyp attach` install - // nothing on exactly the install shape it is most often run on, - // reintroducing the split this change removes. Idempotent and - // cheap, so running it on a no-op attach costs a stat pass. - // @ref LLP 0107#every-attach [implements]: every attach path - // materializes, including the one with nothing left to wire - await materializeAttachAssets({ name, descriptorMap, ctx, dryRun: false, json: parsed.json }) - // The two tails below `client.attach()` are reached from here + // The three tails below `client.attach()` are reached from here // too, because this branch is a successful attach: it is the // exit an explicit `hyp attach ` takes on a // daemon-managed install, the shape an operator most often runs @@ -421,9 +416,9 @@ async function runClientLifecycle(action, argv, ctx) { // never made. // // Reached with the settings already in place rather than freshly - // written, which changes nothing either tail depends on: the + // written, which changes nothing any tail depends on: the // re-arm's precondition is an explicit `hyp attach` that - // succeeded (LLP 0186 scopes it to the manual re-run, not to a + // succeeded (LLP 0295 scopes it to the manual re-run, not to a // write having happened), and the offer's is that this // invocation enabled the adapter. With a live endpoint the // marker was validated against it, so "already correct" is the @@ -431,8 +426,29 @@ async function runClientLifecycle(action, argv, ctx) { // against, a present marker is a no-op success by the pre-#277 // rule above, and this is still the explicit re-run the re-arm // is scoped to. - // @ref LLP 0186#re-arm-explicit-hyp-attach-re-run-only [implements]: the explicit re-run re-arms, including on the daemon-managed install where the settings need no rewrite + // + // Ordered re-arm before materialize, the same order the + // freshly-wired exit below uses. `materializeAttachAssets` + // swallows a per-copy failure, but not everything it does is + // guarded (the plan read, the prune pass, the digest of an + // installed asset), and a throw there lands in the loop's outer + // catch: with the re-arm second, an asset-tail failure would + // leave the `refused` marker short-circuiting the reconciler + // forever after exactly the explicit re-run that is its only + // trigger. The re-arm cannot fail the other way round, since it + // logs and swallows its own marker error. + // @ref LLP 0295#both-success-exits [implements]: the re-arm runs at whichever success exit the explicit re-run takes, ahead of the asset tail so an asset failure cannot swallow it rearmRefusedAttachMarker({ name, ctx, dryRun: false }) + // The settings are already wired, but attach means settings *and* + // assets, and this branch is the one an operator on a + // daemon-managed install actually reaches. Short-circuiting past + // the materialization would make `hyp attach` install nothing on + // exactly the install shape it is most often run on, + // reintroducing the split this change removes. Idempotent and + // cheap, so running it on a no-op attach costs a stat pass. + // @ref LLP 0107#every-attach [implements]: every attach path + // materializes, including the one with nothing left to wire + await materializeAttachAssets({ name, descriptorMap, ctx, dryRun: false, json: parsed.json }) // @ref LLP 0174#prompt [implements]: step 4's backfill consent // follows the accept path to whichever attach exit it reaches if (activatedViaPrompt) { @@ -667,15 +683,21 @@ async function resolveAttachEnablementState({ name, ctx }) { * attach failure in this file uses (the capability gate renders its own * because it also owns the failure span). * + * `quiet` drops only the human stderr line, for the caller whose enable + * prompt already printed a truer one; the structured warn and the `--json` + * payload always run, because no prompt writes either and they are the only + * machine-readable record that this attach failed. + * * @param {{ * name: string, * enablement: { state: 'not_enabled' | 'disabled_central', errorKind: string, message: string }, * parsed: { dryRun: boolean, json: boolean }, * ctx: CommandRunContext, + * quiet?: boolean, * }} args * @returns {void} */ -function reportAttachEnablement({ name, enablement, parsed, ctx }) { +function reportAttachEnablement({ name, enablement, parsed, ctx, quiet = false }) { getLogger('cmd-attach').warn('client.attach.adapter_inactive', { [Attr.COMPONENT]: 'cmd-attach', [Attr.OPERATION]: 'client.attach', @@ -696,6 +718,7 @@ function reportAttachEnablement({ name, enablement, parsed, ctx }) { ) return } + if (quiet) return ctx.stderr.write(`error: ${enablement.message}\n`) } diff --git a/src/core/daemon/status.js b/src/core/daemon/status.js index b5a99b093..b058014d0 100644 --- a/src/core/daemon/status.js +++ b/src/core/daemon/status.js @@ -2009,21 +2009,21 @@ async function collectProxyTrust({ platform, stateRoot, isCaTrustedFn, isLaunchd } if (!ca) return null + // Started together, not one after the other: the two probes read + // unrelated system state (the login keychain, the launchd environment) and + // neither reads the other's answer, so serializing them only adds their + // deadlines. On the wedged host this bound exists for that is the + // difference between the report stalling for one probe timeout and for + // two. `allSettled` keeps the per-probe independence the catches gave: one + // rejection reports its own line unknown and leaves the other's answer. + const [trustedResult, launchdResult] = await Promise.allSettled([ + isCaTrustedFn({ certPath: ca.certPath }), + isLaunchdEnvSetFn(), + ]) /** @type {boolean | null} */ - let trusted = null - try { - trusted = await isCaTrustedFn({ certPath: ca.certPath }) - } catch { - trusted = null - } - + const trusted = trustedResult.status === 'fulfilled' ? trustedResult.value : null /** @type {boolean | null} */ - let launchdEnvSet = null - try { - launchdEnvSet = await isLaunchdEnvSetFn() - } catch { - launchdEnvSet = null - } + const launchdEnvSet = launchdResult.status === 'fulfilled' ? launchdResult.value : null return { caFingerprint: ca.fingerprint, hosts: displayableCaHosts(ca.hosts), trusted, launchdEnvSet } } diff --git a/test/core/attach-daemon-managed-tails.test.js b/test/core/attach-daemon-managed-tails.test.js index 835b59d72..3bed2ed56 100644 --- a/test/core/attach-daemon-managed-tails.test.js +++ b/test/core/attach-daemon-managed-tails.test.js @@ -29,8 +29,9 @@ import { readClientActionStatus } from '../../src/core/config/action_reconciler. * * @import { CommandRunContext } from '../../hypaware-plugin-kernel-types.js' * - * @ref LLP 0186#re-arm-explicit-hyp-attach-re-run-only [tests]: the explicit - * re-run re-arms a refused marker on the daemon-managed install too + * @ref LLP 0295#both-success-exits [tests]: the explicit re-run re-arms a + * refused marker on the daemon-managed exit too, and does so ahead of the + * asset tail that can throw * @ref LLP 0174#prompt [tests]: step 4's backfill consent is reached from the * attach exit the accept path actually lands on here */ @@ -182,14 +183,20 @@ function makeProvider(name, onRun) { * discovery. `preEnabled` picks which of the two shapes this is: an adapter * already enabled coming in, or one the enable prompt turns on here. * + * `brokenAssets` makes the asset tail throw rather than warn: the plan is read + * through `skills.list()`, so a registry that cannot answer propagates out of + * `materializeClientAssets` into attach's outer catch, the same way the + * unguarded halves of the prune and digest passes would. + * * @param {{ * home: string, * preEnabled: boolean, * answers?: string[], * backfillProvider?: ReturnType, + * brokenAssets?: boolean, * }} opts */ -function makeDaemonManagedCtx({ home, preEnabled, answers = [], backfillProvider }) { +function makeDaemonManagedCtx({ home, preEnabled, answers = [], backfillProvider, brokenAssets = false }) { /** @type {string[]} */ const registered = preEnabled ? ['claude'] : [] /** @type {{ name: string }[]} */ @@ -237,6 +244,15 @@ function makeDaemonManagedCtx({ home, preEnabled, answers = [], backfillProvider cwd: home, env: { HOME: home, HYP_HOME: path.join(home, '.hyp') }, config: { version: 2 }, + ...(brokenAssets + ? { + skills: { + list() { + throw new Error('asset registry unavailable') + }, + }, + } + : {}), storage: { cacheRoot: home }, query: {}, backfillMaterializers: { get: () => undefined }, @@ -295,6 +311,28 @@ test('an explicit attach on a daemon-managed install re-arms a refused marker', }) }) +test('the re-arm survives an asset tail that throws after it', async () => { + await withTempHome(async (home) => { + writeLocalConfig(home) + seedDaemonManagedAttach(home, 55555) + seedRefusedMarker(home) + + const { ctx, stderr } = makeDaemonManagedCtx({ home, preEnabled: true, brokenAssets: true }) + // The asset tail's throw is reported by attach's outer catch, so this run + // is a failure overall. That is exactly the case the ordering is for. + assert.equal(await runAttach(['claude'], ctx), 1) + assert.match(stderr.text(), /asset registry unavailable/) + + // Ordered ahead of the asset tail: a failure there costs the assets, not + // the one re-arm a refused marker will ever be offered. + assert.equal( + readClientActionStatus({ stateRoot: stateRoot(home) }).byKind.attach?.claude, + undefined, + 'the re-arm must already have landed before the tail that failed' + ) + }) +}) + test('a done marker still survives that same daemon-managed attach', async () => { await withTempHome(async (home) => { writeLocalConfig(home) From 982c524757481bbeac970f33b89f5f01ab5d87df Mon Sep 17 00:00:00 2001 From: Phillip Cunliffe Date: Wed, 19 Aug 2026 14:46:50 -0700 Subject: [PATCH 4/4] Reconcile CLI names and timeout class after rebase --- src/core/commands/clients.js | 10 ++++---- src/core/daemon/service_ops.js | 24 ------------------- test/core/attach-daemon-managed-tails.test.js | 2 +- test/core/attach-enable-resume.test.js | 4 ++-- 4 files changed, 8 insertions(+), 32 deletions(-) diff --git a/src/core/commands/clients.js b/src/core/commands/clients.js index 3e2010b50..a3184eb64 100644 --- a/src/core/commands/clients.js +++ b/src/core/commands/clients.js @@ -408,7 +408,7 @@ async function runClientLifecycle(action, argv, ctx) { } // The three tails below `client.attach()` are reached from here // too, because this branch is a successful attach: it is the - // exit an explicit `hyp attach ` takes on a + // exit an explicit `hyp client attach ` takes on a // daemon-managed install, the shape an operator most often runs // it on. Skipping them made this the one path where the re-arm // LLP 0186 specifies never happens and where LLP 0174's step-4 @@ -417,7 +417,7 @@ async function runClientLifecycle(action, argv, ctx) { // // Reached with the settings already in place rather than freshly // written, which changes nothing any tail depends on: the - // re-arm's precondition is an explicit `hyp attach` that + // re-arm's precondition is an explicit `hyp client attach` that // succeeded (LLP 0295 scopes it to the manual re-run, not to a // write having happened), and the offer's is that this // invocation enabled the adapter. With a live endpoint the @@ -442,7 +442,7 @@ async function runClientLifecycle(action, argv, ctx) { // The settings are already wired, but attach means settings *and* // assets, and this branch is the one an operator on a // daemon-managed install actually reaches. Short-circuiting past - // the materialization would make `hyp attach` install nothing on + // the materialization would make `hyp client attach` install nothing on // exactly the install shape it is most often run on, // reintroducing the split this change removes. Idempotent and // cheap, so running it on a no-op attach costs a stat pass. @@ -552,7 +552,7 @@ async function runClientLifecycle(action, argv, ctx) { * * Scoped to a `refused` marker, and skipped on `--dry-run`, on purpose. A * `done` marker is the only record naming the files an org-driven attach - * installed, so clearing it would strand them past any later `hyp detach`, + * installed, so clearing it would strand them past any later `hyp client detach`, * which reads exactly this marker to know what to remove (LLP 0138#marker-undo). * A `failed` marker needs no help: nothing short-circuits it, so the next pass * already retries it. And a dry run must leave the marker store exactly as it @@ -573,7 +573,7 @@ async function runClientLifecycle(action, argv, ctx) { * exits, the freshly-wired one and the daemon-managed already-current one, and * the second silently had no re-arm at all while this lived in the first. * - * @ref LLP 0186#re-arm-explicit-hyp-attach-re-run-only [implements]: an explicit hyp attach re-arms a refused marker, and only that; the reconciler never re-arms one on its own + * @ref LLP 0186#re-arm-explicit-hyp-attach-re-run-only [implements]: an explicit hyp client attach re-arms a refused marker, and only that; the reconciler never re-arms one on its own * @param {{ name: string, ctx: CommandRunContext, dryRun: boolean }} args * @returns {void} */ diff --git a/src/core/daemon/service_ops.js b/src/core/daemon/service_ops.js index be2593140..8f3d478a3 100644 --- a/src/core/daemon/service_ops.js +++ b/src/core/daemon/service_ops.js @@ -31,30 +31,6 @@ export class ServiceOpError extends Error { } } -/** - * Error raised when a bounded service command passed its deadline and was - * killed. A distinct name so a caller can tell "the probe did not answer" - * apart from "the probe answered no": only the second is a fact about the - * machine. - */ -export class ServiceCommandTimeoutError extends ServiceOpError { - /** - * @param {string} message - */ - constructor(message) { - super(message) - this.name = 'ServiceCommandTimeoutError' - } -} - -/** - * How long a read-only status probe waits for a service command before it - * gives up. Long enough for a cold `security verify-cert` (keychain open, - * trust evaluation) on a healthy machine, short enough that the offline or - * captive-portal host it exists for still gets a report. - */ -export const SERVICE_PROBE_TIMEOUT_MS = 5000 - /** * Error raised when the test-runner guard refuses to spawn a service * manager. A distinct name so a caller (or a test) can tell "we declined diff --git a/test/core/attach-daemon-managed-tails.test.js b/test/core/attach-daemon-managed-tails.test.js index 3bed2ed56..517dfc6aa 100644 --- a/test/core/attach-daemon-managed-tails.test.js +++ b/test/core/attach-daemon-managed-tails.test.js @@ -13,7 +13,7 @@ import { readClientActionStatus } from '../../src/core/config/action_reconciler. /** * The daemon-managed "already attached at the live port" branch is a success - * exit of `hyp attach`, and it is the one an operator on a default install + * exit of `hyp client attach`, and it is the one an operator on a default install * actually reaches: the gateway is not bound in this CLI process, no `listen` * is configured, and the marker in the client's settings already names the * daemon's live port. It used to `continue` straight after materializing diff --git a/test/core/attach-enable-resume.test.js b/test/core/attach-enable-resume.test.js index 4732aaf46..2213e70e6 100644 --- a/test/core/attach-enable-resume.test.js +++ b/test/core/attach-enable-resume.test.js @@ -223,7 +223,7 @@ test('accept, write succeeds, restart fails: names the restart step, the backup // line above, it denies the write that just landed and instructs an edit // already made. One failure, one report. assert.doesNotMatch(message, /error: the claude adapter is not enabled on this install/) - assert.doesNotMatch(message, /enable it with 'hyp init'/) + assert.doesNotMatch(message, /enable it with 'hyp setup'/) assert.equal( (message.match(/error: /g) ?? []).length, 1, @@ -244,7 +244,7 @@ test('accept, write succeeds, in-process activation fails: the activation report const message = stderr.text() assert.match(message, /could not activate it in this process/) - assert.match(message, /re-run 'hyp attach claude' to finish/) + assert.match(message, /re-run 'hyp client attach claude' to finish/) assert.doesNotMatch(message, /error: the claude adapter is not enabled on this install/) assert.equal( (message.match(/error: /g) ?? []).length,