fix(cron): respect recovered tool errors#84156
Conversation
|
Codex review: found issues before merge. Reviewed June 30, 2026, 4:20 PM ET / 20:20 UTC. Summary PR surface: Source +36, Tests +32. Total +68 across 2 files. Reproducibility: yes. source-level: current main still has a helper test where all payloads are errors and preferred final assistant text exists, yet the last error payload wins and cron finalization derives status from this helper. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Mantis proof suggestion Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Refresh on current main and land a maintainer-approved cron outcome contract that preserves true fatal run-level signals while treating explicitly recovered non-terminal tool errors as diagnostics instead of failed runs. Do we have a high-confidence way to reproduce the issue? Yes, source-level: current main still has a helper test where all payloads are errors and preferred final assistant text exists, yet the last error payload wins and cron finalization derives status from this helper. Is this the best way to solve the issue? No, not as submitted. The resolver is the right layer, but this branch is conflicting and its terminal recovery predicate is broader than current main's warning/metadata contract. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 640258d7b31d. Label changesLabel justifications:
Evidence reviewedPR surface: Source +36, Tests +32. Total +68 across 2 files. View PR surface stats
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 re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
ClawSweeper PR egg ✨ Hatched: 🌱 uncommon Brave Shellbean Hatch commandComment Hatchability rules:
Rarity: 🌱 uncommon. What is this egg doing here?
|
|
Friendly nudge — this PR has been waiting on review since 2026-05-19; ran into another repro of the underlying bug today (full data on #91532 comment). One thing the new repro surfaced that may be worth folding into this PR's scope before merge: the failed cron run had two diagnostics entries with identical text but different sources:
Happy to share the full trajectory + run JSON if useful — just ping. ( |
|
This pull request has been automatically marked as stale due to inactivity. |
|
Closing due to inactivity. |
Summary
isErrortool payloads, even though the agent produced terminal assistant-visible output.resolveCronPayloadOutcomenow recordserrorPayloadRecoveryand treats later payload output, message-delivery warnings, or terminal assistant-visible text as recovery paths when no typed fatal signal exists.meta.error, fatalfailureSignal, denial markers, structured/media delivery payload handling, and real unrecovered trailing errors remain fatal.Motivation
errorbecause an intermediate tool failure wins over the recovered final assistant result.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Real behavior proof (required for external PRs)
okinstead oferrorwhen the only remaining error evidence is recoverable tool payload text.fix/cron-recovered-tool-errorat commit3cb013e77a.{ "hasFatalErrorPayload": false, "status": "ok", "outputText": "Recovered final answer from the cron task.", "deliveryPayloads": [ { "text": "Recovered final answer from the cron task." } ], "errorPayloadRecovery": { "hadErrorPayload": true, "recovered": true, "recoveredBy": "terminalAssistantText" }, "embeddedRunError": null }hasFatalErrorPayloadis false, and the final status derived by cron isok.finalAssistantVisibleTextwas present.Root Cause (if applicable)
resolveCronPayloadOutcomeclassified any lastisErrorpayload as fatal unless a later non-error payload existed, so all-error-payload runs could not be recovered by terminal assistant-visible text.finalAssistantVisibleText, but fatal payload classification ran before that text could act as recovered terminal output.Regression Test Plan (if applicable)
src/cron/isolated-agent.helpers.test.tsisError, final assistant-visible text is present and preferred, no typed fatal signal exists, and cron resolves anokoutcome with recovered final text.User-visible / Behavior Changes
Cron runs that recover from intermediate tool errors can now be recorded as successful and surface the final assistant result instead of the last recoverable tool error.
Diagram (if applicable)
Security Impact (required)
Yes/No): NoYes/No): NoYes/No): NoYes/No): NoYes/No): NoYes, explain risk + mitigation: N/ARepro + Verification
Environment
preferFinalAssistantVisibleText: trueSteps
resolveCronPayloadOutcome.hasFatalErrorPayloadresolves false and output/delivery use final assistant text.Expected
Actual
status: "ok",recoveredBy: "terminalAssistantText", and no embedded run error.Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm test; focused tests andpnpm build/pnpm checkwere run instead.codex review --base origin/main; the installed Codex CLI does not expose areviewsubcommand, and a read-onlycodex execreview attempt failed with401 Unauthorized.AI-assisted: yes. I understand the changed classifier path and the added tests.
Validation run locally:
Review Conversations
No bot review conversations were open when this PR body was updated.
Compatibility / Migration
Yes/No): YesYes/No): NoYes/No): NoRisks and Mitigations
meta.error, fatalfailureSignal, denial markers, structured delivery payloads, or an unrecovered real trailing error should stay fatal.