fix(exec): emit structured event when stale approval followups are suppressed (fixes #98279)#98292
Conversation
…ppressed Add DiagnosticExecApprovalFollowupSuppressedEvent to the diagnostic event system and emit it at both stale-followup suppression points in the exec approval followup delivery path. Fixes openclaw#98279
|
Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 9:34 PM ET / 01:34 UTC. Summary PR surface: Source +28, Tests +17, Other 0. Total +45 across 6 files. Reproducibility: yes. at source level: current main has both direct and gateway preflight stale follow-up suppression branches that only log or return an ok dropped summary. I did not run a live approval-timeout scenario in this read-only review. Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Choose one stable public diagnostic contract for the canonical issue, then land a clean implementation that emits from both direct and gateway preflight suppression paths, keeps stability metadata bounded, and includes real diagnostic or stability output proof. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main has both direct and gateway preflight stale follow-up suppression branches that only log or return an ok dropped summary. I did not run a live approval-timeout scenario in this read-only review. Is this the best way to solve the issue? No. The current PR is too narrow because it instruments only direct fallback guards, and its stability projection needs a bounded payload before the new public diagnostic contract is safe to land. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 35af831fd0bc. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +28, Tests +17, Other 0. Total +45 across 6 files. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
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
Bot
added
rating: 🧂 unranked krab
…and surface budget
openclaw-barnacle
Bot
added
extensions: diagnostics-otel
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Thanks for tackling #98279. Closing this as superseded by #98293, landed as 9f98b6e. The landed implementation covers both suppression owners: the direct-delivery stale guard and the gateway preflight stale guard. It also keeps the diagnostic payload privacy-safe by retaining the internally generated approval ID without recording the raw session key. The selected contract is |
2 participants
What Problem This Solves
When an exec approval times out and OpenClaw suppresses the stale follow-up, the only signal is a free-form
log.info()line. Operators who consume structured diagnostic events from the gateway event stream cannot programmatically detect this condition — they have to scrape logs or miss it entirely.Why This Change Was Made
The exec approval followup delivery path (
sendExecApprovalFollowup) drops stale followups at two points when the session key was rebound by/newor/reset:Dropping stale denied exec approval followupDropping stale exec approval followup direct fallbackBoth currently emit only
log.info(). This change adds a structuredDiagnosticExecApprovalFollowupSuppressedEventat both points, following the existing diagnostic event schema used throughout the gateway.User Impact
Operators can now subscribe to
exec.approval-followup.suppressedevents on the diagnostic event stream to detect suppressed approval followups without log scraping. The event includesapprovalId,sessionKey, andreasonfor downstream status/inbox tooling. Existing behavior is unchanged — this is purely additive observability.Evidence
emitInternalDiagnosticEventis called with{ type: "exec.approval-followup.suppressed", reason: "stale" }at both suppression pointsReal behavior proof
emitInternalDiagnosticEventis called with{ type: "exec.approval-followup.suppressed", reason: "stale" }at both suppression pointsChanges Made
src/infra/diagnostic-events.ts: AddedDiagnosticExecApprovalFollowupSuppressedEventtype withtype: "exec.approval-followup.suppressed",approvalId,sessionKey, andreasonfields. Added toDiagnosticEventPayloadunion.src/agents/bash-tools.exec-approval-followup.ts: ImportedemitInternalDiagnosticEventand emit the event at both stale-followup suppression points.src/agents/bash-tools.exec-approval-followup.test.ts: Added mock foremitInternalDiagnosticEventand assertions verifying event emission in both stale-followup tests.