test(doctor): make scrubbed probes cross-platform

[Hmbown]

Summary

• Serve the process-level doctor probe through an explicitly bound Axum fixture that drains the complete request body and retains only headers for the authorization assertion.
• Keep diagnostic subprocesses credential-isolated with env_clear while preserving only the non-secret SystemRoot and WINDIR values required by Windows networking.
• Preserve the read-only contract: no ambient credential or proxy state is restored, and the legacy credential is neither printed nor migrated.

Evidence

• cargo fmt --all -- --check
• cargo test -p codewhale-tui --test diagnostic_read_only --locked (10/10)
• cargo clippy -p codewhale-tui --test diagnostic_read_only --all-features --locked -- -D warnings
• cargo test --workspace --all-features --locked
• GitHub Windows exact-head full test and lockfile guard passed
• GitHub macOS exact-head full test, lockfile guard, and offline eval harness passed
• All PR checks green on b97333a

Signed-off-by: Hunter B hmbown@gmail.com

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @Hmbown's task in 1m 16s —— View job

---

Review: test(doctor): read request bodies across header orderings

• Gather context (read diff, changed file, CLAUDE.md)
• Understand the request (code review of PR test(doctor): make scrubbed probes cross-platform #4561)
• Review correctness, provider/model safety, reuse, tests, security
• Post final review

Summary: This is a real bug fix disguised as a test-only PR. In crates/tui/tests/diagnostic_read_only.rs, the old read_http_request helper used:

.find_map(|line| line.split_once(':'))
.filter(|(name, _)| name.eq_ignore_ascii_case("content-length"))

find_map returns the first line containing a colon — not the first line whose name matches content-length. Since the filter runs after find_map already committed to a line, any header preceding Content-Length (e.g. authorization:, content-type:) would short-circuit the lookup and silently default the body length to 0. That's the "close with unread request bytes" / Windows client-reset symptom described in the PR body — a genuine, previously-undiagnosed flake source in the doctor probe test harness.

The fix (crates/tui/tests/diagnostic_read_only.rs:515-522) is correct:

fn http_content_length(headers: &str) -> usize {
    headers
        .lines()
        .filter_map(|line| line.split_once(':'))
        .find(|(name, _)| name.eq_ignore_ascii_case("content-length"))
        .and_then(|(_, value)| value.trim().parse::<usize>().ok())
        .unwrap_or(0)
}

filter_map now scans all colon-bearing lines before find selects the one whose name matches, so header order no longer matters.

By review area:

1. Correctness: Bug fix is sound; filter_map/find composition correctly decouples "has a colon" from "is content-length". No panics, no unwrap on fallible paths (the and_then(...).unwrap_or(0) is a safe default, matching prior behavior for missing/malformed headers).
2. Provider/model/route safety (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608): Not applicable — this is HTTP header parsing in a local test-only probe server, unrelated to provider/model selection or wire-id prefixes.
3. Reuse/simplification: http_content_length is extracted once and used only by read_http_request — no duplication introduced. Good, minimal extraction that also makes the parser directly testable.
4. Tests: The new local_probe_server_finds_content_length_after_other_headers test directly reproduces the bug: authorization and content-type headers precede Content-Length: 37 in the fixture, and the first line (POST ... HTTP/1.1) contains no colon at all, so it exercises the exact scenario that broke the old find_map logic. This is a well-targeted regression test.
5. Security: No new input handling beyond what already existed; parsing stays scoped to a local test-only TCP server, not production networking code.

No issues found. This is a small, well-scoped, correctly-tested fix.

[claude]

Claude finished @Hmbown's task in 2m 15s —— View job

---

Review: test(doctor): use protocol-aware probe fixture (update)

• Gather context (new commit b7cf9a5, diff vs origin/main)
• Understand the request (re-review after synchronize push)
• Review correctness, provider/model safety, reuse, tests, security
• Post final review

Summary: The new commit supersedes the earlier fix entirely rather than building on it. Commit cd8e60c patched the hand-rolled HTTP parser's find_map/filter bug (the one reviewed earlier). Commit b7cf9a5 then deletes that whole hand-rolled TcpListener + read_http_request + http_content_length implementation — including the local_probe_server_finds_content_length_after_other_headers regression test — and replaces it with wiremock::MockServer in crates/tui/tests/diagnostic_read_only.rs:463-495. This is a better fix than the one it replaces: instead of patching a reimplemented HTTP/1.1 parser, it removes the reimplementation and delegates to a hyper-backed mock server that reads request bodies correctly regardless of header order, closing the root cause (the Windows connection-reset symptom from unread request bytes) rather than one manifestation of it.

By review area:

1. Correctness:
   • CompletionServer (line 463) mounts Mock::given(method("POST")).and(path("/v1/chat/completions")) against base_url = "{server.uri()}/v1". I traced the client's URL construction (crates/tui/src/client.rs:1666, api_url(&self.base_url, "chat/completions")) — /v1 + chat/completions → /v1/chat/completions, so the matcher lines up with what the doctor probe actually requests.
   • Field order in the CompletionServer struct (server before runtime, lines 464-465) matters: Rust drops struct fields in declaration order, so server (the MockServer) is torn down while runtime is still alive to drive its shutdown, then the runtime itself is dropped. That's the correct order for this pattern, but it's a non-obvious invariant — swapping the field order would silently reintroduce shutdown flakiness. Worth a one-line comment if this file gets touched again.
   • received_requests() (line 490) uses .expect("request recording enabled") — recording is on by default for wiremock::MockServer::start(), so this is a safe test-only assumption, not a latent panic risk.
   • No Cargo.toml/Cargo.lock changes were needed: wiremock = "0.6" is already a [dev-dependencies] entry in crates/tui/Cargo.toml:99, and the workspace tokio dependency already enables rt-multi-thread and net, which this new code relies on.
2. Provider/model/route safety (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608): Not applicable — still local test-only HTTP mocking, no provider/model inference logic touched.
3. Reuse/simplification: Net reduction in hand-maintained protocol-parsing code (-70/+47 lines) in favor of an existing, already-used test dependency. Good simplification — one less bespoke HTTP implementation to keep correct.
4. Tests: The header-ordering-specific unit test from the prior commit is gone, but it's testing a code path (http_content_length) that no longer exists. Coverage for the underlying behavior (doctor probe correctly completing and asserting on Authorization header) is preserved via doctor_text_probe_uses_a_legacy_key_without_migrating_it (lines 187-253), now checking requests[0].headers.get("authorization") instead of substring-matching a raw request dump — this is more precise than the string-search assertion it replaces.
5. Security: No new input handling; still scoped to a local, test-only mock server.

Note on verification: I was not able to run cargo test in this sandbox (Bash tool calls requiring network/build access were blocked pending approval), so this review is static analysis of the diff plus tracing the client URL-construction code, not a live test run. If you'd like me to actually execute the test suite, --allowedTools will need to permit cargo test invocations for this session.

No blocking issues found.

[claude]

Claude finished @Hmbown's task in 2m 18s —— View job

---

Review: test(doctor): read request bodies across header orderings (update)

• Gather context (new commit 2f275aa, diff vs origin/main)
• Understand the request (re-review after latest synchronize push)
• Review correctness, provider/model safety, reuse, tests, security
• Post final review

Summary: Commit 2f275aa moves the wiremock MockServer + Tokio runtime from commit b7cf9a5 onto a dedicated owner thread, replacing the ad-hoc runtime.block_on(...) calls from the main test thread with an explicit command channel (ReceivedRequests / Shutdown). This is a real hardening step, not just a refactor: the server's entire async lifecycle (start, mount, serve, snapshot requests, shut down) now runs inside one continuous block_on future on its own thread, so there's no window where the runtime's worker pool is relied on to keep serving requests without an active driving future between the main thread's blocking calls — which is a more defensible way to avoid the Windows connection-reset race described in the PR body.

By review area:

1. Correctness:

   • CompletionServer::start() (crates/tui/tests/diagnostic_read_only.rs:479-524) spawns the owner thread, builds the runtime there, starts MockServer, mounts the fixed POST /v1/chat/completions mock, then publishes the base URL over a sync_channel(1) before entering the command loop. The ready_receiver.recv_timeout(10s) in the constructor will surface a clear panic if the owner thread dies before publishing, rather than hanging — good.
   • received_requests() (line 526) round-trips through the command channel with its own 10s timeout, so a snapshot request can't hang indefinitely if the owner thread has gone away.
   • Drop for CompletionServer (line 536) sends Shutdown and joins the owner thread, guaranteeing the mock server and runtime are fully torn down before the struct is gone — this also removes the earlier version's implicit reliance on struct field-drop order (server before runtime), which I'd flagged as a non-obvious invariant in the prior review. That concern is now moot since runtime/server no longer exist as struct fields at all.
   • Minor, non-blocking: if the owner thread ever panicked after start() succeeded (e.g., inside the command loop), owner.join() in Drop::drop would return Err, and .expect("stop local probe server") would panic during another panic's unwind (e.g., a failed assertion in the test body), which aborts the process instead of yielding a normal test failure. This is a very low-probability path (the loop body only does channel recv/send and an .await on an already-mounted server), so I wouldn't block on it, but it's worth knowing if a future change adds more fallible work to the command loop.
   • No unwrap/expect calls here are on paths that fire under normal operation.

2. Provider/model/route safety (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608): Not applicable — still local, test-only HTTP mocking; no provider/model inference logic touched.

3. Reuse/simplification: The dedicated-thread-plus-channel pattern is more machinery than a plain block_on-per-call design, but it's justified here: it directly targets the flake source named in the PR body and collapses the two lifecycle-critical fields (server, runtime) into a single ownership boundary. No duplicated logic elsewhere in the file.

4. Tests: No new test cases in this commit — it's purely a harness reliability fix underneath the existing doctor_text_probe_uses_a_legacy_key_without_migrating_it test, which still asserts on requests[0].headers.get("authorization") unchanged.

5. Security: No new input handling; still scoped to a local, test-only mock server.

Verification note: I could not run cargo test -p codewhale-tui --test diagnostic_read_only in this sandbox — the command requires approval that isn't granted for this session, so this is a static review of the diff, not a live test run. CI (CI workflow, run 29671095761) is still in progress at time of writing and will be the real signal on whether this resolves the Windows flake.

No blocking issues found.

[claude]

Claude finished @Hmbown's task in 1m 24s —— View job

---

Review: test(doctor): read request bodies across header orderings (update)

• Gather context (new commit 755c3f1, diff vs origin/main)
• Understand the request (re-review after latest synchronize push)
• Review correctness, provider/model safety, reuse, tests, security
• Post final review

Summary: Commit 755c3f1 swaps the wiremock-based MockServer from b7cf9a5/2f275aa for a real axum::serve instance on the owned probe thread. This is a good final iteration: wiremock internally still has to reassemble the request, whereas hyper/axum's own Bytes extractor is the same body-reading machinery the crate already trusts in production, so draining the body through it (crates/tui/tests/diagnostic_read_only.rs:509-516) closes the root cause directly rather than through another layer of test-only HTTP handling.

By review area:

1. Correctness:

   • Route/base URL match: base_url is http://{address}/v1 (line 528) and the client builds chat/completions requests via api_url_with_suffix (crates/tui/src/client.rs:744-750), which concatenates to .../v1/chat/completions — exactly the mounted route (line 505). Verified this pairing is correct.
   • drop(body) after extracting Bytes (line 514) is deliberate and commented — Bytes extraction forces axum to fully read the body before the handler runs, which is the actual fix for the Windows reset (the body must be drained before the connection can be reused/closed cleanly). Good, minimal way to force full-body consumption without needing the payload.
   • Drop for CompletionServer (line 556) checks thread::panicking() before calling .expect() on the owner thread's join() result — this correctly avoids a double-panic/abort if the test body already failed an assertion, same defensive pattern flagged as missing in the previous review of 2f275aa; good that it's addressed now.
   • ready_receiver.recv_timeout(10s) (line 550) and the Mutex lock .expect() calls are test-only and fail loudly/promptly rather than hanging — consistent with the rest of the file's style.
   • No leftover references to the deleted one_request_completion_server/read_http_request/http_content_length helpers or the earlier wiremock-based CompletionServer — grep confirms CompletionServer has a single call site (line 203) and a single definition.

2. Provider/model/route safety (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608): Not applicable — still a local, test-only mock HTTP server; no provider/model inference logic touched.

3. Reuse/simplification: axum and serde_json (with the Json response helper) are already ordinary (non-dev) workspace dependencies used elsewhere in this crate, so this doesn't introduce a new dependency, and wiremock remains in dev-dependencies for its many other call sites in the crate — nothing orphaned. The owned-thread + channel structure carries over unchanged from 2f275aa, so no new complexity was added beyond swapping the server implementation.

4. Tests: No new test cases in this commit; doctor_text_probe_uses_a_legacy_key_without_migrating_it still asserts on requests[0].get("authorization"), unchanged in behavior from the prior commit.

5. Security: No new input handling; still scoped to a local, test-only HTTP server bound to 127.0.0.1:0.

Verification note: I could not run cargo check/cargo test in this sandbox (the commands require approval that isn't granted for this session), so this is a static review of the diff, not a live build/test run. CI is currently in_progress (CI run 29671598177, Claude PR Review run 29671598175) — that will be the authoritative signal on whether this compiles and resolves the flake. If you'd like a live verification, --allowedTools will need to permit cargo check/cargo test for this session.

No blocking issues found.

[claude]

Claude finished @Hmbown's task in 56s —— View job

---

Review: test(doctor): read request bodies across header orderings (update)

• Gather context (new commit b97333a, diff vs origin/main)
• Understand the request (re-review after latest synchronize push)
• Review correctness, provider/model safety, reuse, tests, security
• Post final review

Summary: Commit b97333a is a small, targeted follow-up to the axum-based probe from 755c3f1. It adds a new preserve_host_platform_runtime helper (crates/tui/tests/diagnostic_read_only.rs:594-601) that re-injects SystemRoot/WINDIR into the credential-isolated subprocess's environment on Windows, called alongside the existing preserve_host_rustup_home at all three env_clear() call sites. This addresses a plausible remaining Windows flake source: without these OS root variables, Windows' network stack (used internally by reqwest/WinSock to resolve loopback) can fail to initialize in a fully-scrubbed child environment, which would prevent the probe subprocess from ever reaching the local fixture server added in the earlier commits — independent of whether the fixture itself reads the body correctly.

By review area:

1. Correctness:

   • The helper is gated with #[cfg(windows)] around the loop body only, with the parameter named _command to avoid an unused-variable warning on non-Windows targets where the function becomes a no-op — verified this compiles cleanly conceptually (mirrors the existing pattern in the file), though I could not run cargo check in this sandbox (see note below).
   • All three sites that call .env_clear() (lines 154, 415, 455) now call both preserve_host_rustup_home and preserve_host_platform_runtime immediately after — consistent coverage, no env_clear() site was missed (verified via grep).
   • SystemRoot/WINDIR are non-secret, well-known Windows environment variables — restoring them doesn't reintroduce ambient credentials or proxy state, matching the intent stated in the commit message and preserving the credential-isolation boundary the tests rely on.
   • Only additive: on non-Windows platforms (Linux/macOS CI runners) this is a no-op, so no behavior change there.

2. Provider/model/route safety (EPIC v0.8.65 EPIC: Separate provider facts, model facts, offerings, and route resolution #2608): Not applicable — environment-variable scrubbing for a local test subprocess, unrelated to provider/model inference.

3. Reuse/simplification: Follows the exact same shape as the pre-existing preserve_host_rustup_home (same call-site pattern, same doc-comment style explaining the why). No duplication; a single new helper reused at all three sites rather than being inlined three times.

4. Tests: No new test cases — this is an environment-preparation fix for existing tests (doctor_json_does_not_inherit_an_ambient_legacy_secret_from_an_explicit_home and others using run_sealed_diagnostic/diagnostic_command) that presumably run on Windows CI. Reasonable, since the fix is about making existing tests reliably runnable on Windows rather than adding new coverage.

5. Security: No new input handling; SystemRoot/WINDIR are non-secret by nature, so this doesn't weaken the credential-isolation guarantee the surrounding tests are designed to enforce.

Verification note: I could not run cargo check/cargo test in this sandbox (the commands require approval not granted for this session), so this remains a static review of the diff. CI is the authoritative signal for both compilation and whether this resolves the Windows flake — worth confirming the Windows CI leg is green before merge, since that's the only environment where this code path is exercised.

No blocking issues found across the full PR history (cd8e60c → b97333a): the series correctly diagnosed a genuine bug (header-order-sensitive Content-Length lookup), iteratively hardened the fix (hand-rolled parser → wiremock → owned-thread axum server → env preservation), and each step is well-reasoned and narrowly scoped.