fix(agents): exclude openai-codex from WebSocket transport + hooks soft/hard blockMode#56340
fix(agents): exclude openai-codex from WebSocket transport + hooks soft/hard blockMode#56340marioperezpereira wants to merge 14 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes a regression where all Key changes:
Confidence Score: 5/5Safe to merge — the core logic is correct, backward-compatible, and covered by tests; remaining findings are P2 style/doc improvements. All open findings are P2: a redundant
|
| Filename | Overview |
|---|---|
| src/plugins/hooks.ts | Refactors mergeResults and shouldStop for before_tool_call to support soft vs hard block semantics. Hard blocks short-circuit as before; soft blocks allow lower-priority handlers to clear them via { block: false }. Logic is correct and backward-compatible. |
| src/plugins/types.ts | Adds `blockMode?: "hard" |
| src/plugins/hooks.security.test.ts | Adds two new test cases for soft-block clearing and hard-default behaviour. The soft-block test is clean. The hard-default test provides redundant result fields alongside handler mocks — per addStaticTestHooks, the result fields are ignored when handler is present. |
| docs/automation/hooks.md | Documents blockMode field and updates block-vs-requireApproval precedence description. Accurate and complete relative to the implementation. |
| docs/concepts/agent-loop.md | Updates hook decision rules for soft/hard blocks but removes the explicit statement that { block: false } is a no-op against hard blocks, leaving a documentation gap. |
| docs/plugins/building-plugins.md | Consistent update to hook guard semantics section; accurately reflects new soft/hard block behaviour. |
| docs/plugins/sdk-overview.md | Removes explicit mention that { block: false } is a no-op (same gap as agent-loop.md); otherwise consistent with implementation. |
| docs/tools/plugin.md | Same no-op documentation gap as the other doc files; logic otherwise correct. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/plugins/hooks.security.test.ts
Line: 127-144
Comment:
**Redundant `result` fields when `handler` is provided**
In both hook entries of this test, a `handler` mock and a `result` object are specified simultaneously. Per `addStaticTestHooks` in `hooks.test-helpers.ts` (line 113):
```typescript
handler: (handler ?? (() => result)) as PluginHookRegistration["handler"],
```
When `handler` is provided, `result` is completely ignored and the mock function is used directly. This means the two `result` fields here are dead code — they have no effect on what the handler actually returns. The test still passes correctly (because the mocks return the same values as the `result` fields), but the extra `result` entries add noise and could mislead a future maintainer into thinking they influence hook behaviour.
Consider removing the redundant `result` fields for clarity:
```suggestion
it("treats omitted blockMode as hard and remains terminal", async () => {
const high = vi.fn().mockReturnValue({ block: true, blockReason: "hard-default" });
const low = vi.fn().mockReturnValue({ block: false });
const result = await runBeforeToolCallWithHooks(registry, [
{
pluginId: "high",
priority: 100,
handler: high,
},
{ pluginId: "low", priority: 10, handler: low },
]);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: docs/concepts/agent-loop.md
Line: 94-102
Comment:
**`block: false` no-op behaviour for hard blocks no longer documented**
The previous bullet —
> `before_tool_call`: `{ block: false }` is a no-op and does not clear a prior block.
— has been removed from this file, and from `docs/plugins/sdk-overview.md` and `docs/tools/plugin.md` as well. The new phrasing ("soft blocks *may* be cleared …") implies that hard blocks cannot be cleared, but it no longer states this explicitly.
A plugin author who sees that `{ block: false }` can clear a soft block may reasonably wonder: "Can I also use `{ block: false }` to override a hard block from a higher-priority plugin?" Without a direct answer in the docs they would need to read the source to be sure.
Consider adding a clarifying bullet, e.g.:
```
- `before_tool_call`: `{ block: false }` is a no-op against a `"hard"` block and does not clear it.
```
The same gap exists in `docs/plugins/sdk-overview.md` and `docs/tools/plugin.md`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "tests: align hard block default expectat..." | Re-trigger Greptile
There was a problem hiding this comment.
💡 Codex Review
https://github.com/openclaw/openclaw/blob/22adbcbc4e5d1a62c866909313cd9a7c8850f205/openclaw-2026.3.27.tgz#L1
Remove bundled release archives from repository history
This commit adds openclaw-2026.3.27.tgz (and a matching -fixed tarball) at repo root even though *.tgz is gitignored and these archives are not referenced by source or tests (repo-wide search). Keeping large packed artifacts in git history adds tens of MB of permanent bloat, slowing clone/fetch and CI cache churn for every developer without improving runtime behavior, so these files should be dropped from the change.
ℹ️ 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".
4429cf5 to
01fa271
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01fa271c09
ℹ️ 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: ab77e10dc4
ℹ️ 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: 50b3ff457b
ℹ️ 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".
…test fields
- Remove redundant result fields in hooks.security.test.ts (they're ignored when handler is present)
- Update addStaticTestHooks to allow optional result when handler is provided
- Add explicit note that { block: false } is a no-op against hard blocks in 3 doc files
- This clarifies the existing implementation and prevents confusion for future maintainers
The WebSocket transport hardcodes wss://api.openai.com/v1/responses, but Codex models use chatgpt.com/backend-api which does not expose a compatible WebSocket endpoint. Routing Codex through the WS path caused tool calls to silently fail (connection to wrong host). Restrict shouldUseOpenAIWebSocketTransport to vanilla OpenAI only so Codex falls back to the HTTP stream path where it works correctly. Fixes regression introduced in d72cc7a (openclaw#53702).
50b3ff4 to
8fb459d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd1ed79b69
ℹ️ 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: 308075a617
ℹ️ 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".
308075a to
605095e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 605095eab9
ℹ️ 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: 56b24ef39f
ℹ️ 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".
|
Ready for review 👌🏻 |
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: found issues before merge. Summary Reproducibility: yes. Static inspection gives high-confidence reproduction paths: merge a soft block plus approval followed by lower-priority Next step before merge Security Review findings
Review detailsBest possible solution: Rebase onto current main, keep the newer endpoint-aware WebSocket guard, implement Do we have a high-confidence way to reproduce the issue? Yes. Static inspection gives high-confidence reproduction paths: merge a soft block plus approval followed by lower-priority Is this the best way to solve the issue? No, not as-is. The direction is plausible, but current main already has the better Codex transport guard and this branch needs the approval ownership, profile fallback, rebase, and changelog defects fixed before it is the narrow maintainable solution. Full review comments:
Overall correctness: patch is incorrect Security concerns:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 4655ee803d27. |
What
Fix Codex tool execution regression: The WebSocket transport (
d72cc7a3, PR fix: route codex responses over websocket and suppress gated core tool warnings #53702) hardcodeswss://api.openai.com/v1/responses, but Codex models usechatgpt.com/backend-apiwhich has no compatible WebSocket endpoint. Routing Codex through WS caused tool calls to silently degrade. Fix: restrictshouldUseOpenAIWebSocketTransportto vanilla OpenAI only so Codex falls back to the working HTTP stream path. Fixes regression introduced in d72cc7a (fix: route codex responses over websocket and suppress gated core tool warnings #53702).Add soft/hard
blockModetobefore_tool_callhooks: Non-security blocks were treated as unconditionally terminal, preventing lower-priority handlers from clearing them or routing through approval. AddsblockMode?: "hard" | "soft"toPluginHookBeforeToolCallResult(default"hard"for backward compat). Hard blocks short-circuit; soft blocks allow downstream handlers to clear with{ block: false }or route throughrequireApproval.Harden
before_tool_callconsumer for soft block + approval semantics:requireApprovalstill block by default.requireApprovalfrom the same plugin go through the approval flow.Fix live test env on Windows / non-bash shells:
loadProfileEnv()now tries multiple bash candidates (including Git Bash paths on Windows) and falls back to static.profileparsing. Profile loading is restricted to explicit live test runs to avoid bleeding into normal CI.Changes
attempt.thread-helpers.ts/attempt.spawn-workspace.websocket.test.ts: Excludeopenai-codexfrom WS transport selectionhooks.ts: Soft/hard block merge logic with block ownership trackingtypes.ts: AddblockModeandblockOwnerPluginIdfieldspi-tools.before-tool-call.ts: Honor soft block + approval semantics in the consumerhooks.security.test.ts,hooks.before-tool-call.test.ts,hooks.test-helpers.ts: Tests for soft/hard semantics and approval ownershippi-tools.before-tool-call.e2e.test.ts: E2E tests for hard block, soft block, soft block + approvaltest/test-env.ts/test/test-env.test.ts: Cross-platform profile env loading, fallback on empty shell loads, restrict to live runsautomation/hooks.md,concepts/agent-loop.md,plugins/building-plugins.md,plugins/sdk-overview.md,tools/plugin.md): Updated hook decision semantics, clarified{ block: false }no-op against hard blocksTesting
test-envtests pass (5 tests, including bash-unavailable fallback)AI Disclosure
AI-assisted (Claude + OpenAI Codex). Fully tested locally and in CI. All bot review conversations addressed.