Attach and status residuals: bound the macOS status probes, drop the contradictory enablement line, reach both attach tails

[philcunliffe]

All three residuals in #887 reproduced, and all three are fixed. Each has a
test that failed on master before the change and passes after.

1. hyp status could hang indefinitely (finding 1)

collectProxyTrust shells out twice on darwin with a CA on disk, and
runServiceCommand only ever settled on the child's close. Its try/catch
turns a thrown probe into null, but a probe that never returns is not a
throw: hyp status printed nothing and never exited.

runServiceCommand gains an opt-in timeoutMs. On the deadline it kills
the child, destroys its stdio and unrefs it (so the caller can exit even if
a grandchild still holds the inherited pipe), and rejects with a new
ServiceCommandTimeoutError. The two read-only probes (isCaTrusted,
isLaunchdEnvSet) pass SERVICE_PROBE_TIMEOUT_MS, so the rejection lands in
the existing catch and the trust line reads unknown, which is the honest
answer, rather than holding up the whole report.

The bound is deliberately opt-in: launchctl bootstrap and
security add-trusted-cert sitting behind its password dialog may
legitimately block for minutes and must not be cut off mid-write, so they
pass nothing and keep the unbounded wait they have always had. Only a
bounded caller is ever killed, so the SIGKILL can never interrupt a
half-applied mutation.

Reproduced in test/core/service-command-timeout.test.js. Both cases drive a
real sh stub that never returns, from a child process: LLP 0181's guard
refuses every spawn inside the test runner, and a fake child would prove
nothing about killing a real one. The second case runs the real
collectHypAwareStatus with real probes against a security on PATH that
never answers, and asserts the report still renders. Before the fix both runs
had to be killed by spawnSync's own timeout ('SIGTERM' !== null).

2. Contradictory enablement message after a partial attach (finding 2)

When maybeInteractiveEnableAttach got past enableClientAdapter but still
returned { activated: false }, it had already reported the failed step and
the state it left on disk. The caller then unconditionally printed
enablement.message, computed before the write, which denies that write and
instructs a config edit already made.

maybeInteractiveEnableAttach now returns a reported bit, set at the two
exits that have already written their own message (reportEnableFailure, and
the activation_incomplete path), and both call sites skip the pre-write
guided error when it is set. Every early return and the decline are unchanged:
none of them wrote anything, so the caller's message is still the right one
there. --json and non-TTY callers never reach the prompt at all, so their
payloads are untouched. This is LLP 0174 #prompt's "each step reports its own
failure", read as one report per failure.

Covered in test/core/attach-enable-resume.test.js: the existing
write-succeeded/restart-failed case now also pins that exactly one error:
line is printed, and a new case does the same for the activation-incomplete
exit.

3. Skipped re-arm and backfill offer (finding 3)

The daemon-managed "already attached at the live port" branch continued
right after materializeAttachAssets, so it reached neither tail below
client.attach(). That made it the one attach path where LLP 0186's re-arm,
the only re-arm a refused marker ever gets and whose sole trigger is this
explicit re-run, never happened, and where LLP 0174 step 4's backfill offer,
the whole point of the accept path, was never made.

Both tails are now reached from that branch too. The re-arm moved into a
shared rearmRefusedAttachMarker helper (unchanged logic, same refused-only
scope and --dry-run skip) so neither of attach's two success exits can
silently lose it again. Reaching it with the settings already current rather
than freshly written changes nothing either tail depends on: every attach
adapter is idempotent over its own output by construction, so "already
correct" is the same fact as "just written", and the offer's precondition is
that this invocation enabled the adapter.

New file test/core/attach-daemon-managed-tails.test.js, four cases: the
re-arm fires on that exit; a done marker and its installed_assets still
survive it (LLP 0138 #marker-undo); the post-enable backfill offer is reached
after the enable prompt lands on it; and an adapter that was already enabled
still reaches no offer. Cases 1 and 3 failed before the change.

Checks

• npm test: 4490 pass, 0 fail, 1 skipped
• npm run typecheck: clean
• npm run smoke -- status_diagnostics, npm run smoke -- client_attach_idempotent: ok

Nothing in #887 was left unfixed.

Fixes#887

[philcunliffe]

Neutral review round: be2b4e33 - findings (1 fixed, 1 comment corrected, 1 accepted)

Verdict: no blocking bugs. All three residuals in #887 are genuinely fixed and each carries a test that fails on the parent commit. On the reviewed head, npm test (4490 pass / 0 fail / 1 skip), npm run typecheck, npm run smoke -- status_diagnostics and npm run smoke -- client_attach_idempotent were all reproduced green in an isolated worktree.

Follow-up pushed as 3ba2abf6.

Findings

1. Medium - src/core/commands/clients.js:811 (reviewed head): a failed config write lost its remedy. FIXED.

reported: true was returned for everyenableClientAdapter failure, including failedStep === 'write'. That shape wrote nothing to disk, so the caller's pre-write guided error does not contradict it - and it is the only line that names the config path and the manual remedy (enable it with 'hyp init', or add @hypaware/claude to <configPath> and run 'hyp daemon restart').

The PR's own justification only covers the post-write case; the call-site comment at clients.js:207-215 literally quotes the restart-failure wording ("the config change already persists") as the reason, and reportEnableFailure's JSDoc at clients.js:1173-1177 already documents the two shapes as deliberately different. The code suppressed both.

Concrete effect: with an unwritable config directory, hyp attach claude printed only

error: could not enable the claude adapter: the config write failed (EACCES...); nothing changed

with no path and no next step. Before this PR the guided remedy printed under it.

Fix: reported: (result.failedStep ?? 'write') !== 'write', so the bit is scoped to the failures whose own report describes state that now exists on disk (restart / wait, and the incomplete in-process activation). The JSDoc and the call-site comment were corrected to match. New case in test/core/attach-enable-resume.test.js ("accept, the write itself fails: the guided remedy still prints..."), verified to fail on be2b4e33 and pass after.

2. Low - src/core/commands/clients.js:428 (reviewed head): the re-arm comment overstated its invariant. COMMENT CORRECTED, behavior unchanged.

The new rearmRefusedAttachMarker(...) fires for the whole alreadyCurrent branch, which includes the liveEndpoint === undefined sub-case (daemon not running) where a bare settings marker is accepted with no port validation at all. There the comment's "'already correct' is the same fact as 'just written'" is not true: nothing was verified and nothing was written. The behavior is still defensible - LLP 0186 scopes the re-arm to the explicit manual re-run that succeeded, not to a write having happened - and the worst case is marker churn (the next reconcile pass re-perform()s and re-refuses). The comment now rests on what LLP 0186 actually says instead of on an invariant that does not hold in that sub-case.

3. Low - test/core/service-command-timeout.test.js case 2: ~5s of tier-1 runtime. ACCEPTED AS-IS, no change.

The second case drives the real SERVICE_PROBE_TIMEOUT_MS (5000ms) end-to-end through collectHypAwareStatus, which has no injection point for it, costing ~5.1s of a ~16s suite and growing if the constant is raised. Left alone deliberately: the case's whole value is "real collectHypAwareStatus, real probes, real spawn, real hanging child", and threading a timeout override through the collector would add production API surface purely for the test while weakening exactly what it proves. Case 1 already pins the bound cheaply at timeoutMs: 250. Worth revisiting only if the constant grows.

Checked and found correct

• promptResult is in scope at both new report sites; --json can never reach reported: true (the prompt returns early on parsed.json), so every JSON failure payload is unchanged.
• The alreadyCurrent branch is unreachable under --dry-run (it lives in the non-dry-run else), so the hardcoded dryRun: false is right.
• rearmRefusedAttachMarker is a behavior-preserving extraction of the deleted inline block; the continue prevents any double re-arm or double backfill offer.
• service_ops.js: the timer is cleared on both error and close; a late close resolving after a timeout reject is a harmless no-op; only the two read-only probes pass timeoutMs, so the SIGKILL can never cut a mutating command mid-write; proc.unref() plus destroying both pipes does let the caller exit with a grandchild still holding the pipe.
• isCaTrusted / isLaunchdEnvSet have no production callers outside collectProxyTrust, which catches rejection and maps it to null; installCaTrust / removeCaTrust and installLaunchdEnv / removeLaunchdEnv keep the unbounded defaultRunner.
• collectHypAwareStatus is not on any daemon loop, so the 5s-per-probe bound is a strict improvement over the previous unbounded hang.
• Every @ref anchor introduced resolves: LLP 0186 #re-arm-explicit-hyp-attach-re-run-only, LLP 0174 #prompt, LLP 0237 #consequences, LLP 0181 #the-guard, LLP 0138 #marker-undo.
• Style: no U+2014 anywhere in the diff, no trailing semicolons, types stay in JSDoc.

Verification of the pushed fix

be2b4e33..3ba2abf6 touches src/core/commands/clients.js and test/core/attach-enable-resume.test.js only. Confirmed against the reviewed tree: the enableClientAdapter-failure exit changed from reported: true to the narrowed expression, the activation_incomplete exit still returns reported: true, and the new test case exists in 3ba2abf6 and not in be2b4e33. npm test on 3ba2abf6: 4491 pass, 0 fail, 1 skipped. npm run typecheck: clean.

[philcunliffe]

Neutral review round: 3ba2abf (own PR)

Verdict: 4 findings, all low / low-medium. All 4 fixed and verified; nothing left open.

Reviewed the head SHA in a detached worktree with /code-review at high effort, reading every hunk plus the surrounding code (clients.js's attach loop, client_enable.js's result shapes, collectProxyTrust, every runServiceCommand caller). The core of the PR holds up: dryRun is provably false on the daemon-managed branch (it sits inside the non-dry-run else), failedStep is set on every pre-write failure path so reported never suppresses a still-true remedy, the probe runners are reachable only from collectProxyTrust, and the timeout's close/error handlers both clear the timer with no unhandled-error path.

Fixes pushed as 2c29e66.

1. Tail ordering loses the re-arm on an asset failure (low-medium) — FIXED

src/core/commands/clients.js:413 (pre-fix): the daemon-managed exit ran materializeAttachAssetsbeforerearmRefusedAttachMarker (line 435), the opposite of the freshly-wired exit below, which re-arms at line 500 and materializes at 509.

The two tails are independent, so the order is only visible on failure, and there it matters one way. materializeClientAssets swallows a per-copy failure but not its plan read (skills.list()), its prune pass, or its asset digest, and a throw there lands in the attach 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. The same failure on the other exit re-armed fine.

Both exits now re-arm first. The re-arm cannot cost the assets in the other direction: it logs and swallows its own marker error (client.attach.marker_retract_failed) rather than throwing.

Pinned by a new case in test/core/attach-daemon-managed-tails.test.js, "the re-arm survives an asset tail that throws after it". Verified it fails with the tails in the old order (not ok 2, 4 pass / 1 fail) and passes after (5 pass / 0 fail).

2. The registry-miss gate suppressed the machine record too (low) — FIXED

src/core/commands/clients.js:305 (pre-fix): if (!promptResult.reported) reportAttachEnablement(...) skipped the helper whole. That helper also emits the client.attach.adapter_inactive structured warn and, under --json, the machine payload on stdout, and no prompt writes either of those. The capability gate at line 207 suppresses only its stderr.write and keeps its JSON payload and failure span, so the two gates disagreed about what "already reported" covers.

The --json half is latent rather than live (reported can only be true after an interactive prompt, which bails on --json), but the dropped warn is reachable today and this repo's log-driven-development rule makes a failed attach that leaves nothing in the log a real loss.

reportAttachEnablement now takes quiet?: boolean that drops only the human stderr line; the warn and the JSON payload always run. Call site at clients.js:310, guard at clients.js:721.

3. The re-arm @ref claimed a scope LLP 0186 does not state (low) — FIXED

src/core/commands/clients.js:434 (pre-fix) annotated the new call @ref LLP 0186#re-arm-explicit-hyp-attach-re-run-only [implements]. LLP 0186 is Active and locates the re-arm "after registration.attach(...) ... resolves without throwing"; this exit never calls the adapter's attach() hook. The behaviour is right (it is still the explicit re-run the doc scopes the re-arm to, and the exit a user reaches after fixing the refused precondition by hand), but extending what an Active doc settled without landing the doc edit in the same commit is exactly what CLAUDE.md's "living docs" / "keep refs honest" rules forbid.

Added llp/0266-rearm-fires-at-both-attach-success-exits.decision.md (Accepted), which settles both this round's design points: the re-arm fires at every success exit of an explicit non---dry-runhyp attach, including the one that calls no adapter hook, and it runs ahead of the asset tail. LLP 0186 keeps its text and gains the forward-ref on its Extended-by: line. The code and test annotations now point at LLP 0266#both-success-exits.

4. Bounded probes still ran in sequence (low) — FIXED

src/core/daemon/status.js:1574 (pre-fix): collectProxyTrust awaited isCaTrustedFn then isLaunchdEnvSetFn, so on the offline / captive-portal host this PR exists for, hyp status blocked up to 2 x SERVICE_PROBE_TIMEOUT_MS (10s) before rendering, with both lines unknown anyway.

They read unrelated system state (the login keychain, the launchd environment) and neither reads the other's answer, so serializing them only added their deadlines. Now started together under Promise.allSettled (status.js:1581), which keeps exactly the per-probe independence the two catches gave: one rejection reports its own line unknown and leaves the other's answer. Worst case halves to one deadline.

Checks

• npm test: 4492 pass, 0 fail, 1 skipped (4493 total, up 2 from the new case)
• npm run typecheck: clean
• Each fix verified against the pre-fix tree by name: rearmRefusedAttachMarker moved from line 435-after-413 to 441-before-451; quiet absent from reportAttachEnablement's signature at HEAD~1:678 and present at HEAD:700; Promise.allSettled count 0 at HEAD~1, present at HEAD:1581; llp/0266-*.md present in the committed tree and named on LLP 0186's Extended-by: line.

No merge, no ready, nothing pushed to master.

[philcunliffe]

Note on the new llp/0266-rearm-fires-at-both-attach-success-exits.decision.md added in this round: three other open branches already claim llp/0266 (#850/#914, #893, and #866), each with a different document. master claims none.

The reviewer could not have seen this from inside the branch, which is exactly the failure issue #907 describes and PR #916 gates. Full claimant table re-derived from every ref: #907. This document will need renumbering before or at merge, depending on the order you land these.

[philcunliffe]

Triage at the review-round cap, head 2c29e66558e2d7eecb840fccb8b3575969cc597b.

Verified against the code at head: every finding from both review rounds is fixed there.

• Round 1 (be2b4e33): the write-failure remedy is restored (clients.js:852 scopes reported to non-write steps), the re-arm comment was corrected, and the ~5s test case was accepted as-is.
• Round 2 (3ba2abf6): the re-arm now runs ahead of the asset tail at both success exits (clients.js:441 before :451, :516 before :525); reportAttachEnablement keeps its structured warn and --json payload under quiet (clients.js:700-722); the @ref now points at LLP 0266#both-success-exits, the doc exists at head, and LLP 0186 carries the forward-ref; the two trust probes run in parallel under Promise.allSettled (status.js:1581).

Two residuals, both non-blocking, deferred to #919:

1. Preference - test/core/service-command-timeout.test.js case 2 costs ~5s of tier-1 runtime; deliberately accepted in round 1.
2. Docs numbering - llp/0266-* is also minted by three other open branches (Three open branches each mint llp/0266-*, so the second one merged silently collides #907); master claims none today, so this PR is clean if it lands first, and the renumber duty if it lands second is recorded on Follow-up: deferred review findings from PR #900 #919.

No blocker: nothing residual can cause wrong behavior, data loss, a security hole, a crash, or a perf regression. The PR can merge safely once a human lands it.

[philcunliffe]

Triage at the review-round cap, head 1df26b5db4c4054ebeee6a3c1bfb59011314b912 (merge of origin/master into fix/issue-887).

The head moved past the last triaged commit (2c29e66) by one merge commit that resolved real conflicts: master independently landed a runServiceCommand timeout in #833, and the merge keeps master's ServiceCommandTimeoutError mechanism plus this branch's deadline handling (stdio destroy + unref). Re-verified in an isolated worktree at head:

• Every finding from both review rounds is still fixed after the merge: the write-failure remedy scope (src/core/commands/clients.js:852), the re-arm ahead of the asset tail at both success exits (clients.js:441 before :451, :516 before :525), reportAttachEnablement's quiet keeping the structured warn and --json payload (clients.js:310, :700), the @ref LLP 0266#both-success-exits doc present with LLP 0186's forward-ref, and the two trust probes in parallel under Promise.allSettled (src/core/daemon/status.js:1863) with the bound wired at status.js:1713-1716.
• npm test: 4537 pass, 0 fail, 1 skipped. npm run typecheck: clean. No U+2014 in any touched file.
• LLP numbering at this head: master claims no llp/0266; the branch's llp/ diff vs master is only the new 0266 doc plus LLP 0186's Extended-by: line, so the merged tree is internally consistent.

Two residuals, both non-blocking, remain deferred to #919:

1. Preference - test/core/service-command-timeout.test.js case 2 still costs ~5.5s of tier-1 runtime (the constant is now TRUST_PROBE_TIMEOUT_MS after the merge; Follow-up: deferred review findings from PR #900 #919's description names the pre-merge constant).
2. Docs numbering - llp/0266-* is still also minted by other open branches (Three open branches each mint llp/0266-*, so the second one merged silently collides #907); this PR is clean if it lands first, and the renumber duty if it lands second is recorded on Follow-up: deferred review findings from PR #900 #919.

No blocker: nothing residual can cause wrong behavior, data loss, a security hole, a crash, or a perf regression. The PR can merge safely once a human lands it.