Closes #174#186
Conversation
#145 deliberately preserves ANTHROPIC_BASE_URL (an intentional custom endpoint) and keeps the user's credentials in that case — but, unlike proxies (surfaced via a content-free status, session.ts:322), a base-URL reroute was invisible: every caption batch, summary, and coaching prompt silently transits an arbitrary host with credentials attached, while the UI still reads "Audio never leaves this Mac". Mirror the #145 proxy notice. Per the Batch 36 amendment (the redactor `proxyHost` was module-private), extract a shared host-redactor: rename `proxyHost` → `safeHost` (used by both detectors) and add an exported `detectCustomEndpoint` that returns `safeHost(ANTHROPIC_BASE_URL)` or null. HostSession.start emits a content-free status beside the proxy notice when it is set — "translation traffic is routing to a custom Anthropic endpoint: <host>" — carrying ONLY the safe host, never the URL path/query/credentials. #145's sanitizeChildEnv carve-out and detectProxy behavior are unchanged. Tests (env.test.ts, mirroring detectProxy — the host has no headless start() harness, so the redactor IS the tested unit, matching how #145's notice is covered): presence → host[:port]; redaction drops embedded credentials + path + query; scheme-less host:port; unparseable → "(set)"; unset/blank → null (no notice). Engine suite 269 green, app 158; typecheck (app+packages), lint, build clean. No new deps; no caption content logged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Epic Alignment: FAIL
The shared redactor and CLI-path notice follow #174, but the ticket explicitly requires a test proving the session status emission; the diff tests only the detector.
Checked (evidence)
- Structural gate: live PR body includes filled EPIC Alignment, Self-Verification, and N/A Design Fidelity sections.
- Contract: live issue #174 requires the session status to surface the endpoint host and tests for notice presence/redaction and absence when unset.
- Redaction: packages/engine/src/env.ts:78-89 reduces a URL to host[:port] or the safe fallback; env tests cover credential/path/query stripping.
- Riskiest part: src/host/session.ts:333-344 emits the user-visible status, but it is untested.
- Kill-list: scanned full diff — hit listed below.
- CI: gh pr checks 186 → color-guard and no-stub-gate pass; app-macos and packages-linux pending.
Findings
- [required] Add an assertion for the emitted HostSession status notice.
- File: src/host/session.ts:333
- Why it fails: packages/engine/test/env.test.ts:160-200 proves only detectCustomEndpoint’s return value. It does not prove HostSession.start calls it, emits the exact status, keeps credentials/path/query out of that status, or emits nothing when unset—the explicit #174 acceptance criterion.
- Do instead: add a HostSession-start test seam/harness (or test the status-construction path) that asserts the CLI path emits exactly the redacted custom-endpoint notice when set and no such status when unset. Keep the detector tests for URL redaction.
Decision
The implementation appears correct, but the visible privacy notice is unproven without a test of its actual emission.
RE2 — APPROVE at PR #186 head (pending
|
…notice Address RE1 review on PR #186: #174 requires proving the visible HostSession notice, not just the detector. `start()` spawns real CLI children (no headless harness), so factor the status string + its redaction into a pure, exported `customEndpointNotice(env)` — exactly what the CLI path emits (`if (endpointNotice) emit({ type: "status", detail: endpointNotice })`). New test/custom-endpoint-notice.test.ts asserts the CLI path's endpoint status: set → exactly "…custom Anthropic endpoint: <safe host>" (bare host / host:port); credentials, path, and query never reach the status; unset/empty/whitespace → null (no notice). The detector redaction tests (env.test.ts) are retained; the proxy notice is untouched. App suite 162 green; typecheck (app+packages), lint, build clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Epic Alignment: PASS
#174’s privacy requirement is met: an active custom endpoint is surfaced once on the CLI path using only a safely redacted host.
Checked (evidence)
- Re-review range: 561afdb..1fb966e.
- Prior finding resolved: src/host/session.ts:155-169 derives the exact notice value emitted at src/host/session.ts:344-345.
- Test coverage: test/custom-endpoint-notice.test.ts:15-45 covers set, unset/blank, exact status text, and credential/path/query redaction.
- Riskiest part: endpoint redaction; the notice uses detectCustomEndpoint’s safe host rather than the raw environment value.
- Kill-list: scanned new range — clean.
- CI: gh pr checks 186 → app-macos, color-guard, no-stub-gate, packages-linux all pass.
Findings
None.
Decision
The tested notice closes the silent custom-endpoint privacy gap without changing #145’s endpoint-preservation behavior.
RE2 — APPROVE at
|
Closes #174
Surface an active
ANTHROPIC_BASE_URLcustom endpoint to the user with a content-free status notice — the same privacy discipline #145 added for proxies — so a base-URL reroute is no longer invisible.EPIC Alignment
Part of EPIC #134 (v1.3 audit follow-up), Batch 43 (standing-order sequence … #175 → #174). Privacy sibling of #145 (env sanitizer, merged PR #154). Scoped to
packages/engine/src/env.ts(+ barrel),src/host/session.ts, and the env unit test. Does not touchresample.rs/render.ts/parse.ts/pipeline.rs(reserved for #178/#179), the #145 sanitize/carve-out behavior, or dashboard/settings.Self-Verification
The bug. #145 preserves
ANTHROPIC_BASE_URLand its credentials as an intentional custom endpoint, and surfaces proxies via a content-free status (session.ts:322—translation traffic is routing through a proxy: <host>) — but nothing surfaced a base-URL override. So a user whose shell profile setsANTHROPIC_BASE_URL(LLM routers/gateways, a corporate relay) has every caption batch, summary, and coaching prompt silently sent to that arbitrary host with credentials attached, while the Settings privacy row still reads "Audio never leaves this Mac". The env read is unconditional in release.The fix (mirror the #145 proxy notice).
proxyHostwas module-private, so it couldn't be reused. Renamed it tosafeHostand documented it as the shared redactor for both detectors (env.ts), and added an exporteddetectCustomEndpoint(base)that returnssafeHost(ANTHROPIC_BASE_URL)when set/non-blank, elsenull.detectProxyis unchanged (now just callssafeHost).customEndpointNotice(env)(session.ts) →translation traffic is routing to a custom Anthropic endpoint: <safe host>whenANTHROPIC_BASE_URLis set, elsenull.HostSession.start, beside thedetectProxyblock and on the CLI path only (the on-device local tier never egresses), just emits it:if (endpointNotice) emit({ type: "status", detail: endpointNotice }). Only the safehost[:port]is surfaced — never the URL path/query or embedded credentials.process.envis the right source: the CLI engine is constructed withenv: process.env(session.ts:278), exactly whatdetectProxyalready inspects.No #145 behavior change.
sanitizeChildEnv(the credential/billing-redirect carve-out) anddetectProxyare untouched — this only adds a read-only notice.Security invariants. No new dependencies. No caption/audio content logged — the notice is content-free (only the redacted host). Simplest fix that solves the ticket; the
proxyHost→safeHostrename directly serves the amendment's shared-redactor requirement (not a drive-by). Touched onlyenv.ts(+ barrel),session.ts, and the two tests.Tests / evidence.
HostSession.startspawns real CLI/llama-server children and has no headless e2e harness (seesession-accounting.test.ts: "the helper IS the fix"), so the notice's status string + redaction are factored into the purecustomEndpointNoticeseam and tested directly (the emit is a trivialif (x) emit(detail: x)).test/custom-endpoint-notice.test.ts— proves the VISIBLE notice (RE1's requirement): set → exactly"…custom Anthropic endpoint: gateway.example:8443"(bare host / host:port); credentials + path + query never reach the status (user:s3cret@gateway.example:8443/v1/messages?key=abc→ host only); unset / empty / whitespace →null(no endpoint status).detectCustomEndpoint (#174)block inenv.test.ts(7 cases, the detector-level redaction — retained per RE1):host[:port](https://gateway.example:8443→gateway.example:8443; bare host when no port);https://user:s3cret@gateway.example:8443/v1/messages?key=abc→gateway.example:8443, asserting the creds, path, and query are all dropped;host:port; unparseable →(set)(never echoes the raw value);null(no notice).detectProxy's 10 existing tests stay green after the rename.pnpm typecheck(app + packages),pnpm lint,pnpm buildall clean.Design Fidelity
N/A — no UI/webview/CSS surface changed. The notice rides the existing
statusevent channel (the same one the proxy notice uses); no new markup, layout, token, or component.Deviations
proxyHost→safeHost(rather than exportingproxyHost), so the name reflects its now-dual use andsession.tsreuses the redaction indirectly through the exporteddetectCustomEndpoint— it never needs the raw redactor. This is the amendment's "shared host-redactor helper" option.ANTHROPIC_BASE_URLis set (including an explicit official endpoint) — an explicitly-set base URL is a deliberate reroute by definition, matching the ticket's unconditional "when set" scope.🤖 Generated with Claude Code