Skip to content

Add more tests for challenge-orchestrator#11

Merged
echobt merged 16 commits intoPlatformNetwork:mainfrom
cuteolaf:test/challenge-orchestrator
Jan 7, 2026
Merged

Add more tests for challenge-orchestrator#11
echobt merged 16 commits intoPlatformNetwork:mainfrom
cuteolaf:test/challenge-orchestrator

Conversation

@cuteolaf
Copy link
Contributor

@cuteolaf cuteolaf commented Jan 7, 2026

Summary by CodeRabbit

  • Tests

    • Significantly expanded unit and integration tests covering backend, container integration, evaluator HTTP handling, health monitoring, config serialization, and lifecycle flows.
  • New Features

    • Backend selection supports Development, Secure, and Fallback modes for more flexible deployment and resilience.
  • Bug Fixes & Improvements

    • Improved robustness with broker/Docker fallback behaviors, more informative error messages, longer HTTP timeouts, and better container lifecycle handling.
  • Refactor

    • Decoupled container and broker integrations to allow implementation injection and easier testing.
  • Documentation

    • Clarified config documentation for durations and registry semantics.

✏️ Tip: You can customize this high-level summary in your review settings.

- introduce ChallengeDocker trait plus mock-driven coverage for lifecycle sync/restart logic
- add broker socket override, backend mode enum, and unit tests to cover selection paths
- introduce a `DockerBridge` trait plus a `BollardBridge` adapter so
  `DockerClient` depends on an abstract Docker backend
- thread the bridge through all client methods (pull, volumes, logging,
  cleanup, network detection) and adjust API calls accordingly
- provide a `with_bridge` constructor for injecting mocks and use it in
  new `RecordingBridge`-based unit tests covering network setup and stale
  container cleanup
- serialize env-sensitive tests with `serial` to avoid `HOSTNAME` races
  and ensure helper structs use owned `String` filters for Bollard APIs
- introduce SecureContainerBridge/SecureClientBridge so SecureBackend can be mocked and add a with_bridge constructor used by the new tests
- rework DirectDockerBackend to hold Arc<dyn ChallengeDocker>, expose with_docker, and add a test override hook; extend ChallengeDocker with running/log/list helpers and update mocks
- add backend coverage: secure backend create/stop/remove/log/list, direct backend delegation/cleanup, and backend creation fallback scenarios
@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

📝 Walkthrough

Walkthrough

Introduces trait-based abstractions for Docker and secure broker-backed containers, adds backend selection via BackendMode with broker socket override support, replaces concrete clients with trait objects for DI/testability, adds extensive tests, and adds dev-dep serial_test = "3.2".

Changes

Cohort / File(s) Summary
Core Docker Bridge
crates/challenge-orchestrator/src/docker.rs
Adds DockerBridge trait and BollardBridge; DockerClient now holds Arc<dyn DockerBridge>; adds ChallengeDocker trait and from_bridge/with_bridge constructors; refactors Docker usage to bridge; adds RecordingBridge test double and related tests.
Secure Backend & Selection
crates/challenge-orchestrator/src/backend.rs
Adds SecureContainerBridge trait and SecureClientBridge; SecureBackend now uses Arc<dyn SecureContainerBridge> with with_bridge(); introduces BackendMode and select_backend_mode(); adds broker socket default/override, Docker fallback path, and extensive backend test scaffolding.
Orchestrator & DI
crates/challenge-orchestrator/src/lib.rs
Exports ChallengeDocker; ChallengeOrchestrator stores Arc<dyn ChallengeDocker>; adds with_docker() and bootstrap_with_docker() constructors and test injection helpers to swap Docker implementations.
Lifecycle Manager
crates/challenge-orchestrator/src/lifecycle.rs
Replaces concrete DockerClient with Box<dyn ChallengeDocker>; LifecycleManager::new accepts impl ChallengeDocker + 'static; adds MockDocker and expanded lifecycle tests.
Evaluator HTTP Client
crates/challenge-orchestrator/src/evaluator.rs
ChallengeEvaluator now contains a private reqwest::Client (3600s timeout); constructor updated; large test suite added for evaluate/proxy/list/info behaviors and network scenarios.
Health Monitoring Tests
crates/challenge-orchestrator/src/health.rs
Adds numerous async tests and helpers (spawn servers, closing servers) covering HealthMonitor transitions, parse/error cases, and percentage logic; no public API changes.
Config Tests & Docs
crates/challenge-orchestrator/src/config.rs
Expands documentation and adds tests for humantime serde, defaults, negative/large durations, and registry config round-trip serialization.
Manifest / CI
crates/challenge-orchestrator/Cargo.toml, .github/workflows/ci.yml
Adds dev-dependency serial_test = "3.2"; CI coverage step split to produce JSON then separate HTML report generation.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Caller as Caller
    participant Creator as create_backend()
    participant Selector as select_backend_mode()
    participant Broker as BrokerSocket / SecureClientBridge
    participant Secure as SecureBackend
    participant Docker as DirectDockerBackend

    Caller->>Creator: create_backend(config)
    activate Creator
    Creator->>Selector: select_backend_mode(env, broker socket)
    Selector-->>Creator: BackendMode (Development / Secure / Fallback)

    alt Development
        Creator->>Docker: with_docker()/connect()
        Docker-->>Creator: DirectDockerBackend instance
    else Secure
        Creator->>Broker: attempt connect to broker socket
        alt Broker available
            Broker-->>Creator: Bridge ready
            Creator->>Secure: SecureBackend::with_bridge(Bridge)
            Secure-->>Creator: SecureBackend instance
        else Broker unavailable
            Creator->>Docker: create_docker_fallback_backend()
            Docker-->>Creator: DirectDockerBackend instance
        end
    else Fallback
        Creator->>Docker: create_docker_fallback_backend()
        Docker-->>Creator: DirectDockerBackend instance
    end

    Creator-->>Caller: backend instance
    deactivate Creator
    note right of Creator: (env override, fallback guidance in errors)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

"🐰 I hopped through code with glee,
Bridges stitched between Docker and me,
Backends choose paths, tests bloom in rows,
Socket whispers guide where the rabbit goes—🥕"

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 61.87% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding comprehensive tests across multiple modules (backend, config, docker, evaluator, health, lib, lifecycle) in the challenge-orchestrator crate.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @crates/challenge-orchestrator/Cargo.toml:
- Line 44: Update the serial_test dependency from version 2 to 3.2.0 in
Cargo.toml; change the dependency line `serial_test = "2"` to `serial_test =
"3.2"` (or `serial_test = "3"` per policy), then run `cargo update -p
serial_test` and rebuild to ensure compatibility with uses in the
`crates/challenge-orchestrator/src/backend.rs` and
`crates/challenge-orchestrator/src/docker.rs` modules (adjust any API changes if
tests referencing the `serial_test` macros need minor fixes).
🧹 Nitpick comments (2)
crates/challenge-orchestrator/src/evaluator.rs (1)

282-291: Extract duplicated sample_instance helper to shared test utilities.

The sample_instance function is duplicated identically in crates/challenge-orchestrator/src/health.rs at lines 211-220. Consider extracting this to a shared test utilities module to avoid duplication.

♻️ Suggested approach

Create a test utilities module that both files can import:

  1. Add a new file crates/challenge-orchestrator/src/test_utils.rs:
#[cfg(test)]
pub(crate) mod test_fixtures {
    use crate::{ChallengeInstance, ContainerStatus};
    use platform_core::ChallengeId;

    pub fn sample_instance(status: ContainerStatus) -> ChallengeInstance {
        ChallengeInstance {
            challenge_id: ChallengeId::new(),
            container_id: "cid".into(),
            image: "ghcr.io/platformnetwork/example:latest".into(),
            endpoint: "http://127.0.0.1:9000".into(),
            started_at: chrono::Utc::now(),
            status,
        }
    }
}
  1. In both evaluator.rs and health.rs test modules:
#[cfg(test)]
mod tests {
    use super::*;
    use crate::test_utils::test_fixtures::sample_instance;
    // ... rest of imports
}
crates/challenge-orchestrator/src/lib.rs (1)

795-823: Consider adding #[serial] to prevent test flakiness.

This test manipulates the HOSTNAME environment variable which could interfere with other parallel tests. The serial_test crate is already a dev dependency (used in backend.rs).

♻️ Suggested fix
+    use serial_test::serial;
+
     #[tokio::test]
+    #[serial]
     async fn test_new_uses_injected_docker_client() {
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 299d25a and ed67e29.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (8)
  • crates/challenge-orchestrator/Cargo.toml
  • crates/challenge-orchestrator/src/backend.rs
  • crates/challenge-orchestrator/src/config.rs
  • crates/challenge-orchestrator/src/docker.rs
  • crates/challenge-orchestrator/src/evaluator.rs
  • crates/challenge-orchestrator/src/health.rs
  • crates/challenge-orchestrator/src/lib.rs
  • crates/challenge-orchestrator/src/lifecycle.rs
🧰 Additional context used
🧬 Code graph analysis (2)
crates/challenge-orchestrator/src/evaluator.rs (1)
crates/challenge-orchestrator/src/health.rs (2)
  • sample_instance (211-220)
  • new (20-34)
crates/challenge-orchestrator/src/lib.rs (1)
crates/challenge-orchestrator/src/docker.rs (30)
  • pull_image (189-189)
  • pull_image (209-211)
  • pull_image (592-630)
  • start_challenge (190-193)
  • start_challenge (213-218)
  • start_challenge (633-988)
  • stop_container (65-69)
  • stop_container (153-159)
  • stop_container (194-194)
  • stop_container (220-222)
  • stop_container (991-1017)
  • stop_container (1427-1433)
  • remove_container (70-74)
  • remove_container (161-167)
  • remove_container (195-195)
  • remove_container (224-226)
  • remove_container (1020-1043)
  • remove_container (1435-1442)
  • is_container_running (196-196)
  • is_container_running (228-230)
  • is_container_running (1046-1057)
  • get_logs (197-197)
  • get_logs (232-234)
  • get_logs (1081-1104)
  • list_challenge_containers (198-198)
  • list_challenge_containers (236-238)
  • list_challenge_containers (1060-1078)
  • cleanup_stale_containers (199-204)
  • cleanup_stale_containers (240-248)
  • cleanup_stale_containers (1117-1199)
🔇 Additional comments (37)
crates/challenge-orchestrator/src/config.rs (1)

75-138: Comprehensive test coverage for config serialization.

The test suite thoroughly validates:

  • Default configuration values
  • Duration serialization/deserialization round-trips
  • Edge case handling (negative values rejection, large durations)

The tests are well-structured and provide good confidence in the config module's correctness.

crates/challenge-orchestrator/src/evaluator.rs (1)

306-638: Excellent test coverage for the evaluator module.

The test suite comprehensively validates:

  • State gating (Running vs Starting/Stopped)
  • Error handling (missing challenges, network failures, parse errors)
  • Success paths for evaluate_generic, proxy_request, get_info, and check_health
  • Edge cases with lightweight HTTP server mocks

The test infrastructure is well-designed and reusable.

crates/challenge-orchestrator/src/lifecycle.rs (2)

14-25: Well-designed trait-based refactoring for testability.

The refactoring from concrete DockerClient to Box<dyn ChallengeDocker> trait object enables dependency injection and comprehensive testing via MockDocker. The constructor signature impl ChallengeDocker + 'static provides flexibility while maintaining type safety.


259-566: Comprehensive lifecycle tests with clean mock implementation.

The test suite provides excellent coverage:

  • restart_unhealthy only affects unhealthy containers
  • sync handles add/update/remove operations correctly
  • stop_all cleans up all challenges
  • MockDocker implementation cleanly records operations for verification

The tests validate both outcomes and side effects, ensuring the lifecycle manager behaves correctly.

crates/challenge-orchestrator/src/health.rs (2)

250-311: Thorough unit tests for health summary and status tracking.

The tests effectively validate:

  • Edge case handling (zero total containers)
  • Filtering unhealthy challenges
  • Accurate status counting across multiple container states

313-568: Comprehensive async health monitoring tests.

The test suite excellently covers:

  • Status transitions (Starting → Running, Running → Unhealthy)
  • Network failure handling via spawn_closing_health_server
  • Parse error tolerance (treating 200 OK with invalid JSON as healthy per line 167 logic)
  • Continuous monitoring via spawn_repeating_health_server with deadline-based polling

The three server helper variants (single-request, repeating, closing) provide flexible test infrastructure for different scenarios.

crates/challenge-orchestrator/src/lib.rs (8)

33-33: LGTM!

Public export of ChallengeDocker correctly expands the API surface to support the new DI model.


44-66: LGTM!

The trait object approach with Arc<dyn ChallengeDocker> cleanly enables dependency injection while maintaining the existing public API through the new() constructor.


84-102: LGTM!

The test injection hooks follow a standard pattern using OnceLock<Mutex<Option<T>>> for thread-safe, single-use test doubles. The take() semantics ensure the injected client is consumed exactly once per test.


104-119: LGTM!

The with_docker constructor is well-designed, accepting any type implementing ChallengeDocker + 'static and properly wrapping it in an Arc for shared ownership.


319-321: LGTM!

The accessor correctly returns a trait object reference by dereferencing the Arc with as_ref().


368-476: LGTM!

The TestDocker mock is well-structured with operation recording for assertions and proper trait implementation. The use of Arc<TestDockerInner> with mutex-protected state enables cloning for access from both test code and orchestrator.


503-793: LGTM!

Comprehensive test coverage for orchestrator operations. The tests verify both the resulting state and the sequence of Docker operations via the recording mock pattern.


825-990: LGTM!

The TestDockerBridge provides a complete mock implementation for testing the DockerClient integration path. Methods not exercised by the current tests appropriately return default/empty values.

crates/challenge-orchestrator/src/backend.rs (12)

35-39: LGTM!

The default_broker_socket_path() helper centralizes socket path resolution with test override support, improving testability.


72-101: LGTM!

The SecureContainerBridge trait cleanly abstracts the broker client interface, enabling dependency injection for testing without a real broker.


103-176: LGTM!

The SecureClientBridge is a clean adapter that delegates to the real SecureContainerClient, allowing the production code to work unchanged while tests can inject mocks.


178-256: LGTM!

The SecureBackend refactoring cleanly introduces DI through the bridge pattern while maintaining backward compatibility via the existing new() constructor.


380-423: LGTM!

The DirectDockerBackend follows the same DI pattern as SecureBackend. Storing Result<DirectDockerBackend> in the test slot enables testing both success and error paths.


527-542: LGTM!

The BackendMode enum and select_backend_mode() function cleanly separate mode selection logic from backend instantiation, improving testability and readability.


556-590: LGTM!

Good use of #[serial] for tests that manipulate environment variables, and the reset_env() helper ensures clean state between tests.


688-787: LGTM!

Thorough tests for SecureBackend operations via the recording bridge, verifying both return values and operation sequences.


789-851: LGTM!

Good coverage of DirectDockerBackend operations including the cleanup_challenge filtering logic.


853-935: LGTM!

The create_backend tests cover all three backend modes (development, secure, fallback) and the error case, using the test hooks effectively.


966-1190: LGTM!

The RecordingSecureBridge is a well-designed test double with configurable responses and operation recording for verification.


1192-1355: LGTM!

The RecordingChallengeDocker provides a complete mock implementation with state management for running status, logs, and container lists.

crates/challenge-orchestrator/src/docker.rs (11)

28-30: LGTM!

Type aliases for the pinned streams improve readability and reduce duplication in the trait signatures.


32-80: LGTM!

The DockerBridge trait provides a comprehensive abstraction over Bollard's Docker client, covering all operations needed by the orchestrator. The stream methods appropriately return pinned boxed streams.


82-179: LGTM!

The BollardBridge cleanly adapts the Bollard Docker client to the DockerBridge trait, handling API differences like discarding response bodies where the trait signature only needs success/failure.


187-249: LGTM!

The ChallengeDocker trait provides a focused, high-level interface for challenge container operations, complementing the low-level DockerBridge. The implementation correctly delegates to DockerClient's inherent methods using qualified paths.


251-310: LGTM!

The constructor hierarchy is well-designed with all paths converging through from_bridge(), enabling both production use via Bollard and test use via mock bridges.


435-468: LGTM!

The ensure_network method properly uses the bridge abstraction with consistent error handling via map_err(anyhow::Error::from).


1217-1246: LGTM!

Good test coverage for the is_image_allowed security function, covering the whitelist, development mode override, and case insensitivity.


1248-1271: LGTM!

Tests for get_validator_suffix properly verify the priority order (VALIDATOR_NAME > container ID from HOSTNAME) and use #[serial] for environment variable safety.


1280-1454: LGTM!

The RecordingBridge provides a complete mock for DockerBridge with state tracking for networks, containers, and connect calls. Unexercised methods appropriately panic to catch unexpected usage.


1456-1526: LGTM!

Good integration tests for ensure_network (create when missing, skip when existing), connect_self_to_network (only when needed), and cleanup_stale_containers (proper filtering by prefix, exclusion patterns, and age).


1529-1554: LGTM!

Adding Clone derive to CleanupResult enables test assertions, and the cleanup_tests module verifies the success() helper logic.

@cuteolaf
Copy link
Contributor Author

cuteolaf commented Jan 7, 2026

@coderabbitai generate docstrings

@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

✅ Actions performed

Initiated docstring generation; will generate only if new commits exist.

@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

Note

Docstrings generation - SUCCESS
Generated docstrings for this pull request at #12

coderabbitai bot added a commit that referenced this pull request Jan 7, 2026
Docstrings generation was requested by @cuteolaf.

* #11 (comment)

The following files were modified:

* `crates/challenge-orchestrator/src/backend.rs`
* `crates/challenge-orchestrator/src/docker.rs`
* `crates/challenge-orchestrator/src/evaluator.rs`
* `crates/challenge-orchestrator/src/health.rs`
* `crates/challenge-orchestrator/src/lib.rs`
* `crates/challenge-orchestrator/src/lifecycle.rs`
@cuteolaf cuteolaf marked this pull request as draft January 7, 2026 14:44
@cuteolaf
Copy link
Contributor Author

cuteolaf commented Jan 7, 2026

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cuteolaf cuteolaf marked this pull request as ready for review January 7, 2026 15:01
@cuteolaf
Copy link
Contributor Author

cuteolaf commented Jan 7, 2026

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/challenge-orchestrator/src/backend.rs (1)

454-468: Substring matching in cleanup_challenge may incorrectly match unrelated containers.

Using container_id.contains(&challenge_id.to_string()) matches any container whose ID contains the challenge ID as a substring. For example, if challenge_id is "abc", this would match containers like "xyzabc123" or "prefix-abcdef" even if they belong to different challenges.

Consider using a more precise matching strategy, such as checking for a known prefix/suffix pattern or using structured container naming.

🐛 Proposed fix: Use prefix or delimiter-based matching
         for container_id in containers {
-            if container_id.contains(&challenge_id.to_string()) {
+            // Match containers named with challenge_id as a prefix segment
+            let pattern = format!("{}-", challenge_id);
+            if container_id.starts_with(&pattern) || container_id.contains(&format!("-{}-", challenge_id)) {
                 let _ = self.docker.stop_container(&container_id).await;
                 if self.docker.remove_container(&container_id).await.is_ok() {
                     removed += 1;
                 }
             }
         }
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Line 74: The coverage step's run command (the line starting with "run: cargo
llvm-cov nextest --workspace --html --output-dir coverage-html ...") only emits
HTML, but the badge step still expects coverage.json; update that run command to
also emit JSON (e.g., add the cargo-llvm-cov flags to produce a JSON report such
as --json and specify an output file like coverage.json or --json --output-file
coverage.json) so coverage.json is generated, and apply the same change to the
other similar coverage command occurrences referenced (lines 92-102) so the
badge generation step can read the JSON.

In @crates/challenge-orchestrator/src/lib.rs:
- Around line 807-835: The test test_new_uses_injected_docker_client modifies
the process-global HOSTNAME env var and can flake when run concurrently; add the
#[serial] attribute from the serial_test crate to the test function (and ensure
the serial macro is imported, e.g. via use serial_test::serial) so the test runs
with mutual exclusion, keeping the rest of the test body unchanged.
🧹 Nitpick comments (3)
crates/challenge-orchestrator/src/evaluator.rs (2)

269-271: Unused Timeout variant.

The EvaluatorError::Timeout variant is defined but never constructed anywhere in this module. The actual timeout errors from reqwest are mapped to NetworkError instead (e.g., line 69). Consider removing it if it's not needed, or use it to wrap timeout-specific errors for better downstream error handling.


285-294: Consider extracting sample_instance to a shared test utilities module.

This helper is duplicated in health.rs (lines 210-219 per the relevant snippets). Extracting it to a shared #[cfg(test)] module would reduce duplication across the crate's test code.

crates/challenge-orchestrator/src/lib.rs (1)

96-114: Document thread-safety requirements for test slot usage.

The static Mutex<Option<DockerClient>> pattern for test injection is effective, but callers must ensure tests using set_test_docker_client run serially. Consider adding a doc comment noting this requirement, or panic if called when the slot is already populated.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ed67e29 and e6e211c.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (8)
  • .github/workflows/ci.yml
  • crates/challenge-orchestrator/Cargo.toml
  • crates/challenge-orchestrator/src/backend.rs
  • crates/challenge-orchestrator/src/config.rs
  • crates/challenge-orchestrator/src/docker.rs
  • crates/challenge-orchestrator/src/evaluator.rs
  • crates/challenge-orchestrator/src/lib.rs
  • crates/challenge-orchestrator/src/lifecycle.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/challenge-orchestrator/Cargo.toml
  • crates/challenge-orchestrator/src/config.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/challenge-orchestrator/src/evaluator.rs (2)
crates/challenge-orchestrator/src/health.rs (2)
  • sample_instance (211-220)
  • new (20-34)
crates/platform-server/src/challenge_proxy.rs (1)
  • new (26-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Docker
🔇 Additional comments (13)
crates/challenge-orchestrator/src/evaluator.rs (1)

595-640: LGTM!

The spawn_static_http_server and spawn_drop_http_server utilities are well-designed for simulating HTTP behavior in tests. They properly handle single-request lifecycle and clean shutdown.

crates/challenge-orchestrator/src/lifecycle.rs (3)

17-35: LGTM!

The refactor to Box<dyn ChallengeDocker> with impl ChallengeDocker + 'static in the constructor cleanly enables dependency injection while maintaining ergonomic usage. This is idiomatic Rust for trait-object-based DI.


265-323: LGTM!

Thorough test coverage for restart_unhealthy that verifies only unhealthy containers are restarted while healthy ones remain untouched. The assertion on recorded operations confirms the expected Docker API calls.


325-396: LGTM!

The sync test comprehensively covers add, update, and remove paths in a single scenario, validating that the lifecycle manager correctly reconciles current state with target state.

crates/challenge-orchestrator/src/lib.rs (1)

515-536: LGTM!

Clean test demonstrating the add_challenge flow and verifying that the challenge instance is properly stored with correct image and container operations recorded.

crates/challenge-orchestrator/src/docker.rs (4)

39-87: LGTM!

The DockerBridge trait provides a clean abstraction over Docker operations. The method signatures closely mirror Bollard's API while using owned types for the return values, enabling straightforward mock implementations.


198-260: LGTM!

The ChallengeDocker trait provides a higher-level abstraction for challenge container management. The implementation for DockerClient delegates to the inherent methods, maintaining separation between the trait contract and the concrete implementation.


1467-1476: LGTM!

Good test coverage for ensure_network verifying that networks are created only when missing.


1519-1537: LGTM!

The cleanup test effectively validates filtering logic: matching prefix, respecting exclusions, and age thresholds. The assertions confirm exactly one container was removed.

crates/challenge-orchestrator/src/backend.rs (4)

72-101: LGTM!

The SecureContainerBridge trait provides a clean abstraction over the broker client, enabling straightforward mocking in tests. The method signatures align well with the SecureContainerClient API.


527-542: LGTM!

The BackendMode enum and select_backend_mode() function provide clear, testable backend selection logic with explicit priority ordering.


688-714: LGTM!

Comprehensive test coverage for SecureBackend::start_challenge verifying that container creation, starting, and endpoint retrieval are properly delegated through the bridge.


853-873: LGTM!

Good integration test for create_backend verifying that DEVELOPMENT_MODE=true correctly routes to DirectDockerBackend with the injected mock.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/challenge-orchestrator/src/lib.rs (1)

180-190: Store full ChallengeContainerConfig to avoid data loss on refresh.

When refreshing a challenge, hardcoded defaults overwrite the original configuration (mechanism_id, emission_weight, timeout_secs, resource limits). This means a refresh will silently change the challenge's behavior if it was configured with non-default values.

💡 Recommended fix

Store the full ChallengeContainerConfig alongside each ChallengeInstance:

 pub struct ChallengeInstance {
     pub challenge_id: ChallengeId,
     pub container_id: String,
     pub image: String,
     pub endpoint: String,
     pub started_at: chrono::DateTime<chrono::Utc>,
     pub status: ContainerStatus,
+    pub config: ChallengeContainerConfig,
 }

Then use instance.config instead of reconstructing with defaults.

🧹 Nitpick comments (1)
crates/challenge-orchestrator/src/lib.rs (1)

97-114: Document test injection mechanism to prevent future race conditions.

The test injection mechanism (static Mutex<Option<DockerClient>>) works correctly because only one test (test_new_uses_injected_docker_client) currently uses it and that test has #[serial]. However, if future tests use set_test_docker_client() without #[serial], they could race and consume each other's injected clients.

📝 Suggested documentation
     #[cfg(test)]
+    /// SAFETY: Tests calling set_test_docker_client() MUST use #[serial]
+    /// to prevent concurrent access to the shared test slot.
     pub(crate) fn set_test_docker_client(docker: DockerClient) {
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e6e211c and 776292f.

📒 Files selected for processing (2)
  • .github/workflows/ci.yml
  • crates/challenge-orchestrator/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/ci.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Docker
🔇 Additional comments (1)
crates/challenge-orchestrator/src/lib.rs (1)

516-837: Excellent test coverage with proper isolation.

The test suite thoroughly validates all lifecycle operations (add, update, remove, refresh, sync, cleanup) with appropriate assertions on both state changes and underlying Docker operations. The mock implementation enables precise verification without real container orchestration.

@echobt echobt merged commit cc4552c into PlatformNetwork:main Jan 7, 2026
6 checks passed
echobt added a commit that referenced this pull request Feb 21, 2026
…enges (#11)

Introduce the new `platform-challenge-sdk-wasm` crate (`crates/challenge-sdk-wasm/`)
targeting `wasm32-unknown-unknown`. This lightweight, `no_std`-compatible SDK
provides everything challenge developers need to build WASM challenges without
pulling in native dependencies (sled, tokio, axum, etc.).

Core components:

- **Challenge trait & register_challenge! macro** (`src/lib.rs`): Defines the
  `Challenge` trait (evaluate, validate, name, version) and a declarative macro
  that generates `extern "C"` WASM ABI export shims (evaluate, validate,
  get_name, get_version) from a user trait impl, handling serialization via
  bincode and memory allocation for returning data to the host.

- **Host function imports** (`src/host_functions.rs`): Declares extern imports
  from `platform_network` (http_get, http_post, dns_resolve) and
  `platform_storage` (storage_get, storage_set) namespaces matching the
  wasm-runtime-interface contract, with safe Rust wrappers around each.

- **Shared types** (`src/types.rs`): Defines `EvaluationInput` and
  `EvaluationOutput` structs with serde Serialize/Deserialize, mirroring the
  native SDK types without native-only dependencies.

- **Bump allocator** (`src/alloc_impl.rs`): Provides a 1 MiB arena bump
  allocator for WASM linear memory, plus an exported `alloc(size) -> ptr`
  function so the host can allocate guest memory for data passing.

- **Build script update** (`scripts/build-wasm.sh`): Extended to accept an
  optional crate name argument for building challenge crates with this SDK,
  while preserving the existing chain-runtime build as the default.

- **Workspace integration** (`Cargo.toml`, `Cargo.lock`): Added
  `crates/challenge-sdk-wasm` to workspace members and updated the lockfile
  with the new crate and its minimal dependencies (serde, bincode).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants