[siebe41]

What changed

• New test EgressRecordingTests (tests/SemanticRouter.Tests/Host/). It starts the tool (semantic-router.dll) in a child process. The child's only proxy is a recording proxy (HTTPS_PROXY, HTTP_PROXY, ALL_PROXY, upper- and lower-case; the machine's own proxy and router variables are removed). Embeddings are Hashing, so there is no model download and no network. The configuration puts a chat model, a judge model (SampleRate = 1), a third HTTPS connector and a webhook on made-up *.egress-test.example hosts. The test then runs start-up, doctor (a second child process), a chat call the judge grades, a classify call, the dashboard page with its assets, and the /router/api/... reads the dashboard's pages make when they open. It asserts that the proxy saw the chat, judge and backup hosts, and that every non-loopback host it saw is named by an egress line printed by the child tool's doctor for that configuration. With --SemanticRouter:Network:Offline=true, the proxy must see no non-loopback host at all, and the chat call must fail.
• RecordingProxy moved to tests/SemanticRouter.Tests/Support/. OnnxTelemetryTests uses the shared copy without behaviour changes: with no reply function it still answers nothing. With a reply function, it reads the request headers and body and answers a plain HTTP request with a stub JSON reply. It answers a CONNECT with 502. It never forwards anything.
• Fix: doctor and the dashboard's connector check no longer check a connector that offline mode or the machine policy turns off (RouterDoctor.CheckConnectorAsync). The new test found this. In offline mode, doctor printed egress: connector X: host (never contacted: … offline mode …) and then sent that connector's credentials to that host anyway. It now prints [ OK ] connector X: not checked: <reason>. A unit test covers it: HostTests.Doctor_does_not_check_a_connector_that_offline_mode_turns_off, with a stub handler that must receive no request.
• docs/network.md: one sentence saying doctor skips blocked connectors, and a paragraph on what the new test checks and what the weekly egress-hosts.yml run still covers (the ONNX download and update redirects).
• CHANGELOG, under Security: one line for the doctor fix.

Why

#185's done-when: "a recording-proxy check in CI keeps the egress list honest". Before this PR, egress-hosts.yml only followed the download redirects. OnnxTelemetryTests ran a recording proxy for ONNX start-up only (Linux only) and never compared what it saw with EgressList. The new test compares what the tool really contacts with the list doctor prints for the same configuration. A new outbound call that is missing from the list now fails CI.

How it was verified

On this PC (Windows 11, shared with other sprint lanes), all on the final commit:

• dotnet build -c Release tests/SemanticRouter.Tests (builds every project the change touches): succeeded, no warnings.
• dotnet test -c Release --filter "EgressRecordingTests|HostTests.Doctor|OnnxTelemetryTests": 12/12 passed. The two egress tests alone take 35 s.
• Full suite dotnet test -c Release: 1465/1465 passed (run with OPENAI_API_KEY unset, see H09).
• dotnet format --verify-no-changes: clean.
• dotnet pack -c Release src/SemanticRouter.AspNetCore (package validation against 0.5.0): succeeded. The doctor fix is internal.

One flake, already fixed. An earlier full run on a heavily loaded machine failed Offline_the_router_contacts_nothing after 11m54s. The two Desktop tests that are known to time out under load failed in the same run. The egress test's limits were 90 s for start-up and doctor and 30 s per HTTP call, the same as OnnxTelemetryTests, and too tight when the suite runs it next to other child-process tests on a loaded machine. One 8-minute cancellation limit now covers the whole drive, including start-up, doctor, HTTP calls and the judge wait. Each wait ends as soon as its condition is met. The offline test watches background activity for a fixed 30 s. After the change, the full suite passed (above).

Other operating systems

The test is not OS-gated, so it runs on Linux, Windows and macOS in CI.

• Proxy selection: .NET's HttpClient.DefaultProxy reads HTTP_PROXY/HTTPS_PROXY/ALL_PROXY first on all three systems. Only when those are unset does it fall back to WinINet (Windows) or the system configuration (macOS). The test sets all of them, in both cases, and removes every inherited *proxy* variable, so a runner's own proxy cannot hide a request.
• Loopback: requests to this machine (the dashboard's LM Studio/Ollama probes) are ignored whether or not they go through the proxy, because offline mode allows them.
• Process: the child is started with DOTNET_HOST_PATH (set by dotnet test), falling back to dotnet, and killed with entireProcessTree. On Windows, the temp directory is deleted with retries because the killed process can hold its SQLite files for a moment.
• Ports: the router listens on a free loopback port and the proxy on port 0, so parallel test runs cannot collide.

Not done

• The webhook's host is checked against the list but not observed through the proxy: the webhook's notice check runs every 5 minutes, which is too slow for this test. WebhookTests cover delivery.
• CLI connectors and the Copilot runtime are not in the configuration. Their traffic belongs to the vendor CLI, not to the router (docs/network.md says so).
• The ONNX model download and the desktop update hosts are not driven here, because a test may not use the network. The weekly egress-hosts.yml run still follows them against the live hosts.

Refs #185

Latest follow-up: the proxy stops accepting and drains every accepted connection before assertions, including silent connections. Every dashboard API read must return a successful status. Merged the latest main while retaining the offline doctor fix.

[siebe41]

Needs changes, small: the doctor fix is right and uses the same MachinePolicy.Blocks the router and EgressList use, but the test's judge step proves nothing, and its host matcher would accept a host the list says is never contacted.

What holds up:

• The proxy really carries the router's traffic. Nothing in src/ sets UseProxy = false or its own Proxy. Every router client comes from IHttpClientFactory with the default handler, and .NET reads HTTP(S)_PROXY/ALL_PROXY first on all three OSes. The test also proves this: the *.egress-test.example hosts can't be resolved, so the chat call succeeds, and backup shows up, only if the requests went through the proxy.
• No network is used, and no environment variables leak. The proxy env vars go only on the child's ProcessStartInfo.Environment; nothing process-wide is set. The test's own client uses UseProxy = false. Embeddings are the template's Hashing, so there is no download.
• The doctor fix: RouterDoctor.cs:96 calls RouterRuntimeProvider.Policy.Blocks(connector), the same check ChatConnectorRegistry and EgressList use, so the check and the router can't drift apart. Before the fix, offline mode turned the models off, models.Length == 0, and the "still worth a probe" branch sent the key anyway. Loopback and OfflineHosts connectors are still checked in offline mode. The new HostTests case would fail without the fix.

Findings

1. Medium: the judge wait is already met before the chat call, so the test never shows that the judge's grading call happens or goes through the proxy. tests/SemanticRouter.Tests/Host/EgressRecordingTests.cs:56 waits for Seen(proxy, Judge). But doctor runs first (:146-149) and checks the grader connector, which sends GET http://judge.egress-test.example/v1/models (ConnectorProbe.cs:80). So Destinations holds the judge host before the chat request is even sent, the 2-minute wait at :173 ends on its first check, and the test can finish before the background judge sends anything. The same goes for the Contain([Llm, Judge, Backup]) assertion at :60: doctor alone meets it for all three hosts. Fix: wait for d.Host == Judge && d.AbsolutePath.EndsWith("/chat/completions") (and assert it), or clear or mark the recording after doctor. Until then, "a chat call the judge grades" in docs/network.md:105 and the PR body says more than the test checks.

2. Low: Names accepts a host that its egress line says is never contacted. At EgressRecordingTests.cs:204-209, any egress line that contains the authority counts, including connector x: host (never contacted: …) and price list: host (never contacted: …). The bug this PR fixes is exactly a contacted host that its own line calls never contacted. The online test would not catch it under a machine policy (AllowedEndpoints, DisabledConnectorTypes); only the offline test's stricter "no non-loopback host" check does. Skip lines that contain never contacted when matching.

3. Low: connections without a parseable request line are not counted. At Support/RecordingProxy.cs:90-93, a destination is recorded only when DestinationOf(line) parses. A connection that sends nothing within 5 s, or sends TLS straight away, lands in Requests as (connected without a request line) but not in Destinations, and both tests only assert on Destinations. The offline test should also fail on any Requests entry without a destination. Related limit that is worth one sentence in docs/network.md: traffic that skips the proxy (a future handler with UseProxy = false, or native code) fails DNS on .example hosts and goes unseen. The connector clients are covered by the chat call and the backup host; webhook, price-list and embeddings clients are not.

4. Low: there is a gap between picking the port and using it. FreePort() (:211-216) closes its listener, and the child binds that port seconds later, after a slow Release start-up. A parallel test that binds port 0 in between makes the child fail to start. It's rare, but this suite already has known flaky socket tests under load. Passing --urls http://127.0.0.1:0 and reading the bound address from the child's output or from status would remove the race.

5. nit: a hung child makes a slow failure. The waits are 3 min for start-up, 3 min for doctor, 2 min for the judge, and a 2-minute HttpClient.Timeout on each of the roughly 25 dashboard and asset requests (:126, :157-171). A child that hangs can keep one test running for most of an hour before it fails. An overall CancellationTokenSource (say 6 min) shared by all the calls would cap it.

6. nit: the egress list and the child can read different settings. EgressAsync (:113-117) builds the router in the test process, where WebApplication reads every environment variable, including any SemanticRouter__* on the runner. The child has those removed (:254-258). A developer's SemanticRouter__… variable could then make the list and the observed traffic disagree. Clear them for the in-process build too, or compare against the egress lines the child's doctor prints.

CHANGELOG: the line goes under Unreleased → Security, which is right because doctor was sending keys to hosts the policy blocks. The dashboard claim also holds: the connector-check endpoint calls the same RouterDoctor.CheckConnectorAsync.

[siebe41]

Review addressed. Merged origin/main in 30c86b0; the fixes are in b416928.

1. Judge wait is met by doctor: fixed in b416928. The test now waits for a chat request to the judge's host (/chat/completions), not just any connection, and asserts it. It also asserts the worker's chat call. backup is still asserted from doctor's check alone, which is what it is for.
2. Names accepts a "never contacted" line: fixed in b416928. A line containing never contacted no longer counts as naming the host.
3. Connections without a request line are not counted: fixed in b416928. RecordingProxy.WithoutDestination collects every connection whose first line names no destination (or that sent nothing), and both tests require it to be empty. docs/network.md now says what the test does not see: the webhook, price-list and download clients are not exercised, and traffic that skips the proxy fails DNS on the made-up hosts without the test noticing.
4. Port race: fixed in b416928. The child gets --urls http://127.0.0.1:0, and the test reads the bound address from the banner's is listening on … line.
5. Hung child makes a slow failure: fixed in b416928. One 8-minute limit covers the whole drive (start-up, doctor, every request and the judge wait). When it runs out, the test throws a TimeoutException with what the proxy saw and what the tool wrote. 8 rather than 6 minutes because the old per-step limits allowed 3 minutes for start-up alone on a loaded runner.
6. The egress list and the child can read different settings: fixed in b416928. The in-process build is gone. The test compares against the egress lines the child's own doctor printed, and that child has SemanticRouter__* removed like the router.

Verified: dotnet build -c Release with 0 errors; EgressRecordingTests + OnnxTelemetryTests 5/5, and EgressRecordingTests passed 3 more times in a row (~34 s each); dotnet format --verify-no-changes clean. Full suite: 1464/1465 passed. The one failure is SecretsTests.Literal_secrets_move_to_the_store_and_resolve_back, which fails on any machine with OPENAI_API_KEY set (queued as H09) and is unrelated. CI is green on Linux, Windows and macOS.

[siebe41]

Looks right. All six findings from the 2026-10-21 review are fixed at b416928, the doctor fix uses the same Policy.Blocks as the connector registry, and CI is green on Linux, Windows and macOS. One thing to do before squash-merging: update the PR body (see Optional).

Re-review at b416928, fresh eyes. I read EgressRecordingTests.cs, Support/RecordingProxy.cs, the RouterDoctor hunk, the new HostTests case, and the OnnxTelemetryTests, docs/network.md and CHANGELOG changes. I pushed nothing.

Previous findings (review of 4127efc)

1. Doctor met the judge wait: fixed. Chatted(proxy, Judge) (EgressRecordingTests.cs:223-224) requires a /chat/completions request to the judge host, and the test waits for it and asserts it (:61, :66). Doctor's GET …/models no longer meets either. The worker's chat call is asserted too (:65).
2. Names accepted a "never contacted" line: fixed (:234).
3. Silent connections weren't counted: fixed. RecordingProxy.WithoutDestination (RecordingProxy.cs:114-117) collects every connection without a parseable destination, and both tests require it to be empty. docs/network.md now says which clients the test does not exercise, and that a client which skips the proxy goes unseen.
4. Port race: fixed. The child gets --urls http://127.0.0.1:0, and the test reads the bound address from the banner (:115, :248-249).
5. A hung child failed slowly: fixed. One 8-minute CancellationTokenSource covers every step (:127), the HttpClient timeout included. A timeout throws a TimeoutException that shows what the proxy saw and what the tool printed.
6. The in-process list could read different settings: fixed. The test now parses the egress lines that the child doctor printed (:198). That child has SemanticRouter__*/SEMANTIC_ROUTER_* and all *proxy* variables removed (:277-281).

Checked and fine

• The doctor fix. RouterDoctor.cs:96 asks RouterRuntimeProvider.Policy.Blocks(connector), the same MachinePolicy.Effective(policy, Network:Offline) that ChatConnectorRegistry uses. It returns before the models.Length == 0 "still worth a probe" branch that sent the key. The dashboard's POST connectors/{name}/check goes through the same method. HostTests.Doctor_does_not_check_a_connector_that_offline_mode_turns_off requires the stub handler to receive no request, so it fails without the fix.
• The proxy carries the traffic on all three OSes. The .example hosts can't resolve, so the chat succeeding and backup showing up prove the requests went through the proxy. On Windows, ProcessStartInfo.Environment is case-insensitive, so the lower-case writes overwrite the same keys, which is harmless.
• The dashboard routes the test calls all exist (MapGet for each of the 18, MapPost("/classify")). classify is asserted 200, which shows the loopback API is reachable without a session.
• OnnxTelemetryTests behaves as before with the shared proxy (no reply means nothing is answered). The CHANGELOG line is under Unreleased › Security, which fits: doctor sent credentials to blocked hosts.

Optional

• The PR body describes the previous commit. Since the body usually becomes the squash commit message, update it before merging:
  • "What changed" still says the hosts are compared with "EgressList.For(...) for that configuration (the router is built the way the tool builds it)". That in-process build is gone: the test compares against the child doctor's printed lines.
  • "One flake, already fixed" still gives the per-step limits (3 min / 2 min / 2 min). It's now one 8-minute limit for the whole drive.
  • The class summary (EgressRecordingTests.cs:17) still points at EgressList.For. That's accurate as the source of doctor's lines, but "the lines the child's doctor printed" would be clearer.
• RecordingProxy.cs:96: a connection that stays silent is only added to WithoutDestination after its 5-second timeout. The online test asserts right after the judge's chat arrives (:68), so a silent connection opened in the last 5 seconds isn't counted. The offline test's 30-second settle covers it there. A short settle before the online assertions, or asserting only after proxy.Dispose() has drained, would close it.
• EgressRecordingTests.cs:174-181: the 18 dashboard GETs don't assert their status. If one starts answering 401 or 404, it stops exercising anything and the test still passes. A Should().NotBe(Unauthorized), or checking IsSuccessStatusCode where 2xx is expected, would keep that coverage honest.

(Cloud reviewer, review-lane checklist. Reviewed at b416928.)

---

Generated by Claude Code