refactor(gateway): delete server.rs shim + relocate tests to slices — ironclaw#2599 stage 6 - #2706
Conversation
… ironclaw#2599 stage 6 Finishes the feature-slice migration started in stage 4a. After this: - `src/channels/web/server.rs` no longer exists. - Every caller of `crate::channels::web::server::*` now points at `platform::router::start_server` or `platform::state::*` directly. - All ~60 caller-level tests that used to live in `server.rs::tests` now live inside the feature slice they actually exercise, next to the handler they test. ## What moved where Classification driven by the handler each test drives: | Slice | Tests | |---|---| | `features/chat/mod.rs::tests` | 3 × history, 4 × auth-token/cancel + gate-resolve, 1 × approval, 3 × pending-gate-extension-name, 1 × test_auth_manager helper | | `features/pairing/mod.rs::tests` | 1 × list, 5 × approve (claim / no-followup / with-thread / external-callback / blank-code), `make_pairing_test_state` helper | | `features/extensions/mod.rs::tests` | 2 × activation classifier, 2 × path-traversal guards, 1 × setup-submit-not-activated, 2 × list-inactive-wasm-channel, 1 × phase-precedence, 1 × readiness handler, 2 × apply_extension_readiness | | `features/oauth/mod.rs::tests` | 13 × oauth callback (missing params / unknown state / expired × 2 / no-ext-mgr / strip-prefix / versioned × 2 / happy × 3 / exchange-fail), 5 × relay oauth callback, + `TestOauthProxy`, `EnvVarGuard`, `set_env_var`, `fresh_pending_oauth_flow`, `expired_flow_created_at`, `test_oauth_router`, `test_relay_oauth_router` helpers | | `platform/static_files.rs::tests` | 3 × CSP header / base / nonce, 2 × css etag, 1 × css handler, 2 × css multi-tenant, 4 × stamp nonce + build frontend HTML, 1 × test_build_frontend_html_returns_none_in_multi_tenant_mode | | `platform/state.rs::tests` | 1 × workspace_pool_resolve_seeds_new_user_workspace | | `handlers/llm.rs::tests` | 3 × llm admin-role guards | | `handlers/users.rs::tests` | 1 × delete_user_evicts_auth_and_pairing_caches | ## Cross-slice test fixtures Four helpers that multiple slices share (`insert_test_user`, `test_secrets_store`, `test_ext_mgr`, `test_ext_mgr_with_db`) moved into `src/channels/web/test_helpers.rs` as `#[cfg(test)] pub(crate)` free functions, following the pattern from stage 6a (#2704) for `test_gateway_state*`. All four keep the exact signatures they had in `server.rs::tests`, so the move was mechanical. Rust expect suppressions on the five `.expect(...)` lines inside these fixtures carry `// safety: cfg(test) fixture` comments — the pre-commit safety check is diff-line based and doesn't look up whether the containing function is already `cfg(test)`-gated. ## Mechanical renames (25 files) `channels::web::server::<item>` call sites now import from: - `platform::router::start_server` - `platform::state::{GatewayState, RateLimiter, PerUserRateLimiter, WorkspacePool, FrontendCacheKey, FrontendHtmlCache, ActiveConfigSnapshot, PromptQueue, RoutineEngineSlot, rate_limit_key_from_headers}` Covers `src/main.rs`, `src/app.rs`, `src/tools/builtin/{job,memory}.rs`, all 13 handlers in `handlers/*.rs`, the four integration tests (`ws_gateway_integration`, `openai_compat_integration`, `multi_tenant_integration`, `oauth_greeting_integration`), plus `tests/support/gateway_workflow_harness.rs` and `src/channels/web/tests/multi_tenant.rs`. No behavior change. ## Boundary checker retained `scripts/check_gateway_boundaries.py` still rejects any `crate::channels::web::server::` path as a defense-in-depth guard against accidental re-introduction (literal new `server.rs`, stray imports, etc.). The explanatory comment and the regression test's docstring now reflect "shim is gone; this guard prevents re-creation" instead of "shim exists; don't route through it." ## Documentation updates - `src/channels/web/CLAUDE.md`: deleted the `server.rs` File Map row, updated the `test_helpers.rs` row to list all seven `pub(crate)` fixtures (stages 6a + 6 together), fixed all prose references that pointed at `server.rs`, and updated the "Adding a New API Endpoint" recipe to point at `features/<slice>/` and `platform/router.rs`. - `src/channels/web/platform/state.rs`: module docstring now says "shim was removed" instead of "shim exists pending migration." - `src/bridge/CLAUDE.md`: `pending_gate_extension_name` reference now points at `features/chat/mod.rs`. ## Quality gate - [x] `cargo fmt --all` - [x] `cargo clippy --all --benches --tests --examples --all-features` — zero warnings - [x] `cargo check -p ironclaw --no-default-features --features libsql --tests` — clean - [x] `cargo test -p ironclaw --lib channels::web` — 434 passed (up from 431 — three tests that were incorrectly filtered under `channels::web::server::tests` now surface under their proper slice's module path) - [x] `cargo test -p ironclaw --test multi_tenant_integration` — 40 passed - [x] `cargo test -p ironclaw --test openai_compat_integration` — 16 passed - [x] `cargo test -p ironclaw --test ws_gateway_integration` — 11 passed - [x] `python3 scripts/check_gateway_boundaries.py` — clean - [x] `python3 scripts/check_gateway_boundaries.py test` — 16/16 - [x] `bash scripts/pre-commit-safety.sh` — clean ## Regression coverage Pure relocation + mechanical rename; no behavior change. The existing ~60 tests from `server.rs::tests` continue to pass unmodified, which is the regression evidence. A "test that would have caught this" would necessarily duplicate the existing tests — no new test adds coverage. [skip-regression-check] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Removes the last channels::web::server compatibility shim and completes stage-6 of the gateway slice migration by redirecting all call sites to platform::{router,state} and relocating the former server.rs::tests into the feature slices they exercise.
Changes:
- Deleted the
server.rsmodule path usage by switching imports toplatform::router::start_serverandplatform::state::*across the app, handlers, and integration tests. - Relocated ~60 caller-level tests from the former
server.rs::testsintofeatures/{chat,extensions,oauth,pairing}, plus new test modules inplatform/{static_files,state}and a few handler test modules. - Expanded
channels::web::test_helperswith additional cross-slice#[cfg(test)] pub(crate)fixtures and updated boundary-checker + docs to reflect shim removal.
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/ws_gateway_integration.rs | Updates server startup/state imports to platform::{router,state}. |
| tests/support/gateway_workflow_harness.rs | Updates harness imports to platform::{router,state}. |
| tests/openai_compat_integration.rs | Updates server startup/state imports to platform::{router,state}. |
| tests/oauth_greeting_integration.rs | Updates server startup/state imports to platform::{router,state}. |
| tests/multi_tenant_integration.rs | Updates server startup calls/imports away from web::server. |
| src/tools/builtin/memory.rs | Switches WorkspacePool reference to platform::state. |
| src/tools/builtin/job.rs | Updates comment to reference platform::state::PromptQueue. |
| src/main.rs | Switches state type aliases/constructors to platform::state. |
| src/channels/web/tests/multi_tenant.rs | Updates imports to platform::state types. |
| src/channels/web/test_helpers.rs | Adds shared cross-slice test fixtures and updates imports to platform. |
| src/channels/web/responses_api.rs | Repoints GatewayState import to platform::state. |
| src/channels/web/platform/static_files.rs | Adds a large in-module test suite for CSP/static serving behavior. |
| src/channels/web/platform/state.rs | Updates module docs and adds a WorkspacePool unit test. |
| src/channels/web/openai_compat.rs | Repoints GatewayState import to platform::state. |
| src/channels/web/mod.rs | Removes pub mod server; and rewires remaining state/start_server references to platform. |
| src/channels/web/handlers/webhooks.rs | Repoints GatewayState import to platform::state. |
| src/channels/web/handlers/users.rs | Repoints GatewayState import and adds deletion cache-eviction test. |
| src/channels/web/handlers/tool_policy.rs | Repoints GatewayState import to platform::state. |
| src/channels/web/handlers/tokens.rs | Repoints GatewayState import to platform::state. |
| src/channels/web/handlers/system_prompt.rs | Repoints GatewayState import to platform::state. |
| src/channels/web/handlers/static_files.rs | Repoints GatewayState import to platform::state. |
| src/channels/web/handlers/skills.rs | Repoints GatewayState import to platform::state. |
| src/channels/web/handlers/secrets.rs | Repoints GatewayState import to platform::state. |
| src/channels/web/handlers/memory.rs | Repoints GatewayState import to platform::state. |
| src/channels/web/handlers/llm.rs | Repoints GatewayState import and adds admin-guard tests. |
| src/channels/web/handlers/engine.rs | Repoints GatewayState import to platform::state. |
| src/channels/web/handlers/auth.rs | Repoints GatewayState import and updates rate-limit key helper call. |
| src/channels/web/features/pairing/mod.rs | Adds relocated pairing tests + shared state builder helper. |
| src/channels/web/features/oauth/mod.rs | Adds relocated OAuth/relay callback test suite and helpers. |
| src/channels/web/features/extensions/mod.rs | Adds relocated extensions tests (validation, readiness, setup-submit, etc.). |
| src/channels/web/features/chat/mod.rs | Adds relocated chat tests (history, auth-token/cancel, gate resolve, pending-gate name). |
| src/channels/web/CLAUDE.md | Updates documentation for shim removal + new test helper locations. |
| src/bridge/CLAUDE.md | Updates references to pending_gate_extension_name new location. |
| src/app.rs | Switches WorkspacePool constructor to platform::state. |
| scripts/check_gateway_boundaries.py | Updates comments/tests to treat web::server as a forbidden “no reintroduction” path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert_eq!(a.len(), 32, "16 bytes hex-encoded should be 32 chars"); | ||
| assert_ne!(a, b, "nonces must be unique per call"); | ||
| assert!( | ||
| a.chars().all(|c| c.is_ascii_hexdigit()), |
There was a problem hiding this comment.
The assertion only checks is_ascii_hexdigit() (accepts A–F) but the failure message says the nonce must be lowercase hex. Either tighten the check to enforce lowercase (since generate_csp_nonce() uses hex::encode which is lowercase) or adjust the message to match what’s actually being validated.
| a.chars().all(|c| c.is_ascii_hexdigit()), | |
| a.chars().all(|c| matches!(c, '0'..='9' | 'a'..='f')), |
There was a problem hiding this comment.
Fixed in b8dda5e. Tightened the assertion to matches!(c, '0'..='9' | 'a'..='f') so it matches the "lowercase hex" message and what generate_csp_nonce actually produces via hex::encode.
There was a problem hiding this comment.
Code Review
This pull request completes the removal of the server.rs backward-compatibility shim as part of the ironclaw#2599 migration, updating all internal and external references to the new platform/ and features/ modules. It also introduces comprehensive unit and integration tests for chat, extensions, OAuth, and pairing features. Feedback suggests improving test robustness by using .expect() instead of early returns when setting up environmental test state to avoid silent false positives.
| eprintln!("Skipping expired OAuth flow test: monotonic uptime below expiry window"); | ||
| return; | ||
| }; | ||
|
|
There was a problem hiding this comment.
The pattern of returning early from a test when expired_flow_created_at() returns None makes the test suite fragile and prone to silent false positives. On systems with low uptime, Instant::now().checked_sub(...) will return None. Per repository rules, you should prefer using .expect() to explicitly fail the test with a clear message if the setup is not possible, rather than allowing the test to silently skip the logic.
References
- In tests, when setting up a state that depends on environmental factors, prefer expect() to explicitly fail the test with a clear message if the setup is not possible. Avoid fallbacks that could cause the test to silently check the wrong logic.
There was a problem hiding this comment.
Good catch — fixed in b8dda5e. Changed expired_flow_created_at() to return Instant directly (moving the .expect(...) inside the helper) and collapsed the five let Some(..) = expired_flow_created_at() else { eprintln!(...); return; }; call sites into let created_at = expired_flow_created_at();.
On a just-booted CI host the test now fails loudly with "monotonic clock must have run long enough for expired_flow_created_at" instead of silently skipping the expired-flow branch. The .expect() carries a // safety: cfg(test) fixture suppression for the pre-commit safety check, consistent with the other test fixtures in test_helpers.rs.
…tage-6-delete-shim Incorporate stage-6 deletion with staging's new features that landed after #2704 merged: - PR #2532 "engine v2 threads in chat history and sidebar" — adds 4 new chat-history tests + `history_request` helper + `default_timezone` field on `ActiveConfigSnapshot`. - PR #1873 "debug inspector panel for web gateway" — adds `features/debug/` slice and `/api/debug/prompt` route. Conflict resolution: - `src/channels/web/server.rs`: deleted (stage 6) — the 4 new chat-history tests staging had added to `server.rs::tests` ported into `features/chat/mod.rs::tests` alongside the tests this PR already moved there. Added missing imports: `axum::extract::{Query, State}`, the third cross-slice builder `test_gateway_state_with_dependencies`, and `crate::db::Database`. - `src/main.rs::with_active_config`: kept this PR's `platform::state::` rename and folded in staging's new `default_timezone` field on `ActiveConfigSnapshot`. Also addresses two review comments on PR #2706: - Copilot on `static_files.rs:1109`: tightened the nonce hex assertion from `is_ascii_hexdigit()` (accepts A–F) to `matches!(c, '0'..='9' | 'a'..='f')` so the check matches the "lowercase hex" message and what `generate_csp_nonce` actually produces (`hex::encode`). - Gemini on `oauth/mod.rs:expired_flow_created_at`: collapsed the `Option<Instant>` return + five `let Some(..) else { eprintln!; return; }` call sites into a single `.expect()` inside the helper, so tests fail loudly on low-uptime CI instead of silently skipping the expired-flow branch coverage. Two `// safety:` suppressions added on staging-authored lines that the pre-commit check flags because the merge surfaces them as newly-added lines from this branch's upstream: - `dispatcher.rs:1220`: `output[..boundary]` is safe because `boundary` is a char-boundary from `floor_char_boundary()`. - `bridge/router.rs:5554`: `.expect("seed thread")` is in a `cfg(test)`-gated fixture. Quality gate: - cargo fmt --all - cargo clippy --all --benches --tests --examples --all-features: zero warnings - cargo test -p ironclaw --lib channels::web: 446 passed (up from 434 — staging added +12) - cargo test -p ironclaw --test ws_gateway_integration --test multi_tenant_integration: 40 + 11 passed - python3 scripts/check_gateway_boundaries.py + test: clean, 16/16 - bash scripts/pre-commit-safety.sh: clean
… ironclaw#2599 stage 6 (nearai#2706) Finishes the feature-slice migration started in stage 4a. After this: - `src/channels/web/server.rs` no longer exists. - Every caller of `crate::channels::web::server::*` now points at `platform::router::start_server` or `platform::state::*` directly. - All ~60 caller-level tests that used to live in `server.rs::tests` now live inside the feature slice they actually exercise, next to the handler they test. ## What moved where Classification driven by the handler each test drives: | Slice | Tests | |---|---| | `features/chat/mod.rs::tests` | 3 × history, 4 × auth-token/cancel + gate-resolve, 1 × approval, 3 × pending-gate-extension-name, 1 × test_auth_manager helper | | `features/pairing/mod.rs::tests` | 1 × list, 5 × approve (claim / no-followup / with-thread / external-callback / blank-code), `make_pairing_test_state` helper | | `features/extensions/mod.rs::tests` | 2 × activation classifier, 2 × path-traversal guards, 1 × setup-submit-not-activated, 2 × list-inactive-wasm-channel, 1 × phase-precedence, 1 × readiness handler, 2 × apply_extension_readiness | | `features/oauth/mod.rs::tests` | 13 × oauth callback (missing params / unknown state / expired × 2 / no-ext-mgr / strip-prefix / versioned × 2 / happy × 3 / exchange-fail), 5 × relay oauth callback, + `TestOauthProxy`, `EnvVarGuard`, `set_env_var`, `fresh_pending_oauth_flow`, `expired_flow_created_at`, `test_oauth_router`, `test_relay_oauth_router` helpers | | `platform/static_files.rs::tests` | 3 × CSP header / base / nonce, 2 × css etag, 1 × css handler, 2 × css multi-tenant, 4 × stamp nonce + build frontend HTML, 1 × test_build_frontend_html_returns_none_in_multi_tenant_mode | | `platform/state.rs::tests` | 1 × workspace_pool_resolve_seeds_new_user_workspace | | `handlers/llm.rs::tests` | 3 × llm admin-role guards | | `handlers/users.rs::tests` | 1 × delete_user_evicts_auth_and_pairing_caches | ## Cross-slice test fixtures Four helpers that multiple slices share (`insert_test_user`, `test_secrets_store`, `test_ext_mgr`, `test_ext_mgr_with_db`) moved into `src/channels/web/test_helpers.rs` as `#[cfg(test)] pub(crate)` free functions, following the pattern from stage 6a (nearai#2704) for `test_gateway_state*`. All four keep the exact signatures they had in `server.rs::tests`, so the move was mechanical. Rust expect suppressions on the five `.expect(...)` lines inside these fixtures carry `// safety: cfg(test) fixture` comments — the pre-commit safety check is diff-line based and doesn't look up whether the containing function is already `cfg(test)`-gated. ## Mechanical renames (25 files) `channels::web::server::<item>` call sites now import from: - `platform::router::start_server` - `platform::state::{GatewayState, RateLimiter, PerUserRateLimiter, WorkspacePool, FrontendCacheKey, FrontendHtmlCache, ActiveConfigSnapshot, PromptQueue, RoutineEngineSlot, rate_limit_key_from_headers}` Covers `src/main.rs`, `src/app.rs`, `src/tools/builtin/{job,memory}.rs`, all 13 handlers in `handlers/*.rs`, the four integration tests (`ws_gateway_integration`, `openai_compat_integration`, `multi_tenant_integration`, `oauth_greeting_integration`), plus `tests/support/gateway_workflow_harness.rs` and `src/channels/web/tests/multi_tenant.rs`. No behavior change. ## Boundary checker retained `scripts/check_gateway_boundaries.py` still rejects any `crate::channels::web::server::` path as a defense-in-depth guard against accidental re-introduction (literal new `server.rs`, stray imports, etc.). The explanatory comment and the regression test's docstring now reflect "shim is gone; this guard prevents re-creation" instead of "shim exists; don't route through it." ## Documentation updates - `src/channels/web/CLAUDE.md`: deleted the `server.rs` File Map row, updated the `test_helpers.rs` row to list all seven `pub(crate)` fixtures (stages 6a + 6 together), fixed all prose references that pointed at `server.rs`, and updated the "Adding a New API Endpoint" recipe to point at `features/<slice>/` and `platform/router.rs`. - `src/channels/web/platform/state.rs`: module docstring now says "shim was removed" instead of "shim exists pending migration." - `src/bridge/CLAUDE.md`: `pending_gate_extension_name` reference now points at `features/chat/mod.rs`. ## Quality gate - [x] `cargo fmt --all` - [x] `cargo clippy --all --benches --tests --examples --all-features` — zero warnings - [x] `cargo check -p ironclaw --no-default-features --features libsql --tests` — clean - [x] `cargo test -p ironclaw --lib channels::web` — 434 passed (up from 431 — three tests that were incorrectly filtered under `channels::web::server::tests` now surface under their proper slice's module path) - [x] `cargo test -p ironclaw --test multi_tenant_integration` — 40 passed - [x] `cargo test -p ironclaw --test openai_compat_integration` — 16 passed - [x] `cargo test -p ironclaw --test ws_gateway_integration` — 11 passed - [x] `python3 scripts/check_gateway_boundaries.py` — clean - [x] `python3 scripts/check_gateway_boundaries.py test` — 16/16 - [x] `bash scripts/pre-commit-safety.sh` — clean ## Regression coverage Pure relocation + mechanical rename; no behavior change. The existing ~60 tests from `server.rs::tests` continue to pass unmodified, which is the regression evidence. A "test that would have caught this" would necessarily duplicate the existing tests — no new test adds coverage. [skip-regression-check] Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Finishes the feature-slice migration started in stage 4a. After this lands:
src/channels/web/server.rsno longer exists.crate::channels::web::server::*points atplatform::router::start_serverorplatform::state::*directly.server.rs::testsnow live inside the feature slice they actually exercise, next to the handler they test.Builds on #2704 (cross-slice test builder promotion), which landed the prerequisite.
What moved where
Classification driven by the handler each test drives:
features/chat/mod.rs::teststest_auth_managerhelperfeatures/pairing/mod.rs::testsmake_pairing_test_statehelperfeatures/extensions/mod.rs::testsapply_extension_readinessfeatures/oauth/mod.rs::tests(new)TestOauthProxy/EnvVarGuard/set_env_var/fresh_pending_oauth_flow/expired_flow_created_at/test_oauth_router/test_relay_oauth_routerhelpersplatform/static_files.rs::tests(new)platform/state.rs::tests(new)workspace_pool_resolve_seeds_new_user_workspacehandlers/llm.rs::testshandlers/users.rs::tests(new)delete_user_evicts_auth_and_pairing_cachesCross-slice fixtures
Four helpers that multiple slices share moved into
src/channels/web/test_helpers.rsas#[cfg(test)] pub(crate)free functions alongside the three builders landed in #2704:insert_test_user(pairing + users)test_secrets_store(extensions + oauth)test_ext_mgr(extensions + oauth)test_ext_mgr_with_db(extensions + oauth)All keep the exact positional signatures they had in
server.rs::tests.Mechanical renames (25 files)
channels::web::server::<item>call sites now import from:platform::router::start_serverplatform::state::{GatewayState, RateLimiter, PerUserRateLimiter, WorkspacePool, FrontendCacheKey, FrontendHtmlCache, ActiveConfigSnapshot, PromptQueue, RoutineEngineSlot, rate_limit_key_from_headers}Covers
src/main.rs,src/app.rs,src/tools/builtin/{job,memory}.rs, all 13 handlers inhandlers/*.rs, the four integration tests (ws_gateway_integration,openai_compat_integration,multi_tenant_integration,oauth_greeting_integration), plustests/support/gateway_workflow_harness.rsandsrc/channels/web/tests/multi_tenant.rs. No behavior change.Boundary checker retained
scripts/check_gateway_boundaries.pystill rejects anycrate::channels::web::server::path as a defense-in-depth guard against accidental re-introduction (literal newserver.rs, stray imports, etc.). The explanatory comment and the regression test's docstring now reflect "shim is gone; this guard prevents re-creation" instead of "shim exists; don't route through it."Documentation updates
src/channels/web/CLAUDE.md: deleted theserver.rsFile Map row, updated thetest_helpers.rsrow to list all sevenpub(crate)fixtures, fixed every prose reference that pointed atserver.rs, and updated the "Adding a New API Endpoint" recipe to point atfeatures/<slice>/andplatform/router.rs.src/channels/web/platform/state.rs: module docstring now says "shim was removed" instead of "shim exists pending migration."src/bridge/CLAUDE.md:pending_gate_extension_namereference now points atfeatures/chat/mod.rs.Quality gate
cargo fmt --allcargo clippy --all --benches --tests --examples --all-features— zero warningscargo check -p ironclaw --no-default-features --features libsql --tests— cleancargo test -p ironclaw --lib channels::web— 434 passed (up from 431 before this PR — three tests that were filtered underchannels::web::server::testsnow surface under their proper slice module path)cargo test -p ironclaw --test multi_tenant_integration— 40 passedcargo test -p ironclaw --test openai_compat_integration— 16 passedcargo test -p ironclaw --test ws_gateway_integration— 11 passedpython3 scripts/check_gateway_boundaries.py— clean, empty allowlist preservedpython3 scripts/check_gateway_boundaries.py test— 16/16bash scripts/pre-commit-safety.sh— cleanRegression coverage
Pure relocation + mechanical rename; no behavior change. The existing ~60 tests from
server.rs::testscontinue to pass unmodified under their new module paths — that's the regression evidence. A "test that would have caught this" would necessarily duplicate the existing tests; no new test adds coverage. Commit carries[skip-regression-check].Migration progress (ironclaw#2599)
server.rsdeletion + test relocation🤖 Generated with Claude Code