test(channels): add Slack E2E tests, integration tests, and smoke runner

[serrrfirat]

Summary

• Add Slack API URL rewriting in wrapper.rs for redirecting slack.com and files.slack.com to a fake test server
• Create fake_slack_api.py mock Slack Web API server (chat.postMessage, file downloads, control endpoints)
• Add 12 Python E2E tests: setup/DM roundtrip, app_mention, URL verification, auth rejection, HMAC validation, bot filtering, mention stripping, threads, malformed payloads, file attachments
• Add 12 Rust integration tests: owner/unauthorized DM, pairing policy, open policy, URL verification, mention stripping, bot/subtype filtering, metadata, respond posting, file attachments, channel message filtering
• Add conftest.py session fixtures (fake_slack_server, slack_e2e_server) for isolated test instances
• Add local smoke test runner (scripts/slack_smoke/) for pre-release validation against real Slack

Test plan

• cargo fmt — clean
• cargo clippy --test slack_auth_integration -- -D warnings — zero warnings
• cargo test --test slack_auth_integration — 5/5 non-integration tests pass
• cargo test --test slack_auth_integration --features integration — 12/12 all tests pass
• Python syntax check on all 3 new Python files — OK
• E2E tests (pytest scenarios/test_slack_e2e.py -v) — requires running IronClaw instance
• Smoke tests — requires live Slack tokens

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request implements a robust testing framework for the Slack WASM channel integration. It introduces a Python-based smoke test runner for live validation, a mock Slack API server for end-to-end testing, and a suite of Rust integration tests covering authentication and message handling. Additionally, the core WASM wrapper now supports Slack API URL rewriting for testing purposes. Review feedback suggests enhancing test reliability by eliminating wall-clock time dependencies, ensuring thread-safe environment variable usage in tests, and consolidating duplicated file-path discovery logic.

[gemini-code-assist]

The fallback to time.time() - 1 is fragile and could lead to flaky tests. To prevent flaky tests, avoid logic based on wall-clock time. If the message timestamp (ts) for the file upload isn't found in the API response, it indicates an unexpected behavior. Instead of using a fallback that might be incorrect, it's better to raise an error to make the test fail explicitly and clearly.

Suggested change

	# Fallback: use current time minus a second
	sent_ts = str(time.time() - 1)
	raise SmokeError(
	f"Could not find message timestamp for file upload in channel {cfg.dm_channel}"
	)

References

1. To prevent flaky tests, avoid assertions based on wall-clock time. Instead, verify state changes by comparing values (e.g., counts) before and after an action.

[gemini-code-assist]

Modifying environment variables in tests modifies shared global state, which can lead to race conditions and flakiness when tests are run in parallel. These tests should be serialized using a mutex to prevent such issues. Additionally, ensure environment variable modifications happen in a single-threaded context and use a RAII guard to ensure the environment variable is restored even in case of a panic.

    static ENV_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(());

    fn test_rewrite_slack_api_url_for_testing_uses_test_override() {
        let _lock = ENV_MUTEX.lock().unwrap();
        struct EnvGuard {
            key: &'static str,
            original_value: Option<String>,
        }

        impl EnvGuard {
            fn new(key: &'static str, value: &str) -> Self {
                let original_value = std::env::var(key).ok();
                std::env::set_var(key, value);
                Self { key, original_value }
            }
        }

        impl Drop for EnvGuard {
            fn drop(&mut self) {
                if let Some(val) = &self.original_value {
                    std::env::set_var(self.key, val);
                } else {
                    std::env::remove_var(self.key);
                }
            }
        }

        let _guard = EnvGuard::new(SLACK_TEST_API_BASE_ENV, "http://localhost:9999");

        let result = rewrite_slack_api_url_for_testing("https://slack.com/api/chat.postMessage");
        assert_eq!(
            result.as_deref(),
            Some("http://localhost:9999/api/chat.postMessage")
        );
        let result = rewrite_slack_api_url_for_testing(
            "https://files.slack.com/files-pri/T123/download/test.txt",
        );
        assert_eq!(
            result.as_deref(),
            Some("http://localhost:9999/files-pri/T123/download/test.txt")
        );
        let result = rewrite_slack_api_url_for_testing("https://api.telegram.org/bot123/getMe");
        assert!(result.is_none());
    }

References

1. Tests that modify shared global state should be serialized using a mutex to prevent race conditions and flakiness when run in parallel.
2. To safely modify environment variables using std::env::set_var, ensure the modification happens in a single-threaded context.

[gemini-code-assist]

The logic for finding files within a git worktree is duplicated in multiple functions. Encapsulate this complex logic within a single helper function to reduce duplication and provide a simpler interface tailored to the caller's needs.

fn find_project_file(relative_path: &str) -> std::path::PathBuf {
    let local = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR"))
        .join(relative_path);
    if local.exists() {
        return local;
    }

    if let Ok(output) = std::process::Command::new("git")
        .args(["worktree", "list", "--porcelain"])
        .output()
        && output.status.success()
    {
        let stdout = String::from_utf8_lossy(&output.stdout);
        for line in stdout.lines() {
            if let Some(path) = line.strip_prefix("worktree ") {
                let candidate = std::path::PathBuf::from(path)
                    .join(relative_path);
                if candidate.exists() {
                    return candidate;
                }
            }
        }
    }

    local
}

fn slack_wasm_path() -> std::path::PathBuf {
    find_project_file("channels-src/slack/target/wasm32-wasip2/release/slack_channel.wasm")
}

fn slack_capabilities_path() -> std::path::PathBuf {
    find_project_file("channels-src/slack/slack.capabilities.json")
}

References

1. Encapsulate complex data structures and logic within a helper function, and expose a simpler interface or data structure tailored to the caller's needs.

[ilblackdragon]

Automated review — Approve with 2 fixes

TL;DR: Comprehensive test suite (12 Python E2E + 12 Rust integration) for Slack channel with HMAC signing, URL verification, and deterministic mock Slack API. Fix the env-var race and time-based fallback before merge; rest is high quality.

Slack-specific coverage

Area	Status	Notes
HMAC-SHA256 v0 verification	✓ Full	`compute_slack_signature()`; headers set in all webhook posts
URL verification challenge	✓ Full	Python + Rust tests
Rate limit handling	⚠ Partial	Mock infrastructure (429, Retry-After) in place but no E2E exercises it
Event / slash / interactive	Event only	Slash/interactive out of scope
Thread / DM / channel variants	✓ Full	`thread_ts` propagation, channel mention variants
Bot filtering	✓ Full	`test_bot_message_with_bot_id_ignored()`
Mention stripping	✓ Full	Rust + Python

Findings

#	Severity	File:Line	Issue	Suggestion
1	Medium	`scripts/slack_smoke/run_smoke.py:219`	Time-based fallback `time.time() - 1` for file-upload ts lookup is fragile — clock skew or parallel runs can match wrong message or miss actual upload	Replace with explicit `raise SmokeError(f"Could not find message timestamp for file upload in channel {cfg.dm_channel}")`
2	Medium	`src/channels/wasm/wrapper.rs:6010-6015`	Unguarded `std::env::set_var()` + `std::env::remove_var()` races with parallel test execution	Wrap with a static `OnceLock<Mutex<()>>` guard. The `ScopedEnvVar` pattern already exists in `slack_auth_integration.rs` — reuse it
3	Low	`tests/slack_auth_integration.rs:46-98`	`slack_wasm_path()` and `slack_capabilities_path()` duplicate git-worktree discovery logic	Extract common `find_project_file(relative_path)` helper
4	Info	`tests/e2e/scenarios/test_slack_e2e.py`	Rate-limit infrastructure exists (`/__mock/set_rate_limit`) but no E2E exercises it	Consider `test_slack_rate_limit_resilience()` if the channel implements backoff

Test quality

• Determinism: Strong. Mock is fully deterministic; tests use UUIDs and reset state. Parallel execution risk exists due to finding #2.
• Cleanup: Excellent. Fixtures handle SIGINT graceful shutdown; Python tests reset via `/__mock/reset`; smoke uses tempfile for attachments.
• CI compatibility: Good. Tests gate on `#[cfg(feature = "integration")]` and WASM artifact existence. Smoke skips gracefully on missing credentials.

Open questions

1. Does the Slack WASM channel implement retry logic for 429? If so, an E2E test should exercise it.
2. Are two tokens (user + bot) the minimum for smoke, or can a single bot token cover all cases?
3. Is the `run_smoke.py` healthcheck endpoint intentionally optional (dry-run mode), or should it be mandatory for early-failure detection?
4. Should test-message cleanup from DM/channels be part of E2E teardown (currently tracked, not deleted)?

[serrrfirat]

Addressed all review findings in 43154df:

1. **Finding 1 — Replaced time.time() - 1 with explicit raise SmokeError(...) so missing file-upload timestamps fail loudly instead of producing flaky matches.

2. **Finding 2 ** — Wrapped set_var/remove_var in the wrapper.rs unit test with OnceLock<Mutex<()>> guard and RAII-style original value restoration, matching the ScopedEnvVar pattern from the integration tests.

3. Finding 3 — Extracted find_project_file(relative_path) helper; slack_wasm_path() and slack_capabilities_path() are now one-liners.

Re: open questions:

• Rate-limit E2E test — the mock infrastructure (/__mock/set_rate_limit, 429 responses) is in place; will add a dedicated test when the channel implements retry/backoff.
• Two tokens are the minimum for smoke — user token triggers the webhook (Slack doesn't fire events for bot-sent messages), bot token reads history to find replies.
• Healthcheck is intentionally optional — allows running smoke against instances without the health endpoint exposed, or skipping the check for faster iteration.
• Test message cleanup — not deleting messages is deliberate; smoke runs use unique UUIDs so old messages don't interfere, and cleanup would require additional token scopes.

[ilblackdragon]

Should avoid having a slack specific rewriting rules in production code.
Should be a general rewriting for testing infra (similar how we have HTTP injector) to ensure that we don't have specifics of every channel polluting code.

Plus please merge latest origin staging to fix the conflicts.

[ilblackdragon]

Code Review

Overview

Adds Slack channel test infrastructure on three layers: a fake Slack Web API server (fake_slack_api.py), 12 Python E2E scenarios, 12 Rust integration tests, and a local smoke runner that exercises real Slack. The reusable bit is a new generic test-only HTTP rewrite hook (IRONCLAW_TEST_HTTP_REWRITE_MAP env var) in src/channels/wasm/wrapper.rs:557 that lets tests redirect any production hostname to a local fake — addressing the earlier request to avoid Slack-specific code in the production path.

Issues

Critical — Likely SSRF check regression that breaks URL rewriting in tests

At src/channels/wasm/wrapper.rs:407, the PR changes:
```rust

• reject_private_ip(&url)?;

• reject_private_ip(&transport_url)?;
  ```

`reject_private_ip` rejects loopback (`v4.is_loopback()` in `src/tools/wasm/http_security.rs:71`, with explicit unit test `test_reject_private_ip_loopback` asserting `127.0.0.1` is `Err`). With this change, every test that rewrites a production hostname to `http://127.0.0.1:PORT/...\` will hit `reject_private_ip(&"http://127.0.0.1:PORT/...\")\` and bail out with "private/internal IP not allowed" — including:

• The new `test_respond_posts_to_slack_api` (`tests/slack_auth_integration.rs:2192`)
• The pre-existing Telegram counterpart `test_telegram_respond_posts_to_api` in `tests/telegram_auth_integration.rs:520` (uses the same rewrite-to-loopback pattern)
• All Python E2E tests in `test_slack_e2e.py` that depend on `fake_slack_server`

The PR claims `cargo test --test slack_auth_integration --features integration` is 12/12 green, but `require_slack_wasm!` (`tests/slack_auth_integration.rs:1591`) silently `return`s when the WASM module isn't built locally — only CI panics. If the Slack WASM module wasn't built when you ran the suite, every integration-feature test passed by skipping. Please rerun after `cd channels-src/slack && cargo build --target wasm32-wasip2 --release` to confirm `test_respond_posts_to_slack_api` actually executes the `respond` path and that `chat.postMessage` payloads reach the fake server.

If this analysis is right, there are two reasonable fixes:

1. Revert to `reject_private_ip(&logical_url)` (post-credential-injection but pre-rewrite) — preserves the original SSRF semantics while still benefiting from credential injection on the leak-scan path.
2. Move the `reject_private_ip` call ahead of the `transport_url` rewrite, so it always validates the public destination, never the private rewrite target.

Either way, the change as written conflates "the URL we'll connect to" with "the URL the WASM module declared it wanted to reach", and the current PR breaks the test escape hatch.

Significant — Generic rewrite hook is in production code, not test code

The `rewrite_http_url_for_testing` helper (`wrapper.rs:557`) is gated by `#[cfg(any(test, debug_assertions))]`. That means debug builds in development environments also honor `IRONCLAW_TEST_HTTP_REWRITE_MAP`. A developer running `cargo run` against a real Slack workspace with that env var leaked into their shell could silently MITM their own bot traffic to wherever the env var points. Recommendations:

• Tighten the gate to `#[cfg(test)]` only, OR
• Refuse rewrites when the build is `--release`, OR
• Log a `tracing::warn!` once at startup if the env var is set, so it's at least loud.

The Telegram precedent (`TELEGRAM_TEST_API_BASE_ENV`) has the same shape, so there's an existing convention — but generalizing it to "any host in any debug build" raises the blast radius meaningfully and is worth a second look.

Significant — `_patch_slack_capabilities_for_testing` mutates a checked-in file from a session-scoped fixture

`tests/e2e/scenarios/test_slack_e2e.py:1065` writes back to `slack.capabilities.json` inside the `channels_dir`. If `channels_dir` ever points at a shared location (e.g. someone runs the suite against a developer install, or two pytest processes share state), this corrupts the manifest mid-run. The current `slack_e2e_server` fixture creates an isolated `tempfile.mkdtemp()`-based `channels_dir`, so it's OK in practice — but the function reads/writes without a lock and the mutation is one-way (no restore on teardown). A safer pattern: assert the path is under the temp dir, write the patched copy explicitly, and don't touch the source file.

Better still, declare the extra capabilities (files.slack.com host) as part of the bundled `slack.capabilities.json` so tests don't need to patch at all.

Significant — `require_slack_wasm!` makes failures invisible locally

This is the second time in this review I've flagged this. The macro:
```rust
if !slack_wasm_path().exists() {
if std::env::var("CI").is_ok() { panic!(...) }
eprintln!("Skipping test: ...");
return; // <-- test reports as PASSED
}
```
A skipped test reports as passed in cargo's output. Local runs of `cargo test --test slack_auth_integration` will report 12/12 green even if zero tests actually executed. This is exactly the failure mode that hides the SSRF regression above. Two possible fixes:

• Use `#[ignore]` + `cargo test -- --include-ignored` so skipped tests are visibly skipped.
• Have `slack_wasm_path()` build the WASM on demand (cargo `build.rs` or `xtask`-style helper).

The same pattern exists for Telegram, so this isn't introduced by this PR — but the PR doubles the surface area, and the SSRF concern above only matters if these tests actually run.

Minor — Logging credentials in `transport_url`

`wrapper.rs:389-393` logs both `logical_url` and `transport_url` at `info!` level when a rewrite happens. After credential injection (Telegram-style `https://api.telegram.org/botREAL_TOKEN/...\`), `logical_url` may contain bot tokens in the path. Pre-existing issue but worth noting because the log message is now broader ("outbound HTTP request" rather than "Telegram API request"). Consider redacting the path component, or downgrading to `debug!`.

Minor — `parse_test_http_rewrite_map` re-parses on every request

`rewrite_http_url_for_testing` calls `std::env::var(...)` and `parse_test_http_rewrite_map(...)` (which calls `serde_json::from_str`) on every WASM HTTP request. For a production debug build that's cheap; for a busy test it's wasteful. A `OnceLock` initialized lazily would be cleaner, though this is genuinely minor since the env var is read-only after process start.

Minor — Brittle assertion in `test_slack_unauthorized_user_rejected`

`test_slack_e2e.py:1357` asserts `"how can i help" not in text` — this depends on the mock LLM's reply phrasing. Stronger assertion: verify the message count is unchanged for that channel after the unauthorized webhook (which is what `test_slack_bot_message_ignored` does correctly).

Minor — Stray formatting change

The diff removes a blank line between `rewrite_telegram_api_url_for_testing` and `should_skip_response_leak_scan` (`wrapper.rs:4000`). Doesn't matter functionally; rustfmt should put it back if you re-run.

Minor — Subprocess stderr is captured but never read

Both `fake_slack_server` and `slack_e2e_server` fixtures use `stderr=asyncio.subprocess.PIPE` but never drain it. If either subprocess writes more than the OS pipe buffer (~64KB on Linux), it blocks. Pipe to a temp file or use `stderr=asyncio.subprocess.DEVNULL` if you don't care about it.

What's good

• Generic rewrite hook addresses the earlier feedback exactly — Slack-specific code stays out of `wrapper.rs`. Reusable for any future channel.
• `ScopedEnvVar` (`tests/slack_auth_integration.rs:1722`) is the right pattern for safe env-var manipulation in concurrent tests; the SAFETY comments correctly identify the invariants.
• `find_project_file` worktree fallback is a thoughtful touch for developers using `git worktree`.
• Fake Slack API has a clean control surface (`/__mock/reset`, `/__mock/set_rate_limit`, etc.) — good infrastructure for future tests.
• 12 Rust + 12 Python tests cover the important policy paths (owner DM, allowlist, pairing, open, HMAC, app_mention, bot filtering, subtype filtering, threads, files, malformed payloads, channel filtering).
• The CI vs. local distinction in `require_slack_wasm!` (panic vs skip) shows awareness of the silent-skip trap, even though I think the local default should be louder.
• Smoke runner is well-documented and supports a useful mock-LLM-with-substring-match mode for deterministic CI later.

Recommendation

The SSRF check change (`reject_private_ip(&url)` → `&transport_url`) needs to be either reverted or proven benign by actually running the integration tests with the WASM module built. Until that's done, the "12/12 pass" claim is unverifiable, and there's a concrete reason to believe `test_respond_posts_to_slack_api` and the existing Telegram counterpart can no longer reach `chat.postMessage`/`sendMessage`. The debug-build guard on `IRONCLAW_TEST_HTTP_REWRITE_MAP` should also be tightened. The remaining items are cleanup-grade and not blockers.

[ilblackdragon]

Verdict: Approve with nits

• src/channels/wasm/wrapper.rs:411 — reject_private_ip switched from &url to &transport_url. Correct in test/debug builds where the rewrite map is active; release path is a no-op. Add a comment explaining the switch.
• Cache the parsed IRONCLAW_TEST_HTTP_REWRITE_MAP in a OnceLock to avoid per-request JSON parsing in debug.
• HMAC reject + bot-loop + owner-vs-stranger tests are valuable additions.
• All .unwrap()/.expect() confined to tests. Test-only env var properly gated behind #[cfg(any(test, debug_assertions))]. No DB code touched.

Strong test coverage: setup, DM round-trip, app_mention, URL verification challenge, HMAC reject, bot/subtype filtering, mention stripping, threading, malformed payload resilience, file attachments, plus a real chat.postMessage round-trip via an in-process axum fake.