fix: make OpenCode ACP transport persistent - #4
Conversation
Replace the non-portable background stdin assumption with a permanent-FD FIFO controller. Reorganize the installable skill bundle, document the standalone no-Python workflow, align ACP v1 examples, and add lifecycle tests and CI coverage.
|
Warning Review limit reached
Next review available in: 31 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded the version 0.4.0 persistent-FIFO ACP transport bundle. The change includes Bash and Python controllers, skill documentation, protocol references, end-to-end tests, updated CI checks, release metadata, and repository documentation. ChangesPersistent FIFO ACP transport
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant run.py
participant helper.sh
participant OpenCode
participant FrameLog
Client->>run.py: Start runtime
run.py->>helper.sh: Launch transport
helper.sh->>OpenCode: Keep stdin FIFO open
Client->>run.py: Send JSON-RPC frame
run.py->>helper.sh: Write frame to FIFO
OpenCode->>helper.sh: Emit response
helper.sh->>FrameLog: Append NDJSON response
Client->>run.py: Read from cursor
run.py->>FrameLog: Return queued frames
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
skills/opencode-acp-control/scripts/run.py (1)
337-346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated ownership check.
Line 341 already calls
process_belongs_to_runtime. Line 344 repeats the same call with the same inputs, so the guard can never raise. Drop the inner check.♻️ Proposed simplification
controller_owned = bool( controller_pid and process_belongs_to_runtime(controller_pid, runtime) ) if controller_owned and controller_pid: - if not process_belongs_to_runtime(controller_pid, runtime): - raise TransportError("refusing to signal a PID not owned by this runtime") - os.kill(controller_pid, signal.SIGTERM) + try: + os.kill(controller_pid, signal.SIGTERM) + except ProcessLookupError: + passThe added
ProcessLookupErrorguard covers the race where the controller exits between the check and the signal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/opencode-acp-control/scripts/run.py` around lines 337 - 346, Remove the redundant process_belongs_to_runtime check inside command_stop after controller_owned is established; retain the existing ownership calculation and os.kill flow, including the ProcessLookupError race handling.skills/opencode-acp-control/scripts/helper.sh (1)
166-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the final partial frame out of the log, or flush it deliberately.
read -rreturns non-zero for a last chunk without a trailing newline, so that chunk is discarded. This is the correct choice forrun.py, becausecomplete_linesalso drops partial lines. Record the intent so a later change does not append a truncated frame.♻️ Optional: document the discard
while IFS= read -r frame <&4; do printf '%s\n' "$frame" >>"$frames_log" done +# A trailing chunk without a newline is intentionally dropped. run.py only +# parses complete lines from frames.ndjson.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/opencode-acp-control/scripts/helper.sh` around lines 166 - 170, Update the stdout-draining loop around read in the helper script to explicitly preserve the current behavior of discarding a final partial frame that lacks a trailing newline, matching run.py’s complete_lines handling. Add a concise comment documenting this intentional discard so future changes do not append truncated frames to frames_log.tests/fake_opencode.py (1)
22-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the decode so one malformed line does not kill the fake process.
json.loadsraises on any non-JSON line. The process then exits, the stdout FIFO reaches EOF, and the controller shuts down. A test that sends a bad frame would fail with a confusing transport error instead of a clear assertion.♻️ Proposed hardening
for line in sys.stdin: - incoming = json.loads(line) + if not line.strip(): + continue + try: + incoming = json.loads(line) + except json.JSONDecodeError: + continue if "id" not in incoming: continue print(json.dumps(response(incoming), separators=(",", ":")), flush=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fake_opencode.py` around lines 22 - 26, Update the stdin-processing loop in the fake process around json.loads to catch malformed JSON lines and continue reading subsequent input instead of terminating. Preserve the existing behavior for valid messages, including skipping entries without an "id" and emitting response(incoming) as JSON.docs/CONTRIBUTING.md (1)
44-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the local Markdown check aligned with CI.
The local command does not include
.github/ISSUE_TEMPLATE/*.md, while CI checks those files at Lines 32-36. A local pass can therefore miss a CI failure. Add the same glob here.Proposed fix
skills/opencode-acp-control/references/*.md \ - skills/opencode-acp-control/assets/*.md + skills/opencode-acp-control/assets/*.md \ + .github/ISSUE_TEMPLATE/*.md🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/CONTRIBUTING.md` around lines 44 - 48, Update the Markdown lint command in the contributing documentation to include the .github/ISSUE_TEMPLATE/*.md glob, matching the Markdown files checked by CI while preserving the existing lint targets..github/workflows/ci.yml (1)
85-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the declared Python minimum in CI.
README.mddeclares Python 3.9+, but this job runs only on Python 3.11. The new compilation and pytest steps do not detect regressions at the documented minimum. Add a Python 3.9/3.11 matrix, or lower the documented minimum.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 85 - 97, Update the CI workflow’s Python test job to run the shell/compile checks and pytest steps across a Python 3.9 and 3.11 matrix, preserving the existing commands and Python 3.9+ minimum declared in README.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@_meta.json`:
- Around line 4-5: Update the publishedAt field in the release metadata for
version 0.4.0 to the August 10, 2026 release timestamp, leaving the version
value unchanged and consistent with the changelog and release-date requirements.
In `@README.md`:
- Around line 77-80: Update the README smoke-test shell snippet around run.py
start so it waits for controller readiness before invoking send. Poll the
runtime status or wait for the READY output, then preserve the existing send
flow once the runtime reports ready.
In `@skills/opencode-acp-control/references/api.md`:
- Around line 68-73: The permission request documentation incorrectly assigns
option selection to the agent. Update the “Permission request” text to state
that the client returns the user-selected optionId, while preserving the
guidance that responses must use offered option IDs rather than legacy reply
values.
In `@skills/opencode-acp-control/scripts/helper.sh`:
- Around line 113-122: Update the shutdown logic around the opencode_pid cleanup
block to enforce bounded termination: send SIGTERM first, wait only through a
finite grace period while checking process liveness, then send SIGKILL if the
process remains alive before performing the existing non-blocking-safe wait.
Ensure the cleanup cannot block indefinitely when OpenCode ignores or traps
SIGTERM.
In `@skills/opencode-acp-control/scripts/run.py`:
- Around line 43-60: Update pid_alive to parse /proc/<pid>/stat by locating the
final closing parenthesis of the comm field, then read the state field from the
text after it instead of using split()[2]. Preserve the existing zombie
detection and fallback behavior when the stat file cannot be read or parsed.
In `@skills/opencode-acp-control/SKILL.md`:
- Around line 131-140: Update skills/opencode-acp-control/SKILL.md lines 131-140
to require explicit user confirmation before removing the default runtime
directory, preserving sensitive logs unless deletion is approved. Update
skills/opencode-acp-control/SKILL.md lines 269-280 so recursive removal occurs
only when the stop result is exactly stopped:0; retain abnormal stopped:* states
for diagnosis. Apply the same exact-success-state and confirmation requirements
to skills/opencode-acp-control/references/guidelines.md lines 41-43.
- Around line 220-235: Update the “Poll complete stdout frames with a line
cursor” instructions so agents do not advance NEXT_LINE past an incomplete final
NDJSON record. Require parsing each emitted line as complete JSON-RPC before
saving the cursor, and ensure incomplete EOF data is retained or explicitly
reported for retry rather than skipped on the next poll.
In `@tests/test_transport.py`:
- Around line 23-35: Update run_cli to assert process.returncode immediately
after subprocess.run when check is true, before selecting or parsing output.
Preserve the existing stream selection and payload parsing for successful or
explicitly unchecked executions, while ensuring failed checked executions report
process.stderr instead of raising IndexError.
- Around line 167-170: Update the teardown around process.wait in the test’s
finally block to catch TimeoutExpired, terminate or kill the foreground
controller, and wait for it to exit. Preserve the existing runtime stop attempt
and ensure cleanup also handles the associated fake OpenCode process when the
controller survives stop.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 85-97: Update the CI workflow’s Python test job to run the
shell/compile checks and pytest steps across a Python 3.9 and 3.11 matrix,
preserving the existing commands and Python 3.9+ minimum declared in README.md.
In `@docs/CONTRIBUTING.md`:
- Around line 44-48: Update the Markdown lint command in the contributing
documentation to include the .github/ISSUE_TEMPLATE/*.md glob, matching the
Markdown files checked by CI while preserving the existing lint targets.
In `@skills/opencode-acp-control/scripts/helper.sh`:
- Around line 166-170: Update the stdout-draining loop around read in the helper
script to explicitly preserve the current behavior of discarding a final partial
frame that lacks a trailing newline, matching run.py’s complete_lines handling.
Add a concise comment documenting this intentional discard so future changes do
not append truncated frames to frames_log.
In `@skills/opencode-acp-control/scripts/run.py`:
- Around line 337-346: Remove the redundant process_belongs_to_runtime check
inside command_stop after controller_owned is established; retain the existing
ownership calculation and os.kill flow, including the ProcessLookupError race
handling.
In `@tests/fake_opencode.py`:
- Around line 22-26: Update the stdin-processing loop in the fake process around
json.loads to catch malformed JSON lines and continue reading subsequent input
instead of terminating. Preserve the existing behavior for valid messages,
including skipping entries without an "id" and emitting response(incoming) as
JSON.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d85280e3-5b9e-4b5b-a95b-4fda456ee575
📒 Files selected for processing (21)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/workflows/ci.ymlCONTRIBUTING.mdREADME.mdSKILL.md_meta.jsondocs/CHANGELOG.mddocs/CODE_OF_CONDUCT.mddocs/CONTRIBUTING.mdexamples/acp_demo.pyskills/opencode-acp-control/SKILL.mdskills/opencode-acp-control/assets/example.jsonskills/opencode-acp-control/assets/template.mdskills/opencode-acp-control/references/api.mdskills/opencode-acp-control/references/guidelines.mdskills/opencode-acp-control/scripts/helper.shskills/opencode-acp-control/scripts/run.pytests/fake_opencode.pytests/test_acp_demo.pytests/test_transport.py
💤 Files with no reviewable changes (4)
- SKILL.md
- examples/acp_demo.py
- tests/test_acp_demo.py
- CONTRIBUTING.md
| "version": "0.4.0", | ||
| "publishedAt": 1770640762925 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Update publishedAt for version 0.4.0.
version now says 0.4.0, but publishedAt remains a February 9, 2026 timestamp. The changelog records this release on August 10, 2026, and docs/CONTRIBUTING.md requires updating both fields on release. Update publishedAt before publishing version 0.4.0.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@_meta.json` around lines 4 - 5, Update the publishedAt field in the release
metadata for version 0.4.0 to the August 10, 2026 release timestamp, leaving the
version value unchanged and consistent with the changelog and release-date
requirements.
| runtime_dir="/tmp/opencode-acp.example.$$" | ||
| python3 skills/opencode-acp-control/scripts/run.py start \ | ||
| --foreground --cwd "$PWD" --runtime-dir "$runtime_dir" & | ||
| ``` |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make the smoke test wait for controller readiness.
If a user runs this block without a manual pause, send can execute while the runtime is not yet ready. run.py rejects sends until the runtime reaches that state. Poll status or wait for the READY line in the shell snippet before invoking send.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 77 - 80, Update the README smoke-test shell snippet
around run.py start so it waits for controller readiness before invoking send.
Poll the runtime status or wait for the READY output, then preserve the existing
send flow once the runtime reports ready.
| ### Permission request | ||
|
|
||
| The agent chooses the offered option IDs. Do not invent legacy `reply` values. | ||
|
|
||
| ```json | ||
| {"jsonrpc":"2.0","id":5,"method":"session/request_permission","params":{"sessionId":"sess_abc123","toolCall":{"toolCallId":"call_1"},"options":[{"optionId":"allow-once","name":"Allow once","kind":"allow_once"},{"optionId":"reject-once","name":"Reject","kind":"reject_once"}]}} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg 'skills/opencode-acp-control/references/api\.md|SKILL\.md|skill.*\.md|permission|request_permission' || true
echo "== api.md excerpt =="
if [ -f skills/opencode-acp-control/references/api.md ]; then
nl -ba skills/opencode-acp-control/references/api.md | sed -n '55,85p'
fi
echo "== related sections =="
rg -n "request_permission|allow-once|reject-once|optionId|session/request_permission|permission" skills/opencode-acp-control -S || trueRepository: berriosb/Opencode-Acp-Control
Length of output: 429
🌐 Web query:
Agent Client Protocol v1 tool-calls session/request_permission client optionId response
💡 Result:
In the Agent Client Protocol (ACP), the session/request_permission method is used by an Agent to request user authorization before executing sensitive operations, such as tool calls [1][2]. When an Agent initiates this request, it provides a list of available options that the user can choose from [1][3]. Each PermissionOption object includes: - optionId: A unique identifier for the specific choice (e.g., "allow-once", "reject-once") [1][2][4]. - name: A human-readable label to display to the user [1][2][4]. - kind: A hint describing the nature of the option, which can be one of: allow_once, allow_always, reject_once, or reject_always [1][2][4]. The Client responds to the session/request_permission method with a RequestPermissionResponse [4]. This response indicates the outcome of the user's decision [4]: - If the user selects an option, the response contains outcome: "selected" along with the optionId of the chosen option [5][4]. - If the request is cancelled (for example, if the prompt turn ends before a selection is made), the response returns outcome: "cancelled" [1][6][4]. Clients are required to respond to all pending session/request_permission requests [7][6]. If a prompt turn is cancelled, the Client must respond to any outstanding permission requests with the "cancelled" outcome [1][6].
Citations:
- 1: https://agentclientprotocol.com/protocol/v1/tool-calls
- 2: https://github.com/zed-industries/agent-client-protocol/blob/4f589532/docs/protocol/tool-calls.mdx
- 3: https://agentclientprotocol.github.io/typescript-sdk/types/RequestPermissionRequest.html
- 4: https://cdn.jsdelivr.net/npm/@zed-industries/agent-client-protocol@0.4.5/dist/schema.d.ts
- 5: https://agentclientprotocol.com/protocol/v2/migration
- 6: https://github.com/zed-industries/agent-client-protocol/blob/4f589532/docs/protocol/schema.mdx
- 7: https://agentclientprotocol.com/protocol/v2/prompt-lifecycle
🌐 Web query:
site:agentclientprotocol.com protocol/v.1/tool-calls session/request_permission Client optionId
💡 Result:
In the Agent Client Protocol (ACP), the session/request_permission method is used by an agent to obtain user authorization before executing sensitive operations, such as tool calls [1][2][3]. The optionId is a unique identifier within the options array of a permission request, which defines the specific choices available to the user (e.g., allow-once, reject-once) [1][3]. When an agent triggers a session/request_permission call, it presents these options to the client, which then renders them for the user [2][3]. The user's selection, associated with the corresponding optionId, allows the agent to proceed or denies the request based on the chosen action [3]. In ACP v2, the protocol evolved to include a more flexible subject structure, but the mechanism of using optionId within a list of permission options remains a core part of handling these authorization workflows [3][4]. The optionId effectively maps a human-readable choice (like "Allow once") to a machine-interpretable action [3].
Citations:
- 1: https://agentclientprotocol.com/protocol/v1/tool-calls
- 2: https://agentclientprotocol.com/protocol/v1/schema
- 3: https://agentclientprotocol.com/protocol/v2/tool-calls
- 4: https://agentclientprotocol.com/rfds/v2/permission-requests
🌐 Web query:
"session/request_permission" "optionId" Agent Client Protocol
💡 Result:
The session/request_permission method is a core component of the Agent-Client Protocol (ACP), used by an Agent to obtain authorization from a Client before executing sensitive operations, such as tool calls [1][2][3]. When an Agent needs permission, it sends a session/request_permission message [1][4]. The structure of this request has evolved between protocol versions [5]: In v1, the request typically includes a sessionId, a toolCall object detailing the operation, and an options array [2][4]. In v2, the request is more structured, using a subject (e.g., of type tool_call or command) to describe the operation, along with a title and optional description to provide context to the user [1][5]. The options array contains objects that define the choices available to the user [1][2]. Each option includes: optionId: A unique string identifier for the specific choice (e.g., allow-once, reject-once) [2][3]. name: A human-readable label displayed to the user [2][3]. kind: A hint specifying the nature of the permission (allow_once, allow_always, reject_once, or reject_always) [2][5]. The Client responds to this request with an outcome object, which indicates whether the user selected an option or if the request was cancelled [2][5][3]. If an option was selected, the response must include the specific optionId chosen by the user [2][3]. An Agent must not interpret an unknown outcome as approval [5]. Clients may also choose to automatically handle these requests based on existing user preferences, effectively bypassing the need for manual user intervention for certain operations [2][4].
Citations:
- 1: https://agentclientprotocol.com/protocol/v2/tool-calls
- 2: https://agentclientprotocol.com/protocol/v1/tool-calls
- 3: https://cdn.jsdelivr.net/npm/@zed-industries/agent-client-protocol@0.4.5/dist/schema.d.ts
- 4: https://github.com/zed-industries/agent-client-protocol/blob/4f589532/docs/protocol/tool-calls.mdx
- 5: https://agentclientprotocol.com/protocol/v2/migration
🏁 Script executed:
#!/bin/bash
set -u
echo "== all files =="
git ls-files
echo "== api.md excerpt around permission =="
sed -n '58,76p' skills/opencode-acp-control/references/api.md
echo "== SKILL excerpt around permissions =="
sed -n '1,220p' skills/opencode-acp-control/SKILL.md | grep -nA8 -B3 -E "request_permission|optionId|permission|allow-once|reject-once" || true
echo "== all permission references =="
rg -n "request_permission|selected|optionId|Allow once|Reject|permission" skills/opencode-acp-control -S || trueRepository: berriosb/Opencode-Acp-Control
Length of output: 2862
🌐 Web query:
"session/request_permission" "outcome" "selected" "optionId" "Agent Client Protocol" "v1"
💡 Result:
In the Agent Client Protocol (ACP) v1, the session/request_permission method is a JSON-RPC request sent from an agent to a client to obtain user approval for a specific operation, such as a tool call [1][2]. When the agent sends this request, it provides a set of permission options, each containing an optionId [1][3]. The client, upon receiving the request, is expected to present these options to the user and respond with the user's decision [1][3]. The response structure for a successful user selection is defined as follows [1][4]: { "jsonrpc": "2.0", "id": <request_id>, "result": { "outcome": { "outcome": "selected", "optionId": "<selected_option_id>" } } } If the user dismisses the prompt or the request is otherwise interrupted, the client must respond with an outcome of "cancelled" [1][2]: { "jsonrpc": "2.0", "id": <request_id>, "result": { "outcome": { "outcome": "cancelled" } } } Key details: - Outcome "selected": Indicates the user chose one of the provided options, and the specific choice is identified by the optionId [1][2]. - Outcome "cancelled": Indicates the request was cancelled (e.g., dismissed by the user or due to a prompt turn cancellation) [1]. - Permission Options: Each option consists of an optionId (a unique identifier), a name (human-readable label), and a kind (e.g., allow_once, allow_always, reject_once, reject_always) [1][4]. This interaction is a blocking round-trip; the agent typically waits for this client response before proceeding with the requested operation [2][3].
Citations:
- 1: https://agentclientprotocol.com/protocol/v1/tool-calls
- 2: https://github.com/zeroclaw-labs/zeroclaw/blob/master/docs/book/src/channels/acp.md
- 3: https://agentao.cn/en/part-3/2-agentao-as-server
- 4: https://github.com/esengine/DeepSeek-Reasonix/blob/9e56c327/internal/acp/protocol.go
Assign permission selection to the client.
ACP session/request_permission is sent by the agent, but the client returns the user-selected optionId. Change this line to avoid implying the agent can choose approval options.
Proposed wording
-The agent chooses the offered option IDs. The agent sends the request and the client returns the selected option ID.
+The client returns the option ID selected by the user or an existing user policy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/opencode-acp-control/references/api.md` around lines 68 - 73, The
permission request documentation incorrectly assigns option selection to the
agent. Update the “Permission request” text to state that the client returns the
user-selected optionId, while preserving the guidance that responses must use
offered option IDs rather than legacy reply values.
Source: MCP tools
| if [[ -n "$opencode_pid" ]] && kill -0 "$opencode_pid" 2>/dev/null; then | ||
| for attempt in {1..20}; do | ||
| kill -0 "$opencode_pid" 2>/dev/null || break | ||
| sleep 0.05 | ||
| done | ||
| if kill -0 "$opencode_pid" 2>/dev/null; then | ||
| kill -TERM "$opencode_pid" 2>/dev/null || true | ||
| fi | ||
| wait "$opencode_pid" 2>/dev/null || true | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a bounded wait and a SIGKILL fallback after SIGTERM.
The loop at Line 114 only observes the child. It never escalates during that second. After the single kill -TERM, wait "$opencode_pid" blocks with no upper bound. If OpenCode ignores or traps SIGTERM, the controller never exits. run.py stop then reports controller did not stop before timeout, and the runtime directory keeps live FIFO nodes.
🛠️ Proposed fix: escalate to SIGKILL after a grace period
if [[ -n "$opencode_pid" ]] && kill -0 "$opencode_pid" 2>/dev/null; then
for attempt in {1..20}; do
kill -0 "$opencode_pid" 2>/dev/null || break
sleep 0.05
done
if kill -0 "$opencode_pid" 2>/dev/null; then
kill -TERM "$opencode_pid" 2>/dev/null || true
+ for attempt in {1..40}; do
+ kill -0 "$opencode_pid" 2>/dev/null || break
+ sleep 0.05
+ done
+ kill -0 "$opencode_pid" 2>/dev/null && kill -KILL "$opencode_pid" 2>/dev/null || true
fi
wait "$opencode_pid" 2>/dev/null || true
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ -n "$opencode_pid" ]] && kill -0 "$opencode_pid" 2>/dev/null; then | |
| for attempt in {1..20}; do | |
| kill -0 "$opencode_pid" 2>/dev/null || break | |
| sleep 0.05 | |
| done | |
| if kill -0 "$opencode_pid" 2>/dev/null; then | |
| kill -TERM "$opencode_pid" 2>/dev/null || true | |
| fi | |
| wait "$opencode_pid" 2>/dev/null || true | |
| fi | |
| if [[ -n "$opencode_pid" ]] && kill -0 "$opencode_pid" 2>/dev/null; then | |
| for attempt in {1..20}; do | |
| kill -0 "$opencode_pid" 2>/dev/null || break | |
| sleep 0.05 | |
| done | |
| if kill -0 "$opencode_pid" 2>/dev/null; then | |
| kill -TERM "$opencode_pid" 2>/dev/null || true | |
| for attempt in {1..40}; do | |
| kill -0 "$opencode_pid" 2>/dev/null || break | |
| sleep 0.05 | |
| done | |
| kill -0 "$opencode_pid" 2>/dev/null && kill -KILL "$opencode_pid" 2>/dev/null || true | |
| fi | |
| wait "$opencode_pid" 2>/dev/null || true | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 114-114: attempt appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/opencode-acp-control/scripts/helper.sh` around lines 113 - 122, Update
the shutdown logic around the opencode_pid cleanup block to enforce bounded
termination: send SIGTERM first, wait only through a finite grace period while
checking process liveness, then send SIGKILL if the process remains alive before
performing the existing non-blocking-safe wait. Ensure the cleanup cannot block
indefinitely when OpenCode ignores or traps SIGTERM.
| def pid_alive(pid: Optional[int]) -> bool: | ||
| if pid is None: | ||
| return False | ||
| proc_stat = Path("/proc") / str(pid) / "stat" | ||
| if proc_stat.exists(): | ||
| try: | ||
| # A zombie has exited even though kill(pid, 0) still succeeds. | ||
| if proc_stat.read_text(encoding="utf-8").split()[2] == "Z": | ||
| return False | ||
| except (OSError, IndexError): | ||
| pass | ||
| try: | ||
| os.kill(pid, 0) | ||
| except ProcessLookupError: | ||
| return False | ||
| except PermissionError: | ||
| return True | ||
| return True |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse /proc/<pid>/stat after the final ) to read the state field.
The comm field of /proc/<pid>/stat is wrapped in parentheses and can contain spaces and parentheses. split()[2] therefore does not always return the state character. For a process named my agent, index 2 returns a part of the name, so the zombie check silently fails, and pid_alive reports a zombie controller as alive.
🐛 Proposed fix
proc_stat = Path("/proc") / str(pid) / "stat"
if proc_stat.exists():
try:
# A zombie has exited even though kill(pid, 0) still succeeds.
- if proc_stat.read_text(encoding="utf-8").split()[2] == "Z":
+ raw = proc_stat.read_text(encoding="utf-8")
+ after_comm = raw[raw.rindex(")") + 2 :]
+ if after_comm.split()[0] == "Z":
return False
- except (OSError, IndexError):
+ except (OSError, IndexError, ValueError):
pass📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def pid_alive(pid: Optional[int]) -> bool: | |
| if pid is None: | |
| return False | |
| proc_stat = Path("/proc") / str(pid) / "stat" | |
| if proc_stat.exists(): | |
| try: | |
| # A zombie has exited even though kill(pid, 0) still succeeds. | |
| if proc_stat.read_text(encoding="utf-8").split()[2] == "Z": | |
| return False | |
| except (OSError, IndexError): | |
| pass | |
| try: | |
| os.kill(pid, 0) | |
| except ProcessLookupError: | |
| return False | |
| except PermissionError: | |
| return True | |
| return True | |
| def pid_alive(pid: Optional[int]) -> bool: | |
| if pid is None: | |
| return False | |
| proc_stat = Path("/proc") / str(pid) / "stat" | |
| if proc_stat.exists(): | |
| try: | |
| # A zombie has exited even though kill(pid, 0) still succeeds. | |
| raw = proc_stat.read_text(encoding="utf-8") | |
| after_comm = raw[raw.rindex(")") + 2 :] | |
| if after_comm.split()[0] == "Z": | |
| return False | |
| except (OSError, IndexError, ValueError): | |
| pass | |
| try: | |
| os.kill(pid, 0) | |
| except ProcessLookupError: | |
| return False | |
| except PermissionError: | |
| return True | |
| return True |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/opencode-acp-control/scripts/run.py` around lines 43 - 60, Update
pid_alive to parse /proc/<pid>/stat by locating the final closing parenthesis of
the comm field, then read the state field from the text after it instead of
using split()[2]. Preserve the existing zombie detection and fallback behavior
when the stat file cannot be read or parsed.
| Always stop the transport when finished: | ||
|
|
||
| ```bash | ||
| python3 <skill-dir>/scripts/run.py stop --runtime-dir <runtime-dir> | ||
| ``` | ||
|
|
||
| This signals the exact controller, closes permanent FD 3, lets OpenCode exit on | ||
| stdin EOF, escalates to `TERM` only if needed, removes the FIFO nodes, and then | ||
| removes the owned runtime directory. Use `--keep-runtime` only when logs are | ||
| needed; later run `clean` on the stopped runtime. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Gate all runtime deletion on a graceful stop and explicit approval.
skills/opencode-acp-control/SKILL.md#L131-L140: require confirmation before default runtime removal and retain sensitive logs unless the user approves deletion.skills/opencode-acp-control/SKILL.md#L269-L280: requirestopped:0, not anystopped:*value, beforerm -r; retain abnormal states for diagnosis.skills/opencode-acp-control/references/guidelines.md#L41-L43: document the same success-state and confirmation requirements.
🧰 Tools
🪛 SkillSpector (2.5.1)
[warning] 285: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
📍 Affects 2 files
skills/opencode-acp-control/SKILL.md#L131-L140(this comment)skills/opencode-acp-control/SKILL.md#L269-L280skills/opencode-acp-control/references/guidelines.md#L41-L43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/opencode-acp-control/SKILL.md` around lines 131 - 140, Update
skills/opencode-acp-control/SKILL.md lines 131-140 to require explicit user
confirmation before removing the default runtime directory, preserving sensitive
logs unless deletion is approved. Update skills/opencode-acp-control/SKILL.md
lines 269-280 so recursive removal occurs only when the stop result is exactly
stopped:0; retain abnormal stopped:* states for diagnosis. Apply the same
exact-success-state and confirmation requirements to
skills/opencode-acp-control/references/guidelines.md lines 41-43.
Source: Linters/SAST tools
| def run_cli(*args: str, check: bool = True) -> tuple[subprocess.CompletedProcess[str], dict]: | ||
| process = subprocess.run( | ||
| [sys.executable, str(RUNNER), *args], | ||
| cwd=REPO_ROOT, | ||
| text=True, | ||
| capture_output=True, | ||
| check=False, | ||
| ) | ||
| output = process.stdout if process.returncode == 0 else process.stderr | ||
| payload = json.loads(output.strip().splitlines()[-1]) | ||
| if check: | ||
| assert process.returncode == 0, process.stderr | ||
| return process, payload |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the return code before you parse the output.
Line 32 indexes splitlines()[-1]. If the selected stream is empty, this raises IndexError and hides the real failure. A crash of run.py with an empty stderr produces an unrelated traceback instead of the message at Line 34.
🐛 Proposed fix
output = process.stdout if process.returncode == 0 else process.stderr
- payload = json.loads(output.strip().splitlines()[-1])
if check:
assert process.returncode == 0, process.stderr
+ lines = output.strip().splitlines()
+ assert lines, f"no output from run.py; stdout={process.stdout!r} stderr={process.stderr!r}"
+ payload = json.loads(lines[-1])
return process, payload📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def run_cli(*args: str, check: bool = True) -> tuple[subprocess.CompletedProcess[str], dict]: | |
| process = subprocess.run( | |
| [sys.executable, str(RUNNER), *args], | |
| cwd=REPO_ROOT, | |
| text=True, | |
| capture_output=True, | |
| check=False, | |
| ) | |
| output = process.stdout if process.returncode == 0 else process.stderr | |
| payload = json.loads(output.strip().splitlines()[-1]) | |
| if check: | |
| assert process.returncode == 0, process.stderr | |
| return process, payload | |
| def run_cli(*args: str, check: bool = True) -> tuple[subprocess.CompletedProcess[str], dict]: | |
| process = subprocess.run( | |
| [sys.executable, str(RUNNER), *args], | |
| cwd=REPO_ROOT, | |
| text=True, | |
| capture_output=True, | |
| check=False, | |
| ) | |
| output = process.stdout if process.returncode == 0 else process.stderr | |
| if check: | |
| assert process.returncode == 0, process.stderr | |
| lines = output.strip().splitlines() | |
| assert lines, f"no output from run.py; stdout={process.stdout!r} stderr={process.stderr!r}" | |
| payload = json.loads(lines[-1]) | |
| return process, payload |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 23-29: Command coming from incoming request
Context: subprocess.run(
[sys.executable, str(RUNNER), *args],
cwd=REPO_ROOT,
text=True,
capture_output=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_transport.py` around lines 23 - 35, Update run_cli to assert
process.returncode immediately after subprocess.run when check is true, before
selecting or parsing output. Preserve the existing stream selection and payload
parsing for successful or explicitly unchecked executions, while ensuring failed
checked executions report process.stderr instead of raising IndexError.
| finally: | ||
| if runtime.exists(): | ||
| run_cli("stop", "--runtime-dir", str(runtime), check=False) | ||
| process.wait(timeout=2) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Terminate the foreground controller if it does not exit.
process.wait(timeout=2) raises TimeoutExpired when the controller survives stop. The teardown then fails and leaves an orphaned controller plus a live fake OpenCode process for the rest of the session.
🛠️ Proposed fix
finally:
if runtime.exists():
run_cli("stop", "--runtime-dir", str(runtime), check=False)
- process.wait(timeout=2)
+ try:
+ process.wait(timeout=2)
+ except subprocess.TimeoutExpired:
+ process.kill()
+ process.wait(timeout=2)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| finally: | |
| if runtime.exists(): | |
| run_cli("stop", "--runtime-dir", str(runtime), check=False) | |
| process.wait(timeout=2) | |
| finally: | |
| if runtime.exists(): | |
| run_cli("stop", "--runtime-dir", str(runtime), check=False) | |
| try: | |
| process.wait(timeout=2) | |
| except subprocess.TimeoutExpired: | |
| process.kill() | |
| process.wait(timeout=2) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_transport.py` around lines 167 - 170, Update the teardown around
process.wait in the test’s finally block to catch TimeoutExpired, terminate or
kill the foreground controller, and wait for it to exit. Preserve the existing
runtime stop attempt and ensure cleanup also handles the associated fake
OpenCode process when the controller survives stop.
Poll every 20 seconds and treat ten minutes as a user-confirmation threshold. Continue waiting on empty polls, unconfirmed cancellation, and isolated malformed stdout frames.
Replace the non-portable background stdin assumption with a permanent-FD FIFO controller. Reorganize the installable skill bundle, document the standalone no-Python workflow, align ACP v1 examples, and add lifecycle tests and CI coverage.
Summary by CodeRabbit
New Features
Documentation
Tests