Bounded proxy buffering for protocol-mode execution (Step 2.5.3)

[leynos]

Summary

• Updates to explicitly implement bounded buffering for protocol-mode exec, enforce stdout-purity by design, and extend tests, docs, and ExecPlan coverage. Includes lifecycle stream-purity tests and regression coverage for zero-prefix/zero-suffix stdout around proxied protocol data. ExecPlan documentation for Step 2.5.3 added and roadmap updated accordingly.
• Tests now cover lifecycle stream purity and error paths, and protocol proxy behavior under varied daemon output scenarios.
• Documentation updates reflect the bounded buffering policy and its effects on protocol-mode execution.
• The ExecPlan document is located at docs/execplans/2-5-3-keep-proxy-buffering-bounded.md.

Changes

• New file: docs/execplans/2-5-3-keep-proxy-buffering-bounded.md
• Core changes to implement bounded buffering for protocol-mode exec:
  • Introduce constants for explicit buffering:
    • STDIN_BUFFER_CAPACITY
    • OUTPUT_BUFFER_CAPACITY
  • Introduce PROTOCOL_OUTPUT_CAPACITY and apply it to Bollard's output_capacity for protocol-mode exec.
  • Update protocol proxy stdin forwarding to use a bounded BufReader and bounded BufWriter on the input path.
  • Update protocol proxy to enforce stdout-purity contract by design (documented in module docs) and tests.
• Code changes:
  • src/engine/connection/exec/mod.rs: Added PROTOCOL_OUTPUT_CAPACITY and wired it into StartExecOptions for protocol mode; keep other modes unchanged.
  • src/engine/connection/exec/protocol.rs: Added STDIN_BUFFER_CAPACITY and OUTPUT_BUFFER_CAPACITY constants; wrap host stdin and container input with BufReader/BufWriter using these capacities; adjust forward path accordingly. Expanded module-level doc comment to state stdout-purity contract.
• Tests:
  • src/engine/connection/exec/tests/protocol_helpers.rs: Updated tests to expect output_capacity of Some(65_536) for protocol mode.
  • src/engine/connection/exec/tests/protocol_proxy_bdd.rs: Extended with lifecycle-purity and error-purity test scaffolding.
  • src/engine/connection/exec/tests/proxy_helpers/lifecycle_purity.rs: New lifecycle purity tests covering startup, steady-state, shutdown, and error paths.
  • tests/bdd_interactive_exec_helpers/steps.rs: Updated to expect protocol mode output_capacity of Some(65_536).
  • tests/features/protocol_proxy.feature: Added scenarios for lifecycle purity and purity on error paths.
• Documentation:
  • docs/podbot-design.md: Added a section describing bounded buffering implementation for protocol-mode execution (stdin/out buffers and per-chunk output capacity).
  • docs/users-guide.md: Updated to mention bounded buffering in protocol mode and the backpressure semantics.
  • docs/podbot-roadmap.md: Updated Step 2.5 checkboxes to reflect completion after gates pass.
• Other:
  • Ensure all tests and gates reflect 64 KiB buffering values and updated expectations.

Plan of work (Stage A-F)

Stage A: Make stdin forwarding buffer explicitly bounded

• Add STDIN_BUFFER_CAPACITY constant and wrap host_stdin in a bounded BufReader before copy.
• Wrap container input writer in a bounded BufWriter with explicit capacity.
• Status: COMPLETE

Stage B: Set Bollard output_capacity for protocol-mode exec

• Add PROTOCOL_OUTPUT_CAPACITY constant and set StartExecOptions.output_capacity for ExecMode::Protocol to Some(PROTOCOL_OUTPUT_CAPACITY); keep other modes unchanged.
• Update tests that assert options to reflect the new capacity.
• Status: COMPLETE

Stage C: Enforce stdout purity at protocol proxy seam

• Document stdout-purity contract in src/engine/connection/exec/protocol.rs module doc comment; ensure routing prevents non-protocol bytes from reaching host stdout.
• Status: COMPLETE

Stage D: Add lifecycle stream-purity tests

• Add unit tests under src/engine/connection/exec/tests/proxy_helpers/lifecycle_purity.rs covering startup, steady-state, shutdown, and error paths.
• Status: COMPLETE

Stage E: Add regression test for zero stdout bytes before/after proxied bytes

• Extend tests to assert no pre/post bytes around proxied output.
• Status: COMPLETE

Stage F: Documentation and roadmap updates

• Update docs/podbot-design.md and docs/users-guide.md to reflect bounded buffering behavior and user-facing expectations.
• Mark Step 2.5 roadmap checkboxes done after gates pass.
• Status: COMPLETE

Validation and acceptance

• STDIN_BUFFER_CAPACITY, OUTPUT_BUFFER_CAPACITY, and PROTOCOL_OUTPUT_CAPACITY are defined and used;
• Bollard output_capacity is set for protocol-mode exec;
• Lifecycle and regression tests exist and pass;
• A module-level stdout-purity doc comment exists in protocol.rs;
• Design and user documentation are updated;
• Roadmap checkboxes are updated after gates pass;
• Full gate stack passes.

Testing plan

• Run gates: make check-fmt, make lint, make test; ensure all pass.

Documentation

• ExecPlan 2-5-3 documented in docs/execplans/2-5-3-keep-proxy-buffering-bounded.md.
• Podbot design and user guide updated to reflect bounded buffering and stdout-purity.

Notes

• This PR includes the ExecPlan document and substantive changes to implement explicit bounded buffering, tests to prove stream-purity across lifecycle stages, and user-facing documentation updates. The plan remains to enforce clear backpressure semantics and maintain protocol-mode purity at the protocol proxy seam.

• Task: https://www.devboxer.com/task/7360e9a2-744d-4153-b769-e143a7027b55

Summary by Sourcery

Enforce bounded buffering and stdout purity for protocol-mode execution while updating tests and documentation to reflect the new behaviour.

New Features:

• Add explicit 64 KiB buffer capacities for stdin forwarding and protocol output chunks.
• Introduce lifecycle and error-path BDD scenarios for protocol proxy stream purity.

Bug Fixes:

• Ensure protocol proxy does not leak error or non-protocol bytes to host stdout when daemon stream errors occur.

Enhancements:

• Refine protocol proxy tests and helpers to capture daemon stream errors and assert exact stdout contents without prefixes or suffixes.
• Document bounded buffering implementation and stdout-purity contract for protocol-mode execution in the design docs and ExecPlan.

Documentation:

• ExecPlan 2-5-3 documents bounded proxy buffering and stream-purity enforcement; Step 2.5 marked complete in roadmap.
• Update design and user guides to describe the 64 KiB bounded buffering policy and backpressure semantics for protocol mode.

Tests:

• Add lifecycle stream-purity unit tests for startup, steady-state, shutdown, and error paths plus regression test for zero extra stdout bytes around proxied output.
• Extend protocol proxy BDD steps and feature scenarios to cover lifecycle purity and daemon stream error behaviour.
• Update existing exec tests to expect a 64 KiB protocol-mode output_capacity from Bollard.

[sourcery-ai]

Reviewer's Guide

Implements explicit 64 KiB bounded buffering for protocol-mode exec (stdin, container input, and daemon output chunks), wires Bollard’s output_capacity accordingly, and adds lifecycle stream-purity tests plus docs/roadmap updates to enforce and document stdout purity for protocol execution.

Sequence diagram for protocol-mode exec with bounded buffering and backpressure

sequenceDiagram
    actor User
    participant Podbot
    participant ProtocolProxy
    participant HostStdin
    participant HostStdout
    participant HostStderr
    participant DockerDaemon
    participant Container

    User->>Podbot: start_exec(mode = Protocol)
    Podbot->>Podbot: build_start_exec_options()
    Note right of Podbot: output_capacity = Some(PROTOCOL_OUTPUT_CAPACITY)
    Podbot->>DockerDaemon: create_exec / start_exec(output_capacity = 64KiB)
    DockerDaemon-->>Podbot: exec_id, output_stream(LogOutput, 64KiB chunks)

    Podbot->>ProtocolProxy: run_protocol_session_with_io_async(ProtocolProxyIo)

    par Stdin_forwarding
        ProtocolProxy->>HostStdin: read via BufReader(STDIN_BUFFER_CAPACITY = 64KiB)
        HostStdin-->>ProtocolProxy: bytes
        ProtocolProxy->>Container: write via BufWriter(OUTPUT_BUFFER_CAPACITY = 64KiB)
        Container-->>ProtocolProxy: ack
        ProtocolProxy->>Container: flush and shutdown on EOF
    and Output_forwarding
        loop For_each_LogOutput_chunk
            DockerDaemon-->>Podbot: LogOutput(chunk <= 64KiB)
            Podbot->>ProtocolProxy: forward LogOutput

            alt Stdout_or_Console
                ProtocolProxy->>HostStdout: write_all(chunk_bytes)
                ProtocolProxy->>HostStdout: flush()
                alt HostStdout_blocks_on_flush
                    HostStdout-->>ProtocolProxy: pending
                    ProtocolProxy-->>DockerDaemon: stop_polling_stream (backpressure)
                else Flush_succeeds
                    HostStdout-->>ProtocolProxy: ok
                end
            else Stderr
                ProtocolProxy->>HostStderr: write_all(stderr_bytes) + flush()
            else StdIn_echo
                ProtocolProxy-->>ProtocolProxy: drop_chunk (stdout_purity)
            end
        end
    end

    alt Normal_shutdown
        DockerDaemon-->>Podbot: stream_EOF
        Podbot->>ProtocolProxy: notify_shutdown
        ProtocolProxy-->>User: session_result_ok
    else Error_path
        DockerDaemon-->>Podbot: stream_error
        Podbot->>ProtocolProxy: propagate_error
        ProtocolProxy-->>User: PodbotError (no extra stdout bytes)
    end

Loading

Updated class diagram for protocol exec buffering and stdout purity

classDiagram
    class ExecMode {
        <<enum>>
        Protocol
        Attached
        Detached
        +is_attached() bool
    }

    class ExecRequest {
        +mode() ExecMode
        +tty() bool
    }

    class StartExecOptions {
        +detach: bool
        +tty: bool
        +output_capacity: Option~usize~
    }

    class ExecModule {
        <<module>>
        +EXEC_INSPECT_POLL_INTERVAL_MS: u64
        +PROTOCOL_OUTPUT_CAPACITY: usize
        +build_start_exec_options(request: &ExecRequest) StartExecOptions
    }

    class ProtocolProxyIo~HostStdin,HostStdout,HostStderr~ {
        +new(host_stdin: HostStdin, host_stdout: HostStdout, host_stderr: HostStderr) ProtocolProxyIo
    }

    class ProtocolProxyModule {
        <<module>>
        +STDIN_SETTLE_TIMEOUT: Duration
        +STDIN_BUFFER_CAPACITY: usize
        +OUTPUT_BUFFER_CAPACITY: usize
        +forward_host_stdin_to_exec_async(host_stdin: HostStdin, input: Pin~Box~dyn AsyncWrite + Send~~) io::Result~()~
        +run_output_loop_async(host_stdout: HostStdout, host_stderr: HostStderr) io::Result~()~
    }

    ExecModule --> ExecRequest : uses
    ExecModule --> ExecMode : matches_mode
    ExecModule --> StartExecOptions : builds
    ExecModule --> ProtocolProxyModule : dispatches_protocol_mode

    ProtocolProxyModule --> ProtocolProxyIo : constructs
    ProtocolProxyModule ..> HostStdin : generic_read
    ProtocolProxyModule ..> HostStdout : generic_write
    ProtocolProxyModule ..> HostStderr : generic_write

    class HostStdin {
        <<trait>>
        +poll_read()
    }

    class HostStdout {
        <<trait>>
        +poll_write()
        +flush()
    }

    class HostStderr {
        <<trait>>
        +poll_write()
        +flush()
    }

Loading

File-Level Changes

Change	Details	Files
Add explicit 64 KiB buffering for protocol stdin forwarding and container input writes in the protocol proxy.	Introduce STDIN_BUFFER_CAPACITY and OUTPUT_BUFFER_CAPACITY constants (both 65_536 bytes) in the protocol exec module. Wrap host stdin in a BufReader and container stdin writer in a BufWriter using these capacities in forward_host_stdin_to_exec_async. Ensure the stdin forwarding path still flushes and cleanly shuts down the container input after copying completes.	src/engine/connection/exec/protocol.rs
Configure protocol-mode exec sessions to use bounded 64 KiB daemon output chunks via Bollard’s output_capacity.	Introduce PROTOCOL_OUTPUT_CAPACITY (65_536) in the exec module. Update build_start_exec_options to set output_capacity = Some(PROTOCOL_OUTPUT_CAPACITY) for ExecMode::Protocol, leaving other modes unchanged. Adjust protocol tests and interactive exec BDD helpers to assert the new output_capacity for protocol mode.	src/engine/connection/exec/mod.rs src/engine/connection/exec/tests/protocol_helpers.rs tests/bdd_interactive_exec_helpers/steps.rs
Strengthen protocol proxy behaviour and tests to guarantee stdout stream purity across lifecycle and error paths.	Refactor protocol_proxy_bdd test harness to support simulated daemon stream errors and to centralise IO capture and outcome recording. Add BDD steps for explicit end-of-stream and daemon error conditions, plus new assertions on stdout concatenation and absence of prefix/suffix bytes. Introduce lifecycle_purity unit tests to cover startup, steady-state, shutdown, error paths, and a regression asserting zero extra stdout bytes before/after proxied bytes.	src/engine/connection/exec/tests/protocol_proxy_bdd.rs src/engine/connection/exec/tests/proxy_helpers/lifecycle_purity.rs tests/features/protocol_proxy.feature
Document the bounded-buffering and stdout-purity contracts and mark roadmap Step 2.5.3 as complete, with an accompanying ExecPlan.	Expand podbot-design to describe bounded buffering for protocol-mode (stdin BufReader, stdin BufWriter, Bollard output_capacity, and backpressure semantics). Update users-guide to mention bounded buffering and backpressure behaviour in protocol mode. Mark Step 2.5 roadmap items as completed and add a detailed ExecPlan document for 2-5-3 describing constraints, plan, decisions, and outcomes.	docs/podbot-design.md docs/users-guide.md docs/podbot-roadmap.md docs/execplans/2-5-3-keep-proxy-buffering-bounded.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.

[coderabbitai]

Walkthrough

Implement Step 2.5.3: add 64 KiB bounded buffering for protocol-mode stdin and Bollard output chunks, enforce a protocol-mode stdout‑purity contract, and add unit/BDD lifecycle and regression tests; mark the step complete and update documentation and test expectations.

Changes

Cohort / File(s)	Summary
Documentation & Roadmap docs/execplans/2-5-3-keep-proxy-buffering-bounded.md, docs/podbot-design.md, docs/podbot-roadmap.md, docs/users-guide.md, docs/developers-guide.md	Add execution plan and completion log; document 64 KiB bounded buffering for stdin and output (including Bollard output_capacity), backpressure behaviour, and the protocol-mode stdout‑purity contract; mark Step 2.5 tasks complete and add developer/testing guidance.
Core Implementation src/engine/connection/exec/mod.rs, src/engine/connection/exec/protocol.rs	Introduce PROTOCOL_OUTPUT_CAPACITY = 65_536 and STDIN_BUFFER_CAPACITY = 65_536; set StartExecOptions.output_capacity = Some(65_536) for ExecMode::Protocol; wrap host stdin in a bounded BufReader; add a Stdout Purity Contract comment.
Test Helpers & Mock Expectations src/engine/connection/exec/tests/protocol_helpers.rs, tests/bdd_interactive_exec_helpers/steps.rs	Update test helpers and mocked expectations to require output_capacity = Some(65_536) for protocol mode.
BDD Tests & Feature Scenarios src/engine/connection/exec/tests/protocol_proxy_bdd.rs, tests/features/protocol_proxy.feature	Unify container output model into OutputEvent; add stream-failure events; build failing/success streams; add BDD assertions verifying host stdout contains only proxied stdout bytes (no extra bytes before first/after last proxied byte) and behaviour on stream failure; add new scenarios.
Lifecycle Purity Tests src/engine/connection/exec/tests/proxy_helpers/mod.rs, src/engine/connection/exec/tests/proxy_helpers/lifecycle_purity.rs	Add lifecycle_purity test module and run_lifecycle_session helper; add parametrised rstest cases covering startup, steady‑state, shutdown and error paths; add regression tests asserting zero extra host stdout bytes and preservation across >64 KiB buffer boundary.
Workspace Tests src/engine/connection/exec/tests.rs	Add parameterised tests for build_start_exec_options covering Protocol/Attached/Detached modes and asserting output_capacity for protocol mode.

Sequence Diagram

sequenceDiagram
    participant Container
    participant Bollard
    participant ProtocolProxy as Protocol Proxy
    participant HostOS as Host OS

    rect rgba(100,150,255,0.5)
    Note over ProtocolProxy: Bounded buffering flow (64 KiB)
    end

    Container->>Bollard: emit LogOutput chunks
    Bollard->>ProtocolProxy: stream LogOutput items
    ProtocolProxy->>ProtocolProxy: assemble chunk (<=64 KiB)
    ProtocolProxy->>HostOS: write_all() then flush()

    rect rgba(200,100,100,0.5)
    Note over HostOS,ProtocolProxy: Backpressure path
    end

    HostOS-->>ProtocolProxy: flush() blocks
    ProtocolProxy->>ProtocolProxy: yield / stop polling
    ProtocolProxy->>Bollard: stop polling stream
    Bollard->>Container: propagate backpressure
    Container->>Container: pause output

    Note over ProtocolProxy: on flush unblock, resume polling and continue delivery
    HostOS-->>ProtocolProxy: flush() returns
    ProtocolProxy->>Bollard: resume polling
    Bollard->>Container: continue delivery

Loading

Possibly related PRs

• Rename test function for protocol proxy consistency #69 — Modifies the same exec/protocol proxy infrastructure; closely related to bounded‑buffering constants, stdin wrapping and test expectations.
• Add protocol exec mode with tty=false enforcement #66 — Introduced earlier Protocol exec-mode plumbing and StartExecOptions wiring that this change builds upon.

Poem

🔒 Sixty‑four KiB hold the stream,
No stray bytes disturb the seam,
Flush yields, backpressure flows,
Tests prove pure stdout as it goes,
Protocol bytes march neat and clean.

🚥 Pre-merge checks | ✅ 6 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Module-Level Documentation	❓ Inconclusive	Cannot verify module-level documentation compliance—files could not be accessed to confirm all modules contain proper docstrings.	Examine repository files to confirm all new/modified modules contain module-level docstrings (//! or /*! */) before any code.

✅ Passed checks (6 passed)

Check name	Status	Explanation
Description check	✅ Passed	Description comprehensively covers the changeset: bounded buffering implementation, stdout-purity enforcement, test coverage, documentation updates, and ExecPlan completion.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Testing	✅ Passed	All new functionality for bounded protocol-mode execution and stdout purity is properly guarded by comprehensive test coverage including parametrised unit tests validating output_capacity across ExecModes, lifecycle stream-purity tests covering startup/shutdown/error paths with >64 KiB regression tests, BDD scenarios verifying output concatenation and stream error handling, and protocol helper assertions enforcing output_capacity expectations.
User-Facing Documentation	✅ Passed	The pull request adequately documents all user-facing functionality and behaviour changes in the user's guide, explicitly describing the new bounded buffering behaviour and backpressure propagation mechanisms.
Developer Documentation	✅ Passed	The developers-guide.md comprehensively documents all new or changed internal APIs, architectural boundaries, and abstractions introduced in this PR, including explicit documentation of constants and enforcement mechanisms.
Title check	✅ Passed	The title 'Bounded proxy buffering for protocol-mode execution (Step 2.5.3)' directly and clearly summarises the main changes: bounded buffering implementation for protocol-mode exec with explicit reference to Step 2.5.3 completion.

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

✨ 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 bollard-protocol-safe-execution-xos6vw

---

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

[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/tests/protocol_proxy_bdd.rs

Comment on file

        .fail_stdout_write
        .get()
        .unwrap_or(false);
    let stream_error = protocol_proxy_state.stream_error.get().unwrap_or(false);

❌ New issue: Large Method
the_protocol_proxy_runs has 76 lines, threshold = 70

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

Large Method

src/engine/connection/exec/tests/protocol_proxy_bdd.rs: the_protocol_proxy_runs

What lead to degradation?

the_protocol_proxy_runs has 76 lines, threshold = 70

Why does this problem occur?

Overly long functions make the code harder to read. The recommended maximum function length for the Rust language is 70 lines of code. Severity: Brain Method - Complex Method - Long Method.

How to fix it?

We recommend to be careful here -- just splitting long functions don't necessarily make the code easier to read. Instead, look for natural chunks inside the functions that expresses a specific task or concern. Often, such concerns are indicated by a Code Comment followed by an if-statement. Use the EXTRACT FUNCTION refactoring to encapsulate that concern.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 37d17841d0

ℹ️ 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]

Actionable comments posted: 4

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/tests/protocol_proxy_bdd.rs (1)

110-133: ⚠️ Potential issue | 🟡 Minor

Preserve feature step order in the synthetic output stream.

Store emitted events in one ordered list instead of replaying every stdout chunk
before every stderr chunk. As written, a scenario such as stdout -> stderr -> stdout -> error is always rebuilt as stdout -> stdout -> stderr -> error,
which weakens the lifecycle/error coverage these new BDD cases are meant to
provide.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/engine/connection/exec/tests/protocol_proxy_bdd.rs` around lines 110 -
133, The helper build_output_stream currently appends all stdout chunks then all
stderr chunks, losing original interleaving; change its API to accept a single
ordered sequence (e.g., Vec<OutputEvent> or Vec<Result<LogOutput,
BollardError>>) and iterate that sequence preserving order, pushing
Ok(LogOutput::StdOut{..}), Ok(LogOutput::StdErr{..}) or
Err(BollardError::DockerResponseServerError{..}) as appropriate inside
build_output_stream (update any tests/call sites to construct the ordered
sequence instead of separate stdout_chunks/stderr_chunks flags). Ensure the
function still returns Vec<Result<LogOutput, BollardError>> and reference the
existing LogOutput and BollardError types when mapping events.

🤖 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-5-3-keep-proxy-buffering-bounded.md`:
- Around line 453-478: Update the three decision-log dates from "2025-04-07" to
"2026-04-07" in the doc section that mentions the buffer constants
(STDIN_BUFFER_CAPACITY, OUTPUT_BUFFER_CAPACITY, PROTOCOL_OUTPUT_CAPACITY), the
change making build_start_exec_options a const fn, and the unit test return-type
change (TestResult -> ()), so the chronology reflects the PR `#72` April 2026
merge.

In `@docs/podbot-roadmap.md`:
- Around line 168-176: Update the Step 2.5 checklist items (the entries
mentioning "exec attachment with `tty = false`", the "byte-stream proxy loops"
items, "bounded proxy buffering", "podbot host emits no non-protocol bytes to
stdout while proxying", and the lifecycle/regression tests) to include a
governing design citation; append a short reference like "See podbot-design.md
§2.5" (or the exact subsection that defines exec/proxy lifecycle and stream
purity) to each completed task so readers can find the source-of-truth
acceptance criteria.

In `@src/engine/connection/exec/tests/protocol_proxy_bdd.rs`:
- Around line 300-315: The test currently only checks ProtocolProxyOutcome via
protocol_proxy_state.outcome and ignores the actual captured host output; update
the host_stdout_contains_no_extra_bytes step (function
host_stdout_contains_no_extra_bytes) to fetch and inspect
protocol_proxy_state.host_stdout (or the appropriate field that holds captured
bytes) after confirming outcome is Success, and fail the step if those bytes are
non-empty (or do not match the expected exact byte slice), returning an Err that
includes the offending bytes for diagnostics; keep the existing outcome check
but add the host_stdout inspection and clear error message referencing the
host_stdout contents.

In `@src/engine/connection/exec/tests/proxy_helpers/lifecycle_purity.rs`:
- Around line 13-29: Several tests (e.g., startup_purity_no_prefix_bytes)
duplicate the same RecordingWriter capture and run_session() choreography;
extract that setup into a reusable rstest fixture or helper function (e.g., a
new fn session_harness(runtime: RuntimeFixture, input_bytes: &[u8],
output_chunks: Vec<Result<LogOutput, _>>) -> (captured_stdout_bytes, /* other
results */)) that encapsulates creating RecordingWriter, RecordingInputWriter,
make_output_stream and calling run_session, then replace the duplicated blocks
in startup_purity_no_prefix_bytes and the other listed test cases with calls to
this fixture/helper and parameterize them using #[rstest(...)] so each test only
supplies the differing stream bytes and assertions.

---

Outside diff comments:
In `@src/engine/connection/exec/tests/protocol_proxy_bdd.rs`:
- Around line 110-133: The helper build_output_stream currently appends all
stdout chunks then all stderr chunks, losing original interleaving; change its
API to accept a single ordered sequence (e.g., Vec<OutputEvent> or
Vec<Result<LogOutput, BollardError>>) and iterate that sequence preserving
order, pushing Ok(LogOutput::StdOut{..}), Ok(LogOutput::StdErr{..}) or
Err(BollardError::DockerResponseServerError{..}) as appropriate inside
build_output_stream (update any tests/call sites to construct the ordered
sequence instead of separate stdout_chunks/stderr_chunks flags). Ensure the
function still returns Vec<Result<LogOutput, BollardError>> and reference the
existing LogOutput and BollardError types when mapping events.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 909234db-1b0a-49b0-ae20-87d71a7db982

📥 Commits

Reviewing files that changed from the base of the PR and between d2cbc94 and 37d1784.

📒 Files selected for processing (12)

• docs/execplans/2-5-3-keep-proxy-buffering-bounded.md
• docs/podbot-design.md
• docs/podbot-roadmap.md
• docs/users-guide.md
• src/engine/connection/exec/mod.rs
• src/engine/connection/exec/protocol.rs
• src/engine/connection/exec/tests/protocol_helpers.rs
• src/engine/connection/exec/tests/protocol_proxy_bdd.rs
• src/engine/connection/exec/tests/proxy_helpers/lifecycle_purity.rs
• src/engine/connection/exec/tests/proxy_helpers/mod.rs
• tests/bdd_interactive_exec_helpers/steps.rs
• tests/features/protocol_proxy.feature

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

docs/execplans/2-5-3-keep-proxy-buffering-bounded.md (1)

453-478: ⚠️ Potential issue | 🟡 Minor

Correct the decision-log dates to April 2026.

The three decision-log entries use 2025-04-07 but this ExecPlan documents work
merged in PR #72 during April 2026. Fix these to 2026-04-07.

📝 Proposed fix

-### 2025-04-07: Chose 64 KiB for all buffer constants
+### 2026-04-07: Chose 64 KiB for all buffer constants

-**2025-04-07: Made `build_start_exec_options` const**
+**2026-04-07: Made `build_start_exec_options` const**

-**2025-04-07: Unit tests do not return `Result`**
+**2026-04-07: Unit tests do not return `Result`**

Triage: [type:docstyle]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/execplans/2-5-3-keep-proxy-buffering-bounded.md` around lines 453 - 478,
Update the three decision-log entry dates from "2025-04-07" to "2026-04-07" in
this exec plan file; specifically edit the three dated paragraphs that start
with "2025-04-07: Chose 64 KiB for all buffer constants", "2025-04-07: Made
`build_start_exec_options` const", and "2025-04-07: Unit tests do not return
`Result`" to read "2026-04-07" so the entries align with PR `#72` (these sections
reference symbols like STDIN_BUFFER_CAPACITY, OUTPUT_BUFFER_CAPACITY,
PROTOCOL_OUTPUT_CAPACITY, and build_start_exec_options to help you find them).

🤖 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-5-3-keep-proxy-buffering-bounded.md`:
- Line 462: Replace the word "amortise" with the Oxford spelling "amortize" in
the phrase "amortise syscall overhead" (i.e., update the occurrence of
"amortise" in the sentence "(large enough to amortise syscall overhead)." to
"amortize") so the document uses the requested spelling.

In `@src/engine/connection/exec/tests/proxy_helpers/lifecycle_purity.rs`:
- Around line 13-29: Tests duplicate the same RecordingWriter capture and
run_session choreography; extract a small helper to DRY them. Create a function
(e.g., make_test_session or run_test_session) that accepts the RuntimeFixture,
input bytes, and output stream and internally constructs host_stdout:
RecordingWriter, captures captured_stdout = host_stdout.bytes.clone(), uses
Box::pin(RecordingInputWriter::new()) and a second RecordingWriter for stderr,
then calls run_session(...) and returns the run_session result plus the
captured_stdout handle for assertions; update the seven test functions to call
this helper instead of repeating the RecordingWriter and run_session setup.

---

Duplicate comments:
In `@docs/execplans/2-5-3-keep-proxy-buffering-bounded.md`:
- Around line 453-478: Update the three decision-log entry dates from
"2025-04-07" to "2026-04-07" in this exec plan file; specifically edit the three
dated paragraphs that start with "2025-04-07: Chose 64 KiB for all buffer
constants", "2025-04-07: Made `build_start_exec_options` const", and
"2025-04-07: Unit tests do not return `Result`" to read "2026-04-07" so the
entries align with PR `#72` (these sections reference symbols like
STDIN_BUFFER_CAPACITY, OUTPUT_BUFFER_CAPACITY, PROTOCOL_OUTPUT_CAPACITY, and
build_start_exec_options to help you find them).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 74a238dc-e7b8-48bc-88ee-1b21a00a5bf0

📥 Commits

Reviewing files that changed from the base of the PR and between 37d1784 and 4ad4a47.

📒 Files selected for processing (6)

• docs/execplans/2-5-3-keep-proxy-buffering-bounded.md
• docs/podbot-design.md
• src/engine/connection/exec/mod.rs
• src/engine/connection/exec/protocol.rs
• src/engine/connection/exec/tests/protocol_proxy_bdd.rs
• src/engine/connection/exec/tests/proxy_helpers/lifecycle_purity.rs

[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 `OUTPUT_BUFFER_CAPACITY` constant in `protocol.rs` is used for the container stdin writer, which can be confusing given the name and neighbouring comments; consider renaming it (e.g. to `STDIN_WRITE_BUFFER_CAPACITY` or similar) to make its role clearer at the call site.
- The BDD step `host stdout contains no prefix or suffix bytes` currently only asserts a successful outcome, relying on the previous step to check content; if you want this step to stand on its own as a regression guard, consider explicitly re-checking that the captured stdout exactly matches the expected concatenated bytes.

## Individual Comments

### Comment 1
<location path="src/engine/connection/exec/mod.rs" line_range="29" />
<code_context>
+/// overhead compared to the default 8 KiB for large protocol messages. A 64 KiB
+/// capacity matches common JSON-RPC frame buffer sizes and aligns with the
+/// stdin/output buffer capacities used by the protocol proxy.
+const PROTOCOL_OUTPUT_CAPACITY: usize = 65_536;
+
 /// Boxed future type returned by [`ContainerExecClient::create_exec`].
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicating the 64 KiB capacity literal in multiple places to prevent configuration drift.

This is set to 65_536 to match `STDIN_BUFFER_CAPACITY` and `OUTPUT_BUFFER_CAPACITY` in `protocol.rs`. If these are intended to remain aligned for protocol-mode sessions, consider defining a shared constant (or deriving one from another) so changing one buffer size doesn’t accidentally desync the others.

Suggested implementation:

```rust
use crate::engine::connection::protocol;

/// Maximum bytes per `LogOutput` chunk from the daemon for protocol-mode exec
/// sessions. This bounds the per-chunk memory consumption while reducing
/// overhead compared to the default 8 KiB for large protocol messages. A 64 KiB
/// capacity matches common JSON-RPC frame buffer sizes and aligns with the
/// stdin/output buffer capacities used by the protocol proxy.
const PROTOCOL_OUTPUT_CAPACITY: usize = protocol::OUTPUT_BUFFER_CAPACITY;

```

To make this compile and keep things consistent:

1. In `src/engine/connection/protocol.rs`, ensure `OUTPUT_BUFFER_CAPACITY` (and/or `STDIN_BUFFER_CAPACITY`) is declared `pub const` so it can be referenced from `exec::mod`:
   - `const OUTPUT_BUFFER_CAPACITY: usize = 65_536;` → `pub const OUTPUT_BUFFER_CAPACITY: usize = 65_536;`
2. If `protocol` is not already a sibling module of `exec` under `engine::connection`, adjust the import path accordingly (for example, `use crate::engine::connection::protocol;` may need to be `use crate::engine::connection::protocol_mode as protocol;` or similar to match your actual module name).
3. If your project’s style discourages `use` in the middle of a module, move the added `use crate::engine::connection::protocol;` to the existing `use` block at the top of `mod.rs` and reference `protocol::OUTPUT_BUFFER_CAPACITY` from there.
</issue_to_address>

### Comment 2
<location path="src/engine/connection/exec/tests/protocol_proxy_bdd.rs" line_range="300-315" />
<code_context>
+    )
+}
+
+#[then("host stdout contains no prefix or suffix bytes")]
+fn host_stdout_contains_no_extra_bytes(
+    protocol_proxy_state: &ProtocolProxyState,
+) -> StepResult<()> {
+    // This step verifies the earlier assertion covered the complete output
+    let outcome = protocol_proxy_state
+        .outcome
+        .get()
+        .ok_or_else(|| String::from("proxy outcome should be recorded"))?;
+    match outcome {
+        ProtocolProxyOutcome::Success => Ok(()),
+        ProtocolProxyOutcome::Failure(message) => Err(format!(
+            "expected successful proxy run, got failure: {message}"
+        )),
</code_context>
<issue_to_address>
**suggestion (testing):** This step doesn’t actually assert that stdout has no extra bytes; it only checks that the proxy run succeeded.

The step name implies we assert there are no prefix/suffix bytes, but this step only checks `ProtocolProxyOutcome` and never re-examines `host_stdout`. It implicitly relies on earlier steps and on `assert_channel_receives` using exact equality. To make the “no extra bytes” guarantee explicit and resilient to future changes, consider also validating here that the captured stdout exactly matches the expected concatenated bytes (including a length check), e.g., by re-reading `host_stdout` or by storing the expected bytes in shared state and checking equality/length in this step.

```suggestion
#[then("host stdout contains no prefix or suffix bytes")]
fn host_stdout_contains_no_extra_bytes(
    protocol_proxy_state: &ProtocolProxyState,
) -> StepResult<()> {
    // First, ensure the proxy run itself was successful.
    let outcome = protocol_proxy_state
        .outcome
        .get()
        .ok_or_else(|| String::from("proxy outcome should be recorded"))?;
    if let ProtocolProxyOutcome::Failure(message) = outcome {
        return Err(format!(
            "expected successful proxy run, got failure: {message}"
        ));
    }

    // Then, explicitly assert that there are no additional bytes on host stdout
    // beyond what earlier steps have already asserted.
    assert_channel_receives(
        &[],
        protocol_proxy_state.host_stdout.get(),
        "host stdout",
    )?;

    Ok(())
}
```
</issue_to_address>

### Comment 3
<location path="src/engine/connection/exec/tests/proxy_helpers/lifecycle_purity.rs" line_range="39-42" />
<code_context>
+
+use super::*;
+
+/// Startup purity: protocol proxy delivers exactly the container output with
+/// no prefix bytes from session setup.
+#[rstest]
+fn startup_purity_no_prefix_bytes(runtime: RuntimeFixture) {
+    let output_chunks = vec![Ok(LogOutput::StdOut {
+        message: b"STARTUP_OUTPUT".to_vec().into(),
+    })];
+    let output = make_output_stream(output_chunks);
+
+    let host_stdout = RecordingWriter::new();
+    let captured_stdout = host_stdout.bytes.clone();
+    let result = run_session(
+        runtime,
+        b"",
+        output,
+        Box::pin(RecordingInputWriter::new()),
+        host_stdout,
+        RecordingWriter::new(),
+    );
+
+    assert!(result.is_ok(), "startup should succeed");
+    let captured = captured_stdout.lock().expect("mutex should not poison");
+    assert_eq!(
+        captured.as_slice(),
+        b"STARTUP_OUTPUT",
</code_context>
<issue_to_address>
**suggestion (testing):** Add a lifecycle purity test for the edge case where the daemon never emits stdout bytes.

Existing lifecycle tests cover various stdout patterns but not the case where the daemon never writes to stdout (only stderr or exits silently). Please add a test that uses an empty `output_chunks` sequence and asserts that `host_stdout` remains empty while `run_session` still succeeds, to lock in the “no output at all” purity guarantee and catch accidental banner/shutdown bytes on stdout.

```suggestion
#[rstest]
fn lifecycle_purity_no_stdout_bytes(runtime: RuntimeFixture) {
    // Daemon never emits stdout bytes; ensure we don't inject any banner/prefix/suffix.
    let output_chunks: Vec<Result<LogOutput, BollardError>> = Vec::new();
    let output = make_output_stream(output_chunks);

    let host_stdout = RecordingWriter::new();
    let captured_stdout = host_stdout.bytes.clone();
    let result = run_session(
        runtime,
        b"",
        output,
        Box::pin(RecordingInputWriter::new()),
        host_stdout,
        RecordingWriter::new(),
    );

    assert!(result.is_ok(), "session should succeed even with no stdout");
    let captured = captured_stdout.lock().expect("mutex should not poison");
    assert_eq!(
        captured.as_slice(),
        b"",
        "host stdout must remain empty when container never writes to stdout"
    );
}

/// Steady-state purity: protocol proxy delivers only container stdout and
/// console bytes, routing stderr separately, and suppressing stdin echoes.
#[rstest]
fn steady_state_purity_mixed_streams(runtime: RuntimeFixture) {
```
</issue_to_address>

### Comment 4
<location path="src/engine/connection/exec/tests/proxy_helpers/lifecycle_purity.rs" line_range="167-176" />
<code_context>
+/// Regression test: zero stdout bytes before the first proxied protocol byte
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a regression test that stresses multiple stdout chunks around the buffer size to exercise the bounded buffering behaviour.

The existing regression test `regression_zero_bytes_before_first_and_after_last_proxied_byte` is useful for the single small `StdOut` case. With the new 64 KiB bounded buffering on stdin/stdout, it would be helpful to add a similar test that (1) uses multiple `StdOut` chunks whose total size exceeds 64 KiB and (2) asserts that `host_stdout` is exactly the concatenation of those chunks, with no extra or missing bytes. That would exercise the BufReader/BufWriter + copy loop under higher throughput and specifically cover behaviour at the buffer boundary.

Suggested implementation:

```rust
/// Regression test: zero stdout bytes before the first proxied protocol byte
/// and after the final proxied byte. This guards the stdout-purity contract
/// stated in the design document and prevents future code from accidentally
/// adding banners, diagnostics, or framing bytes to the protocol stdout path.
#[rstest]
fn regression_zero_bytes_before_first_and_after_last_proxied_byte(runtime: RuntimeFixture) {
    let known_output = b"PROTOCOL_OUTPUT";
    let output_chunks = vec![Ok(LogOutput::StdOut {
        message: known_output.to_vec().into(),
    })];
    let output = make_output_stream(output_chunks);

    // existing assertions and setup for this regression test continue below...
}

/// Regression test: bounded buffering of stdout preserves all bytes across
/// multiple chunks whose total size exceeds the internal buffer size (64 KiB).
/// This specifically exercises the BufReader/BufWriter + copy loop behaviour
/// at and around the buffer boundary to ensure no bytes are lost, duplicated,
/// or reordered when proxying stdout.
#[rstest]
fn regression_stdout_bounded_buffering_preserves_all_bytes(runtime: RuntimeFixture) {
    // Use a total size > 64 KiB to cross the buffer boundary. We pick an odd
    // size to avoid aligning perfectly with any internal buffer sizes.
    const TOTAL_SIZE: usize = 70 * 1024; // 70 KiB
    const CHUNK_SIZE: usize = 8 * 1024 + 123; // ~8 KiB, intentionally non-power-of-two

    let mut expected = Vec::with_capacity(TOTAL_SIZE);
    let mut output_chunks = Vec::new();

    let mut remaining = TOTAL_SIZE;
    let mut byte_value: u8 = 0;

    while remaining > 0 {
        let this_chunk = remaining.min(CHUNK_SIZE);
        let mut chunk = Vec::with_capacity(this_chunk);

        for _ in 0..this_chunk {
            // Deterministic but non-trivial pattern so that reordering or
            // duplication would be visible in the final concat.
            chunk.push(byte_value);
            expected.push(byte_value);
            byte_value = byte_value.wrapping_add(1);
        }

        output_chunks.push(Ok(LogOutput::StdOut {
            message: chunk.into(),
        }));

        remaining -= this_chunk;
    }

    let output = make_output_stream(output_chunks);

    // Run the exec path that proxies stdout from the protocol to the host.
    let (result, captured_stdout) = run_exec_with_proxied_stdout(runtime, output);

    // The exec is expected to succeed; adjust the assertion if this helper
    // reports success differently in your test harness.
    assert_exec_succeeded(&result);

    // The host stdout must exactly equal the concatenation of all stdout chunks,
    // with no extra or missing bytes.
    let captured = captured_stdout.lock().expect("mutex should not poison");
    assert_eq!(
        captured.as_slice(),
        expected.as_slice(),
        "host stdout must be exactly the concatenation of all StdOut chunks \
         even when total size exceeds the bounded buffer size"
    );

```

1. Ensure there is a helper `run_exec_with_proxied_stdout(runtime, output)` returning `(result, captured_stdout)` in this module, matching the pattern used by the existing tests (the first snippet in your message suggests such a helper already exists). If the name or signature differs, update the new test to use the correct helper.
2. Ensure `assert_exec_succeeded(&result)` exists; if your existing tests assert success differently (e.g. `assert_exec_ok(&result)` or pattern matching on `result`), update the assertion accordingly to match the conventions in this file.
3. If your `make_output_stream` or `LogOutput::StdOut` constructors have slightly different argument types (e.g. `Bytes` vs `Vec<u8>`), adjust the `message: chunk.into()` calls to the appropriate conversion method consistent with the rest of the file.
</issue_to_address>

### Comment 5
<location path="tests/bdd_interactive_exec_helpers/steps.rs" line_range="183-186" />
<code_context>
                     Some(bollard::exec::StartExecOptions {
                         detach: false,
                         tty: false,
-                        output_capacity: None
+                        output_capacity: Some(65_536)
                     })
                 );
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding/ensuring explicit coverage that non-protocol exec modes still use `output_capacity: None`.

Since this helper now expects `output_capacity: Some(65_536)` for protocol-mode exec and the code only sets `PROTOCOL_OUTPUT_CAPACITY` for `ExecMode::Protocol`, please add (or confirm) a test/BDD scenario for attached/detached exec that asserts `StartExecOptions.output_capacity` stays `None`. That will protect against future refactors accidentally enabling bounded output for non-protocol modes.

Suggested implementation:

```rust
                    Some(bollard::exec::StartExecOptions {
                        detach: false,
                        tty: false,
                        output_capacity: Some(65_536)
                    })
                );

                // Explicitly cover non-protocol exec: output_capacity must remain None
                let non_protocol_start_exec_options = bollard::exec::StartExecOptions {
                    detach: false,
                    tty: false,
                    output_capacity: None,
                };
                assert_eq!(None, non_protocol_start_exec_options.output_capacity);

                let output_stream = stream::iter(vec![Ok(LogOutput::StdOut {

```

The added check ensures that a `StartExecOptions` instance explicitly configured as non-protocol keeps `output_capacity: None`, but it does not yet exercise your actual helper/exec-mode wiring.

To fully align with the review comment, you should:
1. Identify the helper function (or BDD step) that constructs `StartExecOptions` based on `ExecMode` (e.g., `ExecMode::Attached`, `ExecMode::Detached`, `ExecMode::Protocol`).
2. Add a BDD scenario (or unit test in this file) that:
   - Invokes that helper with a non-protocol mode (attached/detached).
   - Asserts that the resulting `StartExecOptions.output_capacity` is `None`.
3. If the helper currently only has a Protocol-mode path explicitly covered in tests, mirror that structure:
   - Re-use whatever pattern you’re using to assert `output_capacity: Some(65_536)` for `ExecMode::Protocol`.
   - Add analogous expectations for `ExecMode::Attached` / `ExecMode::Detached` asserting `None`.

You’ll need to adapt these steps to the actual helper signatures and step definitions present in `steps.rs`, so the new tests go through the same code path that your production code uses for non-protocol exec.
</issue_to_address>

### Comment 6
<location path="docs/podbot-design.md" line_range="194" />
<code_context>
+  backpressure propagates to the container.
+
+The 64 KiB buffer sizes align with common protocol message sizes (such as
+JSON-RPC frame buffers) and typical OS pipe buffer defaults, keeping latency
+low while preventing unbounded accumulation during high-throughput scenarios.
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** "JSON-RPC" is introduced here without expanding the acronym, which may violate the requirement to define uncommon acronyms on first use.

Consider expanding this as "JSON Remote Procedure Call (JSON-RPC)" the first time it appears, so the acronym is defined explicitly for readers.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.md`

**Instructions:**
Define uncommon acronyms on first use.

</details>
</issue_to_address>

### Comment 7
<location path="docs/execplans/2-5-3-keep-proxy-buffering-bounded.md" line_range="105" />
<code_context>
+  protocol messages to be split into multiple `LogOutput` chunks, increasing
+  per-chunk overhead. Setting it too high defeats the backpressure goal.
+  Severity: medium. Likelihood: low. Mitigation: choose a value that matches
+  typical protocol message sizes (64 KiB is common for JSON-RPC frame
+  buffers). Document the choice in the decision log and design document so it
+  can be tuned later.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** "JSON-RPC" is used here without an accompanying expansion, which may not satisfy the requirement to define uncommon acronyms on first use.

Please expand this on first use, for example: "JSON Remote Procedure Call (JSON-RPC) frame buffers". Subsequent references can then use just "JSON-RPC".

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.md`

**Instructions:**
Define uncommon acronyms on first use.

</details>
</issue_to_address>

### Comment 8
<location path="docs/execplans/2-5-3-keep-proxy-buffering-bounded.md" line_range="323" />
<code_context>
+   error-related bytes written to stdout. The error should surface as a
+   `PodbotError`.
+
+Required BDD test scenarios (add to
+`tests/features/protocol_proxy.feature`):
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** "BDD" is introduced here without being defined, which conflicts with the requirement to define uncommon acronyms on first use.

Consider expanding this as "behaviour-driven development (BDD)" at its first occurrence, so the acronym is clear to readers.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.md`

**Instructions:**
Define uncommon acronyms on first use.

</details>
</issue_to_address>

[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/tests/proxy_helpers/lifecycle_purity.rs

Comment on file

fn startup_purity_no_prefix_bytes(runtime: RuntimeFixture) {
    let output_chunks = vec![Ok(LogOutput::StdOut {
        message: b"STARTUP_OUTPUT".to_vec().into(),
    })];
    let output = make_output_stream(output_chunks);

    let host_stdout = RecordingWriter::new();
    let captured_stdout = host_stdout.bytes.clone();
    let result = run_session(
        runtime,
        b"",
        output,
        Box::pin(RecordingInputWriter::new()),
        host_stdout,
        RecordingWriter::new(),
    );

    assert!(result.is_ok(), "startup should succeed");
    let captured = captured_stdout.lock().expect("mutex should not poison");
    assert_eq!(
        captured.as_slice(),
        b"STARTUP_OUTPUT",
        "host stdout must contain exactly the container output with no prefix"
    );
}

❌ New issue: Code Duplication
The module contains 3 functions with similar structure: lifecycle_purity_no_stdout_bytes,shutdown_purity_no_suffix_bytes,startup_purity_no_prefix_bytes

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== src/engine/connection/exec/protocol.rs (constants) ==="
rg -n "BUFFER_CAPACITY|STDIN_WRITE|OUTPUT_BUFFER|pub.*(const)" src/engine/connection/exec/protocol.rs

echo ""
echo "=== src/engine/connection/exec/mod.rs (PROTOCOL_OUTPUT_CAPACITY) ==="
rg -n "PROTOCOL_OUTPUT_CAPACITY|protocol::" src/engine/connection/exec/mod.rs

echo ""
echo "=== protocol_proxy_bdd.rs host_stdout_contains_no_extra_bytes ==="
rg -n -A 30 'host_stdout_contains_no_extra_bytes|host stdout contains no prefix' src/engine/connection/exec/tests/protocol_proxy_bdd.rs

echo ""
echo "=== lifecycle_purity.rs function names ==="
rg -n "^fn " src/engine/connection/exec/tests/proxy_helpers/lifecycle_purity.rs

echo ""
echo "=== bdd_interactive_exec_helpers/steps.rs output_capacity ==="
rg -n -B2 -A2 "output_capacity" tests/bdd_interactive_exec_helpers/steps.rs

echo ""
echo "=== docs/podbot-design.md JSON-RPC ==="
rg -n "JSON-RPC|JSON Remote" docs/podbot-design.md

echo ""
echo "=== docs/execplans/2-5-3-keep-proxy-buffering-bounded.md JSON-RPC and BDD ==="
rg -n "JSON-RPC|JSON Remote|BDD|behaviour-driven" "docs/execplans/2-5-3-keep-proxy-buffering-bounded.md"

echo ""
echo "=== assert_channel_receives in protocol_proxy_bdd.rs ==="
rg -n "assert_channel_receives" src/engine/connection/exec/tests/protocol_proxy_bdd.rs

Length of output: 4391

---

@leynos: All of the comments are now resolved. Here is the full status, with no annulments required (none of the requirements violated en-GB-oxendict spelling or introduced unnecessary cyclomatic complexity):

#	Location	Status
Overall 1	Rename OUTPUT_BUFFER_CAPACITY → STDIN_WRITE_BUFFER_CAPACITY	✅ Resolved — constant is now pub(super) const STDIN_WRITE_BUFFER_CAPACITY in protocol.rs
Overall 2	BDD step re-checks captured bytes, not just outcome	✅ Resolved — step builds expected from stdout_chunks and calls assert_channel_receives(&expected, …)
1	PROTOCOL_OUTPUT_CAPACITY derived, not duplicated	✅ Resolved — mod.rs line 30: = protocol::STDIN_WRITE_BUFFER_CAPACITY
2	host_stdout_contains_no_extra_bytes asserts byte equality	✅ Resolved — concatenates chunks into expected and passes to assert_channel_receives
3	Empty-stdout lifecycle purity test	✅ Resolved — lifecycle_purity_no_stdout_bytes present in lifecycle_purity.rs
4	>64 KiB multi-chunk regression test	✅ Resolved — regression_stdout_bounded_buffering_preserves_all_bytes present
5	Non-protocol modes assert output_capacity: None	✅ Resolved — steps.rs asserts None for both attached (line 165) and detached (line 207) modes
6	Expand "JSON-RPC" in podbot-design.md	✅ Resolved — "JSON Remote Procedure Call (JSON-RPC)" on first use
7	Expand "JSON-RPC" in execplan	✅ Resolved — expanded on first use at lines 105–106
8	Expand "BDD" in execplan	✅ Resolved — "behaviour-driven development (BDD)" at line 323

[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: 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 `@docs/developers-guide.md`:
- Line 92: The heading "## 4. Stdout Purity Contract" should be changed to
sentence case; update the Markdown heading string in the docs to "## 4. Stdout
purity contract" (replace the exact text "## 4. Stdout Purity Contract" with "##
4. Stdout purity contract") to comply with the docstyle guideline.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 25a52e8b-588e-48af-924b-f62a0bbf2488

📥 Commits

Reviewing files that changed from the base of the PR and between 342b0fd and 847cd3f.

📒 Files selected for processing (2)

• docs/developers-guide.md
• src/engine/connection/exec/tests.rs

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

Check name	Status	Explanation	Resolution
Module-Level Documentation	❓ Inconclusive	Cannot verify module-level documentation compliance—files could not be accessed to confirm all modules contain proper docstrings.	Examine repository files to confirm all new/modified modules contain module-level docstrings (//! or /*! */) before any code.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check module-level docstrings (//! lines) in all new/modified Rust files for PR `#72`
FILES=(
  "src/engine/connection/exec/mod.rs"
  "src/engine/connection/exec/protocol.rs"
  "src/engine/connection/exec/tests/proxy_helpers/lifecycle_purity.rs"
  "src/engine/connection/exec/tests/proxy_helpers/mod.rs"
  "src/engine/connection/exec/tests.rs"
  "tests/bdd_interactive_exec_helpers/steps.rs"
)

for f in "${FILES[@]}"; do
  echo "=== $f ==="
  if [ -f "$f" ]; then
    head -20 "$f"
  else
    echo "FILE NOT FOUND"
  fi
  echo ""
done

# Also find protocol_proxy_bdd.rs
echo "=== protocol_proxy_bdd.rs ==="
fd -e rs protocol_proxy_bdd | xargs head -20

Length of output: 4788

---

@leynos: Yes, this check is fully resolved. Every new and modified Rust module carries a //! docstring as its very first content:

File	Docstring
src/engine/connection/exec/mod.rs	//! Container exec lifecycle with attached/detached modes and terminal handling.
src/engine/connection/exec/protocol.rs	//! Protocol-mode exec proxy loops for byte-preserving stdio forwarding. (plus the full stdout-purity contract)
src/engine/connection/exec/tests/proxy_helpers/lifecycle_purity.rs	//! Lifecycle stream-purity tests covering startup, steady-state, shutdown, and error paths.
src/engine/connection/exec/tests/proxy_helpers/mod.rs	//! Shared protocol proxy test helpers and focused submodules.
src/engine/connection/exec/tests.rs	//! Unit tests for container exec lifecycle handling.
tests/bdd_interactive_exec_helpers/steps.rs	//! Given/When steps for interactive execution scenarios.
src/engine/connection/exec/tests/protocol_proxy_bdd.rs	//! Behavioural tests for protocol exec byte proxying.

No further work is needed for this check.