Skip to content

fix: ignore stale approval resolve errors#98394

Merged
steipete merged 2 commits into
openclaw:mainfrom
haruaiclone-droid:codex/fix-approval-error-race
Jul 5, 2026
Merged

fix: ignore stale approval resolve errors#98394
steipete merged 2 commits into
openclaw:mainfrom
haruaiclone-droid:codex/fix-approval-error-race

Conversation

@haruaiclone-droid

@haruaiclone-droid haruaiclone-droid commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Closes #98392

AI-assisted: Yes (Codex)

What Problem This Solves

An approval resolve can finish after a newer approval becomes active or after the Control UI reconnects. The older completion could attach its error to the wrong prompt, or clear the busy state owned by a newer decision.

Why This Change Was Made

Approval decisions now have an explicit operation owner: the exact Gateway client and approval id that started the request. Only that operation may release the busy state. Non-stale failures are shown only while the same client and approval remain active. This keeps reconnect and queue transitions on one canonical ownership path instead of adding error-specific fallbacks.

User Impact

Operators no longer see a stale approval error on a newer prompt, and an older request cannot re-enable controls while a newer approval decision is still in flight.

Evidence

  • Blacksmith Testbox tbx_01kwt3gy25f6vrxsven71k22mz: corepack pnpm test ui/src/app/overlays.test.ts (2/2 pass).
  • Same Testbox: node scripts/run-vitest.mjs run --config test/vitest/vitest.ui-e2e.config.ts --configLoader runner ui/src/e2e/approval-flow.e2e.test.ts (Chromium, 1/1 pass).
  • Same Testbox: corepack pnpm check:changed (pass).
  • In-app browser live run with a mocked Gateway WebSocket: rejected the older deferred resolve after activating a newer approval; the newer command remained visible, its actions remained enabled, and no approval error rendered.
  • Fresh branch autoreview against origin/main: no accepted/actionable findings.
  • git diff --check (pass).

@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 5, 2026, 5:59 PM ET / 21:59 UTC.

Summary
The PR scopes Control UI approval resolve results to the originating Gateway client and active approval id, adds unit and browser E2E coverage, and adds a changelog entry.

PR surface: Source -4, Tests +225, Docs +1. Total +222 across 4 files.

Reproducibility: yes. at source level. Current main can write a global approval error for an older id that remains anywhere in the queue while the component renders that error on the newer active prompt; I did not run the browser repro in this read-only pass.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #98392
Summary: This PR is the current candidate fix for the canonical stale Control UI approval resolve error issue; nearby queued-approval work is adjacent but distinct.

Members:

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

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

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

Next step before merge

  • Continue normal maintainer review; ClawSweeper found no patch-correctness issue.

Security
Cleared: The diff changes in-memory Control UI approval result ownership plus tests and a changelog line; it does not add dependencies, workflows, secrets handling, or authorization broadening.

Review details

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

Yes, at source level. Current main can write a global approval error for an older id that remains anywhere in the queue while the component renders that error on the newer active prompt; I did not run the browser repro in this read-only pass.

Is this the best way to solve the issue?

Yes for the runtime fix. Scoping resolve results to the operation's client and active approval id is narrower than view-only filtering or gateway changes, but the release-owned changelog edit should be removed.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body and maintainer verification comment describe after-fix live in-app browser proof with a mocked Gateway WebSocket on the rewritten head, showing the stale error no longer appears and the newer approval remains actionable.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body and maintainer verification comment describe after-fix live in-app browser proof with a mocked Gateway WebSocket on the rewritten head, showing the stale error no longer appears and the newer approval remains actionable.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: This is a normal-priority Control UI approval state bug with a narrow web UI fix and limited blast radius.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body and maintainer verification comment describe after-fix live in-app browser proof with a mocked Gateway WebSocket on the rewritten head, showing the stale error no longer appears and the newer approval remains actionable.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body and maintainer verification comment describe after-fix live in-app browser proof with a mocked Gateway WebSocket on the rewritten head, showing the stale error no longer appears and the newer approval remains actionable.
Evidence reviewed

PR surface:

Source -4, Tests +225, Docs +1. Total +222 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 1 22 26 -4
Tests 2 225 0 +225
Docs 1 1 0 +1
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 248 26 +222

What I checked:

  • AGENTS.md policy read: Root AGENTS.md and ui/AGENTS.md were read fully; the root changelog policy applies because this PR edits CHANGELOG.md. (AGENTS.md:144, 1a37cafd63b6)
  • Current main stale-error path: Current main writes a global approval error when the original approval id exists anywhere in the queue, which can attach an older failure to a newer active prompt. (ui/src/app/overlays.ts:609, 1a37cafd63b6)
  • Current main queue ordering: New approval entries are unshifted, so a newer approval becomes the visible head of the queue while an older resolve request can still be pending. (ui/src/app/exec-approval.ts:212, 1a37cafd63b6)
  • Visible error surface: The approval component renders the active queue item with the single overlay error field, matching the linked stale-error report. (ui/src/components/exec-approval.ts:208, 1a37cafd63b6)
  • Patch ownership guard: The PR stores the active approval decision operation, validates the current client before applying results, and only shows non-stale failures for the same active approval id. (ui/src/app/overlays.ts:219, 7d625a4e11ae)
  • Regression coverage: The new overlay tests cover a newer active approval receiving an older failure and an old client resolve settling while a new client decision remains busy. (ui/src/app/overlays.test.ts:88, 7d625a4e11ae)

Likely related people:

  • steipete: Current-main blame shows the ui/src/app approval overlay files were introduced in commit 1833c0d, and the PR discussion shows steipete rewrote and verified this head. (role: recent area contributor and verifier; confidence: high; commits: 1833c0d77180, 44f842aa56b7, 7d625a4e11ae; files: ui/src/app/overlays.ts, ui/src/app/exec-approval.ts, ui/src/components/exec-approval.ts)
  • joshavant: Commit 6ade9c added async plugin approval support and related gateway approval methods that underpin the approval flow this UI surface resolves. (role: introduced adjacent approval capability; confidence: medium; commits: 6ade9c474cf1; files: src/gateway/server-methods/plugin-approval.ts, src/gateway/exec-approval-manager.ts, src/infra/exec-approval-forwarder.ts)
  • vincentkoc: Recent approval gateway history includes e085fa and related approval-path changes by Vincent Koc, making this a plausible routing contact for approval runtime context. (role: recent approval/gateway area contributor; confidence: low; commits: e085fa1a3ffd, 0e47ce58bc1b; files: src/gateway/server-methods/exec-approval.ts, src/gateway/exec-approval-manager.ts, src/infra/exec-approval-forwarder.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.
Review history (1 earlier review cycle)
  • reviewed 2026-07-01T04:25:02.894Z sha 580a3e7 :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jul 1, 2026
@steipete steipete self-assigned this Jul 5, 2026
@steipete
steipete force-pushed the codex/fix-approval-error-race branch from 580a3e7 to 7d625a4 Compare July 5, 2026 21:50
@steipete
steipete marked this pull request as ready for review July 5, 2026 21:51
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Maintainer verification complete on rewritten head 7d625a4e11ae92e5e2e48c06bd3e435aba72ebe0.

  • Refactored the stale-error guard into operation ownership covering both the initiating Gateway client and approval id.
  • Added regression coverage for a newer queued approval and for overlapping old/new client decisions across reconnect.
  • Blacksmith Testbox tbx_01kwt3gy25f6vrxsven71k22mz: corepack pnpm test ui/src/app/overlays.test.ts — 2/2 passed.
  • Same Testbox: node scripts/run-vitest.mjs run --config test/vitest/vitest.ui-e2e.config.ts --configLoader runner ui/src/e2e/approval-flow.e2e.test.ts — Chromium 1/1 passed.
  • Same Testbox: corepack pnpm check:changed — passed.
  • Live in-app browser proof with a mocked Gateway WebSocket: after rejecting an older deferred resolve, the newer approval remained visible and actionable with no stale error.
  • Fresh branch autoreview against current origin/main: no accepted/actionable findings, confidence 0.94.
  • git diff --check — passed.

Known proof gaps: none for the touched Control UI surface. Ready for exact-head CI and landing.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 5, 2026
@steipete
steipete merged commit ff44039 into openclaw:main Jul 5, 2026
135 of 149 checks passed
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 6, 2026
* fix(ui): scope approval decision results

Co-authored-by: haruaiclone-droid <[email protected]>

* docs(changelog): credit approval race fix

---------

Co-authored-by: Peter Steinberger <[email protected]>
Co-authored-by: haruaiclone-droid <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app: web-ui App: web-ui P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: M status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Control UI can show stale approval resolve errors on the wrong prompt

2 participants