Illia Polosukhin 9cf37364af fix(staging): repair broken test build and macOS-incompatible SSRF tests (#2064) ...

* fix(staging): repair broken test build and macOS-incompatible SSRF tests

Staging tip (f9ed8152) was failing `cargo test` for several unrelated
reasons. This commit gets the test suite back to green on both Linux CI
and macOS.

1. **wrapper.rs:5595** — `PairingStore::new()` was called with zero args
   in `test_http_request_rejects_private_ip_targets`. The signature
   changed to `(db, cache)` in #1898 and the other test in the same file
   (line 5573) was updated to use `PairingStore::new_noop()`, but this
   one was missed. Switch to `new_noop()` to match.

2. **CLI snapshot** — clap's `render_long_help` for `--auto-approve`
   now emits an indented blank line between the short and long
   description (10 spaces, not empty). Update the snapshot to match the
   new output and refresh `assertion_line` (438 -> 461).

3. **validate_base_url IPv6 bracket bug** — `Url::host_str()` returns
   IPv6 literals WITH the surrounding brackets (e.g. `[::1]`), but
   `IpAddr::parse` does not accept brackets — it wants bare `::1`. As a
   result the IPv6 SSRF defense was effectively dead code: every `[…]`
   host failed to parse and fell through to the DNS-resolution path.
   This passed on Linux CI by accident (because `to_socket_addrs` on
   Linux also fails on bracketed strings), but broke on any host whose
   resolver returns a public IP for unresolvable lookups (ISP captive
   portals, ad-injecting DNS providers). Strip the brackets before
   parsing so the IPv6 detection actually works as intended.

4. **DNS-hijack-tolerant test guards** — two tests
   (`validate_base_url_rejects_dns_failure`,
   `test_validate_public_https_url_fails_closed_on_dns_error`) rely on
   RFC 6761's promise that `.invalid` lookups fail. On networks with
   DNS hijacking that promise doesn't hold and the lookups succeed
   (typically resolving to a public ad-server IP). Probe with
   `ironclaw-dns-hijack-probe.invalid` and skip the test with an
   eprintln on hijacked-DNS networks. Coverage on CI is unchanged.

5. **ExtensionManager test isolation** — the
   `extension_manager_with_process_manager_constructs` integration test
   passes `store: None`, which makes `list()` fall back to file-based
   `load_mcp_servers()` reading `~/.ironclaw/mcp-servers.json`. Any
   locally installed MCP server (e.g. notion) leaked into the test and
   broke the empty assertion. Set `IRONCLAW_BASE_DIR` to a fresh
   tempdir at the top of the test (before the LazyLock is initialized)
   to fully isolate.

After this commit, `./scripts/dev-setup.sh` runs end-to-end and
`cargo test` passes 4709/0 locally on macOS as well as CI.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(staging): address PR review feedback

- helpers.rs: simplify IPv6 bracket stripping to `host.trim_matches(...)`
  per gemini-code-assist suggestion. Functionally equivalent to the
  chained strip_prefix/strip_suffix/unwrap_or but cleaner; for any host
  string from `Url::host_str()` (which only ever returns matched
  brackets), the result is identical.

- setup/channels.rs: replace blocking `std::net::ToSocketAddrs` probe
  with `tokio::time::timeout(2s, tokio::net::lookup_host(...))` per
  Copilot review. The previous synchronous lookup could block a tokio
  worker thread inside `#[tokio::test]` and stall the suite on slow or
  offline DNS. The async resolver with a hard 2-second cap eliminates
  both risks.

- module_init_integration.rs: drop the brittle `is_empty()` assertion
  and the env-var override entirely, addressing both Copilot's review
  comment about parallel test ordering and the reviewer's deeper
  concern about touching process env from an integration test that
  cannot access the crate-private ENV_MUTEX. The test's actual purpose
  is to verify that ExtensionManager constructs and `list()` returns
  Ok — that's exactly what `is_ok()` checks. The empty assertion was
  always brittle (the test creates empty TOOL/CHANNEL dirs but does not
  isolate ~/.ironclaw, so any locally installed MCP server leaks in)
  and trying to "fix" it by mutating IRONCLAW_BASE_DIR from inside an
  integration test introduces order-dependent behaviour that the user
  flagged: parallel tests in the same binary can race the LazyLock,
  and there is no integration-test-visible mutex to serialise env
  mutations. Removing the assertion is the principled fix.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

2026-04-06 08:34:27 -07:00

8.8 KiB

Raw Blame History

View Raw