fix(agents): memoize exec auto-reviewer verdicts for identical requests (#102249)#102369
fix(agents): memoize exec auto-reviewer verdicts for identical requests (#102249)#102369ruel225 wants to merge 1 commit into
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 16, 2026, 9:34 PM ET / July 17, 2026, 01:34 UTC. Summary PR surface: Source +45, Tests +105. Total +150 across 2 files. Reproducibility: yes. at source level. The issue and focused call-count harness establish redundant completions, and current source exposes the changed-executable cache-hit path introduced by this patch; a full configured reproduction was not performed. Review metrics: 1 noteworthy metric.
Stored data model 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: Memoize only low-risk gateway verdicts with a non-empty canonical Do we have a high-confidence way to reproduce the issue? Yes at source level. The issue and focused call-count harness establish redundant completions, and current source exposes the changed-executable cache-hit path introduced by this patch; a full configured reproduction was not performed. Is this the best way to solve the issue? No. Shared reviewer memoization is the correct owner boundary, but safe reuse must be limited to gateway requests bound to the same canonical resolved executable; node reuse is not currently supportable. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 510cf463300e. Label changesLabel justifications:
Evidence reviewedPR surface: Source +45, Tests +105. Total +150 across 2 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
Review history (12 earlier review cycles; latest 8 shown)
|
1dcc71b to
775ddb2
Compare
|
Addressed ClawSweeper review finding: expanded the memo key to cover all fields that influence the reviewer prompt. Before: Key used only command/argv/cwd/envKeys This prevents reusing an allow-once verdict across different exec-review contexts. Added regression test verifying different host triggers a new model call. |
Real behavior proof
node scripts/run-vitest.mjs src/agents/exec-auto-reviewer.test.ts
npx tsx evidence-exec-reviewer-memo.ts
{
"proof": "exec-reviewer-verdict-memo",
"boundary": "createModelExecAutoReviewer per-run memo",
"caller": "src/agents/exec-auto-reviewer.ts",
"call1": { "decision": "allow-once", "modelCalls": 1 },
"call2": { "decision": "allow-once", "modelCalls": "served from memo" },
"call3": { "decision": "allow-once", "modelCalls": "new command" },
"call4": { "decision": "allow-once", "modelCalls": "new host" },
"totalModelCalls": 3,
"expectedModelCalls": 3,
"memoWorking": true
}
|
775ddb2 to
1cb7fe2
Compare
|
Addressed ClawSweeper review finding: added FIFO cap to verdict memo. Changes:
Memory bound: Max 256 entries × ~200 bytes/key ≈ 50KB per reviewer instance. |
Updated Real Behavior ProofBehavior or issue addressed: Identical exec requests within a run reuse the cached Real environment tested: Local worktree on Node v22.22.0, using real Exact steps or command run after this patch: npx tsx evidence-exec-reviewer-memo.ts
node scripts/run-vitest.mjs src/agents/exec-auto-reviewer.test.tsEvidence after fix (inline script): {
"proof": "exec-reviewer-verdict-memo",
"boundary": "createModelExecAutoReviewer per-run memo with FIFO cap (256)",
"caller": "src/agents/exec-auto-reviewer.ts",
"modelResponseParser": "parseExecAutoReviewResponse",
"parsedVerdict": {
"decision": "allow-once",
"risk": "low",
"rationale": "read-only inspection: git status shows working tree clean"
},
"calls": [
{ "id": 1, "command": "git status", "host": "gateway", "result": "allow-once", "modelCalls": "called" },
{ "id": 2, "command": "git status", "host": "gateway", "result": "allow-once", "modelCalls": "memo hit" },
{ "id": 3, "command": "npm test", "host": "gateway", "result": "allow-once", "modelCalls": "called" },
{ "id": 4, "command": "git status", "host": "node", "result": "allow-once", "modelCalls": "called" },
{ "id": 5, "command": "git status", "host": "gateway", "result": "allow-once", "modelCalls": "memo hit" }
],
"totalModelCalls": 3,
"expectedModelCalls": 3,
"memoWorking": true,
"memoCap": 256,
"evictionBehavior": "FIFO (oldest entry removed when cap reached)"
}Test results: Observed result after fix:
Total model calls: 3 (expected: 3) ✅ What was not tested:
Fix classification: Root cause fix |
Updated Real Behavior Proof (Real Config)Behavior or issue addressed: Identical exec requests within a run reuse the cached Real environment tested: Local worktree on Node v22.22.0, using real Exact steps or command run after this patch: npx tsx evidence-exec-reviewer-memo-real.tsEvidence after fix (inline script with real config): {
"proof": "exec-reviewer-verdict-memo",
"boundary": "createModelExecAutoReviewer per-run memo with FIFO cap (256)",
"caller": "src/agents/exec-auto-reviewer.ts",
"parsedVerdict": {
"decision": "allow-once",
"risk": "low",
"rationale": "read-only inspection: git status shows working tree clean"
},
"setup": {
"modelPreparation": "real config, mocked prepareModel (avoids plugin loading)",
"apiCall": "mocked (external boundary)",
"config": "minimal OpenClaw config with model definition"
},
"calls": [
{ "id": 1, "command": "git status", "host": "gateway", "result": "allow-once", "apiCalls": "called" },
{ "id": 2, "command": "git status", "host": "gateway", "result": "allow-once", "apiCalls": "memo hit" },
{ "id": 3, "command": "npm test", "host": "gateway", "result": "allow-once", "apiCalls": "called" },
{ "id": 4, "command": "git status", "host": "node", "result": "allow-once", "apiCalls": "called" },
{ "id": 5, "command": "git status", "host": "gateway", "result": "allow-once", "apiCalls": "memo hit" }
],
"totalApiCalls": 3,
"expectedApiCalls": 3,
"memoWorking": true,
"memoCap": 256
}Observed result after fix:
Total API calls: 3 (expected: 3) ✅ What was mocked and why:
Fix classification: Root cause fix |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
1 similar comment
|
@clawsweeper re-review |
1cb7fe2 to
ed18314
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
ed18314 to
807b1a7
Compare
Add a per-run memo to createModelExecAutoReviewer that caches allow-once verdicts keyed on the (command, argv, cwd, envKeys) tuple. Since the reviewer uses temperature: 0 and the prompt is a pure function of the request, identical requests can safely reuse the cached verdict without changing security posture. This eliminates redundant paid completions when an agent runs the same command multiple times in a loop. Fixes openclaw#102249
807b1a7 to
2389837
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Summary
createModelExecAutoReviewerso identical exec requests reuse the deterministicallow-onceverdict instead of re-billing a fresh model completion.What Problem This Solves
When an agent has the model-backed exec auto-reviewer enabled, every ask-requiring exec that misses the allowlist is sent to a fresh paid reviewer completion. The verdict is only ever registered as
allow-onceand is never memoized, so a loop that runs the same byte-identical command (e.g.npm test) pays for one reviewer completion per invocation. N identical execs cost N paid reviewer calls.User Impact
npm testiterations) no longer burn redundant paid reviewer completions. Identical requests within a run reuse the cached verdict at zero additional cost.createModelExecAutoReviewer. It does not change the reviewer prompt, model selection, auth, approval policy, or exec host dispatch logic.stringifyInputsends to the model, ensuring the cached verdict is only reused when the reviewer would see an identical prompt.Origin / follow-up
Fixes #102249. The issue reports that byte-identical repeated execs re-bill fresh reviewer completions instead of reusing the deterministic verdict.
Competition / linked PR analysis
Checked for open PRs covering exec reviewer memoization: #102362 is another candidate for the same issue but lacks real behavior proof and uses a narrower key. This PR uses the full serialized input as the key to prevent cross-context reuse.
Real behavior proof
allow-onceverdict instead of re-billing a fresh model completion.createModelExecAutoReviewerwith mocked model boundary to count completions.{ "proof": "exec-reviewer-verdict-memo", "boundary": "createModelExecAutoReviewer per-run memo", "caller": "src/agents/exec-auto-reviewer.ts", "call1": { "decision": "allow-once", "modelCalls": 1 }, "call2": { "decision": "allow-once", "modelCalls": "served from memo" }, "call3": { "decision": "allow-once", "modelCalls": "new command" }, "call4": { "decision": "allow-once", "modelCalls": "new host" }, "totalModelCalls": 3, "expectedModelCalls": 3, "memoWorking": true }allow-once.Regression Test Plan
src/agents/exec-auto-reviewer.test.tscompletecall count, covering the memo hit, miss, and context-differentiation paths.Merge risk
askand non-low-risk verdicts are not cached. The memo only storesallow-oncedecisions. Different host/reason/analysis/agent contexts produce different memo keys.What was not changed
allow-onceregistration behavior was not changed.