Skip to content

fix(agents): prevent native hook relay bridge race condition on renew and registration#100300

Closed
nankingjing wants to merge 1 commit into
openclaw:mainfrom
nankingjing:fix/98650-native-hook-relay-race
Closed

fix(agents): prevent native hook relay bridge race condition on renew and registration#100300
nankingjing wants to merge 1 commit into
openclaw:mainfrom
nankingjing:fix/98650-native-hook-relay-race

Conversation

@nankingjing

@nankingjing nankingjing commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Remove synchronous bridge record write after server.listen() that races before the TCP server binds, and guard renew handler with server.listening check to prevent stale relay registrations.

Closes #98650

@nankingjing
fix(agents): prevent native hook relay bridge race condition on renew…
c6c3f82 
… and registration

Remove synchronous bridge record write after server.listen() that races
before the TCP server binds, and guard renew handler with server.listening
check to prevent stale relay registrations.

Closes openclaw#98650
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 5, 2026
@clawsweeper

clawsweeper Bot commented Jul 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 5, 2026, 7:31 AM ET / 11:31 UTC.

Summary
The PR changes native hook relay bridge registration so renewals only rewrite bridge records while the server is listening and removes an immediate post-listen bridge-record write.

PR surface: Source -3. Total -3 across 1 file.

Reproducibility: Partially: the linked issue gives concrete Gateway steps and source inspection confirms the address-unavailable log plus fail-closed PreToolUse path. I did not run the isolated multi-agent Gateway flow, and the reported valid-record failure remains unproven by this PR.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #98650
Summary: This PR is a candidate fix for the linked canonical native hook relay outage, but nearby open relay issues remain only partially overlapping without proof that this branch fixes them.

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: 🦪 silver shellfish
Result: blocked until real behavior proof is added.

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

Rank-up moves:

  • [P1] Add authored problem and Evidence sections to the PR body.
  • Post redacted live output or logs showing an isolated Gateway Codex tool call succeeds after the change.
  • [P1] Add or update focused relay coverage for the bridge-record availability path being fixed.

Proof guidance:

  • [P1] Needs real behavior proof before merge: No after-fix real behavior proof is present; the PR body has no authored Evidence section, no live Gateway run, and the Real behavior proof check failed for missing problem/evidence content. 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

Maintainer options:

  1. Decide the mitigation before merge
    Prove a real isolated Gateway Codex tool call recovers while PreToolUse enforcement still works, or narrow this PR to a non-closing cleanup and leave Codex harness: "native hook relay unavailable" — bridge record written but every tool fail-closes (2026.6.11, isolated multi-agent gateway) #98650 open.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] Manual review is needed because the remaining blocker is proof and security-sensitive relay behavior, not a narrow mechanical repair ClawSweeper can safely supply.

Maintainer decision needed

  • Question: Should this PR be allowed to merge as the fix for the linked native hook relay outage, or should it be treated only as cleanup until live Codex Gateway proof exists?
  • Rationale: The code change only addresses a pre-listen write/log path, while the linked report says a valid bridge record already exists and still fails; deciding whether that is acceptable needs maintainer proof/security judgment.
  • Likely owner: steipete — The no-policy fallback and fail-closed relay contract are central to the proof question and recent merged history points to steipete on that path.
  • Options:
    • Require live fix proof (recommended): Keep this PR blocked until it demonstrates a real isolated Gateway Codex tool call succeeding after the change while PreToolUse policy remains enforced.
    • Accept cleanup only: Allow a narrow cleanup version only if the closing reference is removed and the linked issue remains open for the actual relay outage.
    • Close in favor of issue work: Close this PR if maintainers want the security-sensitive relay recovery designed from the canonical issue instead of carrying a partial branch.

Security
Cleared: The diff does not add dependencies, broaden permissions, or weaken the existing fail-closed PreToolUse handling, but the security-sensitive behavior still needs live proof before merge.

Review findings

  • [P1] Prove the relay fix before closing the outage — src/agents/harness/native-hook-relay.ts:477-478
Review details

Best possible solution:

Prove a real isolated Gateway Codex tool call recovers while PreToolUse enforcement still works, or narrow this PR to a non-closing cleanup and leave #98650 open.

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

Partially: the linked issue gives concrete Gateway steps and source inspection confirms the address-unavailable log plus fail-closed PreToolUse path. I did not run the isolated multi-agent Gateway flow, and the reported valid-record failure remains unproven by this PR.

Is this the best way to solve the issue?

No: the patch is too narrow to be the best fix for the linked outage because it removes the pre-listen write path but does not address a valid bridge record that is still unusable. The safer path is a proven relay lookup/generation/session-lifecycle fix with focused regression coverage.

Full review comments:

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 14eb460dd21a.

Label changes

Label changes:

  • add P1: The PR targets a linked P1 Codex agent workflow outage where native tools fail closed for real users, and the proposed fix is not yet proven.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: No after-fix real behavior proof is present; the PR body has no authored Evidence section, no live Gateway run, and the Real behavior proof check failed for missing problem/evidence content. 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.

Label justifications:

  • P1: The PR targets a linked P1 Codex agent workflow outage where native tools fail closed for real users, and the proposed fix is not yet proven.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: No after-fix real behavior proof is present; the PR body has no authored Evidence section, no live Gateway run, and the Real behavior proof check failed for missing problem/evidence content. 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 -3. Total -3 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 1 4 -3
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 1 4 -3

What I checked:

  • PR diff scope: The patch changes only src/agents/harness/native-hook-relay.ts: one renew guard and removal of the immediate bridge-record write after server.listen(). (src/agents/harness/native-hook-relay.ts:477, c6c3f82efbbd)
  • Current bridge write behavior: Current main writes the bridge record in the listen callback and also immediately afterward; writeNativeHookRelayBridgeRecordForRegistration returns without writing when server.address() is unavailable. (src/agents/harness/native-hook-relay.ts:989, 14eb460dd21a)
  • Linked issue says the valid-record case still fails: The linked issue reports that the bridge record already contains a valid host, port, token, and expiry, yet Codex still blocks every tool call with native hook relay unavailable.
  • Fail-closed Codex path remains downstream: Current main still turns unavailable PreToolUse relay failures into a blocking Codex response unless the generated command carried the explicit no-policy noop marker. (src/agents/harness/native-hook-relay.ts:789, 14eb460dd21a)
  • App-server approval bridge remains fail-closed: The Codex app-server approval bridge catches native relay invocation errors and returns a blocked approval result with an unavailable-relay reason. (extensions/codex/src/app-server/approval-bridge.ts:759, 14eb460dd21a)
  • Codex contract checked: Sibling Codex source shows PreToolUse block reasons are returned to the model as Command blocked by PreToolUse hook, matching the issue symptom and preserving the security-sensitive fail-closed behavior. (../codex/codex-rs/core/src/hook_runtime.rs:208)

Likely related people:

  • pashpashpash: GitHub commit-to-PR lookup ties commit 7a958d920c88 to the merged PR that bridged Codex native hooks into OpenClaw. (role: feature introducer; confidence: medium; commits: 7a958d920c88; files: src/agents/harness/native-hook-relay.ts, extensions/codex/src/app-server/native-hook-relay.ts)
  • steipete: Commit-to-PR lookup ties 76eb478ac9fe to merged work preserving no-policy native hook fallback behavior in the same fail-closed relay path. (role: fallback contract contributor; 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)
  • joshavant: Commit-to-PR lookup ties 68dabb735245 to merged native hook relay lifetime hardening across the core and Codex app-server surfaces. (role: recent relay lifecycle contributor; confidence: high; commits: 68dabb735245, 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)
  • vincentkoc: Local blame on current main attributes the touched native relay bridge and renew code in this shallow checkout to Vincent Koc's recent grafted commit. (role: recent area contributor; confidence: medium; commits: 97935832abc1; files: src/agents/harness/native-hook-relay.ts, extensions/codex/src/app-server/approval-bridge.ts, extensions/codex/src/app-server/run-attempt.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. labels Jul 5, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 5, 2026
Closed
@steipete steipete self-assigned this Jul 5, 2026
This was referenced Jul 5, 2026
Closed
Closed
Merged
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Thank you — the safe pre-listen bridge-record cleanup landed through maintainer batch PR #100399 in commit b22c36f, with contributor credit and changelog thanks preserved. This does not resolve the broader native-hook failure in #98650, which remains open. Closing this source PR as superseded.

@steipete steipete closed this Jul 5, 2026
@609NFT 609NFT mentioned this pull request Jul 14, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

agents Agent runtime and tooling 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: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Codex harness: "native hook relay unavailable" — bridge record written but every tool fail-closes (2026.6.11, isolated multi-agent gateway)

2 participants

@nankingjing @steipete