Missing tests: SIGHUP scaffolding, worker-orchestrator contract parity, and terminal job-state characterisation

[coderabbitai]

Missing tests (distinct from structural resistance)

• SIGHUP integration scaffolding: tests/sighup_reload_integration.rs is mostly ignored placeholder scaffolding rather than executable regression coverage. It should be replaced with real integration tests that exercise the hot-reload path end-to-end.

• Worker–orchestrator contract parity: There is no obvious shared contract test proving that worker and orchestrator endpoint parity is maintained. A contract test suite (or at minimum a shared protocol module with accompanying tests) should be introduced to catch route/auth-convention drift at compile or test time rather than at runtime.

• Terminal job-state persistence characterisation: Characterisation tests for terminal job-state persistence (covering the persist_status, log_event, and terminal-transition paths in src/worker/job.rs) should be written and passing before any refactoring of the lifecycle path is attempted, to prevent silent regressions.

---

Raised by @leynos. Backlink: #7

[traycerai]

Warning

Insufficient credits. Please purchase more credits or upgrade your plan to continue using Traycer.

[leynos]

@CodeRabbit plan

[coderabbitai]

Coding Plan

Summary

• Replace SIGHUP placeholders with integration tests that exercise WebhookServer and HttpChannelState directly, avoiding flaky process-spawn mechanics.
• Introduce a contract test module verifying route, method, and type alignment between worker client and orchestrator router using parameterised tests and tower::ServiceExt.
• Extend inline worker tests with a capturing mock database to characterise the exact side-effect sequence of terminal state transitions.

Design Choices

Design Choice 1: SIGHUP testing approach — full binary spawn vs. component integration

Options Considered:

1. Spawn a live binary, send SIGHUP via nix::sys::signal, and assert HTTP behaviour changes.
2. Test the composable seams (WebhookServer, HttpChannelState, config reload) in integration without a full binary, relying on existing unit tests for signal receipt.

Chosen Option: 2

Rationale: The signal-receipt path is trivial (tokio::signal::unix); the valuable coverage is in the reload orchestration, which can be exercised via direct API calls without the flakiness of process management.

Design Choice 2: Contract test granularity — exhaustive round-trip vs. structural alignment

Options Considered:

1. Full round-trip tests for every endpoint (client → real axum router → handler → response deserialisation).
2. Structural alignment tests verifying route constants, HTTP methods, and type serialisation symmetry, complementing existing handler tests.

Chosen Option: 2

Rationale: The handler logic is already covered by src/orchestrator/api/tests/. Contract tests focus on the wire-level agreement (paths, methods, shapes) to catch drift without duplicating handler coverage.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: SIGHUP Hot-Reload Integration Tests

Replace the placeholder scaffolding in tests/infrastructure/sighup_reload.rs with executable integration tests that exercise the reload orchestration without spawning a live binary.

Task 1: Remove placeholder tests and establish real test structure

Gut the three #[ignore] placeholder tests. Retain the #![cfg(unix)] gate and the module's position in tests/infrastructure.rs.

• Delete the body of test_sighup_config_reload_address_change, test_sighup_secret_update_zero_downtime, and test_sighup_rollback_on_address_bind_failure.
• Add imports for WebhookServer, HttpChannelState, SecretString, and reqwest::Client.

Task 2: Implement address-change reload test

Verify that WebhookServer::restart_with_addr correctly rebinds the listener and the old port becomes unreachable.

• Build a minimal axum::Router returning StatusCode::OK.
• Start WebhookServer on a random port via TcpListener::bind("127.0.0.1:0").
• Call restart_with_addr with a second random port.
• Assert the new port responds and the old port connection-refuses.
• This mirrors the existing unit test test_restart_with_addr_rebinds_listener but at the integration level with HTTP assertions.

Task 3: Implement zero-downtime secret swap test

Confirm that HttpChannelState::update_secret atomically swaps the webhook secret without listener restart.

• Construct HttpChannel and extract shared_state().
• Mount HttpChannel::routes() in a WebhookServer.
• Send a webhook request with the old secret; assert 200.
• Call state.update_secret(Some(new_secret)).
• Send a webhook request with the old secret; assert 401.
• Send a webhook request with the new secret; assert 200.

Task 4: Implement rollback-on-bind-failure test

Ensure that if binding a new address fails, the original listener remains active.

• Start WebhookServer on a random port.
• Attempt restart_with_addr targeting a privileged port (e.g. 127.0.0.1:1).
• Assert the call returns Err.
• Confirm the original port still responds.

🤖 Prompt for AI agents

Replace placeholder SIGHUP tests in `tests/infrastructure/sighup_reload.rs` with
real, executable integration tests that drive `WebhookServer` and
`HttpChannelState` directly — no live binary spawning.

- Retain the `#![cfg(unix)]` gate and the module's registration in
`tests/infrastructure.rs`.
- Remove the bodies of all three `#[ignore]` placeholder tests:
`test_sighup_config_reload_address_change`,
`test_sighup_secret_update_zero_downtime`, and
`test_sighup_rollback_on_address_bind_failure`.
- Add imports for `WebhookServer`, `HttpChannelState`, `SecretString`, and
`reqwest::Client`.

**Test 1 — Address-change reload (`test_sighup_config_reload_address_change`):**
- Build a minimal `axum::Router` returning `StatusCode::OK`.
- Start `WebhookServer` on a random port via `TcpListener::bind("127.0.0.1:0")`.
- Call `restart_with_addr` with a second random port.
- Assert the new port responds with a successful status and the old port
connection-refuses.

**Test 2 — Zero-downtime secret swap
(`test_sighup_secret_update_zero_downtime`):**
- Construct `HttpChannel`, extract `shared_state()`.
- Mount `HttpChannel::routes()` in a `WebhookServer`.
- Send a webhook request with the old secret; assert 200.
- Call `state.update_secret(Some(new_secret))`.
- Send a webhook request with the old secret; assert 401.
- Send a webhook request with the new secret; assert 200.

**Test 3 — Rollback on bind failure
(`test_sighup_rollback_on_address_bind_failure`):**
- Start `WebhookServer` on a random port.
- Attempt `restart_with_addr` targeting a privileged port (e.g. `127.0.0.1:1`).
- Assert the call returns `Err`.
- Confirm the original port still responds.

Phase 2: Worker–Orchestrator Contract Parity Tests

Introduce a dedicated contract test module that verifies route paths, HTTP methods, and request/response shapes are symmetric between WorkerHttpClient and OrchestratorApi.

Task 1: Create contract test module

Add tests/worker_orchestrator_contract.rs as a new integration test file.

• Import WorkerHttpClient, OrchestratorApi, shared types from src/worker/api/types.rs, and tower::ServiceExt.
• Use rstest for parameterised route checks.

Task 2: Verify route-path alignment

Parameterise over all client methods and assert the URL constructed matches the axum route.

• For each WorkerHttpClient method, call url(path) (or equivalent) and compare against the literal route string registered in OrchestratorApi::router.
• Include remote-tool routes using the shared REMOTE_TOOL_CATALOG_ROUTE and REMOTE_TOOL_EXECUTE_ROUTE constants.

Task 3: Verify HTTP method correctness

Assert that each endpoint uses the expected verb (GET vs POST).

• Build a table: (method_name, expected_http_method, route_path).
• For each entry, construct a request via tower::ServiceExt::oneshot with the wrong method and assert 405 Method Not Allowed.
• Confirm the correct method returns a non-405 status (auth failure is acceptable; the goal is to prove the method matches).

Task 4: Verify request/response JSON shape symmetry

Round-trip serialise and deserialise the shared types used on the wire.

• For ProxyCompletionRequest, ProxyToolCompletionRequest, StatusUpdate, JobEventPayload, RemoteToolExecutionRequest, CompletionReport: serialise to JSON, deserialise back, assert equality.
• For ProxyCompletionResponse, JobDescription, RemoteToolCatalogResponse, CredentialResponse, PromptResponse: deserialise from a known-good JSON fixture, re-serialise, assert byte-level equality.
• Include ProxyFinishReason alias coverage ("tool_calls" → ToolUse, unknown → Unknown).

Task 5: Verify auth-header convention

Confirm that the middleware rejects missing or wrong tokens consistently.

• Send a request with no Authorization header; assert 401.
• Send a request with Authorization: Bearer wrong; assert 401.
• Send a request with a valid token for a different job ID; assert 401.
• This consolidates the existing auth tests into the contract module for discoverability.

🤖 Prompt for AI agents

Create a new integration test file at `tests/worker_orchestrator_contract.rs`
that verifies route paths, HTTP methods, and JSON type shapes are symmetric
between `WorkerHttpClient` and `OrchestratorApi`.

- Import `WorkerHttpClient`, `OrchestratorApi`, shared types from
`src/worker/api/types.rs`, and `tower::ServiceExt`.
- Use `rstest` for parameterised route checks.

**Route-path alignment:**
- Parameterise over all `WorkerHttpClient` methods; call `url(path)` (or
equivalent) and compare against the literal route string registered in
`OrchestratorApi::router`.
- Include remote-tool routes using the shared `REMOTE_TOOL_CATALOG_ROUTE` and
`REMOTE_TOOL_EXECUTE_ROUTE` constants.

**HTTP method correctness:**
- Build a table of `(method_name, expected_http_method, route_path)`.
- For each entry, send a request via `tower::ServiceExt::oneshot` with the
**wrong** HTTP method and assert 405 Method Not Allowed.
- Confirm the correct method returns a non-405 status (auth failure is
acceptable).

**JSON shape symmetry:**
- For `ProxyCompletionRequest`, `ProxyToolCompletionRequest`, `StatusUpdate`,
`JobEventPayload`, `RemoteToolExecutionRequest`, `CompletionReport`: serialise
to JSON, deserialise back, assert equality.
- For `ProxyCompletionResponse`, `JobDescription`, `RemoteToolCatalogResponse`,
`CredentialResponse`, `PromptResponse`: deserialise from a known-good JSON
fixture, re-serialise, assert byte-level equality.
- Cover `ProxyFinishReason` aliases: `"tool_calls"` → `ToolUse`, unknown →
`Unknown`.

**Auth-header convention:**
- Send a request with no `Authorization` header; assert 401.
- Send a request with `Authorization: Bearer wrong`; assert 401.
- Send a request with a valid token for a different job ID; assert 401.

Phase 3: Terminal Job-State Characterisation Tests

Extend the inline test suite in src/worker/job.rs with characterisation tests that capture the exact sequence of side-effects for mark_completed, mark_failed, and mark_stuck.

Task 1: Introduce a capturing mock database

Create a CapturingStore struct implementing the Database trait that records calls to update_job_status and save_job_event.

• Store captured calls in Arc<Mutex<Vec<CapturedCall>>>.
• Implement all required Database methods; only update_job_status and save_job_event record; others return defaults.
• Place in the existing #[cfg(test)] mod tests block in src/worker/job.rs.

Task 2: Add characterisation test for `mark_completed`

Verify the full side-effect sequence.

• Build a Worker with the capturing store.
• Transition to InProgress via context_manager().update_context(...).
• Call mark_completed().
• Assert:
  • ContextManager shows JobState::Completed.
  • CapturingStore received save_job_event with event_type = "result" and status = "completed".
  • CapturingStore received update_job_status with JobState::Completed.
• Await briefly to let fire-and-forget tasks complete.

Task 3: Add characterisation test for `mark_failed`

Same setup as mark_completed; call mark_failed("budget exceeded").

• Assert state is Failed, captured event is "result" with status = "failed", and status update is Failed.

Task 4: Add characterisation test for `mark_stuck`

Same setup as mark_completed; call mark_stuck("timeout").

• Assert state is Stuck, captured event is "result" with status = "stuck", and status update is Stuck.

Task 5: Add double-transition rejection test

Verify that calling a terminal marker twice returns an error and does not duplicate side-effects.

• Call mark_completed() twice.
• Assert the second call returns Err.
• Assert the capturing store received exactly one save_job_event and one update_job_status.

🤖 Prompt for AI agents

Extend the `#[cfg(test)] mod tests` block in `src/worker/job.rs` with a
capturing mock database and characterisation tests for all three terminal state
transitions.

**`CapturingStore` struct:**
- Implement the `Database` trait.
- Store captured calls in `Arc<Mutex<Vec<CapturedCall>>>`.
- Only `update_job_status` and `save_job_event` record calls; all other required
methods return defaults.

**Characterisation test for `mark_completed`:**
- Build a `Worker` with the capturing store.
- Transition to `InProgress` via `context_manager().update_context(...)`.
- Call `mark_completed()`.
- Assert `ContextManager` shows `JobState::Completed`.
- Assert `CapturingStore` received `save_job_event` with `event_type = "result"`
and `status = "completed"`.
- Assert `CapturingStore` received `update_job_status` with
`JobState::Completed`.
- Await briefly to let fire-and-forget tasks complete.

**Characterisation test for `mark_failed`:**
- Same setup; call `mark_failed("budget exceeded")`.
- Assert state is `Failed`, captured event is `"result"` with `status =
"failed"`, and status update is `Failed`.

**Characterisation test for `mark_stuck`:**
- Same setup; call `mark_stuck("timeout")`.
- Assert state is `Stuck`, captured event is `"result"` with `status = "stuck"`,
and status update is `Stuck`.

**Double-transition rejection test:**
- Call `mark_completed()` twice.
- Assert the second call returns `Err`.
- Assert the capturing store received exactly one `save_job_event` and one
`update_job_status`.

Research

The project uses Tokio-based async tests with hand-rolled trait stubs (no mockall). Integration tests live under tests/ with shared support modules in tests/support/. The SIGHUP handler resides in src/main.rs (Unix-only) and drives WebhookServer::restart_with_addr and HttpChannelState::update_secret, both of which already have unit-level tests. Worker–orchestrator communication is HTTP/JSON with shared types in src/worker/api/types.rs and route constants shared between both sides. The src/worker/job.rs module contains mark_completed, mark_failed, and mark_stuck methods with existing inline tests using store: None to bypass DB I/O.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Replace placeholder SIGHUP tests in `tests/infrastructure/sighup_reload.rs` with
real, executable integration tests that drive `WebhookServer` and
`HttpChannelState` directly — no live binary spawning.

- Retain the `#![cfg(unix)]` gate and the module's registration in
`tests/infrastructure.rs`.
- Remove the bodies of all three `#[ignore]` placeholder tests:
`test_sighup_config_reload_address_change`,
`test_sighup_secret_update_zero_downtime`, and
`test_sighup_rollback_on_address_bind_failure`.
- Add imports for `WebhookServer`, `HttpChannelState`, `SecretString`, and
`reqwest::Client`.

**Test 1 — Address-change reload (`test_sighup_config_reload_address_change`):**
- Build a minimal `axum::Router` returning `StatusCode::OK`.
- Start `WebhookServer` on a random port via `TcpListener::bind("127.0.0.1:0")`.
- Call `restart_with_addr` with a second random port.
- Assert the new port responds with a successful status and the old port
connection-refuses.

**Test 2 — Zero-downtime secret swap
(`test_sighup_secret_update_zero_downtime`):**
- Construct `HttpChannel`, extract `shared_state()`.
- Mount `HttpChannel::routes()` in a `WebhookServer`.
- Send a webhook request with the old secret; assert 200.
- Call `state.update_secret(Some(new_secret))`.
- Send a webhook request with the old secret; assert 401.
- Send a webhook request with the new secret; assert 200.

**Test 3 — Rollback on bind failure
(`test_sighup_rollback_on_address_bind_failure`):**
- Start `WebhookServer` on a random port.
- Attempt `restart_with_addr` targeting a privileged port (e.g. `127.0.0.1:1`).
- Assert the call returns `Err`.
- Confirm the original port still responds.
===============================================================================

Task: 2

Create a new integration test file at `tests/worker_orchestrator_contract.rs`
that verifies route paths, HTTP methods, and JSON type shapes are symmetric
between `WorkerHttpClient` and `OrchestratorApi`.

- Import `WorkerHttpClient`, `OrchestratorApi`, shared types from
`src/worker/api/types.rs`, and `tower::ServiceExt`.
- Use `rstest` for parameterised route checks.

**Route-path alignment:**
- Parameterise over all `WorkerHttpClient` methods; call `url(path)` (or
equivalent) and compare against the literal route string registered in
`OrchestratorApi::router`.
- Include remote-tool routes using the shared `REMOTE_TOOL_CATALOG_ROUTE` and
`REMOTE_TOOL_EXECUTE_ROUTE` constants.

**HTTP method correctness:**
- Build a table of `(method_name, expected_http_method, route_path)`.
- For each entry, send a request via `tower::ServiceExt::oneshot` with the
**wrong** HTTP method and assert 405 Method Not Allowed.
- Confirm the correct method returns a non-405 status (auth failure is
acceptable).

**JSON shape symmetry:**
- For `ProxyCompletionRequest`, `ProxyToolCompletionRequest`, `StatusUpdate`,
`JobEventPayload`, `RemoteToolExecutionRequest`, `CompletionReport`: serialise
to JSON, deserialise back, assert equality.
- For `ProxyCompletionResponse`, `JobDescription`, `RemoteToolCatalogResponse`,
`CredentialResponse`, `PromptResponse`: deserialise from a known-good JSON
fixture, re-serialise, assert byte-level equality.
- Cover `ProxyFinishReason` aliases: `"tool_calls"` → `ToolUse`, unknown →
`Unknown`.

**Auth-header convention:**
- Send a request with no `Authorization` header; assert 401.
- Send a request with `Authorization: Bearer wrong`; assert 401.
- Send a request with a valid token for a different job ID; assert 401.
===============================================================================

Task: 3

Extend the `#[cfg(test)] mod tests` block in `src/worker/job.rs` with a
capturing mock database and characterisation tests for all three terminal state
transitions.

**`CapturingStore` struct:**
- Implement the `Database` trait.
- Store captured calls in `Arc<Mutex<Vec<CapturedCall>>>`.
- Only `update_job_status` and `save_job_event` record calls; all other required
methods return defaults.

**Characterisation test for `mark_completed`:**
- Build a `Worker` with the capturing store.
- Transition to `InProgress` via `context_manager().update_context(...)`.
- Call `mark_completed()`.
- Assert `ContextManager` shows `JobState::Completed`.
- Assert `CapturingStore` received `save_job_event` with `event_type = "result"`
and `status = "completed"`.
- Assert `CapturingStore` received `update_job_status` with
`JobState::Completed`.
- Await briefly to let fire-and-forget tasks complete.

**Characterisation test for `mark_failed`:**
- Same setup; call `mark_failed("budget exceeded")`.
- Assert state is `Failed`, captured event is `"result"` with `status =
"failed"`, and status update is `Failed`.

**Characterisation test for `mark_stuck`:**
- Same setup; call `mark_stuck("timeout")`.
- Assert state is `Stuck`, captured event is `"result"` with `status = "stuck"`,
and status update is `Stuck`.

**Double-transition rejection test:**
- Call `mark_completed()` twice.
- Assert the second call returns `Err`.
- Assert the capturing store received exactly one `save_job_event` and one
`update_job_status`.

💡 Iterate on the plan with: @coderabbitai <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!