fix(approvals): distinguish expired, already-resolved, and unknown approval ids by Hidetsugu55 · Pull Request #92376 · openclaw/openclaw

Hidetsugu55 · 2026-06-12T07:23:07Z

Resolving an exec/plugin approval that was not currently pending previously collapsed three different situations — the approval expired, it was already resolved, or the id was never seen — into a single unknown or expired approval id error. Operators could not tell whether to re-run the command (expired) or whether their decision had already landed (resolved), which is one of the concrete failure modes behind "Failed to submit approval: unknown or expired approval id".

What changed

ExecApprovalManager keeps a bounded FIFO archive of terminated records (512 entries) and exposes classifyApprovalId(), which respects caller visibility so it never leaks the existence of approvals the caller could not otherwise see.
handleApprovalResolve now reports an elapsed window as approval expired (reason: APPROVAL_EXPIRED, with remediation) both inside the resolved-grace window and after the record is archived, and reports a post-grace decided approval as approval already resolved. Genuinely unknown ids keep the existing unknown or expired approval id message. Same-decision idempotent retries during the grace window are unchanged.
Adds isApprovalExpiredError / isApprovalAlreadyResolvedError detectors alongside the existing isApprovalNotFoundError.

Tests

pnpm exec vitest run --config test/vitest/vitest.gateway.config.ts src/gateway/exec-approval-manager.test.ts src/gateway/server-methods/approval-shared.test.ts — classify pending/resolved/expired/unknown, visibility filtering, post-grace archive, FIFO eviction; in-grace and post-grace expired vs already-resolved resolution.
pnpm exec vitest run --config vitest.config.ts src/infra/approval-errors.test.ts — new detectors.
./node_modules/.bin/oxfmt --check on all changed files.

Real behavior proof

Behavior or issue addressed: Submitting an approval that is no longer pending returned the same unknown or expired approval id whether it had expired, had already been resolved, or never existed, so the operator had no idea whether re-running the command would help. This change makes an expired approval report approval expired (with an APPROVAL_EXPIRED reason and remediation) distinctly from a genuinely unknown id, including after the in-memory grace window has elapsed and the record has moved to the bounded archive.

Real environment tested: A real OpenClaw gateway runtime started from this branch's worktree on macOS (Darwin, Apple Silicon), exercising the genuine gateway server, ExecApprovalManager, and the exec.approval.request / exec.approval.resolve server methods over a websocket operator connection. No model backend is involved — approvals are created and resolved directly through the real gateway methods.

Exact steps or command run after this patch: A standalone operator client connects to the in-process gateway and calls exec.approval.request (with a short or long timeoutMs) to register a real approval, lets the short one elapse, waits past the 15s resolved-grace window so the record is archived, then calls exec.approval.resolve for: an expired-but-in-grace id, an expired-and-archived id, and a never-issued id, capturing each error.

Evidence after fix: Live error responses returned by the running gateway's exec.approval.resolve:

# expired, still inside the 15s grace window
{"ok":false,"error":{"message":"approval expired","reason":"APPROVAL_EXPIRED","code":"INVALID_REQUEST"}}

# expired, AFTER the grace window (record served from the bounded terminal archive)
request("echo expired-archived", timeout=1200) -> id=ff40df94-4ee1-467a-bf4c-1759e3f13aaa
expired post-grace -> {"ok":false,"error":{"message":"approval expired","reason":"APPROVAL_EXPIRED","code":"INVALID_REQUEST"}}

# genuinely unknown id
scenario 4: unknown -> {"ok":false,"error":{"message":"unknown or expired approval id","reason":"APPROVAL_NOT_FOUND","code":"INVALID_REQUEST"}}

Observed result after fix: An expired approval returns approval expired / APPROVAL_EXPIRED both within the grace window and after the record is archived, while a never-issued id still returns unknown or expired approval id / APPROVAL_NOT_FOUND. Before this patch all of these returned the same unknown or expired approval id. The remediation text now tells the operator to re-run the command to request a fresh approval.

What was not tested: The already-resolved-after-grace path (approval already resolved) was exercised through the unit suite rather than this live run, because a gateway approval with no interactive delivery route is expired by the gateway before it can be decided in this headless harness; the resolved-vs-expired classification itself is covered by exec-approval-manager.test.ts and approval-shared.test.ts. No third-party model or channel was used.

…proval ids Resolving an exec/plugin approval that was not pending previously collapsed expired, already-resolved, and never-seen ids into a single "unknown or expired approval id" error, so operators could not tell whether to re-run the command or whether their decision had already landed. - ExecApprovalManager keeps a bounded FIFO archive of terminated records and exposes classifyApprovalId(), which respects caller visibility so it never leaks the existence of approvals the caller could not see. - handleApprovalResolve now reports an elapsed window as "approval expired" (reason APPROVAL_EXPIRED, with remediation) both inside the grace window and after the record is archived, and reports a post-grace decided approval as "approval already resolved". Genuinely unknown ids still return the existing message. Same-decision idempotent retries are unchanged. - Add isApprovalExpiredError / isApprovalAlreadyResolvedError detectors. Tests: exec-approval-manager (classify pending/resolved/expired/unknown, visibility, post-grace archive, FIFO eviction), approval-shared (in-grace and post-grace expired vs already-resolved), approval-errors detectors.

clawsweeper · 2026-06-12T07:24:55Z

Codex review: needs changes before merge. Reviewed June 30, 2026, 7:29 PM ET / 23:29 UTC.

Summary
The PR changes gateway approval resolution to distinguish expired, already-resolved, and unknown approval ids, adds approval error detectors, and expands gateway/infra tests.

PR surface: Source +154, Tests +272. Total +426 across 6 files.

Reproducibility: yes. Source inspection of current main shows stale cleanup consumers only recognize isApprovalNotFoundError, while the proposed gateway responses add distinct expired/already-resolved terminal reasons.

Review metrics: 2 noteworthy metrics.

Approval terminal reasons: 2 added, 1 existing classifier unchanged. The gateway would emit two new terminal reasons that current plugin SDK and channel cleanup code do not classify deliberately.
Terminal archive payload: 1 full-record archive added. The archive extends retention of approval request content beyond prior cleanup, so data minimization matters before merge.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦞 diamond lobster
Patch quality: 🦐 gold shrimp
Result: needs maintainer review before merge.

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

Rank-up moves:

Migrate or centralize terminal approval error classification for affected channel/plugin consumers.
Reduce the terminal archive to classification/visibility metadata rather than complete approval request records.

Mantis proof suggestion
A native Telegram stale approval callback recording would directly show whether the new terminal reasons still clear or update stale approval buttons correctly. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis telegram desktop proof: verify that an expired or already-resolved approval callback shows the intended stale-approval message and clears the inline approval buttons.

Risk before merge

[P1] Existing Telegram, WhatsApp, Signal, and iMessage stale approval cleanup still keys on isApprovalNotFoundError, so the new terminal reasons can bypass stale button/reaction cleanup until consumers migrate to a canonical terminal classifier.
[P1] The terminal archive keeps full exec/plugin approval records for up to 512 terminated approvals beyond the old grace cleanup path, extending in-memory retention of command and plugin request content.
[P1] The approval error taxonomy is consumed through plugin SDK/channel code, so compatibility and visible callback cleanup need explicit handling before merge even with green CI.

Maintainer options:

Migrate consumers and minimize archive (recommended)
Update SDK/channel stale-terminal handling and reduce the archive to id, terminal state, and visibility metadata before merging the gateway taxonomy change.
Accept split taxonomy temporarily
Maintainers could intentionally ship the gateway reasons first, but they would own stale callback cleanup drift and a follow-up SDK/channel migration.
Pause for replacement PR
If the source branch cannot be safely repaired, keep this useful patch as source material for a narrower maintainer replacement that includes the consumer migration.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Add a canonical stale-terminal approval detector/export, migrate Telegram, WhatsApp, Signal, and equivalent cleanup tests to handle APPROVAL_EXPIRED and APPROVAL_ALREADY_RESOLVED while preserving exec-to-plugin fallback only for APPROVAL_NOT_FOUND, and store only minimal terminal id/state/visibility metadata in ExecApprovalManager's archive.

Next step before merge

[P2] A focused repair can preserve the useful gateway taxonomy while completing SDK/channel migration and data-minimized archive storage.

Security
Needs attention: The diff has a concrete data-minimization concern because the new archive retains full approval request payloads longer than the previous cleanup window.

Review findings

[P1] Handle the new terminal reasons in cleanup — src/gateway/server-methods/approval-shared.ts:108-124
[P2] Store only terminal approval metadata — src/gateway/exec-approval-manager.ts:86

Review details

Best possible solution:

Keep the finer gateway taxonomy, but merge only after terminal approval errors are classified through one cleanup-compatible API and the terminal archive stores only minimal state/visibility metadata.

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

Yes. Source inspection of current main shows stale cleanup consumers only recognize isApprovalNotFoundError, while the proposed gateway responses add distinct expired/already-resolved terminal reasons.

Is this the best way to solve the issue?

No, not as submitted. Distinct gateway reasons are the right direction, but the best fix must also migrate stale-approval consumers and avoid retaining full approval payloads in the terminal archive.

Full review comments:

[P1] Handle the new terminal reasons in cleanup — src/gateway/server-methods/approval-shared.ts:108-124
This adds APPROVAL_EXPIRED and APPROVAL_ALREADY_RESOLVED responses, but existing channel stale approval cleanup still branches on isApprovalNotFoundError. Export/use a canonical terminal approval classifier so expired or already-resolved callbacks clear controls or unregister reaction targets without broadening exec-to-plugin fallback beyond true not-found errors.
Confidence: 0.97
[P2] Store only terminal approval metadata — src/gateway/exec-approval-manager.ts:86
recentlyTerminated stores complete approval records even though classification only needs id, terminal state, and caller-visibility metadata. Full exec/plugin payloads can include command text, argv, cwd, titles, and descriptions, so the archive should retain only the minimal metadata needed after grace cleanup.
Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

P2: This is a bounded approval-resolution UX and cleanup fix with real gateway/plugin impact but limited blast radius.
merge-risk: 🚨 compatibility: The PR changes terminal approval error behavior consumed by existing plugin SDK and channel cleanup paths.
merge-risk: 🚨 security-boundary: The new terminal archive retains full approval records that can include command and plugin request content beyond the old cleanup window.
rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body includes after-fix live gateway output from a real macOS runtime showing expired and unknown approval classifications; already-resolved is covered by unit tests rather than the live run.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix live gateway output from a real macOS runtime showing expired and unknown approval classifications; already-resolved is covered by unit tests rather than the live run.
mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The PR can affect visible Telegram stale approval callback cleanup because Telegram currently handles stale callback errors through isApprovalNotFoundError.

Evidence reviewed

PR surface:

Source +154, Tests +272. Total +426 across 6 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	3	163	9	+154
Tests	3	273	1	+272
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	6	436	10	+426

Security concerns:

[medium] Full approval payload retention — src/gateway/exec-approval-manager.ts:86
The terminal archive keeps complete approval records even though classification only needs terminal state and caller visibility fields; exec and plugin approval payloads can contain sensitive commands, descriptions, and session metadata.
Confidence: 0.88

Acceptance criteria:

[P1] node scripts/run-vitest.mjs src/gateway/exec-approval-manager.test.ts src/gateway/server-methods/approval-shared.test.ts src/infra/approval-errors.test.ts src/plugins/contracts/plugin-sdk-subpaths.test.ts.
[P1] node scripts/run-vitest.mjs extensions/telegram/src/bot.test.ts extensions/whatsapp/src/approval-reactions.test.ts extensions/signal/src/approval-reactions.test.ts.
[P1] git diff --check.

What I checked:

Repository policy applied: Root AGENTS.md and scoped gateway/channel/plugin guidance were read fully; they require whole-surface review for gateway, plugin SDK, channel, compatibility, and security-sensitive approval changes. (AGENTS.md:10, a841c2788202)
PR introduces new terminal response shapes: The patch adds approval expired and approval already resolved response helpers and consults classifyApprovalId() after pending/resolved lookup misses. (src/gateway/server-methods/approval-shared.ts:108, 6bab93dca6f3)
Existing classifier does not cover new terminal reasons: Current main's shared classifier only matches structured APPROVAL_NOT_FOUND or the legacy unknown/expired message, so the proposed APPROVAL_EXPIRED and APPROVAL_ALREADY_RESOLVED reasons are separate from existing stale-cleanup classification. (src/infra/approval-errors.ts:23, a841c2788202)
Plugin SDK exports only the old classifier: The plugin-facing error runtime currently exports isApprovalNotFoundError but not a terminal approval classifier that channel plugins can use for expired/already-resolved cleanup. (src/plugin-sdk/error-runtime.ts:26, a841c2788202)
Stale callback consumers key on approval-not-found: Telegram, WhatsApp, Signal, and iMessage stale approval cleanup paths branch on isApprovalNotFoundError, so new non-not-found terminal errors can bypass button/reaction cleanup unless those consumers are migrated. (extensions/telegram/src/bot-handlers.runtime.ts:2718, a841c2788202)
Archive retains full records: The proposed recentlyTerminated map stores complete ExecApprovalRecord objects, while classification only needs id, terminal state, and visibility metadata. (src/gateway/exec-approval-manager.ts:86, 6bab93dca6f3)

Likely related people:

gumadeiras: PR history shows core: dedupe approval not-found handling introduced the shared approval-not-found classifier and plugin SDK export that this PR now needs to extend or preserve alongside a terminal classifier. (role: shared classifier introducer; confidence: high; commits: e627f53d24a9; files: src/infra/approval-errors.ts, src/plugin-sdk/error-runtime.ts)
steipete: Git history shows the shared gateway auth/approval helper refactor and earlier approval security matching work on the same approval manager/server-method boundary. (role: gateway approval helper contributor; confidence: medium; commits: bb01e49192d3, 92eb3dfc9d25, 03e689fc89bb; files: src/gateway/server-methods/approval-shared.ts, src/gateway/exec-approval-manager.ts)
pgondhi987: The merged requester-metadata approval access PR is directly relevant to the archive visibility filter this PR reuses for terminal approval classification. (role: approval visibility contributor; confidence: medium; files: src/gateway/server-methods/approval-shared.ts, src/gateway/exec-approval-manager.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-06-21T13:04:42.930Z sha 6bab93d :: needs changes before merge. :: [P1] Handle the new terminal reasons in cleanup | [P2] Store only terminal approval metadata

steipete · 2026-07-01T04:39:22Z

Maintainer sequencing note after #66685 landed as d68ba5e: sendExecApprovalFollowupResult(...) now suppresses warnings only when isApprovalNotFoundError(...) matches the currently collapsed terminal lifecycle error.

This PR splits that contract into APPROVAL_EXPIRED, already-resolved, and genuinely unknown cases. Before landing, please preserve the benign follow-up boundary for the new expired/already-resolved shapes (for example, by extending the shared boundary to the new detectors) and add focused assertions that those terminal errors do not warn while unrelated transport failures still do. Otherwise this PR would reintroduce exec approval followup dispatch failed noise for the newly distinct terminal codes.

openclaw-barnacle · 2026-07-21T04:35:47Z

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

openclaw-barnacle Bot added gateway Gateway runtime size: M proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 12, 2026

clawsweeper Bot had a problem deploying to qa-live-shared June 12, 2026 07:36 Failure

clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 12, 2026

clawsweeper Bot mentioned this pull request Jun 14, 2026

Approvals lost on gateway restart — stale callback buttons show confusing error #64664

Open

clawsweeper Bot added the merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. label Jun 15, 2026

This was referenced Jun 19, 2026

fix(approvals): handle stale plugin waits #88203

Merged

Stale plugin approval waits should fail closed instead of failing agent turns #88111

Closed

Suppress expired exec approval followup warnings #66685

Merged

openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 21, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(approvals): distinguish expired, already-resolved, and unknown approval ids#92376

fix(approvals): distinguish expired, already-resolved, and unknown approval ids#92376
Hidetsugu55 wants to merge 1 commit into
openclaw:mainfrom
Hidetsugu55:hide/fix/approval-expired-resolved-ux

Hidetsugu55 commented Jun 12, 2026

Uh oh!

clawsweeper Bot commented Jun 12, 2026 •

edited

Loading

Uh oh!

steipete commented Jul 1, 2026

Uh oh!

openclaw-barnacle Bot commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

Hidetsugu55 commented Jun 12, 2026

What changed

Tests

Real behavior proof

Uh oh!

clawsweeper Bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steipete commented Jul 1, 2026

Uh oh!

openclaw-barnacle Bot commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clawsweeper Bot commented Jun 12, 2026 •

edited

Loading