Repository navigation
Preserve proxy-cleanup diagnostics on stderr when client detach uses JSON output #829
Description
Ready scope
Fix the remaining live output defect: hyp client detach <client|all> --purge --json currently discards every line from purgeProxyTrustResidue. Keep the existing per-client NDJSON on stdout unchanged, and write the purge diagnostic lines to stderr in JSON mode. Text mode keeps its current output. Preserve current best-effort cleanup and exit-code semantics.
No new JSON envelope, warnings field, trust installation, CA rotation policy, timeout tuning or keychain mutation is part of this fix. The remaining historical findings are disposed of in the triage comment below.
Original issue, retained for history
Filed at triage of PR #800 (head b8a4033d54997bd59e014cb1f462a1c2b427a45e), whose Fixes #793 closes that issue on merge. Every residual finding from the two review rounds on #800 was re-verified against the current head from Linux; all are non-blocking. This issue is the carrier so none is lost. Full analysis lives in the two review comments on PR #800 and on #793 / #790.
Deferred review findings (each re-verified at the PR head)
1. Exactly 8 duplicate certificates produce a false residue warning (nit)
src/core/tls/darwin_trust.js:129-155. The sweep's only exit that clears the warning is the not-found pass, so 8 successful deletes consume the whole bound and fall out with detail set: a keychain the sweep just cleared completely still prints ! keychain trust may not be fully removed (stopped after 8 passes...), sending the user to Keychain Access for nothing. Inherent to any bound, hedged by "may", and reached only after eight CA re-mints. Both review rounds concluded the obvious fix (a confirming extra pass) just moves the edge to 9 and churns the bound test; leave it unless the bound is touched for another reason, and then add a doc-comment line.
2. The two trust probes run serially, so hyp status can block 2 x 5s (nit)
src/core/daemon/status.js:1345-1358 awaits isCaTrustedFn then isLaunchdEnvSetFn, each bounded by TRUST_PROBE_TIMEOUT_MS (5s, status.js:1284). Worst case (locked login keychain) is a 10s stall instead of 5s, then both render unknown. Parallelising with per-probe catches attached before the await keeps the tri-state exactly, but restructures the block that carries the argument for independent catches. Not worth it alone; noted for whoever next touches the collector.
3. hyp detach --purge --json drops the purge lines entirely (pre-existing, wants a design decision)
src/core/commands/clients.js:150-153 gates purged.lines on if (!parsed.json), so the residue warning from finding 1 above, and every other purge line, is invisible to a scripted caller. Pre-existing shape, not made worse by #800. The fix is a JSON-envelope decision (a purge block? fold into a warnings key?), which wants a short decision doc, not a drive-by repair.
4. A dNSName containing ", " renders as two entries in the joined text line (nit)
displayableCaHosts (src/core/tls/ca.js:414-421) strips control bytes, bounds length and count, but a printable comma-space inside one entry is indistinguishable from the separator in the joined text surfaces (src/core/commands/status.js:643, hypaware-core/plugins-workspace/claude/src/index.js:550). It can only appear to widen the stated grant, never narrow it; --json is unambiguous; unreachable through our own mint (assertAsciiHost). Left as recorded.
Design items still open from #793 (carried so the close loses nothing)
- F1 (medium): the keychain grant is all-policy.
security add-trusted-cert -r trustRootwith no-ptrusts every policy while the name constraints only bound TLS server identity. Adding-p ssltouches the exact keychain-merge path the live macOS runs proved, so it needs a Mac plus a Remote Control acceptance re-run; alternatives (EKU serverAuth re-mint, or accept-and-record via a doc extending LLP 0237) are on Follow-up: deferred review findings from PR #782 #790/Follow-up: F1 and F2 from #790, plus proxy-trust status findings deferred from PR #792 #793. - F3 residue: lifecycle gaps. Fingerprint-drift detection between the on-disk CA and the trusted keychain entry (LLP 0238#ten-year-validity requires renewal to surface; nothing does), and narrower re-mint triggers so config churn does not rotate the CA. Both change lifecycle behaviour and want a decision doc.
- Post-detach repair prompt (low). After
hyp detach claude,hyp statusrenderslaunchd env: NODE_USE_SYSTEM_CA not set - run 'hyp attach claude' to set itindefinitely for a state the user chose. Gating on "a proxy attach is live" needsmodeplumbed out of the attach probe intoClientAttachReport, plus a decision about what status claims in the CA-minted-never-attached state.
macOS acceptance items (explicitly owed; unprovable from Linux)
Whichever change next ships in this area should carry a docs/ACCEPTANCE.md pass on a real Mac covering:
security delete-certificate -cmulti-match semantics. The hyp status names the CA's permitted hosts, uninstall clears every duplicate root (#793) #800 sweep is safe under either reading (byte-identical argv per pass, hard bound, every non-zero exit either ends cleanly or carries a detail), but which reading is true is unverified. On a Mac with a 2-3x re-minted CA, runhyp daemon uninstalland assert Keychain Access holds noHypAware Local CA.- The 5s probe timeout in situ. The unit test proves
runServiceCommandkills and rejects; it cannot prove a locked login keychain's GUI prompt is actually cleared by the SIGKILL, nor that 5s is right for a cold keychain. Lock the login keychain, runhyp status, confirm the line readsunknown - the keychain probe could not runand no dialog is left on screen. - The rendered attach-dialog line.
ensureDarwinProxyTrustis darwin-gated and unexported, so the round-2 sanitize wiring is proven only at the shared-helper level. On a real CA the dialog preamble should read(limited to: api.anthropic.com, api.openai.com, chatgpt.com), exactly as before. - F1, if attempted: the
-p sslvariant needs the Remote Control acceptance check re-run, since Bun's keychain merge honouring a policy-scoped trust setting is the unverified part.
Backlinks: PR #800 (fix and both review rounds), #793 (the findings issue this PR closes), #790 (original analysis).
Activity
philcunliffe commentedon Aug 18, 2026
Picked this up as a
neutral:fixfollow-up. I worked from a detached worktree oforigin/master(a14246d9, green) with a freshnpm install, on Linux. Three of the four deferred findings need code that is only on PR #800's branch; the fourth is onmasterbut is blocked on a design decision the issue itself reserves. No PR.Per finding
1. Eight duplicate certificates produce a false residue warning. Needs the bounded sweep at
src/core/tls/darwin_trust.js:129-155.master'sdarwin_trust.jsis 112 lines long and has no pass loop and no bound at all; the sweep is added by PR #800 (origin/fix/issue-793, +62 lines in that file), which is still OPEN and unmerged. Unreachable frommaster.2. The two trust probes run serially, so
hyp statuscan block 2 x 5s. NeedsTRUST_PROBE_TIMEOUT_MS, which does not exist onmaster; it is introduced atsrc/core/daemon/status.js:1284onorigin/fix/issue-793.masterdoes awaitisCaTrustedFnthenisLaunchdEnvSetFnserially (src/core/daemon/status.js:1315and:1323), but with no bound on either, so the finding as written (worst case 10s instead of 5s, both renderingunknown) describes the PR's code and notmaster's. Parallelisingmaster's version would be a different change carrying a different argument. Unreachable as stated.3.
hyp detach --purge --jsondrops the purge lines. Reachable, and I confirmed it onmaster:src/core/commands/clients.js:150-153gatesfor (const line of purged.lines)onif (!parsed.json), andpurgeProxyTrustResidue(:1602) really does return user-facing warning lines onmastertoday, including the CA-not-removed andNODE_USE_SYSTEM_CA-not-unset cases. So a scripted caller loses real warnings, independent of #800. I did not fix it, because the issue is right that the fix is an envelope decision and not a repair:detachClientViaCorewrites one JSON object per client to stdout as NDJSON and the purge runs after that loop, so there is no existing object to hang apurgeorwarningskey on. The three candidate shapes (a trailing purge object, awarningskey folded into the last client object, or warnings to stderr leaving stdout pure NDJSON) are a user-visible contract choice, and picking one unasked on a finding both review rounds deliberately deferred is exactly the drive-by the issue warns against. This is a decision block, not a sequencing block.4. A dNSName containing
", "renders as two entries. NeedsdisplayableCaHosts, which does not exist anywhere onmaster; it is added atsrc/core/tls/ca.js:413onorigin/fix/issue-793. Unreachable frommaster. The issue records it as left alone deliberately in any case.The rest of the issue
The design items carried from #793 (F1's all-policy keychain grant, F3's fingerprint-drift and re-mint-trigger lifecycle gaps, the post-detach repair prompt) are each described in the issue as wanting a decision doc or a Mac, and the four macOS acceptance items are unprovable from this Linux host by construction. None of them is worker-fixable here.
Why this is a block, not a failed attempt
Findings 1, 2 and 4 become ordinary follow-ups the moment PR #800 merges and
darwin_trust.js's sweep,TRUST_PROBE_TIMEOUT_MSanddisplayableCaHostsland onmaster. Finding 3 is workable today and is waiting only on you naming the envelope.What I need from you
warningskey, or stderr) and neutral will implement it againstmasterimmediately, with a short decision doc if you want one. It is the only item here that does not need a merge.Reply on this issue; neutral monitors this thread and will re-engage with your guidance on its next tick.
philcunliffe commentedon Aug 19, 2026
Triage update at PR #800 head 306210b (two further merges of green master; the second resolved conflicts with #818 / LLP 0262, which moved Claude Code off proxy capture). Two adjustments to this list:
Everything else on this list stands unchanged. Verified at that head: npm test 4495 pass / 0 fail, npm run typecheck clean, npm run smoke -- status_diagnostics ok.
philcunliffe commentedon Oct 6, 2026
Decision: use stderr for all existing purge diagnostic lines when --json is selected. Stdout remains the same per-client NDJSON, with no extra trailing purge object and no arbitrary attachment to the last client. This is the usual data/diagnostic split already used by the command and settles the old three-way envelope question without adding a field or a new output API. Plain-text mode stays unchanged. Best-effort cleanup still reports failures without changing the established detach exit status.
Fresh master still has the exact bug: runClients calls purgeProxyTrustResidue after the detach loop and writes its lines only under !parsed.json. Those lines include actual CA deletion, keychain residue and launchd-environment cleanup failures. Route the existing lines to the appropriate stream rather than silently suppressing them. Keep dry-run non-mutating and preserve any detach errors already written to stderr. Test the command boundary with injected/sandboxed cleanup: valid unchanged stdout NDJSON plus visible stderr diagnostics in JSON mode, successful cleanup lines as well as partial-failure warnings, all-client iteration, and no cleanup on dry-run. Do not exercise the real login keychain or launchd domain in unit tests.
Disposition of the other historical items: the two status probes now run concurrently with Promise.allSettled and independent unknown results, so the 10-second serial-wait finding is fixed. The old post-detach attach-to-repair advice has been replaced by bare trust states and a config-aware live/residue/unknown note. Current trust code installs no keychain trust; current OTEL attach writes none under LLP 0258, so do not add a new all-policy-versus-SSL trust grant or resurrect the deleted attach dialog. Keep the existing bounded eight-pass cleanup warning: it accurately says may remain at the limit; another pass only moves the edge. Keep the existing foreign-certificate comma rendering nit as a recorded limitation, not another work item. Do not open a speculative CA lifecycle redesign from the historical fingerprint/remint suggestions: the actual certificate fingerprint and trust probe remain visible, and explicit gateway proxy behavior stays supported.
The old live-Mac checks are not reported as passed. Follow applicable repository acceptance requirements when a release changes a live keychain/probe boundary; this focused output repair requires no new trust grant, probe implementation or CA lifecycle behavior. CPU/memory: no new subprocesses, probes or scans, and only the already-produced bounded diagnostic lines are routed. Close this issue when the output repair and focused checks land; do not revive the rest as follow-up issues.
[-]Follow-up: deferred review findings from PR #800[/-][+]Preserve proxy-cleanup diagnostics on stderr when client detach uses JSON output[/+]HypForge commentedon Oct 18, 2026
Landed in #2722 (3e77cb4). Under
--json,hyp client detach --purgenow writes its purge lines to stderr, and stdout stays the unchanged per-client NDJSON. The 'Skipped --purge' line is no longer silenced under--json. Closing, as the 2026-10-06 disposition said; none of the other items are revived.