fix(openai): sync OAuth context and retry fixes#302
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
🧰 Additional context used📓 Path-based instructions (2)packages/opencode/**/*.ts📄 CodeRabbit inference engine (packages/opencode/AGENTS.md)
Files:
packages/opencode/test/**/*.test.{ts,tsx}📄 CodeRabbit inference engine (packages/opencode/test/AGENTS.md)
Files:
🧠 Learnings (29)📓 Common learnings📚 Learning: 2026-04-28T14:46:33.806ZApplied to files:
📚 Learning: 2026-04-21T16:00:44.910ZApplied to files:
📚 Learning: 2026-04-24T06:50:05.142ZApplied to files:
📚 Learning: 2026-04-28T04:38:11.771ZApplied to files:
📚 Learning: 2026-04-28T08:29:02.858ZApplied to files:
📚 Learning: 2026-04-28T04:56:13.350ZApplied to files:
📚 Learning: 2026-04-28T07:27:49.810ZApplied to files:
📚 Learning: 2026-04-28T14:46:41.449ZApplied to files:
📚 Learning: 2026-04-28T04:56:21.528ZApplied to files:
📚 Learning: 2026-04-28T04:37:56.001ZApplied to files:
📚 Learning: 2026-04-27T08:58:00.665ZApplied to files:
📚 Learning: 2026-04-28T12:49:09.792ZApplied to files:
📚 Learning: 2026-04-27T12:59:45.694ZApplied to files:
📚 Learning: 2026-04-28T11:24:35.312ZApplied to files:
📚 Learning: 2026-04-28T04:56:21.338ZApplied to files:
📚 Learning: 2026-04-28T05:36:24.561ZApplied to files:
📚 Learning: 2026-04-27T11:18:47.332ZApplied to files:
📚 Learning: 2026-04-27T11:19:24.963ZApplied to files:
📚 Learning: 2026-04-25T12:52:35.631ZApplied to files:
📚 Learning: 2026-04-28T05:36:25.456ZApplied to files:
📚 Learning: 2026-04-28T04:38:21.935ZApplied to files:
📚 Learning: 2026-04-27T10:33:12.228ZApplied to files:
📚 Learning: 2026-04-21T12:14:30.524ZApplied to files:
📚 Learning: 2026-04-23T08:51:00.819ZApplied to files:
📚 Learning: 2026-04-27T11:18:47.298ZApplied to files:
📚 Learning: 2026-04-20T14:36:31.032ZApplied to files:
📚 Learning: 2026-04-20T14:36:21.288ZApplied to files:
📚 Learning: 2026-04-28T11:24:23.345ZApplied to files:
🔇 Additional comments (11)
📝 WalkthroughWalkthroughAdds gpt-5.5-specific token limit assignment during Codex OAuth model setup, enhances streaming error parsing to handle nested/error-wrapped payloads and recognize Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces specific limit overrides for GPT-5.5 models within the Codex OAuth plugin and improves error handling for server-side errors in the stream parser. The review feedback suggests using a more robust type guard for error objects and recommends using the canonical model API ID for configuration checks to ensure consistency.
Review: fix(openai): sync OAuth context and retry fixesOverall the two backports are well-scoped and the test coverage is good. A few concerns from the PawWork side, ordered by severity. P0 — Potential regression:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Inline comment on P0 — Potential regression: Marking all |
|
Inline comment on P1 — Type smell: The |
|
Inline comment on P2 — Fragile matching: |
|
Inline comment on P1 — Missing negative test: The test only asserts the happy path ( |
|
Inline comment on P2 — Missing assertion: The test asserts |
|
Inline comment on P3 — Double-stringification concern: When input is a double-wrapped JSON string ( |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/plugin/codex.ts`:
- Around line 453-460: Replace the brittle check model.id.includes("gpt-5.5")
with the same semver-based discriminator used by shouldKeepCodexOAuthModel:
inspect model.api.id and run the same regex match (/^gpt-(\d+)\.(\d+)/) to
detect a 5.5 series model before applying the smaller limits in the block that
sets model.limit; keep the existing `@ts-expect-error` on limit.input since the
upstream Provider SDK v1 lacks that type, but change the ID/source used to
model.api.id and the semver match to ensure consistency with
shouldKeepCodexOAuthModel.
In `@packages/opencode/test/session/retry.test.ts`:
- Around line 298-315: Add two negative/regression cases to retry.test.ts using
MessageV2.fromError with ProviderID.make("openai"): (1) create a server_error
chunk where the error object lacks message (e.g., error: { code: "server_error"
}) and assert MessageV2.APIError.isInstance(result) is true, (result as
MessageV2.APIError).data.isRetryable is true, and SessionRetry.retryable(result)
equals the fallback "Server error."; (2) create a server_error chunk that
explicitly marks non-retryable (e.g., error: { code: "server_error", message:
"X", retryable: false }) and assert MessageV2.APIError.isInstance(result) is
true, (result as MessageV2.APIError).data.isRetryable is false, and
SessionRetry.retryable(result) returns null/does not indicate a retryable
message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a905950c-3f84-4f7d-b92e-b93c86b1f7da
📒 Files selected for processing (4)
packages/opencode/src/plugin/codex.tspackages/opencode/src/provider/error.tspackages/opencode/test/plugin/codex.test.tspackages/opencode/test/session/retry.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
packages/opencode/**/*.ts
📄 CodeRabbit inference engine (packages/opencode/AGENTS.md)
packages/opencode/**/*.ts: UseEffect.gen(function* () { ... })for Effect composition
UseEffect.fn("Domain.method")for named/traced effects andEffect.fnUntracedfor internal helpers; these accept pipeable operators as extra arguments to avoid unnecessary outer.pipe()wrappers
UseEffect.callbackfor callback-based APIs
PreferDateTime.nowAsDateovernew Date(yield* Clock.currentTimeMillis)when you need aDatein Effect code
UseSchema.Classfor multi-field data in Effect schemas
Use branded schemas (Schema.brand) for single-value types in Effect
UseSchema.TaggedErrorClassfor typed errors in Effect schemas
UseSchema.Defectinstead ofunknownfor defect-like causes in Effect code
InEffect.gen/Effect.fn, preferyield* new MyError(...)overyield* Effect.fail(new MyError(...))for direct early-failure branches
UsemakeRuntimefromsrc/effect/run-service.tsfor all services; it returns{ runPromise, runFork, runCallback }backed by a sharedmemoMapthat deduplicates layers
UseInstanceStatefromsrc/effect/instance-state.tsfor per-directory or per-project state that needs per-instance cleanup; do work directly in theInstanceState.makeclosure whereScopedCachehandles run-once semantics
UseEffect.addFinalizerorEffect.acquireReleaseinside theInstanceState.makeclosure for cleanup (subscriptions, process teardown, etc.)
UseEffect.forkScopedinside theInstanceState.makeclosure for background stream consumers — the fiber is interrupted when the instance is disposed
PreferFileSystem.FileSysteminstead of rawfs/promisesfor effectful file I/O in Effect services
PreferChildProcessSpawner.ChildProcessSpawnerwithChildProcess.make(...)instead of custom process wrappers in Effect services
PreferHttpClient.HttpClientinstead of rawfetchin Effect services
PreferPath.Path,Config,Clock, andDateTimeservices when those concerns are already inside Effect code
For backgroun...
Files:
packages/opencode/test/session/retry.test.tspackages/opencode/src/plugin/codex.tspackages/opencode/test/plugin/codex.test.tspackages/opencode/src/provider/error.ts
packages/opencode/test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (packages/opencode/test/AGENTS.md)
packages/opencode/test/**/*.test.{ts,tsx}: Use thetmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup. Useawait usingsyntax to ensure automatic cleanup when the variable goes out of scope.
When using thetmpdirfunction with git repository support, pass thegit: trueoption to initialize a git repo with a root commit.
Use theconfigoption intmpdirto write anopencode.jsonconfig file during test setup by passing a partial Config.Info object.
Use theinitoption intmpdirto define custom setup functions that can return extra data accessible viatmp.extra, and use thedisposeoption for custom cleanup logic.
UsetestEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services or Effect-based workflows.
Useit.effect(...)when the test should run withTestClockandTestConsole. Useit.live(...)when the test depends on real time, filesystem mtimes, child processes, git, locks, or other live OS behavior.
Prefer Effect-aware helpers fromfixture/fixture.tsover building manual runtimes in tests: usetmpdirScoped()for scoped temp directories,provideInstance(dir)(effect)for low-level binding without directory creation,provideTmpdirInstance(...)for single temp instance binding, orprovideTmpdirServer(...)for tests that also need the test LLM server.
Defineconst it = testEffect(...)near the top of the test file and keep the test body insideEffect.gen(function* () { ... }). Yield services directly withyield* MyService.Serviceoryield* MyTool.
Avoid customManagedRuntime,attach(...), or ad hocrun(...)wrappers in Effect tests whentestEffect(...)already provides the runtime.
When a test needs instance-local state, preferprovideTmpdirInstance(...)orprovideInstance(...)over manualInstance.provide(...)inside Promise-style tests.
Files:
packages/opencode/test/session/retry.test.tspackages/opencode/test/plugin/codex.test.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:108-169
Timestamp: 2026-04-27T10:33:12.228Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/session/prompt.ts` and `packages/opencode/src/session/processor.ts`, PR `#264`), the loop-gate race condition between `buildLoopContext()` and `recordSyntheticBlock`/`recordSyntheticStop` is intentionally handled via idempotence guards (re-check sigKey presence / `hasStopped` inside the record helpers) rather than a full per-parent `Effect.Mutex`. Threading a `Map<MessageID, Mutex>` through the processor was considered too large a surface change for this edge case; the residual TOCTOU window only produces extra synthetic parts with no behavioral drift on the "turn ends" contract. A code comment documents the trade-off and points to a full-mutex follow-up if the race is observed in practice. Do NOT re-flag the absence of a per-parent mutex as a blocking issue in future reviews.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/agent.ts:23-27
Timestamp: 2026-04-28T04:38:11.771Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/tool/agent.ts`), the `subagent_session_id` field in the `Parameters` schema accepts any `Schema.String` rather than a branded `SessionID`. This is inherited upstream behavior (adopted in PR `#270`, an upstream-sync graft of upstream PR `#23244`). The fix — validating `subagent_session_id` as a `SessionID` brand up front so malformed/typo'd IDs fail explicitly rather than silently forking a new subagent session — is intentionally deferred to a follow-up PR or upstream report. Do NOT re-flag this as a blocking issue in PR `#270` or in future upstream-sync PRs that carry the same schema; flag it only in a PawWork-authored PR that touches `agent.ts` parameter validation.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/patch/index.ts:337-346
Timestamp: 2026-04-28T04:38:05.946Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/patch/index.ts`), the BOM-transition surfacing gap — where `Bom.split` strips BOM before building `unified_diff`/`new_content` but `Bom.join` later re-attaches BOM on disk write, meaning BOM changes are not reflected in the diff payload — is intentionally deferred. PR `#270` is an upstream-sync graft; fixing the issue here would mix refactor + bugfix intents and drift the diff from the upstream baseline needed for clean future grafts. A dedicated follow-up PR (or upstream report) will address this. Do NOT re-flag the missing BOM-change surfacing in `ApplyPatchFileUpdate`/`ApplyPatchFileChange` as a blocking issue in PR `#270` or in future sync PRs that carry the same upstream baseline.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/tool/tool.ts:34-42
Timestamp: 2026-04-28T04:56:21.528Z
Learning: In `packages/opencode/src/tool/tool.ts` (Astro-Han/pawwork, PR `#270`), the `Def.execute` signature uses `ctx: Context` (without the `M` generic) instead of `ctx: Context<M>`, meaning `ctx.metadata(...)` is not checked against the same `M` as `ExecuteResult<M>`. This is inherited upstream behavior adopted via the graft strategy in PR `#270` (an upstream-sync PR per `project_upstream_strategy.md`). Fixing it here would mix refactor + bugfix intents and drift the diff from the upstream baseline. The fix is deferred to a dedicated follow-up PR or upstream report. Do NOT re-flag the missing `Context<M>` generic in `Def.execute` during upstream-sync PRs; only flag it in a PawWork-authored PR that directly touches `tool.ts` type signatures.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/file/ripgrep.ts:469-470
Timestamp: 2026-04-28T08:29:02.858Z
Learning: In `packages/opencode/src/file/ripgrep.ts` (Astro-Han/pawwork, PR `#270`), the `tree()` function filters files with `if (file.includes(".opencode")) continue`, which is a raw substring match that would also drop unrelated paths whose names merely contain `.opencode` (e.g., `.opencode-backup`). The correct fix is `if (file.split(path.sep).includes(".opencode")) continue` to match only the exact path segment. However, upstream `dev:packages/opencode/src/file/ripgrep.ts` carries the identical false-positive filter. PR `#270` is an upstream-sync graft; fixing it here would diverge from the baseline. The fix is deferred to a PawWork ripgrep filter cleanup PR or an upstream-first fix, to land alongside the other ripgrep deferrals (Schema.Class, bytes-union, abort short-circuit, mixed-separator). Do NOT re-flag the `.opencode` substring filter in upstream-sync PRs.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/src/file/ripgrep.ts:45-72
Timestamp: 2026-04-28T07:27:49.810Z
Learning: In `packages/opencode/src/file/ripgrep.ts` (Astro-Han/pawwork), `PathText`, the `lines` field inside `SearchMatch`, and `submatches[].match` accept only `{text: string}` and do NOT handle ripgrep's `{bytes: base64}` JSON variant (emitted for non-UTF-8 paths/match text). This matches the upstream `dev:packages/opencode/src/file/ripgrep.ts` baseline exactly. The fix — adding `Schema.Union([Schema.Struct({text: Schema.String}), Schema.Struct({bytes: Schema.String})])` for each location plus base64-decode in downstream consumers — is intentionally deferred to either a dedicated PawWork PR or an upstream-first fix inherited via the next sync. Do NOT re-flag the missing `{bytes: ...}` union variants in upstream-sync PRs; flag it only in a PawWork-authored PR that directly touches these schema definitions or their downstream consumers.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 224
File: packages/app/src/i18n/zh.ts:0-0
Timestamp: 2026-04-24T17:08:46.780Z
Learning: In Astro-Han/pawwork PR `#224`, the first-occurrence `PawWork 爪印` branding rule originally specified in issue `#196` was superseded by an updated Chinese-branding spec. On all zh UI surfaces in `packages/app/src/i18n/zh.ts` (e.g., `dialog.model.unpaid.freeModels.title`, `session.new.subtitle`, `sidebar.gettingStarted.line1`), the correct and intentional target is fully localized `爪印` branding — no `PawWork` prefix. Do NOT flag these strings as missing the first-occurrence `PawWork 爪印` rule in future reviews.
📚 Learning: 2026-04-27T12:59:45.694Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/test/session/prompt-effect.test.ts:0-0
Timestamp: 2026-04-27T12:59:45.694Z
Learning: In session prompt/diagnostics tests (e.g., when verifying injected LLM “recovery reminder” copy), do not assert a single exact reminder phrasing tied to signature kind. If a scenario can produce both `input:` and `target:` signatures (for example, a `read` tool call with a `filePath` parameter), accept either phrasing: the input-repeat variant may say “repeated the same tool input 3 times” (literal count), and the target-repeat variant may say “failed against the same target multiple times” (“multiple times” without a count). Your assertions should allow both variants rather than narrowing to only the input-variant text.
Applied to files:
packages/opencode/test/session/retry.test.ts
📚 Learning: 2026-04-28T11:24:35.312Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/test/session/subagent-lifecycle-integration.test.ts:41-47
Timestamp: 2026-04-28T11:24:35.312Z
Learning: In `packages/opencode/test/session/subagent-lifecycle-integration.test.ts` (Astro-Han/pawwork, PR `#287`), the test suite intentionally uses a manual `run` helper (`Effect.runPromise(program.pipe(Effect.provide(Layer.mergeAll(SubagentRun.defaultLayer, Session.defaultLayer)), Effect.orDie))`) and raw `bun:test` test cases rather than the `testEffect(...)` harness. All 27 lifecycle tests pass with this pattern. Migration to `testEffect` + `it.live(...)` is deferred to a dedicated test-harness sweep PR to avoid fixture drift before merge. Do NOT re-flag the manual `Effect.runPromise` runner in this file as needing harness migration until that sweep PR lands.
Applied to files:
packages/opencode/test/session/retry.test.ts
📚 Learning: 2026-04-23T08:51:00.819Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 186
File: packages/opencode/test/plugin/workspace-adaptor.test.ts:139-144
Timestamp: 2026-04-23T08:51:00.819Z
Learning: For pawwork tests under packages/opencode/test/**, auth.json teardown may intentionally combine `Filesystem.write` (from `packages/opencode/src/util/filesystem.ts`) with `node:fs/promises` `unlink` for cleanup. Do not flag this as inconsistent style; it is the established/intentional pattern because `Filesystem` does not provide a `remove`/`unlink` helper.
Applied to files:
packages/opencode/test/session/retry.test.tspackages/opencode/test/plugin/codex.test.ts
📚 Learning: 2026-04-21T16:00:44.910Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 102
File: packages/opencode/src/config/model-id.ts:9-11
Timestamp: 2026-04-21T16:00:44.910Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/config/model-id.ts`), `ConfigModelID` uses a regex that requires a non-empty provider segment and a non-empty model segment separated by `/`, but intentionally allows additional slashes within the model segment. This is because `Provider.parseModel` supports model IDs like `synthetic/hf:zai-org/GLM-4.7`. Do not flag the permissive slash handling as a bug.
Applied to files:
packages/opencode/src/plugin/codex.ts
📚 Learning: 2026-04-27T08:58:00.665Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:531-538
Timestamp: 2026-04-27T08:58:00.665Z
Learning: When using Effect (e.g., `yield*` with `Effect`-style generator yielding), only use `yield* new SomeErrorClass(...)` if `SomeErrorClass` extends `Schema.TaggedErrorClass` (i.e., it implements Effect’s Yieldable interface). For plain `Error` subclasses (like `BlockedLoopError` / `LoopStopError`) or inline `new Error(...)` values, they are not yieldable and must be wrapped as `yield* Effect.fail(new PlainError(...))`. Do not recommend changing `yield* Effect.fail(new SomePlainError(...))` to `yield* new SomePlainError(...)` unless the error class extends `Schema.TaggedErrorClass`.
Applied to files:
packages/opencode/src/plugin/codex.tspackages/opencode/src/provider/error.ts
📚 Learning: 2026-04-28T12:49:09.792Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/session/subagent-run.ts:445-445
Timestamp: 2026-04-28T12:49:09.792Z
Learning: In this repo’s `packages/opencode/src` module files, the canonical module-as-namespace pattern is load-bearing: at the bottom of each module file, add a self-reexport in the form `export * as <ModuleName> from "./<module-file>"` (e.g., in `subagent-run.ts`: `export * as SubagentRun from "./subagent-run"`). Consumers must import the namespace via a named binding (e.g., `import { SubagentRun } from ".../subagent-run"`) and then access members as `SubagentRun.Service`, `SubagentRun.defaultLayer`, `SubagentRun.AgentListFilter`, etc. During code review, do NOT flag these self-reexports as dead code or redundant; removing or altering them will break the consumer named binding.
Applied to files:
packages/opencode/src/plugin/codex.tspackages/opencode/src/provider/error.ts
📚 Learning: 2026-04-27T11:19:24.963Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 271
File: packages/opencode/test/tool/websearch-auth.test.ts:0-0
Timestamp: 2026-04-27T11:19:24.963Z
Learning: In `packages/opencode/test/tool/websearch-auth.test.ts` (Astro-Han/pawwork), the tests intentionally use a small local `runWith` runner with raw `bun:test` and `Effect.runPromise` rather than the `testEffect` harness. Each test case injects a custom in-memory `Auth.Service` layer; switching to `testEffect` would be style-only churn without changing risk coverage. Do not flag these tests as needing harness migration.
Applied to files:
packages/opencode/test/plugin/codex.test.ts
📚 Learning: 2026-04-27T11:18:47.298Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 271
File: packages/opencode/test/tool/websearch.test.ts:21-78
Timestamp: 2026-04-27T11:18:47.298Z
Learning: In `packages/opencode/test/tool/websearch.test.ts`, the tests intentionally use manual `Effect.runPromise` with explicit `Effect.provide(...)` chains (including `Layer.succeed(Auth.Service, ...)`, `Layer.succeed(HttpClient.HttpClient, http)`, `WebSearchAuth.layer`, `Truncate.defaultLayer`, and `Agent.defaultLayer`) rather than the `testEffect(...)` harness. This is by design: the fake Auth and HTTP recovery-metadata layers must be explicitly injected and kept visible/scoped at the test site. Do NOT suggest migrating these tests to `testEffect` or removing the manual layer provides.
Applied to files:
packages/opencode/test/plugin/codex.test.ts
📚 Learning: 2026-04-28T04:56:21.338Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 270
File: packages/opencode/test/tool/tool-define.test.ts:2-9
Timestamp: 2026-04-28T04:56:21.338Z
Learning: In `packages/opencode/test/tool/tool-define.test.ts` (Astro-Han/pawwork, PR `#270`), the test suite intentionally uses a `ManagedRuntime.make(Layer.mergeAll(Truncate.defaultLayer, Agent.defaultLayer))` wrapper and raw `bun:test` `test(...)` cases rather than the `testEffect(...)` harness. This code was adopted wholesale from upstream via the graft strategy (per `project_upstream_strategy.md`). Migrating to `testEffect` is deferred to a follow-up PR so as not to mix refactor + harness-migration intents and to avoid drifting the diff from the upstream baseline needed for clean future grafts. Do NOT re-flag the `ManagedRuntime.make` usage in this file as needing harness migration until that follow-up lands.
Applied to files:
packages/opencode/test/plugin/codex.test.ts
📚 Learning: 2026-04-20T14:36:31.032Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/opencode/test/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:31.032Z
Learning: Applies to packages/opencode/test/**/*.test.{ts,tsx} : Define `const it = testEffect(...)` near the top of the test file and keep the test body inside `Effect.gen(function* () { ... })`. Yield services directly with `yield* MyService.Service` or `yield* MyTool`.
Applied to files:
packages/opencode/test/plugin/codex.test.ts
📚 Learning: 2026-04-22T09:32:54.556Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/opencode/test/provider/provider.test.ts:64-85
Timestamp: 2026-04-22T09:32:54.556Z
Learning: In `packages/opencode/test/provider/provider.test.ts`, the file intentionally uses AppRuntime-based async helpers (`run`, `list`, `getProvider`, etc.) rather than `testEffect(...)` for all tests. Converting individual tests to `testEffect` while leaving the rest on the async pattern would create internal inconsistency. A full harness migration of this file is the right approach if the pattern needs to change, but that should be a separate PR.
Applied to files:
packages/opencode/test/plugin/codex.test.ts
📚 Learning: 2026-04-28T11:24:23.345Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 287
File: packages/opencode/src/session/subagent-run.ts:8-16
Timestamp: 2026-04-28T11:24:23.345Z
Learning: In Astro-Han/pawwork, the established pattern for service-internal errors (e.g., in `packages/opencode/src/session/subagent-run.ts`) is plain classes (with a `readonly _tag` discriminant) or `NamedError.create` (zod-based), NOT `Schema.TaggedErrorClass`. Converting individual modules to `Schema.TaggedErrorClass` one-off would diverge from the surrounding code. The migration to `Schema.TaggedErrorClass` for service-internal errors is intentionally deferred to a dedicated repo-wide error-class sweep PR. Do NOT flag plain error classes in `subagent-run.ts` or other service-internal modules as needing `Schema.TaggedErrorClass` conversion until that sweep PR lands.
Applied to files:
packages/opencode/src/provider/error.ts
🔇 Additional comments (4)
packages/opencode/test/plugin/codex.test.ts (1)
147-199: Solid regression coverage for the OAuth Codex override.This locks in both the GPT-5.5 limit override and the zero-cost mutation, so the new behavior is protected end-to-end.
packages/opencode/src/provider/error.ts (3)
112-115:isRetryablebeing required here matches the downstream contract.This aligns with
MessageV2.APIErrorconsumption and avoidsundefinedleaking into retry handling.
118-120: Confirm the outer envelope is disposable.Parsing
raw.messageas JSON and then replacingbodywith the inner object silently drops any sibling fields on the wrapper. If this stream shape is guaranteed to be{"message": "<json>"}only, please document that contract; otherwise keep the wrapper and parse the nested payload separately.
154-160: No action needed. OpenAI's official documentation explicitly classifiesserver_erroras a transient server-side failure that should be retried. The code correctly implements this per the upstream API contract, and other error codes with semantic meaning (quota, usage limits, invalid input) are already appropriately marked as non-retryable.
|
Review follow-up pushed in 32a79ec. P0 server_error retryability: verified upstream anomalyco/opencode#24063 treats OpenAI stream server_error as retryable. Kept parity and did not add sub-code/message filtering in this sync PR. Also checked the local retry policy: RETRY_MAX_DELAY caps delay length, not attempt count, so I did not rely on it as an infinite-retry guard. P1 tests and typing: added fallback coverage for "Server error." and a non-retryable default-path stream error with bad_request. Removed the ts-expect-error by using a local typed limit object that bridges the old SDK hook Model shape to opencode's input-aware limit usage. P2 matching and overwrite coverage: switched the limit override to canonical model.api.id with an explicit /^gpt-5.5(?:$|-)/ matcher, added variant and nonmatch tests, and asserted the original 1,050,000 limit before loader mutation. P3 responseBody and commit scope: documented why the inner OpenAI provider payload is used for responseBody. Left the existing commit scopes in place and added this review-fix commit for the follow-up. Inline review threads have been replied to and resolved. CodeRabbit's retryable:false server_error suggestion was intentionally not applied because it conflicts with the upstream behavior this PR is syncing. |
Summary
Syncs the first missed upstream runtime/provider fixes tracked in #298.
gpt-5.5uses the effective Codex plan limits:context: 400_000,input: 272_000,output: 128_000.server_errorchunks wrapped inside amessageJSON string become retryable API errors instead of unknown errors.Why
PawWork was showing API-side GPT-5.5 context metadata for OpenAI OAuth sessions even though OAuth requests are routed through the Codex backend. That made the context UI and proactive compaction threshold look safe near 270K tokens while the effective Codex limit was smaller. The retry parser fix is part of the same upstream OpenAI runtime follow-up batch.
Related Issue
Refs #298
How To Verify
Screenshots or Recordings
No visible UI changes.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
New Features
Bug Fixes
Tests