LCO 1.7: Eva remote Codex control - #800
Conversation
evaOS review status: completedPR: #800 - LCO 1.7: Eva remote Codex control evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #800 Review URL: #800 (review) |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis release adds opt-in local Codex daemon transport over Unix sockets, opaque target routing with approval-gated delivery and interrupt support, expanded MCP tool contracts, audit lifecycle tracking, runtime configuration, validation coverage, and 1.7.0 release documentation. ChangesCodex daemon transport
Codex control flow
MCP and release surface
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Hermes
participant MCPServer
participant CodexControlRouter
participant CodexDaemon
Hermes->>MCPServer: lco_codex_control_route(hint)
MCPServer->>CodexControlRouter: route(hint)
CodexControlRouter->>CodexDaemon: read loaded threads
CodexDaemon-->>CodexControlRouter: thread projections
CodexControlRouter-->>Hermes: opaque target_ref
Hermes->>MCPServer: lco_codex_deliver(target_ref, message, dry_run)
MCPServer->>CodexControlRouter: deliver(target_ref, message)
CodexControlRouter->>CodexDaemon: revalidate target and deliver or steer
CodexDaemon-->>CodexControlRouter: control response
CodexControlRouter-->>Hermes: delivery result
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #800 - LCO 1.7: Eva remote Codex control
Head: 69e36e0aa749b68ab34eb878da382e13bad588f9 into main. Review event: COMMENT.
Provider: Unregistered provider id (builtin:zai-coding-plan, zcode (registry miss), model unknown).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
README.md |
modified | +20/-2 | Documentation | Low |
docs/BETA_RELEASE_RUNBOOK.md |
modified | +1/-1 | Documentation | Low |
docs/CLAIM_AUDIT.md |
modified | +10/-4 | Documentation | Low |
docs/HERMES_ADAPTER_BOUNDARY.md |
modified | +17/-2 | Documentation | Low |
docs/SETUP.md |
modified | +33/-4 | Documentation | Low |
docs/releases/CHANGELOG.md |
modified | +2/-1 | Documentation | Low |
docs/releases/RELEASE_NOTES_1.7.0.md |
added | +53/-0 | Documentation | Low |
package-lock.json |
modified | +37/-2 | Configuration | Low |
package.json |
modified | +5/-1 | Configuration | Low |
packages/adapters/src/codex-jsonrpc.ts |
modified | +158/-22 | Changed file | Moderate: validated P3 finding |
packages/adapters/src/index.ts |
modified | +15/-8 | Changed file | Low |
packages/adapters/src/policy.ts |
modified | +2/-0 | Changed file | Low |
packages/cli/src/hermes-smoke.ts |
modified | +2/-0 | Changed file | Low |
packages/mcp-server/src/codex-control-router.ts |
added | +344/-0 | Changed file | Moderate: validated P2 finding |
packages/mcp-server/src/codex-runtime-transport.ts |
added | +73/-0 | Changed file | Moderate: validated P3 finding |
packages/mcp-server/src/server-runtime.ts |
modified | +6/-18 | Changed file | Low |
packages/mcp-server/src/tools.ts |
modified | +62/-8 | Changed file | Low |
packages/openclaw-plugin/openclaw.plugin.json |
modified | +187/-23 | Changed file | Elevated: large change |
tests/bridge-safety-core.test.ts |
modified | +89/-0 | Test coverage | Low |
tests/codex-control-router.test.ts |
added | +382/-0 | Test coverage | Elevated: large change |
tests/codex-runtime-transport.test.ts |
added | +45/-0 | Test coverage | Low |
tests/control-and-mcp.test.ts |
modified | +17/-1 | Test coverage | Low |
tests/hermes-smoke.test.ts |
modified | +1/-1 | Test coverage | Low |
tests/openclaw-plugin-manifest.test.ts |
modified | +1/-1 | Test coverage | Low |
tests/qa-lab-tool-coverage.test.ts |
modified | +7/-7 | Test coverage | Low |
1 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 4 (P0: 0, P1: 0, P2: 1, P3: 3).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- Proof gap: 1
- Runtime correctness: 2
- Security boundary: 1
Validation and Proof
2 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
- required: CI/release smoke proof - CI, release, launchd, or package metadata changed. Proof: green GitHub check; release-status; coverage-audit; rollback note.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Call out evidence leakage, replay/collision risks, duplicate side effects, and brittle sanitizer logic.
Profile proof expectations: Look for focused sanitizer, signature, orchestration, or fixture proof.
Related Context
Related issues/PRs: #787, #797, #798, #799.
Suggested labels: docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: orchestration, security, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
![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: 69e36e0aa7
ℹ️ 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".
evaOS review status: completedPR: #800 - LCO 1.7: Eva remote Codex control evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #800 Review URL: #800 (review) |
coderabbitai
Bot
added
codex
There was a problem hiding this comment.
Walkthrough
PR: #800 - LCO 1.7: Eva remote Codex control
Head: a6f9de3c775af041580217d945a30f446de3dd7e into main. Review event: COMMENT.
Provider: Unregistered provider id (builtin:zai-coding-plan, zcode (registry miss), model unknown).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
README.md |
modified | +20/-2 | Documentation | Low |
docs/BETA_RELEASE_RUNBOOK.md |
modified | +1/-1 | Documentation | Low |
docs/CLAIM_AUDIT.md |
modified | +10/-4 | Documentation | Low |
docs/HERMES_ADAPTER_BOUNDARY.md |
modified | +17/-2 | Documentation | Low |
docs/SETUP.md |
modified | +33/-4 | Documentation | Moderate: validated P3 finding |
docs/releases/CHANGELOG.md |
modified | +2/-1 | Documentation | Low |
docs/releases/RELEASE_NOTES_1.7.0.md |
added | +53/-0 | Documentation | Low |
package-lock.json |
modified | +37/-2 | Configuration | Low |
package.json |
modified | +5/-1 | Configuration | Low |
packages/adapters/src/codex-jsonrpc.ts |
modified | +158/-22 | Changed file | Moderate: validated P2 finding |
packages/adapters/src/index.ts |
modified | +92/-23 | Changed file | Elevated: validated P1 finding |
packages/adapters/src/policy.ts |
modified | +2/-0 | Changed file | Low |
packages/cli/src/hermes-smoke.ts |
modified | +2/-0 | Changed file | Low |
packages/mcp-server/src/codex-control-router.ts |
added | +367/-0 | Changed file | Moderate: validated P2 finding |
packages/mcp-server/src/codex-runtime-transport.ts |
added | +73/-0 | Changed file | Low |
packages/mcp-server/src/server-runtime.ts |
modified | +6/-18 | Changed file | Low |
packages/mcp-server/src/tools.ts |
modified | +62/-8 | Changed file | Low |
packages/openclaw-plugin/openclaw.plugin.json |
modified | +187/-23 | Changed file | Elevated: large change |
tests/bridge-safety-core.test.ts |
modified | +89/-0 | Test coverage | Low |
tests/codex-control-router.test.ts |
added | +442/-0 | Test coverage | Elevated: large change |
tests/codex-runtime-transport.test.ts |
added | +45/-0 | Test coverage | Low |
tests/control-and-mcp.test.ts |
modified | +38/-5 | Test coverage | Low |
tests/hermes-smoke.test.ts |
modified | +1/-1 | Test coverage | Low |
tests/openclaw-plugin-manifest.test.ts |
modified | +1/-1 | Test coverage | Low |
tests/qa-lab-tool-coverage.test.ts |
modified | +7/-7 | Test coverage | Low |
1 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 7 (P0: 0, P1: 1, P2: 3, P3: 3).
Dropped findings before posting: 0. High-severity findings: 1.
Risk Taxonomy
- Docs only: 1
- Flaky test risk: 1
- Migration: 1
- Runtime correctness: 3
- Security boundary: 1
Validation and Proof
2 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
- required: CI/release smoke proof - CI, release, launchd, or package metadata changed. Proof: green GitHub check; release-status; coverage-audit; rollback note.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Call out evidence leakage, replay/collision risks, duplicate side effects, and brittle sanitizer logic.
Profile proof expectations: Look for focused sanitizer, signature, orchestration, or fixture proof.
Related Context
Related issues/PRs: #787, #797, #798, #799.
Suggested labels: bug, docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: orchestration, security, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
evaOS review status: completedPR: #800 - LCO 1.7: Eva remote Codex control evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #800 Review URL: #800 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #800 - LCO 1.7: Eva remote Codex control
Head: e472ad0b39cef070d8136228bdaa325e66352a23 into main. Review event: COMMENT.
Provider: Unregistered provider id (builtin:zai-coding-plan, zcode (registry miss), model unknown).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
README.md |
modified | +20/-2 | Documentation | Low |
docs/BETA_RELEASE_RUNBOOK.md |
modified | +1/-1 | Documentation | Low |
docs/CLAIM_AUDIT.md |
modified | +10/-4 | Documentation | Low |
docs/HERMES_ADAPTER_BOUNDARY.md |
modified | +17/-2 | Documentation | Low |
docs/SETUP.md |
modified | +33/-4 | Documentation | Low |
docs/releases/CHANGELOG.md |
modified | +2/-1 | Documentation | Low |
docs/releases/RELEASE_NOTES_1.7.0.md |
added | +53/-0 | Documentation | Low |
package-lock.json |
modified | +37/-2 | Configuration | Low |
package.json |
modified | +5/-1 | Configuration | Low |
packages/adapters/src/codex-jsonrpc.ts |
modified | +158/-22 | Changed file | Moderate: validated P2 finding |
packages/adapters/src/index.ts |
modified | +108/-23 | Changed file | Elevated: validated P1 finding |
packages/adapters/src/policy.ts |
modified | +2/-0 | Changed file | Low |
packages/cli/src/hermes-smoke.ts |
modified | +2/-0 | Changed file | Low |
packages/mcp-server/src/codex-control-router.ts |
added | +367/-0 | Changed file | Moderate: validated P2 finding |
packages/mcp-server/src/codex-runtime-transport.ts |
added | +73/-0 | Changed file | Moderate: validated P2 finding |
packages/mcp-server/src/server-runtime.ts |
modified | +6/-18 | Changed file | Low |
packages/mcp-server/src/tools.ts |
modified | +65/-8 | Changed file | Low |
packages/openclaw-plugin/openclaw.plugin.json |
modified | +187/-23 | Changed file | Elevated: large change |
tests/bridge-safety-core.test.ts |
modified | +89/-0 | Test coverage | Low |
tests/codex-control-router.test.ts |
added | +452/-0 | Test coverage | Elevated: large change |
tests/codex-runtime-transport.test.ts |
added | +45/-0 | Test coverage | Low |
tests/control-and-mcp.test.ts |
modified | +38/-5 | Test coverage | Low |
tests/hermes-smoke.test.ts |
modified | +1/-1 | Test coverage | Low |
tests/openclaw-plugin-manifest.test.ts |
modified | +1/-1 | Test coverage | Low |
tests/qa-lab-tool-coverage.test.ts |
modified | +7/-7 | Test coverage | Low |
1 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 6 (P0: 0, P1: 2, P2: 4, P3: 0).
Dropped findings before posting: 0. High-severity findings: 2.
Risk Taxonomy
- Auth: 1
- Runtime correctness: 5
Validation and Proof
2 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
- required: CI/release smoke proof - CI, release, launchd, or package metadata changed. Proof: green GitHub check; release-status; coverage-audit; rollback note.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Call out evidence leakage, replay/collision risks, duplicate side effects, and brittle sanitizer logic.
Profile proof expectations: Look for focused sanitizer, signature, orchestration, or fixture proof.
Related Context
Related issues/PRs: #787, #797, #798, #799.
Suggested labels: bug, docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: orchestration, security, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/adapters/src/codex-jsonrpc.ts (1)
577-611: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersistent clients accumulate
notificationsacross requests, which now leaks into every response.
CodexJsonRpcClient.notificationsis append-only and eachrequestRaw/turn-resolution result returns[...this.notifications]. Withpersistent: truethe same client instance now serves many requests, so:
- response payloads grow unbounded over the daemon connection's lifetime, and
- a later turn's proof/result carries notifications emitted for earlier, unrelated turns, which is exactly the signal
liveProofState/turnResolutionFromResponseconsumers interpret.Stdio mode was immune because each request got a fresh client. Consider snapshotting per-request (e.g. capture
notifications.lengthat request start and return only the delta), or bounding the buffer.#!/bin/bash # Inspect how response.notifications is consumed for proof/turn state. rg -nP -C3 '\bnotifications\b' packages/adapters/src/codex-jsonrpc.ts | head -80 rg -nP -C3 'notificationMethods|response\.notifications' packages/adapters/src/index.ts | head -60🤖 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 `@packages/adapters/src/codex-jsonrpc.ts` around lines 577 - 611, Prevent persistent CodexJsonRpcClient notifications from leaking across requests by scoping returned notifications to the current request. Update requestRaw and turn-resolution response construction to capture the notifications length at request start and return only newly emitted entries, preserving existing behavior for fresh clients and live proof/turn consumers.packages/mcp-server/src/tools.ts (1)
1642-1648: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpread
schemaExtrasbefore the fixed schema keys.As written, extras take precedence and can override
type,additionalProperties: false,properties, orrequired— silently weakening the plugin-boundary guard atvalidateOpenClawToolInput. Ordering extras first keepsanyOfworking while making the invariants unoverridable.♻️ Proposed fix
inputSchema: { + ...schemaExtras, type: "object", additionalProperties: false, properties, - ...(required.length > 0 ? { required } : {}), - ...schemaExtras + ...(required.length > 0 ? { required } : {}) },🤖 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 `@packages/mcp-server/src/tools.ts` around lines 1642 - 1648, Update the inputSchema object in the tool definition so schemaExtras is spread before the fixed type, additionalProperties, properties, and conditional required fields. Preserve schemaExtras support, including anyOf, while ensuring validateOpenClawToolInput’s fixed schema invariants cannot be overridden.packages/mcp-server/src/server-runtime.ts (1)
103-121: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTransport misconfiguration surfaces as
tool_registry_unavailable.
createConfiguredCodexClients()throws for an unknownLCO_CODEX_TRANSPORTor a relativeLCO_CODEX_DAEMON_SOCKET, and the barecatchmaps that totool_registry_unavailablewhile discarding the message. For an opt-in transport flag, misconfiguration is the most likely first-run failure and the operator gets no pointer to the env var. Split the client construction into its own guarded step with a distinct code.🩺 Proposed fix
+ let codexClients: ReturnType<typeof createConfiguredCodexClients>; + try { + codexClients = createConfiguredCodexClients(); + } catch { + db.close(); + return { ok: false, failure: createStartupUnavailableResult("codex_transport_unavailable") }; + } + try { - const codexClients = createConfiguredCodexClients(); runtimeState = {Requires
codex_transport_unavailableto be an acceptedcreateStartupUnavailableResultreason.🤖 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 `@packages/mcp-server/src/server-runtime.ts` around lines 103 - 121, Update the startup flow around createConfiguredCodexClients so transport-client construction is guarded separately from createLooTools initialization. Catch transport configuration errors and return createStartupUnavailableResult with the distinct codex_transport_unavailable reason, preserving cleanup and the existing tool_registry_unavailable result for tool registry failures; ensure the new reason is accepted by createStartupUnavailableResult.
🤖 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 `@packages/adapters/src/codex-jsonrpc.ts`:
- Around line 527-546: Harden the persistent reconnect flow around the queued
operation: ensure a failed persistentClient.connect() immediately closes the
newly created client, clears persistentClient, and rethrows the connection
error. Also pre-validate the requested method with assertCodexMethodAllowed
before entering withClient, matching requestSequence, so policy rejections do
not trigger the catch block’s connection teardown.
In `@packages/adapters/src/index.ts`:
- Around line 846-849: Update the approval reuse check around
assertCodexControlTransportCapability to scan audit records by time rather than
limiting the search to audit.tail(1000). Inspect records newer than
previous.createdAt (or continue scanning until passing the dry-run record), and
preserve the existing error when a matching approvalAuditId is found.
- Around line 903-913: Replace the bare catch around the direct request in the
rawResponse assignment with the same error classifier used by the sequence path
near line 872. Preserve deterministic errors such as policy rejections and only
map errors classified as indeterminate to codex_control_attempt_indeterminate;
rethrow all other errors unchanged.
- Around line 1387-1398: Update isIndeterminateControlResponse and
isIndeterminateControlError to remove the broad bare socket and websocket
alternatives, matching only concrete transport I/O failure patterns such as
socket hang up/closed and EPIPE alongside the existing timeout, connection,
reset, and broken-pipe patterns. Ensure deterministic validation errors like the
absolute Unix socket path requirement no longer normalize to
codex_control_attempt_indeterminate.
- Around line 673-675: Make the tail capability required on ControlAuditStore so
approval-reuse detection in the live control execution path cannot be skipped.
Update the ControlAuditStore type near its definition and adjust any affected
implementations or test doubles to provide tail; do not leave the optional-chain
behavior in the approval check as a fail-open path.
In `@packages/mcp-server/src/codex-control-router.ts`:
- Around line 211-243: Bound the loaded-thread enumeration in loadDaemonTargets
by applying a documented maximum to the ids returned by loadedThreadIds, and
surface an explicit reason code or equivalent result metadata when truncation
occurs. Replace the strictly serial per-thread reads with concurrency-limited
processing if supported by the shared CodexClient transport, while preserving
TargetProjection filtering and active-thread refresh behavior.
- Around line 74-84: The hint contract must match title sanitization and its
160-character limit. In packages/mcp-server/src/codex-control-router.ts lines
74-84, update the hint matching flow to pass the trimmed hint through
sanitizeTitle before comparing it with target.title, while preserving threadId
matching. In packages/mcp-server/src/tools.ts lines 1105-1109, reduce the hint
schema maxLength from 240 to 160, or reuse a shared title-length constant.
- Around line 169-181: Update selectedRoute to prune expired entries from the
targets map before inserting each new ref. Remove entries whose expiresAtMs is
at or before the current timestamp, then retain the existing target storage and
routeResult behavior for the newly selected target.
In `@packages/mcp-server/src/codex-runtime-transport.ts`:
- Around line 52-57: Update the daemon read wrapper around codexReadClient so it
delegates the complete CodexClient surface, not only request, while preserving
assertCodexMethodAllowed(method, "read") for every read request. Ensure members
such as requestSequenceUntilTurnResolved remain available in daemon mode and
retain the shared persistent connection and read-only enforcement behavior.
In `@packages/mcp-server/src/tools.ts`:
- Around line 1133-1148: Update the lco_codex_interrupt_thread handler to reject
inputs that contain both target_ref and the explicit thread_id/expected_turn_id
pair before dispatching. Preserve the existing target_ref and raw-thread paths
for unambiguous inputs, and replace the nested ternary with a guarded named
handler if needed to make the ambiguity check clear.
In `@tests/bridge-safety-core.test.ts`:
- Around line 908-914: Replace the fixed delay after sockets[0]?.close() with
polling of the server-side connection/transport state until the close is
observed, then issue the thread/loaded/list request. Keep the existing reconnect
and header/method assertions unchanged, and use the test’s existing state or
helper symbols where available.
In `@tests/codex-runtime-transport.test.ts`:
- Around line 29-45: Add a successful assertion in the test covering
resolveCodexRuntimeTransportConfig that supplies an absolute
LCO_CODEX_DAEMON_SOCKET alongside CODEX_HOME-related inputs, then verify the
resolved daemon socket uses that absolute override rather than CODEX_HOME
resolution. Keep the existing relative-path rejection and unknown-transport
assertions unchanged.
In `@tests/tool-exposure-profile.test.ts`:
- Around line 368-371: Update the lco_codex_deliver sample input in the
tool-exposure test to explicitly set dry_run to true alongside target_ref and
message. Keep the existing values and behavior unchanged while preventing
reliance on the schema default.
---
Outside diff comments:
In `@packages/adapters/src/codex-jsonrpc.ts`:
- Around line 577-611: Prevent persistent CodexJsonRpcClient notifications from
leaking across requests by scoping returned notifications to the current
request. Update requestRaw and turn-resolution response construction to capture
the notifications length at request start and return only newly emitted entries,
preserving existing behavior for fresh clients and live proof/turn consumers.
In `@packages/mcp-server/src/server-runtime.ts`:
- Around line 103-121: Update the startup flow around
createConfiguredCodexClients so transport-client construction is guarded
separately from createLooTools initialization. Catch transport configuration
errors and return createStartupUnavailableResult with the distinct
codex_transport_unavailable reason, preserving cleanup and the existing
tool_registry_unavailable result for tool registry failures; ensure the new
reason is accepted by createStartupUnavailableResult.
In `@packages/mcp-server/src/tools.ts`:
- Around line 1642-1648: Update the inputSchema object in the tool definition so
schemaExtras is spread before the fixed type, additionalProperties, properties,
and conditional required fields. Preserve schemaExtras support, including anyOf,
while ensuring validateOpenClawToolInput’s fixed schema invariants cannot be
overridden.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58503632-85c1-43d5-8473-4fe99fe96e29
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.codex-plugin/plugin.jsonREADME.mddocs/BETA_RELEASE_RUNBOOK.mddocs/CLAIM_AUDIT.mddocs/HERMES_ADAPTER_BOUNDARY.mddocs/SETUP.mddocs/releases/CHANGELOG.mddocs/releases/RELEASE_NOTES_1.7.0.mdopenclaw.plugin.jsonpackage.jsonpackages/adapters/src/codex-jsonrpc.tspackages/adapters/src/index.tspackages/adapters/src/policy.tspackages/cli/src/hermes-smoke.tspackages/mcp-server/src/codex-control-router.tspackages/mcp-server/src/codex-runtime-transport.tspackages/mcp-server/src/server-runtime.tspackages/mcp-server/src/tools.tspackages/openclaw-plugin/openclaw.plugin.jsontests/bridge-safety-core.test.tstests/codex-control-router.test.tstests/codex-runtime-transport.test.tstests/control-and-mcp.test.tstests/hermes-smoke.test.tstests/openclaw-plugin-manifest.test.tstests/qa-lab-tool-coverage.test.tstests/tool-exposure-profile.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (2)
docs/SETUP.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep detailed first-run instructions in
docs/SETUP.md.
Files:
docs/SETUP.md
README.md
📄 CodeRabbit inference engine (AGENTS.md)
README.md: KeepREADME.mdas the public landing page: product value, install, setup, first workflow, OpenClaw/MCP entrypoints, safety boundaries, and links.
Do not put active sprint ledgers, long issue history, exhaustive release-gate command inventories, or internal agent maintenance loops into the publicREADME.md; linkVISION.md, GitHub trackers, or runbooks instead.
Files:
README.md
🪛 ast-grep (0.45.0)
packages/adapters/src/codex-jsonrpc.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🪛 LanguageTool
docs/HERMES_ADAPTER_BOUNDARY.md
[style] ~56-~56: Since ownership is already implied, this phrasing may be redundant.
Context: ...the shared LCO daemon-control tools and its own Computer Use integration. - No claim th...
(PRP_OWN)
README.md
[grammar] ~390-~390: Use a hyphen to join words.
Context: ... only to the already-running local Codex managed daemon through its Unix socket. ...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (44)
packages/adapters/src/codex-jsonrpc.ts (4)
385-406:socket.once("error")still leaves long-lived connections one error away from an unhandlederrorevent.After
open, that single listener is consumed by the first mid-stream error (rejecting an already-settled promise, i.e. a no-op), so a seconderrorevent has no listener and crashes the process. Same shape as previously flagged.
2-14: LGTM!Also applies to: 159-162
490-504: LGTM!
408-411: 🩺 Stability & AvailabilityGuard
sendJsonwhen the socket is no longer open.
await this.readyonly covers the initial connection; after a later close,socket.send()can fail on a non-OPEN transport instead of rejecting this call cleanly. Add a last-momentreadyState/closedcheck or pass a send callback so reconnect logic can handle the failure deterministically.package.json (2)
3-3: LGTM!Also applies to: 78-81
82-83: 🔒 Security & PrivacyNo action needed for
ws^8.21.1already includes the patched release.tests/bridge-safety-core.test.ts (1)
3-8: LGTM!Also applies to: 17-21, 861-907, 915-944
packages/adapters/src/index.ts (3)
708-708: LGTM!Also applies to: 810-810, 836-836
990-990: LGTM!Also applies to: 1008-1019
1353-1378: LGTM!tests/control-and-mcp.test.ts (2)
483-496: LGTM!Also applies to: 539-548, 1638-1639, 1669-1672, 1880-1884
1885-1890: 🎯 Functional CorrectnessNo duplicate
errormember hereThe inline type assertion declares
error?: { message?: string }only once, so this is not a duplicate-identifier issue.> Likely an incorrect or invalid review comment..codex-plugin/plugin.json (1)
3-3: LGTM!openclaw.plugin.json (1)
5-5: LGTM!Also applies to: 56-57, 95-96, 1330-1330, 1390-1460, 1543-1543, 1652-1702, 3253-3253, 3314-3386, 3471-3471, 3583-3634, 7078-7079
packages/openclaw-plugin/openclaw.plugin.json (1)
5-5: LGTM! Root and package manifests stay in lockstep — canonical/alias declarations,anyOfinterrupt contract, tier demotions, and facade list all match.Also applies to: 56-57, 95-96, 1330-1330, 1390-1460, 1543-1543, 1652-1702, 3253-3253, 3314-3386, 3471-3471, 3583-3634, 7078-7079
tests/openclaw-plugin-manifest.test.ts (1)
154-154: LGTM!tests/qa-lab-tool-coverage.test.ts (1)
124-131: LGTM! Tier counts still sum to the declared total (9+19+8+3 = 39), and the demotion/addition math is internally consistent.Also applies to: 159-159, 399-400, 601-601
tests/tool-exposure-profile.test.ts (1)
71-71: LGTM!Also applies to: 83-84, 151-153
docs/releases/RELEASE_NOTES_1.7.0.md (2)
1-22: LGTM! Highlights, opt-in transport framing, and the safety boundary match the PR scope (no Codex start/restart, no Remote Control, approval-bound delivery).Also applies to: 28-53
23-26: 📐 Maintainability & Code QualityKeep the install command as-is The active npm package is
lossless-codex-orchestrator;lossless-openclaw-orchestratoris the deprecated compatibility alias. No change needed here.> Likely an incorrect or invalid review comment.README.md (2)
313-316: LGTM!
382-393: 🎯 Functional CorrectnessVerify the documented Codex environment-variable aliases.
All three documents expose
LCO_CODEX_TRANSPORT,LCO_CODEX_DAEMON_SOCKET, andLCO_CODEX_APP_SERVER_ARGS, while the supplied runtime contract callsreadEnv("CODEX_*", env). Confirm that the environment reader maps these documented names; otherwise daemon configuration may silently remain on stdio.
README.md#L382-L393: add or verify the alias contract for the public configuration.docs/HERMES_ADAPTER_BOUNDARY.md#L30-L36: keep Hermes daemon guidance consistent with runtime resolution.docs/SETUP.md#L348-L360: add a setup-level regression test for daemon selection and socket override.docs/BETA_RELEASE_RUNBOOK.md (1)
448-448: LGTM!docs/CLAIM_AUDIT.md (1)
3-13: LGTM!Also applies to: 60-64, 111-111
docs/HERMES_ADAPTER_BOUNDARY.md (1)
11-12: LGTM!Also applies to: 55-56
docs/SETUP.md (1)
413-413: LGTM!Also applies to: 594-600
docs/releases/CHANGELOG.md (1)
7-9: LGTM!packages/mcp-server/src/codex-control-router.ts (6)
119-137: Previously flagged: idle send has no turn-resolution proof, and the catch collapses approval-mismatch into a generic reason.Both remain as-is on this revision (
loadedThread:true+awaitTurn:falsefor the idle path;approval_or_control_rejectedcovering stale-approval and daemon failure alike). See prior comments on lines 119 and 129.
1-52: LGTM!
140-167: LGTM!
183-206: LGTM!
261-291: LGTM!
293-367: LGTM! Redaction before truncation insanitizeTitleis the right order — no partially-sliced secret can escape.tests/codex-control-router.test.ts (2)
250-268: Idle live delivery is still only asserted atstatus === "accepted".This is the test-side of the previously flagged proof gap on
codex-control-router.tsline 119: nothing here pinscompleted === false/control_sent === truefor a fire-and-forget idle send, so a future change that starts derivingcompletedfromcontrol_sentwould pass silently.
23-117: LGTM! The forbidden-substring canaries (thread id, turn id, transcript, absolute path) plus theincludeTurnscall-shape assertions and the approval-replay/indeterminate cases are exactly the right regression surface.Also applies to: 119-208, 285-442
packages/mcp-server/src/codex-runtime-transport.ts (1)
16-38: LGTM! Unknown modes and relative socket overrides both fail closed, and readingCODEX_HOMEdirectly (unprefixed) while everything else goes through theLCO_-prefixedreadEnvis the right split.packages/mcp-server/src/server-runtime.ts (1)
5-7: LGTM!Also applies to: 20-20
tests/codex-runtime-transport.test.ts (1)
6-27: LGTM! Passingenvexplicitly instead of mutatingprocess.envkeeps these hermetic.packages/adapters/src/policy.ts (1)
180-181: LGTM!readOnlyfor route (metadata reads only) andapprovalGatedControlfor deliver (idle send / active steer) match the existingcodex_directcontrol entries.packages/mcp-server/src/tools.ts (3)
470-479: LGTM! Scoping theanyOfinterpretation torequiredsets (and keeping the message field-free) is the right minimal extension for this boundary validator.
553-556: LGTM! Reads routed throughcodexReadClientwhile mutations go viacontrolkeeps the surface split intact.
233-242: 📐 Maintainability & Code QualityEnsure
operatorPathRank8 and 9 are unique
- Check these don’t collide with the rest of
LOO_TOOL_SURFACE; duplicate ranks would scramble operator-path ordering.packages/cli/src/hermes-smoke.ts (1)
18-19: LGTM! Names match the registrations intools.ts, and inserting mid-array leaves theslice(0, -1)omit-a-tool fixture intests/hermes-smoke.test.tsintact.tests/hermes-smoke.test.ts (1)
39-39: LGTM! 16 matches the updatedEVA_HERMES_REQUIRED_LCO_TOOLSexactly.
evaOS review status: completedPR: #800 - LCO 1.7: Eva remote Codex control evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #800 Review URL: #800 (review) |
Current-head review dispositionsCandidate: Fixed now
False or not applicable
Accepted tradeoffs or existing follow-up
Proof remains separated: current-head CI/review prove source readiness; package and isolated Hermes canaries prove the candidate; merge, publication, Eva installation, Telegram direction, and Desktop CUA each require their own evidence. |
There was a problem hiding this comment.
Walkthrough
PR: #800 - LCO 1.7: Eva remote Codex control
Head: cc971ef9628c61610c201f0cd56307abe3c17dba into main. Review event: COMMENT.
Provider: Unregistered provider id (builtin:zai-coding-plan, zcode (registry miss), model unknown).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
README.md |
modified | +20/-2 | Documentation | Low |
docs/BETA_RELEASE_RUNBOOK.md |
modified | +1/-1 | Documentation | Low |
docs/CLAIM_AUDIT.md |
modified | +10/-4 | Documentation | Low |
docs/HERMES_ADAPTER_BOUNDARY.md |
modified | +17/-2 | Documentation | Low |
docs/SETUP.md |
modified | +33/-4 | Documentation | Low |
docs/releases/CHANGELOG.md |
modified | +2/-1 | Documentation | Low |
docs/releases/RELEASE_NOTES_1.7.0.md |
added | +53/-0 | Documentation | Low |
package-lock.json |
modified | +37/-2 | Configuration | Low |
package.json |
modified | +5/-1 | Configuration | Low |
packages/adapters/src/codex-jsonrpc.ts |
modified | +176/-26 | Changed file | Moderate: validated P3 finding |
packages/adapters/src/index.ts |
modified | +132/-23 | Changed file | Moderate: validated P3 finding |
packages/adapters/src/policy.ts |
modified | +2/-0 | Changed file | Low |
packages/cli/src/hermes-smoke.ts |
modified | +2/-0 | Changed file | Low |
packages/mcp-server/src/codex-control-router.ts |
added | +368/-0 | Changed file | Moderate: validated P2 finding |
packages/mcp-server/src/codex-runtime-transport.ts |
added | +73/-0 | Changed file | Low |
packages/mcp-server/src/server-runtime.ts |
modified | +6/-18 | Changed file | Low |
packages/mcp-server/src/tools.ts |
modified | +65/-8 | Changed file | Low |
packages/openclaw-plugin/openclaw.plugin.json |
modified | +187/-23 | Changed file | Elevated: large change |
tests/bridge-safety-core.test.ts |
modified | +115/-0 | Test coverage | Low |
tests/codex-control-router.test.ts |
added | +493/-0 | Test coverage | Elevated: large change |
tests/codex-runtime-transport.test.ts |
added | +45/-0 | Test coverage | Low |
tests/control-and-mcp.test.ts |
modified | +64/-5 | Test coverage | Low |
tests/hermes-smoke.test.ts |
modified | +1/-1 | Test coverage | Low |
tests/openclaw-plugin-manifest.test.ts |
modified | +1/-1 | Test coverage | Low |
tests/qa-lab-tool-coverage.test.ts |
modified | +7/-7 | Test coverage | Low |
1 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 4 (P0: 0, P1: 0, P2: 2, P3: 2).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- Proof gap: 1
- Runtime correctness: 3
Validation and Proof
2 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
- required: CI/release smoke proof - CI, release, launchd, or package metadata changed. Proof: green GitHub check; release-status; coverage-audit; rollback note.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Call out evidence leakage, replay/collision risks, duplicate side effects, and brittle sanitizer logic.
Profile proof expectations: Look for focused sanitizer, signature, orchestration, or fixture proof.
Related Context
Related issues/PRs: #787, #797, #798, #799.
Suggested labels: docs, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: orchestration, security, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
Latest-head review delta dispositionsHead:
No further code change is warranted for these comments under the bounded 1.7 gate. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc971ef962
ℹ️ 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".
| if (options.audit.hasApprovalUse(previous.id)) { | ||
| throw new Error("approval_audit_id has already been used"); | ||
| } | ||
| options.audit.append({ |
There was a problem hiding this comment.
When two MCP, CLI, or native-plugin processes share the default audit file and concurrently submit the same live approval_audit_id, both can finish hasApprovalUse() before either appends its claim. Each process then records a claim and sends the live control, allowing one approval to execute twice. Replace this check-then-append sequence with a filesystem-locked or otherwise atomic claim operation.
Useful? React with 👍 / 👎.
| return createCodexClientFromTransport( | ||
| () => new UnixSocketWebSocketTransport(options.socketPath, options.timeoutMs), | ||
| { ...options, persistent: true } | ||
| ); |
There was a problem hiding this comment.
When any daemon-owned turn emits a JSON-RPC server request—such as an approval, user-input, or MCP elicitation request—while an ordinary thread/loaded/list or thread/read call is awaiting its response, requestRaw() discards the message because its ID differs from the client request. Making this connection persistent leaves the daemon request assigned to a client that can no longer observe or answer it, so the active turn can remain stuck and later turn proof omits the pending server request. Demultiplex and retain server requests across daemon operations, or close the connection when one cannot be handled.
Useful? React with 👍 / 👎.
1 participant
Summary
Runtime findings addressed
accepted, allowing active re-resolution and steeringValidation
npm run check— 1,281 tests passednpm pack --dry-run1.7.0Boundaries
Closes #787
Closes #797
Related to #798
Related to #799
Agent-authored under the user-approved LCO 1.7 delivery plan.
Summary by CodeRabbit
New Features
Documentation
Release