Skip to content

[codex] Tighten transient retry policy suppression - #5025

Merged
PerishCode merged 1 commit into
mainfrom
codex/retry-policy-transient-suppression
Jul 2, 2026
Merged

[codex] Tighten transient retry policy suppression#5025
PerishCode merged 1 commit into
mainfrom
codex/retry-policy-transient-suppression

Conversation

@PerishCode

Copy link
Copy Markdown
Contributor

Summary

  • Tighten transient retry policy handling so suppressed transient failures do not over-trigger retries.
  • Update retry analytics contract coverage for the adjusted suppression behavior.
  • Extend daemon retry-policy tests for the transient suppression cases.

Validation

  • Not run locally; branch was inspected for a clean merge against latest main before opening the PR.

@lefarcen
lefarcen requested a review from mrcfps July 1, 2026 15:37
@lefarcen lefarcen added size/M PR changes 100-300 lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/bugfix Bug fix needs-validation Runtime change detected; needs human or /explore agent validation. labels Jul 1, 2026
@lefarcen

lefarcen commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Quick follow-up: the Summary already covers the why / user-visible effect pretty well, but could you add the Surface area checklist and tick the right box (or None if this is truly internal-only)?

Also marking this for QA early since a retry-policy regression here would affect a live run path; nothing needed from you yet — we'll use the label as the reminder to route QA once the PR is merge-ready.

@mrcfps mrcfps left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PerishCode I reviewed the retry-policy changes across apps/daemon/src/run-retry-policy.ts, the focused daemon retry-policy tests, and the analytics suppressed-reason contract. The new allowlist is consistent with the classifier detail values I checked, and the added cases cover the main suppression paths introduced here. Nice work tightening this behavior while keeping the telemetry reason codes explicit.

Note: I attempted to run pnpm --filter @open-design/daemon test -- run-retry-policy.test.ts, but this reviewer worktree does not have dependencies installed (vitest was not found).

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@lefarcen
lefarcen requested a review from AmyShang-alt July 1, 2026 15:55
@lefarcen

lefarcen commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

One PR-body follow-up before the final pass: could you add the Surface area checklist and a short Bug fix verification note describing the repro / regression seam this change covers?

The Summary section already does the Why / user-visible-effect part, so no heading rename needed.

AmyShang-alt

This comment was marked as off-topic.

@AmyShang-alt AmyShang-alt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA validation update: I re-checked the retry/billing safety boundaries locally on head db05ef4.

Validated commands:

  • pnpm --dir apps/daemon exec vitest run tests/run-retry-policy.test.ts -> passed
  • pnpm --dir apps/daemon typecheck -> passed

I also added temporary local boundary cases to exercise the concern directly:

  • rate_limit + hard_quota + retryable=true still suppresses retry with hard_quota
  • newly-allowed process_exit/fatal_rpc_error + retryable=true is still suppressed by userVisibleOutputSeen, toolCallSeen, artifactWriteSeen, and liveArtifactSeen

Those expanded local tests passed as 21/21. My previous request-changes review was too strong for this result; the existing policy gates do cover the retry/cost edge I was worried about. Approving based on this validation.

@AmyShang-alt AmyShang-alt added validated Runtime change validated (via /explore Pass or manual QA). and removed needs-validation Runtime change detected; needs human or /explore agent validation. labels Jul 2, 2026
@AmyShang-alt
AmyShang-alt added this pull request to the merge queue Jul 2, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 2, 2026
@PerishCode
PerishCode added this pull request to the merge queue Jul 2, 2026
Merged via the queue into main with commit 4f15da1 Jul 2, 2026
31 checks passed
xxiaoxiong pushed a commit to xxiaoxiong/open-design that referenced this pull request Jul 9, 2026
Co-authored-by: Siri-Ray <2667192167@qq.com>
xxiaoxiong pushed a commit to xxiaoxiong/open-design that referenced this pull request Jul 13, 2026
Co-authored-by: Siri-Ray <2667192167@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/M PR changes 100-300 lines type/bugfix Bug fix validated Runtime change validated (via /explore Pass or manual QA).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants