Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces single-string ChangesStream-aware output chunks and offset pagination
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/mcp/server.rs (1)
763-767:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid dropping output receivers on capture timeout.
At Line 763, wrapping
initial_captureintimeout(...)can hitElapsed; then Line 895 falls back to(None, None). That drops receiver handoff, so reader tasks keep writing into bounded channels with no consumer, which can block pipe draining and stall/loss output for running tasks.Suggested fix
- let capture_result = timeout( - capture_duration + Duration::from_millis(100), - initial_capture, - ) - .await; + // initial_capture already has an internal deadline; await it directly + // so receivers are always handed off. + let capture_result = initial_capture.await; ... - let (mut stdout_rx_opt, mut stderr_rx_opt) = if let Ok(Ok((rx1, rx2))) = capture_result + let (mut stdout_rx_opt, mut stderr_rx_opt) = if let Ok((rx1, rx2)) = capture_result { (Some(rx1), Some(rx2)) } else { (None, None) };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/server.rs` around lines 763 - 767, The timeout wrapper around initial_capture at line 763 can return an Elapsed error, which then falls back to (None, None) at line 895, dropping the receiver handoff. This causes reader tasks to continue writing into bounded channels with no consumer, potentially blocking output. Instead of discarding the receivers when the timeout elapses, ensure the receiver from initial_capture is still properly handed off even in the Elapsed case, so that reader tasks have a consumer and channels do not block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/docker_mcp/test_mcp.py`:
- Around line 171-184: The output_text function has overly strict chunk
validation that does not match the DTO schema contract. The assertion on line
178 checking `len(chunk) == 1` should be removed or relaxed, since
OutputChunkDto allows chunks to have 0, 1, or 2 keys depending on which optional
fields are present. Additionally, the code using `next(iter(chunk.items()))`
only extracts the first key-value pair, causing potential data loss if both
stdout and stderr are present in the same chunk. Instead of asserting exactly
one key, iterate through the chunk items to check for both "stdout" and "stderr"
keys independently, appending their values if present and if they match the
stream filter parameter, to handle all valid chunk shapes that the schema
permits.
---
Outside diff comments:
In `@src/mcp/server.rs`:
- Around line 763-767: The timeout wrapper around initial_capture at line 763
can return an Elapsed error, which then falls back to (None, None) at line 895,
dropping the receiver handoff. This causes reader tasks to continue writing into
bounded channels with no consumer, potentially blocking output. Instead of
discarding the receivers when the timeout elapses, ensure the receiver from
initial_capture is still properly handed off even in the Elapsed case, so that
reader tasks have a consumer and channels do not block.
🪄 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: 8c2778fa-828a-4b7a-a061-0b44d8e0b3de
📒 Files selected for processing (5)
dev_docs/mcp_design.mddev_docs/project_plan.mdsrc/mcp/dto.rssrc/mcp/server.rstests/docker_mcp/test_mcp.py
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 (2)
src/mcp/server.rs (1)
1700-1702:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
task_outputtool description to match the new contract.Line 1701 still says “Tail last N lines for a PID”, but this endpoint now returns stream-aware
outputchunks with optionaloffsetpaging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/server.rs` around lines 1700 - 1702, The description string for the "task_output" tool is outdated and no longer matches its current behavior. Replace the existing description "Tail last N lines for a PID" with an updated description that accurately reflects the tool's new contract of returning stream-aware output chunks with optional offset paging.tests/docker_mcp/test_mcp.py (1)
558-558: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRequest ID should continue global increment for consistency.
The test file uses globally incrementing request IDs (1→19) for clarity, even though each test starts a fresh process. Test 10 now uses IDs 18–19, so test 11 should use ID 20 to maintain the pattern.
📝 Proposed fix for consistency
process, tool_request( - 19, + 20, "task_start",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/docker_mcp/test_mcp.py` at line 558, The request ID value at line 558 should be incremented to maintain the globally incrementing pattern across all tests in the file. Since test 10 uses IDs 18-19, the next test (test 11) should continue with ID 20. Change the value from 19 to 20 at the identified location to ensure consistency with the global increment pattern used throughout the test file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mcp/server.rs`:
- Around line 809-812: The `add_job_output_chunks` and `add_job_output_chunk`
method calls are discarding errors using the `let _ =` pattern, which silently
loses output persistence failures and can leave task_output inconsistent. At all
three affected locations (line 810 in the first if block where
Self::add_job_output_chunks is called, and the two additional sites at lines
907-909 and 940-942 where add_job_output_chunk is called), replace the `let _ =`
pattern with proper error handling such as logging the error or propagating it
appropriately so that output persistence failures are no longer silently
discarded.
- Around line 1148-1183: The else branch at the end of the truncation logic
returns output_entries without any size validation when there is only one entry,
allowing a single oversized entry to bypass the MAX_CHUNK_SIZE limit. Apply the
same oversized entry handling logic that exists within the if
output_entries.len() > 1 block to the else branch: check if the single entry's
JSON representation exceeds MAX_CHUNK_SIZE and truncate the entry's text field
if necessary, following the same pattern as the nested if
truncated_entries.is_empty() check, so that a single oversized output entry is
properly constrained before being returned.
In `@tests/docker_mcp/test_mcp.py`:
- Line 483: The assertion checking next_offset advancement uses the `>=`
operator, but since output is already confirmed to be non-empty on the preceding
line, next_offset must be strictly greater than offset to indicate proper
pagination. Change the comparison operator from `>=` to `>` in the
assert_condition call that validates next_offset, ensuring that the assertion
correctly rejects cases where next_offset equals offset despite entries being
returned, which would indicate a pagination bug.
---
Outside diff comments:
In `@src/mcp/server.rs`:
- Around line 1700-1702: The description string for the "task_output" tool is
outdated and no longer matches its current behavior. Replace the existing
description "Tail last N lines for a PID" with an updated description that
accurately reflects the tool's new contract of returning stream-aware output
chunks with optional offset paging.
In `@tests/docker_mcp/test_mcp.py`:
- Line 558: The request ID value at line 558 should be incremented to maintain
the globally incrementing pattern across all tests in the file. Since test 10
uses IDs 18-19, the next test (test 11) should continue with ID 20. Change the
value from 19 to 20 at the identified location to ensure consistency with the
global increment pattern used throughout the test file.
🪄 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: f3d3178b-2cf2-41b2-a9f5-831d8136d11f
📒 Files selected for processing (7)
README.mddev_docs/mcp_design.mddev_docs/project_plan.mdsrc/mcp/dto.rssrc/mcp/job_manager.rssrc/mcp/server.rstests/docker_mcp/test_mcp.py
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)
tests/docker_mcp/test_mcp.py (1)
559-559: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRequest ID reused across tests (safe but potentially confusing).
Test 10 (
test_nonexistent_job_tools) already uses request ID19on line 543. While safe because each test starts a fresh MCP process, reusing the same ID in test 11 could be confusing during debugging. Consider continuing the sequence (e.g., ID20) or restarting from1for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/docker_mcp/test_mcp.py` at line 559, The request ID 19 at line 559 in test 11 is reused from test 10 (line 543), which could be confusing during debugging even though it's functionally safe. Change the request ID value at line 559 from 19 to a new value that either continues the sequence (e.g., 20) or restarts from 1 for clarity within the test.
♻️ Duplicate comments (1)
tests/docker_mcp/test_mcp.py (1)
484-484:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPagination assertion should require strict advancement when output is non-empty.
Line 482 already asserts that
outputis non-empty (empty lists are falsy in Python). Given that,next_offsetmust be strictly greater thanoffsetper the server'snext_offset = offset + returned_lineslogic. The current>=allowsnext_offset == offseteven when entries are returned, which would indicate a pagination bug in the server.✏️ Tighten the assertion
- assert_condition(output_payload["next_offset"] >= output_payload["offset"], "task_output should report next offset", output_payload) + assert_condition(output_payload["next_offset"] > output_payload["offset"], "task_output should advance next_offset when entries are returned", output_payload)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/docker_mcp/test_mcp.py` at line 484, The assertion in the assert_condition call comparing next_offset and offset is too lenient. Since line 482 confirms that output is non-empty (non-empty lists are truthy), the pagination logic guarantees that next_offset must be strictly greater than offset when entries are returned. Change the comparison operator from >= to > to enforce strict advancement of the offset in the assert_condition call on the pagination assertion, ensuring the server's pagination logic correctly advances the offset based on the number of returned lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/docker_mcp/test_mcp.py`:
- Around line 180-181: The defensive `if value is None: continue` check silently
skips None values instead of failing the test, which masks potential schema
violations in the server's serialization. According to the OutputChunkDto
schema, None values should never appear in the JSON because fields are omitted
entirely via skip_serializing_if. Remove the None check entirely and allow the
`isinstance(value, str)` assertion that follows to fail fast if a None value
unexpectedly appears, providing stricter validation that catches server-side
serialization bugs.
---
Outside diff comments:
In `@tests/docker_mcp/test_mcp.py`:
- Line 559: The request ID 19 at line 559 in test 11 is reused from test 10
(line 543), which could be confusing during debugging even though it's
functionally safe. Change the request ID value at line 559 from 19 to a new
value that either continues the sequence (e.g., 20) or restarts from 1 for
clarity within the test.
---
Duplicate comments:
In `@tests/docker_mcp/test_mcp.py`:
- Line 484: The assertion in the assert_condition call comparing next_offset and
offset is too lenient. Since line 482 confirms that output is non-empty
(non-empty lists are truthy), the pagination logic guarantees that next_offset
must be strictly greater than offset when entries are returned. Change the
comparison operator from >= to > to enforce strict advancement of the offset in
the assert_condition call on the pagination assertion, ensuring the server's
pagination logic correctly advances the offset based on the number of returned
lines.
🪄 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: a28266a6-5378-4e13-9d57-a6dbe898eb38
📒 Files selected for processing (1)
tests/docker_mcp/test_mcp.py
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 (1)
src/mcp/server.rs (1)
1737-1739:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTool description is inconsistent with actual behavior.
The
task_outputdescription here says "Tail last N lines for a PID" but the tool was refactored to read output chunks with offset-based pagination. This contradicts both the#[tool]attribute at line 1123 and the actual implementation.✏️ Suggested fix
Tool::new_with_raw( "task_output", - Some("Tail last N lines for a PID".into()), + Some("Read output chunks for a PID".into()), task_output_schema, ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/server.rs` around lines 1737 - 1739, Update the tool description for task_output in the Tool::new_with_raw call to accurately reflect the actual implementation behavior. The current description "Tail last N lines for a PID" is incorrect since the tool was refactored to use offset-based pagination for reading output chunks rather than tailing lines. Replace the description to accurately describe the offset-based pagination functionality and ensure it aligns with the tool definition in the #[tool] attribute at line 1123.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/mcp/server.rs`:
- Around line 1737-1739: Update the tool description for task_output in the
Tool::new_with_raw call to accurately reflect the actual implementation
behavior. The current description "Tail last N lines for a PID" is incorrect
since the tool was refactored to use offset-based pagination for reading output
chunks rather than tailing lines. Replace the description to accurately describe
the offset-based pagination functionality and ensure it aligns with the tool
definition in the #[tool] attribute at line 1123.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56237375-5440-434a-a46e-511ef451bb38
📒 Files selected for processing (2)
src/mcp/server.rstests/docker_mcp/test_mcp.py
aleyan
changed the title
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)
tests/docker_mcp/test_mcp.py (2)
489-500:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPage from
next_offset, not the currentoffset.The follow-up request reuses
output_payload["offset"], so it rereads the first page instead of proving that clients can continue pagination from the previous response. Usingoutput_payload["next_offset"]here would catch overlapping-page regressions intask_output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/docker_mcp/test_mcp.py` around lines 489 - 500, In the tool_request call that creates the offset_response (line 490-492), the offset parameter is currently using output_payload["offset"], which rereads the same page instead of testing true pagination. Change the offset parameter in the tool_request call from output_payload["offset"] to output_payload["next_offset"] to properly validate that clients can continue pagination from the previous response and catch any overlapping-page regressions in the task_output function.
381-390: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winCover stderr in the bounded-wait end-to-end assertion.
This fixture exercises the stream-aware start response, but the docker test only inspects stdout here. A regression that drops stderr from
task_start.outputwould still pass end to end, so add anoutput_text(payload, "stderr")assertion for the bounded-wait stderr line as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/docker_mcp/test_mcp.py` around lines 381 - 390, The bounded-wait end-to-end test in the assertion block checks stdout output but does not verify stderr output from the bounded-wait task. Add an assert_condition call that uses output_text(payload, "stderr") to verify that expected stderr content from the bounded-wait task is present in the output, similar to the existing stdout assertions. This ensures that a regression dropping stderr from task_start.output would be caught by the test.src/mcp/server.rs (1)
764-787:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDrain the post-window receivers before taking the completed-task fast path.
If the child exits just after
initial_capturereturns, this branch snapshotscaptured_output_chunksand then waits on the reader tasks while nobody is consumingstdout_rx/stderr_rxanymore. That drops any post-deadline output, and a verbose process can hangtask_starthere once the 100-entry channel fills becausetx.send(...).awaitnever completes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp/server.rs` around lines 764 - 787, The process_exited branch snapshots captured_output_chunks and waits on the reader tasks (stdout_task and stderr_task) without draining the stdout_rx and stderr_rx receivers. Since these are bounded channels, if the reader tasks have more output to send after the process exits, the channels will fill up and the reader tasks will block on send, causing the task.await calls to hang indefinitely. Before waiting on the reader tasks in this branch, drain any remaining data from the stdout_rx and stderr_rx receivers to allow the reader tasks to complete without blocking on the full channel.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/mcp/server.rs`:
- Around line 764-787: The process_exited branch snapshots
captured_output_chunks and waits on the reader tasks (stdout_task and
stderr_task) without draining the stdout_rx and stderr_rx receivers. Since these
are bounded channels, if the reader tasks have more output to send after the
process exits, the channels will fill up and the reader tasks will block on
send, causing the task.await calls to hang indefinitely. Before waiting on the
reader tasks in this branch, drain any remaining data from the stdout_rx and
stderr_rx receivers to allow the reader tasks to complete without blocking on
the full channel.
In `@tests/docker_mcp/test_mcp.py`:
- Around line 489-500: In the tool_request call that creates the offset_response
(line 490-492), the offset parameter is currently using
output_payload["offset"], which rereads the same page instead of testing true
pagination. Change the offset parameter in the tool_request call from
output_payload["offset"] to output_payload["next_offset"] to properly validate
that clients can continue pagination from the previous response and catch any
overlapping-page regressions in the task_output function.
- Around line 381-390: The bounded-wait end-to-end test in the assertion block
checks stdout output but does not verify stderr output from the bounded-wait
task. Add an assert_condition call that uses output_text(payload, "stderr") to
verify that expected stderr content from the bounded-wait task is present in the
output, similar to the existing stdout assertions. This ensures that a
regression dropping stderr from task_start.output would be caught by the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: da14e5a3-a129-43b3-b0a9-b740317aa728
📒 Files selected for processing (2)
src/mcp/server.rstests/docker_mcp/test_mcp.py
1 participant
Summary
outputchunks fromtask_startinstead ofinitial_output.task_outputchunks withoffset/next_offsetpagination and no legacylinesfield.Validation
cargo fmt --allPYTHONPYCACHEPREFIX=/tmp/dela_pycache python3 -m py_compile tests/docker_mcp/test_mcp.pycargo testmake lintmake test_mcpSummary by CodeRabbit
Release Notes
New Features
Improvements