refactor(session): add unified retry decision metadata#928
Conversation
|
Warning Review limit reached
More reviews will be available in 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request refactors the stream retry decision logic in processor.ts by extracting it into a dedicated module retry-decision.ts. It introduces the buildModelRetryDecision function to structure retry decisions based on technical retryability, safety gate decisions, and timeout policies, and adds accompanying unit tests. The feedback suggests expanding the unit tests to cover other safety gate recommendations, such as offer_continue and ask_user_before_retry, to ensure robust branch coverage.
Summary
Adds a small
retry-decisionmetadata layer for model retry decisions and lightly wiressession.processorto consume it for the existing safe-recovery replay path.This is PR 1 for #925. It does not move retry scheduling into
SessionRetry.policyyet and does not broaden automatic replay behavior.Why
#925 needs a shared representation that keeps these facts separate before the larger retry-pipeline migration:
This PR introduces that metadata without changing the #922 runtime behavior. Review follow-up also renames the aggregate stream counter to
modelStreamAttemptand records blocked-boundary reasoning attempts asreasoning_global_protectedso the metadata matches the actual timeout path.Related Issue
Closes none. Part of #925.
Human Review Status
Pending
Review Focus
Please focus on whether the metadata names and boundaries are future-proof enough for #925 without implying full Run Recovery from #927. In particular, check that shared decision metadata still keeps provider retry attempt count and safe-recovery replay attempt count separate.
Risk Notes
Low behavior risk: the processor still uses the existing retry loop, backoff, timeout behavior, and safe-retry notice behavior. The main risk is naming drift if the metadata is too narrow for the next #925 slices.
Skipped conditional checklist items:
How To Verify
Screenshots or Recordings
Not applicable; no visible UI changed.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.