Add synchronous ACP harness tool interception - #2372
Conversation
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR introduces a complex synchronous tool-policy feature across shared rollout infrastructure and multiple third-party harness adapters, including new credential and proxy paths. Its runtime and security impact is broad, with unresolved concerns involving Codex protocol handling, Pool credential exposure, and OpenClaw scope. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4edcd30858
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4edcd30 to
68941f5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68941f52ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
68941f5 to
e34d4e0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e34d4e0758
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e34d4e0 to
df227d6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df227d6ee3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
df227d6 to
cccba87
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cccba87422
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cccba87 to
4726f72
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4726f72bce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4726f72 to
a6d70fd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6d70fd828
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a6d70fd to
38186a5
Compare
bfc6b6d to
90bb79a
Compare
90bb79a to
f251c8a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4050c7f1b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a04144d22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
412a3f1 to
d22f55d
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
xeophon
changed the title
d22f55d to
f1dbb9a
Compare
3325ab5 to
fe0623c
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fe0623c. Configure here.
| }, | ||
| }, | ||
| } | ||
| ) |
There was a problem hiding this comment.
Codex terminate treated as wait
High Severity
session/terminate is handled with tool name wait and, on block, answered as wait/completed. Post interception only recognizes wait/completed, so terminate allow/block paths can bind the wrong call, skip the post hook, or fail the rollout’s prepared-result check.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fe0623c. Configure here.
| SUPPORTS_MCP = True | ||
| SUPPORTS_SKILLS = True | ||
| SUPPORTS_PRE_TOOL_INTERCEPTION = True | ||
| SUPPORTS_POST_TOOL_INTERCEPTION = True |
There was a problem hiding this comment.
OpenClaw interception still enabled
High Severity
OpenClaw still sets SUPPORTS_PRE_TOOL_INTERCEPTION and SUPPORTS_POST_TOOL_INTERCEPTION and ships configure_tool_interception plus the plugin. The PR scope reduction said native OpenClaw interception was removed and those flags stay false, so policy-backed OpenClaw rollouts still take an out-of-scope path.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit fe0623c. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe0623c78e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| credentialsPath = f"{stateDir}/{uuid.uuid4().hex}.credentials" | ||
| payload = json.dumps({"url": url, "secret": secret}).encode() |
There was a problem hiding this comment.
The fresh Pool adapter now stores the /tool URL and bearer in a file under the agent-visible .vf-pool workspace, and tool_hook.py reopens that same file for every hook, so it remains present throughout the rollout. When the model invokes Pool's shell or read tools, those same-UID processes can enumerate and read the file despite mode 0600, then forge policy requests or exfiltrate the capability; keep this credential in a trusted proxy or another channel unavailable to model-controlled tools.
Useful? React with 👍 / 👎.
| # stable shape so the next model request can be verified exactly. | ||
| if ( | ||
| content == "nonempty_text" | ||
| and (phase != "before" or candidate != message) |
There was a problem hiding this comment.
🟠 High v1/session.py:613
A successful no-output command is rejected in the after phase when content == "nonempty_text", even though no replacement was made, causing the Pool bridge to terminate the rollout. The phase != "before" clause makes the check run for unchanged post-tool results; apply the non-empty constraint only when candidate != message.
| and (phase != "before" or candidate != message) | |
| and candidate != message |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/session.py around line 613:
A successful no-output command is rejected in the `after` phase when `content == "nonempty_text"`, even though no replacement was made, causing the Pool bridge to terminate the rollout. The `phase != "before"` clause makes the check run for unchanged post-tool results; apply the non-empty constraint only when `candidate != message`.
1 participant
Overview
Add synchronous native tool interception to ACP-backed coding harnesses while retaining Bash as the built-in reference path. Task request policies can block proposed tools before execution and replace completed results before the harness advances to the next model turn.
Details
Note
High Risk
Touches security-critical policy enforcement: a new tool-only bearer, credential handoff into agent processes, and fail-closed native hooks that can allow, rewrite, or stop tool execution. Bugs here can leak secrets or skip policy on coding-agent tools.
Overview
Task request policies can now block tools before they run and rewrite results before the next model turn, on both in-process loops and ACP agents.
The interception slot gains a separate
tool_secret./toolis no longer reachable with the model bearer. Harnesses take(url, secret)instead of a URL, andSUPPORTS_TOOL_INTERCEPTIONsplits into pre/post flags. Session state keys approved results by assistant node + call id, serializes concurrent hooks, and raisesHarnessErrorif a required native boundary is skipped or the delivered result does not match.In-process harnesses (bash, browser, null, mini-swe) inject a shared HTTP client and receive the secret over stdin via
Runtime.run_with_input. ACP agents get per-product bridges: Claude SDK hooks via ACP permission metadata, Codex through a trusted Code Mode websocket proxy, Pi/OpenClaw/Pool via native plugins or command hooks. Generic ACP notifications stay observational. Node is bumped to 22.21 so env proxies work.Reviewed by Cursor Bugbot for commit 52b803e. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add synchronous tool interception across harnesses and session
SUPPORTS_TOOL_INTERCEPTIONwith separateSUPPORTS_PRE_TOOL_INTERCEPTIONandSUPPORTS_POST_TOOL_INTERCEPTIONflags on harnesses, allowing each phase to be advertised independently.ToolInterceptionClientandread_tool_secretin direct.py for synchronous HTTP policy calls, delivering a one-shot secret over stdin.fatal_errorbefore non-fatal errors.interception.base.Slottype alias changed from a 3-tuple to a 4-tuple with an addedtool_secret; all consumers must handle the new element.Macroscope summarized 52b803e.