Centralize worker API routes, DI, and error URL handling#69
Merged
Conversation
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSummarise the refactor: centralise shared worker–orchestrator route constants and transport types; make WorkerHttpClient construction fallible and add accessors; extract client HTTP methods; inject the client into WorkerRuntime with config validation; centralise reporting; queue delegate events to a background flusher; update tests and add const_format. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runtime as WorkerRuntime
participant Client as WorkerHttpClient
participant Delegate as ContainerDelegate
participant Orch as Orchestrator
Runtime->>Client: GET `PROMPT_PATH` / GET `CREDENTIALS_PATH`
Client->>Orch: HTTP GET to `*_ROUTE`
Orch-->>Client: 200 / 204 / 4xx / 5xx
Client-->>Runtime: Ok / Parsed / WorkerError
Runtime->>Client: POST `STATUS_PATH` / POST `COMPLETE_PATH`
Client->>Orch: HTTP POST to `*_ROUTE`
Orch-->>Client: 200 / 4xx / 5xx
Client-->>Runtime: Ok or WorkerError
Runtime->>Delegate: post_event(type, data) (enqueue, non-async)
Delegate->>Delegate: background task recv() loop
Delegate->>Client: POST event -> Orch
Orch-->>Client: HTTP response
Client-->>Delegate: Ok or mapped WorkerError (logged)
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
scope: orchestrator
Contributor
Reviewer's GuideCentralizes worker/orchestrator HTTP routing behind shared path/route constants and refactors WorkerRuntime construction to use an injected HTTP client with a separate env-based constructor, updating tests and server wiring accordingly. Sequence diagram for WorkerRuntime environment-based and injected client constructionsequenceDiagram
actor Operator
participant WorkerProcess
participant WorkerRuntime
participant WorkerHttpClient
participant Env as Environment
Operator->>WorkerProcess: run_worker()
WorkerProcess->>WorkerRuntime: from_env(config)
WorkerRuntime->>WorkerHttpClient: from_env(config.orchestrator_url, config.job_id)
WorkerHttpClient->>Env: read IRONCLAW_WORKER_TOKEN
Env-->>WorkerHttpClient: token
WorkerHttpClient-->>WorkerRuntime: Result(Arc~WorkerHttpClient~)
WorkerRuntime-->>WorkerProcess: Result(WorkerRuntime)
WorkerProcess->>WorkerRuntime: run()
WorkerRuntime->>WorkerHttpClient: get_job()
WorkerHttpClient->>WorkerHttpClient: get_json(JOB_PATH, op_name)
WorkerHttpClient-->>WorkerRuntime: JobDescription
rect rgb(230,230,250)
note over WorkerRuntime,WorkerHttpClient: Test path with injected client
Operator->>WorkerProcess: construct_test_runtime(client)
WorkerProcess->>WorkerRuntime: new(config, Arc~WorkerHttpClient~)
WorkerRuntime-->>WorkerProcess: WorkerRuntime
end
Class diagram for WorkerRuntime refactor and shared worker API routesclassDiagram
class WorkerConfig {
+String orchestrator_url
+String job_id
+Duration timeout
}
class WorkerError
class WorkerHttpClient {
-Client client
-String base_url
-String job_id
-String token
+from_env(orchestrator_url, job_id) Result~WorkerHttpClient, WorkerError~
+get_job() JobDescription
+llm_complete(request) CompletionResponse
+llm_complete_with_tools(request) ToolCompletionResponse
+report_status(update) Result~void, WorkerError~
+post_event(payload) void
+poll_prompt() Result~PromptResponse, WorkerError~
+fetch_credentials() Result~CredentialResponse, WorkerError~
+report_complete(report) Result~void, WorkerError~
-url(path) String
-get_json(path, op_name) T
-post_json(path, body, op_name) T
}
class WorkerRuntime {
-Arc~WorkerHttpClient~ client
-Arc~LlmProvider~ llm
-Arc~ToolRegistry~ tools
+new(config, client) WorkerRuntime
+from_env(config) Result~WorkerRuntime, WorkerError~
+run() Result~void, WorkerError~
}
class OrchestratorApi {
+router(state) Router
}
class WorkerApiTypes {
<<module>>
+String JOB_PATH
+String JOB_ROUTE
+String LLM_COMPLETE_PATH
+String LLM_COMPLETE_ROUTE
+String LLM_COMPLETE_WITH_TOOLS_PATH
+String LLM_COMPLETE_WITH_TOOLS_ROUTE
+String STATUS_PATH
+String STATUS_ROUTE
+String COMPLETE_PATH
+String COMPLETE_ROUTE
+String EVENT_PATH
+String EVENT_ROUTE
+String PROMPT_PATH
+String PROMPT_ROUTE
+String CREDENTIALS_PATH
+String CREDENTIALS_ROUTE
+String REMOTE_TOOL_CATALOG_PATH
+String REMOTE_TOOL_CATALOG_ROUTE
+String REMOTE_TOOL_EXECUTE_PATH
+String REMOTE_TOOL_EXECUTE_ROUTE
+String WORKER_HEALTH_ROUTE
}
class WorkerState
class OrchestratorState {
+TokenStore token_store
}
class LlmProvider
class ProxyLlmProvider {
+new(client, provider_id, tools) ProxyLlmProvider
}
LlmProvider <|.. ProxyLlmProvider
class ToolRegistry {
+new() ToolRegistry
+register_container_tools() void
}
WorkerRuntime --> WorkerHttpClient : uses
WorkerRuntime --> LlmProvider : uses
WorkerRuntime --> ToolRegistry : builds
WorkerRuntime ..> WorkerConfig : constructed_with
WorkerRuntime ..> WorkerError
WorkerHttpClient ..> WorkerError
WorkerHttpClient ..> WorkerApiTypes : uses PATH constants
OrchestratorApi ..> OrchestratorState : router_state
OrchestratorApi ..> WorkerApiTypes : uses ROUTE constants
OrchestratorState --> WorkerState
ProxyLlmProvider --> WorkerHttpClient : delegates HTTP
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
Bot
added
size: M
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/worker/api.rs (1)
1-407: 🧹 Nitpick | 🔵 TrivialFile marginally exceeds the 400-line limit.
At 407 lines, this file is just over the limit. Extract the
error_mappingmodule contents inline or move some of the client methods into a separateclient_methods.rssubmodule if the file grows further.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/api.rs` around lines 1 - 407, The file exceeds the 400-line guideline; split out code to reduce size by either inlining the small external module `error_mapping` into this file or moving non-core methods into a new submodule. Concretely: 1) either copy the contents of the `mod error_mapping;` module into this file (below or above WorkerHttpClient) and remove the external mod declaration, or 2) create a new file (e.g. `client_methods.rs`) and move less central impl methods such as `report_status`, `report_status_lossy`, `post_event`, `poll_prompt`, `fetch_credentials`, and `report_complete` into an `impl WorkerHttpClient { ... }` block in that module, then replace them here with a `mod client_methods;` and `use crate::worker::client_methods::*;` (or appropriate module path). Ensure all referenced symbols (WorkerHttpClient, error_mapping::map_remote_tool_status, send_post_json, post_json, get_json, STATUS_PATH, EVENT_PATH, PROMPT_PATH, CREDENTIALS_PATH, COMPLETE_PATH) remain available/imported so compilation and visibility are preserved.src/worker/container.rs (1)
1-438: 🧹 Nitpick | 🔵 TrivialFile exceeds the 400-line limit.
This file is 438 lines. Extract the
WorkerExecutionResultenum and related reporting logic (e.g.,report_completion,report_failure,fail_pre_loop) into a separatereporting.rssubmodule to bring the file under the limit. As per coding guidelines: "Files must not exceed 400 lines in length".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/container.rs` around lines 1 - 438, Extract the WorkerExecutionResult enum plus the reporting methods (report_completion, report_failure, fail_pre_loop and the helper post_event) into a new submodule reporting.rs and update WorkerRuntime to call into that module; specifically, move the enum WorkerExecutionResult and refactor report_completion(&self, execution, iterations), report_failure(&self, iterations, message) and fail_pre_loop<T>(&self, stage, error) out of container.rs into reporting.rs (make them either free async functions or functions on a small Reporting helper that accept &WorkerRuntime or the minimal fields they need: config.job_id, client, and post_event behavior), re-export or use the new reporting functions from container.rs so existing call sites (run -> fail_pre_loop, run -> report_completion, and any other internal calls) keep the same behavior and signatures, and ensure post_event is moved or exposed from reporting.rs so report_* can post events; update use/imports accordingly and run tests to verify no public API changes.src/worker/container/tests.rs (1)
1-730: 🧹 Nitpick | 🔵 TrivialFile exceeds the 400-line limit.
This test file is 730 lines. Break it into smaller, focused test modules (e.g.,
pre_loop_tests.rs,remote_tool_tests.rs,runtime_lifecycle_tests.rs) to improve maintainability. As per coding guidelines: "Files must not exceed 400 lines in length".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/container/tests.rs` around lines 1 - 730, This file is over 400 lines; split tests into smaller modules by moving related tests and shared helpers into focused files: create a test helpers module (e.g., tests/runtime_helpers.rs) containing RuntimeTestState, take_next_status, spawn_runtime_test_server, build_test_runtime, RuntimeTestHarness, expected_* helpers and shared types (so functions like job_handler, credentials_handler, complete_handler, event_handler, prompt_handler live there), then move pre-loop lifecycle tests (worker_runtime_reports_failed_status_for_pre_loop_errors, worker_runtime_emits_failed_status_for_initial_status_rejections) into a pre_loop_tests.rs module, sanitization and completion-report tests (worker_runtime_sanitizes_failure_messages, worker_runtime_build_tools_preserves_container_local_tools) into lifecycle_tests.rs, and all remote-tool/catalog tests (remote_tool_catalog helpers, spawn_hosted_guidance_catalog_server, build_runtime_with_remote_tools, hosted_worker_remote_tool_catalog_registers_remote_tools, worker_runtime_build_reasoning_context_merges_local_and_remote_tools, worker_runtime_refresh_keeps_merged_tools_without_duplicate_guidance, hosted_worker_remote_tool_catalog_degraded_startup_keeps_local_tools) into remote_tool_tests.rs; update each new file to import the shared helpers and symbols (RuntimeTestState, build_test_runtime, expected_remote_tool_definition, WorkerRuntime, WorkerHttpClient, ContainerDelegate, JobDescription, HOSTED_GUIDANCE_HEADING, etc.), add mod declarations in the parent test module so the test suite compiles, and ensure any private items used by multiple files are made pub(crate) in the helpers module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/worker/api.rs`:
- Around line 1-407: The file exceeds the 400-line guideline; split out code to
reduce size by either inlining the small external module `error_mapping` into
this file or moving non-core methods into a new submodule. Concretely: 1) either
copy the contents of the `mod error_mapping;` module into this file (below or
above WorkerHttpClient) and remove the external mod declaration, or 2) create a
new file (e.g. `client_methods.rs`) and move less central impl methods such as
`report_status`, `report_status_lossy`, `post_event`, `poll_prompt`,
`fetch_credentials`, and `report_complete` into an `impl WorkerHttpClient { ...
}` block in that module, then replace them here with a `mod client_methods;` and
`use crate::worker::client_methods::*;` (or appropriate module path). Ensure all
referenced symbols (WorkerHttpClient, error_mapping::map_remote_tool_status,
send_post_json, post_json, get_json, STATUS_PATH, EVENT_PATH, PROMPT_PATH,
CREDENTIALS_PATH, COMPLETE_PATH) remain available/imported so compilation and
visibility are preserved.
In `@src/worker/container.rs`:
- Around line 1-438: Extract the WorkerExecutionResult enum plus the reporting
methods (report_completion, report_failure, fail_pre_loop and the helper
post_event) into a new submodule reporting.rs and update WorkerRuntime to call
into that module; specifically, move the enum WorkerExecutionResult and refactor
report_completion(&self, execution, iterations), report_failure(&self,
iterations, message) and fail_pre_loop<T>(&self, stage, error) out of
container.rs into reporting.rs (make them either free async functions or
functions on a small Reporting helper that accept &WorkerRuntime or the minimal
fields they need: config.job_id, client, and post_event behavior), re-export or
use the new reporting functions from container.rs so existing call sites (run ->
fail_pre_loop, run -> report_completion, and any other internal calls) keep the
same behavior and signatures, and ensure post_event is moved or exposed from
reporting.rs so report_* can post events; update use/imports accordingly and run
tests to verify no public API changes.
In `@src/worker/container/tests.rs`:
- Around line 1-730: This file is over 400 lines; split tests into smaller
modules by moving related tests and shared helpers into focused files: create a
test helpers module (e.g., tests/runtime_helpers.rs) containing
RuntimeTestState, take_next_status, spawn_runtime_test_server,
build_test_runtime, RuntimeTestHarness, expected_* helpers and shared types (so
functions like job_handler, credentials_handler, complete_handler,
event_handler, prompt_handler live there), then move pre-loop lifecycle tests
(worker_runtime_reports_failed_status_for_pre_loop_errors,
worker_runtime_emits_failed_status_for_initial_status_rejections) into a
pre_loop_tests.rs module, sanitization and completion-report tests
(worker_runtime_sanitizes_failure_messages,
worker_runtime_build_tools_preserves_container_local_tools) into
lifecycle_tests.rs, and all remote-tool/catalog tests (remote_tool_catalog
helpers, spawn_hosted_guidance_catalog_server, build_runtime_with_remote_tools,
hosted_worker_remote_tool_catalog_registers_remote_tools,
worker_runtime_build_reasoning_context_merges_local_and_remote_tools,
worker_runtime_refresh_keeps_merged_tools_without_duplicate_guidance,
hosted_worker_remote_tool_catalog_degraded_startup_keeps_local_tools) into
remote_tool_tests.rs; update each new file to import the shared helpers and
symbols (RuntimeTestState, build_test_runtime, expected_remote_tool_definition,
WorkerRuntime, WorkerHttpClient, ContainerDelegate, JobDescription,
HOSTED_GUIDANCE_HEADING, etc.), add mod declarations in the parent test module
so the test suite compiles, and ensure any private items used by multiple files
are made pub(crate) in the helpers module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bdd81663-972b-43ed-8674-f1a92aa4b9ab
📒 Files selected for processing (6)
src/orchestrator/api.rssrc/worker/api.rssrc/worker/api/types.rssrc/worker/container.rssrc/worker/container/tests.rssrc/worker/mod.rs
github-actions
Bot
added
size: XL
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worker/api/client_methods.rs`:
- Around line 115-152: The fetch_credentials function currently treats both 204
and 404 as "no credentials"; change it so only 204
(reqwest::StatusCode::NO_CONTENT) returns Ok(vec![]), and treat 404 as a
non-success error path so the call returns a WorkerError (e.g.
SecretResolveFailed with the existing status-based message). Update the
conditional in fetch_credentials (method name: fetch_credentials, symbol:
CREDENTIALS_PATH) to remove the check for NOT_FOUND so that route drift vs
missing route is surfaced (get_credentials_handler uses 204 for no-grants).
In `@src/worker/api/types.rs`:
- Around line 44-101: Create and export a single helper in the same module that
builds concrete job-scoped paths from the central WORKER_PREFIX (e.g. pub fn
job_scoped_path(job_id: &str, relative: &str) -> String) and use that from
WorkerHttpClient::url instead of hardcoding "/worker/{job_id}/" there; update
WorkerHttpClient::url to call job_scoped_path(job_id, relative_path) so all
consumers use the centralized prefix (refer to WORKER_PREFIX and
WorkerHttpClient::url).
- Around line 351-379: Replace the repetitive asserts in the tests module's
test_route_constants_are_correctly_derived with a parameterized rstest: add a
#[rstest] test function that takes (expected: &str, route: &str) pairs for each
route constant (e.g., JOB_ROUTE, LLM_COMPLETE_ROUTE,
LLM_COMPLETE_WITH_TOOLS_ROUTE, STATUS_ROUTE, COMPLETE_ROUTE, EVENT_ROUTE,
PROMPT_ROUTE, CREDENTIALS_ROUTE, REMOTE_TOOL_CATALOG_ROUTE,
REMOTE_TOOL_EXECUTE_ROUTE, WORKER_HEALTH_ROUTE) and assert_eq!(route, expected)
for each case; update the test signature and attribute to iterate the pairs so
adding a new endpoint requires only adding a new rstest case row.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4dab57fa-9823-48ef-8f87-9967c3ebb13a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlsrc/worker/api.rssrc/worker/api/client_methods.rssrc/worker/api/error_mapping.rssrc/worker/api/types.rs
💤 Files with no reviewable changes (1)
- src/worker/api/error_mapping.rs
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
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)
src/worker/container.rs (1)
90-125: 🛠️ Refactor suggestion | 🟠 MajorValidate the injected client against
WorkerConfig.Do not let
WorkerRuntimecarry two independent copies of the job target.config.job_id/orchestrator_urland the injectedWorkerHttpClientcan diverge here, which means logs refer to one job while HTTP calls hit another. Collapse the transport fields into the client, or assert consistency in the constructor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/container.rs` around lines 90 - 125, The constructor allows config.job_id/orchestrator_url to diverge from the injected WorkerHttpClient; update Self::new to validate consistency by comparing config.job_id and config.orchestrator_url against the values stored in the provided WorkerHttpClient (or return a WorkerError) and fail fast if they differ, or alternatively remove the redundant fields from WorkerConfig so the transport is only represented by WorkerHttpClient; reference the functions/structs WorkerConfig, WorkerHttpClient, Self::new, from_env and WorkerError when making the change.
♻️ Duplicate comments (2)
src/worker/api/types.rs (2)
355-371: 🛠️ Refactor suggestion | 🟠 MajorParameterize the route matrix with
#[rstest].Replace the hand-written assertion list with parameterized cases. Each new endpoint currently needs another bespoke
assert_eq!, which is exactly the duplication the test rules forbid.As per coding guidelines, "Replace duplicated tests with
#[rstest(...)]parameterised cases".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/api/types.rs` around lines 355 - 371, The test test_route_constants_are_correctly_derived contains many duplicated assert_eq! calls; replace them with a parameterized rstest table that iterates pairs of (route_constant, expected_string) so adding new endpoints requires only a new case. Update the test to use #[rstest] (or #[rstest::rstest] depending on imports) with parameters like (route, expected) and a single assert_eq!(route, expected) inside the test body, referencing the existing constants (JOB_ROUTE, LLM_COMPLETE_ROUTE, LLM_COMPLETE_WITH_TOOLS_ROUTE, STATUS_ROUTE, COMPLETE_ROUTE, EVENT_ROUTE, PROMPT_ROUTE, CREDENTIALS_ROUTE, REMOTE_TOOL_CATALOG_ROUTE, REMOTE_TOOL_EXECUTE_ROUTE, WORKER_HEALTH_ROUTE) so the assertions are expressed as parameterized cases rather than duplicated lines.
44-101: 🛠️ Refactor suggestion | 🟠 MajorExport a concrete job-scoped path builder.
Centralize the
/worker/{job_id}/prefix here as well.WORKER_PREFIXstays private and the public*_PATHconstants are only relative paths, so the client still has to re-encode the job-scoped prefix elsewhere when building request URLs. That leaves route drift possible between router registration and outbound calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/api/types.rs` around lines 44 - 101, The router defines a private WORKER_PREFIX and many public relative *_PATH constants which forces callers to duplicate the job-scoped prefix and risks route drift; add and export a single canonical job-scoped path builder (e.g. pub fn worker_scoped(path: &str, job_id: &str) -> String or a pub const/func that returns the full "/worker/{job_id}/<path>") and update usages to call this builder instead of composing WORKER_PREFIX externally—refer to WORKER_PREFIX, REMOTE_TOOL_CATALOG_PATH, REMOTE_TOOL_CATALOG_ROUTE and other *_PATH/*_ROUTE symbols to centralize construction so router registration and clients use the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worker/container/reporting.rs`:
- Around line 39-45: The pre-loop failure call to report_worker_status currently
reports iterations as 100 which is inconsistent with the completion path; update
the call in the failing startup block so
report_worker_status(WorkerState::Failed, Some("pre-loop failure".to_string()),
100) uses 0 instead of 100 to report iterations=0 for never-started jobs (look
for the report_worker_status invocation paired with WorkerState::Failed and the
"pre-loop failure" message).
- Around line 163-167: The current post_event method awaits the orchestrator
post and can block terminal flows; change post_event (and callers
report_completion/report_failure) so terminal completion is authoritative:
either call report_complete before posting the auxiliary event, or fire the
event post without awaiting by spawning a background task with a bounded timeout
(e.g., tokio::spawn + tokio::time::timeout) so slow event endpoints can't delay
report_complete; update post_event to return immediately after scheduling the
bounded-timeout request and ensure errors are logged inside the spawned task
rather than propagated.
---
Outside diff comments:
In `@src/worker/container.rs`:
- Around line 90-125: The constructor allows config.job_id/orchestrator_url to
diverge from the injected WorkerHttpClient; update Self::new to validate
consistency by comparing config.job_id and config.orchestrator_url against the
values stored in the provided WorkerHttpClient (or return a WorkerError) and
fail fast if they differ, or alternatively remove the redundant fields from
WorkerConfig so the transport is only represented by WorkerHttpClient; reference
the functions/structs WorkerConfig, WorkerHttpClient, Self::new, from_env and
WorkerError when making the change.
---
Duplicate comments:
In `@src/worker/api/types.rs`:
- Around line 355-371: The test test_route_constants_are_correctly_derived
contains many duplicated assert_eq! calls; replace them with a parameterized
rstest table that iterates pairs of (route_constant, expected_string) so adding
new endpoints requires only a new case. Update the test to use #[rstest] (or
#[rstest::rstest] depending on imports) with parameters like (route, expected)
and a single assert_eq!(route, expected) inside the test body, referencing the
existing constants (JOB_ROUTE, LLM_COMPLETE_ROUTE,
LLM_COMPLETE_WITH_TOOLS_ROUTE, STATUS_ROUTE, COMPLETE_ROUTE, EVENT_ROUTE,
PROMPT_ROUTE, CREDENTIALS_ROUTE, REMOTE_TOOL_CATALOG_ROUTE,
REMOTE_TOOL_EXECUTE_ROUTE, WORKER_HEALTH_ROUTE) so the assertions are expressed
as parameterized cases rather than duplicated lines.
- Around line 44-101: The router defines a private WORKER_PREFIX and many public
relative *_PATH constants which forces callers to duplicate the job-scoped
prefix and risks route drift; add and export a single canonical job-scoped path
builder (e.g. pub fn worker_scoped(path: &str, job_id: &str) -> String or a pub
const/func that returns the full "/worker/{job_id}/<path>") and update usages to
call this builder instead of composing WORKER_PREFIX externally—refer to
WORKER_PREFIX, REMOTE_TOOL_CATALOG_PATH, REMOTE_TOOL_CATALOG_ROUTE and other
*_PATH/*_ROUTE symbols to centralize construction so router registration and
clients use the same source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f91d931-67cc-4ff5-92ea-170035f35bc7
📒 Files selected for processing (3)
src/worker/api/types.rssrc/worker/container.rssrc/worker/container/reporting.rs
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13511
[resolve] |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/worker/api.rs (1)
78-85: 🛠️ Refactor suggestion | 🟠 MajorRoute all worker URLs through the shared path builder.
Line 84 still hard-codes the concrete
"/worker/{job_id}/"prefix, so the client can drift fromsrc/worker/api/types.rsagain on the next prefix change. Export a concrete job-scoped path helper from the shared protocol module and call it here instead of formatting the prefix by hand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/api.rs` around lines 78 - 85, The url method currently builds worker endpoints by hardcoding the "/worker/{job_id}/" prefix, risking drift from the shared route definitions; replace the string formatting in fn url(&self, path: &str) with a call to the shared protocol's job-scoped path helper (the exported helper from src/worker/api/types.rs or the protocol module) so it constructs the prefix consistently using the same symbol used across the codebase (use the exported helper when composing the final URL together with self.orchestrator_url and self.job_id, leaving pub fn job_id(&self) unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worker/container.rs`:
- Around line 85-107: The current WorkerRuntime::new constructor uses assert_eq!
to panic on mismatched WorkerConfig(job_id/orchestrator_url) vs
WorkerHttpClient, so change it to a fallible constructor (e.g., try_new) that
returns Result<WorkerRuntime, ConfigError> (or Result<Self, WorkerConfigError>)
instead of panicking; perform the same validation checks (comparing
config.job_id to client.job_id() and trimmed config.orchestrator_url to
client.orchestrator_url()) and return a descriptive typed error when they
differ, and update from_env (or any callers) to call the new try_new and
propagate the error.
In `@src/worker/container/reporting.rs`:
- Around line 162-186: The success log in reporting.rs’s post_event is
misleading because WorkerHttpClient::post_event currently returns () even on
failures; change WorkerHttpClient::post_event (in
src/worker/api/client_methods.rs) to return a Result<(), E> (choose an
appropriate error type or Box<dyn Error + Send + Sync>), propagate that Result
through client.post_event(&payload), and update the match in
reporting.rs::post_event to log on Ok(()) only for true success and log detailed
errors on Err(e) (include job_id and ?event_type). Alternatively, if you prefer
not to change the client API, remove the "Posted job event" success trace from
reporting.rs::post_event and only log warnings/errors when the post times out or
fails.
---
Duplicate comments:
In `@src/worker/api.rs`:
- Around line 78-85: The url method currently builds worker endpoints by
hardcoding the "/worker/{job_id}/" prefix, risking drift from the shared route
definitions; replace the string formatting in fn url(&self, path: &str) with a
call to the shared protocol's job-scoped path helper (the exported helper from
src/worker/api/types.rs or the protocol module) so it constructs the prefix
consistently using the same symbol used across the codebase (use the exported
helper when composing the final URL together with self.orchestrator_url and
self.job_id, leaving pub fn job_id(&self) unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8948dd6-a0f3-4906-8c67-0cc7dc6d1ab2
📒 Files selected for processing (5)
src/worker/api.rssrc/worker/api/types.rssrc/worker/container.rssrc/worker/container/reporting.rssrc/worker/container/tests.rs
leynos
force-pushed
the
centralize-worker-api-routes-75ynjm
branch
from
171132d to
33c918d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/worker/api.rs (1)
60-71:⚠️ Potential issue | 🟡 MinorAlign
new()withfrom_env()error handling for consistency.The constructor uses
.unwrap_or_default()when building the HTTP client, which silently substitutes a default client if construction fails. Thefrom_env()constructor (line 42) follows the correct pattern by returningResultand propagating errors via.map_err(). Makenew()consistent by returningResult<Self, WorkerError>.All call sites are in test code (11 usages across test modules and
#[test]functions), so migration is straightforward.Proposed fix – return Result
/// Create with an explicit token (for testing). - pub fn new(orchestrator_url: String, job_id: Uuid, token: String) -> Self { - Self { - client: reqwest::Client::builder() + pub fn new(orchestrator_url: String, job_id: Uuid, token: String) -> Result<Self, WorkerError> { + Ok(Self { + client: reqwest::Client::builder() .timeout(REQUEST_TIMEOUT) .build() - .unwrap_or_default(), + .map_err(|e| WorkerError::ConnectionFailed { + url: orchestrator_url.clone(), + reason: format!("failed to build HTTP client: {}", e), + })?, orchestrator_url: orchestrator_url.trim_end_matches('/').to_string(), job_id, token, - } + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/api.rs` around lines 60 - 71, The constructor new(orchestrator_url: String, job_id: Uuid, token: String) currently uses unwrap_or_default() when building reqwest::Client which hides build failures; change new to return Result<Self, WorkerError> (matching from_env()) and propagate reqwest::Client::builder().build() errors via map_err to create a WorkerError::ConnectionFailed (include orchestrator_url and the underlying error message), keep REQUEST_TIMEOUT and other fields the same, and adjust tests/call sites to handle the Result.
♻️ Duplicate comments (4)
src/worker/container/reporting.rs (2)
41-59:⚠️ Potential issue | 🟡 MinorReport zero iterations for start-up failures.
Emit
0here. This path runs before the loop starts, so100makes a never-started job look fully progressed.🔧 Patch
if let Err(report_error) = self .report_worker_status( WorkerState::Failed, Some("pre-loop failure".to_string()), - 100, + 0, ) .await { tracing::warn!( job_id = %self.config.job_id, stage, error = %report_error, "Failed to emit terminal pre-loop worker status" ); } if let Err(report_error) = self - .report_failure(100, "Worker failed during startup") + .report_failure(0, "Worker failed during startup") .await { tracing::warn!( job_id = %self.config.job_id, stage, error = %report_error, "Failed to emit terminal pre-loop completion" ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/container/reporting.rs` around lines 41 - 59, The progress percentage used for pre-loop startup failures is incorrect: change the hardcoded 100 to 0 in the calls to report_worker_status (with WorkerState::Failed and message "pre-loop failure") and report_failure (with message "Worker failed during startup") so failed jobs that never entered the loop report 0% progress; locate these calls on the startup error path in reporting.rs and update the percentage argument from 100 to 0.
173-192:⚠️ Potential issue | 🟡 MinorOnly log success on a real success signal.
This branch only proves that
client.post_event(&payload)returned. Because the future resolves to(), the trace cannot distinguish an accepted post from a lower-level failure. Return aResultfromWorkerHttpClient::post_event, or drop the success log here.🔧 Minimal patch
match result { - Ok(()) => { - tracing::debug!(job_id = %job_id, ?event_type, "Posted job event"); - } + Ok(()) => {} Err(_) => { tracing::warn!( job_id = %job_id, ?event_type, "Job event post timed out after 5s" ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/container/reporting.rs` around lines 173 - 192, The current tokio::spawn block logs success on any resolved future but client.post_event(&payload) returns unit so it can't convey real success; update WorkerHttpClient::post_event to return Result<(), E> (e.g., Result<(), anyhow::Error> or your crate's error type), adjust its callers to propagate/handle that Result, and then change the match in the spawn block to distinguish timeout (Err(_)), an inner Err(e) from post_event (log warn/error with e), and only emit tracing::debug!(...) when the call returns Ok(()); alternatively, if you don't want to change the client API, remove the success log in the tokio::spawn block so you no longer report success on mere resolution of the future.src/worker/container.rs (1)
85-107:⚠️ Potential issue | 🟠 MajorReturn a configuration error instead of panicking.
Make this constructor fallible, or add
try_new. Theseassert_eq!s abort the worker on a mismatched injected client and bypass the normal failure-reporting path.As per coding guidelines, "Update helpers to return errors rather than panicking".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/container.rs` around lines 85 - 107, The WorkerRuntime::new currently uses assert_eq! to validate WorkerConfig against the injected Arc<WorkerHttpClient>, which panics on mismatch; change this to a fallible constructor (e.g., make new return Result<Self, WorkerConfigError> or add try_new) so configuration mismatches return an error instead of aborting. Replace the assert_eq! checks comparing config.job_id to client.job_id() and config.orchestrator_url.trim_end_matches('/') to client.orchestrator_url() with conditional checks that construct and return a descriptive error (including which field mismatched) instead of panicking; keep the same comparison logic and preserve WorkerConfig and WorkerHttpClient usage so callers can handle the Result.src/worker/api/client_methods.rs (1)
115-152:⚠️ Potential issue | 🟠 MajorStop treating
404as "no credentials" – this masks route drift.The orchestrator's
get_credentials_handlerreturns204 No Contentwhen no credentials are granted; it does not use404. Silently returning an empty vec on404conceals route misconfiguration or rollout skew, allowing the worker to proceed without required secrets.Remove the
NOT_FOUNDcheck so that a404surfaces as an error:Proposed fix
- /// Returns an empty vec if no credentials are granted (204 No Content) - /// or if the endpoint returns 404. The caller should set each credential + /// Returns an empty vec if no credentials are granted (204 No Content). + /// The caller should set each credential /// as an environment variable before starting the execution loop. pub async fn fetch_credentials(&self) -> Result<Vec<CredentialResponse>, WorkerError> { ... - // 204 or 404 means no credentials granted, not an error - if resp.status() == reqwest::StatusCode::NO_CONTENT - || resp.status() == reqwest::StatusCode::NOT_FOUND - { - return Ok(vec![]); + // 204 means no credentials granted + if resp.status() == reqwest::StatusCode::NO_CONTENT { + return Ok(Vec::new()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/api/client_methods.rs` around lines 115 - 152, fetch_credentials currently treats HTTP 404 as "no credentials" which hides route drift; remove the NOT_FOUND check in the response-status handling inside fetch_credentials (the block that currently checks reqwest::StatusCode::NO_CONTENT || reqwest::StatusCode::NOT_FOUND) so only 204 returns Ok(vec![]); let a 404 fall through to the existing non-success branch and return the SecretResolveFailed error (keeping the existing secret_name "(all)" and reason formatting). This change affects the fetch_credentials method and the CREDENTIALS_PATH usage when calling the orchestrator's get_credentials_handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worker/mod.rs`:
- Around line 60-61: Import anyhow::Context as _ at the top of the file and
replace the map_err stringification around WorkerRuntime::from_env(config) with
.context("Worker init failed") so the original error chain and backtrace are
preserved (i.e., change the call that assigns rt from WorkerRuntime::from_env to
use .context instead of map_err(|e| anyhow::anyhow!(...))).
---
Outside diff comments:
In `@src/worker/api.rs`:
- Around line 60-71: The constructor new(orchestrator_url: String, job_id: Uuid,
token: String) currently uses unwrap_or_default() when building reqwest::Client
which hides build failures; change new to return Result<Self, WorkerError>
(matching from_env()) and propagate reqwest::Client::builder().build() errors
via map_err to create a WorkerError::ConnectionFailed (include orchestrator_url
and the underlying error message), keep REQUEST_TIMEOUT and other fields the
same, and adjust tests/call sites to handle the Result.
---
Duplicate comments:
In `@src/worker/api/client_methods.rs`:
- Around line 115-152: fetch_credentials currently treats HTTP 404 as "no
credentials" which hides route drift; remove the NOT_FOUND check in the
response-status handling inside fetch_credentials (the block that currently
checks reqwest::StatusCode::NO_CONTENT || reqwest::StatusCode::NOT_FOUND) so
only 204 returns Ok(vec![]); let a 404 fall through to the existing non-success
branch and return the SecretResolveFailed error (keeping the existing
secret_name "(all)" and reason formatting). This change affects the
fetch_credentials method and the CREDENTIALS_PATH usage when calling the
orchestrator's get_credentials_handler.
In `@src/worker/container.rs`:
- Around line 85-107: The WorkerRuntime::new currently uses assert_eq! to
validate WorkerConfig against the injected Arc<WorkerHttpClient>, which panics
on mismatch; change this to a fallible constructor (e.g., make new return
Result<Self, WorkerConfigError> or add try_new) so configuration mismatches
return an error instead of aborting. Replace the assert_eq! checks comparing
config.job_id to client.job_id() and
config.orchestrator_url.trim_end_matches('/') to client.orchestrator_url() with
conditional checks that construct and return a descriptive error (including
which field mismatched) instead of panicking; keep the same comparison logic and
preserve WorkerConfig and WorkerHttpClient usage so callers can handle the
Result.
In `@src/worker/container/reporting.rs`:
- Around line 41-59: The progress percentage used for pre-loop startup failures
is incorrect: change the hardcoded 100 to 0 in the calls to report_worker_status
(with WorkerState::Failed and message "pre-loop failure") and report_failure
(with message "Worker failed during startup") so failed jobs that never entered
the loop report 0% progress; locate these calls on the startup error path in
reporting.rs and update the percentage argument from 100 to 0.
- Around line 173-192: The current tokio::spawn block logs success on any
resolved future but client.post_event(&payload) returns unit so it can't convey
real success; update WorkerHttpClient::post_event to return Result<(), E> (e.g.,
Result<(), anyhow::Error> or your crate's error type), adjust its callers to
propagate/handle that Result, and then change the match in the spawn block to
distinguish timeout (Err(_)), an inner Err(e) from post_event (log warn/error
with e), and only emit tracing::debug!(...) when the call returns Ok(());
alternatively, if you don't want to change the client API, remove the success
log in the tokio::spawn block so you no longer report success on mere resolution
of the future.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d169fce8-2793-4a06-922d-f75edf582bbd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlsrc/orchestrator/api.rssrc/worker/api.rssrc/worker/api/client_methods.rssrc/worker/api/error_mapping.rssrc/worker/api/types.rssrc/worker/container.rssrc/worker/container/reporting.rssrc/worker/container/tests/remote_tools.rssrc/worker/container/tests/test_support.rssrc/worker/mod.rs
💤 Files with no reviewable changes (1)
- src/worker/api/error_mapping.rs
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…und-trip test helper Refactor client_methods.rs tests to use a shared setup_for_test helper that creates and returns test state, client, and server handle to reduce duplication. Add assert_round_trips generic helper in transport_types.rs to test JSON serialization round-trip fidelity for various transport types, replacing repeated individual serialization/deserialization assertions with a single function. This improves test code maintainability and readability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/worker/api.rs (1)
130-133:⚠️ Potential issue | 🟡 MinorUse the full endpoint URL in
ConnectionFailedfor consistency.The
get_jsonmethod passesself.orchestrator_url.clone()toConnectionFailed, omitting the endpoint path. Inclient_methods.rs, the pattern usesself.url(path)to include the full URL. Align this method with that pattern for easier debugging.🐛 Proposed fix
async fn get_json<T: serde::de::DeserializeOwned>( &self, path: &str, context: &str, ) -> Result<T, WorkerError> { + let url = self.url(path); let resp = self .client - .get(self.url(path)) + .get(&url) .bearer_auth(&self.token) .send() .await .map_err(|e| WorkerError::ConnectionFailed { - url: self.orchestrator_url.clone(), + url, reason: e.to_string(), })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/api.rs` around lines 130 - 133, The ConnectionFailed error in get_json is using self.orchestrator_url.clone() which omits the endpoint path; update the error to include the full endpoint by passing self.url(path) (or equivalent that composes base + path) into WorkerError::ConnectionFailed so the logged URL matches the pattern used in client_methods.rs and aids debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/worker-orchestrator-contract.md`:
- Around line 120-126: Update the two sentences to add missing commas: in the
sentence describing ContainerDelegate, insert a comma after "therefore"
(referenced in the ContainerDelegate description and shutdown() behavior) so it
reads "therefore, uses..."; and in the WorkerRuntime sentence insert a comma
after "publication" (referenced in WorkerRuntime::post_event(...) and
report_complete(...)) so it reads "publication, so...". Ensure punctuation is
adjusted only—no other wording changes.
- Around line 3-6: Reflow the opening paragraph ("This document is for
maintainers who need to change the hosted worker path...") so every line is
wrapped at 80 columns or fewer; preserve the existing wording and meaning but
break long sentences across lines to meet the docstyle requirement, keep
markdown/spacing intact, and ensure no resulting line exceeds 80 characters.
In `@src/worker/api/tests/transport_types.rs`:
- Around line 138-149: The test terminal_result_round_trip_preserves_all_fields
duplicates manual field checks; replace the serialize/deserialize and three
assert_eq! calls with the common helper by calling assert_round_trips(&result)
(or assert_round_trips(result) if the helper takes ownership) to perform the
round-trip check for TerminalResult, and remove the explicit
serde_json::to_string/from_str and field-by-field assertions; ensure the
assert_round_trips helper is in scope for this test.
---
Outside diff comments:
In `@src/worker/api.rs`:
- Around line 130-133: The ConnectionFailed error in get_json is using
self.orchestrator_url.clone() which omits the endpoint path; update the error to
include the full endpoint by passing self.url(path) (or equivalent that composes
base + path) into WorkerError::ConnectionFailed so the logged URL matches the
pattern used in client_methods.rs and aids debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dfc7fd60-1a68-443c-9421-99d447e81703
📒 Files selected for processing (6)
docs/worker-orchestrator-contract.mdsrc/worker/api.rssrc/worker/api/client_methods.rssrc/worker/api/tests/client_methods.rssrc/worker/api/tests/transport_types.rssrc/worker/container.rs
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. src/worker/api/tests/client_methods.rs Comment on file async fn worker_http_client_report_status_posts_status_payload() -> anyhow::Result<()> {
async fn handler(
State(state): State<Arc<ClientMethodTestState>>,
Path(_job_id): Path<Uuid>,
headers: HeaderMap,
Json(update): Json<StatusUpdate>,
) -> StatusCode {
record_auth(&headers, &state).await;
state.status_updates.lock().await.push(update);
StatusCode::OK
}
let (state, client, handle) = setup_for_test(|state| {
Router::new()
.route(STATUS_ROUTE, post(handler))
.with_state(state)
})
.await?;
let update = StatusUpdate::new(WorkerState::InProgress, Some("working".to_string()), 3);
client.report_status(&update).await?;
let updates = state.status_updates.lock().await;
assert_eq!(updates.len(), 1);
assert_eq!(updates[0].state, update.state);
assert_eq!(updates[0].message, update.message);
assert_eq!(updates[0].iteration, update.iteration);
drop(updates);
assert_eq!(
state.auth_headers.lock().await.as_slice(),
&[format!("Bearer {TEST_BEARER_TOKEN}")]
);
handle.abort();
let _ = handle.await;
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Remove verbose serialization/deserialization field checks from terminal_result_round_trip_preserves_all_fields test and replace with generic assert_round_trips helper for clearer, concise tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/worker/api/types.rs (2)
213-214:⚠️ Potential issue | 🟡 MinorUse en-GB-oxendict "-ized" spelling.
The doc comments use "normalised" but en-GB-oxendict requires the "-ize" suffix: "normalized".
Triage:
[type:spelling]📝 Proposed fix
- /// Provider finish reason normalised into a transport enum. + /// Provider finish reason normalized into a transport enum.Apply the same change to both occurrences (lines 213 and 251).
Based on learnings: "In this repository's en-GB Oxford (OED) spelling guidelines, use the suffix '-ize' (e.g., initialize, recognize, organize) rather than '-ise'."
Also applies to: 251-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/api/types.rs` around lines 213 - 214, Update the doc comments that use the en-GB-OED "-ize" preference: change "normalised" to "normalized" for the FinishReason field and the other occurrence in the same file; specifically edit the comment above the struct field finish_reason: FinishReason and the second doc comment referenced around the other FinishReason usage to use "normalized" instead of "normalised".
1-413: 🧹 Nitpick | 🔵 TrivialFile exceeds 400-line limit.
This file is approximately 413 lines, exceeding the 400-line guideline. Decompose the module by extracting related type groups into separate submodules (e.g.,
proxy_types.rsforProxyCompletionRequest/ProxyCompletionResponse,remote_tool_types.rsfor remote tool request/response types).🤖 Prompt for AI Agents
Split src/worker/api/types.rs into smaller submodules to bring the file under 400 lines. Extract proxy completion types (ProxyCompletionRequest, ProxyCompletionResponse, ProxyToolCompletionRequest, ProxyToolCompletionResponse, FinishReason) into a new src/worker/api/proxy_types.rs submodule. Extract remote tool types (RemoteToolExecutionRequest, RemoteToolExecutionResponse, RemoteToolCatalogResponse) into src/worker/api/remote_tool_types.rs. Re-export all types from src/worker/api/types.rs to maintain the existing public API.As per coding guidelines: "Keep file size manageable—no single code file should exceed 400 lines; break large dispatch tables, switches, or long test data blocks into smaller units."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker/api/types.rs` around lines 1 - 413, The file is over 400 lines; split related type groups into submodules and re-export them: create src/worker/api/proxy_types.rs and move FinishReason, ProxyCompletionRequest, ProxyCompletionResponse, ProxyToolCompletionRequest, and ProxyToolCompletionResponse into it (keeping serde derives and From impls), create src/worker/api/remote_tool_types.rs and move RemoteToolExecutionRequest, RemoteToolExecutionResponse, and RemoteToolCatalogResponse into it, then in src/worker/api/types.rs add mod declarations for proxy_types and remote_tool_types and pub use the moved symbols (e.g., pub use proxy_types::{FinishReason, ProxyCompletionRequest, ...}; pub use remote_tool_types::{RemoteToolExecutionRequest, ...}) and update any imports in this module that referenced those types to use the re-exports so the public API and intra-crate references remain unchanged.
♻️ Duplicate comments (1)
docs/worker-orchestrator-contract.md (1)
120-122: 🧹 Nitpick | 🔵 TrivialComma placement after "therefore" is correct.
The sentence now reads "ContainerDelegate therefore, uses..." with the comma in place as suggested in previous review. However, static analysis flags a possible missing comma after "worker" in "waits for the event worker so buffered events flush". This is a minor stylistic suggestion that does not affect comprehension.
Triage:
[type:grammar]📝 Optional comma for clarity
`ContainerDelegate` therefore, uses a background task and bounded queue for -event posting. `shutdown()` closes the queue and waits for the event worker so +event posting. `shutdown()` closes the queue and waits for the event worker, so buffered events flush before the delegate disappears.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/worker-orchestrator-contract.md` around lines 120 - 122, Add a comma after "worker" in the sentence describing ContainerDelegate's shutdown behavior to improve clarity: update the sentence that references ContainerDelegate and its shutdown() method so it reads "...shutdown() closes the queue and waits for the event worker, so buffered events flush before the delegate disappears." Ensure you edit the docs/worker-orchestrator-contract.md text where ContainerDelegate and shutdown() are discussed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worker/api/types.rs`:
- Line 376: Update the doc comment that currently reads "Provides a consistent
serialised shape for job completion events" to use the en-GB-oxendict "-ized"
spelling by replacing "serialised" with "serialized" in the comment for the
corresponding type (the doc string beginning "Provides a consistent serialised
shape for job completion events"); ensure only the spelling is changed and
punctuation/formatting remain unchanged.
---
Outside diff comments:
In `@src/worker/api/types.rs`:
- Around line 213-214: Update the doc comments that use the en-GB-OED "-ize"
preference: change "normalised" to "normalized" for the FinishReason field and
the other occurrence in the same file; specifically edit the comment above the
struct field finish_reason: FinishReason and the second doc comment referenced
around the other FinishReason usage to use "normalized" instead of "normalised".
- Around line 1-413: The file is over 400 lines; split related type groups into
submodules and re-export them: create src/worker/api/proxy_types.rs and move
FinishReason, ProxyCompletionRequest, ProxyCompletionResponse,
ProxyToolCompletionRequest, and ProxyToolCompletionResponse into it (keeping
serde derives and From impls), create src/worker/api/remote_tool_types.rs and
move RemoteToolExecutionRequest, RemoteToolExecutionResponse, and
RemoteToolCatalogResponse into it, then in src/worker/api/types.rs add mod
declarations for proxy_types and remote_tool_types and pub use the moved symbols
(e.g., pub use proxy_types::{FinishReason, ProxyCompletionRequest, ...}; pub use
remote_tool_types::{RemoteToolExecutionRequest, ...}) and update any imports in
this module that referenced those types to use the re-exports so the public API
and intra-crate references remain unchanged.
---
Duplicate comments:
In `@docs/worker-orchestrator-contract.md`:
- Around line 120-122: Add a comma after "worker" in the sentence describing
ContainerDelegate's shutdown behavior to improve clarity: update the sentence
that references ContainerDelegate and its shutdown() method so it reads
"...shutdown() closes the queue and waits for the event worker, so buffered
events flush before the delegate disappears." Ensure you edit the
docs/worker-orchestrator-contract.md text where ContainerDelegate and shutdown()
are discussed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a502f13a-4e10-4278-867d-2508f05f8735
📒 Files selected for processing (4)
docs/worker-orchestrator-contract.mdsrc/worker/api.rssrc/worker/api/tests/transport_types.rssrc/worker/api/types.rs
- Extracted finish reason and proxy completion types into src/worker/api/proxy_types.rs - Extracted remote tool execution and catalog types into src/worker/api/remote_tool_types.rs - Cleaned up src/worker/api/types.rs to re-export these modules - Minor doc fix in worker-orchestrator-contract.md This modularization improves separation of concerns and maintainability of the worker API types. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worker/api/proxy_types.rs`:
- Around line 1-4: Add a module-level doc comment at the top of proxy_types.rs
using a //! comment that briefly describes the module's purpose (e.g., defines
proxy API types used by the worker, including ChatMessage, ToolCall, and
ToolDefinition re-exports/aliases) so the module meets the coding guideline;
place the comment above the use statements and reference the key types
ChatMessage, ToolCall, and ToolDefinition in the description.
In `@src/worker/api/remote_tool_types.rs`:
- Around line 1-5: Add a module-level doc comment at the top of this file using
the inner-documentation style (//! ) describing the purpose of the
remote_tool_types module (what it models and how ToolDefinition and ToolOutput
are used) so it conforms to coding guidelines; place the //! comment before the
use statements and include a brief sentence about the module's responsibility
(e.g., serializable types used for remote tool definitions and outputs involving
ToolDefinition and ToolOutput).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 722f43ee-5319-416c-a987-f5481218fe80
📒 Files selected for processing (4)
docs/worker-orchestrator-contract.mdsrc/worker/api/proxy_types.rssrc/worker/api/remote_tool_types.rssrc/worker/api/types.rs
Add detailed module-level documentation to explain the serializable types used by the worker-orchestrator boundary in proxy_types.rs and remote_tool_types.rs. This improves code clarity regarding the data structures used for proxied completions, remote tool definitions, and execution outputs. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worker/api/proxy_types.rs`:
- Around line 48-50: The Rustdoc comment incorrectly calls
ProxyCompletionRequest a "Completion result"; update the docstring to describe
it as the request payload sent to the orchestrator (e.g., "Request payload for
proxied completion" or similar). Edit the triple-slash comment above the struct
ProxyCompletionRequest to reflect it's a request type rather than a result so
public API docs are accurate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c69a9441-8f14-475e-99b2-b427aab055b8
📒 Files selected for processing (2)
src/worker/api/proxy_types.rssrc/worker/api/remote_tool_types.rs
Owner
Author
|
@coderabbitai pause |
Contributor
✅ Actions performedReviews paused. |
Updated the documentation comment to accurately describe ProxyCompletionRequest as a request payload rather than a completion result. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WorkerRuntime::from_env(config)and explicitWorkerRuntime::new(config, client)for DI.TerminalResultpayload and background event posting.Changes
API Routing
src/worker/api/types.rs(e.g.JOB_PATH,JOB_ROUTE,LLM_COMPLETE_PATH,LLM_COMPLETE_ROUTE,PROMPT_PATH,PROMPT_ROUTE,CREDENTIALS_PATH,CREDENTIALS_ROUTE,STATUS_PATH,STATUS_ROUTE,COMPLETE_PATH,COMPLETE_ROUTE,EVENT_PATH,EVENT_ROUTE,REMOTE_TOOL_*_ROUTE,WORKER_HEALTH_ROUTE).src/orchestrator/api.rswith the centralized constants.WORKER_HEALTH_ROUTEand wired it into the router.job_scoped_pathandworker_job_urlto share path construction logic across client and server.Client/Runtime
WorkerRuntime::newdepend on an explicit client (dependency injection).WorkerRuntime::from_env(config)to construct a client from environment variables.src/worker/api/client_methods.rs) to work with status, events, credentials, prompt polling, and completion using centralized routes.Errors
ConfigMismatchFieldand extendedWorkerErrorwith aConfigMismatchvariant to enforce runtime/config consistency.src/worker/api/error_mapping.rsas a separate module in favor of integrated error handling in client methods.WorkerError::ConnectionFailedcarries the failing URL and provides consistent context for debugging.Internal Runtime & Container
ContainerDelegateand a newworker/container/reporting.rsto manage terminal results and reporting flows.ContainerDelegateto support a factory methodnew(...)that wires a background task for posting events.Types & Payloads
src/worker/api/types.rs(e.g. health, job, prompt, status, complete, etc.).TerminalResultpayload type for consistent terminal event reporting.Tests
JOB_ROUTE,CREDENTIALS_ROUTE,PROMPT_ROUTE,STATUS_ROUTE,COMPLETE_ROUTE,EVENT_ROUTE, etc.WorkerRuntime::newwith an explicit client instead of environment-based construction where appropriate.from_envwhere appropriate and to verify URL normalization.Docs & Contract
docs/worker-orchestrator-contract.mddescribing the worker-runtime boundary, shared route contract, and the dependency-injection model.docs/contents.mdto reference the worker-orchestrator contract.Other
WORKER_HEALTH_ROUTE(unauthenticated health checks).Task