Skip to content

fix(cli): make native hook relay resilient to stale bridges#88718

Closed
NickMartyniuk wants to merge 2 commits into
openclaw:mainfrom
NickMartyniuk:fix/native-hook-relay-resilience
Closed

fix(cli): make native hook relay resilient to stale bridges#88718
NickMartyniuk wants to merge 2 commits into
openclaw:mainfrom
NickMartyniuk:fix/native-hook-relay-resilience

Conversation

@NickMartyniuk

@NickMartyniuk NickMartyniuk commented May 31, 2026

Copy link
Copy Markdown

Summary

  • fall back from stale/unusable direct native hook bridge records to the gateway nativeHook.invoke path
  • fail open for unavailable pre-tool relay events so tool execution is not blocked by relay outages
  • keep permission_request unavailable handling fail-closed

Verification

  • pnpm exec vitest run src/cli/native-hook-relay-cli.test.ts
  • TMPDIR="/var/folders/fh/106677c12vb2dpynx68j804c0000gn/T/tmp.KOtcmuzV9h" pnpm exec vitest run src/agents/harness/native-hook-relay.test.ts --testNamePattern "dead foreign|expired foreign|live unexpired foreign|stale direct bridge"

@openclaw-barnacle openclaw-barnacle Bot added cli CLI command changes agents Agent runtime and tooling size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 31, 2026
@clawsweeper

clawsweeper Bot commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 27, 2026, 5:00 PM ET / 21:00 UTC.

Summary
The PR changes native hook relay CLI stale-bridge errors to fall through to gateway RPC and changes unavailable PreToolUse relay output from deny to no-op, with focused CLI tests.

PR surface: Source -12, Tests -33. Total -45 across 3 files.

Reproducibility: yes. at source level: current code and tests show stale direct-bridge errors stop before gateway fallback, and Codex source shows empty successful PreToolUse output does not block. I did not run the reported WSL2/OpenClaw/Codex gateway restart scenario in this read-only review.

Review metrics: 1 noteworthy metric.

  • Native hook fallback contract: 1 fail-closed PreToolUse unavailable path changed to no-op. This path decides whether Codex native tool execution continues when OpenClaw cannot evaluate before-tool policy.

Stored data model
Persistent data-model change detected: serialized state: src/cli/native-hook-relay-cli.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #89325
Summary: This PR is a flawed candidate for the stale native hook relay restart/drain problem, while related work covers safer no-policy fallback and a broader open restart repair.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Preserve fail-closed unavailable PreToolUse unless the explicit no-policy noop marker is present.
  • [P1] Add redacted real OpenClaw/Codex stale-bridge terminal logs, runtime logs, a recording, or a linked artifact.
  • [P1] Refresh the conflicting branch after maintainers choose the canonical restart/drain repair direction.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body has Vitest commands only; it needs redacted terminal output, logs, a recording, or a linked artifact from a real stale-bridge setup before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Merging as-is can allow Codex native tools to proceed when OpenClaw cannot evaluate before_tool_call policy, loop detection, or deferred approval state.
  • [P1] The PR body lists Vitest commands only, so the stale-bridge recovery is not proven in a real OpenClaw/Codex setup.
  • [P1] The branch currently conflicts with main and overlaps fix(gateway): regenerate native hook descriptors when relay unavailable after restart #94870, so maintainers need a refreshed canonical direction before merge.

Maintainer options:

  1. Rework Around The Guarded Fallback (recommended)
    Preserve stale direct-bridge gateway fallback only if unknown unavailable PreToolUse and PermissionRequest still fail closed and the explicit no-policy marker remains the only no-op path.
  2. Accept Broad Fail-Open Semantics
    Maintainers could intentionally accept unavailable PreToolUse as observational, but that needs explicit security and compatibility approval plus real behavior proof.
  3. Pause For The Canonical Restart Repair
    Pause or close this branch if fix(gateway): regenerate native hook descriptors when relay unavailable after restart #94870 becomes the selected restart/drain implementation for [Bug]: Native Hook Relay — Stale Relay After Gateway Restart/Drain #89325.

Next step before merge

  • [P1] Manual/security review plus contributor real-behavior proof are needed; automation cannot provide the external PR author's real stale-bridge setup proof.

Security
Needs attention: The diff changes a native tool-execution permission boundary by making unavailable PreToolUse fail open.

Review findings

  • [P1] Preserve fail-closed PreToolUse outage handling — src/agents/harness/native-hook-relay.ts:744
Review details

Best possible solution:

Keep stale direct-bridge gateway fallback only if unknown unavailable PreToolUse remains fail-closed and no-op behavior stays limited to the explicit no-policy marker, or choose #94870 after real proof and security acceptance.

Do we have a high-confidence way to reproduce the issue?

Yes at source level: current code and tests show stale direct-bridge errors stop before gateway fallback, and Codex source shows empty successful PreToolUse output does not block. I did not run the reported WSL2/OpenClaw/Codex gateway restart scenario in this read-only review.

Is this the best way to solve the issue?

No. Falling through from stale direct bridge to gateway is plausible, but making all unavailable PreToolUse events no-op is not the best fix because current main intentionally keeps unknown policy state fail-closed.

Full review comments:

  • [P1] Preserve fail-closed PreToolUse outage handling — src/agents/harness/native-hook-relay.ts:744
    This returns an empty successful response for unavailable pre_tool_use, and Codex treats empty stdout with exit code 0 as non-blocking. Keep unknown relay outages fail-closed and only no-op when the generated command carries the explicit no-policy marker.
    Confidence: 0.92

Overall correctness: patch is incorrect
Overall confidence: 0.92

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 1b8b8500cee0.

Label changes

Label justifications:

  • P1: The PR targets a broken native hook relay workflow and touches native tool approval behavior.
  • merge-risk: 🚨 compatibility: The diff changes fallback semantics for existing Codex native hook relay commands during relay outages.
  • merge-risk: 🚨 security-boundary: PreToolUse controls native tool execution, and the PR can make unavailable policy evaluation non-blocking.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body has Vitest commands only; it needs redacted terminal output, logs, a recording, or a linked artifact from a real stale-bridge setup before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source -12, Tests -33. Total -45 across 3 files.

View PR surface stats
Area Files Added Removed Net
Source 2 2 14 -12
Tests 1 23 56 -33
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 3 25 70 -45

Security concerns:

  • [high] PreToolUse relay outage can bypass policy — src/agents/harness/native-hook-relay.ts:744
    Returning no-op output for unavailable PreToolUse means Codex can continue native tool execution when OpenClaw cannot evaluate before_tool_call policy, loop detection, or deferred approval state.
    Confidence: 0.9

What I checked:

Likely related people:

  • steipete: Authored and merged the guarded no-policy native hook fallback in fix: preserve no-policy native hook fallback #88846, directly overlapping this PR's PreToolUse fallback semantics. (role: current fallback contract contributor and merger; confidence: high; commits: 76eb478ac9fe, fb64546d9e6f; files: src/agents/harness/native-hook-relay.ts, src/cli/native-hook-relay-cli.ts, extensions/codex/src/app-server/native-hook-relay.ts)
  • pashpashpash: Authored the merged commit that bridged Codex native hooks into OpenClaw, creating the affected relay CLI and harness area. (role: introduced behavior; confidence: medium; commits: 7a958d920c88; files: src/agents/harness/native-hook-relay.ts, src/cli/native-hook-relay-cli.ts, extensions/codex/src/app-server/native-hook-relay.ts)
  • Josh Avant: Authored recent native hook relay lifetime hardening across the CLI, harness, and Codex app-server relay paths. (role: recent adjacent contributor; confidence: medium; commits: 14b1ebd640c6; files: src/agents/harness/native-hook-relay.ts, src/cli/native-hook-relay-cli.ts, extensions/codex/src/app-server/native-hook-relay.ts)
  • Alex Knight: Authored recent Codex native hook relay restart and generation preservation work adjacent to stale relay behavior. (role: recent adjacent contributor; confidence: high; commits: 42e9504114f3; files: extensions/codex/src/app-server/native-hook-relay.ts, extensions/codex/src/app-server/run-attempt.ts, extensions/codex/src/app-server/session-binding.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels May 31, 2026
This was referenced Jun 20, 2026
Closed
Open
@clawsweeper clawsweeper Bot mentioned this pull request Jul 3, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling cli CLI command changes merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@NickMartyniuk