Skip to content

refactor(session/retry): single-pass classify(); policy() no longer double-calls transportMessage#8

Closed
tesdal wants to merge 2 commits intophase-ab-basefrom
audit-f5-retry-consolidation
Closed

refactor(session/retry): single-pass classify(); policy() no longer double-calls transportMessage#8
tesdal wants to merge 2 commits intophase-ab-basefrom
audit-f5-retry-consolidation

Conversation

@tesdal
Copy link
Copy Markdown
Owner

@tesdal tesdal commented Apr 24, 2026

Summary

  • Addresses audit finding F5 (Opus diamond review, 2026-04-22): policy() previously called retryable(error) (which internally called transportMessage) and then called transportMessage(error) again on the next line to decide whether to apply TRANSPORT_RETRY_CAP.
  • Consolidates classification into a single classify() returning { message, isTransport? }. retryable() is kept as a thin wrapper (one-liner) to avoid churning the existing retry.test.ts suite; prefer classify() in new code.
  • Preserves legacy cap behavior for plain-text errors whose message matches BOTH a rate-limit phrase AND a transport pattern: classify() still picks the rate-limit message text but sets isTransport: true so policy() applies TRANSPORT_RETRY_CAP as before.

Test plan

New file: packages/opencode/test/session/retry-classification.test.ts — 7 tests:

  • ContextOverflowError → undefined
  • SSEStallError → {message, isTransport: true}
  • APIError 503 → non-transport message
  • retryable() legacy wrapper still returns string for transport errors
  • Plain-text ETIMEDOUT → isTransport: true
  • Mixed rate limit (ETIMEDOUT during retry) → rate-limit message and isTransport: true (regression)
  • APIError 529 "Overloaded" → {message: "Provider is overloaded"} without isTransport

Results from packages/opencode/:

  • bun test test/session/retry-classification.test.ts: 7 pass, 0 fail
  • bun test test/session/retry: 44 pass, 0 fail
  • bun typecheck: clean

Review history

Diamond review (2026-04-24) before push:

  • codex-5.3 spec review: REQUEST_CHANGES → flagged missing cap equivalence for mixed rate-limit + transport messages. Fixed.
  • Opus quality review: REQUEST_CHANGES → flagged misleading retryable() comment (TUI only uses the constant, not the function), redundant isTransport: false literals, missing plain-text transport test. All addressed.

Both reviewers re-evaluated via rework-in-place pass. This PR is review-only on the fork for Copilot feedback; merges to integration via --no-ff locally.

Base

Branches off phase-ab-base which tracks local/integration-v2 at commit 257bdb28b (F4 merged).

@tesdal
refactor(session/retry): single-pass classify(); policy() no longer d…
389e265 
…ouble-calls transportMessage

policy() previously called retryable(error) (which internally called
transportMessage) and then called transportMessage(error) again on the
next line to decide whether to apply TRANSPORT_RETRY_CAP. Consolidates
to a single classify() returning { message, isTransport? }; retryable()
becomes a thin wrapper kept to avoid churning the existing retry.test.ts
suite.

Preserves legacy cap behavior for plain-text errors whose message matches
BOTH a rate-limit phrase AND a transport pattern: classify() still picks
the rate-limit message text but sets isTransport so policy() applies
TRANSPORT_RETRY_CAP as before.

Adds retry-classification.test.ts with 7 cases including the mixed
rate-limit + ETIMEDOUT regression test.

Addresses audit finding F5 (Opus diamond review, 2026-04-22).
@tesdal tesdal requested a review from Copilot April 24, 2026 10:32
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 24, 2026

Hey! Your PR title refactor(session/retry): single-pass classify(); policy() no longer double-calls transportMessage doesn't follow conventional commit format.

Please update it to start with one of:

  • feat: or feat(scope): new feature
  • fix: or fix(scope): bug fix
  • docs: or docs(scope): documentation changes
  • chore: or chore(scope): maintenance tasks
  • refactor: or refactor(scope): code refactoring
  • test: or test(scope): adding or updating tests

Where scope is the package name (e.g., app, desktop, opencode).

See CONTRIBUTING.md for details.

Copilot AI reviewed Apr 24, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors session retry classification to avoid double-calling transportMessage() inside retry policy evaluation, addressing audit finding F5 while preserving legacy retry-cap semantics.

Changes:

  • Introduces SessionRetry.classify() to perform single-pass retry classification and transport detection.
  • Keeps retryable() as a compatibility wrapper returning only the message string for existing callers/tests.
  • Adds a dedicated test suite for classification behavior, including mixed rate-limit + transport cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
packages/opencode/src/session/retry.ts Adds classify() and updates policy() to use it, eliminating redundant transport classification calls.
packages/opencode/test/session/retry-classification.test.ts New tests covering classify() output, legacy retryable() behavior, and mixed rate-limit/transport regression case.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/opencode/test/session/retry-classification.test.ts Outdated
Comment on lines +17 to +22
test("returns non-transport classification for APIError 500", () => {
const err = new MessageV2.APIError({
message: "upstream exploded",
statusCode: 503,
isRetryable: false,
}).toObject()
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

Test name says "APIError 500" but the constructed error uses statusCode: 503. Rename the test description to match the actual status code (or make it generic like "APIError 5xx") to avoid confusion when debugging failures.

Copilot uses AI. Check for mistakes.
@tesdal
test(session/retry): rename APIError test to 5xx to match 503 status
42e4dac 
Copilot review feedback — test name said '500' but constructed error uses
statusCode 503.

Addresses audit finding F5 (Opus diamond review, 2026-04-22).
@tesdal tesdal requested a review from Copilot April 24, 2026 10:46
Copilot AI reviewed Apr 24, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tesdal
Copy link
Copy Markdown
Owner Author

tesdal commented Apr 24, 2026

Closing — review-only PR on fork. Diamond (codex-5.3 + Opus) returned REQUEST_CHANGES; all findings applied. Copilot round 1 flagged one test-name nit (APIError 500 → 5xx, fixed in 42e4dac). Copilot round 2 clean. Merging to local/integration-v2 via --no-ff.

@tesdal tesdal closed this Apr 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

needs:title

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tesdal