Plan ACP runtime method denylist enforcement (2.6.2)

[lodyai]

Summary

• Drafts docs/execplans/2-6-2-runtime-denylist.md, the governing execution plan for Step 2.6.2 of docs/podbot-roadmap.md: enforce a runtime denylist for blocked Agentic Control Protocol (ACP) methods after initialisation.
• Extends the protocol proxy seam established by 2.6.1 with a pure policy module (acp_policy), a newline-bounded frame assembler (acp_frame), and an output-direction adapter plus dedicated container-stdin sink task (acp_runtime). The two prior boolean opt-ins collapse into a single CapabilityPolicy::{Disabled, MaskOnly, MaskAndDeny} enum.
• Bundles the synthesised JSON-RPC error response (code -32001) and the stderr denial line into 2.6.2 because enforcement without a response would hang hosted agents. Subsequent roadmap items 2.6.3 (richer diagnostics), 2.6.4 (operator override), and 2.6.5 (override-test battery) remain explicitly out of scope.
• Incorporates a Logisphere pre-implementation review (structure, alternatives, scale, JSON-RPC contract, failure modes, long-term viability) and adds a Stage A.5 architecture spike comparing the sink-task model against a single bidirectional tokio::select! task before committing to either.

Status

DRAFT — this PR is the planning artefact only. No production code or tests are added. Implementation begins after the plan is approved.

Test plan

• Reviewer reads docs/execplans/2-6-2-runtime-denylist.md end-to-end and confirms the scope, constraints, tolerances, and decision log match intent.
• Reviewer confirms the architecture spike (Stage A.5) is the right gating point before committing to the dedicated sink task vs the single bidirectional task.
• Reviewer confirms the JSON-RPC error code choice (-32001 with data.reason = "podbot_capability_policy") and the trailing-slash prefix matching for terminal/ and fs/.
• Reviewer confirms CapabilityPolicy enum collapsing the two prior booleans is acceptable as an internal API change.
• make markdownlint already runs clean on the new file (verified locally).

🤖 Generated with Claude Code

Summary by Sourcery

Plan and partially wire a new ACP runtime method denylist enforcement path, introducing capability policy selection, runtime ACP framing and policy modules, and documenting the architecture and behaviour across developer, design, and user guides.

New Features:

• Introduce an ACP runtime enforcement path that inspects agent-emitted ACP frames, blocks denied method families, and routes synthesized JSON-RPC error responses back to the agent via a dedicated container-stdin sink.
• Add a CapabilityPolicy enum to control ACP behaviour per exec session, supporting disabled, initialization-only masking, and combined mask-and-deny modes.

Enhancements:

• Refine ACP initialization masking by factoring out a reusable frame-reading helper, allowing both direct forwarding and channel-based stdin pipelines to share the same logic.
• Extend the protocol exec session wiring to choose between byte-transparent and runtime-enforced ACP paths based on capability policy, while preserving existing stdout/stderr and buffering contracts.
• Document the new ACP runtime denylist architecture, observability expectations, and testing approach in the developers guide and design docs, including metrics and tracing contracts for future work.

Documentation:

• Add a detailed execution plan for ACP runtime denylist enforcement (Step 2.6.2), covering constraints, architecture, policy rules, and validation strategy.
• Update the design and user guides to describe ACP runtime denylist semantics, JSON-RPC error shape, and operator-visible behaviour when blocked methods are attempted.

Tests:

• Add focused unit tests for ACP policy decisions, runtime frame assembly, and the runtime adapter plus sink task, including edge cases around framing, buffer overflow, and partial frames.
• Introduce BDD-style feature scenarios and bindings to assert end-to-end ACP runtime denylist behaviour, covering blocked requests, notifications, permitted methods, and multi-chunk framing.

[sourcery-ai]

Sorry @LodyAI[bot], you have reached your weekly rate limit of 2500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c973dc39-1faa-4834-9f9c-719a2ab15c81

📥 Commits

Reviewing files that changed from the base of the PR and between c9d13e0 and 21ea3f7.

📒 Files selected for processing (1)

• src/engine/connection/exec/protocol_acp_tests.rs

---

Walkthrough

Add Step 2.6.2 ExecPlan and implement ACP runtime denylist: policy module, newline-frame assembler with overflow/raw-fallback rules, runtime adapter with stdin sink and synthesized denial responses, CapabilityPolicy wiring, and unit/BDD/integration tests; update docs and feature scenarios.

Changes

Runtime Denylist ExecPlan

Layer / File(s)	Summary
Policy: method-family matching & error builder src/engine/connection/exec/acp_policy.rs, src/engine/connection/exec/acp_policy_tests.rs	Add MethodFamily, MethodDenylist, FrameDecision, evaluate_agent_outbound_frame, JSON-RPC error constants and build_method_blocked_error; unit tests for matching, decisions and error payloads.
Framer: streaming assembler & overflow handling src/engine/connection/exec/acp_frame.rs, src/engine/connection/exec/acp_frame_tests.rs	Add OutboundFrameAssembler with MAX_RUNTIME_FRAME_BYTES=131_072, FrameOutput/FallbackReason, ingest_chunk/finish semantics, raw-fallback on overflow, residual partial-frame drop at EOS, and exhaustive split/reassembly tests.
Runtime adapter & stdin sink src/engine/connection/exec/acp_runtime.rs, src/engine/connection/exec/acp_runtime_tests.rs, src/engine/connection/exec/acp_runtime_bdd_tests.rs	Add WriteCmd, run_container_stdin_sink, OutboundPolicyAdapter; synthesise JSON-RPC errors for blocked requests queued to stdin sink, forward permitted bytes to host stdout, handle BrokenPipe, and test ordering/flush-before-shutdown behaviour plus BDD scenarios.
Protocol/session wiring & helpers src/engine/connection/exec/protocol.rs, src/engine/connection/exec/session.rs, src/engine/connection/exec/acp_helpers.rs, src/engine/connection/exec/mod.rs, src/engine/connection/exec/protocol_acp_tests.rs, src/engine/connection/exec/protocol_acp_policy_integration_tests.rs	Replace boolean rewrite flag with CapabilityPolicy enum (Disabled, MaskOnly, MaskAndDeny); add runtime-enforced IO path that spawns stdin sink and uses OutboundPolicyAdapter; refactor initial-frame masking into read_and_mask_initial_acp_frame; update imports and module declarations; add integration tests.
Tests, BDD and documentation tests/features/acp_method_denylist.feature, docs/execplans/2-6-2-runtime-denylist.md, docs/podbot-design.md, docs/users-guide.md, docs/developers-guide.md	Add Gherkin feature, ExecPlan doc for Step 2.6.2, update design/user/developer docs describing framing, policy modes, stderr diagnostics, metrics contract, and test expectations.

Sequence Diagram(s)

sequenceDiagram
  participant Agent as Container stdout
  participant Adapter as OutboundPolicyAdapter
  participant Host as Host stdout
  participant Sink as StdinSink (run_container_stdin_sink)
  participant Container as Container stdin

  Agent->>Adapter: chunked stdout bytes
  Adapter->>Adapter: assemble frames (OutboundFrameAssembler)
  Adapter->>Adapter: call evaluate_agent_outbound_frame
  alt Forward
    Adapter->>Host: write bytes verbatim
  else BlockRequest
    Adapter->>Sink: enqueue Synthesised JSON-RPC error (preserve id & line ending)
    Sink->>Container: write & flush synthesized bytes (ordered)
  else BlockNotification
    Adapter->>Adapter: drop notification (no sink enqueue)
  end
  Sink->>Container: tolerate BrokenPipe, log once, drain remaining commands

Loading

Possibly related issues

• ACP masking: extract CapabilityRestrictionPolicy domain abstraction #86 — Extract CapabilityPolicy and move ACP options into session/protocol; this PR introduces CapabilityPolicy.
• Step 2.6.3: Add observability metrics for ACP runtime denylist enforcement #98 — Document ACP denylist observability/metrics; this PR adds the docs/observability contract but does not implement metrics.

Possibly related PRs

• leynos/podbot#79 — Prior work on ACP initialize-frame masking and rewiring; closely related to protocol/acp_helpers changes here.

Poem

A runtime gate where frames align,
Mask at start, deny at line,
Stages chart the code’s design,
Frame by frame, the proxy signs,
Guard the protocol, keep it fine.

You are the reviewer. Provide concise, targeted review comments referencing the hidden review stack ranges when requesting code changes.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 2-6-2-runtime-denylist

[coderabbitai]

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 `@docs/execplans/2-6-2-runtime-denylist.md`:
- Around line 270-271: The sentence uses first-person wording "in our proxy";
update the text so it uses neutral phrasing (e.g., replace "in our proxy" with
"in the protocol proxy" or "in the proxy") in the sentence that reads "so the
bytes flow agent stdout to host stdout in our proxy" to remove personal pronouns
and match repository documentation tone rules.
- Around line 519-520: The doc uses two different helper names for the same
concept—`allows_runtime_enforcement` and `enforces_runtime_denylist`—which is
confusing; pick a single canonical name (suggest `allows_runtime_enforcement`)
and update all mentions in Stage E, the "Interfaces and dependencies" section,
and the other occurrences around lines 1018–1020 to use that name consistently,
and ensure the helper signatures shown (e.g., `pub(crate) const fn
allows_runtime_enforcement(self) -> bool` and `pub(crate) const fn
rewrites_initialize(self) -> bool`) reflect the chosen name everywhere in the
document.
- Line 21: The document uses Oxford '-ise' spellings; update all occurrences to
the OED '-ize' forms (e.g., change initialisation→initialization,
synthesised→synthesized, parameterised→parameterized, parallelise→parallelize,
optimisation→optimization) across this file (including the other referenced
locations). Search for the specific tokens "initialisation", "synthesised",
"parameterised", "parallelise", "optimisation" and replace with their '-ize'
counterparts, and run a pass for other '-ise' suffixed words to convert them to
'-ize' to comply with the en-GB-oxendict rule. Ensure changes preserve
surrounding punctuation and capitalization (e.g., Initialisation→Initialization)
and update all listed lines (35, 62, 90, 540, 630, 769) plus any additional
occurrences.

🪄 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: 2ac0614e-8061-4174-a50c-33d00fa353c6

📥 Commits

Reviewing files that changed from the base of the PR and between 150ef83 and 6ebecad.

📒 Files selected for processing (1)

• docs/execplans/2-6-2-runtime-denylist.md

[leynos]

@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/engine/connection/exec/acp_runtime_bdd_tests.rs

Comment on lines +69 to +79

fn permitted_request_frame(method: &str, id: i64) -> Vec<u8> {
    let mut bytes = serde_json::to_vec(&serde_json::json!({
        "jsonrpc": "2.0",
        "id": id,
        "method": method,
        "params": {},
    }))
    .expect("permitted request serializes");
    bytes.push(b'\n');
    bytes
}

❌ New issue: Code Duplication
The module contains 5 functions with similar structure: assert_host_stdout_matches_permitted,assert_host_stdout_matches_permitted_after_blocked,blocked_notification_frame,blocked_request_frame and 1 more functions

[leynos]

@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/engine/connection/exec/acp_runtime_tests.rs

Comment on lines +85 to +95

fn permitted_frame() -> Vec<u8> {
    let mut bytes = serde_json::to_vec(&serde_json::json!({
        "jsonrpc": "2.0",
        "id": 1,
        "method": "session/new",
        "params": {},
    }))
    .expect("permitted frame serializes");
    bytes.push(b'\n');
    bytes
}

❌ New issue: Code Duplication
The module contains 3 functions with similar structure: blocked_notification_frame,blocked_request_frame,permitted_frame

[leynos]

@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/engine/connection/exec/acp_frame.rs

Comment on lines +100 to +142

    pub(crate) fn ingest_chunk(
        &mut self,
        chunk: &[u8],
    ) -> (Vec<FrameOutput>, Option<FallbackReason>) {
        if self.raw_fallback {
            return (
                if chunk.is_empty() {
                    Vec::new()
                } else {
                    vec![FrameOutput::Forward(chunk.to_vec())]
                },
                None,
            );
        }

        let mut outputs = Vec::new();
        let mut fallback = None;
        let mut cursor = 0;

        while cursor < chunk.len() {
            let remaining = chunk.get(cursor..).unwrap_or(&[]);
            if let Some(newline_offset) = remaining.iter().position(|byte| *byte == b'\n') {
                let frame_end = cursor + newline_offset + 1;
                let frame_slice = chunk.get(cursor..frame_end).unwrap_or(&[]);
                outputs.push(self.complete_frame(frame_slice));
                cursor = frame_end;
                continue;
            }

            // No newline in the rest of the chunk; append and stop.
            let pending = chunk.get(cursor..).unwrap_or(&[]);
            if self.buffer.len() + pending.len() > MAX_RUNTIME_FRAME_BYTES {
                outputs.push(self.flush_buffer_for_overflow(pending));
                fallback = Some(FallbackReason::BufferOverflow);
                self.raw_fallback = true;
                break;
            }
            self.buffer.extend_from_slice(pending);
            break;
        }

        (outputs, fallback)
    }

❌ New issue: Bumpy Road Ahead
OutboundFrameAssembler.ingest_chunk has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function

[leynos]

@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/engine/connection/exec/protocol.rs

Comment on lines +290 to +324

async fn forward_host_stdin_to_channel<HostStdin>(
    host_stdin: HostStdin,
    sender: tokio::sync::mpsc::Sender<WriteCmd>,
    rewrite_acp_initialize: bool,
) -> io::Result<()>
where
    HostStdin: AsyncRead + Unpin,
{
    use tokio::io::AsyncReadExt;

    let mut buffered_stdin = tokio::io::BufReader::with_capacity(STDIN_BUFFER_CAPACITY, host_stdin);

    if rewrite_acp_initialize {
        let bytes = acp_helpers::read_and_mask_initial_acp_frame(&mut buffered_stdin).await?;
        if !bytes.is_empty() && sender.send(WriteCmd::Forward(bytes)).await.is_err() {
            return Ok(());
        }
    }

    let mut buf = vec![0u8; STDIN_BUFFER_CAPACITY];
    loop {
        let bytes_read = buffered_stdin.read(&mut buf).await?;
        if bytes_read == 0 {
            break;
        }
        let chunk = buf
            .get(..bytes_read)
            .map(<[u8]>::to_vec)
            .unwrap_or_default();
        if sender.send(WriteCmd::Forward(chunk)).await.is_err() {
            break;
        }
    }
    Ok(())
}

❌ New issue: Bumpy Road Ahead
forward_host_stdin_to_channel has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function

[leynos]

@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/engine/connection/exec/acp_policy_tests.rs

Comment on lines +43 to +53

fn jsonrpc_request(id: &Value, method: &str) -> Vec<u8> {
    let payload = serde_json::json!({
        "jsonrpc": "2.0",
        "id": id,
        "method": method,
        "params": {},
    });
    let mut bytes = serde_json::to_vec(&payload).expect("request serializes");
    bytes.push(b'\n');
    bytes
}

❌ New issue: Code Duplication
The module contains 4 functions with similar structure: evaluate_forwards_frame_when_method_not_string,evaluate_forwards_response_without_method_field,jsonrpc_notification,jsonrpc_request

[leynos]

@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 (2 warnings)

Check name	Status	Explanation	Resolution
Developer Documentation	⚠️ Warning	PR introduces planned ACP modules but lacks implementation code and required developer guide documentation for new architectural boundaries.	Complete full implementation stages and update docs/developers-guide.md with the three new ACP modules, or clarify this is planning-only with implementation to follow.
Observability	⚠️ Warning	Plan provides adequate stderr logging but lacks metrics observability for bounded mpsc channel, denial rates, queue depth, and frame overflow incidents required by custom check.	Add metrics specifications covering blocked method attempt counters, enforcement policy state gauge, WriteCmd channel queue depth or send-failure metrics, frame buffer overflow counters, and distributed tracing spans before production rollout.

[sourcery-ai]

Reviewer's Guide

Plans and scaffolds ACP runtime method denylist enforcement (Step 2.6.2) by introducing pure policy/assembly/runtime modules, a container-stdin sink architecture, and a new CapabilityPolicy enum, and wires the design plus contracts into developer, design, and user docs while adding minimal code changes to support the future implementation.

Sequence diagram for handling a blocked ACP method at runtime

sequenceDiagram
    participant Agent
    participant CO as ContainerStdout
    participant Out as run_output_loop_with_adapter
    participant Ad as OutboundPolicyAdapter
    participant Fr as OutboundFrameAssembler
    participant Po as acp_policy
    participant Tx as mpsc_Sender_WriteCmd
    participant Sink as run_container_stdin_sink
    participant CSI as ContainerStdin
    participant HS as HostStdout

    Agent->>CO: blocked ACP request frame
    CO->>Out: LogOutput::StdOut{message}
    Out->>Ad: handle_chunk(message)
    Ad->>Fr: ingest_chunk(message)
    Fr-->>Ad: FrameOutput::Decision(BlockRequest, line_ending)
    Ad->>Po: build_method_blocked_error(id, method, line_ending)
    Po-->>Ad: synthesized_error_bytes
    Ad->>Tx: send(WriteCmd::Synthesised(synthesized_error_bytes))
    Tx-->>Sink: WriteCmd::Synthesised
    Sink->>CSI: write_all(bytes) + flush

    Note over Ad,HS: No FrameOutput::Forward emitted -> nothing written to host stdout

Loading

File-Level Changes

Change	Details	Files
Introduce ACP runtime enforcement scaffolding in the protocol exec path with a container-stdin sink and adapter, gated by a new CapabilityPolicy enum.	Expose STDIN_BUFFER_CAPACITY so new helpers can share the same buffer size. Refactor run_protocol_session_with_io_async to split into runtime-enforced vs non-enforced paths using CapabilityPolicy::allows_runtime_enforcement. Add run_session_with_runtime_enforcement that wires a bounded mpsc channel, a dedicated container-stdin sink task, a channel-based stdin forwarder, and an OutboundPolicyAdapter driving the output loop. Change ProtocolSessionOptions to carry a CapabilityPolicy instead of a boolean rewrite_acp_initialize flag and use rewrites_initialize() when constructing stdin forwarders.	src/engine/connection/exec/protocol.rs
Replace the boolean ACP rewrite flag in session options with a CapabilityPolicy enum and associated helpers.	Update ExecSessionOptions to store capability_policy with default Disabled and provide with_capability_policy builder. Update protocol_session_options to propagate CapabilityPolicy instead of the old rewrite flag. Add CapabilityPolicy enum with Disabled, MaskOnly, MaskAndDeny plus rewrites_initialize and allows_runtime_enforcement helpers. Extend unit tests to cover default policy, MaskOnly/MaskAndDeny propagation, and helper semantics.	src/engine/connection/exec/session.rs
Refactor ACP initialization masking helper to return bytes instead of writing directly, so it can be reused by the new stdin-channel pipeline.	Change forward_initial_acp_frame_async to delegate to a new read_and_mask_initial_acp_frame helper and then write returned bytes. Introduce read_and_mask_initial_acp_frame that reads, decides on masking, logs reasons, and returns the bytes to forward instead of writing them immediately. Remove forward_unmodified_initial_frame and forward_masked_initial_frame in favour of the new helper returning Vec.	src/engine/connection/exec/acp_helpers.rs
Add pure ACP runtime policy, frame assembler, and runtime adapter modules plus comprehensive unit and BDD tests for the denylist behaviour.	Create acp_policy with MethodFamily/MethodDenylist, FrameDecision, evaluate_agent_outbound_frame, and build_method_blocked_error (JSON-RPC error code -32001 with data.reason = "podbot_capability_policy"). Create acp_frame with OutboundFrameAssembler, FrameOutput, FallbackReason and a newline-delimited framing buffer capped at MAX_RUNTIME_FRAME_BYTES = 128 KiB with raw-fallback and dropped-partial-frame semantics. Create acp_runtime with WriteCmd enum, SINK_CHANNEL_CAPACITY = 16, run_container_stdin_sink sink task, and OutboundPolicyAdapter that sends synthesized denial responses via the sink and logs tracing::warn! diagnostics. Add focused unit tests for acp_policy, acp_frame, and acp_runtime plus rstest-bdd scenarios in acp_runtime_bdd_tests.rs and feature file acp_method_denylist.feature to validate blocked/permitted behaviour, chunk reassembly, ordering, and error shapes.	src/engine/connection/exec/acp_policy.rs src/engine/connection/exec/acp_policy_tests.rs src/engine/connection/exec/acp_frame.rs src/engine/connection/exec/acp_frame_tests.rs src/engine/connection/exec/acp_runtime.rs src/engine/connection/exec/acp_runtime_tests.rs src/engine/connection/exec/acp_runtime_bdd_tests.rs tests/features/acp_method_denylist.feature
Wire new ACP modules into the exec subsystem and protocol ACP tests.	Register acp_frame, acp_helpers, acp_policy, and acp_runtime as submodules of exec::mod so they are available to protocol and tests. Adjust protocol_acp_tests imports to use the new module layout while still reusing existing helpers.	src/engine/connection/exec/mod.rs src/engine/connection/exec/protocol_acp_tests.rs
Document the ACP runtime denylist design, contracts, and internal architecture, including metrics and observability expectations.	Add a detailed execution plan docs/execplans/2-6-2-runtime-denylist.md describing constraints, risks, architecture (policy/assembler/runtime split, sink task), CapabilityPolicy, and validation strategy. Extend developers-guide with sections on acp_policy, acp_frame, acp_runtime modules, their responsibilities, contracts, observability metrics, and how to test them. Update podbot-design to spell out the runtime denylist behaviour, matching rules for terminal/ and fs/ families, JSON-RPC error contract (-32001, data.reason), sink-task model, buffer overflow / fallback semantics, and byte-identical pass-through guarantees. Update users-guide to describe user-visible behaviour when ACP enforcement is enabled (masked capabilities, runtime denials, JSON-RPC error, stderr warnings) and to clarify that this applies only to hosted/protocol mode until podbot host exists.	docs/execplans/2-6-2-runtime-denylist.md docs/developers-guide.md docs/podbot-design.md docs/users-guide.md docs/execplans/2-6-1-intercept-acp-initialization.md

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8f1b9ca423

ℹ️ 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".

[leynos]

@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.

Please address the comments from this code review:

## Overall Comments
- The execution plan and docs (e.g. references to `WriteCmd::{Forward, Synthesised, Shutdown}` and explicit Shutdown semantics) no longer match the implementation where `WriteCmd` has only `Forward`/`Synthesised` and the sink terminates on channel close; either reintroduce the shutdown command or update all planning/design docs and comments to describe the channel-close behaviour explicitly.
- In `OutboundPolicyAdapter::handle_decision`, the `FrameDecision::Forward` match arm is unreachable because `FrameOutput::Decision` is only constructed for non-forward decisions; consider tightening the types so `FrameOutput::Decision` cannot carry `FrameDecision::Forward` and remove this dead branch.

## Individual Comments

### Comment 1
<location path="src/engine/connection/exec/acp_runtime_bdd_tests.rs" line_range="310-319" />
<code_context>
+    let _ = denylist_state;
+}
+
+#[scenario(
+    path = "tests/features/acp_method_denylist.feature",
+    name = "Blocked notification is dropped silently"
+)]
+fn blocked_notification_dropped(denylist_state: DenylistState) {
+    let _ = denylist_state;
+}
+
+#[scenario(
+    path = "tests/features/acp_method_denylist.feature",
+    name = "Frame split across two chunks reassembles before the policy applies"
+)]
+fn split_frame_reassembles(denylist_state: DenylistState) {
+    let _ = denylist_state;
+}
+
+#[scenario(
+    path = "tests/features/acp_method_denylist.feature",
+    name = "Permitted frame following a blocked frame still flushes correctly"
+)]
+fn permitted_after_blocked(denylist_state: DenylistState) {
+    let _ = denylist_state;
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider higher-level tests that exercise CapabilityPolicy modes end-to-end

The current unit and BDD tests validate the denylist adapter and sink task in isolation, but nothing verifies that `CapabilityPolicy` actually drives the runtime path selection. In particular, there’s no automated check that:
- `MaskAndDeny` both masks `initialize` on stdin and enforces the outbound denylist via the sink task, and
- `Disabled` / `MaskOnly` skip creating the sink path and behave like the existing streaming proxy.

Please add an integration-style test (e.g., near these BDD scenarios or in `protocol_acp_tests.rs`) that:
1) constructs a session with each `CapabilityPolicy` variant,
2) drives a simple in-memory stdin/stdout/stderr stream, and
3) asserts that only `MaskAndDeny` intercepts blocked methods and synthesizes responses, while other policies preserve the previous behaviour.

Suggested implementation:

```rust
impl RecordingHostStdout {
    fn snapshot(&self) -> Result<Vec<u8>, String> {
        self.bytes
            .lock()
            .map(|guard| guard.clone())
            .map_err(|err| format!("recording writer mutex poisoned: {err}"))
    }
}

#[cfg(test)]
mod capability_policy_integration_tests {
    use super::*;
    use crate::engine::connection::exec::CapabilityPolicy;

    /// Drive a single JSON‑RPC request through an in‑memory exec session for the
    /// given policy, returning what the *container* would see on stdin/stdout/stderr.
    fn drive_policy_session(
        policy: CapabilityPolicy,
        request: &str,
    ) -> (Vec<u8>, Vec<u8>, Vec<u8>) {
        // The concrete wiring into the exec runtime is expected to be provided
        // by the test harness in this module.
        let (host_stdin, host_stdout, host_stderr) = build_in_memory_exec_io();

        run_exec_session_with_policy(
            policy,
            request.as_bytes(),
            &host_stdin,
            &host_stdout,
            &host_stderr,
        )
        .expect("policy session should complete without transport errors");

        let recorded_stdin = host_stdin.snapshot().expect("stdin snapshot");
        let recorded_stdout = host_stdout.snapshot().expect("stdout snapshot");
        let recorded_stderr = host_stderr.snapshot().expect("stderr snapshot");

        (recorded_stdin, recorded_stdout, recorded_stderr)
    }

    #[test]
    fn mask_and_deny_intercepts_blocked_methods_and_synthesizes_response() {
        // This method name should appear in the denylist configured for tests.
        let blocked_method =
            r#"{"jsonrpc":"2.0","id":1,"method":"blocked_method","params":[]}"#;

        let (recorded_stdin, recorded_stdout, _recorded_stderr) =
            drive_policy_session(CapabilityPolicy::MaskAndDeny, blocked_method);

        // The blocked request must not reach the container stdin.
        assert!(
            !String::from_utf8(recorded_stdin)
                .unwrap()
                .contains("blocked_method"),
            "blocked method must not be forwarded to container stdin"
        );

        // A synthetic JSON‑RPC error response should be produced.
        let stdout_str = String::from_utf8(recorded_stdout).unwrap();
        assert!(
            stdout_str.contains(r#""id":1"#),
            "synthetic response should preserve the original id"
        );
        assert!(
            stdout_str.contains("method is not permitted")
                || stdout_str.contains("denied")
                || stdout_str.contains("blocked"),
            "synthetic response should explain that the method was denied; got: {stdout_str}"
        );
    }

    #[test]
    fn disabled_policy_behaves_like_plain_streaming_proxy() {
        let blocked_method =
            r#"{"jsonrpc":"2.0","id":1,"method":"blocked_method","params":[]}"#;

        let (recorded_stdin, _recorded_stdout, _recorded_stderr) =
            drive_policy_session(CapabilityPolicy::Disabled, blocked_method);

        // With Disabled, the request should be forwarded unchanged to stdin.
        let stdin_str = String::from_utf8(recorded_stdin).unwrap();
        assert!(
            stdin_str.contains("blocked_method"),
            "Disabled policy must not intercept blocked methods"
        );
    }

    #[test]
    fn mask_only_policy_masks_initialize_but_does_not_deny_blocked_methods() {
        // Combine an initialize request (which should be masked) with a blocked method call.
        let initialize = r#"{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"capabilities":{}}}"#;
        let blocked =
            r#"{"jsonrpc":"2.0","id":2,"method":"blocked_method","params":[]}"#;
        let batched = format!("[{initialize},{blocked}]");

        let (recorded_stdin, _recorded_stdout, _recorded_stderr) =
            drive_policy_session(CapabilityPolicy::MaskOnly, &batched);

        let stdin_str = String::from_utf8(recorded_stdin).unwrap();

        // initialize should be transformed according to the masking rules (implementation defined),
        // but the blocked method must still flow through to stdin.
        assert!(
            stdin_str.contains("blocked_method"),
            "MaskOnly policy must not deny methods; blocked_method should reach stdin"
        );
        assert!(
            stdin_str.contains("initialize"),
            "initialize should still appear on stdin after masking transformation"
        );
    }
}

```

To make these tests compile and behave as intended, the following additional changes are likely required elsewhere in `acp_runtime_bdd_tests.rs` (or nearby test helpers):

1. **Expose in‑memory exec IO harness**
   - Implement `build_in_memory_exec_io()` to construct the same in‑memory stdin/stdout/stderr objects used by the existing BDD tests:
     - Return something like `(RecordingHostStdin, RecordingHostStdout, RecordingHostStderr)`.
     - Ensure each type has a `snapshot(&self) -> Result<Vec<u8>, String>` method analogous to `RecordingHostStdout::snapshot` shown in your snippet, so the new tests can inspect what the container would have seen.
   - If such a helper already exists under a different name, either:
     - Rename it to `build_in_memory_exec_io`, or
     - Change the tests to call the existing helper instead.

2. **Wire CapabilityPolicy into the exec runtime helper**
   - Implement `run_exec_session_with_policy(...)` with a signature compatible with these tests, e.g.:
     ```rust
     fn run_exec_session_with_policy(
         policy: CapabilityPolicy,
         stdin_bytes: &[u8],
         host_stdin: &RecordingHostStdin,
         host_stdout: &RecordingHostStdout,
         host_stderr: &RecordingHostStderr,
     ) -> Result<(), ExecError> { /* ... */ }
     ```
   - Inside this function, construct the session/connection the same way your production code does, but choose the ACP mode based on the `policy` parameter and hook up the provided in‑memory IO.
   - If there is already a lower‑level helper that builds an exec session given a `CapabilityPolicy`, this function should be a thin wrapper around it.

3. **Ensure `CapabilityPolicy` is imported correctly**
   - The path `crate::engine::connection::exec::CapabilityPolicy` assumes the enum is defined or re‑exported there; adjust the import path to match your actual definition (e.g. `crate::engine::connection::exec::acp::CapabilityPolicy` or similar).

4. **Align the “blocked_method” name and denylist configuration**
   - The tests assume `"blocked_method"` is part of the denylist configured for your test environment. Either:
     - Add `"blocked_method"` to the denylist used in `acp_method_denylist.feature` / the test harness, or
     - Change the method string in the tests to a name that is already blocked by your existing denylist configuration.

5. **Error type for `run_exec_session_with_policy`**
   - Adjust the `.expect("...")` call in `drive_policy_session` to match the concrete error type returned by `run_exec_session_with_policy`, if it isn’t `Result<(), E>` where `E: std::fmt::Display`.

Once these helpers are wired to the real exec runtime, these tests will exercise the three `CapabilityPolicy` variants end‑to‑end, verifying that:
- `MaskAndDeny` prevents blocked methods from reaching container stdin and produces a synthetic error response.
- `Disabled` forwards the blocked method unchanged (no denylist enforcement).
- `MaskOnly` applies masking to `initialize` but still forwards blocked methods, matching the legacy streaming proxy behaviour apart from masking.
</issue_to_address>

### Comment 2
<location path="docs/developers-guide.md" line_range="421-424" />
<code_context>
+  `(Vec<FrameOutput>, Option<FallbackReason>)` per chunk so the adapter can act
+  on per-chunk fallback events. Permitted frames are returned byte-identical;
+  the assembler never re-serializes them.
+- `acp_runtime.rs` is the only ACP module that owns
+  `tokio::sync::mpsc` and `tracing`. It exposes the bounded sink task
+  `run_container_stdin_sink` (capacity `SINK_CHANNEL_CAPACITY = 16`), the
+  `WriteCmd` enum (`Forward`, `Synthesised`), and the `OutboundPolicyAdapter`
+  that translates assembler output into host stdout writes (permitted) or
+  sink-channel sends (synthesized error responses) plus `tracing::warn!` denial
</code_context>
<issue_to_address>
**suggestion (typo):** Align the spelling of “Synthesised” with the rest of the document and the stated `-ize` spelling guideline.

This section uses both `synthesized` (with `z`) and `Synthesised` (with `s`). Given the British English `-ize` guideline, update `Synthesised` (and the corresponding code identifier) to `Synthesized` for consistency.

Suggested implementation:

```
- `acp_runtime.rs` is the only ACP module that owns
  `tokio::sync::mpsc` and `tracing`. It exposes the bounded sink task
  `run_container_stdin_sink` (capacity `SINK_CHANNEL_CAPACITY = 16`), the
  `WriteCmd` enum (`Forward`, `Synthesized`), and the `OutboundPolicyAdapter`
  that translates assembler output into host stdout writes (permitted) or
  sink-channel sends (synthesized error responses) plus `tracing::warn!` denial
  lines.

```

To keep the docs consistent with the code, the Rust enum variant should also be renamed from `Synthesised` to `Synthesized`. That will require:
1. Updating the `WriteCmd` enum definition in `acp_runtime.rs` (or wherever it is defined).
2. Adjusting all references to `WriteCmd::Synthesised` to use `WriteCmd::Synthesized` across the codebase and tests.
3. If this is part of a public API, consider adding a deprecation shim or documenting the rename in the changelog.
</issue_to_address>

### Comment 3
<location path="docs/podbot-design.md" line_range="244-246" />
<code_context>
+  contract for non-Agentic Control Protocol traffic. `MaskOnly` enables the
+  Step 2.6.1 first-frame rewrite without runtime enforcement; `MaskAndDeny`
+  activates both behaviours together.
+- When `MaskAndDeny` is selected, container stdin has a single owner: a
+  dedicated sink task that drains a bounded `tokio::sync::mpsc` channel of
+  `WriteCmd::{Forward, Synthesised}` values. Both the host-stdin forwarder and
+  the output-direction policy adapter are _senders_. The ordering invariant is
+  established by channel construction: the protocol coordinator drops every
</code_context>
<issue_to_address>
**suggestion (typo):** Consider standardising the spelling of “Synthesised” to match the rest of the documentation and identifiers.

`WriteCmd::{Forward, Synthesised}` uses `-ised`, while most of the docs use `synthesized` with `z` and the style guide prefers `-ize`. For consistency between docs and code, consider renaming this to `Synthesized` if that matches the enum variant used elsewhere.

Suggested implementation:

```
  dedicated sink task that drains a bounded `tokio::sync::mpsc` channel of
  `WriteCmd::{Forward, Synthesized}` values. Both the host-stdin forwarder and

```

1. If the actual Rust enum variant is still named `Synthesised`, it should also be renamed to `Synthesized` in the codebase, and all references updated accordingly.
2. Search the rest of `docs/podbot-design.md` and related docs for `Synthesised`/`synthesised` and standardise to `Synthesized`/`synthesized` to keep terminology consistent.
</issue_to_address>

[leynos]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

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

[coderabbitai]

Actionable comments posted: 2

🤖 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/engine/connection/exec/protocol_acp_tests.rs`:
- Around line 388-390: Add a module-level documentation comment for the test
module by inserting a `//!` comment immediately inside `mod
capability_policy_routing` (right after the opening brace) that briefly states
the module's purpose and utility (e.g., what behaviors or scenarios the tests
cover and any important context), so the module complies with the guideline that
every Rust module begins with a `//!` module comment; update the comment text to
be concise and relevant to `capability_policy_routing`.
- Around line 467-505: Replace the three near-duplicate tests
mask_and_deny_routes_through_enforcement_path,
disabled_policy_forwards_all_frames_raw, and
mask_only_policy_forwards_blocked_frames_raw with a single parameterised rstest
that iterates over CapabilityPolicy cases and expected routing outcomes; keep
the shared arrange/act using blocked_terminal_create_frame() and
run_policy_output_frame(), and drive assertions from the case data (e.g. for
each case assert host_stdout != frame and
synthesized_response_for_terminal_create(&container_stdin) for MaskAndDeny, and
assert host_stdout == frame for Disabled and MaskOnly). Use a small per-case
flag (e.g. expect_forward_raw or expect_synthesized_response) to choose between
asserting equality of host_stdout to frame or asserting
synthesized_response_for_terminal_create(&container_stdin).

🪄 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: 86f29399-620f-47c6-83f8-5666549d69fd

📥 Commits

Reviewing files that changed from the base of the PR and between 338038a and c9d13e0.

📒 Files selected for processing (1)

• src/engine/connection/exec/protocol_acp_tests.rs