Document ACP capability masking modules and session knobs

[lodyai]

Summary

This branch documents the Agentic Control Protocol (ACP) capability-masking
seam introduced in #79. It updates the exec subsystem repository layout in
the developer's guide and adds field- and method-level /// doc comments
to the new ACP session knobs so that cargo build no longer surfaces
missing_docs warnings against them and so that future contributors can
discover the seam from the documented module map.

The change is documentation only; no production behaviour changes.

Review walkthrough

• Start with docs/developers-guide.md
  to see the refreshed exec-subsystem layout: acp_helpers.rs is listed
  immediately after protocol.rs, the session.rs entry now mentions the
  with_acp_initialize_rewrite_enabled opt-in, and the three ACP test
  modules (protocol_acp_tests.rs, protocol_acp_bdd_tests.rs,
  protocol_acp_forwarding_tests.rs) are appended to the tests/
  subtree.
• Then review src/engine/connection/exec/session.rs
  for the /// doc comment on the rewrite_acp_initialize field of
  ExecSessionOptions, which describes the masking behaviour applied to
  the first ACP initialize frame.
• Finish with src/engine/connection/exec/protocol.rs
  for the matching field-level doc on ProtocolSessionOptions::rewrite_acp_initialize
  and the new /// doc comment on the with_stdin_forwarding_disabled
  builder.

Validation

• cargo build: clean (no missing_docs warnings)
• cargo clippy --workspace --all-targets --all-features -- -D warnings:
  clean
• cargo test --workspace: all suites pass (33 result lines, 0 failures)

Notes

No execplan is associated with this branch; the work is a follow-up to the
ACP capability-masking implementation merged in #79 and addresses the
developer-documentation gaps noted against that change.

[sourcery-ai]

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

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 817bf961-8fc6-4169-8280-9486dfa76676

📥 Commits

Reviewing files that changed from the base of the PR and between b1884a7 and c951451.

📒 Files selected for processing (1)

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

---

This documentation-only change fills developer-documentation gaps introduced alongside the ACP capability-masking implementation in #79. All edits are doc-comments and docs updates; no runtime behaviour, logic, signatures, tests or exported API were changed.

Changes

• docs/developers-guide.md (+23/-4)

  • Updated the exec subsystem layout to list the new acp_helpers.rs module (covers ACP initialize-frame rewriting and capability masking).
  • Documented ExecSessionOptions knobs: per-call option to disable protocol stdin forwarding for tests and the opt-in knob to enable ACP initialize rewriting.
  • Expanded the documented test-tree to include ACP-focused tests: protocol_acp_tests.rs, protocol_acp_bdd_tests.rs, protocol_acp_forwarding_tests.rs.

• src/engine/connection/exec/protocol.rs (+70/-4)

  • Added extensive module-level and item-level Rust doc comments describing the protocol-mode exec proxy’s behaviour:
    • Stdout purity and chunk routing contracts.
    • The concurrency model for protocol sessions: two concurrent paths (a spawned stdin-forwarding task and the caller-driven output loop), task coordination (await output loop first, then call settle_stdin_forwarding_task), the 50 ms STDIN_SETTLE_TIMEOUT grace period, abort semantics (JoinHandle::abort then drop without awaiting), and how the HeldOpenStdin adapter behaves when stdin forwarding is disabled.
    • Field- and method-level docs for ProtocolProxyIo and ProtocolSessionOptions, plus private-item docs added to satisfy doc-coverage checks.
    • Documentation for output draining, write/flush error mapping, and related helpers.
  • No functional changes to logic or public APIs.

• src/engine/connection/exec/session.rs (+7/-0)

  • Added/expanded docs for ExecSessionOptions explaining that disable_protocol_stdin_forwarding replaces host stdin with a held-open no-op reader and that rewrite_acp_initialize rewrites the first ACP initialize frame to remove terminal/fs capabilities. Documented the protocol_session_options conversion helper.

• src/engine/connection/exec/acp_helpers.rs (+20/-0)

  • Added/strengthened docs for first-frame scanning enums (InitialFrameAction, ForwardUnchangedReason) and the helpers involved in reading the first frame chunk and emitting debug traces. Split and documented debug-forwarding tracing (exceeded-size vs EOF-before-newline) and clarified masked-forwarding trace helper. Also included a brief concurrency note describing the forwarding task ownership and cancellation behaviour.
  • No changes to masking logic, buffering control flow, or function signatures.

Validation

• cargo build: no missing_docs warnings
• cargo clippy --workspace --all-targets --all-features -- -D warnings: clean
• cargo test --workspace: all suites passed (33 result lines, 0 failures)

Notes: No execplan document was introduced. Work documents the ACP capability-masking seam (from #79), adds an explicit concurrency model section for protocol-mode exec proxy loops, and resolves docstring-coverage/tooling warnings without altering runtime behaviour.

Walkthrough

Summarise the documentation-only updates across the exec subsystem: add acp_helpers.rs with ACP initialize-frame rewriting and capability masking, expose per-call ExecSessionOptions knobs for disabling protocol stdin forwarding and enabling ACP rewriting, expand protocol-level docs, and list new ACP-focused tests. No code behaviour or public signatures changed.

Changes

ACP Documentation and Repository Layout

Layer / File(s)	Summary
Repository Layout / Guide docs/developers-guide.md	Add acp_helpers.rs to exec subsystem tree; document first-frame ACP initialize rewriting that masks terminal/* and fs/* clientCapabilities, reserialises JSON, and respects MAX_FIRST_FRAME_BYTES. Document new ACP test files: protocol_acp_tests.rs, protocol_acp_bdd_tests.rs, protocol_acp_forwarding_tests.rs.
Session Options Docs src/engine/connection/exec/session.rs	Expand ExecSessionOptions docs: make disable_protocol_stdin_forwarding a per-call knob that supplies a held-open no-op reader; add rewrite_acp_initialize opt-in knob and document protocol_session_options conversion to ProtocolSessionOptions.
Protocol Proxy Docs src/engine/connection/exec/protocol.rs	Add module- and item-level doc comments: describe ProtocolSessionOptions effects, HeldOpenStdin behaviour, stdin-forwarding lifecycle (settle_stdin_forwarding_task, abort semantics), optional ACP initialize rewriting during stdin copy, output-draining loop, chunk routing to stdout/stderr, and error mapping on write/flush failures.
ACP Helpers Docs src/engine/connection/exec/acp_helpers.rs	Document InitialFrameAction and ForwardUnchangedReason enums and next_initial_frame_action; split unmodified-forwarding debug traces into reason-specific helpers (log_exceeded_maximum_size, log_eof_before_newline); document masked-forwarding path and remove_masked_acp_capabilities; clarify read_next_bounded_frame_chunk return semantics.

Suggested labels

Issue

Poem

Document the frame where capabilities hide,
Mask the terminals and files with care,
Guide the session knobs that gently decide,
Tests now wait where first newlines stare,
Code unchanged — the map grows bright and fair.

🚥 Pre-merge checks | ✅ 17 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Observability	❓ Inconclusive	No result was produced after verification. Marking as INCONCLUSIVE.	Re-run the check or adjust instructions to produce a final result.

✅ Passed checks (17 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically summarises the primary change: documenting ACP capability masking modules and session knobs across the codebase.
Description check	✅ Passed	The description thoroughly details the documentation updates, references the related issue #79, explains the changes across multiple files, and validates the work with test results.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Testing (Overall)	✅ Passed	This PR is documentation-only with no functional or behavioural changes. All code changes are doc comments. The test files were introduced in prior PR #79.
User-Facing Documentation	✅ Passed	No new user-facing functionality introduced. Existing ACP capability masking is documented in docs/users-guide.md. This PR adds only internal developer documentation.
Developer Documentation	✅ Passed	Documentation fully covers ACP changes: developers-guide section 3 and 8.2.1; design document capability masking architecture; concurrency model in protocol.rs; roadmap step 2.6 tracked.
Module-Level Documentation	✅ Passed	All three modified modules have module-level docstrings with clear purpose. Protocol.rs and acp_helpers.rs provide comprehensive docs; session.rs includes adequate concise documentation.
Testing (Unit And Behavioural)	✅ Passed	Documentation-only PR with no functional code changes. Comprehensive unit and behaviour tests for ACP functionality already exist from prior commits. Testing check not applicable.
Testing (Property / Proof)	✅ Passed	This is a documentation-only PR that introduces no functional code, invariants, axioms, or proof assumptions. Property tests or formal verification are not required for documentation changes.
Testing (Compile-Time / Ui)	✅ Passed	PR contains only documentation changes. No functional code, compile-time behaviour, or UI-adjacent output changes. Trybuild and snapshot tests not applicable.
Unit Architecture	✅ Passed	PR is documentation-only. No functional code, control flow, error handling, or architectural boundaries were changed. Documentation additions do not violate Unit Architecture requirements.
Domain Architecture	✅ Passed	PR is documentation only. Domain architecture properly maintained with ACP protocol concerns confined to adapter layer. No domain policy leakage or functional changes detected.
Security And Privacy	✅ Passed	Documentation-only PR adding doc comments to ACP modules. No secrets, credentials, authentication bypasses, or sensitive data exposure detected.
Performance And Resource Use	✅ Passed	Documentation-only PR with no functional code changes. Adds Rust doc comments to existing code with no new loops, allocations, I/O, or algorithmic logic.
Concurrency And State	✅ Passed	Documentation properly addresses concurrency requirements: explicit concurrency model section covering task coordination, timeout/cancellation, owned lifetimes, and resource cleanup in protocol.rs.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch acp-developer-docs

---

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

[leynos]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

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

[leynos]

@coderabbitai Have the following failed checks now been resolved?

If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures.

Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already.

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Developer Documentation	❓ Inconclusive	Cannot verify developer documentation requirements; the repository does not contain the files referenced in the PR summary (docs/developers-guide.md, src/engine/connection/exec/ files). Repository structure does not align with described changes.	Confirm the correct repository is being evaluated. Once available, verify docs/developers-guide.md has been updated with architectural changes, all new APIs are documented, and design documents exist for significant decisions.

[coderabbitai]

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== Check docs/developers-guide.md ==="
fd "developers-guide.md" --type f

echo ""
echo "=== Check exec directory ==="
fd . --type f --full-path | grep "exec/"

echo ""
echo "=== Repo top-level structure ==="
tree -L 3 --dirsfirst 2>/dev/null || find . -maxdepth 3 -type d | sort

echo ""
echo "=== Git branch ==="
git branch --show-current

echo ""
echo "=== Git log (last 5 commits) ==="
git log --oneline -5

Length of output: 9430

---

@leynos: The "Developer Documentation" check is a false inconclusive. Every file it could not locate is demonstrably present on the PR branch:

File	Status
docs/developers-guide.md	✅ Present
src/engine/connection/exec/protocol.rs	✅ Present
src/engine/connection/exec/session.rs	✅ Present
src/engine/connection/exec/acp_helpers.rs	✅ Present
src/engine/connection/exec/protocol_acp_tests.rs	✅ Present
src/engine/connection/exec/protocol_acp_bdd_tests.rs	✅ Present
src/engine/connection/exec/protocol_acp_forwarding_tests.rs	✅ Present

The checker evaluated either a stale snapshot or an incorrect repository path and reported an absence that does not reflect the actual branch state. No documentation work is outstanding for this PR.

The unreliability of the automated "Developer Documentation" checker is a tooling concern that sits outside the scope of this PR. Shall I open a GitHub issue to track improving its environment resolution so it evaluates the correct branch state?

[coderabbitai]

Tip

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

🧩 Analysis chain

I need to inspect the current state of each file to verify which findings remain open.

---

🏁 Script executed:

#!/bin/bash
echo "=== clippy.toml ==="
cat clippy.toml 2>/dev/null || echo "NOT FOUND"

echo ""
echo "=== src/protocol.rs lines 120-155 ==="
fd "protocol.rs" --type f --exclude "exec" | head -5 | xargs -I{} sed -n '120,155p' {}

echo ""
echo "=== src/server.rs lines 290-500 ==="
fd "server.rs" --type f | head -3 | xargs -I{} sed -n '290,500p' {}

echo ""
echo "=== src/upstream.rs lines 10-40 ==="
fd "upstream.rs" --type f | head -3 | xargs -I{} sed -n '10,40p' {}

echo ""
echo "=== src/upstream.rs lines 95-140 ==="
fd "upstream.rs" --type f | head -3 | xargs -I{} sed -n '95,140p' {}

echo ""
echo "=== src/upstream.rs lines 160-175 ==="
fd "upstream.rs" --type f | head -3 | xargs -I{} sed -n '160,175p' {}

echo ""
echo "=== src/upstream.rs lines 270-290 ==="
fd "upstream.rs" --type f | head -3 | xargs -I{} sed -n '270,290p' {}

Length of output: 534

---

🏁 Script executed:

#!/bin/bash
echo "=== Locate protocol.rs, server.rs, upstream.rs ==="
fd "(protocol|server|upstream)\.rs" --type f

echo ""
echo "=== Full clippy.toml ==="
cat clippy.toml

echo ""
echo "=== Repo top-level structure ==="
tree -L 3 --gitignore -I "target|.git" 2>/dev/null || find . -maxdepth 3 -not -path "./.git/*" -not -path "./target/*" | sort | head -80

Length of output: 7931

---

@leynos: None of the five findings apply to this repository. Here is the verification for each:

---

clippy.toml — Line 2: cognitive-complexity-threshold

The file exists and reads:

# Align with CodeScene's ceiling
cognitive-complexity-threshold = 9     # default is 25

The threshold is already at 9 — stricter than the Clippy default of 25, not looser. There is no src/server.rs in this repository, so neither capture_and_check_stream nor persist_and_respond exist here. Finding does not apply.

---

src/protocol.rs — is_hop_by_hop_header

src/protocol.rs does not exist in this repository. The only protocol.rs present is src/engine/connection/exec/protocol.rs, which is the ACP exec proxy and contains no is_hop_by_hop_header function. Finding does not apply.

---

src/server.rs — test helper extraction

src/server.rs does not exist in this repository. Finding does not apply.

---

src/upstream.rs — doctests, weak assertion, swallowed error (×3)

src/upstream.rs does not exist in this repository. Finding does not apply.

---

These inline comments appear to have originated from a different project. No action is required in this PR, and no issues need to be raised against findings that have no corresponding code in this repository.

[coderabbitai]

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== protocol.rs module doc block ==="
fd "protocol.rs" --type f --full-path | grep "exec/protocol" | xargs sed -n '1,50p'

Length of output: 2493

---

@leynos: Yes — the "Concurrency And State" warning is resolved. The ## Concurrency Model section is present and complete in src/engine/connection/exec/protocol.rs. It covers:

• The two concurrent paths (spawn_stdin_forwarding_task and run_output_loop_async)
• Task coordination (output loop awaited first, then 50 ms grace period via settle_stdin_forwarding_task)
• Timeout and cancellation semantics (JoinHandle::abort, drop without awaiting, is_cancelled() mapped to Ok(()))
• HeldOpenStdin behaviour when disable_stdin_forwarding is set

No further work is required on this PR.