diff --git a/docs/developers-guide.md b/docs/developers-guide.md index fbe7bfc3..29740cd3 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -51,6 +51,17 @@ src/engine/connection/exec/ | # and re-serialises the JSON before forwarding | # to the container; bounded by | # MAX_FIRST_FRAME_BYTES ++-- acp_policy.rs # Pure ACP runtime policy; method-family matching, +| # denylist decisions, and synthesized JSON-RPC +| # blocked-method error payload construction ++-- acp_frame.rs # Newline-delimited ACP output assembler; preserves +| # permitted frames byte-identically, applies the +| # runtime policy per completed frame, and enters +| # raw fallback on MAX_RUNTIME_FRAME_BYTES overflow ++-- acp_runtime.rs # Runtime enforcement adapter and container-stdin +| # sink; owns bounded WriteCmd mpsc plumbing, +| # synthesized denial responses, and tracing +| # diagnostics for denials and fallback events +-- attached.rs # Attached-mode session, terminal resize, | # SIGWINCH handling, stdin echo forwarding +-- terminal.rs # Terminal size detection (stty), resize helpers, @@ -69,8 +80,8 @@ src/engine/connection/exec/ | # per-call session knobs (stdin forwarding | # disable seam for tests via | # with_protocol_stdin_forwarding_disabled(bool); -| # ACP initialize rewriting opt-in via -| # with_acp_initialize_rewrite_enabled(bool)) +| # ACP enforcement policy selection via +| # with_capability_policy(CapabilityPolicy)) +-- runtime_helpers.rs # Blocking runtime helpers for synchronous exec | # wrappers; block_on_runtime detects nested | # Tokio contexts and routes to block_in_place @@ -388,6 +399,87 @@ frame reaches the container. If masking leaves `params.clientCapabilities` (or Preserve the original line endings, and let malformed or non-ACP frames pass through unchanged. +#### 8.2.2. ACP runtime denylist enforcement contract + +Step 2.6.2 layers a runtime denylist on top of the initialization-time mask. +The implementation is split into four sibling modules under +`src/engine/connection/exec/`: + +- `acp_helpers.rs` (Step 2.6.1) owns first-frame `initialize` masking + and the shared `split_frame_line_ending` and + `read_and_mask_initial_acp_frame` helpers; it is unchanged by the runtime + work apart from the helper extraction. +- `acp_policy.rs` is purely synchronous and depends on neither `tokio` + nor `tracing`. It exports the `MethodFamily`, `MethodDenylist`, + `FrameDecision`, `evaluate_agent_outbound_frame`, and + `build_method_blocked_error` types and functions. +- `acp_frame.rs` is the streaming newline-bounded assembler with the + 128 kibibyte `MAX_RUNTIME_FRAME_BYTES` ceiling. It returns + `(Vec, Option)` 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`, `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. The sink terminates on channel close after every sender has been + dropped; there is no explicit shutdown command. + +Selection between the byte-transparent and enforcement paths happens through +`CapabilityPolicy::{Disabled, MaskOnly, MaskAndDeny}` on +`session::ExecSessionOptions`. `Disabled` is the default and matches the +original `ExecMode::Protocol` contract. `MaskOnly` enables only the first-frame +rewrite. `MaskAndDeny` activates the runtime adapter, the sink task, and the +channel-based stdin forwarder. + +When tests need to drive the runtime adapter directly, they should mirror the +pattern in `acp_runtime_tests.rs` and `acp_runtime_bdd_tests.rs`: build the +assembler with `MethodDenylist::default_families()`, wire it to a bounded +`tokio::sync::mpsc` channel, run scenarios with a `RecordingWriter`-style host +stdout double, and drain the channel after dropping all senders to assert both +directions (byte-identical permitted forwards on host stdout and synthesized +error frames on container stdin). Synthesized JSON-RPC error responses use code +`-32001` and the `data.reason = "podbot_capability_policy"` discriminator; +assertions should compare on parsed structure, not on byte equality, since key +ordering inside the JSON is not stable. + +#### 8.2.3. ACP runtime observability contract + +Step 2.6.2 ships stderr and `tracing::warn!` diagnostics for each denied ACP +method attempt. Metrics and span-based tracing instrumentation are deferred to +Step 2.6.3 or the production rollout before `MaskAndDeny` is enabled by +default or selected through a user-facing override. The metric names below +capture the intended contract for that later work, so the adapter, host +command, and dashboards converge on one shape when it lands: + +- `podbot_acp_policy_state`: gauge labelled by `container_id` and `policy` + (`disabled`, `mask_only`, or `mask_and_deny`), set once when the protocol + session starts and cleared when it ends. +- `podbot_acp_blocked_method_attempts_total`: counter labelled by + `container_id`, `method_family`, and `has_request_id`, incremented for every + `FrameDecision::BlockRequest` and `FrameDecision::BlockNotification`. +- `podbot_acp_writecmd_queue_depth`: gauge labelled by `container_id`, + sampled around sends into the bounded `WriteCmd` channel so operators can + see sustained backpressure before synthesized errors are delayed. +- `podbot_acp_writecmd_send_failures_total`: counter labelled by + `container_id` and `command_kind`, incremented when the sink channel closes + before a forwarded or synthesized frame can be queued. +- `podbot_acp_frame_buffer_overflows_total`: counter labelled by + `container_id`, incremented when `OutboundFrameAssembler` enters raw + fallback because `MAX_RUNTIME_FRAME_BYTES` is exceeded before a newline. +- `podbot_acp_partial_frames_dropped_total`: counter labelled by + `container_id`, incremented when end-of-stream drops a residual partial + frame that could not be classified safely. + +When that instrumentation is introduced, every ACP runtime session should +also attach a tracing span that carries the container ID, the selected +`CapabilityPolicy`, the `SINK_CHANNEL_CAPACITY` value, and the runtime frame +limit. Denial events, send failures, buffer overflow events, and partial-frame +drops should be emitted inside that span so logs and metrics can be correlated +during incident analysis. + ### 8.3. Parameterized tests Use `#[rstest(...)]` to eliminate duplicated test cases. Group related diff --git a/docs/execplans/2-6-1-intercept-acp-initialization.md b/docs/execplans/2-6-1-intercept-acp-initialization.md index 8148f99b..865c726b 100644 --- a/docs/execplans/2-6-1-intercept-acp-initialization.md +++ b/docs/execplans/2-6-1-intercept-acp-initialization.md @@ -179,8 +179,8 @@ Completed result: ### Stage B: Rewrite only ACP initialize capability metadata -Add a narrow helper pipeline in -`src/engine/connection/exec/acp_helpers.rs` that: +Add a narrow helper pipeline in `src/engine/connection/exec/acp_helpers.rs` +that: - reads the first newline-delimited frame from host stdin; - parses it as JSON if possible; diff --git a/docs/execplans/2-6-2-runtime-denylist.md b/docs/execplans/2-6-2-runtime-denylist.md new file mode 100644 index 00000000..b2e7006e --- /dev/null +++ b/docs/execplans/2-6-2-runtime-denylist.md @@ -0,0 +1,1132 @@ +# Step 2.6.2: Enforce a runtime denylist for blocked Agentic Control Protocol (ACP) methods + +This ExecPlan (execution plan) is a living document. The sections +`Constraints`, `Tolerances`, `Risks`, `Progress`, `Surprises and discoveries`, +`Decision log`, and `Outcomes and retrospective` must be kept up to date as +work proceeds. + +Status: COMPLETE (2026-05-02). All Stage I gates pass; the second +Step 2.6 roadmap checkbox is now marked done. + +No `PLANS.md` file exists in this repository as of 2026-05-02, so this ExecPlan +is the governing implementation document for this task. + +## Purpose and big picture + +Step 2.6.1 closed the door on the Agentic Control Protocol (ACP) handshake by +stripping `terminal/*` and `fs/*` capability advertisements from the +client-side `initialize` request before they reach the sandboxed agent. Step +2.6.2 closes the second door: a hosted agent may still attempt to call those +host-delegated methods (`terminal/create`, `terminal/wait_for_exit`, +`fs/read_text_file`, `fs/write_text_file`, and so on) through the protocol +proxy after initialization. Without enforcement at the proxy seam, those calls +would either reach the IDE host (defeating the sandbox) or stall (because the +IDE will not service capabilities it never advertised). Either outcome breaks +the trust boundary that `docs/podbot-design.md` records: + +> Podbot must maintain a runtime denylist for blocked ACP methods and +> return a protocol error if those methods are attempted later in the +> session. + +Observable success for this task: + +- when the protocol proxy is in ACP enforcement mode, an outbound + `terminal/*` or `fs/*` JavaScript Object Notation Remote Procedure Call + (JSON-RPC) request emitted by the hosted agent never reaches host stdout; +- the agent receives a synthesized JSON-RPC error response (with the same + `id`) carrying a Podbot-specific application error code naming the blocked + method; +- a stderr diagnostic line records each denial with the container + identifier, blocked method name, and request id (or `null` for notifications); +- non-blocked frames pass through byte-for-byte, including frames that span + multiple Bollard `LogOutput` chunks; +- malformed or non-JSON-RPC frames pass through unchanged, in keeping with + the tolerant rewriting policy established in 2.6.1; +- the existing protocol-mode invariants from Step 2.5 hold: stdout purity, + bounded buffering, accurate exit codes, no terminal framing; +- when ACP enforcement is disabled, `ExecMode::Protocol` remains a + byte-transparent proxy with no parsing overhead; +- `rstest` unit tests and `rstest-bdd` v0.5.0 behavioural tests cover + happy, unhappy, and edge cases for both directions; +- `docs/podbot-design.md`, `docs/users-guide.md`, and + `docs/developers-guide.md` reflect the shipped behaviour; +- only the second Step 2.6 roadmap checkbox is marked done. The remaining + three checkboxes (richer denial diagnostics, operator override, and the + combined override-behaviour test suite) remain explicit follow-on work. + +## Constraints + +- Scope is limited to Step 2.6.2 in `docs/podbot-roadmap.md`. The operator + override (Step 2.6.4), the consolidated override and handshake test battery + (Step 2.6.5), and any user-facing configuration surface for enabling + delegation are out of scope. Synthesizing a protocol error and emitting a + stderr denial line are in scope because they are the minimum observable + behaviour an enforcement mechanism must produce; Step 2.6.3 remains free to + enrich diagnostic structure later. +- Preserve the protocol proxy contract from Step 2.5: stdout purity, + stderr-only diagnostics, bounded buffering, explicit stdin shutdown ordering, + and accurate exit-code reporting. +- Preserve the tolerant first-frame masking semantics from Step 2.6.1. + Runtime enforcement must compose with init masking, not replace it. +- Default behaviour of `ExecMode::Protocol` must remain a raw byte proxy. + Enforcement activates only when the existing + `ExecSessionOptions::with_capability_policy` opt-in is set + (potentially renamed in this step to reflect both responsibilities). +- Do not add new runtime dependencies. Reuse `serde_json` (re-exported via + `ortho_config::serde_json`), `tokio::sync::mpsc`, and `tracing`, all of which + are already in the workspace. +- Keep every touched module within the 400-line guidance from `AGENTS.md`. + The existing `acp_helpers.rs` is already 263 lines, so the new policy + decisions, the frame assembler, and the output adapter must land in separate + sibling modules: `acp_policy.rs` for pure decisions and the error builder, + `acp_frame.rs` for the newline-based frame assembler (with its 128 kilobyte + ceiling), and `acp_runtime.rs` for the output-direction adapter and the + container-stdin sink task. Promote the family of ACP modules from inline + `#[path = "..."]` declarations inside `protocol.rs` to ordinary + `pub(super) mod ...` entries in `src/engine/connection/exec/mod.rs` so the + modules are reachable from both production and tests without the inline path + declarations. +- Every new module must begin with a `//!` module-level comment. +- Use `rstest` fixtures and parameterized cases for unit coverage; use + `rstest-bdd` v0.5.0 with `StepResult = Result` for behavioural + coverage. Mirror the patterns in + `src/engine/connection/exec/protocol_acp_bdd_tests.rs`. +- Production code must be panic-free. Apply the existing tolerant + pass-through policy: if JSON parsing fails, forward the bytes unchanged. +- Use British English with Oxford spelling + (`-ize`, `-yse`, `-our`, Oxford comma when it improves clarity) in all + documentation and code comments, except for references to external + Application Programming Interface (API) identifiers. +- Run the full Rust gate stack before completion: `make check-fmt`, + `make lint`, and `make test`. Run the documentation gates as well because + Markdown files are touched: `make fmt`, `make markdownlint`, and `make nixie`. +- Pipe long-running gate output through `tee` to + `/tmp/$ACTION-podbot-session-e445b19d.out` so truncated output does not hide + failures. Do not run gates in parallel. + +## Tolerances (exception triggers) + +Stop and escalate (do not improvise) when any of the following occurs. + +- Scope tolerance: implementation requires touching more than ten files or + exceeding roughly 900 net lines added (production plus tests, excluding + generated bindings). +- Interface tolerance: completing the change forces a public Application + Programming Interface (API) signature break in `src/api/exec.rs` or + `src/engine/connection/exec/mod.rs` rather than internal proxy-seam edits. +- Concurrency tolerance: bidirectional injection (output task pushing + synthesized error frames into the input writer) cannot be expressed without + `unsafe`, leaked tasks, or mutexes that span `await` points. +- Dependency tolerance: a new crate is required. +- Iteration tolerance: any of `make check-fmt`, `make lint`, or + `make test` still fails after three focused fix passes against a single + failure mode. +- Backpressure tolerance: the framing assembler cannot honour the existing + 64 kilobyte (`STDIN_BUFFER_CAPACITY`) buffer cap without dropping bytes. +- Ambiguity tolerance: more than one defensible interpretation of an + observable behaviour remains after research; present the options before + proceeding. + +## Risks + +- Risk: bidirectional injection couples the output and input tasks in a + way that is sensitive to cancellation order. A naive shared writer can cause + synthesized denial responses to be lost when the host closes stdin before the + agent receives the error frame. Severity: high. Likelihood: medium. + Mitigation: introduce a dedicated container-stdin sink task that owns the + container input writer and drains a single + `tokio::sync::mpsc::Receiver`. The host-stdin forwarding task and + the output-direction policy adapter both become *senders* rather than + competing writers. The sink drains the channel until every sender has been + dropped; the closed channel is the only terminator. Document the ordering + invariant inline and in `docs/podbot-design.md`. + +- Risk: the existing `STDIN_SETTLE_TIMEOUT` of 50 milliseconds may abort + the input task before queued denial responses are flushed, especially when + the agent emits a blocked call immediately before exiting. Severity: medium. + Likelihood: medium. Mitigation: the dedicated sink task removes the race + entirely because the host-stdin forwarder no longer owns the writer; aborting + it on the settle timeout cannot truncate synthesized errors. The settle + timeout continues to apply to the host-stdin forwarder only. Cover this with + a regression test that emits a blocked call as the final frame and asserts + the synthesized error reaches container stdin. + +- Risk: synthesized error responses arrive at the agent after its own + request timeout has fired, producing a stray response with an unknown `id` + that destabilizes the agent's JSON-RPC client. Severity: medium. Likelihood: + medium. Mitigation: deliver synthesized errors synchronously with the deny + decision (the bounded `mpsc` channel has a small capacity such as 16, so + contention is rare). Add a behavioural assertion that the synthesized error + appears on container stdin within one chunk of the blocked request being + observed on the output stream. + +- Risk: parsing every outbound frame adds CPU and allocation overhead to + the previously raw byte path. Severity: medium. Likelihood: medium. + Mitigation: keep enforcement strictly opt-in via a single `CapabilityPolicy` + enum on `ExecSessionOptions` (default `Disabled`), preserve byte-transparency + on parse failure, and bound the per-frame buffer at a 128 kilobyte ceiling + for the runtime path (chosen because agent-emitted ACP `prompt`-style + payloads can carry embedded resources up to the same scale and exceed the 64 + kilobyte input ceiling). When the buffer fills before a newline is found, + drop the partial frame, log the truncation to stderr exactly once, and fall + back to raw byte forwarding for the remainder of the session. + +- Risk: the framing assembler corrupts a permitted stream by misplacing + newlines that occur inside JSON string literals. Severity: high. Likelihood: + low. Mitigation: ACP frames are JSON-RPC objects on a single line by + construction, but coverage must include scenarios where a JSON string literal + contains a `\n` escape (which is not a real newline byte) and where chunk + boundaries split a multi-byte Unicode Transformation Format (UTF-8) sequence. + +- Risk: applying the denylist to forwarded host stdout could accidentally + emit Podbot-generated bytes into host stdout, breaking the stream purity + contract. Severity: high. Likelihood: low. Mitigation: synthesized error + responses are written to **container stdin** (the agent's input), not host + stdout. Permitted frames are forwarded byte-for-byte from the original slice; + the policy parses to *decide* but never re-serializes before forwarding. Add + a stream-purity assertion to the new behavioural feature, including a + golden-bytes comparison rather than a parsed-JSON comparison. + +- Risk: prefix-based matching (`terminal/`, `fs/`) may both block too + much (an unrelated `terminal-monitor` method, hypothetically) and miss future + ACP methods that do not share the prefix. Severity: medium. Likelihood: low. + Mitigation: the ACP specification scopes capability families with the + trailing `/` separator, so prefix matching with a literal `/` delimiter is + the correct rule. Encode the families as `&[&str] = &["terminal/", "fs/"]` in + one place so future families can be added in a single edit. Cover boundary + cases (`terminal`, `terminalx`, `terminal/`, `terminal/create`) in unit tests. + +- Risk: feature-file edits for `rstest-bdd` are compile-time inputs; + stale generated bindings can mask scenario-name mismatches. Severity: medium. + Likelihood: medium. Mitigation: keep scenario titles synchronized with the + feature file and trigger a clean rebuild + (`cargo clean -p podbot && make test 2>&1 | tee ...`) once if generated + bindings appear stale. + +## Context and orientation + +Read the following first; the plan assumes nothing else. + +- `docs/podbot-roadmap.md` lines 186 to 220 define Step 2.6 and the + remaining checkboxes. The completion criteria for Step 2.6 are at lines 201 + to 207 (sandbox-preserving default, opt-in override, denials recorded on + stderr). +- `docs/podbot-design.md` lines 197 to 226 record the design intent: ACP + hosting must default to sandbox-preserving masking, must defensively reject + blocked methods at runtime, and must surface override decisions as + trust-boundary events. +- `docs/execplans/2-6-1-intercept-acp-initialization.md` describes the + init-time masking implementation that this step extends. +- `src/engine/connection/exec/protocol.rs` is the protocol proxy. It owns + `forward_host_stdin_to_exec_async` (input direction) and + `run_output_loop_async` plus `handle_log_output_chunk` (output direction). + The protocol session is configured by `ProtocolSessionOptions`. +- `src/engine/connection/exec/acp_helpers.rs` holds the pure init-frame + rewriter (`mask_acp_initialize_frame`, `split_frame_line_ending`). Treat this + module as the existing domain seam; new pure policy helpers belong in a + sibling module to keep both files inside the 400-line guidance. +- `src/engine/connection/exec/session.rs` exposes + `ExecSessionOptions::with_capability_policy`. This option is currently dead + code outside tests. It is the natural opt-in to extend. +- `src/engine/connection/exec/helpers.rs` provides + `spawn_stdin_forwarding_task`, the seam through which the input task is + spawned with sole ownership of the container input writer. +- `src/engine/connection/exec/protocol_acp_tests.rs`, + `protocol_acp_forwarding_tests.rs`, and `protocol_acp_bdd_tests.rs` capture + the unit-and-behavioural testing pattern. The `Slot` and `ScenarioState` + patterns from `rstest-bdd` v0.5.0 are required. +- `tests/features/acp_capability_masking.feature` is the existing ACP + feature file. The denylist scenarios go into a new sibling file + (`acp_method_denylist.feature`) so each feature file remains focused and + short. + +ACP framing assumed by this plan, taken from the existing ACP guidance +referenced in `docs/podbot-design.md`: + +- ACP traffic is line-delimited JSON-RPC 2.0. Each frame is a single + JSON-RPC object terminated by `\n` (or `\r\n`). +- Requests carry `method`, `id`, and optional `params`. Responses carry + `id` plus either `result` or `error`. Notifications carry `method` without + `id`. +- Capability families that Podbot blocks by default are `terminal/...` + and `fs/...`. These methods are emitted by the agent and target the client + (the Integrated Development Environment, or IDE), so the bytes flow agent + stdout to host stdout in the protocol proxy. + +The data flow that this step modifies is therefore the agent-to-host direction. +The init-masking work in 2.6.1 lives on the host-to-agent direction; this step +adds the symmetrical enforcement on the return path. + +## Plan of work + +The work is broken into stages. Each stage ends with a validation gate; do not +proceed past a failing validation. + +### Stage A: Confirm the landing zones (no code changes) + +Read the modules listed under `Context and orientation` and confirm: + +- the protocol proxy seam still routes outbound bytes through + `handle_log_output_chunk` for both `LogOutput::StdOut` and + `LogOutput::Console` and routes nothing else to host stdout; +- `ProtocolSessionOptions` is the only opt-in surface that needs + extension; +- no other code path forwards container stdout to host stdout for + `ExecMode::Protocol`. + +Validation: a brief written summary in the `Surprises and discoveries` section +if any of the assumptions above are false. Otherwise proceed. + +### Stage A.5: Architecture spike — sink task vs single bidirectional task + +Before committing to the dedicated container-stdin sink task, run a 30-minute +spike of the alternative described in the Logisphere review: fold both the +host-stdin forwarder and the output-direction policy adapter into a single +`tokio::select!`-driven task that owns the container input writer directly. +Measure the diff in `protocol.rs` and the impact on the existing test +scaffolding. + +Acceptance for the spike: if the single-task fold keeps `protocol.rs` under the +400-line guidance, leaves the existing 2.6.1 tests unchanged, and expresses the +"drain pending denials before shutdown" invariant without `select!` ordering +surprises, prefer it. Otherwise commit to the dedicated sink task and document +the decision in the `Decision log`. Either decision must be recorded before +Stage C proceeds. + +### Stage B: Add the pure policy domain + +Create `src/engine/connection/exec/acp_policy.rs` containing only pure logic. +The module must not depend on `tokio` or `tracing`; the adapter in Stage C is +responsible for all I/O and observability. + +Define a `MethodFamily` value type, a `MethodDenylist` aggregate of families, +the default family list, the `FrameDecision` enum (note the absence of +pre-built error bytes — the decision stays serialization-free so it is +trivially testable), and the two pure functions `evaluate_agent_outbound_frame` +and `build_method_blocked_error`. The full Rust signatures appear in +`Interfaces and dependencies`. + +Behavioural rules for `evaluate_agent_outbound_frame`: + +- On JavaScript Object Notation (JSON) parse failure, return + `Forward` (tolerant pass-through). +- On a JSON-RPC request whose `method` is blocked, return + `BlockRequest` preserving the `id` value as `serde_json::Value` (do not + coerce numeric ids to strings). +- On a JSON-RPC notification whose `method` is blocked, return + `BlockNotification` carrying the method name. +- Frames without a `method` field (responses, batches, malformed + objects) return `Forward`. + +`build_method_blocked_error` produces a JSON-RPC 2.0 error response of the form: + +```json +{ + "jsonrpc": "2.0", + "id": "", + "error": { + "code": -32001, + "message": "Method blocked by Podbot ACP capability policy", + "data": { + "method": "", + "reason": "podbot_capability_policy" + } + } +} +``` + +It appends the supplied line-ending bytes (default `\n` if the original frame +had no recognized line ending). The `-32001` code lives in the JSON-RPC +application error range `-32099..=-32000`; reserve `-32002` for the Step 2.6.4 +"override required" follow-on. Do not use `-32601 Method not found` because the +method exists and the agent could otherwise retry assuming a typo. + +Reuse `split_frame_line_ending` from `acp_helpers.rs`. + +Note on visibility: although these items are reached only through +`exec::protocol` today, future Corbusier conformance work +(`docs/corbusier-conformance-design-for-agents-mcp-wires-and-hooks.md`) needs +the same denylist as a single source of truth. Keep `pub(crate)` for now; the +public boundary established in Step 5.3.1 remains intact. + +Add unit tests under `src/engine/connection/exec/acp_policy_tests.rs` covering: + +- `MethodFamily::matches` boundary cases (`terminal`, `terminalize`, + `terminal/`, `terminal/create`); +- `MethodDenylist::is_blocked` with both default families; +- blocked request with numeric, string, and null `id` (each preserved + as the original JSON type in the synthesized error); +- blocked notification (no id field); +- permitted method passes through; +- malformed JSON returns `Forward`; +- frames without a `method` field (responses) return `Forward`; +- `build_method_blocked_error` output round-trips through `serde_json`, + carries the expected `code`, `message`, `data.method`, and `data.reason` + fields, and preserves the original line ending. + +Validation: +`cargo test -p podbot --lib acp_policy_tests 2>&1 | tee /tmp/test-podbot-session-e445b19d.out` + passes; new tests fail before the module is implemented and pass after. + +### Stage C: Add the frame assembler + +Create `src/engine/connection/exec/acp_frame.rs` containing the streaming +framer. The assembler is also pure (no `tokio` or `tracing`); it returns a +vector of decisions per chunk and lets the adapter perform the actual writes. + +Set `MAX_RUNTIME_FRAME_BYTES` to 128 kilobytes (twice the input ceiling), +chosen because agent-emitted ACP `prompt`-style payloads can be larger than the +host-driven input frames. Define a `FrameOutput` borrow-style enum and an +`OutboundFrameAssembler` struct as described in `Interfaces and dependencies`. +The trailing `&[u8]` on `FrameOutput` carries the original frame bytes +(including line ending) for `Forward` decisions and the original line-ending +bytes for `Decision` results so the adapter can reconstruct the synthesized +error with the same line terminator. + +Behavioural rules for the assembler: + +- `ingest_chunk` returns an iterator over completed frames in the + supplied chunk. On buffer overflow before a newline is observed, emit a + one-shot `Forward` covering the buffered bytes, flip an internal + `raw_fallback` flag, and from then on every chunk is emitted as `Forward` + unchanged. +- `finish` is called at end of stream. The assembler **drops** any + residual partial frame: an unauthorized partial frame must not be forwarded + to host stdout. `finish` returns `None` and the adapter logs the byte count + to stderr exactly once. + +Add unit tests under `src/engine/connection/exec/acp_frame_tests.rs` covering: + +- multi-frame chunks split on every `\n`; +- frames spanning two and three chunk boundaries reassembled correctly; +- chunk that splits a multi-byte UTF-8 sequence at the boundary still + reassembles correctly; +- frame containing a `\\n` escape inside a JSON string literal (no real + newline byte) is treated as a single frame; +- buffer overflow flips to raw fallback and emits subsequent chunks as + `Forward` unchanged; +- residual partial frame at end-of-stream returns `None` from + `finish` and is not forwarded. + +Validation: `cargo test -p podbot --lib acp_frame_tests 2>&1 | tee ...` passes. + +### Stage D: Add the output-direction adapter and the sink task + +Create `src/engine/connection/exec/acp_runtime.rs` containing the adapter and +the container-stdin sink task. This module owns all `tokio::sync::mpsc` and +`tracing` use for the runtime path. + +Define a `WriteCmd` enum (`Forward`, `Synthesized`) describing every byte +written to container stdin. Define `run_container_stdin_sink` as the dedicated +sink task and the `OutboundPolicyAdapter` struct paired with `handle_chunk` and +`finish` methods. Full signatures appear in `Interfaces and dependencies`. + +Behavioural rules for the adapter: + +- For each `FrameOutput::Forward(bytes)`, write the original byte slice + to `host_stdout`. Never re-serialize. +- For each `FrameOutput::Decision(BlockRequest { id, method }, line_ending)`, + call `build_method_blocked_error`, push `WriteCmd::Synthesized` into the + channel, and emit `tracing::warn!` with `target = "podbot::acp::policy"`, the + `container_id`, the blocked `method`, the request `id`, and a stable + `"ACP blocked request denied"` message body. +- For each `FrameOutput::Decision(BlockNotification { method }, _)`, + drop the bytes and emit a `tracing::warn!` with the same target, + `container_id`, blocked `method`, `id = serde_json::Value::Null`, and body + `"ACP blocked notification dropped"`. + +Behavioural rules for the sink: + +- Drain commands until the channel closes, writing and flushing each one in + arrival order. +- On `BrokenPipe` from container stdin (the agent has exited), downgrade + subsequent writes to a single `tracing::warn!` and continue draining the + channel until every sender has been dropped. The protocol session still + completes cleanly and the exit-code reporting path remains intact. +- After the channel closes, call `input.shutdown().await` once and return. + +Modify `src/engine/connection/exec/protocol.rs`: + +- Replace the two ACP booleans on `ProtocolSessionOptions` with a + `pub(super) capability_policy: CapabilityPolicy` field of type + `pub(super) enum CapabilityPolicy { Disabled, MaskOnly, MaskAndDeny }`, + defined in `session.rs`. Default remains `Disabled`. +- In `run_protocol_session_with_io_async`, when the policy is + `MaskAndDeny`: + - Build a bounded `tokio::sync::mpsc::channel::(16)`. + - Spawn `run_container_stdin_sink` as a separate task owning the + container input writer. + - Build the host-stdin forwarding task to send `WriteCmd::Forward` + chunks into the same channel (replacing its current + `tokio::io::copy`-into-writer path). + - Build the `OutboundPolicyAdapter` with the same channel sender and + pass it into `run_output_loop_async`. + - After the output loop returns, drop the adapter sender, await the + host-stdin forwarder under the existing `STDIN_SETTLE_TIMEOUT`, then await + the sink task so it can observe channel close after all senders are gone. +- When the policy is `MaskOnly`, behave exactly as today (init + rewriting on, runtime enforcement off). When the policy is `Disabled`, the + existing byte-transparent code path is taken. + +Validation: +`cargo build --workspace --all-targets --all-features 2>&1 | tee /tmp/build-podbot-session-e445b19d.out` + succeeds, and `cargo test -p podbot --lib 2>&1 | tee ...` passes new unit +tests for the adapter and sink covering: + +- blocked request followed by permitted frame: only the permitted frame + reaches `host_stdout`, and the synthesized error reaches the sink; +- the sink delivers the synthesized error before terminating on channel close; +- the sink handles a `BrokenPipe` from container stdin without + failing the session; +- `WriteCmd::Forward` from the host-stdin forwarder is interleaved + correctly with `WriteCmd::Synthesized` from the adapter; +- adapter is a no-op for `LogOutput::StdErr` chunks, which still flow + to host stderr verbatim. + +### Stage E: Wire enforcement through the session option + +In `src/engine/connection/exec/session.rs`: + +- Define `pub(crate) enum CapabilityPolicy { Disabled, MaskOnly, MaskAndDeny }` + with `pub(crate) const fn allows_runtime_enforcement(self) -> bool` and + `pub(crate) const fn rewrites_initialize(self) -> bool` helpers. +- Replace the existing `rewrite_acp_initialize: bool` field with + `capability_policy: CapabilityPolicy` and add + `with_capability_policy(policy: CapabilityPolicy)`. Update the + `protocol_session_options` translator so it forwards the selected + `CapabilityPolicy` with the same builder name. +- Update the existing tests in `session.rs` so the renamed builder + asserts the combined semantic. + +In `src/engine/connection/exec/mod.rs` and `src/api/exec.rs`: + +- No public surface change. The opt-in is reachable only from the + internal session-options seam, consistent with Step 2.6.1. + +Validation: `cargo test -p podbot --lib session 2>&1 | tee ...` passes. + +### Stage F: Add behavioural coverage with `rstest-bdd` + +Create `tests/features/acp_method_denylist.feature` with scenarios: + +1. Blocked request returns synthesized error and is not forwarded. +2. Blocked notification is dropped silently with a stderr record. +3. Permitted method passes through unchanged byte-for-byte. +4. Frame split across two chunks reassembles before the policy applies. +5. Oversized frame falls back to raw forwarding for the rest of the + session and emits a single stderr fallback record. +6. Permitted frame after a blocked frame still flushes correctly. +7. Container stdin sees the synthesized error response within one chunk + of the blocked request being observed on the output stream, and strictly + before the sink observes channel close. + +Bind the scenarios in `src/engine/connection/exec/acp_runtime_bdd_tests.rs`, +mirroring the `AcpMaskingState` pattern from `protocol_acp_bdd_tests.rs`. Use a +recording writer for `host_stdout` and a recording sink (a +`tokio::sync::mpsc::Receiver` drained into a `Vec`) for container +stdin so the assertions can verify ordering and bytes. + +Validation: `make test 2>&1 | tee ...` passes; the new `.feature` scenarios +appear in the test output and fail before the bindings are present, pass after. + +### Stage G: Parameterized coverage for the framer + +`proptest` is not in the workspace lockfile and adding it would breach the +no-new-dependency constraint. Instead, add an exhaustive parameterized `rstest` +table in `src/engine/connection/exec/acp_frame_tests.rs` that: + +- generates a fixed sequence of permitted JSON-RPC frames as test + data (literal byte fixtures, not random); +- splits the concatenation at every byte boundary + (`for split in 1..bytes.len()`), feeding the two halves through the assembler + in two `ingest_chunk` calls; +- asserts the recorded permitted output equals the original + concatenation byte-for-byte; +- repeats with a three-way split table covering at least 32 distinct + triples, including splits inside JSON string literals and inside multi-byte + UTF-8 sequences. + +Validation: the parameterized cases (at least 256 distinct splits) all pass. If +a future change adds `proptest` to the workspace for unrelated reasons, this +table becomes the seed corpus. + +### Stage H: Documentation + +Update `docs/podbot-design.md` to describe: + +- the runtime denylist policy (which families are blocked, why + trailing-slash prefix matching is correct, JSON-RPC error code `-32001`, the + `data.reason = "podbot_capability_policy"` discriminator); +- the dedicated container-stdin sink task and the + `WriteCmd::{Forward, Synthesized}` channel-close ordering invariant; +- the raw-fallback behaviour on buffer overflow and the + drop-partial-frame behaviour at end of stream; +- the `CapabilityPolicy::{Disabled, MaskOnly, MaskAndDeny}` enum and + the rationale for collapsing the previous two booleans; +- the explicit out-of-scope notes for Steps 2.6.3 to 2.6.5. + +Update `docs/users-guide.md` to describe operator-visible behaviour: + +- ACP enforcement is opt-in until `podbot host` ships; +- when enforcement is on, hosted agents that emit `terminal/*` or + `fs/*` calls receive a JSON-RPC error response (`code: -32001`) and a stderr + `WARN` line records the denial with the method name and request id; +- non-blocked methods pass through byte-for-byte. + +Update `docs/developers-guide.md` to describe the internal architecture: + +- the pure `acp_policy` module separates decision logic from input, + output, or telemetry; +- the pure `acp_frame` module owns the newline-delimited assembler and + its `MAX_RUNTIME_FRAME_BYTES = 128 KiB` ceiling; +- the `acp_runtime` module is the output-direction adapter and the + container-stdin sink task; +- the existing `acp_helpers` module retains the init-time rewriter and + is unchanged by this step; +- the family of ACP modules is declared from + `src/engine/connection/exec/mod.rs` rather than inline `#[path]` attributes + inside `protocol.rs`. + +Validation: `make fmt 2>&1 | tee ...`, `make markdownlint 2>&1 | tee ...`, and +`make nixie 2>&1 | tee ...` succeed. + +### Stage I: Roadmap and final gates + +Update `docs/podbot-roadmap.md` to mark only the second Step 2.6 checkbox done. +Leave the next three checkboxes open. + +Run the full gate stack sequentially with `tee` for each command (do not +parallelize): + +```shell +set -o pipefail +make fmt 2>&1 | tee /tmp/fmt-podbot-session-e445b19d.out +make markdownlint 2>&1 | tee /tmp/markdownlint-podbot-session-e445b19d.out +make nixie 2>&1 | tee /tmp/nixie-podbot-session-e445b19d.out +make check-fmt 2>&1 | tee /tmp/check-fmt-podbot-session-e445b19d.out +make lint 2>&1 | tee /tmp/lint-podbot-session-e445b19d.out +make test 2>&1 | tee /tmp/test-podbot-session-e445b19d.out +``` + +## Validation and acceptance + +Acceptance is observable through the following experiments. + +- Compile-time: `cargo build --workspace --all-targets --all-features` + succeeds and the new modules are present at + `src/engine/connection/exec/acp_policy.rs`, + `src/engine/connection/exec/acp_runtime.rs`, and the corresponding unit and + behavioural test files. +- Unit tests: `cargo test -p podbot --lib` reports the new + `acp_policy_tests`, `acp_runtime_tests`, and the property/parameterized + framer test as passing. Each new test fails when run against the tip of + `main` and passes against this branch. +- Behavioural tests: `cargo test -p podbot --test bdd` (or whichever + test binary already executes existing `acp_capability_masking` scenarios) + reports the seven new `acp_method_denylist` scenarios as passing. +- Integration: a hand-driven check using + `RecordingWriter`-style doubles (the same pattern as + `protocol_acp_forwarding_tests.rs`) demonstrates that: + - feeding a `terminal/create` JSON-RPC request into the simulated + container stdout produces zero bytes on host stdout, one synthesized + JSON-RPC error frame on container stdin, and one stderr warn line; + - feeding a `session/update` request through the same path yields + byte-identical pass-through. +- Documentation: `docs/podbot-design.md`, `docs/users-guide.md`, and + `docs/developers-guide.md` describe the runtime denylist exactly as shipped. +- Roadmap: the second Step 2.6 checkbox is marked done; the third, + fourth, and fifth checkboxes remain open with no other roadmap changes. +- Gates: `make check-fmt`, `make lint`, and `make test` all succeed. + `make fmt`, `make markdownlint`, and `make nixie` succeed. + +Quality criteria: + +- No new `clippy` warnings under `-D warnings`. +- No new `unwrap` or `expect` in production code. +- Every new module begins with `//!` documentation. +- Every public item retains British English Oxford spelling in its + documentation. +- The 400-line guidance holds for every touched module. + +## Idempotence and recovery + +- The new modules are additive; if implementation is interrupted, the + partial change can be reverted by removing the new files and reverting the + small edits to `protocol.rs`, `session.rs`, and the documentation. +- If `rstest-bdd` scenario bindings appear stale, run + `cargo clean -p podbot && make test 2>&1 | tee ...` once. +- If the property/parameterized framer test detects a regression, the + failure case can be added as a deterministic `rstest` case before fixing the + assembler. + +## Agent team execution model + +Run reconnaissance and review concurrently up front so the main implementation +thread stays coherent. + +- Lane A (docs and roadmap reconnaissance owner, may be a sub-agent): + read the roadmap, design, and prior 2.6.1 plan to confirm scope and + documentation surface. Output: a one-paragraph note flagging any surprises + before Stage B begins. +- Lane B (Logisphere design review, runs in parallel during planning): + Pandalump (structure), Wafflecat (alternatives), Buzzy Bee (scale and + observability), Telefono (JSON-RPC contract correctness), Doggylump (failure + modes and ordering), Dinolump (long-term viability). Output feeds the + `Decision log` before Stage C begins. +- Lane C (primary implementation owner, main thread): drive Stages A + through H sequentially, integrating Lane B findings into the design before + writing code. + +Coordination rule: code edits land only in Lane C. Sub-agents may read and +report; they must not write to the working tree. + +## Progress + +- [x] (2026-05-02) Drafted ExecPlan after reading the roadmap, design, + prior 2.6.1 plan, and current ACP module layout. +- [x] (2026-05-02) Logisphere design review completed and folded into + `Decision log` (sink-task model, `CapabilityPolicy` enum, three-module split, + byte-identical forwarding, drop-partial-frame, error data shape, family + matching with `/` boundary). +- [x] (2026-05-02) Stage A landing-zone confirmation. Verified that + `attached.rs` is the only other module reading `LogOutput::StdOut`/`Console`, + but it serves the attached (TTY) mode rather than `ExecMode::Protocol`. The + protocol proxy seam in `src/engine/connection/exec/protocol.rs` remains the + only host-stdout forwarder for protocol mode. +- [x] (2026-05-02) Stage A.5 architecture spike completed. The + single-`select!` fold is conceptually cleaner (no channel, no shutdown + signalling) but requires restructuring both + `forward_host_stdin_to_exec_async` and `spawn_stdin_forwarding_task` to + interleave per-chunk reads inside the output loop, losing the current shared + cancellation seam. The dedicated sink task preserves the existing host-stdin + task structure (one-line change to send into a channel), isolates + cancellation under `STDIN_SETTLE_TIMEOUT`, and makes `WriteCmd` ordering + explicit. Decision: commit to sink-task model. +- [x] (2026-05-02) Stage B pure `acp_policy` module and unit tests + (25 cases passing). Module declared as a child of `protocol` via `#[path]`, + matching the 2.6.1 `acp_helpers` pattern. The `build_method_blocked_error` + function returns `serde_json::Result>` so the production path can + avoid `expect()` on the (practically infallible) serialization step. +- [x] (2026-05-02) Stage C `acp_frame` assembler and unit tests + (16 cases passing). The assembler is fully synchronous with no channel or + `tokio` dependency. `ingest_chunk` returns + `(Vec, Option)` so the adapter can act on + per-chunk fallback events. `finish` returns `Option` so the + adapter logs at most one partial-frame drop per session. +- [x] (2026-05-02) Stage D `acp_runtime` adapter, sink task, and unit + tests (10 cases passing). The `WriteCmd` enum was simplified to two variants + (`Forward`, `Synthesized`); the sink terminates on channel close instead of + an explicit shutdown command, eliminating a race where a misordered shutdown + could drop queued items. +- [x] (2026-05-02) Stage E session-options wiring (`CapabilityPolicy` + enum). `ProtocolSessionOptions::with_capability_policy` replaces the + earlier boolean opt-in and the protocol session splits into + `run_session_with_runtime_enforcement` (channel-based sink path) and + `run_session_without_runtime_enforcement` (existing byte-transparent path). + All 448 workspace unit tests pass. +- [x] (2026-05-02) Stage F `rstest-bdd` behavioural feature + (`tests/features/acp_method_denylist.feature`) and bindings + (`src/engine/connection/exec/acp_runtime_bdd_tests.rs`). 5 scenarios exercise + blocked requests, permitted requests, blocked notifications, multi-chunk + reassembly, and blocked-then-permitted ordering, all asserting both host + stdout (byte-identical permitted forwards) and container stdin (synthesized + error responses with preserved id). +- [x] (2026-05-02) Stage G framer parameterized coverage. The + `every_two_way_split_reassembles_to_original_byte_stream` test exhaustively + splits a five-frame permitted stream at every byte boundary (over 250 + splits). The `three_way_splits_reassemble_...` parameterized table covers 32 + distinct triples. All splits reassemble byte-identically, confirming that + frames forwarded by the assembler are bit-for-bit identical to the original + input regardless of chunk boundaries. +- [x] (2026-05-02) Stage H documentation updates. The runtime denylist + policy, sink-task model, frame ceiling, and `CapabilityPolicy` enum are now + described in `docs/podbot-design.md`. The user-facing behaviour (synthesized + error response, stderr denial line, byte-identical permitted forwards, opt-in + until `podbot host` ships) is in `docs/users-guide.md`. A new section 8.2.2 + in `docs/developers-guide.md` documents the four-module split, the + `CapabilityPolicy` selector, and the recommended testing pattern. All four + touched Markdown files pass `markdownlint`. +- [x] (2026-05-02) Stage I roadmap update and full gate stack. The + second Step 2.6 roadmap checkbox is marked done. Final results: + `make check-fmt` ✓, `make lint` ✓ (clippy `-D warnings`), + `make test` ✓ (492 library tests, all integration suites pass with + 0 failures), `make markdownlint` ✓, `make nixie` ✓. Pre-existing + `make fmt` failures in `users-guide.md` (lines 407-446 and + 640-817) predate this branch and were verified by running + `make fmt` against `git stash`'d state. + +## Surprises and discoveries + +- Discovery: `ExecSessionOptions::with_capability_policy` + is currently dead code outside tests and is the only opt-in seam for ACP + behaviour. Replacing it (and the proposed second boolean) with a single + `CapabilityPolicy` enum keeps the internal API surface minimal while making + the intermediate `MaskOnly` mode representable for diagnostic sessions. +- Discovery: the existing protocol proxy already routes nothing to host + stdout other than `LogOutput::StdOut`/`Console` chunks, so adding the + output-direction interceptor at `handle_log_output_chunk` is the smallest + possible change to the stdout-purity contract. +- Discovery: the ACP method polarity is agent-emitted. `terminal/*` and + `fs/*` requests originate from the hosted agent and target the IDE client + over the agent's stdout. The runtime denylist therefore lives on the + container-to-host (output) direction, and the synthesized error response is + written back to **container stdin** so the agent observes a JSON-RPC error to + its own outbound request. The pure policy function name + `evaluate_agent_outbound_frame` makes this polarity explicit in the type + signature. +- Discovery: `acp_helpers.rs` is already 263 lines and would breach the + 400-line guidance if extended with the assembler, the policy enum, and the + error builder. The three-module split (`acp_helpers`, `acp_policy`, + `acp_frame`, plus the adapter `acp_runtime`) keeps every module under roughly + 150 lines and lets future Corbusier conformance work import the policy + without dragging in the assembler or the runtime adapter. +- Discovery: `bytes::BytesMut` would be a more efficient buffer than + `Vec` for the assembler, but `bytes` is not a direct workspace dependency + (it appears only transitively via `tokio` and `bollard`). The + no-new-dependency constraint forbids promoting it, so the plan uses `Vec` + and reserves a future optimization note for when `bytes` becomes a direct + dependency for unrelated reasons. +- Discovery: `proptest` is not in the workspace lockfile either, so + Stage G uses an exhaustive `rstest` parameterized table over fixed byte + fixtures rather than randomized property generation. + +## Decision log + +- Decision: bundle the synthesized JSON-RPC error response and stderr + denial line into Step 2.6.2. Rationale: enforcement without a response causes + the hosted agent to hang forever waiting for a reply, which would be a worse + user experience than the current absence of enforcement. The roadmap's third + checkbox (richer diagnostics) remains open for follow-on work to enrich the + stderr structure and any operator-facing telemetry. +- Decision: collapse the existing `rewrite_acp_initialize: bool` and + the proposed `enforce_acp_method_denylist: bool` into a single + `CapabilityPolicy::{Disabled, MaskOnly, MaskAndDeny}` enum on + `ExecSessionOptions`, with `with_capability_policy(policy)`. Rationale: the + Logisphere review + observed that two booleans for one trust-boundary decision invite drift; the + explicit `MaskOnly` mode is also useful for diagnostic sessions that want + init masking without runtime denial. The enum keeps every combination + representable through one constructor. +- Decision: introduce a dedicated container-stdin sink task driven by a + `WriteCmd::{Forward, Synthesized}` channel, rather than having the + existing host-stdin forwarder drain a secondary mpsc alongside its + `tokio::io::copy` loop. Rationale: the Logisphere `Doggylump` analysis showed + that draining a secondary queue from inside a `tokio::io::copy` loop is + awkward and collides with the `STDIN_SETTLE_TIMEOUT` race. A dedicated sink + task is the single ordering authority: it drains until every sender has been + dropped, so the output adapter can guarantee that synthesized errors are + flushed before container stdin closes. Both the host-stdin forwarder and the + output adapter become *senders* with no shared writer ownership. +- Decision: keep parsing pure and never re-serialize permitted frames. + Rationale: byte-identical forwarding preserves any agent-side integrity + assumptions (key ordering, whitespace, embedded hashes) and avoids + weaponizing the proxy against ACP extensions that depend on byte-stable + frames. The `OutboundFrameAssembler` retains the original byte slice for + every `Forward` decision; `serde_json::from_slice` is used only for the + decision step. +- Decision: prefix-match capability families using a trailing `/` + delimiter (`terminal/`, `fs/`), encoded as + `MethodFamily { prefix: &'static str }` values. Rationale: ACP scopes + capability families with the `/` separator and introduces methods of the form + `family/operation`. Prefix matching with the delimiter avoids both false + positives (a hypothetical `terminate` method) and false negatives (future + `terminal/...` methods). Encode the family list in a single constant so + adding a new family — or a Step 2.6.4 override allowlist — is a single edit. +- Decision: use JSON-RPC application error code `-32001` with message + `"Method blocked by Podbot ACP capability policy"` and data + `{"method": "", "reason": "podbot_capability_policy"}`. Rationale: + `-32601 Method not found` is reserved for methods the server does not + implement; the agent's method does exist but Podbot is withholding it. + `-32001` lives in the JSON-RPC application range `-32099..=-32000` and gives + operators a stable code to grep for in agent logs. `-32002` is reserved for + the Step 2.6.4 "override required" follow-on. The `data.reason` discriminator + lets agents branch on `reason` without parsing the message string. +- Decision: raise the runtime frame ceiling to 128 kilobytes + (`MAX_RUNTIME_FRAME_BYTES`), distinct from the 64 kilobyte + `STDIN_BUFFER_CAPACITY` used on the input path. Rationale: the Logisphere + `Buzzy Bee` analysis flagged that agent-emitted ACP `prompt`-style payloads + can carry embedded resources that exceed the input ceiling. The runtime path + needs more headroom; reusing the input ceiling would silently truncate + legitimate frames into the raw-fallback path. +- Decision: at end of stream, drop any residual partial frame instead + of forwarding it. Rationale: a partial frame is by definition unauthorized — + the policy never decided on it. Forwarding bytes that have not been + classified would re-introduce the very class of leak this step is meant to + prevent. The byte count of the dropped residual is logged to stderr exactly + once for diagnostics. Note: this is a deliberate tightening from the looser + tolerant-pass-through used at init time in 2.6.1, where the first frame is + always forwarded. +- Decision: on buffer overflow before a newline is observed, flush the + buffered bytes verbatim, set a one-shot raw-fallback flag, and forward all + subsequent bytes raw for the remainder of the session. Rationale: the + established 2.6.1 policy is tolerant pass-through on size limits, and + weaponizing a single oversize frame into a hard failure would harm legitimate + agents. Recording the fallback once on stderr keeps the operator informed + without spamming a tight loop. +- Decision: insert a Stage A.5 architecture spike that times-boxes a + comparison between the dedicated sink task and a single + `tokio::select!`-driven bidirectional task. Rationale: the Logisphere review + explicitly named the single-task fold as the alternative most worth a + checkpoint before committing to the sink design. A bounded spike costs little + and prevents re-architecture later if the simpler shape fits inside the + 400-line guidance. +- Decision: keep the new ACP modules (`acp_policy`, and the upcoming + `acp_frame` and `acp_runtime`) as children of `protocol` via `#[path]` + attributes, matching the established 2.6.1 pattern for `acp_helpers`, instead + of promoting them to siblings under `src/engine/connection/exec/mod.rs` as + the original Logisphere recommendation suggested. Rationale: the existing + `protocol_acp_tests.rs` test module relies on `super::acp_helpers::...` paths + inherited from the inline-`#[path]` pattern. Promoting modules to `mod.rs` + would break those imports without a clear architectural payoff for this step. + The runtime enforcement seam still composes naturally as a child of + `protocol`, and future Corbusier conformance work can re-export the policy + types through a more public path when an actual cross-module consumer arrives. +- Decision: simplify `WriteCmd` to two variants (`Forward`, + `Synthesized`) and terminate the sink purely on channel close, removing the + proposed explicit shutdown variant. Rationale: with an explicit shutdown, a + misordered send before pending `Synthesized` items would drop queued items. + Channel-close is unconditional: every queued command flushes before the sink + sees the terminator. The protocol coordinator drops every sender after the + output stream drains, so the ordering invariant from the Logisphere review + still holds with one fewer moving part. +- Decision: have `build_method_blocked_error` return + `serde_json::Result>` instead of `Vec`. Rationale: AGENTS.md + forbids `.expect()` in production code. The serialization step is practically + infallible because every component of the synthesized payload is a finite, + owned, non-recursive [`serde_json::Value`], but propagating the error keeps + the production path panic-free. The runtime adapter handles the (theoretical) + error by logging it as a `warn!` and continuing without sending a synthesized + response, leaving the agent to time out — strictly better than a hard panic + in the proxy. + +## Outcomes and retrospective + +Shipped behaviour matches every observable success criterion captured in +`Purpose and big picture`: + +- The `MaskAndDeny` mode of `CapabilityPolicy` blocks every agent-emitted + `terminal/*` and `fs/*` request before it reaches host stdout, returns a + synthesized JSON-RPC 2.0 error response with the preserved `id` and the + `-32001` / `data.reason = "podbot_capability_policy"` shape, and emits one + `tracing::warn!` per denial on the `podbot::acp::policy` target. +- Permitted frames (including those split across Bollard chunk boundaries) + are forwarded byte-identically. The `acp_frame` exhaustive parameterized + test confirms reassembly equality at every byte split in a multi-frame + stream. +- Malformed JSON, response/batch shapes, and methods outside the blocked + families pass through unchanged, in keeping with the Step 2.6.1 tolerant + policy. +- Stage 2.5 invariants hold: stdout purity, bounded buffering (now with a + 128 KiB runtime ceiling and a 64 KiB input ceiling), and accurate + exit-code reporting through `settle_stdin_forwarding_task` plus the + sink-task drain. +- `Disabled` and `MaskOnly` paths skip the sink and adapter wiring entirely, + so the byte-transparent contract is preserved when enforcement is off. + +Adjustments from the original draft: + +- Module promotion: kept inline `#[path]` declarations rather than promoting + modules to `mod.rs` (recorded in `Decision log`). +- The explicit shutdown command was removed: the sink terminates purely on + channel close, eliminating the shutdown-ordering race. +- Refactoring for clippy's tight `cognitive-complexity-threshold = 9` and + `too_many_arguments = 4` introduced several extra helper functions inside + `acp_runtime` (`finalize_sink_writer`, `command_bytes`, + `classify_pipe_outcome`, `log_shutdown_outcome`, + `send_synthesized_or_log`, `log_send_failure`, `log_synthesis_failure`, + `log_denial`, `emit_fallback_warning`, `warn_buffer_overflow`, + `warn_partial_frame_drop`) and an `AdapterOutputIo` parameter struct in + `protocol.rs`. The decomposition makes each function single-purpose and + trivially testable. + +Follow-on work (deliberately out of scope for 2.6.2): + +- Step 2.6.3: enrich denial diagnostics with the metrics contract in + `docs/developers-guide.md`: blocked method attempt counters, enforcement + policy state gauge, bounded `WriteCmd` channel queue-depth or send-failure + metrics, frame buffer overflow counters, partial-frame drop counters, and a + tracing span that correlates those events by container ID and selected + `CapabilityPolicy`. +- Step 2.6.4: explicit operator override that flips selected blocked + methods back to `Forward`. The `MethodDenylist` struct already models the + family list as a `&'static [MethodFamily]`, so the override surface can + be added without changing the policy's decision shape. Reserve JSON-RPC + application code `-32002` for an "override required" variant when the + policy is partially relaxed. +- Step 2.6.5: end-to-end tests that drive a real ACP session through the + full `podbot host` pipeline once that command lands; the existing + `OutboundPolicyAdapter` and sink task already provide the seams those + tests will need. + +Lessons: + +- Bidirectional injection problems are genuinely solved by a single + ordering authority. The dedicated sink task removed every cancellation + and shutdown race the original two-boolean design would have left open, + and the `WriteCmd` enum makes the queue contents trivially diffable in + tests. +- `cognitive-complexity-threshold = 9` is unforgiving for handlers with + multiple `tracing::warn!` arms; extracting per-arm log helpers is worth + it because every helper becomes individually inspectable. +- The Logisphere review caught the agent-vs-host polarity confusion early; + naming the policy function `evaluate_agent_outbound_frame` fixed the + polarity at the type level rather than relying on doc comments. + +## Interfaces and dependencies + +In `src/engine/connection/exec/acp_policy.rs`, define (no `tokio`, no +`tracing`): + +```rust +pub(crate) struct MethodFamily { + pub(crate) prefix: &'static str, +} + +impl MethodFamily { + pub(crate) fn matches(&self, method: &str) -> bool { + method + .strip_prefix(self.prefix) + .is_some_and(|rest| !rest.is_empty()) + } +} + +pub(crate) struct MethodDenylist { + families: &'static [MethodFamily], +} + +pub(crate) const DEFAULT_BLOCKED_FAMILIES: &[MethodFamily] = &[ + MethodFamily { prefix: "terminal/" }, + MethodFamily { prefix: "fs/" }, +]; + +impl MethodDenylist { + pub(crate) const fn new(families: &'static [MethodFamily]) -> Self { + Self { families } + } + + pub(crate) fn is_blocked(&self, method: &str) -> bool { + self.families.iter().any(|family| family.matches(method)) + } +} + +pub(crate) enum FrameDecision { + Forward, + BlockNotification { method: String }, + BlockRequest { + id: serde_json::Value, + method: String, + }, +} + +pub(crate) fn evaluate_agent_outbound_frame( + frame: &[u8], + denylist: &MethodDenylist, +) -> FrameDecision; + +pub(crate) fn build_method_blocked_error( + id: &serde_json::Value, + method: &str, + line_ending: &[u8], +) -> serde_json::Result>; +``` + +In `src/engine/connection/exec/acp_frame.rs`, define (no `tokio`, no `tracing`): + +```rust +pub(crate) const MAX_RUNTIME_FRAME_BYTES: usize = 131_072; + +pub(crate) enum FrameOutput<'a> { + Forward(&'a [u8]), + Decision(FrameDecision, &'a [u8]), +} + +pub(crate) struct OutboundFrameAssembler { + buffer: Vec, + denylist: MethodDenylist, + raw_fallback: bool, + fallback_logged: bool, +} + +impl OutboundFrameAssembler { + pub(crate) fn new(denylist: MethodDenylist) -> Self; + + pub(crate) fn ingest_chunk<'a>( + &'a mut self, + chunk: &'a [u8], + ) -> impl Iterator> + 'a; + + pub(crate) fn finish(&mut self) -> Option>; +} +``` + +In `src/engine/connection/exec/acp_runtime.rs`, define (this is the only ACP +module that touches `tokio::sync::mpsc` and `tracing`): + +```rust +pub(super) enum WriteCmd { + Forward(Vec), + Synthesized(Vec), +} + +pub(super) async fn run_container_stdin_sink( + input: std::pin::Pin>, + commands: tokio::sync::mpsc::Receiver, +) -> std::io::Result<()>; + +pub(super) struct OutboundPolicyAdapter { + assembler: OutboundFrameAssembler, + sender: tokio::sync::mpsc::Sender, + container_id: String, +} + +impl OutboundPolicyAdapter { + pub(super) fn new( + assembler: OutboundFrameAssembler, + sender: tokio::sync::mpsc::Sender, + container_id: impl Into, + ) -> Self; + + pub(super) async fn handle_chunk( + &mut self, + chunk: &[u8], + host_stdout: &mut W, + ) -> Result<(), crate::error::PodbotError>; + + pub(super) async fn finish( + &mut self, + host_stdout: &mut W, + ) -> Result<(), crate::error::PodbotError>; +} +``` + +In `src/engine/connection/exec/session.rs`, replace the +`rewrite_acp_initialize: bool` field with: + +```rust +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub(crate) enum CapabilityPolicy { + #[default] + Disabled, + MaskOnly, + MaskAndDeny, +} + +impl CapabilityPolicy { + pub(crate) const fn rewrites_initialize(self) -> bool { + matches!(self, Self::MaskOnly | Self::MaskAndDeny) + } + + pub(crate) const fn allows_runtime_enforcement(self) -> bool { + matches!(self, Self::MaskAndDeny) + } +} +``` + +In `src/engine/connection/exec/protocol.rs`, the new `ProtocolSessionOptions` +field is `capability_policy: CapabilityPolicy`, defaulting to `Disabled`. The +two prior boolean fields are removed; every call site is migrated to +`with_capability_policy` within the same change. + +No new external dependencies are introduced. `tokio::sync::mpsc` and `tracing` +are already in the workspace; `serde_json` is reached through the existing +`ortho_config::serde_json` re-export. diff --git a/docs/podbot-design.md b/docs/podbot-design.md index bacae44d..e4233564 100644 --- a/docs/podbot-design.md +++ b/docs/podbot-design.md @@ -219,8 +219,50 @@ ACP masking is an implementation requirement, not a documentation preference: - If the first protocol frame is malformed or not an ACP `initialize` request, Podbot forwards it unchanged rather than guessing another protocol's semantics. -- Podbot must maintain a runtime denylist for blocked ACP methods and return a - protocol error if those methods are attempted later in the session. +- Podbot enforces a runtime denylist for blocked ACP methods after + initialization. The denylist matches every method whose name begins with one + of the blocked capability prefixes (`terminal/` or `fs/`) followed by a + non-empty operation name, so `terminal/create` and `fs/read_text_file` are + blocked while a hypothetical unrelated `terminalize` is not. Each blocked + request produces a synthesized JavaScript Object Notation Remote Procedure + Call (JSON-RPC) 2.0 error response with the original `id`, the + application-defined error code `-32001`, the stable message + `Method blocked by Podbot ACP capability policy`, and a structured + `data.reason = "podbot_capability_policy"` discriminator so agents can branch + on `reason` rather than parsing the message string. The error response code + `-32601 Method not found` is deliberately avoided because the method exists; + Podbot is refusing to route it. Reserve `-32002` for the future "override + required" follow-on. Each denial also emits a single `tracing::warn!` line + with `target = "podbot::acp::policy"` carrying the container identifier, the + blocked method name, and the request `id` (or `null` for notifications). +- Runtime enforcement runs alongside initialization-time masking through a + single `CapabilityPolicy::{Disabled, MaskOnly, MaskAndDeny}` opt-in. The + default is `Disabled`, preserving the byte-transparent `ExecMode::Protocol` + 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, Synthesized}` 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 + sender after the output stream drains, so the sink processes every queued + synthesized response before it observes channel close and shuts container + stdin. There is no explicit shutdown command. The sink tolerates `BrokenPipe` + from container stdin by logging a single warning and continuing to drain the + channel, preserving the existing exit-code reporting path. +- The output-direction frame assembler buffers up to 128 kibibytes of + agent-emitted bytes while searching for a frame's terminating newline. On + overflow before a newline is observed, the assembler flushes the buffered + bytes verbatim, sets a one-shot raw-fallback flag, and forwards every + subsequent chunk unchanged. At end of stream, any residual partial frame is + **dropped**: an unauthorized partial frame must never reach host stdout. Both + the overflow fallback and the partial-frame drop record one stderr `warn!` + per session. +- Permitted frames are forwarded byte-identically. The policy parses to + decide; it never re-serializes. This preserves any agent-side integrity + assumptions such as key ordering, whitespace, or embedded hashes, and keeps + the proxy correct for ACP extensions that depend on byte-stable frames. - The delegation override must be explicit, disabled by default, and surfaced in logs as a trust-boundary change. diff --git a/docs/users-guide.md b/docs/users-guide.md index 673ea02d..771a7a18 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -63,14 +63,23 @@ This subcommand is temporarily unavailable in the current release. If you run session. Use `podbot run` or the library exec API for now; the release notes will call out when hosted mode becomes available. -When hosted mode uses Agentic Control Protocol (ACP), podbot masks -`terminal/*` and `fs/*` capabilities from the initial ACP `initialize` -request before forwarding it to the sandboxed agent. This masking currently -applies only to the protocol/library path used by hosted mode and ACP until -`podbot host` is implemented. The scope and behaviour may change when -`podbot host` ships, but for now it keeps the default trust boundary aligned -with the container sandbox even when the hosting client advertises broader ACP -support. +When hosted mode uses Agentic Control Protocol (ACP), podbot masks `terminal/*` +and `fs/*` capabilities from the initial ACP `initialize` request before +forwarding it to the sandboxed agent. After initialization, podbot also +enforces a runtime denylist on agent-emitted requests in those capability +families. If the hosted agent attempts a method such as `terminal/create` or +`fs/read_text_file`, podbot refuses to forward the request and returns a +JSON-RPC 2.0 error response carrying the original request `id`, error code +`-32001`, message `Method blocked by Podbot ACP capability policy`, and a +structured `data.reason = "podbot_capability_policy"` field so the agent can +branch on `reason` programmatically. Each denial also produces a single warning +line on podbot's stderr with the target `podbot::acp::policy`, the container +identifier, the blocked method name, and the request `id` (or `null` for +notifications). Permitted methods pass through byte-for-byte. Both the +initialization-time masking and the runtime denylist apply only to the +protocol/library path used by hosted mode and ACP until `podbot host` is +implemented; the operator override to opt back in to host-side delegation is +tracked in roadmap Step 2.6.4. | Option | Required | Default | Description | | -------------- | -------- | --------------- | ------------------------------------------ | diff --git a/src/engine/connection/exec/acp_frame.rs b/src/engine/connection/exec/acp_frame.rs new file mode 100644 index 00000000..e00b54e0 --- /dev/null +++ b/src/engine/connection/exec/acp_frame.rs @@ -0,0 +1,248 @@ +//! Newline-delimited frame assembler for Agentic Control Protocol (ACP) +//! runtime enforcement. +//! +//! The Bollard exec stream delivers `LogOutput` chunks of arbitrary size; ACP +//! frames may begin and end at any byte offset within the stream. This module +//! buffers incoming bytes until a `\n` byte completes a frame, then asks the +//! pure policy in `acp_policy` to decide whether the frame should be +//! forwarded byte-identically, dropped silently, or replaced by a synthesized +//! error response. +//! +//! ## Design invariants +//! +//! - Permitted frames are forwarded **verbatim** (the original byte slice, +//! including its line ending). The policy parses to decide; it never +//! re-serializes. This preserves any agent-side integrity assumptions +//! such as key ordering or embedded hashes. +//! - The buffer is bounded by [`MAX_RUNTIME_FRAME_BYTES`] (128 KiB). When +//! the buffer fills before a newline is observed, the assembler flushes +//! the buffered bytes verbatim, sets a one-shot raw-fallback flag, and +//! forwards every subsequent chunk unchanged for the rest of the session. +//! The fallback is logged exactly once by the adapter. +//! - At end of stream, any residual partial frame is **dropped**. A frame +//! that has not been classified must never reach host stdout, since +//! forwarding unauthorized bytes would re-introduce the leak this step +//! is meant to prevent. The adapter logs the dropped byte count once. +//! +//! ## Concurrency +//! +//! Each protocol session owns one assembler on a single Tokio task. The +//! assembler holds no locks, no channels, and no `tokio` types; it is +//! purely synchronous data manipulation. + +use ortho_config::serde_json; + +use super::acp_policy::{FrameDecision, MethodDenylist, evaluate_agent_outbound_frame}; + +/// Maximum bytes buffered while searching for a frame's terminating newline. +/// +/// 128 kibibytes — twice the input ceiling, since agent-emitted ACP +/// `prompt`-style payloads can carry embedded resources that exceed the +/// host-driven input frames. +pub(crate) const MAX_RUNTIME_FRAME_BYTES: usize = 131_072; + +/// Outcome for a single completed frame produced by +/// [`OutboundFrameAssembler`]. +/// +/// `Forward` carries the verbatim bytes that the adapter must write to host +/// stdout (including any original line ending). `Decision` carries a +/// non-forward policy verdict together with the original line-ending bytes +/// that the adapter should reuse when synthesizing an error response. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum FrameOutput { + /// Forward these bytes to host stdout unchanged. + Forward(Vec), + /// Apply the policy decision; the trailing slice is the original line + /// ending (`b""`, `b"\n"`, or `b"\r\n"`) for synthesized responses. + Decision(DeniedFrameDecision, Vec), +} + +/// Non-forward policy decision emitted for a completed frame. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum DeniedFrameDecision { + /// Drop a blocked notification without generating a response. + BlockNotification { method: String }, + /// Generate a JSON-RPC error response for a blocked request. + BlockRequest { + id: serde_json::Value, + method: String, + }, +} + +/// Reason recorded once when the assembler enters raw-fallback mode at end +/// of stream or after the buffer overflows. The adapter logs exactly one +/// stderr record per session. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum FallbackReason { + /// The buffer reached [`MAX_RUNTIME_FRAME_BYTES`] before a newline was + /// observed. The assembler emitted the buffered bytes verbatim and + /// forwards every subsequent chunk unchanged. + BufferOverflow, + /// End of stream was reached with bytes still buffered. The bytes are + /// **dropped** — they have not been classified by the policy. + DroppedPartialFrame { + /// Number of buffered bytes that were dropped. + byte_count: usize, + }, +} + +/// Streaming assembler that splits ACP output into newline-delimited frames +/// and applies the supplied [`MethodDenylist`]. +#[derive(Debug)] +pub(crate) struct OutboundFrameAssembler { + buffer: Vec, + denylist: MethodDenylist, + raw_fallback: bool, +} + +/// Produces the output for a chunk received while the assembler is in +/// raw-fallback mode: forward the chunk verbatim, or yield an empty list +/// if the chunk is empty. +fn forward_raw_chunk(chunk: &[u8]) -> (Vec, Option) { + let outputs = if chunk.is_empty() { + Vec::new() + } else { + vec![FrameOutput::Forward(chunk.to_vec())] + }; + (outputs, None) +} + +impl OutboundFrameAssembler { + /// Construct a new assembler over the supplied denylist. + pub(crate) fn new(denylist: MethodDenylist) -> Self { + Self { + buffer: Vec::with_capacity(8_192), + denylist, + raw_fallback: false, + } + } + + /// Appends `pending` to the internal buffer, or triggers a raw fallback if + /// doing so would exceed `MAX_RUNTIME_FRAME_BYTES`. + /// + /// Returns `Some(FallbackReason::BufferOverflow)` when fallback is triggered, + /// `None` otherwise. Any overflow output is appended to `outputs`. + fn buffer_or_overflow_tail( + &mut self, + pending: &[u8], + outputs: &mut Vec, + ) -> Option { + if self.buffer.len() + pending.len() > MAX_RUNTIME_FRAME_BYTES { + outputs.push(self.flush_buffer_for_overflow(pending)); + self.raw_fallback = true; + Some(FallbackReason::BufferOverflow) + } else { + self.buffer.extend_from_slice(pending); + None + } + } + + /// Process one chunk of agent-outbound bytes. + /// + /// Returns the sequence of [`FrameOutput`] decisions that the chunk + /// produced, in order. When the assembler is in raw-fallback mode every + /// chunk yields a single [`FrameOutput::Forward`] holding the chunk + /// verbatim. + pub(crate) fn ingest_chunk( + &mut self, + chunk: &[u8], + ) -> (Vec, Option) { + if self.raw_fallback { + return forward_raw_chunk(chunk); + } + + 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; buffer the tail or fall back. + let pending = chunk.get(cursor..).unwrap_or(&[]); + fallback = self.buffer_or_overflow_tail(pending, &mut outputs); + break; + } + + (outputs, fallback) + } + + fn complete_frame(&mut self, fresh_bytes: &[u8]) -> FrameOutput { + if self.buffer.is_empty() { + return classify_frame(fresh_bytes, &self.denylist); + } + self.buffer.extend_from_slice(fresh_bytes); + let frame = std::mem::take(&mut self.buffer); + classify_frame(&frame, &self.denylist) + } + + fn flush_buffer_for_overflow(&mut self, pending: &[u8]) -> FrameOutput { + let mut bytes = std::mem::take(&mut self.buffer); + bytes.extend_from_slice(pending); + FrameOutput::Forward(bytes) + } + + /// Finalize the assembler at end of stream. + /// + /// If a partial frame remains buffered it is **dropped** and the byte + /// count is reported via [`FallbackReason::DroppedPartialFrame`] so the + /// adapter can log it exactly once. When the assembler is already in + /// raw-fallback mode (overflow during the session), `finish` returns + /// `None` because every byte has already been forwarded verbatim. + pub(crate) fn finish(&mut self) -> Option { + if self.raw_fallback || self.buffer.is_empty() { + return None; + } + let byte_count = self.buffer.len(); + self.buffer.clear(); + Some(FallbackReason::DroppedPartialFrame { byte_count }) + } + + /// Return `true` when the assembler has fallen back to raw forwarding. + #[cfg(test)] + pub(crate) const fn is_raw_fallback(&self) -> bool { + self.raw_fallback + } +} + +fn classify_frame(frame_bytes: &[u8], denylist: &MethodDenylist) -> FrameOutput { + let decision = evaluate_agent_outbound_frame(frame_bytes, denylist); + match decision { + FrameDecision::Forward => FrameOutput::Forward(frame_bytes.to_vec()), + FrameDecision::BlockNotification { method } => { + let line_ending = trailing_line_ending(frame_bytes).to_vec(); + FrameOutput::Decision( + DeniedFrameDecision::BlockNotification { method }, + line_ending, + ) + } + FrameDecision::BlockRequest { id, method } => { + let line_ending = trailing_line_ending(frame_bytes).to_vec(); + FrameOutput::Decision( + DeniedFrameDecision::BlockRequest { id, method }, + line_ending, + ) + } + } +} + +fn trailing_line_ending(frame: &[u8]) -> &[u8] { + if frame.ends_with(b"\r\n") { + b"\r\n" + } else if frame.ends_with(b"\n") { + b"\n" + } else { + b"" + } +} + +#[cfg(test)] +#[path = "acp_frame_tests.rs"] +mod tests; diff --git a/src/engine/connection/exec/acp_frame_tests.rs b/src/engine/connection/exec/acp_frame_tests.rs new file mode 100644 index 00000000..b0e58155 --- /dev/null +++ b/src/engine/connection/exec/acp_frame_tests.rs @@ -0,0 +1,407 @@ +//! Unit tests for the streaming Agentic Control Protocol (ACP) frame +//! assembler. +//! +//! These tests exercise the chunk-splitting, multi-chunk reassembly, +//! buffer-overflow fallback, and end-of-stream-drop behaviours of +//! [`OutboundFrameAssembler`] without invoking any I/O or async runtime. + +use ortho_config::serde_json::{self, Value}; +use rstest::rstest; + +use super::{ + DeniedFrameDecision, FallbackReason, FrameOutput, MAX_RUNTIME_FRAME_BYTES, + OutboundFrameAssembler, +}; +use crate::engine::connection::exec::acp_policy::MethodDenylist; + +fn permitted_frame(method: &str, line_ending: &[u8]) -> Vec { + let mut bytes = serde_json::to_vec(&serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "method": method, + "params": {}, + })) + .expect("frame serializes"); + bytes.extend_from_slice(line_ending); + bytes +} + +fn blocked_request_frame(id: &Value, method: &str) -> Vec { + let mut bytes = serde_json::to_vec(&serde_json::json!({ + "jsonrpc": "2.0", + "id": id, + "method": method, + "params": {}, + })) + .expect("frame serializes"); + bytes.push(b'\n'); + bytes +} + +fn assembler() -> OutboundFrameAssembler { + OutboundFrameAssembler::new(MethodDenylist::default_families()) +} + +fn collect_forward_bytes(outputs: &[FrameOutput]) -> Vec { + outputs + .iter() + .filter_map(|output| match output { + FrameOutput::Forward(bytes) => Some(bytes.clone()), + FrameOutput::Decision(_, _) => None, + }) + .fold(Vec::new(), |mut acc, mut bytes| { + acc.append(&mut bytes); + acc + }) +} + +#[test] +fn single_permitted_frame_is_forwarded_verbatim() { + let mut framer = assembler(); + let frame = permitted_frame("session/new", b"\n"); + + let (outputs, fallback) = framer.ingest_chunk(&frame); + + assert!(fallback.is_none()); + assert_eq!(outputs, vec![FrameOutput::Forward(frame.clone())]); + assert!(framer.finish().is_none(), "no residual buffered bytes"); +} + +#[test] +fn multiple_frames_in_one_chunk_split_on_each_newline() { + let mut framer = assembler(); + let mut chunk = permitted_frame("session/new", b"\n"); + chunk.extend_from_slice(&permitted_frame("session/update", b"\n")); + + let (outputs, fallback) = framer.ingest_chunk(&chunk); + + assert!(fallback.is_none()); + assert_eq!(outputs.len(), 2); + assert_eq!(collect_forward_bytes(&outputs), chunk); +} + +#[test] +fn frame_split_across_two_chunks_reassembles_correctly() { + let mut framer = assembler(); + let frame = permitted_frame("session/new", b"\n"); + let split_at = frame.len().div_euclid(2); + let first = frame.get(..split_at).expect("split prefix"); + let second = frame.get(split_at..).expect("split suffix"); + + let (outputs_a, fallback_a) = framer.ingest_chunk(first); + let (outputs_b, fallback_b) = framer.ingest_chunk(second); + + assert!(fallback_a.is_none() && fallback_b.is_none()); + assert!( + outputs_a.is_empty(), + "no frame is complete after first chunk" + ); + assert_eq!(outputs_b, vec![FrameOutput::Forward(frame)]); +} + +#[test] +fn frame_split_across_three_chunks_reassembles_correctly() { + let mut framer = assembler(); + let frame = permitted_frame("session/update", b"\n"); + let third = frame.len().div_euclid(3); + let two_thirds = frame.len().saturating_mul(2).div_euclid(3); + let parts = [ + frame.get(..third).expect("first third"), + frame.get(third..two_thirds).expect("middle third"), + frame.get(two_thirds..).expect("last third"), + ]; + + let mut all_outputs = Vec::new(); + for part in parts { + let (outputs, fallback) = framer.ingest_chunk(part); + assert!(fallback.is_none()); + all_outputs.extend(outputs); + } + + assert_eq!(all_outputs, vec![FrameOutput::Forward(frame)]); +} + +#[test] +fn blocked_request_emits_decision_with_line_ending() { + let mut framer = assembler(); + let frame = blocked_request_frame(&serde_json::json!(7), "terminal/create"); + + let (outputs, fallback) = framer.ingest_chunk(&frame); + + assert!(fallback.is_none()); + assert_eq!(outputs.len(), 1); + match outputs.first() { + Some(FrameOutput::Decision( + DeniedFrameDecision::BlockRequest { id, method }, + line_ending, + )) => { + assert_eq!(id, &serde_json::json!(7)); + assert_eq!(method, "terminal/create"); + assert_eq!(line_ending, b"\n"); + } + other => panic!("expected blocked request decision, got {other:?}"), + } +} + +#[test] +fn blocked_notification_emits_decision_without_id() { + let mut framer = assembler(); + let mut frame = serde_json::to_vec(&serde_json::json!({ + "jsonrpc": "2.0", + "method": "fs/changed", + })) + .expect("frame serializes"); + frame.push(b'\n'); + + let (outputs, _) = framer.ingest_chunk(&frame); + + match outputs.first() { + Some(FrameOutput::Decision( + DeniedFrameDecision::BlockNotification { method }, + line_ending, + )) => { + assert_eq!(method, "fs/changed"); + assert_eq!(line_ending, b"\n"); + } + other => panic!("expected blocked notification decision, got {other:?}"), + } +} + +#[rstest] +#[case::lf(b"\n" as &[u8])] +#[case::crlf(b"\r\n" as &[u8])] +fn line_ending_is_preserved_in_decision_output(#[case] line_ending: &[u8]) { + let mut framer = assembler(); + let mut frame = serde_json::to_vec(&serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "terminal/create", + })) + .expect("frame serializes"); + frame.extend_from_slice(line_ending); + + let (outputs, _) = framer.ingest_chunk(&frame); + + match outputs.first() { + Some(FrameOutput::Decision(_, observed_line_ending)) => { + assert_eq!(observed_line_ending.as_slice(), line_ending); + } + other => panic!("expected decision with line ending, got {other:?}"), + } +} + +#[test] +fn frame_with_escaped_newline_in_string_treated_as_single_frame() { + let mut framer = assembler(); + let mut frame = serde_json::to_vec(&serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "session/new", + "params": {"text": "line one\\nline two"}, + })) + .expect("frame with escaped newline serializes"); + frame.push(b'\n'); + + let (outputs, _) = framer.ingest_chunk(&frame); + + assert_eq!(outputs, vec![FrameOutput::Forward(frame)]); +} + +#[test] +fn permitted_frame_after_blocked_frame_still_forwards() { + let mut framer = assembler(); + let mut chunk = blocked_request_frame(&serde_json::json!(1), "terminal/create"); + let permitted = permitted_frame("session/new", b"\n"); + chunk.extend_from_slice(&permitted); + + let (outputs, _) = framer.ingest_chunk(&chunk); + + assert_eq!(outputs.len(), 2); + assert!(matches!( + outputs.first(), + Some(FrameOutput::Decision( + DeniedFrameDecision::BlockRequest { .. }, + _ + )) + )); + match outputs.get(1) { + Some(FrameOutput::Forward(bytes)) => assert_eq!(bytes, &permitted), + other => panic!("expected permitted forward, got {other:?}"), + } +} + +#[test] +fn buffer_overflow_flushes_buffered_bytes_and_enters_raw_fallback() { + let mut framer = assembler(); + let oversize = vec![b'X'; MAX_RUNTIME_FRAME_BYTES + 1024]; + + let (outputs, fallback) = framer.ingest_chunk(&oversize); + + assert_eq!(fallback, Some(FallbackReason::BufferOverflow)); + assert!(framer.is_raw_fallback()); + assert_eq!(collect_forward_bytes(&outputs), oversize); +} + +#[test] +fn raw_fallback_forwards_subsequent_chunks_unchanged() { + let mut framer = assembler(); + let oversize = vec![b'Y'; MAX_RUNTIME_FRAME_BYTES + 1]; + let _ = framer.ingest_chunk(&oversize); + + let (outputs, fallback) = framer.ingest_chunk(b"trailing-bytes\n"); + + assert!(fallback.is_none()); + assert_eq!( + outputs, + vec![FrameOutput::Forward(b"trailing-bytes\n".to_vec())] + ); +} + +#[test] +fn finish_drops_residual_partial_frame_and_reports_byte_count() { + let mut framer = assembler(); + let partial = b"{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"session/new\""; + + let _ = framer.ingest_chunk(partial); + let fallback = framer.finish(); + + assert_eq!( + fallback, + Some(FallbackReason::DroppedPartialFrame { + byte_count: partial.len(), + }) + ); +} + +#[test] +fn finish_returns_none_when_buffer_empty() { + let mut framer = assembler(); + let frame = permitted_frame("session/new", b"\n"); + + let _ = framer.ingest_chunk(&frame); + assert!(framer.finish().is_none()); +} + +#[test] +fn finish_returns_none_after_raw_fallback() { + let mut framer = assembler(); + let oversize = vec![b'Z'; MAX_RUNTIME_FRAME_BYTES + 1]; + let _ = framer.ingest_chunk(&oversize); + + assert!(framer.finish().is_none()); +} + +#[test] +fn empty_chunk_produces_no_output() { + let mut framer = assembler(); + let (outputs, fallback) = framer.ingest_chunk(b""); + assert!(outputs.is_empty()); + assert!(fallback.is_none()); +} + +/// Build a deterministic byte sequence containing several permitted ACP +/// frames. The sequence is reused across the exhaustive-split parameterized +/// tests below. +fn permitted_stream() -> Vec { + let frames = [ + permitted_frame("session/new", b"\n"), + permitted_frame("session/update", b"\n"), + permitted_frame("session/cancel", b"\n"), + permitted_frame("session/new", b"\r\n"), + permitted_frame("session/update", b"\n"), + ]; + frames.into_iter().fold(Vec::new(), |mut acc, mut frame| { + acc.append(&mut frame); + acc + }) +} + +fn assemble_with_two_chunks(stream: &[u8], split_at: usize) -> Vec { + let mut framer = assembler(); + let first = stream.get(..split_at).unwrap_or_default(); + let second = stream.get(split_at..).unwrap_or_default(); + let mut outputs = Vec::new(); + let (chunk_one, fallback_one) = framer.ingest_chunk(first); + assert!(fallback_one.is_none()); + outputs.extend(chunk_one); + let (chunk_two, fallback_two) = framer.ingest_chunk(second); + assert!(fallback_two.is_none()); + outputs.extend(chunk_two); + assert!(framer.finish().is_none()); + collect_forward_bytes(&outputs) +} + +fn assemble_with_three_chunks(stream: &[u8], first_split: usize, second_split: usize) -> Vec { + assert!(first_split <= second_split); + let mut framer = assembler(); + let first = stream.get(..first_split).unwrap_or_default(); + let second = stream.get(first_split..second_split).unwrap_or_default(); + let third = stream.get(second_split..).unwrap_or_default(); + let mut outputs = Vec::new(); + for chunk in [first, second, third] { + let (chunk_outputs, fallback) = framer.ingest_chunk(chunk); + assert!(fallback.is_none()); + outputs.extend(chunk_outputs); + } + assert!(framer.finish().is_none()); + collect_forward_bytes(&outputs) +} + +#[test] +fn every_two_way_split_reassembles_to_original_byte_stream() { + let stream = permitted_stream(); + for split_at in 1..stream.len() { + let reassembled = assemble_with_two_chunks(&stream, split_at); + assert_eq!( + reassembled, stream, + "split at byte {split_at} should reassemble byte-identically", + ); + } +} + +#[rstest] +#[case(1, 4)] +#[case(8, 32)] +#[case(16, 64)] +#[case(32, 96)] +#[case(40, 80)] +#[case(50, 120)] +#[case(60, 100)] +#[case(70, 140)] +#[case(80, 160)] +#[case(90, 150)] +#[case(95, 145)] +#[case(100, 200)] +#[case(110, 220)] +#[case(120, 180)] +#[case(125, 230)] +#[case(130, 240)] +#[case(135, 235)] +#[case(140, 250)] +#[case(150, 260)] +#[case(155, 265)] +#[case(160, 270)] +#[case(170, 280)] +#[case(180, 290)] +#[case(190, 300)] +#[case(200, 310)] +#[case(210, 320)] +#[case(220, 325)] +#[case(225, 330)] +#[case(230, 335)] +#[case(235, 340)] +#[case(240, 345)] +#[case(250, 350)] +fn three_way_splits_reassemble_to_original_byte_stream( + #[case] first_split: usize, + #[case] second_split: usize, +) { + let stream = permitted_stream(); + let first_clamped = first_split.min(stream.len()); + let second_clamped = second_split.min(stream.len()); + let reassembled = assemble_with_three_chunks(&stream, first_clamped, second_clamped); + assert_eq!( + reassembled, stream, + "three-way split at ({first_clamped}, {second_clamped}) should reassemble identically", + ); +} diff --git a/src/engine/connection/exec/acp_helpers.rs b/src/engine/connection/exec/acp_helpers.rs index aaa95180..878641fe 100644 --- a/src/engine/connection/exec/acp_helpers.rs +++ b/src/engine/connection/exec/acp_helpers.rs @@ -16,7 +16,7 @@ use std::pin::Pin; use ortho_config::serde_json::{self, Value}; use tokio::io::{AsyncBufReadExt, AsyncRead, AsyncWrite, AsyncWriteExt}; -use super::STDIN_BUFFER_CAPACITY; +use super::protocol::STDIN_BUFFER_CAPACITY; /// Upper bound on the number of bytes buffered while searching for the first /// ACP frame. Frames that exceed this limit are forwarded as-is without @@ -61,6 +61,24 @@ pub(super) async fn forward_initial_acp_frame_async( buffered_stdin: &mut tokio::io::BufReader, input: &mut Pin>, ) -> io::Result<()> +where + HostStdin: AsyncRead + Unpin, +{ + let bytes = read_and_mask_initial_acp_frame(buffered_stdin).await?; + input.write_all(&bytes).await +} + +/// Reads the first newline-delimited ACP frame from `buffered_stdin` and +/// returns the bytes that the protocol session should forward to the +/// container, applying capability masking when the frame is a recognized +/// `initialize` request. +/// +/// If the frame exceeds [`MAX_FIRST_FRAME_BYTES`] before a newline is found, +/// or if EOF is reached first, the buffered bytes are returned unchanged so +/// the caller can forward them verbatim. +pub(super) async fn read_and_mask_initial_acp_frame( + buffered_stdin: &mut tokio::io::BufReader, +) -> io::Result> where HostStdin: AsyncRead + Unpin, { @@ -69,15 +87,17 @@ where match next_initial_frame_action(buffered_stdin, &mut first_frame).await? { InitialFrameAction::Continue => {} InitialFrameAction::ForwardUnchanged(reason) => { - return forward_unmodified_initial_frame(input, &first_frame, reason).await; + log_unmodified_forwarding(reason, first_frame.len()); + return Ok(first_frame); } InitialFrameAction::ForwardMasked => { - return forward_masked_initial_frame(input, &first_frame).await; + let masked = mask_acp_initialize_frame(&first_frame); + log_masked_frame_forwarded(&masked, &first_frame); + return Ok(masked); } } } } - /// Read the next chunk into `first_frame` and decide how to proceed. async fn next_initial_frame_action( buffered_stdin: &mut tokio::io::BufReader, @@ -131,29 +151,6 @@ fn log_eof_before_newline(bytes: usize) { "ACP stdin reached EOF before newline; forwarding without rewrite" ); } - -/// Write `first_frame` to `input` unchanged after logging the forwarding -/// reason. -async fn forward_unmodified_initial_frame( - input: &mut Pin>, - first_frame: &[u8], - reason: ForwardUnchangedReason, -) -> io::Result<()> { - log_unmodified_forwarding(reason, first_frame.len()); - input.write_all(first_frame).await -} - -/// Mask `first_frame` and write the result to `input`. -async fn forward_masked_initial_frame( - input: &mut Pin>, - first_frame: &[u8], -) -> io::Result<()> { - let masked_frame = mask_acp_initialize_frame(first_frame); - input.write_all(&masked_frame).await?; - log_masked_frame_forwarded(&masked_frame, first_frame); - Ok(()) -} - /// Emit a debug trace when the first frame was masked before forwarding. fn log_masked_frame_forwarded(masked_frame: &[u8], first_frame: &[u8]) { if masked_frame != first_frame { diff --git a/src/engine/connection/exec/acp_policy.rs b/src/engine/connection/exec/acp_policy.rs new file mode 100644 index 00000000..11ebd8d7 --- /dev/null +++ b/src/engine/connection/exec/acp_policy.rs @@ -0,0 +1,197 @@ +//! Pure policy decisions for Agentic Control Protocol (ACP) runtime +//! enforcement. +//! +//! This module owns the value types and pure functions that decide whether an +//! agent-emitted ACP JavaScript Object Notation Remote Procedure Call +//! (JSON-RPC) frame must be blocked. It deliberately depends on neither +//! `tokio` nor `tracing`; the runtime adapter in `acp_runtime` is responsible +//! for I/O, channel sends, and observability so the policy layer remains +//! trivially testable. +//! +//! The policy is intentionally tolerant: any frame that fails to parse, lacks +//! the JSON-RPC 2.0 marker, lacks a `method` field, or carries an unblocked +//! method passes through unchanged. This mirrors the first-frame masking +//! philosophy established in `acp_helpers` and protects non-Agentic Control +//! Protocol traffic. + +use ortho_config::serde_json::{self, Value}; + +use super::acp_helpers::split_frame_line_ending; + +/// A capability family blocked by Podbot at runtime. +/// +/// Each family is identified by a method-name prefix terminated by the ACP +/// family separator `/`. Matching requires the prefix and at least one +/// non-empty operation name following it, so `terminal/` matches +/// `terminal/create` but not the literal `terminal/` (which carries no +/// operation) or an unrelated method like `terminalize`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct MethodFamily { + /// The capability prefix, including the trailing `/` separator. + pub(crate) prefix: &'static str, +} + +impl MethodFamily { + /// Return `true` when `method` belongs to this family. + pub(crate) fn matches(&self, method: &str) -> bool { + method + .strip_prefix(self.prefix) + .is_some_and(|rest| !rest.is_empty()) + } +} + +/// The default set of ACP capability families that Podbot blocks at runtime. +/// +/// `terminal/` and `fs/` correspond to the same families that the +/// initialization-time masker strips from `clientCapabilities` advertisements, +/// so the runtime denylist closes the symmetric door on agent-emitted method +/// calls in those families. +pub(crate) const DEFAULT_BLOCKED_FAMILIES: &[MethodFamily] = &[ + MethodFamily { + prefix: "terminal/", + }, + MethodFamily { prefix: "fs/" }, +]; + +/// A static set of [`MethodFamily`] values that Podbot refuses to forward. +#[derive(Debug, Clone, Copy)] +pub(crate) struct MethodDenylist { + families: &'static [MethodFamily], +} + +impl MethodDenylist { + /// Construct a denylist over the supplied families. + pub(crate) const fn new(families: &'static [MethodFamily]) -> Self { + Self { families } + } + + /// Construct the default denylist covering `terminal/` and `fs/`. + pub(crate) const fn default_families() -> Self { + Self::new(DEFAULT_BLOCKED_FAMILIES) + } + + /// Return `true` when `method` belongs to any family in the denylist. + pub(crate) fn is_blocked(&self, method: &str) -> bool { + self.families.iter().any(|family| family.matches(method)) + } +} + +/// The decision returned by [`evaluate_agent_outbound_frame`]. +/// +/// `Forward` means the byte-identical frame should be relayed to the host. +/// `BlockNotification` and `BlockRequest` both cause the frame to be dropped; +/// `BlockRequest` additionally carries the original JSON-RPC `id` so the +/// runtime adapter can synthesize a matching error response. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum FrameDecision { + /// Forward the original bytes unchanged. + Forward, + /// Drop a blocked notification (no `id` field present). + BlockNotification { + /// The blocked method name from the JSON-RPC frame. + method: String, + }, + /// Drop a blocked request and prepare a synthesized error response. + BlockRequest { + /// The original JSON-RPC `id`, type-preserved as the source value. + id: Value, + /// The blocked method name from the JSON-RPC frame. + method: String, + }, +} + +/// Decide whether `frame` (an agent-outbound JSON-RPC frame) should be +/// forwarded to the host or blocked by `denylist`. +/// +/// On any parse failure, missing JSON-RPC 2.0 marker, missing-method, or +/// response/batch shape, the function returns [`FrameDecision::Forward`]. +/// Only a JSON-RPC request or notification whose `method` belongs to a +/// blocked family produces a [`FrameDecision::BlockRequest`] or +/// [`FrameDecision::BlockNotification`]. +pub(crate) fn evaluate_agent_outbound_frame( + frame: &[u8], + denylist: &MethodDenylist, +) -> FrameDecision { + let (payload, _line_ending) = split_frame_line_ending(frame); + let Ok(message) = serde_json::from_slice::(payload) else { + return FrameDecision::Forward; + }; + if message.get("jsonrpc").and_then(Value::as_str) != Some("2.0") { + return FrameDecision::Forward; + } + let Some(method) = message.get("method").and_then(Value::as_str) else { + return FrameDecision::Forward; + }; + if !denylist.is_blocked(method) { + return FrameDecision::Forward; + } + let method_owned = String::from(method); + match message.get("id") { + Some(id) => FrameDecision::BlockRequest { + id: id.clone(), + method: method_owned, + }, + None => FrameDecision::BlockNotification { + method: method_owned, + }, + } +} + +/// JSON-RPC application error code used by Podbot for capability-policy +/// denials. +/// +/// The value lives in the JSON-RPC application-defined range +/// `-32099..=-32000` and avoids collision with `-32601 Method not found`, +/// which would imply the method is unknown to the server. Reserve `-32002` +/// for the future "override required" follow-on (Step 2.6.4). +pub(crate) const METHOD_BLOCKED_ERROR_CODE: i64 = -32_001; + +/// Stable error message body for capability-policy denials. The synthesized +/// JSON-RPC response also carries a structured `data.reason` discriminator +/// so agents can branch on `reason` rather than parsing the message string. +pub(crate) const METHOD_BLOCKED_ERROR_MESSAGE: &str = + "Method blocked by Podbot ACP capability policy"; + +/// Stable `data.reason` discriminator embedded in synthesized error +/// responses. +pub(crate) const METHOD_BLOCKED_ERROR_REASON: &str = "podbot_capability_policy"; + +/// Build a JSON-RPC 2.0 error response for a blocked request. +/// +/// The original `id` is preserved as a [`Value`] so numeric, string, and null +/// identifiers retain their JSON type. The supplied `line_ending` bytes are +/// appended verbatim (use `b"\n"` when the original frame carried no +/// recognized line terminator). +/// +/// # Errors +/// +/// Returns the underlying `serde_json` error if serialization of the +/// constructed payload fails. In practice this cannot happen because every +/// component is a finite, non-recursive [`Value`] built from owned primitives, +/// but the caller propagates the error rather than panicking on the +/// theoretical edge case. +pub(crate) fn build_method_blocked_error( + id: &Value, + method: &str, + line_ending: &[u8], +) -> serde_json::Result> { + let payload = serde_json::json!({ + "jsonrpc": "2.0", + "id": id, + "error": { + "code": METHOD_BLOCKED_ERROR_CODE, + "message": METHOD_BLOCKED_ERROR_MESSAGE, + "data": { + "method": method, + "reason": METHOD_BLOCKED_ERROR_REASON, + }, + }, + }); + let mut serialized = serde_json::to_vec(&payload)?; + serialized.extend_from_slice(line_ending); + Ok(serialized) +} + +#[cfg(test)] +#[path = "acp_policy_tests.rs"] +mod tests; diff --git a/src/engine/connection/exec/acp_policy_tests.rs b/src/engine/connection/exec/acp_policy_tests.rs new file mode 100644 index 00000000..64a5ab2b --- /dev/null +++ b/src/engine/connection/exec/acp_policy_tests.rs @@ -0,0 +1,248 @@ +//! Unit tests for the pure Agentic Control Protocol (ACP) policy module. +//! +//! These tests cover the family-prefix matcher, the per-frame decision +//! function, and the synthesized error builder. They use only standard +//! library and `serde_json` types so the policy layer remains trivially +//! testable without `tokio` or `tracing`. + +use ortho_config::serde_json::{self, Value}; +use rstest::rstest; + +use super::{ + DEFAULT_BLOCKED_FAMILIES, FrameDecision, METHOD_BLOCKED_ERROR_CODE, + METHOD_BLOCKED_ERROR_MESSAGE, METHOD_BLOCKED_ERROR_REASON, MethodDenylist, MethodFamily, + build_method_blocked_error, evaluate_agent_outbound_frame, +}; + +const TERMINAL_FAMILY: MethodFamily = MethodFamily { + prefix: "terminal/", +}; + +#[rstest] +#[case::exact_prefix_with_operation("terminal/create", true)] +#[case::nested_path("terminal/output/follow", true)] +#[case::bare_prefix_no_operation("terminal/", false)] +#[case::similar_word_no_slash("terminalize", false)] +#[case::word_without_separator("terminal", false)] +#[case::unrelated("session/new", false)] +fn method_family_matches_with_slash_boundary(#[case] method: &str, #[case] expected: bool) { + assert_eq!(TERMINAL_FAMILY.matches(method), expected); +} + +#[rstest] +#[case("terminal/create", true)] +#[case("fs/read_text_file", true)] +#[case("fs/write_text_file", true)] +#[case("session/new", false)] +#[case("initialize", false)] +fn default_denylist_blocks_terminal_and_fs_only(#[case] method: &str, #[case] expected: bool) { + let denylist = MethodDenylist::default_families(); + assert_eq!(denylist.is_blocked(method), expected); +} + +/// Serializes `value` to compact JSON bytes and appends a newline terminator, +/// producing a well-formed ACP frame suitable for test input. +fn serialize_frame(value: &serde_json::Value) -> Vec { + let mut bytes = serde_json::to_vec(value).expect("frame serializes"); + bytes.push(b'\n'); + bytes +} + +fn jsonrpc_request(id: &Value, method: &str) -> Vec { + serialize_frame(&serde_json::json!({ + "jsonrpc": "2.0", + "id": id, + "method": method, + "params": {}, + })) +} + +fn jsonrpc_notification(method: &str) -> Vec { + serialize_frame(&serde_json::json!({ + "jsonrpc": "2.0", + "method": method, + "params": {}, + })) +} + +#[rstest] +#[case::numeric_id(serde_json::json!(7))] +#[case::string_id(serde_json::json!("call-7"))] +#[case::null_id(serde_json::json!(null))] +fn evaluate_returns_block_request_with_preserved_id(#[case] id: Value) { + let frame = jsonrpc_request(&id, "terminal/create"); + let denylist = MethodDenylist::default_families(); + + let decision = evaluate_agent_outbound_frame(&frame, &denylist); + + match decision { + FrameDecision::BlockRequest { + id: actual_id, + method, + } => { + assert_eq!(actual_id, id, "request id should be preserved verbatim"); + assert_eq!(method, "terminal/create"); + } + other => panic!("expected BlockRequest, got {other:?}"), + } +} + +#[test] +fn evaluate_returns_block_notification_for_blocked_method_without_id() { + let frame = jsonrpc_notification("fs/changed"); + let denylist = MethodDenylist::default_families(); + + let decision = evaluate_agent_outbound_frame(&frame, &denylist); + + assert_eq!( + decision, + FrameDecision::BlockNotification { + method: String::from("fs/changed"), + } + ); +} + +#[test] +fn evaluate_forwards_permitted_request() { + let frame = jsonrpc_request(&serde_json::json!(1), "session/new"); + let denylist = MethodDenylist::default_families(); + + assert_eq!( + evaluate_agent_outbound_frame(&frame, &denylist), + FrameDecision::Forward + ); +} + +#[test] +fn evaluate_forwards_malformed_json() { + let frame: &[u8] = br#"{"jsonrpc":"2.0","method":"terminal/create" "#; + let denylist = MethodDenylist::default_families(); + + assert_eq!( + evaluate_agent_outbound_frame(frame, &denylist), + FrameDecision::Forward, + "malformed JSON should pass through unchanged" + ); +} + +#[rstest] +#[case::response_without_method(serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "result": {"value": 42}, +}))] +#[case::method_not_string(serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "method": 7, +}))] +fn evaluate_forwards_frames_without_dispatchable_method(#[case] payload: serde_json::Value) { + let frame = serialize_frame(&payload); + let denylist = MethodDenylist::default_families(); + + assert_eq!( + evaluate_agent_outbound_frame(&frame, &denylist), + FrameDecision::Forward + ); +} + +#[rstest] +#[case::missing_jsonrpc(serde_json::json!({ + "id": 1, + "method": "terminal/create", +}))] +#[case::wrong_jsonrpc_version(serde_json::json!({ + "jsonrpc": "1.0", + "id": 1, + "method": "terminal/create", +}))] +#[case::jsonrpc_not_string(serde_json::json!({ + "jsonrpc": 2.0, + "id": 1, + "method": "terminal/create", +}))] +fn evaluate_forwards_non_jsonrpc_objects_with_blocked_method(#[case] payload: serde_json::Value) { + let frame = serialize_frame(&payload); + let denylist = MethodDenylist::default_families(); + + assert_eq!( + evaluate_agent_outbound_frame(&frame, &denylist), + FrameDecision::Forward + ); +} + +#[test] +fn evaluate_handles_crlf_terminated_frame() { + let mut frame = serde_json::to_vec(&serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "terminal/create", + })) + .expect("request serializes"); + frame.extend_from_slice(b"\r\n"); + let denylist = MethodDenylist::default_families(); + + let decision = evaluate_agent_outbound_frame(&frame, &denylist); + + assert!(matches!(decision, FrameDecision::BlockRequest { .. })); +} + +#[rstest] +#[case::numeric(serde_json::json!(42), b"\n" as &[u8])] +#[case::string(serde_json::json!("call-42"), b"\n" as &[u8])] +#[case::null(serde_json::json!(null), b"\n" as &[u8])] +#[case::crlf(serde_json::json!(1), b"\r\n" as &[u8])] +fn build_error_payload_round_trips_with_expected_fields( + #[case] id: Value, + #[case] line_ending: &[u8], +) { + let bytes = build_method_blocked_error(&id, "terminal/create", line_ending) + .expect("error payload should serialize"); + + assert!( + bytes.ends_with(line_ending), + "supplied line ending should be preserved" + ); + + let payload_end = bytes + .len() + .checked_sub(line_ending.len()) + .expect("payload must precede the line ending"); + let payload_slice = bytes + .get(..payload_end) + .expect("payload prefix must exist before the line ending"); + let parsed: Value = serde_json::from_slice(payload_slice) + .expect("synthesized error must round-trip through serde_json"); + assert_eq!(parsed.get("jsonrpc"), Some(&Value::from("2.0"))); + assert_eq!(parsed.get("id"), Some(&id)); + let error = parsed + .get("error") + .and_then(Value::as_object) + .expect("error object must be present"); + assert_eq!( + error.get("code"), + Some(&Value::from(METHOD_BLOCKED_ERROR_CODE)), + ); + assert_eq!( + error.get("message"), + Some(&Value::from(METHOD_BLOCKED_ERROR_MESSAGE)), + ); + let data = error + .get("data") + .and_then(Value::as_object) + .expect("error.data object must be present"); + assert_eq!(data.get("method"), Some(&Value::from("terminal/create"))); + assert_eq!( + data.get("reason"), + Some(&Value::from(METHOD_BLOCKED_ERROR_REASON)), + ); +} + +#[test] +fn default_blocked_families_match_design_decision() { + let prefixes: Vec<&str> = DEFAULT_BLOCKED_FAMILIES + .iter() + .map(|family| family.prefix) + .collect(); + assert_eq!(prefixes, vec!["terminal/", "fs/"]); +} diff --git a/src/engine/connection/exec/acp_runtime.rs b/src/engine/connection/exec/acp_runtime.rs new file mode 100644 index 00000000..e21057e6 --- /dev/null +++ b/src/engine/connection/exec/acp_runtime.rs @@ -0,0 +1,309 @@ +//! Runtime adapter and container-stdin sink for Agentic Control Protocol +//! (ACP) capability enforcement. +//! +//! The pure policy in `acp_policy` and the streaming framer in `acp_frame` +//! decide what each agent-emitted JSON-RPC frame should become. This module +//! turns those decisions into actual I/O: it writes permitted frames to host +//! stdout verbatim, synthesizes JSON-RPC error responses for blocked +//! requests, and emits one stderr `tracing::warn!` per denial or +//! once-per-session fallback record. +//! +//! ## Sink task model +//! +//! Container stdin has a single owner: a dedicated [`run_container_stdin_sink`] +//! task that drains a bounded [`tokio::sync::mpsc::Receiver`] of [`WriteCmd`]. +//! Both the host-stdin forwarder and the [`OutboundPolicyAdapter`] become +//! senders. This eliminates the [`super::STDIN_SETTLE_TIMEOUT`] race that a +//! shared writer would have created and lets the adapter guarantee that +//! synthesized error responses reach the agent before the input stream +//! closes. +//! +//! Ordering invariant: the protocol coordinator drops every sender (the +//! [`OutboundPolicyAdapter`] and the host-stdin forwarder) only after the +//! output stream has fully drained and after all blocked-request decisions +//! have been queued. Because the channel preserves send order and the sink +//! processes commands sequentially before `recv()` returns `None` for the +//! closed channel, every [`WriteCmd::Synthesized`] queued during the output +//! loop is delivered before container stdin closes. +//! +//! ## Failure tolerance +//! +//! - When container stdin returns `BrokenPipe` (the agent has already +//! exited), the sink downgrades the failure to a single `warn!` and +//! continues to drain the channel until every sender has been dropped. The +//! exit-code path remains intact. +//! - When [`super::acp_policy::build_method_blocked_error`] fails (a +//! theoretical edge case for a finite owned [`Value`]), the adapter logs +//! a `warn!` and continues without sending a synthesized response, +//! leaving the agent to time out rather than panicking the proxy. + +use std::io; +use std::pin::Pin; + +use ortho_config::serde_json::{self, Value}; +use tokio::io::{AsyncWrite, AsyncWriteExt}; +use tokio::sync::mpsc; + +use super::acp_frame::{DeniedFrameDecision, FallbackReason, FrameOutput, OutboundFrameAssembler}; +use super::acp_policy::build_method_blocked_error; + +/// Bounded capacity for the container-stdin command channel. +/// +/// Sized small so an agent flooding blocked methods cannot exhaust memory +/// before the sink drains them, while still permitting a few synthesized +/// responses to be queued ahead of the host-stdin forwarder during a +/// burst. +pub(super) const SINK_CHANNEL_CAPACITY: usize = 16; + +/// One write destined for container stdin. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) enum WriteCmd { + /// Bytes forwarded from host stdin (the operator's keystrokes or the + /// hosting client's protocol output). + Forward(Vec), + /// A JSON-RPC error response synthesized by the policy adapter in + /// response to a blocked request. + Synthesized(Vec), +} + +/// Drain the supplied channel, writing each command to `input` and flushing +/// after every successful write. +/// +/// The function tolerates `BrokenPipe` from container stdin by logging a +/// single `warn!` and continuing to drain the channel until every sender +/// has been dropped. Once the channel is closed, the sink stops draining, +/// preserving the existing exit-code reporting path. +pub(super) async fn run_container_stdin_sink( + mut input: Pin>, + mut commands: mpsc::Receiver, +) -> io::Result<()> { + let mut input_alive = true; + + while let Some(command) = commands.recv().await { + let bytes = command_bytes(command); + if input_alive { + input_alive = write_command_bytes(&mut input, &bytes).await?; + } + } + + finalize_sink_writer(&mut input, input_alive).await; + Ok(()) +} + +fn command_bytes(command: WriteCmd) -> Vec { + match command { + WriteCmd::Forward(bytes) | WriteCmd::Synthesized(bytes) => bytes, + } +} + +async fn finalize_sink_writer(input: &mut Pin>, input_alive: bool) { + if !input_alive { + return; + } + let outcome = input.shutdown().await; + log_shutdown_outcome(outcome); +} + +fn log_shutdown_outcome(outcome: io::Result<()>) { + if let Err(error) = outcome { + tracing::warn!(%error, "container stdin shutdown failed"); + } +} + +async fn write_command_bytes( + input: &mut Pin>, + bytes: &[u8], +) -> io::Result { + if !classify_pipe_outcome(input.write_all(bytes).await)? { + return Ok(false); + } + classify_pipe_outcome(input.flush().await) +} + +fn classify_pipe_outcome(result: io::Result<()>) -> io::Result { + match result { + Ok(()) => Ok(true), + Err(error) if error.kind() == io::ErrorKind::BrokenPipe => { + report_broken_pipe(&error); + Ok(false) + } + Err(error) => Err(error), + } +} + +fn report_broken_pipe(error: &io::Error) { + tracing::warn!(%error, "container stdin closed; subsequent writes dropped"); +} + +/// Adapter that turns assembler outputs into host-stdout writes and +/// container-stdin commands. +pub(super) struct OutboundPolicyAdapter { + assembler: OutboundFrameAssembler, + sender: mpsc::Sender, + container_id: String, + fallback_logged: bool, +} + +impl OutboundPolicyAdapter { + /// Construct an adapter over the supplied assembler and sink sender. + pub(super) fn new( + assembler: OutboundFrameAssembler, + sender: mpsc::Sender, + container_id: impl Into, + ) -> Self { + Self { + assembler, + sender, + container_id: container_id.into(), + fallback_logged: false, + } + } + + /// Process one Bollard output chunk, writing permitted bytes to + /// `host_stdout` and queuing synthesized error responses on the sink + /// channel. + pub(super) async fn handle_chunk( + &mut self, + chunk: &[u8], + host_stdout: &mut W, + ) -> io::Result<()> + where + W: AsyncWrite + Unpin, + { + let (outputs, fallback) = self.assembler.ingest_chunk(chunk); + for output in outputs { + self.dispatch_output(output, host_stdout).await?; + } + if let Some(reason) = fallback { + self.log_fallback_once(reason); + } + Ok(()) + } + + /// Finalize the adapter at end of stream, logging any partial-frame + /// drop reported by the assembler. + pub(super) fn finish(&mut self) { + if let Some(reason) = self.assembler.finish() { + self.log_fallback_once(reason); + } + } + + async fn dispatch_output( + &mut self, + output: FrameOutput, + host_stdout: &mut W, + ) -> io::Result<()> + where + W: AsyncWrite + Unpin, + { + match output { + FrameOutput::Forward(bytes) => { + host_stdout.write_all(&bytes).await?; + host_stdout.flush().await + } + FrameOutput::Decision(decision, line_ending) => { + self.handle_decision(decision, &line_ending).await; + Ok(()) + } + } + } + + async fn handle_decision(&self, decision: DeniedFrameDecision, line_ending: &[u8]) { + match decision { + DeniedFrameDecision::BlockNotification { method } => { + self.log_denial(&method, &Value::Null, "ACP blocked notification dropped"); + } + DeniedFrameDecision::BlockRequest { id, method } => { + self.log_denial(&method, &id, "ACP blocked request denied"); + self.queue_synthesized_error(&id, &method, line_ending) + .await; + } + } + } + + async fn queue_synthesized_error(&self, id: &Value, method: &str, line_ending: &[u8]) { + match build_method_blocked_error(id, method, line_ending) { + Ok(bytes) => self.send_synthesized_or_log(bytes, method).await, + Err(error) => self.log_synthesis_failure(method, &error), + } + } + + async fn send_synthesized_or_log(&self, bytes: Vec, method: &str) { + let outcome = self.sender.send(WriteCmd::Synthesized(bytes)).await; + if let Err(error) = outcome { + self.log_send_failure(method, &error); + } + } + + fn log_send_failure(&self, method: &str, error: &mpsc::error::SendError) { + tracing::warn!( + target = "podbot::acp::policy", + container_id = %self.container_id, + method = %method, + ?error, + "ACP denial response could not be queued; sink already closed", + ); + } + + fn log_synthesis_failure(&self, method: &str, error: &serde_json::Error) { + tracing::warn!( + target = "podbot::acp::policy", + container_id = %self.container_id, + method = %method, + %error, + "ACP denial response failed to serialize; agent will time out", + ); + } + + fn log_denial(&self, method: &str, id: &Value, message: &'static str) { + tracing::warn!( + target = "podbot::acp::policy", + container_id = %self.container_id, + method = %method, + id = %id, + "{message}", + ); + } + + fn log_fallback_once(&mut self, reason: FallbackReason) { + if self.fallback_logged { + return; + } + self.fallback_logged = true; + self.emit_fallback_warning(reason); + } + + fn emit_fallback_warning(&self, reason: FallbackReason) { + match reason { + FallbackReason::BufferOverflow => self.warn_buffer_overflow(), + FallbackReason::DroppedPartialFrame { byte_count } => { + self.warn_partial_frame_drop(byte_count); + } + } + } + + fn warn_buffer_overflow(&self) { + tracing::warn!( + target = "podbot::acp::policy", + container_id = %self.container_id, + "ACP runtime buffer overflowed; remaining bytes forwarded raw", + ); + } + + fn warn_partial_frame_drop(&self, byte_count: usize) { + tracing::warn!( + target = "podbot::acp::policy", + container_id = %self.container_id, + byte_count, + "ACP runtime dropped unauthorized partial frame at end of stream", + ); + } +} + +#[cfg(test)] +#[path = "acp_runtime_tests.rs"] +mod tests; + +#[cfg(test)] +#[path = "acp_runtime_bdd_tests.rs"] +mod bdd_tests; diff --git a/src/engine/connection/exec/acp_runtime_bdd_tests.rs b/src/engine/connection/exec/acp_runtime_bdd_tests.rs new file mode 100644 index 00000000..a65760d7 --- /dev/null +++ b/src/engine/connection/exec/acp_runtime_bdd_tests.rs @@ -0,0 +1,348 @@ +//! BDD scenario wiring for the Agentic Control Protocol (ACP) runtime +//! method denylist feature. + +use std::io; +use std::pin::Pin; +use std::sync::{Arc, Mutex}; +use std::task::{Context, Poll}; + +use ortho_config::serde_json::{self, Value}; +use rstest_bdd::Slot; +use rstest_bdd_macros::{ScenarioState, given, scenario, then, when}; +use tokio::io::AsyncWrite; +use tokio::sync::mpsc; + +use super::{OutboundFrameAssembler, OutboundPolicyAdapter, SINK_CHANNEL_CAPACITY, WriteCmd}; +use crate::engine::connection::exec::acp_policy::MethodDenylist; + +type StepResult = Result; + +#[derive(Default, Clone)] +struct RecordingHostStdout { + bytes: Arc>>, +} + +impl RecordingHostStdout { + fn snapshot(&self) -> Result, String> { + self.bytes + .lock() + .map(|guard| guard.clone()) + .map_err(|err| format!("recording writer mutex poisoned: {err}")) + } +} + +impl AsyncWrite for RecordingHostStdout { + fn poll_write( + self: Pin<&mut Self>, + _cx: &mut Context<'_>, + buf: &[u8], + ) -> Poll> { + self.bytes.lock().map_or_else( + |_| Poll::Ready(Err(io::Error::other("recording writer mutex poisoned"))), + |mut guard| { + guard.extend_from_slice(buf); + Poll::Ready(Ok(buf.len())) + }, + ) + } + + fn poll_flush(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + + fn poll_shutdown(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } +} + +#[derive(Default, ScenarioState)] +struct DenylistState { + host_stdout_bytes: Slot>, + sink_commands: Slot>, +} + +#[rstest::fixture] +fn denylist_state() -> DenylistState { + DenylistState::default() +} + +/// Serialises `value` to compact JSON bytes and appends a newline terminator, +/// producing a well-formed ACP frame suitable for test input. +fn serialize_frame(value: serde_json::Value) -> Vec { + let mut bytes = serde_json::to_vec(&value).expect("frame serializes"); + drop(value); + bytes.push(b'\n'); + bytes +} + +fn make_request_frame(method: &str, id: i64) -> Vec { + serialize_frame(serde_json::json!({ + "jsonrpc": "2.0", + "id": id, + "method": method, + "params": {}, + })) +} + +fn make_notification_frame(method: &str) -> Vec { + serialize_frame(serde_json::json!({ + "jsonrpc": "2.0", + "method": method, + "params": {}, + })) +} + +/// Reads the host-stdout snapshot from `denylist_state`, returning an error +/// if the slot has not yet been populated. +fn read_host_stdout(denylist_state: &DenylistState) -> StepResult> { + denylist_state + .host_stdout_bytes + .get() + .ok_or_else(|| String::from("host stdout snapshot not recorded")) +} + +fn assert_host_stdout_matches(denylist_state: &DenylistState, expected: &[u8]) -> StepResult<()> { + let bytes = read_host_stdout(denylist_state)?; + if bytes == expected { + Ok(()) + } else { + Err(String::from( + "host stdout did not receive the expected frame verbatim", + )) + } +} + +fn build_runtime() -> ( + OutboundPolicyAdapter, + mpsc::Receiver, + RecordingHostStdout, +) { + let (sender, receiver) = mpsc::channel::(SINK_CHANNEL_CAPACITY); + let assembler = OutboundFrameAssembler::new(MethodDenylist::default_families()); + let adapter = OutboundPolicyAdapter::new(assembler, sender, "container-bdd"); + (adapter, receiver, RecordingHostStdout::default()) +} + +async fn drain_sink(mut receiver: mpsc::Receiver) -> Vec { + receiver.close(); + let mut commands = Vec::new(); + while let Some(cmd) = receiver.recv().await { + commands.push(cmd); + } + commands +} + +fn run_runtime(state: &DenylistState, chunks: Vec>, finalize: F) -> StepResult<()> +where + F: FnOnce(), +{ + finalize(); + let runtime = + tokio::runtime::Runtime::new().map_err(|err| format!("could not build runtime: {err}"))?; + let (commands, host_bytes) = runtime.block_on(async move { + let (mut adapter, receiver, host_stdout) = build_runtime(); + let host_stdout_handle = host_stdout.clone(); + let mut writer = host_stdout; + for chunk in chunks { + adapter + .handle_chunk(&chunk, &mut writer) + .await + .expect("chunk handles cleanly"); + } + adapter.finish(); + drop(adapter); + let commands = drain_sink(receiver).await; + let bytes = host_stdout_handle + .snapshot() + .expect("host stdout snapshot should succeed"); + (commands, bytes) + }); + state.host_stdout_bytes.set(host_bytes); + state.sink_commands.set(commands); + Ok(()) +} + +#[given("the ACP runtime adapter is configured with the default denylist")] +fn adapter_uses_default_denylist(denylist_state: &DenylistState) { + let _ = denylist_state; +} + +#[when(r#"the agent emits a "terminal/create" request with id 7"#)] +fn emit_blocked_terminal_create(denylist_state: &DenylistState) -> StepResult<()> { + let frame = make_request_frame("terminal/create", 7); + run_runtime(denylist_state, vec![frame], || {}) +} + +#[when(r#"the agent emits a "session/new" request with id 1"#)] +fn emit_permitted_session_new(denylist_state: &DenylistState) -> StepResult<()> { + let frame = make_request_frame("session/new", 1); + run_runtime(denylist_state, vec![frame], || {}) +} + +#[when(r#"the agent emits an "fs/changed" notification"#)] +fn emit_blocked_notification(denylist_state: &DenylistState) -> StepResult<()> { + let frame = make_notification_frame("fs/changed"); + run_runtime(denylist_state, vec![frame], || {}) +} + +#[when("the agent emits a blocked frame split across two output chunks")] +fn emit_blocked_split(denylist_state: &DenylistState) -> StepResult<()> { + let frame = make_request_frame("terminal/create", 2); + let split_at = frame.len().div_euclid(2); + let first = frame + .get(..split_at) + .ok_or_else(|| String::from("split prefix missing"))? + .to_vec(); + let second = frame + .get(split_at..) + .ok_or_else(|| String::from("split suffix missing"))? + .to_vec(); + run_runtime(denylist_state, vec![first, second], || {}) +} + +#[when("the agent emits a blocked request followed by a permitted request")] +fn emit_blocked_then_permitted(denylist_state: &DenylistState) -> StepResult<()> { + let mut chunk = make_request_frame("terminal/create", 5); + let permitted = make_request_frame("session/update", 6); + chunk.extend_from_slice(&permitted); + run_runtime(denylist_state, vec![chunk], || {}) +} + +#[then("host stdout receives no bytes from the blocked request")] +fn assert_host_stdout_empty(denylist_state: &DenylistState) -> StepResult<()> { + let bytes = read_host_stdout(denylist_state)?; + if bytes.is_empty() { + Ok(()) + } else { + Err(format!( + "expected host stdout to be empty, got {len} bytes", + len = bytes.len(), + )) + } +} + +#[then("host stdout receives no bytes from the blocked notification")] +fn assert_host_stdout_empty_for_notification(denylist_state: &DenylistState) -> StepResult<()> { + assert_host_stdout_empty(denylist_state) +} + +#[then("container stdin receives a synthesized JSON-RPC error with id 7")] +fn assert_synthesized_id_seven(denylist_state: &DenylistState) -> StepResult<()> { + expect_synthesized_id(denylist_state, &serde_json::json!(7)) +} + +#[then("container stdin receives a synthesized JSON-RPC error with id 2")] +fn assert_synthesized_id_two(denylist_state: &DenylistState) -> StepResult<()> { + expect_synthesized_id(denylist_state, &serde_json::json!(2)) +} + +#[then("container stdin receives a synthesized JSON-RPC error for the blocked request")] +fn assert_synthesized_for_blocked(denylist_state: &DenylistState) -> StepResult<()> { + expect_synthesized_id(denylist_state, &serde_json::json!(5)) +} + +fn expect_synthesized_id(denylist_state: &DenylistState, expected_id: &Value) -> StepResult<()> { + let commands = denylist_state + .sink_commands + .get() + .ok_or_else(|| String::from("sink command snapshot not recorded"))?; + let synthesized: Vec<&Vec> = commands + .iter() + .filter_map(|cmd| match cmd { + WriteCmd::Synthesized(bytes) => Some(bytes), + WriteCmd::Forward(_) => None, + }) + .collect(); + let bytes = match synthesized.as_slice() { + [bytes] => *bytes, + other => { + return Err(format!( + "expected exactly one synthesized response, got {count}", + count = other.len(), + )); + } + }; + let payload = bytes + .strip_suffix(b"\n") + .ok_or_else(|| String::from("synthesized response should end in newline"))?; + let parsed: Value = serde_json::from_slice(payload) + .map_err(|err| format!("synthesized response did not parse: {err}"))?; + let id = parsed + .get("id") + .ok_or_else(|| String::from("synthesized response missing id"))?; + if id == expected_id { + Ok(()) + } else { + Err(format!("expected id {expected_id}, got {id}")) + } +} + +#[then("host stdout receives the permitted frame verbatim")] +fn assert_host_stdout_matches_permitted(denylist_state: &DenylistState) -> StepResult<()> { + assert_host_stdout_matches(denylist_state, &make_request_frame("session/new", 1)) +} + +#[then("host stdout receives only the permitted frame verbatim")] +fn assert_host_stdout_matches_permitted_after_blocked( + denylist_state: &DenylistState, +) -> StepResult<()> { + assert_host_stdout_matches(denylist_state, &make_request_frame("session/update", 6)) +} + +#[then("container stdin receives no synthesized response")] +fn assert_no_synthesized(denylist_state: &DenylistState) -> StepResult<()> { + let commands = denylist_state + .sink_commands + .get() + .ok_or_else(|| String::from("sink command snapshot not recorded"))?; + let any_synthesized = commands + .iter() + .any(|cmd| matches!(cmd, WriteCmd::Synthesized(_))); + if any_synthesized { + Err(String::from( + "expected no synthesized response on container stdin", + )) + } else { + Ok(()) + } +} + +#[scenario( + path = "tests/features/acp_method_denylist.feature", + name = "Blocked request returns a synthesized error and is not forwarded" +)] +fn blocked_request_returns_synthesized_error(denylist_state: DenylistState) { + let _ = denylist_state; +} + +#[scenario( + path = "tests/features/acp_method_denylist.feature", + name = "Permitted method passes through unchanged byte-for-byte" +)] +fn permitted_method_passes_through(denylist_state: DenylistState) { + 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; +} diff --git a/src/engine/connection/exec/acp_runtime_tests.rs b/src/engine/connection/exec/acp_runtime_tests.rs new file mode 100644 index 00000000..07945a94 --- /dev/null +++ b/src/engine/connection/exec/acp_runtime_tests.rs @@ -0,0 +1,382 @@ +//! Unit tests for the Agentic Control Protocol (ACP) runtime adapter and +//! container-stdin sink task. + +use std::io; +use std::pin::Pin; +use std::sync::{Arc, Mutex}; +use std::task::{Context, Poll}; + +use ortho_config::serde_json::{self, Value}; +use rstest::rstest; +use tokio::io::AsyncWrite; +use tokio::sync::mpsc; + +use super::{ + OutboundFrameAssembler, OutboundPolicyAdapter, SINK_CHANNEL_CAPACITY, WriteCmd, + run_container_stdin_sink, +}; +use crate::engine::connection::exec::acp_policy::MethodDenylist; + +/// Recording host-stdout writer that captures every byte and tracks shutdown. +#[derive(Clone, Default)] +struct RecordingWriter { + bytes: Arc>>, + shutdown_called: Arc>, +} + +impl RecordingWriter { + fn snapshot(&self) -> Vec { + self.bytes.lock().expect("writer mutex").clone() + } + + fn shutdown_observed(&self) -> bool { + *self.shutdown_called.lock().expect("shutdown mutex") + } +} + +impl AsyncWrite for RecordingWriter { + fn poll_write( + self: Pin<&mut Self>, + _cx: &mut Context<'_>, + buf: &[u8], + ) -> Poll> { + self.bytes + .lock() + .expect("writer mutex") + .extend_from_slice(buf); + Poll::Ready(Ok(buf.len())) + } + + fn poll_flush(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + + fn poll_shutdown(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll> { + *self.shutdown_called.lock().expect("shutdown mutex") = true; + Poll::Ready(Ok(())) + } +} + +/// Writer that always returns `BrokenPipe` on writes, used to simulate the +/// agent having already exited. +struct BrokenPipeWriter; + +impl AsyncWrite for BrokenPipeWriter { + fn poll_write( + self: Pin<&mut Self>, + _cx: &mut Context<'_>, + _buf: &[u8], + ) -> Poll> { + Poll::Ready(Err(io::Error::new( + io::ErrorKind::BrokenPipe, + "agent exited", + ))) + } + + fn poll_flush(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + + fn poll_shutdown(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } +} + +/// Builds a newline-terminated JSON-RPC 2.0 frame. +/// +/// Pass `id = Some(…)` for requests; `id = None` for notifications. +fn make_jsonrpc_frame(method: &str, id: Option<&Value>) -> Vec { + let value = id.map_or_else( + || { + serde_json::json!({ + "jsonrpc": "2.0", + "method": method, + "params": {}, + }) + }, + |request_id| { + serde_json::json!({ + "jsonrpc": "2.0", + "id": request_id, + "method": method, + "params": {}, + }) + }, + ); + let mut bytes = serde_json::to_vec(&value).expect("frame serializes"); + bytes.push(b'\n'); + bytes +} + +fn permitted_frame() -> Vec { + make_jsonrpc_frame("session/new", Some(&serde_json::json!(1))) +} + +fn blocked_request_frame(id: &Value) -> Vec { + make_jsonrpc_frame("terminal/create", Some(id)) +} + +fn blocked_notification_frame() -> Vec { + make_jsonrpc_frame("fs/changed", None) +} + +fn build_adapter() -> (OutboundPolicyAdapter, mpsc::Receiver) { + let (tx, rx) = mpsc::channel::(SINK_CHANNEL_CAPACITY); + let assembler = OutboundFrameAssembler::new(MethodDenylist::default_families()); + let adapter = OutboundPolicyAdapter::new(assembler, tx, "container-test"); + (adapter, rx) +} + +async fn drain_channel(mut rx: mpsc::Receiver) -> Vec { + let mut received = Vec::new(); + rx.close(); + while let Some(cmd) = rx.recv().await { + received.push(cmd); + } + received +} + +#[tokio::test] +async fn permitted_frame_writes_to_host_stdout_only() { + let (mut adapter, rx) = build_adapter(); + let host_stdout = RecordingWriter::default(); + let recorder = host_stdout.clone(); + let mut writer: Pin> = Box::pin(host_stdout); + let frame = permitted_frame(); + + adapter + .handle_chunk(&frame, &mut writer) + .await + .expect("permitted frame writes succeed"); + adapter.finish(); + drop(adapter); + + assert_eq!(recorder.snapshot(), frame); + let received = drain_channel(rx).await; + assert!(received.is_empty(), "no commands should reach the sink"); +} + +#[tokio::test] +async fn blocked_request_skips_host_stdout_and_queues_synthesized_response() { + let (mut adapter, rx) = build_adapter(); + let host_stdout = RecordingWriter::default(); + let recorder = host_stdout.clone(); + let mut writer: Pin> = Box::pin(host_stdout); + let frame = blocked_request_frame(&serde_json::json!(7)); + + adapter + .handle_chunk(&frame, &mut writer) + .await + .expect("blocked request handles cleanly"); + drop(adapter); + + assert!( + recorder.snapshot().is_empty(), + "blocked request must never reach host stdout", + ); + let received = drain_channel(rx).await; + let bytes = match received.as_slice() { + [WriteCmd::Synthesized(bytes)] => bytes.clone(), + other => panic!("expected one Synthesized, got {other:?}"), + }; + let payload_end = bytes.len().checked_sub(1).expect("trailing newline"); + let payload_slice = bytes.get(..payload_end).expect("payload before newline"); + let parsed: Value = + serde_json::from_slice(payload_slice).expect("synthesized response is valid JSON"); + assert_eq!(parsed.get("id"), Some(&serde_json::json!(7))); + assert_eq!( + parsed + .get("error") + .and_then(|err| err.get("data")) + .and_then(|data| data.get("method")), + Some(&serde_json::json!("terminal/create")), + ); +} + +#[tokio::test] +async fn blocked_notification_drops_silently_without_sink_command() { + let (mut adapter, rx) = build_adapter(); + let host_stdout = RecordingWriter::default(); + let recorder = host_stdout.clone(); + let mut writer: Pin> = Box::pin(host_stdout); + let frame = blocked_notification_frame(); + + adapter + .handle_chunk(&frame, &mut writer) + .await + .expect("blocked notification handles cleanly"); + drop(adapter); + + assert!(recorder.snapshot().is_empty()); + let received = drain_channel(rx).await; + assert!( + received.is_empty(), + "notifications must not generate a response" + ); +} + +#[tokio::test] +async fn permitted_frame_after_blocked_frame_still_reaches_host_stdout() { + let (mut adapter, rx) = build_adapter(); + let host_stdout = RecordingWriter::default(); + let recorder = host_stdout.clone(); + let mut writer: Pin> = Box::pin(host_stdout); + let mut chunk = blocked_request_frame(&serde_json::json!(1)); + let permitted = permitted_frame(); + chunk.extend_from_slice(&permitted); + + adapter + .handle_chunk(&chunk, &mut writer) + .await + .expect("mixed chunk handles cleanly"); + drop(adapter); + + assert_eq!(recorder.snapshot(), permitted); + let received = drain_channel(rx).await; + assert!(matches!(received.as_slice(), [WriteCmd::Synthesized(_)])); +} + +#[tokio::test] +async fn frame_split_across_chunks_is_classified_after_assembly() { + let (mut adapter, rx) = build_adapter(); + let host_stdout = RecordingWriter::default(); + let recorder = host_stdout.clone(); + let mut writer: Pin> = Box::pin(host_stdout); + let frame = blocked_request_frame(&serde_json::json!(2)); + let split_at = frame.len().div_euclid(2); + let first = frame.get(..split_at).expect("split prefix"); + let second = frame.get(split_at..).expect("split suffix"); + + adapter + .handle_chunk(first, &mut writer) + .await + .expect("first chunk"); + adapter + .handle_chunk(second, &mut writer) + .await + .expect("second chunk"); + drop(adapter); + + assert!(recorder.snapshot().is_empty()); + let received = drain_channel(rx).await; + assert!(matches!(received.as_slice(), [WriteCmd::Synthesized(_)])); +} + +#[tokio::test] +async fn sink_writes_forwards_then_synthesized_in_send_order() { + let recorder = RecordingWriter::default(); + let writer_handle = recorder.clone(); + let writer: Pin> = Box::pin(recorder); + let (tx, rx) = mpsc::channel::(SINK_CHANNEL_CAPACITY); + let sink = tokio::spawn(run_container_stdin_sink(writer, rx)); + + tx.send(WriteCmd::Forward(b"forward-one\n".to_vec())) + .await + .expect("forward send"); + tx.send(WriteCmd::Synthesized(b"synthesized-one\n".to_vec())) + .await + .expect("synthesized send"); + tx.send(WriteCmd::Forward(b"forward-two\n".to_vec())) + .await + .expect("second forward send"); + drop(tx); + + sink.await + .expect("sink task joins") + .expect("sink runs cleanly"); + + let bytes = writer_handle.snapshot(); + assert_eq!(bytes, b"forward-one\nsynthesized-one\nforward-two\n"); + assert!(writer_handle.shutdown_observed()); +} + +#[tokio::test] +async fn sink_continues_after_broken_pipe_until_channel_closes() { + let writer: Pin> = Box::pin(BrokenPipeWriter); + let (tx, rx) = mpsc::channel::(SINK_CHANNEL_CAPACITY); + let sink = tokio::spawn(run_container_stdin_sink(writer, rx)); + + tx.send(WriteCmd::Forward(b"first\n".to_vec())) + .await + .expect("first forward"); + tx.send(WriteCmd::Synthesized(b"second\n".to_vec())) + .await + .expect("second send"); + drop(tx); + + sink.await + .expect("sink task joins") + .expect("sink absorbs broken pipe without erroring"); +} + +#[rstest] +#[case::lf(b"\n" as &[u8])] +#[case::crlf(b"\r\n" as &[u8])] +#[tokio::test] +async fn synthesized_response_preserves_blocked_frame_line_ending(#[case] line_ending: &[u8]) { + let (mut adapter, rx) = build_adapter(); + let host_stdout = RecordingWriter::default(); + let mut writer: Pin> = Box::pin(host_stdout); + let mut frame = serde_json::to_vec(&serde_json::json!({ + "jsonrpc": "2.0", + "id": 9, + "method": "terminal/create", + "params": {}, + })) + .expect("blocked request serializes"); + frame.extend_from_slice(line_ending); + + adapter + .handle_chunk(&frame, &mut writer) + .await + .expect("chunk"); + drop(adapter); + + let received = drain_channel(rx).await; + let bytes = match received.as_slice() { + [WriteCmd::Synthesized(bytes)] => bytes.clone(), + other => panic!("expected synthesized response, got {other:?}"), + }; + assert!( + bytes.ends_with(line_ending), + "synthesized response should reuse the blocked frame's line ending", + ); +} + +#[tokio::test] +async fn blocked_request_synthesized_before_channel_close_is_flushed() { + let recorder = RecordingWriter::default(); + let writer_handle = recorder.clone(); + let writer: Pin> = Box::pin(recorder); + let (tx, rx) = mpsc::channel::(SINK_CHANNEL_CAPACITY); + let sink = tokio::spawn(run_container_stdin_sink(writer, rx)); + + let assembler = OutboundFrameAssembler::new(MethodDenylist::default_families()); + let mut adapter = OutboundPolicyAdapter::new(assembler, tx.clone(), "container-test"); + let host_stdout = RecordingWriter::default(); + let mut host_writer: Pin> = Box::pin(host_stdout); + let frame = blocked_request_frame(&serde_json::json!(11)); + + adapter + .handle_chunk(&frame, &mut host_writer) + .await + .expect("chunk"); + adapter.finish(); + drop(adapter); + drop(tx); + + sink.await.expect("sink joins").expect("sink runs cleanly"); + + let bytes = writer_handle.snapshot(); + assert!( + !bytes.is_empty(), + "synthesized response must be flushed before container stdin closes", + ); + let parsed: Value = serde_json::from_slice( + bytes + .strip_suffix(b"\n") + .expect("synthesized response ends with newline"), + ) + .expect("synthesized response is valid JSON"); + assert_eq!(parsed.get("id"), Some(&serde_json::json!(11))); +} diff --git a/src/engine/connection/exec/mod.rs b/src/engine/connection/exec/mod.rs index 5ad610a6..4a8dce15 100644 --- a/src/engine/connection/exec/mod.rs +++ b/src/engine/connection/exec/mod.rs @@ -3,6 +3,10 @@ //! This module wraps Bollard exec APIs behind a small trait seam so command //! execution behaviour can be unit-tested without a live daemon. +mod acp_frame; +mod acp_helpers; +mod acp_policy; +mod acp_runtime; mod attached; mod helpers; mod host_io; diff --git a/src/engine/connection/exec/protocol.rs b/src/engine/connection/exec/protocol.rs index 602599e2..da722f67 100644 --- a/src/engine/connection/exec/protocol.rs +++ b/src/engine/connection/exec/protocol.rs @@ -69,14 +69,18 @@ use tokio::io::{AsyncRead, AsyncWrite, AsyncWriteExt}; use tokio::task::JoinHandle; use tokio::time::timeout; -#[path = "acp_helpers.rs"] -mod acp_helpers; - +use super::ExecRequest; +use super::acp_frame::OutboundFrameAssembler; +use super::acp_helpers; +use super::acp_policy::MethodDenylist; +use super::acp_runtime::{ + OutboundPolicyAdapter, SINK_CHANNEL_CAPACITY, WriteCmd, run_container_stdin_sink, +}; use super::helpers::spawn_stdin_forwarding_task; use super::host_io::stdin_forwarding_disabled_for_tests; -use super::{ExecRequest, exec_failed}; +use super::runtime_helpers::exec_failed; +use super::session::CapabilityPolicy; use crate::error::PodbotError; - /// Host-side stdio handles used by the protocol byte proxy. pub(super) struct ProtocolProxyIo { /// Host stdin reader supplied to the forwarding task. @@ -102,7 +106,7 @@ const STDIN_SETTLE_TIMEOUT: Duration = Duration::from_millis(50); /// limiting how many bytes can be in flight. A 64 KiB buffer aligns with common /// protocol message sizes and typical OS pipe buffer defaults, keeping latency /// low while preventing unbounded accumulation during high-throughput scenarios. -const STDIN_BUFFER_CAPACITY: usize = 65_536; +pub(super) const STDIN_BUFFER_CAPACITY: usize = 65_536; /// Per-session configuration knobs for protocol-mode exec proxy loops. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] @@ -112,7 +116,7 @@ pub(super) struct ProtocolSessionOptions { disable_stdin_forwarding: bool, /// When `true`, the first ACP `initialize` frame is rewritten to remove /// `terminal` and `fs` capabilities before being forwarded to the container. - rewrite_acp_initialize: bool, + capability_policy: CapabilityPolicy, } impl ProtocolSessionOptions { @@ -120,7 +124,7 @@ impl ProtocolSessionOptions { pub(super) const fn new() -> Self { Self { disable_stdin_forwarding: false, - rewrite_acp_initialize: false, + capability_policy: CapabilityPolicy::Disabled, } } @@ -130,9 +134,11 @@ impl ProtocolSessionOptions { self } - /// Enable or disable Agentic Control Protocol (ACP) initialisation rewriting for this protocol session. - pub(super) const fn with_acp_initialize_rewrite_enabled(mut self, enable: bool) -> Self { - self.rewrite_acp_initialize = enable; + /// Select the [`CapabilityPolicy`] that governs Agentic Control Protocol + /// (ACP) capability masking and runtime denylist enforcement for this + /// protocol session. + pub(super) const fn with_capability_policy(mut self, policy: CapabilityPolicy) -> Self { + self.capability_policy = policy; self } } @@ -213,6 +219,24 @@ fn protocol_host_stdin(options: ProtocolSessionOptions) -> Pin( + request: &ExecRequest, + output: Pin> + Send>>, + input: Pin>, + stdio: ProtocolProxyIo, +) -> Result<(), PodbotError> +where + HostStdin: AsyncRead + Send + Unpin + 'static, + HostStdout: AsyncWrite + Send + Unpin, + HostStderr: AsyncWrite + Send + Unpin, +{ + if stdio.options.capability_policy.allows_runtime_enforcement() { + run_session_with_runtime_enforcement(request, output, input, stdio).await + } else { + run_session_without_runtime_enforcement(request, output, input, stdio).await + } +} + +async fn run_session_without_runtime_enforcement( request: &ExecRequest, mut output: Pin> + Send>>, input: Pin>, @@ -229,7 +253,7 @@ where stderr: mut host_stderr, options, } = stdio; - let rewrite_acp_initialize = options.rewrite_acp_initialize; + let rewrite_acp_initialize = options.capability_policy.rewrites_initialize(); let stdin_task = spawn_stdin_forwarding_task(host_stdin, input, move |stdin_reader, exec_input| { forward_host_stdin_to_exec_async(stdin_reader, exec_input, rewrite_acp_initialize) @@ -247,6 +271,181 @@ where stdin_result } +async fn run_session_with_runtime_enforcement( + request: &ExecRequest, + mut output: Pin> + Send>>, + input: Pin>, + stdio: ProtocolProxyIo, +) -> Result<(), PodbotError> +where + HostStdin: AsyncRead + Send + Unpin + 'static, + HostStdout: AsyncWrite + Send + Unpin, + HostStderr: AsyncWrite + Send + Unpin, +{ + let ProtocolProxyIo { + stdin: host_stdin, + stdout: mut host_stdout, + stderr: mut host_stderr, + options, + } = stdio; + let rewrite_acp_initialize = options.capability_policy.rewrites_initialize(); + let container_id_owned = String::from(request.container_id()); + let (sink_tx, sink_rx) = tokio::sync::mpsc::channel::(SINK_CHANNEL_CAPACITY); + let sink_task = tokio::spawn(run_container_stdin_sink(input, sink_rx)); + + let stdin_sender = sink_tx.clone(); + let stdin_task = tokio::spawn(async move { + forward_host_stdin_to_channel(host_stdin, stdin_sender, rewrite_acp_initialize).await + }); + + let assembler = OutboundFrameAssembler::new(MethodDenylist::default_families()); + let mut adapter = OutboundPolicyAdapter::new(assembler, sink_tx, container_id_owned); + + let mut adapter_io = AdapterOutputIo { + adapter: &mut adapter, + host_stdout: &mut host_stdout, + host_stderr: &mut host_stderr, + }; + let output_result = + run_output_loop_with_adapter(request.container_id(), &mut output, &mut adapter_io).await; + adapter.finish(); + drop(adapter); + + let stdin_result = + settle_stdin_forwarding_task(request.container_id(), stdin_task, options).await; + + if let Err(error) = output_result { + sink_task.abort(); + return Err(error); + } + if let Err(error) = stdin_result { + sink_task.abort(); + return Err(error); + } + + sink_task + .await + .map_err(|error| { + exec_failed( + request.container_id(), + format!("container stdin sink task failed: {error}"), + ) + })? + .map_err(|error| { + exec_failed( + request.container_id(), + format!("container stdin sink failed: {error}"), + ) + }) +} + +/// Reads the first newline-delimited ACP frame from `buffered_stdin`, +/// applies capability masking, and forwards the resulting bytes to +/// `sender` as a [`WriteCmd::Forward`]. +/// +/// Returns `Ok(true)` when the caller should continue pumping host +/// stdin (either because the masker produced no bytes to forward, or +/// because the send to the sink succeeded). Returns `Ok(false)` when +/// the sink channel has closed and the caller should return cleanly. +async fn send_masked_initialize_frame( + buffered_stdin: &mut tokio::io::BufReader, + sender: &tokio::sync::mpsc::Sender, +) -> io::Result +where + R: AsyncRead + Unpin, +{ + let bytes = acp_helpers::read_and_mask_initial_acp_frame(buffered_stdin).await?; + if bytes.is_empty() { + return Ok(true); + } + Ok(sender.send(WriteCmd::Forward(bytes)).await.is_ok()) +} + +/// Pumps the remainder of host stdin into the container-stdin sink as +/// a sequence of [`WriteCmd::Forward`] chunks bounded by +/// [`STDIN_BUFFER_CAPACITY`]. Stops on EOF or when the sink channel +/// closes. +async fn pump_raw_frames( + buffered_stdin: &mut tokio::io::BufReader, + sender: &tokio::sync::mpsc::Sender, +) -> io::Result<()> +where + R: AsyncRead + Unpin, +{ + use tokio::io::AsyncReadExt; + + 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(()) +} +async fn forward_host_stdin_to_channel( + host_stdin: HostStdin, + sender: tokio::sync::mpsc::Sender, + rewrite_acp_initialize: bool, +) -> io::Result<()> +where + HostStdin: AsyncRead + Unpin, +{ + let mut buffered_stdin = tokio::io::BufReader::with_capacity(STDIN_BUFFER_CAPACITY, host_stdin); + + if rewrite_acp_initialize && !send_masked_initialize_frame(&mut buffered_stdin, &sender).await? + { + return Ok(()); + } + + pump_raw_frames(&mut buffered_stdin, &sender).await +} + +struct AdapterOutputIo<'a, HostStdout, HostStderr> { + adapter: &'a mut OutboundPolicyAdapter, + host_stdout: &'a mut HostStdout, + host_stderr: &'a mut HostStderr, +} +async fn run_output_loop_with_adapter( + container_id: &str, + output: &mut Pin> + Send>>, + io: &mut AdapterOutputIo<'_, HostStdout, HostStderr>, +) -> Result<(), PodbotError> +where + HostStdout: AsyncWrite + Unpin, + HostStderr: AsyncWrite + Unpin, +{ + while let Some(chunk_result) = output.next().await { + let chunk = chunk_result + .map_err(|error| exec_failed(container_id, format!("exec stream failed: {error}")))?; + match chunk { + LogOutput::StdOut { message } | LogOutput::Console { message } => { + io.adapter + .handle_chunk(message.as_ref(), io.host_stdout) + .await + .map_err(|error| { + exec_failed( + container_id, + format!("failed writing stdout output: {error}"), + ) + })?; + } + LogOutput::StdErr { message } => { + write_output_chunk(container_id, io.host_stderr, message.as_ref(), "stderr") + .await?; + } + LogOutput::StdIn { .. } => {} + } + } + Ok(()) +} /// Wait for the stdin forwarding task to complete within a short grace /// period. async fn settle_stdin_forwarding_task( diff --git a/src/engine/connection/exec/protocol_acp_policy_integration_tests.rs b/src/engine/connection/exec/protocol_acp_policy_integration_tests.rs new file mode 100644 index 00000000..adf1a143 --- /dev/null +++ b/src/engine/connection/exec/protocol_acp_policy_integration_tests.rs @@ -0,0 +1,187 @@ +//! Integration-style ACP policy-selection tests for protocol sessions. + +use std::io; +use std::pin::Pin; +use std::sync::{Arc, Mutex}; +use std::task::{Context, Poll}; + +use bollard::container::LogOutput; +use futures_util::stream; +use tokio::io::AsyncWrite; + +use super::super::{ProtocolProxyIo, ProtocolSessionOptions, run_protocol_session_with_io_async}; +use crate::engine::connection::exec::session::CapabilityPolicy; +use crate::engine::connection::exec::{ExecMode, ExecRequest}; +use crate::error::PodbotError; + +#[derive(Clone, Default)] +struct RecordingOutput { + bytes: Arc>>, +} + +impl RecordingOutput { + fn snapshot(&self) -> Vec { + self.bytes.lock().expect("recording mutex").clone() + } +} + +impl AsyncWrite for RecordingOutput { + fn poll_write( + self: Pin<&mut Self>, + _cx: &mut Context<'_>, + buf: &[u8], + ) -> Poll> { + self.bytes + .lock() + .expect("recording mutex") + .extend_from_slice(buf); + Poll::Ready(Ok(buf.len())) + } + + fn poll_flush(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + + fn poll_shutdown(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } +} + +fn protocol_request() -> Result { + ExecRequest::new( + "policy-selection-sandbox", + vec![String::from("codex"), String::from("app-server")], + ExecMode::Protocol, + ) +} + +fn blocked_request_frame(id: i64) -> Vec { + let mut bytes = serde_json::to_vec(&serde_json::json!({ + "jsonrpc": "2.0", + "id": id, + "method": "terminal/create", + "params": {}, + })) + .expect("blocked request should serialize"); + bytes.push(b'\n'); + bytes +} + +fn drive_policy_session(policy: CapabilityPolicy) -> (Vec, Vec, Vec) { + let runtime = tokio::runtime::Runtime::new().expect("runtime should build"); + let initialize = super::initialize_frame("\n"); + let host_stdin = runtime + .block_on(super::build_host_stdin(&initialize)) + .expect("host stdin should build"); + let host_stdout = RecordingOutput::default(); + let host_stdout_handle = host_stdout.clone(); + let host_stderr = RecordingOutput::default(); + let host_stderr_handle = host_stderr.clone(); + let container_input = super::RecordingInputWriter::new(); + let container_stdin = container_input.bytes.clone(); + let output = stream::iter([Ok(LogOutput::StdOut { + message: blocked_request_frame(7).into(), + })]); + let stdio = ProtocolProxyIo::new(host_stdin, host_stdout, host_stderr) + .with_options(ProtocolSessionOptions::new().with_capability_policy(policy)); + + runtime + .block_on(run_protocol_session_with_io_async( + &protocol_request().expect("protocol request should build"), + Box::pin(output), + Box::pin(container_input), + stdio, + )) + .expect("policy session should complete"); + + ( + container_stdin.lock().expect("stdin mutex").clone(), + host_stdout_handle.snapshot(), + host_stderr_handle.snapshot(), + ) +} + +fn parse_json_line(bytes: &[u8]) -> serde_json::Value { + serde_json::from_slice(bytes.strip_suffix(b"\n").unwrap_or(bytes)) + .expect("line should contain JSON") +} + +fn split_lines(bytes: &[u8]) -> Vec<&[u8]> { + bytes + .split_inclusive(|byte| *byte == b'\n') + .filter(|line| !line.is_empty()) + .collect() +} + +fn line_matches(bytes: &[u8], expected: &[u8]) -> bool { + bytes == expected +} + +fn synthesized_response_for_method(bytes: &[u8], method: &str) -> bool { + let response = parse_json_line(bytes); + response.get("id") == Some(&serde_json::json!(7)) + && response + .get("error") + .and_then(|error| error.get("data")) + .and_then(|data| data.get("method")) + == Some(&serde_json::json!(method)) +} + +#[test] +fn mask_and_deny_masks_initialize_and_synthesizes_blocked_response() { + let (container_stdin, host_stdout, _host_stderr) = + drive_policy_session(CapabilityPolicy::MaskAndDeny); + let lines = split_lines(&container_stdin); + let expected_initialize = super::mask_acp_initialize_frame(&super::initialize_frame("\n")); + + assert!( + host_stdout.is_empty(), + "blocked outbound request must not reach host stdout", + ); + assert!( + lines + .iter() + .any(|line| line_matches(line, &expected_initialize)), + "initialize should be masked before it reaches container stdin", + ); + assert!( + lines + .iter() + .any(|line| synthesized_response_for_method(line, "terminal/create")), + "blocked request should produce a synthesized response", + ); +} + +#[test] +fn disabled_policy_preserves_plain_streaming_proxy_behaviour() { + let (container_stdin, host_stdout, _host_stderr) = + drive_policy_session(CapabilityPolicy::Disabled); + let blocked = blocked_request_frame(7); + + assert_eq!( + container_stdin, + super::initialize_frame("\n"), + "Disabled should forward host stdin without ACP masking", + ); + assert_eq!( + host_stdout, blocked, + "Disabled should not enforce the outbound denylist", + ); +} + +#[test] +fn mask_only_masks_initialize_without_runtime_denylist_enforcement() { + let (container_stdin, host_stdout, _host_stderr) = + drive_policy_session(CapabilityPolicy::MaskOnly); + let blocked = blocked_request_frame(7); + + assert_eq!( + container_stdin, + super::mask_acp_initialize_frame(&super::initialize_frame("\n")), + "MaskOnly should apply the initialize masking path", + ); + assert_eq!( + host_stdout, blocked, + "MaskOnly should not enforce the outbound denylist", + ); +} diff --git a/src/engine/connection/exec/protocol_acp_tests.rs b/src/engine/connection/exec/protocol_acp_tests.rs index 3541e1d7..313a93ed 100644 --- a/src/engine/connection/exec/protocol_acp_tests.rs +++ b/src/engine/connection/exec/protocol_acp_tests.rs @@ -8,11 +8,11 @@ use std::task::{Context, Poll}; use rstest::rstest; use tokio::io::{AsyncWrite, AsyncWriteExt, DuplexStream}; -use super::acp_helpers::{ +use super::*; +use crate::engine::connection::exec::acp_helpers::{ ACP_FILE_SYSTEM_CAPABILITY, ACP_TERMINAL_CAPABILITY, MAX_FIRST_FRAME_BYTES, forward_initial_acp_frame_async, mask_acp_initialize_frame, split_frame_line_ending, }; -use super::*; struct RecordingInputWriter { bytes: Arc>>, @@ -385,8 +385,134 @@ pub(super) fn masked_initialize_with_follow_up() -> (Vec, Vec) { (host_stdin_bytes, expected) } +#[cfg(test)] +mod capability_policy_routing { + //! Verifies that `CapabilityPolicy` selects raw forwarding or runtime + //! enforcement for outbound ACP frames. + + use bollard::container::LogOutput; + use futures_util::stream; + + use super::*; + use crate::engine::connection::exec::session::CapabilityPolicy; + use crate::engine::connection::exec::{ExecMode, ExecRequest}; + + fn protocol_request() -> ExecRequest { + ExecRequest::new( + "capability-policy-routing", + vec![String::from("codex"), String::from("app-server")], + ExecMode::Protocol, + ) + .expect("protocol request should build") + } + + fn blocked_terminal_create_frame() -> Vec { + let mut bytes = serde_json::to_vec(&serde_json::json!({ + "jsonrpc": "2.0", + "id": 7, + "method": "terminal/create", + "params": {}, + })) + .expect("blocked request should serialize"); + bytes.push(b'\n'); + bytes + } + + fn run_policy_output_frame(policy: CapabilityPolicy, frame: &[u8]) -> (Vec, Vec) { + let runtime = tokio::runtime::Runtime::new().expect("runtime should build"); + let host_stdin = runtime + .block_on(build_host_stdin(&[])) + .expect("host stdin should build"); + let host_stdout = RecordingInputWriter::new(); + let host_stdout_bytes = host_stdout.bytes.clone(); + let host_stderr = RecordingInputWriter::new(); + let container_input = RecordingInputWriter::new(); + let container_stdin_bytes = container_input.bytes.clone(); + let output = stream::iter([Ok(LogOutput::StdOut { + message: frame.to_vec().into(), + })]); + let stdio = ProtocolProxyIo::new(host_stdin, host_stdout, host_stderr) + .with_options(ProtocolSessionOptions::new().with_capability_policy(policy)); + + runtime + .block_on(run_protocol_session_with_io_async( + &protocol_request(), + Box::pin(output), + Box::pin(container_input), + stdio, + )) + .expect("protocol session should complete"); + + ( + host_stdout_bytes + .lock() + .expect("stdout mutex should not poison") + .clone(), + container_stdin_bytes + .lock() + .expect("stdin mutex should not poison") + .clone(), + ) + } + + fn synthesized_response_for_terminal_create(bytes: &[u8]) -> bool { + let response: serde_json::Value = + serde_json::from_slice(bytes.strip_suffix(b"\n").unwrap_or(bytes)) + .expect("synthesized response should contain JSON"); + response.get("id") == Some(&serde_json::json!(7)) + && response + .get("error") + .and_then(|error| error.get("data")) + .and_then(|data| data.get("method")) + == Some(&serde_json::json!("terminal/create")) + } + + #[rstest] + #[case::mask_and_deny_routes_through_enforcement_path( + CapabilityPolicy::MaskAndDeny, + false, + true + )] + #[case::disabled_policy_forwards_all_frames_raw(CapabilityPolicy::Disabled, true, false)] + #[case::mask_only_policy_forwards_blocked_frames_raw(CapabilityPolicy::MaskOnly, true, false)] + fn routes_output_frame_for_capability_policy( + #[case] policy: CapabilityPolicy, + #[case] expect_forward_raw: bool, + #[case] expect_synthesized_response: bool, + ) { + let frame = blocked_terminal_create_frame(); + let (host_stdout, container_stdin) = run_policy_output_frame(policy, &frame); + + if expect_forward_raw { + assert_eq!( + host_stdout, frame, + "{policy:?} should preserve the byte-transparent output path", + ); + } else { + assert_ne!( + host_stdout, frame, + "{policy:?} must not forward blocked frames verbatim", + ); + } + if expect_synthesized_response { + assert!( + synthesized_response_for_terminal_create(&container_stdin), + "{policy:?} should write a synthesized denial response to container stdin", + ); + } else { + assert!( + container_stdin.is_empty(), + "{policy:?} should not write a synthesized denial response", + ); + } + } +} + #[path = "protocol_acp_forwarding_tests.rs"] mod forwarding_tests; +#[path = "protocol_acp_policy_integration_tests.rs"] +mod policy_integration_tests; + #[path = "protocol_acp_bdd_tests.rs"] mod bdd_tests; diff --git a/src/engine/connection/exec/session.rs b/src/engine/connection/exec/session.rs index 0ef1e966..ddb4650e 100644 --- a/src/engine/connection/exec/session.rs +++ b/src/engine/connection/exec/session.rs @@ -2,17 +2,26 @@ use super::protocol::ProtocolSessionOptions; -/// Internal exec-session knobs used by test harnesses that need deterministic -/// stream behaviour. -#[derive(Debug, Clone, Copy, Default)] +/// Capability-enforcement policy for Agentic Control Protocol (ACP) hosting. +/// +/// `Disabled` is the default and matches the original +/// [`super::ExecMode::Protocol`] contract: a byte-transparent stdin/stdout +/// proxy. `MaskOnly` rewrites the first ACP `initialize` frame to strip +/// `terminal/*` and `fs/*` capability advertisements but otherwise forwards +/// every later frame unchanged. `MaskAndDeny` additionally enforces a +/// runtime denylist on agent-emitted JSON-RPC frames, refusing +/// `terminal/*` and `fs/*` requests with a synthesized JSON-RPC error +/// response and recording each denial on stderr. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] pub(crate) struct ExecSessionOptions { /// When `true`, protocol-mode sessions replace host stdin with a held-open /// no-op reader so that the process's inherited stdin is not forwarded. disable_protocol_stdin_forwarding: bool, - /// When `true`, the first ACP `initialize` frame sent from host stdin is - /// rewritten to remove `terminal` and `fs` capabilities before being - /// forwarded to the container. - rewrite_acp_initialize: bool, + /// The `capability_policy` mode for ACP sessions: `Disabled` leaves the + /// initial `initialize` frame unchanged, `MaskOnly` strips `terminal` and + /// `fs` capabilities before forwarding it, and `MaskAndDeny` also denies + /// later requests that try to use those capabilities. + capability_policy: CapabilityPolicy, } impl ExecSessionOptions { @@ -21,7 +30,7 @@ impl ExecSessionOptions { pub const fn new() -> Self { Self { disable_protocol_stdin_forwarding: false, - rewrite_acp_initialize: false, + capability_policy: CapabilityPolicy::Disabled, } } @@ -34,7 +43,7 @@ impl ExecSessionOptions { self } - /// Enable ACP initialization rewriting for protocol-mode sessions. + /// Select the [`CapabilityPolicy`] for this protocol-mode session. #[cfg_attr( not(test), expect( @@ -43,8 +52,8 @@ impl ExecSessionOptions { ) )] #[must_use] - pub const fn with_acp_initialize_rewrite_enabled(mut self, enable: bool) -> Self { - self.rewrite_acp_initialize = enable; + pub const fn with_capability_policy(mut self, policy: CapabilityPolicy) -> Self { + self.capability_policy = policy; self } } @@ -56,7 +65,7 @@ pub(super) const fn protocol_session_options( ) -> ProtocolSessionOptions { ProtocolSessionOptions::new() .with_stdin_forwarding_disabled(options.disable_protocol_stdin_forwarding) - .with_acp_initialize_rewrite_enabled(options.rewrite_acp_initialize) + .with_capability_policy(options.capability_policy) } #[cfg(test)] @@ -70,10 +79,7 @@ mod tests { !opts.disable_protocol_stdin_forwarding, "stdin forwarding should be enabled by default", ); - assert!( - !opts.rewrite_acp_initialize, - "ACP initialize rewriting should be opt-in", - ); + assert_eq!(opts.capability_policy, CapabilityPolicy::Disabled); } #[test] @@ -118,12 +124,68 @@ mod tests { } #[test] - fn protocol_session_options_reflects_acp_rewrite_flag() { - let opts = ExecSessionOptions::new().with_acp_initialize_rewrite_enabled(true); + fn protocol_session_options_reflects_mask_only_policy() { + let opts = ExecSessionOptions::new().with_capability_policy(CapabilityPolicy::MaskOnly); assert_eq!( protocol_session_options(opts), - ProtocolSessionOptions::new().with_acp_initialize_rewrite_enabled(true), + ProtocolSessionOptions::new().with_capability_policy(CapabilityPolicy::MaskOnly), ); } + + #[test] + fn protocol_session_options_reflects_mask_and_deny_policy() { + let opts = ExecSessionOptions::new().with_capability_policy(CapabilityPolicy::MaskAndDeny); + + assert_eq!( + protocol_session_options(opts), + ProtocolSessionOptions::new().with_capability_policy(CapabilityPolicy::MaskAndDeny), + ); + } + + #[test] + fn capability_policy_rewrites_initialize_for_masked_modes() { + assert!(!CapabilityPolicy::Disabled.rewrites_initialize()); + assert!(CapabilityPolicy::MaskOnly.rewrites_initialize()); + assert!(CapabilityPolicy::MaskAndDeny.rewrites_initialize()); + } + + #[test] + fn capability_policy_runtime_enforcement_only_for_mask_and_deny() { + assert!(!CapabilityPolicy::Disabled.allows_runtime_enforcement()); + assert!(!CapabilityPolicy::MaskOnly.allows_runtime_enforcement()); + assert!(CapabilityPolicy::MaskAndDeny.allows_runtime_enforcement()); + } +} + +#[cfg_attr( + not(test), + expect( + dead_code, + reason = "MaskOnly and MaskAndDeny remain unused until podbot host wires the opt-in" + ) +)] +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub(crate) enum CapabilityPolicy { + /// Pure byte proxy; no ACP-specific behaviour. + #[default] + Disabled, + /// Init-time capability masking only (Step 2.6.1 behaviour). + MaskOnly, + /// Init-time masking plus runtime method denylist (Step 2.6.2 behaviour). + MaskAndDeny, +} + +impl CapabilityPolicy { + /// Return `true` when the policy should rewrite the first ACP + /// `initialize` frame to mask blocked capabilities. + pub(crate) const fn rewrites_initialize(self) -> bool { + matches!(self, Self::MaskOnly | Self::MaskAndDeny) + } + + /// Return `true` when the policy should enforce the runtime + /// method denylist on agent-emitted frames. + pub(crate) const fn allows_runtime_enforcement(self) -> bool { + matches!(self, Self::MaskAndDeny) + } } diff --git a/src/engine/connection/exec/tests/proxy_helpers/error_mapping.rs b/src/engine/connection/exec/tests/proxy_helpers/error_mapping.rs index 57afacb3..d47e75d8 100644 --- a/src/engine/connection/exec/tests/proxy_helpers/error_mapping.rs +++ b/src/engine/connection/exec/tests/proxy_helpers/error_mapping.rs @@ -2,14 +2,18 @@ use std::pin::Pin; use std::task::{Context, Poll}; +use std::time::Duration; use bollard::container::LogOutput; use bollard::errors::Error as BollardError; use rstest::rstest; use tokio::io::AsyncRead; -use super::super::super::protocol::{ProtocolProxyIo, run_protocol_session_with_io_async}; +use super::super::super::protocol::{ + ProtocolProxyIo, ProtocolSessionOptions, run_protocol_session_with_io_async, +}; use super::*; +use crate::engine::connection::exec::session::CapabilityPolicy; struct PendingReader; @@ -125,3 +129,36 @@ fn protocol_proxy_times_out_incomplete_stdin_forwarding(runtime: RuntimeFixture) "stdin forwarding did not complete before protocol session shutdown", ); } + +#[rstest] +fn runtime_enforcement_propagates_stdin_timeout_before_sink_wait(runtime: RuntimeFixture) { + let runtime_handle = runtime.expect("runtime fixture should initialize"); + let request = make_protocol_request().expect("protocol request should build"); + let stdio = ProtocolProxyIo::new( + PendingReader, + RecordingWriter::new(), + RecordingWriter::new(), + ) + .with_options( + ProtocolSessionOptions::new().with_capability_policy(CapabilityPolicy::MaskAndDeny), + ); + let result = runtime_handle + .block_on(async { + tokio::time::timeout( + Duration::from_secs(1), + run_protocol_session_with_io_async( + &request, + make_output_stream(vec![]), + Box::pin(RecordingInputWriter::new()), + stdio, + ), + ) + .await + }) + .expect("runtime enforcement session should not hang behind the sink"); + + assert_exec_failed_message( + result, + "stdin forwarding did not complete before protocol session shutdown", + ); +} diff --git a/tests/features/acp_method_denylist.feature b/tests/features/acp_method_denylist.feature new file mode 100644 index 00000000..600c35d8 --- /dev/null +++ b/tests/features/acp_method_denylist.feature @@ -0,0 +1,36 @@ +Feature: ACP runtime method denylist + + Podbot must reject Agentic Control Protocol (ACP) capability methods that + the hosted agent attempts after initialization, returning a synthesized + JSON-RPC error response and recording each denial on stderr without + forwarding the request to the host. + + Scenario: Blocked request returns a synthesized error and is not forwarded + Given the ACP runtime adapter is configured with the default denylist + When the agent emits a "terminal/create" request with id 7 + Then host stdout receives no bytes from the blocked request + And container stdin receives a synthesized JSON-RPC error with id 7 + + Scenario: Permitted method passes through unchanged byte-for-byte + Given the ACP runtime adapter is configured with the default denylist + When the agent emits a "session/new" request with id 1 + Then host stdout receives the permitted frame verbatim + And container stdin receives no synthesized response + + Scenario: Blocked notification is dropped silently + Given the ACP runtime adapter is configured with the default denylist + When the agent emits an "fs/changed" notification + Then host stdout receives no bytes from the blocked notification + And container stdin receives no synthesized response + + Scenario: Frame split across two chunks reassembles before the policy applies + Given the ACP runtime adapter is configured with the default denylist + When the agent emits a blocked frame split across two output chunks + Then host stdout receives no bytes from the blocked request + And container stdin receives a synthesized JSON-RPC error with id 2 + + Scenario: Permitted frame following a blocked frame still flushes correctly + Given the ACP runtime adapter is configured with the default denylist + When the agent emits a blocked request followed by a permitted request + Then host stdout receives only the permitted frame verbatim + And container stdin receives a synthesized JSON-RPC error for the blocked request