Implement ACP capability masking at protocol proxy seam (2.6.1) - #79
Merged
Conversation
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntercept the first newline‑delimited stdin frame in protocol mode, detect ACP ChangesACP Initialize Masking
Sequence DiagramsequenceDiagram
participant Client
participant ProtocolHandler as Protocol Handler
participant ACPHelper as ACP Helper
participant Container
Client->>ProtocolHandler: stdin stream (newline‑delimited)
ProtocolHandler->>ACPHelper: capture up‑to‑first‑frame (bounded)
ACPHelper->>ACPHelper: detect line ending, attempt JSON‑RPC parse
alt initialize message with clientCapabilities
ACPHelper->>ACPHelper: remove terminal & fs from clientCapabilities
ACPHelper->>ProtocolHandler: rewritten frame (preserve line ending)
else non‑initialize or parse failure
ACPHelper->>ProtocolHandler: original first‑frame bytes
end
ProtocolHandler->>Container: forwarded first frame
Client->>ProtocolHandler: remaining stdin bytes
ProtocolHandler->>Container: passthrough raw bytes
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 13 | ❌ 5❌ Failed checks (5 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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.
Reviewer's GuideImplements ACP capability masking in the protocol stdin proxy by rewriting only the first ACP initialize frame to strip terminal/fs capabilities, adds focused unit and BDD tests, and updates design/docs/roadmap to describe and mark this behaviour as complete while performing minor documentation reflow elsewhere. Sequence diagram for ACP initialize masking in protocol stdin proxysequenceDiagram
actor IDE
participant PodbotProtocolProxy as PodbotProtocolProxy
participant HostStdin as HostStdin
participant ContainerStdin as ContainerStdin
IDE->>HostStdin: Write ACP initialize frame and subsequent bytes
HostStdin->>PodbotProtocolProxy: ExecMode::Protocol stdin stream
activate PodbotProtocolProxy
PodbotProtocolProxy->>PodbotProtocolProxy: run_input_loop_async()
PodbotProtocolProxy->>HostStdin: read_until('\n') via forward_initial_protocol_frame_async
HostStdin-->>PodbotProtocolProxy: First_frame_bytes
PodbotProtocolProxy->>PodbotProtocolProxy: mask_acp_initialize_frame(frame)
PodbotProtocolProxy->>PodbotProtocolProxy: split_frame_line_ending(frame)
PodbotProtocolProxy->>PodbotProtocolProxy: parse_json_message(payload)
PodbotProtocolProxy->>PodbotProtocolProxy: remove_masked_acp_capabilities(message)
PodbotProtocolProxy->>PodbotProtocolProxy: serialize_masked_message(message, line_ending)
PodbotProtocolProxy->>ContainerStdin: write_all(masked_or_original_first_frame)
PodbotProtocolProxy->>HostStdin: copy remaining bytes via tokio::io::copy
HostStdin-->>PodbotProtocolProxy: Remaining_stdin_bytes
PodbotProtocolProxy->>ContainerStdin: Forward_remaining_bytes
PodbotProtocolProxy->>ContainerStdin: flush()
PodbotProtocolProxy->>ContainerStdin: shutdown()
deactivate PodbotProtocolProxy
Class diagram for ProtocolProxyIo ACP masking helpersclassDiagram
class ProtocolProxyIo {
+new(host_stdin, host_stdout, host_stderr)
+run_input_loop_async(host_stdin, input)
+run_output_loop_async(container_id, output, host_stdout, host_stderr)
-STDIN_SETTLE_TIMEOUT: Duration
-STDIN_BUFFER_CAPACITY: usize
-ACP_INITIALIZE_METHOD: &str
-ACP_CLIENT_CAPABILITIES_FIELD: &str
-ACP_FILE_SYSTEM_CAPABILITY: &str
-ACP_TERMINAL_CAPABILITY: &str
}
class ACPMaskingHelpers {
+forward_initial_protocol_frame_async(buffered_stdin, input) io::Result
+mask_acp_initialize_frame(frame) Vec~u8~
+split_frame_line_ending(frame) (&[u8], &[u8])
+parse_json_message(payload) Option~Value~
+remove_masked_acp_capabilities(message) bool
+serialize_masked_message(message, line_ending) Option~Vec~u8~~
}
ProtocolProxyIo ..> ACPMaskingHelpers: uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
leynos
marked this pull request as ready for review
April 22, 2026 22:18
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/engine/connection/exec/protocol_acp_tests.rs" line_range="65-74" />
<code_context>
+ }
+}
+
+fn initialize_frame(line_ending: &str) -> Vec<u8> {
+ let payload = serde_json::json!({
+ "jsonrpc": "2.0",
+ "id": 0,
+ "method": "initialize",
+ "params": {
+ "protocolVersion": 1,
+ "clientCapabilities": {
+ "fs": {
+ "readTextFile": true,
+ "writeTextFile": true
+ },
+ "terminal": true,
+ "_meta": {
+ "custom": true
+ }
+ },
+ "clientInfo": {
+ "name": "podbot-tests",
+ "version": "1.0.0"
+ }
+ }
+ });
+
+ let mut frame = serde_json::to_vec(&payload).expect("initialize payload should serialize");
+ frame.extend_from_slice(line_ending.as_bytes());
+ frame
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for initialize frames where `clientCapabilities` only contains blocked capabilities so the field gets removed
Currently all tests keep a non-blocked `_meta` entry, so `clientCapabilities` never becomes empty and the branch that removes the entire field is untested. Please add a unit test that builds an `initialize` frame whose `clientCapabilities` only includes `fs` and/or `terminal`, and then assert that after masking: (1) `clientCapabilities` is removed, and (2) other `params` fields such as `clientInfo` and `protocolVersion` are unchanged. This will cover the `client_capabilities.is_empty()` path in `remove_masked_acp_capabilities`.
Suggested implementation:
```rust
use rstest::rstest;
use rstest_bdd::Slot;
use rstest_bdd_macros::{ScenarioState, given, scenario, then, when};
use tokio::io::{AsyncWrite, AsyncWriteExt, DuplexStream};
#[test]
fn remove_masked_acp_capabilities_removes_empty_client_capabilities() {
// Build an initialize payload whose `clientCapabilities` only contains
// capabilities that should be fully masked (`fs` and `terminal`).
let mut payload = serde_json::json!({
"jsonrpc": "2.0",
"id": 0,
"method": "initialize",
"params": {
"protocolVersion": 1,
"clientCapabilities": {
"fs": {
"readTextFile": true,
"writeTextFile": true
},
"terminal": true
},
"clientInfo": {
"name": "podbot-tests",
"version": "1.0.0"
}
}
});
// Exercise the masking logic.
remove_masked_acp_capabilities(&mut payload);
// `clientCapabilities` should be completely removed once all entries are masked.
assert!(
payload["params"].get("clientCapabilities").is_none(),
"`clientCapabilities` should be removed when all capabilities are masked"
);
// Other `params` fields must remain unchanged.
assert_eq!(
payload["params"]["protocolVersion"],
serde_json::json!(1),
"`protocolVersion` should remain unchanged after masking"
);
assert_eq!(
payload["params"]["clientInfo"],
serde_json::json!({
"name": "podbot-tests",
"version": "1.0.0"
}),
"`clientInfo` should remain unchanged after masking"
);
}
```
1. Ensure that `remove_masked_acp_capabilities` is in scope for this test. If it lives in the same module as other ACP tests, you may need a `use super::remove_masked_acp_capabilities;` near the top of the file.
2. If the real API operates on frames/bytes instead of a `serde_json::Value`, adapt the test body to:
* Serialize `payload` to a frame (including the correct line ending),
* Pass that frame through the masking function under test,
* Deserialize back to `serde_json::Value` before asserting on `params`.
3. If `remove_masked_acp_capabilities` also strips the `protocolVersion` or modifies `clientInfo` in your implementation, adjust the expectations accordingly so they match the intended behavior described in your comment.
</issue_to_address>
### Comment 2
<location path="src/engine/connection/exec/protocol.rs" line_range="209" />
<code_context>
+ .await
+}
+
+fn mask_acp_initialize_frame(frame: &[u8]) -> Vec<u8> {
+ let (payload, line_ending) = split_frame_line_ending(frame);
+ let Some(mut message) = parse_json_message(payload) else {
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining the serde parsing/serialization calls into `mask_acp_initialize_frame` and removing the extra helper functions to keep the masking logic in one readable place.
You can simplify `mask_acp_initialize_frame` by inlining the thin serde wrappers and reducing the number of tiny helpers. This keeps the masking behavior identical but makes the control flow easier to follow.
For example:
```rust
fn mask_acp_initialize_frame(frame: &[u8]) -> Vec<u8> {
let (payload, line_ending) = split_frame_line_ending(frame);
let mut message: Value = match serde_json::from_slice(payload) {
Ok(v) => v,
Err(_) => return frame.to_vec(),
};
if !remove_masked_acp_capabilities(&mut message) {
return frame.to_vec();
}
let mut serialized = match serde_json::to_vec(&message) {
Ok(v) => v,
Err(_) => return frame.to_vec(),
};
serialized.extend_from_slice(line_ending);
serialized
}
```
With this, you can drop `parse_json_message` and `serialize_masked_message` entirely, and keep `remove_masked_acp_capabilities` as the one domain-specific helper:
```rust
fn remove_masked_acp_capabilities(message: &mut Value) -> bool {
if message.get("method").and_then(Value::as_str) != Some(ACP_INITIALIZE_METHOD) {
return false;
}
let Some(params) = message.get_mut("params").and_then(Value::as_object_mut) else {
return false;
};
let Some(client_capabilities) = params
.get_mut(ACP_CLIENT_CAPABILITIES_FIELD)
.and_then(Value::as_object_mut)
else {
return false;
};
let removed_terminal = client_capabilities.remove(ACP_TERMINAL_CAPABILITY).is_some();
let removed_fs = client_capabilities.remove(ACP_FILE_SYSTEM_CAPABILITY).is_some();
if !removed_terminal && !removed_fs {
return false;
}
if client_capabilities.is_empty() {
params.remove(ACP_CLIENT_CAPABILITIES_FIELD);
}
true
}
```
This keeps all fallback behavior (`return frame.to_vec()` on any parse/serialize failure or no-op case) but removes indirection and makes the overall transformation readable in one place.
</issue_to_address>
### Comment 3
<location path="docs/execplans/2-6-1-intercept-acp-initialization.md" line_range="1" />
<code_context>
+# Step 2.6.1: Intercept ACP initialization
+
+This ExecPlan (execution plan) is a living document. The sections
</code_context>
<issue_to_address>
**issue (review_instructions):** ACP is introduced in this file without being expanded on first use, which violates the acronym-definition requirement.
Please expand ACP on first use in this ExecPlan heading or immediately afterwards, for example: `Intercept Agentic Control Protocol (ACP) initialization`, and then use `ACP` thereafter.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1f6093609
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
implement-acp-masking-jabwf2
branch
from
April 30, 2026 13:24
b1f6093 to
91f9f27
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
implement-acp-masking-jabwf2
branch
from
April 30, 2026 13:27
91f9f27 to
1d1f746
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
implement-acp-masking-jabwf2
branch
from
April 30, 2026 15:55
1d1f746 to
3ba5558
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Intercept the first newline-delimited Agentic Control Protocol (ACP) `initialize` frame from protocol stdin and remove host-delegated `terminal` and `fs` capabilities before the container sees the metadata. Bound the first-frame inspection buffer so oversized or unterminated input streams forward promptly into the container instead of waiting indefinitely. Document the ACP masking trust boundary, add unit and BDD coverage, and mark roadmap Step 2.6.1 as complete.
lodyai
Bot
force-pushed
the
implement-acp-masking-jabwf2
branch
from
April 30, 2026 16:02
3ba5558 to
aa90773
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
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.
♻️ Duplicate comments (4)
docs/users-guide.md (1)
66-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify the ACP scope while
podbot hostis still unavailable.State explicitly that this masking applies only to the current
protocol/library path untilpodbot hostships; otherwise this paragraph reads
as if the public subcommand already exists.Triage:
[type:docstyle]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/users-guide.md` around lines 66 - 69, Update the paragraph to explicitly state that the masking of host-delegated `terminal/*` and `fs/*` capabilities applies only to the current protocol/library path used by hosted mode and ACP (i.e., the behavior of the existing `initialize` forwarding to the sandboxed agent) until the `podbot host` subcommand is implemented; mention `podbot host` by name as a forthcoming feature and clarify that once `podbot host` ships the scope/behavior may change. Reference the existing terms `podbot host`, ACP `initialize`, `terminal/*`, `fs/*`, and "sandboxed agent" so readers can locate and understand the exact codepath and intended temporary scope.src/engine/connection/exec/protocol.rs (1)
238-249:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGate the first-frame interception to ACP sessions only.
This helper still runs on the shared protocol path, so non-ACP transports now
get a newline-delimited pre-read they did not ask for. Any protocol that does
not start with a line break can block here before normal byte copying begins.
Thread the protocol kind through this call site and bypass the rewrite unless
the session is ACP.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/connection/exec/protocol.rs` around lines 238 - 249, The helper forward_host_stdin_to_exec_async is unconditionally pre-reading the first frame via forward_initial_protocol_frame_async and must instead only do that for ACP sessions: add a parameter (e.g., protocol_kind or is_acp: bool) to forward_host_stdin_to_exec_async, propagate this change to its callers, and conditionally call forward_initial_protocol_frame_async(&mut buffered_stdin, &mut input).await? only when the session is ACP; for non-ACP sessions skip the initial-frame rewrite and go straight to tokio::io::copy, then flush/shutdown as before. Ensure the function signature and all call sites (places that invoke forward_host_stdin_to_exec_async) are updated to pass the protocol indicator.docs/execplans/2-6-1-intercept-acp-initialization.md (1)
257-259:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Oxford spelling for
containerized.Replace
containerisedwithcontainerizedso the plan follows the
en-GB-oxendict rule.Triage:
[type:spelling][type:docstyle]✏️ Proposed edit
- containerised agent; + containerized agent;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/2-6-1-intercept-acp-initialization.md` around lines 257 - 259, Change the British spelling "containerised" to American/Oxford "containerized" in the sentence that reads "ACP `initialize` capability metadata is masked before reaching the containerised agent"; specifically replace the token "containerised" with "containerized" so the doc line containing "ACP `initialize` capability metadata is masked before reaching the containerised agent" is updated accordingly.src/engine/connection/exec/protocol_acp_tests.rs (1)
1-459: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit this module before it exceeds the repository limit.
Move the BDD state, helper writers, and scenario bindings into a sibling file
so this test module drops back under the 400-line guidance. While you are
there, swap the trailing-bytes regression to a second ACPinitializeframe
so it actually exercises the first-frame-only contract.🧪 Tighten the regression
- let trailing = session_new_bytes(); + let trailing = initialize_frame("\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/connection/exec/protocol_acp_tests.rs` around lines 1 - 459, This file exceeds the size guideline; split out the BDD state, helper writer, and scenario bindings into a sibling module: move AcpMaskingState, RecordingInputWriter, build_host_stdin/run_forwarding helpers, the rstest fixture acp_masking_state, and the scenario functions (acp_initialize_masks_blocked_capabilities, malformed_acp_initialize_is_forwarded_unchanged, acp_initialize_without_blocked_capabilities_stays_unchanged) into a new file and re-export or import them so tests still compile; additionally tighten the trailing-bytes regression by changing forwarding_masks_initialize_and_preserves_trailing_bytes and masked_initialize_with_follow_up to append a second initialize frame (not session_new_bytes) as the trailing bytes to ensure the test exercises the first-frame-only behavior (update masked_initialize_with_follow_up expected/host_stdin construction accordingly and keep mask_acp_initialize_frame usage unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/execplans/2-6-1-intercept-acp-initialization.md`:
- Around line 257-259: Change the British spelling "containerised" to
American/Oxford "containerized" in the sentence that reads "ACP `initialize`
capability metadata is masked before reaching the containerised agent";
specifically replace the token "containerised" with "containerized" so the doc
line containing "ACP `initialize` capability metadata is masked before reaching
the containerised agent" is updated accordingly.
In `@docs/users-guide.md`:
- Around line 66-69: Update the paragraph to explicitly state that the masking
of host-delegated `terminal/*` and `fs/*` capabilities applies only to the
current protocol/library path used by hosted mode and ACP (i.e., the behavior of
the existing `initialize` forwarding to the sandboxed agent) until the `podbot
host` subcommand is implemented; mention `podbot host` by name as a forthcoming
feature and clarify that once `podbot host` ships the scope/behavior may change.
Reference the existing terms `podbot host`, ACP `initialize`, `terminal/*`,
`fs/*`, and "sandboxed agent" so readers can locate and understand the exact
codepath and intended temporary scope.
In `@src/engine/connection/exec/protocol_acp_tests.rs`:
- Around line 1-459: This file exceeds the size guideline; split out the BDD
state, helper writer, and scenario bindings into a sibling module: move
AcpMaskingState, RecordingInputWriter, build_host_stdin/run_forwarding helpers,
the rstest fixture acp_masking_state, and the scenario functions
(acp_initialize_masks_blocked_capabilities,
malformed_acp_initialize_is_forwarded_unchanged,
acp_initialize_without_blocked_capabilities_stays_unchanged) into a new file and
re-export or import them so tests still compile; additionally tighten the
trailing-bytes regression by changing
forwarding_masks_initialize_and_preserves_trailing_bytes and
masked_initialize_with_follow_up to append a second initialize frame (not
session_new_bytes) as the trailing bytes to ensure the test exercises the
first-frame-only behavior (update masked_initialize_with_follow_up
expected/host_stdin construction accordingly and keep mask_acp_initialize_frame
usage unchanged).
In `@src/engine/connection/exec/protocol.rs`:
- Around line 238-249: The helper forward_host_stdin_to_exec_async is
unconditionally pre-reading the first frame via
forward_initial_protocol_frame_async and must instead only do that for ACP
sessions: add a parameter (e.g., protocol_kind or is_acp: bool) to
forward_host_stdin_to_exec_async, propagate this change to its callers, and
conditionally call forward_initial_protocol_frame_async(&mut buffered_stdin,
&mut input).await? only when the session is ACP; for non-ACP sessions skip the
initial-frame rewrite and go straight to tokio::io::copy, then flush/shutdown as
before. Ensure the function signature and all call sites (places that invoke
forward_host_stdin_to_exec_async) are updated to pass the protocol indicator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b1fe6f42-f48f-4aef-b3e0-20593d97887a
📒 Files selected for processing (7)
docs/execplans/2-6-1-intercept-acp-initialization.mddocs/podbot-design.mddocs/podbot-roadmap.mddocs/users-guide.mdsrc/engine/connection/exec/protocol.rssrc/engine/connection/exec/protocol_acp_tests.rstests/features/acp_capability_masking.feature
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/engine/connection/exec/protocol_acp_tests.rs (1)
1-484: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffSplit this test module to stay within the 400-line limit.
The file is 484 lines, exceeding the repository's 400-line budget. Extract the BDD scenario infrastructure (lines 364–483) into a sibling module such as
protocol_acp_bdd_tests.rs, keeping the unit tests and forwarding tests in this file.Proposed split
Create
src/engine/connection/exec/protocol_acp_bdd_tests.rs://! BDD scenarios for ACP capability masking. use super::protocol_acp_tests::{ AcpMaskingState, initialize_frame, initialize_without_blocked_capabilities, malformed_initialize_bytes, masked_initialize_with_follow_up, run_forwarding, session_new_bytes, }; // ... move lines 364-483 hereUpdate
protocol_acp_tests.rsto expose necessary items aspub(super)and add:#[cfg(test)] #[path = "protocol_acp_bdd_tests.rs"] mod bdd_tests;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/connection/exec/protocol_acp_tests.rs` around lines 1 - 484, The test module exceeds the 400-line limit; split the BDD scenario infrastructure into a sibling file and export the helper symbols so the moved scenarios can reference them: create src/engine/connection/exec/protocol_acp_bdd_tests.rs and move the rstest_bdd ScenarioState, given/when/then functions and the three #[scenario] functions (the BDD wiring that currently starts around masked_initialize_with_follow_up usage through the three scenario functions), leaving the unit tests and forwarding tests in the original file; make the helpers and types used by the moved file pub(super) — e.g. AcpMaskingState, initialize_frame, initialize_without_blocked_capabilities, malformed_initialize_bytes, masked_initialize_with_follow_up, run_forwarding, session_new_bytes, and any fixtures — so the new module can import them via super::protocol_acp_tests, and add in the original file a test-only include: #[cfg(test)] #[path = "protocol_acp_bdd_tests.rs"] mod bdd_tests; to wire the moved scenarios back into the test suite.
🤖 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/developers-guide.md`:
- Around line 360-367: Replace the British-spelled word "initialisation" with
Oxford spelling "initialization" in the section titled "ACP initialisation
rewrite contract" (update both occurrences in the header and the body where the
phrase appears) so the document uses "initialization" consistently; ensure no
other occurrences of "initialisation" remain in that paragraph and preserve
surrounding punctuation and capitalization.
---
Outside diff comments:
In `@src/engine/connection/exec/protocol_acp_tests.rs`:
- Around line 1-484: The test module exceeds the 400-line limit; split the BDD
scenario infrastructure into a sibling file and export the helper symbols so the
moved scenarios can reference them: create
src/engine/connection/exec/protocol_acp_bdd_tests.rs and move the rstest_bdd
ScenarioState, given/when/then functions and the three #[scenario] functions
(the BDD wiring that currently starts around masked_initialize_with_follow_up
usage through the three scenario functions), leaving the unit tests and
forwarding tests in the original file; make the helpers and types used by the
moved file pub(super) — e.g. AcpMaskingState, initialize_frame,
initialize_without_blocked_capabilities, malformed_initialize_bytes,
masked_initialize_with_follow_up, run_forwarding, session_new_bytes, and any
fixtures — so the new module can import them via super::protocol_acp_tests, and
add in the original file a test-only include: #[cfg(test)] #[path =
"protocol_acp_bdd_tests.rs"] mod bdd_tests; to wire the moved scenarios back
into the test suite.
🪄 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 Plus
Run ID: 81442c05-1e4f-41a5-ba9f-4321365dde85
📒 Files selected for processing (6)
docs/developers-guide.mddocs/execplans/2-6-1-intercept-acp-initialization.mdsrc/engine/connection/exec/acp_helpers.rssrc/engine/connection/exec/protocol.rssrc/engine/connection/exec/protocol_acp_tests.rssrc/engine/connection/exec/session.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 chainLet me inspect the current state of the relevant files. 🏁 Script executed: Length of output: 6040
|
lodyai
Bot
force-pushed
the
implement-acp-masking-jabwf2
branch
from
April 30, 2026 20:32
c9ff729 to
a2eb186
Compare
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/engine/connection/exec/session.rs`:
- Around line 33-38: The setter with_acp_initialize_rewrite_enabled on
ExecSessionOptions is test-gated while the downstream protocol_session_options()
and ProtocolSessionOptions::with_acp_initialize_rewrite_enabled are public,
causing inconsistency; fix by choosing one of two coherent approaches: either
remove #[cfg(test)] from the
ExecSessionOptions::with_acp_initialize_rewrite_enabled method (and ensure the
rewrite_acp_initialize field is publically usable) so production code can toggle
the flag, or apply #[cfg(test)] consistently by gating the
rewrite_acp_initialize field, the with_acp_initialize_rewrite_enabled setter,
protocol_session_options() conversion logic, and
ProtocolSessionOptions::with_acp_initialize_rewrite_enabled() behind
#[cfg(test)] so the entire feature is test-only.
🪄 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 Plus
Run ID: e7e15b2b-3c3f-4f7b-b840-55c12f8a2267
📒 Files selected for processing (8)
docs/developers-guide.mddocs/execplans/2-6-1-intercept-acp-initialization.mddocs/users-guide.mdsrc/engine/connection/exec/acp_helpers.rssrc/engine/connection/exec/protocol.rssrc/engine/connection/exec/protocol_acp_bdd_tests.rssrc/engine/connection/exec/protocol_acp_tests.rssrc/engine/connection/exec/session.rs
lodyai
Bot
force-pushed
the
implement-acp-masking-jabwf2
branch
from
April 30, 2026 22:31
a2eb186 to
22cb62b
Compare
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
♻️ Duplicate comments (1)
src/engine/connection/exec/protocol.rs (1)
179-186:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winEnable ACP initialization rewriting in non-test builds.
Fix Line 182. The
#[cfg(not(test))]branch hard-codesrewrite_acp_initialize
tofalse, so production never executes first-frame masking and the ACP seam
contract is not enforced outside tests.🔧 Proposed fix
#[cfg(test)] let rewrite_acp_initialize = options.rewrite_acp_initialize; #[cfg(not(test))] - let rewrite_acp_initialize = false; + let rewrite_acp_initialize = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/connection/exec/protocol.rs` around lines 179 - 186, The code currently forces rewrite_acp_initialize to false in non-test builds, preventing ACP first-frame masking; change the assignment so rewrite_acp_initialize is always initialized from options.rewrite_acp_initialize (i.e., remove the #[cfg(not(test))] branch and use let rewrite_acp_initialize = options.rewrite_acp_initialize), ensuring spawn_stdin_forwarding_task and its call to forward_host_stdin_to_exec_async receive the real setting in production.
🤖 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/execplans/2-6-1-intercept-acp-initialization.md`:
- Around line 194-197: Update the documentation references so they point to the
extracted ACP helper and BDD binding modules: change mentions of the old masking
helpers to refer to the ACP masking helpers module and the BDD bindings to refer
to protocol_acp_bdd_tests; ensure the ExecPlan text reflects the shipped module
split (i.e., call out the separate ACP helper module and the
protocol_acp_bdd_tests BDD bindings) and apply the same adjustments to the other
occurrences of these module references in the document.
---
Duplicate comments:
In `@src/engine/connection/exec/protocol.rs`:
- Around line 179-186: The code currently forces rewrite_acp_initialize to false
in non-test builds, preventing ACP first-frame masking; change the assignment so
rewrite_acp_initialize is always initialized from options.rewrite_acp_initialize
(i.e., remove the #[cfg(not(test))] branch and use let rewrite_acp_initialize =
options.rewrite_acp_initialize), ensuring spawn_stdin_forwarding_task and its
call to forward_host_stdin_to_exec_async receive the real setting in production.
🪄 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 Plus
Run ID: f4f0d0d9-1ce5-4d5e-b0ad-dec8bbdad893
📒 Files selected for processing (8)
docs/developers-guide.mddocs/execplans/2-6-1-intercept-acp-initialization.mddocs/users-guide.mdsrc/engine/connection/exec/acp_helpers.rssrc/engine/connection/exec/protocol.rssrc/engine/connection/exec/protocol_acp_bdd_tests.rssrc/engine/connection/exec/protocol_acp_tests.rssrc/engine/connection/exec/session.rs
Move ACP initialize masking helpers out of the protocol proxy module so the proxy loop stays focused and under the file-length budget. Keep ACP first-frame rewriting behind a protocol-session option so generic protocol sessions retain raw stdin forwarding semantics unless ACP masking is selected. Split ACP BDD scenario wiring out of the unit-test module, extend coverage for the first-frame-only contract, document the current hosted-mode ACP masking scope, and add the helper documentation and tracing needed by the review gates.
lodyai
Bot
force-pushed
the
implement-acp-masking-jabwf2
branch
from
May 1, 2026 21:49
5faeb10 to
9a6a1bd
Compare
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/engine/connection/exec/protocol_acp_tests.rs (1)
1-523: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit this test module back under the file-length budget.
Move the frame builders, assertion helpers, or forwarding cases into sibling
test modules.src/engine/connection/exec/protocol_acp_tests.rsis 523 lines
long, so it still breaches the repository’s 400-line limit and will keep
attracting unrelated changes into one test blob.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/engine/connection/exec/protocol_acp_tests.rs` around lines 1 - 523, This test module exceeds the 400-line limit; split it by moving reusable helpers and builders (RecordingInputWriter, initialize_frame, initialize_without_blocked_capabilities, initialize_with_only_blocked_capabilities, malformed_initialize_bytes, session_new_bytes, parse_frame_payload, client_capabilities, params, assert_masked_client_capabilities, masked_initialize_with_follow_up) into one or more sibling test modules (e.g. protocol_acp_helpers.rs) and leave only the forwarding/tests that exercise behavior (tests using run_forwarding, run_forwarding_with_rewrite, forward_initial_acp_frame_async, mask_acp_initialize_frame, MAX_FIRST_FRAME_BYTES, STDIN_BUFFER_CAPACITY, STDIN_SETTLE_TIMEOUT) in protocol_acp_tests.rs; update module declarations (use/super::acp_helpers or mod entries such as the existing bdd_tests) so tests reference the moved symbols and ensure public visibility where needed.
🤖 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/developers-guide.md`:
- Around line 362-368: Update the paragraph describing the ACP initialization
rewrite to match the implementation in
src/engine/connection/exec/acp_helpers.rs: state that when protocol tests wrap
run_session with RecordingWriter handles the rewrite is only triggered if the
first newline-delimited ACP initialize frame contains blocked entries under
params.clientCapabilities (specifically terminal and/or fs); the rewrite must
remove those capability families from params.clientCapabilities and, if that
removal leaves clientCapabilities empty, remove the entire clientCapabilities
object; preserve original line endings and allow malformed or non-ACP frames to
pass through unchanged.
In `@docs/users-guide.md`:
- Around line 67-73: Update the paragraph describing hosted-mode ACP masking to
explicitly state that Podbot inspects only the first newline-delimited ACP
"initialize" frame and that it rewrites/masks `terminal/*` and `fs/*`
capabilities only for that first frame on the hosted ACP path; also document
that malformed or non-ACP first frames are forwarded unchanged, and that this
behavior is specific to the hosted-mode/ACP path (until `podbot host` is
implemented) rather than applying to every ACP initialize request.
---
Outside diff comments:
In `@src/engine/connection/exec/protocol_acp_tests.rs`:
- Around line 1-523: This test module exceeds the 400-line limit; split it by
moving reusable helpers and builders (RecordingInputWriter, initialize_frame,
initialize_without_blocked_capabilities,
initialize_with_only_blocked_capabilities, malformed_initialize_bytes,
session_new_bytes, parse_frame_payload, client_capabilities, params,
assert_masked_client_capabilities, masked_initialize_with_follow_up) into one or
more sibling test modules (e.g. protocol_acp_helpers.rs) and leave only the
forwarding/tests that exercise behavior (tests using run_forwarding,
run_forwarding_with_rewrite, forward_initial_acp_frame_async,
mask_acp_initialize_frame, MAX_FIRST_FRAME_BYTES, STDIN_BUFFER_CAPACITY,
STDIN_SETTLE_TIMEOUT) in protocol_acp_tests.rs; update module declarations
(use/super::acp_helpers or mod entries such as the existing bdd_tests) so tests
reference the moved symbols and ensure public visibility where needed.
🪄 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 Plus
Run ID: 639a0370-8734-4166-b340-e1d98642f39a
📒 Files selected for processing (8)
docs/developers-guide.mddocs/execplans/2-6-1-intercept-acp-initialization.mddocs/users-guide.mdsrc/engine/connection/exec/acp_helpers.rssrc/engine/connection/exec/protocol.rssrc/engine/connection/exec/protocol_acp_bdd_tests.rssrc/engine/connection/exec/protocol_acp_tests.rssrc/engine/connection/exec/session.rs
Contributor
✅ Actions performedReviews resumed. |
Define the initialize-frame skeleton once and pass only the capability payload into tests that vary ACP client capabilities. Move forwarding-specific ACP tests into a child module so the main ACP test file stays below the repository line budget while preserving the same private test helpers.
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/engine/connection/exec/protocol_acp_tests.rs (1)
1-523: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit this test module below the 400-line budget.
Bring this file back under the repository limit. It currently packs fixtures,
frame builders, forwarding harnesses, unit tests, shared BDD helpers, and the
BDD module include into 523 lines, which makes the ACP coverage harder to
maintain than the surrounding exec test layout.Move the reusable frame builders and forwarding harness into a sibling helper
module, or move the exported BDD fixtures into the BDD file so this unit-test
module stays focused.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/engine/connection/exec/protocol_acp_tests.rs` around lines 1 - 523, This module exceeds the 400-line limit because it mixes reusable fixtures/harnesses with unit tests; split out the reusable frame builders and forwarding harness into a sibling helper module (or move the exported BDD fixtures into protocol_acp_bdd_tests.rs). Specifically, extract RecordingInputWriter, initialize_frame, initialize_without_blocked_capabilities, initialize_with_only_blocked_capabilities, session_new_bytes, malformed_initialize_bytes, parse_frame_payload, assert_masked_client_capabilities, client_capabilities, params, build_host_stdin, run_forwarding, run_forwarding_with_rewrite, and masked_initialize_with_follow_up into a new helper module (e.g., protocol_acp_helpers.rs) and update imports so the remaining tests only contain test functions; alternatively move masked_initialize_with_follow_up and any pub(super) fixtures into protocol_acp_bdd_tests.rs and remove their definitions here to bring this file under 400 lines.
♻️ Duplicate comments (1)
docs/developers-guide.md (1)
362-368:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the narrower ACP rewrite trigger and the empty-object elision.
Mirror the shipped behaviour here. The implementation only rewrites when the
first ACPinitializeframe carries blockedterminaland/orfsentries, and
it dropsparams.clientCapabilitiesentirely when masking leaves it empty. The
current wording still reads as though anyinitializewith
params.clientCapabilitiesis rewritten.Triage:
[type:docstyle]✏️ Suggested edit
-When protocol tests wrap `run_session` with `RecordingWriter` handles, they -must preserve the Agentic Control Protocol (ACP) initialization rewrite -contract: inspect only the first newline-delimited frame, rewrite only ACP -`initialize` requests that expose `params.clientCapabilities`, and remove the -`terminal` and `fs` capability families before the frame reaches the container. -Preserve the original line endings, and let malformed or non-ACP frames pass -through unchanged. +When protocol tests wrap `run_session` with `RecordingWriter` handles, they +must preserve the Agentic Control Protocol (ACP) initialization rewrite +contract: inspect only the first newline-delimited frame, rewrite only ACP +`initialize` requests whose `params.clientCapabilities` contain blocked +`terminal` and/or `fs` entries, remove those capability families before the +frame reaches the container, and drop `clientCapabilities` entirely when +masking leaves it empty. Preserve the original line endings, and let malformed +or non-ACP frames pass through unchanged.As per coding guidelines, "New or changed internal APIs, architectural
boundaries or abstractions, tooling or build requirements must be clearly
documented in the developer's guide (docs/developers-guide.md)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/developers-guide.md` around lines 362 - 368, The documentation wording is too broad: update the paragraph describing the ACP initialization rewrite to match shipped behavior by specifying that RecordingWriter-wrapped run_session should inspect only the first newline-delimited frame and rewrite only ACP "initialize" requests whose params.clientCapabilities contain blocked "terminal" and/or "fs" entries; remove those capability families before the frame reaches the container and if masking leaves params.clientCapabilities (or clientCapabilities) empty, drop the entire clientCapabilities object; preserve original line endings and allow malformed or non-ACP frames to pass through 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 `@docs/users-guide.md`:
- Around line 66-73: The paragraph in docs/users-guide.md that begins "When
hosted mode uses Agentic Control Protocol (ACP)..." disables MD013 and exceeds
the 80-column wrap rule; remove the HTML suppression and reflow the paragraph to
wrap at or under 80 columns so lines like the ones mentioning ACP, `terminal/*`,
`fs/*`, and `podbot host` are split into shorter lines (for example breaking
after "masks", "initialize", "currently", and "implemented") while preserving
wording and punctuation.
In `@src/engine/connection/exec/protocol_acp_tests.rs`:
- Around line 446-473: The test
forwarding_does_not_wait_indefinitely_for_oversized_initial_frame uses the
production STDIN_SETTLE_TIMEOUT as the tokio::time::timeout deadline; introduce
a stable test-specific timeout (e.g., let test_timeout =
std::time::Duration::from_secs(1)) and use that instead of STDIN_SETTLE_TIMEOUT
when calling tokio::time::timeout for forward_initial_acp_frame_async so the CI
assertion budget is decoupled from production shutdown timing.
---
Outside diff comments:
In `@src/engine/connection/exec/protocol_acp_tests.rs`:
- Around line 1-523: This module exceeds the 400-line limit because it mixes
reusable fixtures/harnesses with unit tests; split out the reusable frame
builders and forwarding harness into a sibling helper module (or move the
exported BDD fixtures into protocol_acp_bdd_tests.rs). Specifically, extract
RecordingInputWriter, initialize_frame, initialize_without_blocked_capabilities,
initialize_with_only_blocked_capabilities, session_new_bytes,
malformed_initialize_bytes, parse_frame_payload,
assert_masked_client_capabilities, client_capabilities, params,
build_host_stdin, run_forwarding, run_forwarding_with_rewrite, and
masked_initialize_with_follow_up into a new helper module (e.g.,
protocol_acp_helpers.rs) and update imports so the remaining tests only contain
test functions; alternatively move masked_initialize_with_follow_up and any
pub(super) fixtures into protocol_acp_bdd_tests.rs and remove their definitions
here to bring this file under 400 lines.
---
Duplicate comments:
In `@docs/developers-guide.md`:
- Around line 362-368: The documentation wording is too broad: update the
paragraph describing the ACP initialization rewrite to match shipped behavior by
specifying that RecordingWriter-wrapped run_session should inspect only the
first newline-delimited frame and rewrite only ACP "initialize" requests whose
params.clientCapabilities contain blocked "terminal" and/or "fs" entries; remove
those capability families before the frame reaches the container and if masking
leaves params.clientCapabilities (or clientCapabilities) empty, drop the entire
clientCapabilities object; preserve original line endings and allow malformed or
non-ACP frames to pass through 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 Plus
Run ID: afbc5542-dc3c-4b0f-a56b-6caca6b38129
📒 Files selected for processing (8)
docs/developers-guide.mddocs/execplans/2-6-1-intercept-acp-initialization.mddocs/users-guide.mdsrc/engine/connection/exec/acp_helpers.rssrc/engine/connection/exec/protocol.rssrc/engine/connection/exec/protocol_acp_bdd_tests.rssrc/engine/connection/exec/protocol_acp_tests.rssrc/engine/connection/exec/session.rs
Reflow the hosted-mode ACP user-guide paragraph without suppressing Markdown line-length checks. Narrow the developer guide contract to the shipped first-frame initialize rewrite behaviour, including empty capability object removal. Use a test-specific timeout in the oversized ACP frame regression so CI timing is decoupled from production stdin settle timing.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Have the following now been resolved? src/engine/connection/exec/protocol_acp_tests.rs (1)
frame builders, forwarding harnesses, unit tests, shared BDD helpers, and the BDD module include into 523 lines, which makes the ACP coverage harder to maintain than the surrounding exec test layout. module, or move the exported BDD fixtures into the BDD file so this unit-test module stays focused. 🤖 Prompt for AI Agents |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2629
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Demote recoverable ACP frame parse and serialization failures to debug logs so valid pass-through cases do not appear as operational warnings. Document the per-session stdin forwarding ownership model and cancellation behaviour in the ACP helper module.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/engine/connection/exec/acp_helpers.rs (1)
5-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize the remaining doc spelling to Oxford
-ize.Replace
synchronisation,initialisation,re-serialises, andserialisationwithsynchronization,initialization,re-serializes, andserializationto match the repository rule.✍️ Proposed fix
-//! and no synchronisation primitives are required. +//! and no synchronization primitives are required. ... -/// The JSON-RPC `method` value that identifies an ACP initialisation request. +/// The JSON-RPC `method` value that identifies an ACP initialization request. ... -/// `params.clientCapabilities`, then re-serialises the JSON preserving the +/// `params.clientCapabilities`, then re-serializes the JSON preserving the ... -/// serialisation failure, or if no capabilities were removed. +/// serialization failure, or if no capabilities were removed.As per coding guidelines, "Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar".
Also applies to: 25-25, 176-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/connection/exec/acp_helpers.rs` around lines 5 - 10, Update the top-level module doc comment in acp_helpers.rs (the crate-level/comment block containing the forwarding task description) to use Oxford -ize spellings: replace "synchronisation" with "synchronization", "initialisation" with "initialization", "re-serialises" with "re-serializes", and "serialisation" with "serialization"; apply the same substitutions to the other occurrences noted (around lines ~25 and ~176-179) so all documentation in this file follows the en-GB-oxendict (-ize) rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/engine/connection/exec/acp_helpers.rs`:
- Around line 5-10: Update the top-level module doc comment in acp_helpers.rs
(the crate-level/comment block containing the forwarding task description) to
use Oxford -ize spellings: replace "synchronisation" with "synchronization",
"initialisation" with "initialization", "re-serialises" with "re-serializes",
and "serialisation" with "serialization"; apply the same substitutions to the
other occurrences noted (around lines ~25 and ~176-179) so all documentation in
this file follows the en-GB-oxendict (-ize) rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 43deb9d6-e2d9-4376-9f3f-d8d1aaf6b904
📒 Files selected for processing (1)
src/engine/connection/exec/acp_helpers.rs
Owner
Author
|
@coderabbitai pause |
Contributor
✅ Actions performedReviews paused. |
Update ACP helper documentation and the related Clippy expectation reason to use Oxford -ize spellings consistently across initialization, serialization, and synchronization wording.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 16837
Status of each check
No further in-scope work remains for this PR. All residual failures are covered by open issues |
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. ❌ Failed checks (5 warnings)
|
This comment was marked as resolved.
This comment was marked as resolved.
5 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
Changes
Core functionality
Tests
Tests scaffolding
Documentation & Roadmap
Misc
Why this matters
Validation & Testing
Potential follow-ups
◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/afef9711-3240-4f69-8b15-8bbb229d1ab0
Summary by Sourcery
Implement ACP capability masking at the protocol stdin proxy seam while documenting and validating the behaviour with unit, BDD, and roadmap updates.
New Features:
initializerequests in protocol mode to removeterminal/*andfs/*capabilities before forwarding to the container while preserving non-ACP traffic semantics.Enhancements:
Documentation:
Tests: