Follow-up: deferred review findings from PR #800

[philcunliffe]

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 trustRoot with no -p trusts every policy while the name constraints only bound TLS server identity. Adding -p ssl touches 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 status renders launchd env: NODE_USE_SYSTEM_CA not set - run 'hyp attach claude' to set it indefinitely for a state the user chose. Gating on "a proxy attach is live" needs mode plumbed out of the attach probe into ClientAttachReport, 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:

1. security delete-certificate -c multi-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, run hyp daemon uninstall and assert Keychain Access holds no HypAware Local CA.
2. The 5s probe timeout in situ. The unit test proves runServiceCommand kills 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, run hyp status, confirm the line reads unknown - the keychain probe could not run and no dialog is left on screen.
3. The rendered attach-dialog line. ensureDarwinProxyTrust is 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.
4. F1, if attempted: the -p ssl variant 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).

[philcunliffe]

Picked this up as a neutral:fix follow-up. I worked from a detached worktree of origin/master (a14246d9, green) with a fresh npm install, on Linux. Three of the four deferred findings need code that is only on PR #800's branch; the fourth is on master but 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's darwin_trust.js is 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 from master.

2. The two trust probes run serially, so hyp status can block 2 x 5s. Needs TRUST_PROBE_TIMEOUT_MS, which does not exist on master; it is introduced at src/core/daemon/status.js:1284 on origin/fix/issue-793. master does await isCaTrustedFn then isLaunchdEnvSetFn serially (src/core/daemon/status.js:1315 and :1323), but with no bound on either, so the finding as written (worst case 10s instead of 5s, both rendering unknown) describes the PR's code and not master's. Parallelising master's version would be a different change carrying a different argument. Unreachable as stated.

3. hyp detach --purge --json drops the purge lines. Reachable, and I confirmed it on master: src/core/commands/clients.js:150-153 gates for (const line of purged.lines) on if (!parsed.json), and purgeProxyTrustResidue (:1602) really does return user-facing warning lines on master today, including the CA-not-removed and NODE_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: detachClientViaCore writes one JSON object per client to stdout as NDJSON and the purge runs after that loop, so there is no existing object to hang a purge or warnings key on. The three candidate shapes (a trailing purge object, a warnings key 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. Needs displayableCaHosts, which does not exist anywhere on master; it is added at src/core/tls/ca.js:413 on origin/fix/issue-793. Unreachable from master. 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_MS and displayableCaHosts land on master. Finding 3 is workable today and is waiting only on you naming the envelope.

What I need from you

1. Decide finding 3's JSON shape (trailing purge object, warnings key, or stderr) and neutral will implement it against master immediately, with a short decision doc if you want one. It is the only item here that does not need a merge.
2. Merge PR hyp status names the CA's permitted hosts, uninstall clears every duplicate root (#793) #800, after which 1, 2 and 4 can be worked, noting that all three are nits the reviews explicitly recommended leaving unless the surrounding code is touched for another reason.
3. Assign the macOS acceptance pass to whoever has a Mac; it cannot be simulated and the issue is explicit that it is owed.
4. Or close this issue if the residuals are wanted only as a record.

Reply on this issue; neutral monitors this thread and will re-engage with your guidance on its next tick.

[philcunliffe]

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:

• The macOS acceptance item "eyeball the rendered attach-dialog line" is moot: OTEL telemetry attach replaces proxy capture for Claude Code (LLP 0262) #818 deleted ensureDarwinProxyTrust and the dialog it printed. The remaining sanitized surface is hyp status, which still routes through displayableCaHosts (src/core/daemon/status.js:1615).
• New nit from the merge resolution: the doc comment at src/core/daemon/status.js:1574 still says displayableCaHosts is "shared with the attach dialog that names the same grant"; that dialog no longer exists. Comment-only staleness, fix on next touch.

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.