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

[ilblackdragon]

Summary

Staging tip was failing cargo test for several unrelated reasons. This PR gets the test suite back to green on both Linux CI and macOS without bypassing any safety logic.

• wrapper.rs — PairingStore::new() was called with zero args in test_http_request_rejects_private_ip_targets. The signature changed to (db, cache) in feat(ownership): centralized ownership model with typed identities, DB-backed pairing, and OwnershipCache #1898 and the sibling test on the same file was updated to use PairingStore::new_noop(), but this one was missed. Switch to new_noop().
• CLI snapshot — clap's render_long_help for --auto-approve now emits an indented blank line (10 spaces) between the short and long description. Refresh the snapshot.
• 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. 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. Strip the brackets before parsing so the IPv6 detection actually works as intended.
• DNS-hijack-tolerant test guards — two tests rely on RFC 6761's promise that .invalid lookups fail. On networks with ISP-level DNS hijacking that promise doesn't hold. Probe with ironclaw-dns-hijack-probe.invalid and skip with an eprintln on hijacked-DNS networks. Coverage on CI is unchanged.
• extension_manager_with_process_manager_constructs test isolation — test passes store: None, which makes ExtensionManager.list() fall back to file-based load_mcp_servers() reading the developer's real ~/.ironclaw/mcp-servers.json. Set IRONCLAW_BASE_DIR to a fresh tempdir at the top of the test (before the LazyLock is initialized) to fully isolate.

Test plan

• cargo build — green
• cargo test --lib — 4241 passed, 0 failed
• cargo test (full suite, parallel) — 4709 passed, 0 failed
• cargo test --lib -- --test-threads=1 (serial) — 4238 passed, 0 failed
• ./scripts/dev-setup.sh — completes end-to-end including the git-hook install step
• Manually verified each of the 5 fixes in isolation:
  • Pre-fix: cargo test test_http_request_rejects_private_ip_targets → error[E0061]: this function takes 2 arguments but 0 arguments were supplied
  • Post-fix: passes
  • Pre-fix: cargo test test_long_help_output_without_import → snapshot mismatch
  • Post-fix: passes
  • Pre-fix on macOS: validate_base_url("https://[::1]", "TEST") returned Ok(()) (silently bypassing SSRF defense)
  • Post-fix: returns Err(ConfigError::InvalidValue { message: "URL points to a private/internal IP '::1'..." })
  • Pre-fix on hijacked-DNS network: validate_base_url_rejects_dns_failure panicked
  • Post-fix on same network: prints skip message and passes
  • Pre-fix with notion MCP server in ~/.ironclaw/mcp-servers.json: extension_manager_with_process_manager_constructs failed is_empty() assertion
  • Post-fix: passes

Notes for reviewer

• Fix Move whatsapp channel source to channels-src/ for consistency #1 and feat: adding Web UI #2 also exist on stranded branches origin/fix/wasm-wrapper-pairing-store-noop and origin/fix/ci-pairing-store-noop (no open PRs). This PR supersedes both with a broader test-suite repair.
• Fix Onboarding: show Telegram in channel selection and auto-install bundled channel #3 (the IPv6 bracket bug) is a real security fix, not just a test fix. Without it, validate_base_url is currently relying on to_socket_addrs failing for bracketed IPv6 strings (which is OS-dependent behavior) rather than its own IP-classification logic. This PR makes the defense work as designed regardless of resolver behavior.
• Fixes feat: Sandbox jobs #4 and feat: Improve CLI #5 are test-quality improvements to make the suite reliable on developer machines that aren't identical to CI.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Repairs several failing tests and hardens SSRF URL validation so the suite is green on both Linux CI and macOS, while keeping safety checks intact.

Changes:

• Fix SSRF base URL validation for bracketed IPv6 literals by stripping [/] before IpAddr parsing.
• Make DNS-failure SSRF tests tolerant to .invalid DNS hijacking by probing and skipping when hijacked.
• Refresh CLI long-help snapshot and improve test isolation to avoid reading developer-local MCP config.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
tests/module_init_integration.rs	Isolates extension manager test from user ~/.ironclaw by overriding IRONCLAW_BASE_DIR.
src/setup/channels.rs	Adds DNS-hijack probe/skip to stabilize validate_public_https_url DNS-error test.
src/config/helpers.rs	Fixes IPv6 bracket parsing in validate_base_url and adds DNS-hijack-aware skipping for DNS-failure test.
src/cli/snapshots/ironclaw__cli__tests__long_help_output_without_import.snap	Updates snapshot for clap long-help whitespace/line-number changes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This test uses unsafe { std::env::set_var(...) } and assumes no other test/code path has already initialized the IRONCLAW_BASE_DIR LazyLock. Because Rust tests in the same binary can run in parallel and in any order, another test could call ironclaw_base_dir() first, making this override ineffective and reintroducing flakiness. Consider setting IRONCLAW_BASE_DIR via a single global Once/LazyLock initializer that is forced at the start of every test in this file (and keep the temp dir alive for the process lifetime) so the env var is definitely set before any potential ironclaw_base_dir() call.

[ilblackdragon]

Addressed in f292ec9 — but in a different way than suggested. Rather than wrap the env-var override in a global Once/LazyLock, I removed the env mutation entirely.

The reviewer (separately) raised the same architectural concern: integration tests cannot reach the crate-private ENV_MUTEX (it's pub(crate) #[cfg(test)] in src/config/helpers.rs), and any unsafe { std::env::set_var } from a tests/*.rs binary is racing arbitrary getenv calls in code under test running on parallel threads.

Looking at what the test actually verifies, the is_empty() assertion was the only thing forcing the override. The test name is extension_manager_with_process_manager_constructs — its purpose is to exercise the constructor and confirm list() doesn't error. 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 via the file-based load_mcp_servers() fallback when store: None).

Dropping the assertion eliminates the env-var pressure altogether. The test still meaningfully exercises the constructor wiring and confirms list().await.is_ok(). Documented in a multi-line comment in the test body so the reasoning is preserved.

[Copilot]

This async test performs a blocking DNS lookup via std::net::ToSocketAddrs. In a #[tokio::test], that can block a runtime worker thread and can hang for a long time on slow/offline DNS, making the test suite flaky. Prefer using tokio::net::lookup_host (optionally wrapped in a short tokio::time::timeout) or spawn_blocking for the probe lookup.

Suggested change

	use std::net::ToSocketAddrs;
	if ("ironclaw-dns-hijack-probe.invalid", 443u16)
	.to_socket_addrs()
	.is_ok()
	if tokio::time::timeout(
	std::time::Duration::from_secs(2),
	tokio::net::lookup_host(("ironclaw-dns-hijack-probe.invalid", 443u16)),
	)
	.await
	.map(|result| result.is_ok())
	.unwrap_or(false)

[ilblackdragon]

Applied in f292ec9. Now uses tokio::time::timeout(2s, tokio::net::lookup_host(...)) so the probe never blocks the runtime and can't stall the suite on slow/offline DNS. Thanks for catching this!

[gemini-code-assist]

Code Review

This pull request improves the robustness of URL validation and test isolation. It correctly handles IPv6 literals by stripping brackets before parsing, adds checks to skip DNS-related tests on networks with DNS hijacking, and ensures integration tests are isolated from the user's local configuration directory. A suggestion was made to simplify the bracket-stripping logic using trim_matches.

[gemini-code-assist]

The logic for stripping brackets from IPv6 literals can be simplified using trim_matches instead of chaining strip_prefix and strip_suffix.

Suggested change

	let host_for_parse = host
	.strip_prefix('[')
	.and_then(|s| s.strip_suffix(']'))
	.unwrap_or(host);
	let host_for_parse = host.trim_matches(|c| c == '[' || c == ']');

[ilblackdragon]

Applied in f292ec9 — switched to host.trim_matches(|c| c == '[' || c == ']'). Functionally equivalent for any host string from Url::host_str() (which only ever returns matched brackets) and a lot easier to read. Thanks!

[serrrfirat]

One test-isolation concern before this lands:

tests/module_init_integration.rs now sets IRONCLAW_BASE_DIR with unsafe std::env::set_var, but this integration test does not hold the repo's shared env-test mutex and it does not restore the previous value afterward. Because ironclaw_base_dir() is backed by a process-wide LazyLock, once this test causes that value to be computed under the temp override, later tests in the same integration-test binary can be pinned to a temp dir that gets deleted at the end of the test.

That makes the fix order-dependent and still relies on unsynchronized env mutation under the parallel test harness. I think this should either avoid mutating IRONCLAW_BASE_DIR in-process, or wrap it in a test-local guard that restores the old value and prevents concurrent env access.

[serrrfirat]

Approving because the main IPv6 SSRF fix and the test-suite repairs look correct overall. I left one follow-up comment about the new IRONCLAW_BASE_DIR test isolation relying on unsynchronized global env mutation plus cached LazyLock state.

[claude]

No issues found.

[claude]

Code Review - Updated

Found 4 issues:

1. [CRITICAL:75] IPv6 bracket parsing bug in src/tools/mcp/auth.rs:383 — Same vulnerability fixed in config/helpers.rs but not applied here

ironclaw/src/tools/mcp/auth.rs

Lines 378 to 390 in 3670290

	let host = parsed
	.host_str()
	.ok_or_else(|| AuthError::DiscoveryFailed("URL has no host".to_string()))?;
	// For IP literals, parse directly and check.
	if let Ok(ip) = host.parse::<IpAddr>()
	&& is_dangerous_ip(ip)
	{
	return Err(AuthError::DiscoveryFailed(format!(
	"URL points to a restricted IP address: {}",
	host
	)));
	}

2. [CRITICAL:75] IPv6 bracket parsing bug in src/tools/builtin/http.rs:158 — Same vulnerability as above

ironclaw/src/tools/builtin/http.rs

Lines 155 to 165 in 3670290

	}
	// Check literal IP addresses
	if let Ok(ip) = host.parse::<IpAddr>()
	&& is_disallowed_ip(&ip)
	{
	return Err(ToolError::NotAuthorized(
	"private or local IPs are not allowed".to_string(),
	));
	}

3. [CRITICAL:70] IPv6 bracket parsing bug in src/registry/installer.rs:75 — Opposite polarity (rejects IPs) but same parse issue allows bypass

ironclaw/src/registry/installer.rs

Lines 67 to 84 in 3670290

	let host = parsed
	.host_str()
	.ok_or_else(|| RegistryError::InvalidManifest {
	name: manifest_name.to_string(),
	field,
	reason: "URL host is missing".to_string(),
	})?;
	if host.parse::<IpAddr>().is_ok() || !is_allowed_artifact_host(host) {
	return Err(RegistryError::InvalidManifest {
	name: manifest_name.to_string(),
	field,
	reason: format!("host '{}' is not allowed", host),
	});
	}
	Ok(())
	}

4. [MEDIUM:40] Incomplete security fix — IPv6 bracket parsing fixed in config/helpers.rs but not propagated to all SSRF-defense code paths

Per .claude/rules/review-discipline.md: "Fix the pattern, not just the instance" — when a bug is fixed in one location, all instances across the codebase should be fixed.