feat: add GPT-5.4 support (plugin v5.4.3)#67
Merged
Conversation
- add gpt-5.4/gpt-5.4-pro snapshot alias normalization and remap legacy gpt-5* aliases to gpt-5.4 - split gpt-5.4-pro into an isolated prompt family/cache key while preserving fallback behavior - update OpenCode templates/docs for 1M context on gpt-5.4* and expand regression coverage across model mapping/family tests Co-authored-by: Codex <noreply@openai.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNormalizes legacy GPT‑5 aliases to gpt-5.4, adds a gpt-5.4‑pro family with dated snapshot aliases and isolated prompts/cache, raises gpt-5.4 context windows to 1,000,000 tokens, updates docs/tests, and adds a global accounts fallback with seeding and concurrency handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Loader as loadAccountsInternal
participant ProjectFS as Project Storage FS
participant Migrator as Legacy Migrator
participant GlobalFS as Global Storage FS
participant Persister as Persister
Client->>Loader: loadAccounts(projectPath)
Loader->>ProjectFS: read project storage
alt project file exists
ProjectFS-->>Loader: accounts
Loader-->>Client: accounts
else ENOENT (missing)
Loader->>Migrator: attempt migrate legacy storage
alt migration succeeded
Migrator-->>Loader: migratedAccounts
Loader->>Persister: persist migratedAccounts to projectPath
Persister-->>Loader: persisted
Loader-->>Client: migratedAccounts
else migration absent/failed
Loader->>GlobalFS: loadGlobalAccountsFallback(globalPath)
alt global accounts found
GlobalFS-->>Loader: globalAccounts
Loader->>Persister: seed projectPath with globalAccounts (withStorageLock)
Persister-->>Loader: seeded / seed failed
Loader-->>Client: globalAccounts
else no global fallback / corrupted
GlobalFS-->>Loader: null / error
Loader-->>Client: null / error
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
|
Request timed out after 900000ms (requestId=39ea9e9d-6cf2-4217-a6a0-27749c0b7197) |
Owner
Author
|
@greptile-ai @coderabbitai review |
Contributor
|
✅ Actions performedReview triggered.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/edge-cases.test.ts (1)
42-45: Add one explicit canonical Pro-case assertion for stronger coverage.Since this block now documents mixed-separator fallback behavior, consider also asserting a canonical Pro input (e.g.,
gpt-5.4-pro-high) in the same test group to keep Pro-family normalization guarded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/edge-cases.test.ts` around lines 42 - 45, Add a direct assertion for a canonical Pro-form input to this test group to ensure Pro-family normalization is preserved: call normalizeModel with a canonical Pro variant like "gpt-5.4-pro-high" and assert it returns the Pro canonical form (e.g., "gpt-5.4-pro"); place this expect alongside the existing expectations in the same test block that contains normalizeModel("gpt_5.4-high") and normalizeModel("gpt-5_4 pro").lib/request/helpers/model-map.ts (1)
70-75: Consider generating dated alias variants to reduce duplication drift.These blocks are correct but highly repetitive; a small helper would make future snapshot rollovers less error-prone.
♻️ Proposed refactor
+const EFFORT_SUFFIXES = ["", "-none", "-low", "-medium", "-high", "-xhigh"] as const; + +function expandAliasFamily(prefix: string, target: string): Record<string, string> { + return Object.fromEntries( + EFFORT_SUFFIXES.map((suffix) => [`${prefix}${suffix}`, target]), + ); +} + export const MODEL_MAP: Record<string, string> = { @@ - "gpt-5.4-2026-03-05": "gpt-5.4", - "gpt-5.4-2026-03-05-none": "gpt-5.4", - "gpt-5.4-2026-03-05-low": "gpt-5.4", - "gpt-5.4-2026-03-05-medium": "gpt-5.4", - "gpt-5.4-2026-03-05-high": "gpt-5.4", - "gpt-5.4-2026-03-05-xhigh": "gpt-5.4", + ...expandAliasFamily("gpt-5.4-2026-03-05", "gpt-5.4"), @@ - "gpt-5.4-pro-2026-03-05": "gpt-5.4-pro", - "gpt-5.4-pro-2026-03-05-none": "gpt-5.4-pro", - "gpt-5.4-pro-2026-03-05-low": "gpt-5.4-pro", - "gpt-5.4-pro-2026-03-05-medium": "gpt-5.4-pro", - "gpt-5.4-pro-2026-03-05-high": "gpt-5.4-pro", - "gpt-5.4-pro-2026-03-05-xhigh": "gpt-5.4-pro", + ...expandAliasFamily("gpt-5.4-pro-2026-03-05", "gpt-5.4-pro"),Also applies to: 86-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/request/helpers/model-map.ts` around lines 70 - 75, Replace the repeated dated alias entries with a small helper that generates those variants: implement a function (e.g., generateDatedAliases or addDatedAliases) that takes a base alias like "gpt-5.4" and a date string "2026-03-05" and returns the mapping keys ("gpt-5.4-2026-03-05", "gpt-5.4-2026-03-05-none", "-low", "-medium", "-high", "-xhigh") mapped to the base alias; then call that helper when building the model map instead of listing the six lines for each base model (the current literal keys such as "gpt-5.4-2026-03-05" should be removed and replaced by the helper invocation). Ensure the helper is reusable and used similarly for the block at lines 86-91.test/gpt54-models.test.ts (1)
232-235: Clarify test naming to match the new fallback expectation.The expectation now resolves to
gpt-5.4via generic GPT-5 fallback, so the current wording can read as contradictory.✏️ Suggested rename for clarity
- it("should not match gpt-5.4x patterns as gpt-5.4", () => { + it("should route gpt-5.4x patterns through generic GPT-5 fallback", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gpt54-models.test.ts` around lines 232 - 235, The test description is misleading given the new behavior where normalizeModel("gpt-5.40") and normalizeModel("gpt-5.44") resolve via the generic GPT-5 fallback to "gpt-5.4"; update the test case name to clearly state it expects generic GPT-5 fallback rather than “boundary-aware matching” (locate the test that calls normalizeModel with "gpt-5.40"/"gpt-5.44" and change its describe/it string to mention “generic GPT-5 fallback to gpt-5.4”).test/codex-prompts.test.ts (1)
483-504: Strengthen the cache-isolation assertion with a negative check.This test verifies the pro cache file is written, but it would still pass if the base
gpt-5.4cache key were also written. Add a “base key is not written” assertion to enforce true isolation.♻️ Suggested test hardening
const writeTargets = mockedWriteFile.mock.calls.map(([target]) => String(target)); expect(rawGitHubCall?.[0]).toContain("gpt_5_2_prompt.md"); expect(writeTargets.some((target) => target.includes("gpt-5.4-pro-instructions.md"))).toBe(true); + expect( + writeTargets.some((target) => /gpt-5\.4-instructions\.md$/.test(target)), + ).toBe(false); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-prompts.test.ts` around lines 483 - 504, The test "should map gpt-5.4-pro prompts to gpt_5_2 prompt file with isolated cache key" currently only asserts the pro cache file was written; add a negative assertion that the base cache key file for the non-pro model is NOT written to enforce isolation. After collecting writeTargets from mockedWriteFile.mock.calls, add an expectation that writeTargets does not include a target containing "gpt-5.4-instructions.md" (or the exact base filename used by getCodexInstructions), e.g., expect(writeTargets.some(t => t.includes("gpt-5.4-instructions.md"))).toBe(false), referencing getCodexInstructions, mockedWriteFile and writeTargets to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 136: Update the README note for `gpt-5.4-pro` to clarify the scope of the
fallback: change the phrasing around "fallback default is `gpt-5.4-pro ->
gpt-5.4`" to explicitly state this behavior occurs only when the fallback policy
is enabled (e.g., "If fallback policy is enabled, default fallback: `gpt-5.4-pro
-> gpt-5.4`"); reference the `gpt-5.4-pro` token and the existing fallback
phrase so the rewording makes the condition (fallback policy enabled) explicit.
---
Nitpick comments:
In `@lib/request/helpers/model-map.ts`:
- Around line 70-75: Replace the repeated dated alias entries with a small
helper that generates those variants: implement a function (e.g.,
generateDatedAliases or addDatedAliases) that takes a base alias like "gpt-5.4"
and a date string "2026-03-05" and returns the mapping keys
("gpt-5.4-2026-03-05", "gpt-5.4-2026-03-05-none", "-low", "-medium", "-high",
"-xhigh") mapped to the base alias; then call that helper when building the
model map instead of listing the six lines for each base model (the current
literal keys such as "gpt-5.4-2026-03-05" should be removed and replaced by the
helper invocation). Ensure the helper is reusable and used similarly for the
block at lines 86-91.
In `@test/codex-prompts.test.ts`:
- Around line 483-504: The test "should map gpt-5.4-pro prompts to gpt_5_2
prompt file with isolated cache key" currently only asserts the pro cache file
was written; add a negative assertion that the base cache key file for the
non-pro model is NOT written to enforce isolation. After collecting writeTargets
from mockedWriteFile.mock.calls, add an expectation that writeTargets does not
include a target containing "gpt-5.4-instructions.md" (or the exact base
filename used by getCodexInstructions), e.g., expect(writeTargets.some(t =>
t.includes("gpt-5.4-instructions.md"))).toBe(false), referencing
getCodexInstructions, mockedWriteFile and writeTargets to locate the change.
In `@test/edge-cases.test.ts`:
- Around line 42-45: Add a direct assertion for a canonical Pro-form input to
this test group to ensure Pro-family normalization is preserved: call
normalizeModel with a canonical Pro variant like "gpt-5.4-pro-high" and assert
it returns the Pro canonical form (e.g., "gpt-5.4-pro"); place this expect
alongside the existing expectations in the same test block that contains
normalizeModel("gpt_5.4-high") and normalizeModel("gpt-5_4 pro").
In `@test/gpt54-models.test.ts`:
- Around line 232-235: The test description is misleading given the new behavior
where normalizeModel("gpt-5.40") and normalizeModel("gpt-5.44") resolve via the
generic GPT-5 fallback to "gpt-5.4"; update the test case name to clearly state
it expects generic GPT-5 fallback rather than “boundary-aware matching” (locate
the test that calls normalizeModel with "gpt-5.40"/"gpt-5.44" and change its
describe/it string to mention “generic GPT-5 fallback to gpt-5.4”).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c346fa4-8c77-442d-860d-4e0208e19a34
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
CHANGELOG.mdREADME.mdconfig/README.mdconfig/opencode-legacy.jsonconfig/opencode-modern.jsondocs/configuration.mddocs/development/TESTING.mddocs/getting-started.mdlib/prompts/codex.tslib/request/helpers/model-map.tslib/request/request-transformer.tspackage.jsontest/codex-prompts.test.tstest/codex.test.tstest/edge-cases.test.tstest/gpt54-models.test.tstest/model-map.test.tstest/request-transformer.test.ts
- default unknown/empty model fallback to gpt-5.4 across transform and runtime family selection - load global account storage when project storage is missing, then seed project-scoped storage - align tests and internal testing docs with the new fallback behavior Co-authored-by: Codex <noreply@openai.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/gpt54-models.test.ts (1)
55-63: Optional: extract snapshot date literal into a shared local constant.Using a small constant for the repeated snapshot suffix would reduce future churn when snapshots roll forward.
♻️ Optional cleanup
describe("GPT-5.4 Model Support", () => { + const SNAPSHOT = "2026-03-05"; + describe("GPT-5.4 Model Normalization", () => { @@ - expect(MODEL_MAP["gpt-5.4-2026-03-05"]).toBe("gpt-5.4"); - expect(MODEL_MAP["gpt-5.4-2026-03-05-low"]).toBe("gpt-5.4"); + expect(MODEL_MAP[`gpt-5.4-${SNAPSHOT}`]).toBe("gpt-5.4"); + expect(MODEL_MAP[`gpt-5.4-${SNAPSHOT}-low`]).toBe("gpt-5.4"); @@ - expect(normalizeModel("gpt-5.4-2026-03-05")).toBe("gpt-5.4"); - expect(normalizeModel("gpt-5.4-2026-03-05-high")).toBe("gpt-5.4"); - expect(getNormalizedModel("gpt-5.4-2026-03-05-medium")).toBe("gpt-5.4"); + expect(normalizeModel(`gpt-5.4-${SNAPSHOT}`)).toBe("gpt-5.4"); + expect(normalizeModel(`gpt-5.4-${SNAPSHOT}-high`)).toBe("gpt-5.4"); + expect(getNormalizedModel(`gpt-5.4-${SNAPSHOT}-medium`)).toBe("gpt-5.4");Also applies to: 110-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gpt54-models.test.ts` around lines 55 - 63, The tests repeat the snapshot suffix "2026-03-05" in multiple expectations (MODEL_MAP checks and normalizeModel/getNormalizedModel assertions); extract that suffix into a single local constant (e.g., const SNAPSHOT = "-2026-03-05") at the top of the test block and use `${SNAPSHOT}`, `${SNAPSHOT}-low`, `${SNAPSHOT}-high`, `${SNAPSHOT}-medium` in the calls and expectations involving MODEL_MAP, normalizeModel, and getNormalizedModel so future snapshot date updates only require one edit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/gpt54-models.test.ts`:
- Around line 55-63: The tests repeat the snapshot suffix "2026-03-05" in
multiple expectations (MODEL_MAP checks and normalizeModel/getNormalizedModel
assertions); extract that suffix into a single local constant (e.g., const
SNAPSHOT = "-2026-03-05") at the top of the test block and use `${SNAPSHOT}`,
`${SNAPSHOT}-low`, `${SNAPSHOT}-high`, `${SNAPSHOT}-medium` in the calls and
expectations involving MODEL_MAP, normalizeModel, and getNormalizedModel so
future snapshot date updates only require one edit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04a2e860-25e2-4922-a030-f69722dd122b
📒 Files selected for processing (9)
docs/development/TESTING.mdindex.tslib/request/request-transformer.tslib/storage.tstest/edge-cases.test.tstest/gpt54-models.test.tstest/property/transformer.property.test.tstest/request-transformer.test.tstest/storage.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/edge-cases.test.ts
Clarify fallback docs, harden fallback seeding under concurrent loads, and add regression coverage for storage fallback edge cases. Co-authored-by: Codex <noreply@openai.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/storage.ts (1)
678-680:⚠️ Potential issue | 🔴 CriticalLock-order inversion can deadlock concurrent callers.
withFallbackSeedLockwraps a call topersistMigration, and forloadAccounts()that persister issaveAccounts(storage lock). Concurrently,withAccountStorageTransaction()already holds the storage lock and can block on the fallback lock. That creates a circular wait and hangs both operations.🐛 Suggested fix (unify lock order by loading under storage lock with unlocked persister)
export async function loadAccounts(): Promise<AccountStorageV3 | null> { - return loadAccountsInternal(saveAccounts); + return withStorageLock(async () => loadAccountsInternal(saveAccountsUnlocked)); }Also applies to: 772-795
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 678 - 680, loadAccounts calling loadAccountsInternal(saveAccounts) can deadlock because withFallbackSeedLock (used by persistMigration) and the storage lock (held by withAccountStorageTransaction) are acquired in inconsistent order; to fix, ensure the storage lock is acquired first and the persister runs without holding the storage lock. Concretely, modify loadAccounts/loadAccountsInternal so they load data while holding the storage lock (use the same locking mechanism as withAccountStorageTransaction) but pass an "unlocked" persister wrapper for saveAccounts (or defer persistMigration/unlock inside withFallbackSeedLock) so persistMigration/withFallbackSeedLock does not try to acquire the fallback lock while the storage lock is held; update calls that pass saveAccounts (e.g., at loadAccounts and the other sites noted around persistMigration) to use this unlocked-persister wrapper or reverse lock order consistently.
🧹 Nitpick comments (2)
lib/request/helpers/model-map.ts (1)
85-97: Centralize snapshot date literal to reduce update drift.Using a single date constant avoids accidental mismatch between base/pro snapshot mappings in future bumps.
♻️ Suggested refactor
+const GPT_54_SNAPSHOT_DATE = "2026-03-05" as const; ... - ...expandDatedAliases("gpt-5.4-2026-03-05", "gpt-5.4"), + ...expandDatedAliases(`gpt-5.4-${GPT_54_SNAPSHOT_DATE}`, "gpt-5.4"), ... - ...expandDatedAliases("gpt-5.4-pro-2026-03-05", "gpt-5.4-pro"), + ...expandDatedAliases(`gpt-5.4-pro-${GPT_54_SNAPSHOT_DATE}`, "gpt-5.4-pro"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/request/helpers/model-map.ts` around lines 85 - 97, The mappings duplicate the snapshot date literal in calls to expandDatedAliases (e.g., "gpt-5.4-2026-03-05" and "gpt-5.4-pro-2026-03-05"); introduce a single constant (e.g., MODEL_SNAPSHOT_DATE or SNAPSHOT_DATE) near the top of model-map.ts and replace those inline dated strings with template composition using expandDatedAliases(`${base}-${MODEL_SNAPSHOT_DATE}`, base) (or equivalent), updating both "gpt-5.4" and "gpt-5.4-pro" usages so future bumps only change one constant; ensure the constant name is exported or scoped appropriately for tests if needed.test/codex-prompts.test.ts (1)
45-48: Avoid brittle fixed-length assertion forMODEL_FAMILIES.Pinning exact length makes this test fail on valid future family additions unrelated to behavior.
♻️ Suggested test adjustment
- expect(MODEL_FAMILIES.length).toBe(7); + expect(MODEL_FAMILIES.length).toBeGreaterThanOrEqual(7); + expect(new Set(MODEL_FAMILIES).size).toBe(MODEL_FAMILIES.length);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-prompts.test.ts` around lines 45 - 48, The test in it("should be a readonly array") is brittle because it asserts MODEL_FAMILIES.length === 7; change the assertion to avoid fixed-length checks: keep the Array.isArray(MODEL_FAMILIES) expectation and replace the length equality check with either a readonly/immutability assertion (e.g., ensure MODEL_FAMILIES cannot be mutated or is frozen) or an inclusion-based check (e.g., expect(MODEL_FAMILIES).toEqual(expect.arrayContaining([...knownFamilies]))), referencing the MODEL_FAMILIES symbol and the same test block when you make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/storage.ts`:
- Around line 776-780: The catch currently treats any fs.access failure as
"missing" and proceeds to seeding; change the catch to only continue when the
error indicates the file does not exist (error.code === 'ENOENT') and rethrow
for other errors (e.g., EACCES, EPERM). Locate the fs.access call and its
surrounding try/catch that references seedPath and alter the catch to inspect
the caught error object and rethrow unless error.code === 'ENOENT', so only true
"file missing" cases fall through to seeding.
---
Outside diff comments:
In `@lib/storage.ts`:
- Around line 678-680: loadAccounts calling loadAccountsInternal(saveAccounts)
can deadlock because withFallbackSeedLock (used by persistMigration) and the
storage lock (held by withAccountStorageTransaction) are acquired in
inconsistent order; to fix, ensure the storage lock is acquired first and the
persister runs without holding the storage lock. Concretely, modify
loadAccounts/loadAccountsInternal so they load data while holding the storage
lock (use the same locking mechanism as withAccountStorageTransaction) but pass
an "unlocked" persister wrapper for saveAccounts (or defer
persistMigration/unlock inside withFallbackSeedLock) so
persistMigration/withFallbackSeedLock does not try to acquire the fallback lock
while the storage lock is held; update calls that pass saveAccounts (e.g., at
loadAccounts and the other sites noted around persistMigration) to use this
unlocked-persister wrapper or reverse lock order consistently.
---
Nitpick comments:
In `@lib/request/helpers/model-map.ts`:
- Around line 85-97: The mappings duplicate the snapshot date literal in calls
to expandDatedAliases (e.g., "gpt-5.4-2026-03-05" and "gpt-5.4-pro-2026-03-05");
introduce a single constant (e.g., MODEL_SNAPSHOT_DATE or SNAPSHOT_DATE) near
the top of model-map.ts and replace those inline dated strings with template
composition using expandDatedAliases(`${base}-${MODEL_SNAPSHOT_DATE}`, base) (or
equivalent), updating both "gpt-5.4" and "gpt-5.4-pro" usages so future bumps
only change one constant; ensure the constant name is exported or scoped
appropriately for tests if needed.
In `@test/codex-prompts.test.ts`:
- Around line 45-48: The test in it("should be a readonly array") is brittle
because it asserts MODEL_FAMILIES.length === 7; change the assertion to avoid
fixed-length checks: keep the Array.isArray(MODEL_FAMILIES) expectation and
replace the length equality check with either a readonly/immutability assertion
(e.g., ensure MODEL_FAMILIES cannot be mutated or is frozen) or an
inclusion-based check (e.g.,
expect(MODEL_FAMILIES).toEqual(expect.arrayContaining([...knownFamilies]))),
referencing the MODEL_FAMILIES symbol and the same test block when you make the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 21fbc77f-2db9-4b8f-bce6-02ca3ed28583
📒 Files selected for processing (7)
README.mdlib/request/helpers/model-map.tslib/storage.tstest/codex-prompts.test.tstest/edge-cases.test.tstest/gpt54-models.test.tstest/storage.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/edge-cases.test.ts
Harden storage fallback lock/error handling, preserve xhigh for GPT-5 legacy aliases mapped to GPT-5.4, and address review nits in tests/model map. Co-authored-by: Codex <noreply@openai.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
lib/request/request-transformer.ts (1)
546-551: Add a clarifying comment explaining the lightweight default behavior for legacy aliases.The code correctly preserves explicit xhigh requests for legacy aliases like
gpt-5-miniandgpt-5-nano(which normalize togpt-5.4). However, thedefaultEffortcalculation still applies theisLightweightcheck based on the original model name, resulting indefaultEffort = "minimal"for these aliases.This behavior is intentional (respecting the user's lightweight model choice while allowing explicit xhigh override), but a brief comment at lines 519–529 would clarify that legacy aliases intentionally retain lightweight defaults despite canonical support for higher efforts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/request/request-transformer.ts` around lines 546 - 551, Add a short clarifying comment near the defaultEffort/isLightweight logic to explain that legacy aliases (e.g., gpt-5-mini/gpt-5-nano) normalize to a canonical model (gpt-5.4) which supports higher efforts, but we intentionally keep the lightweight default based on the original alias; also note that explicit requests for xhigh are still honored via supportsRequestedXhigh (computed from supportsXhigh and canonicalSupportsXhigh) and the subsequent effort downgrade logic that sets effort = "high" when xhigh isn't supported.test/storage.test.ts (1)
1828-1874: The “concurrent” seed test is effectively serialized.Line 1867 runs
Promise.all, but eachloadAccounts()call acquires the same storage lock, so this doesn’t truly exercise parallel seed execution. Consider renaming the test or adjusting the setup so the intent matches behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/storage.test.ts` around lines 1828 - 1874, The test name claims concurrency but loadAccounts() serializes via the storage lock; either rename the test to reflect serialized behavior (e.g., "seeds project storage once when loadAccounts serializes") or change the setup to truly exercise parallel seeding by disabling/mocking the lock used inside loadAccounts (mock the storage lock/acquire function used by loadAccounts to a no-op so both Promise.all calls run concurrently) and keep the existing renameSpy/projectSeedWriteCount assertions; refer to loadAccounts, setStoragePath, getStoragePath, and the renameSpy/projectSeedWriteCount helpers when making the change.lib/storage.ts (1)
121-128:withFallbackSeedLockappears redundant in the current call graph.
loadAccountsInternalis reached vialoadAccounts(Line 679) andwithAccountStorageTransaction(Line 868-870), both already serialized bywithStorageLock. Consider removing this extra mutex (or documenting future intent) to reduce locking complexity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 121 - 128, withFallbackSeedLock and its fallbackSeedMutex add redundant serialization because loadAccountsInternal is already called under withStorageLock via loadAccounts and withAccountStorageTransaction; remove the extra mutex to simplify locking: delete the fallbackSeedMutex and the withFallbackSeedLock function and update any callers of withFallbackSeedLock (e.g., places calling loadAccountsInternal through withFallbackSeedLock) to call the underlying async function directly (or rely on withStorageLock), then run tests to ensure no race conditions; alternatively, if you intend to keep it for a future reason, add a clear comment above withFallbackSeedLock documenting that intent and why it is required despite withStorageLock (reference symbols: withFallbackSeedLock, fallbackSeedMutex, loadAccountsInternal, loadAccounts, withAccountStorageTransaction, withStorageLock).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/request/request-transformer.ts`:
- Around line 546-551: Add a short clarifying comment near the
defaultEffort/isLightweight logic to explain that legacy aliases (e.g.,
gpt-5-mini/gpt-5-nano) normalize to a canonical model (gpt-5.4) which supports
higher efforts, but we intentionally keep the lightweight default based on the
original alias; also note that explicit requests for xhigh are still honored via
supportsRequestedXhigh (computed from supportsXhigh and canonicalSupportsXhigh)
and the subsequent effort downgrade logic that sets effort = "high" when xhigh
isn't supported.
In `@lib/storage.ts`:
- Around line 121-128: withFallbackSeedLock and its fallbackSeedMutex add
redundant serialization because loadAccountsInternal is already called under
withStorageLock via loadAccounts and withAccountStorageTransaction; remove the
extra mutex to simplify locking: delete the fallbackSeedMutex and the
withFallbackSeedLock function and update any callers of withFallbackSeedLock
(e.g., places calling loadAccountsInternal through withFallbackSeedLock) to call
the underlying async function directly (or rely on withStorageLock), then run
tests to ensure no race conditions; alternatively, if you intend to keep it for
a future reason, add a clear comment above withFallbackSeedLock documenting that
intent and why it is required despite withStorageLock (reference symbols:
withFallbackSeedLock, fallbackSeedMutex, loadAccountsInternal, loadAccounts,
withAccountStorageTransaction, withStorageLock).
In `@test/storage.test.ts`:
- Around line 1828-1874: The test name claims concurrency but loadAccounts()
serializes via the storage lock; either rename the test to reflect serialized
behavior (e.g., "seeds project storage once when loadAccounts serializes") or
change the setup to truly exercise parallel seeding by disabling/mocking the
lock used inside loadAccounts (mock the storage lock/acquire function used by
loadAccounts to a no-op so both Promise.all calls run concurrently) and keep the
existing renameSpy/projectSeedWriteCount assertions; refer to loadAccounts,
setStoragePath, getStoragePath, and the renameSpy/projectSeedWriteCount helpers
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 37e04308-c2bb-47cb-8b1c-46f7ba3e282f
📒 Files selected for processing (6)
lib/request/helpers/model-map.tslib/request/request-transformer.tslib/storage.tstest/codex-prompts.test.tstest/request-transformer.test.tstest/storage.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/request-transformer.test.ts
- lib/request/helpers/model-map.ts
- remove redundant fallback seed mutex in storage path - clarify fallback policy scope for gpt-5.4-pro docs - rename storage fallback test to match serialized execution Co-authored-by: Codex <noreply@openai.com>
ndycode
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/getting-started.md`:
- Line 218: The summary line "Total: 21 template presets" currently folds
optional manual IDs and the wildcard "gpt-5.4*" (which implies gpt-5.4-pro) into
the count and is misleading; update the sentence to explicitly state that the 21
shipped templates exclude optional/manual Spark IDs and to replace "gpt-5.4*"
with a clear listing or note that gpt-5.4-pro is not part of the shipped
templates (refer to the existing mentions of "Total: 21 template presets",
"gpt-5.4*" and "gpt-5.4-pro") so readers understand which IDs are counted and
which are optional/excluded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c303b000-23d2-4d2d-b060-25b8f59148d6
📒 Files selected for processing (3)
docs/getting-started.mdlib/storage.tstest/storage.test.ts
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
- add JSDoc to storage locking/fallback transaction helpers - add JSDoc to request transformer collaboration/config helpers - clarify legacy lightweight GPT-5 alias default reasoning behavior Co-authored-by: Codex <noreply@openai.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/request/request-transformer.ts (2)
390-399:⚠️ Potential issue | 🟠 MajorFilter direct
name-based tools here too.This helper only inspects
function.name, but the sametoolspayload is treated as supporting bothtool.nameandtool.function.nameinextractRuntimeToolNames(). A directrequest_user_inputentry will survive non-plan mode and still be exposed to the model.Suggested fix
const filtered = tools.filter((entry) => { if (!entry || typeof entry !== "object") return true; + const directName = (entry as { name?: unknown }).name; + if (typeof directName === "string" && PLAN_MODE_ONLY_TOOLS.has(directName)) { + removed++; + return false; + } const functionDef = (entry as { function?: unknown }).function; if (!functionDef || typeof functionDef !== "object") return true; const name = (functionDef as { name?: unknown }).name;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/request/request-transformer.ts` around lines 390 - 399, The filter currently only checks (entry as { function?: unknown }).function.name and thus misses tools specified as a top-level name; update the predicate used for const filtered = tools.filter(...) to also inspect (entry as { name?: unknown }).name (or check both function.name and name) and treat either matching PLAN_MODE_ONLY_TOOLS the same way as function.name: increment removed and filter out the entry when a match is found; ensure behavior aligns with extractRuntimeToolNames() by checking both places and keeping the removed counter in sync.
484-577:⚠️ Potential issue | 🔴 CriticalUse the canonical model for default reasoning, not just
xhighpreservation.
canonicalModelNameonly affectssupportsRequestedXhighhere. The default-effort branch still follows the raw alias, sogpt-5stays onmediumandgpt-5-mini/gpt-5-nanostay onminimaleven thoughtransformRequestBody()sendsgpt-5.4. That leaves legacy aliases on different defaults than their canonical target and can still sendminimalwith the normalized model.Suggested fix
- const canonicalSupportsXhigh = - canonicalModelName === "gpt-5.4" || - canonicalModelName === "gpt-5.4-pro" || - canonicalModelName === "gpt-5.2"; + const canonicalIsGpt54General = canonicalModelName === "gpt-5.4"; + const canonicalIsGpt54Pro = canonicalModelName === "gpt-5.4-pro"; + const canonicalIsGpt52General = canonicalModelName === "gpt-5.2"; + const canonicalSupportsXhigh = + canonicalIsGpt54General || + canonicalIsGpt54Pro || + canonicalIsGpt52General; ... - const supportsXhigh = - isGpt54General || - isGpt54Pro || - isGpt52General || - isGpt53Codex || - isGpt52Codex || - isCodexMax; + const supportsXhigh = + canonicalSupportsXhigh || + isGpt53Codex || + isGpt52Codex || + isCodexMax; ... - : supportsXhigh - ? "high" - : isLightweight - ? "minimal" - : "medium"; + : supportsXhigh + ? "high" + : isLightweight && !canonicalSupportsXhigh + ? "minimal" + : "medium";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/request/request-transformer.ts` around lines 484 - 577, The defaultEffort calculation uses alias-based flags (e.g., isCodexMini, isLightweight, isGpt51General) derived from normalizedName instead of the canonical model, causing legacy aliases normalized to canonicalModelName (like gpt-5.4) to keep the wrong default; update defaultEffort to base its decisions on canonical-model-derived flags (create or reuse canonical variants such as canonicalIsCodexMini, canonicalIsGpt5Codex, canonicalIsGpt53Codex/canonicalIsGpt52Codex, canonicalIsLightweight or use canonicalSupportsXhigh) so the fallback default aligns with canonicalModelName, and ensure any subsequent adjustments (the isCodexMini clamp and supportsRequestedXhigh logic) use the canonical flags where appropriate (reference defaultEffort, isCodexMini, canonicalSupportsXhigh, supportsRequestedXhigh, and transformRequestBody).
🧹 Nitpick comments (1)
lib/storage.ts (1)
780-811: Consider documenting the multi-process race limitation.The seeding logic correctly uses
fs.accesswith ENOENT checking to guard against single-process races withinwithStorageLock. However, there's a TOCTOU window betweenfs.access(line 783) andpersistMigration(line 798) where another process could seed concurrently.Per the PR objectives, this is a known limitation. A brief inline comment would help future maintainers understand this trade-off.
📝 Suggested documentation
if (persistMigration) { const seedPath = getStoragePath(); + // Note: withStorageLock protects against single-process races, but concurrent + // processes seeding simultaneously may still race (known Windows gap). + // renameWithWindowsRetry mitigates write-side EPERM/EBUSY but doesn't prevent + // the TOCTOU between fs.access and persistMigration. try { await fs.access(seedPath); return globalFallback;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 780 - 811, Add a brief inline comment near the fs.access ... persistMigration(globalFallback) block documenting the known TOCTOU multi-process race: explain that while fs.access plus withStorageLock guards single-process races, there is a window between the access check and the call to persistMigration where another process could seed concurrently, and that this is a deliberate/accepted limitation; reference the symbols getStoragePath, fs.access, persistMigration(globalFallback), and globalFallback so future maintainers can find the exact region.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/request/request-transformer.ts`:
- Around line 390-399: The filter currently only checks (entry as { function?:
unknown }).function.name and thus misses tools specified as a top-level name;
update the predicate used for const filtered = tools.filter(...) to also inspect
(entry as { name?: unknown }).name (or check both function.name and name) and
treat either matching PLAN_MODE_ONLY_TOOLS the same way as function.name:
increment removed and filter out the entry when a match is found; ensure
behavior aligns with extractRuntimeToolNames() by checking both places and
keeping the removed counter in sync.
- Around line 484-577: The defaultEffort calculation uses alias-based flags
(e.g., isCodexMini, isLightweight, isGpt51General) derived from normalizedName
instead of the canonical model, causing legacy aliases normalized to
canonicalModelName (like gpt-5.4) to keep the wrong default; update
defaultEffort to base its decisions on canonical-model-derived flags (create or
reuse canonical variants such as canonicalIsCodexMini, canonicalIsGpt5Codex,
canonicalIsGpt53Codex/canonicalIsGpt52Codex, canonicalIsLightweight or use
canonicalSupportsXhigh) so the fallback default aligns with canonicalModelName,
and ensure any subsequent adjustments (the isCodexMini clamp and
supportsRequestedXhigh logic) use the canonical flags where appropriate
(reference defaultEffort, isCodexMini, canonicalSupportsXhigh,
supportsRequestedXhigh, and transformRequestBody).
---
Nitpick comments:
In `@lib/storage.ts`:
- Around line 780-811: Add a brief inline comment near the fs.access ...
persistMigration(globalFallback) block documenting the known TOCTOU
multi-process race: explain that while fs.access plus withStorageLock guards
single-process races, there is a window between the access check and the call to
persistMigration where another process could seed concurrently, and that this is
a deliberate/accepted limitation; reference the symbols getStoragePath,
fs.access, persistMigration(globalFallback), and globalFallback so future
maintainers can find the exact region.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 311fd39e-36c5-4729-924f-e9ccf116bda8
📒 Files selected for processing (3)
docs/getting-started.mdlib/request/request-transformer.tslib/storage.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ormalizeModel and runtime family fallback)
Validation
pm run typecheck
pm run lint
pm test
Notes
Summary by CodeRabbit
New Features
Changes
Bug Fixes
Documentation
Tests
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr promotes gpt-5.4 as the canonical default family across the plugin — replacing gpt-5.1 fallbacks in
normalizeModel,MODEL_MAPlegacy aliases,index.tsfamily routing, and all docs/configs. it also ships two independently useful changes: gpt-5.4-pro family isolation (separate prompt/cache state from gpt-5.4 base) and project-scoped account storage resilience (load from~/.opencode/openai-codex-accounts.jsonas a global fallback when project storage is absent, with optional seeding).key things that look solid:
withStorageLocknow wrapsloadAccounts(), serialising the fallback-seeding flow and eliminating the concurrent double-write race flagged in the previous reviewcanonicalSupportsXhighcorrectly lets legacy aliases likegpt-5-mini/gpt-5-nano(which normalise to gpt-5.4) pass xhigh through without downgrade — the newsupportsRequestedXhighguard is well-testedgetModelFamilyregex ordering puts thegpt-5.4-proboundary check before thegpt-5.4check, preventing false family routingtwo items worth addressing:
GPT_54_SNAPSHOT_DATEis a single hardcoded constant; future snapshot dates get regex coverage fromnormalizeModelbut noMODEL_MAPentry — consider an array so adding the next snapshot is a one-linerString(error)inloadGlobalAccountsFallback'slog.warnand the twolog.warncalls insideloadAccountsInternalfor seed-path failures can surface Windows filesystem paths (including username) beyond what the structuredfrom/tofields already carry; per the org's concurrency/token-safety policy, the error field should be sanitised to the error code or message prefix onlyConfidence Score: 4/5
Important Files Changed
expandDatedAliaseshelper and GPT_54_SNAPSHOT_DATE constant; reroutes legacy gpt-5/mini/nano to gpt-5.4; single hardcoded date means future snapshot ids need a manual code changecanonicalSupportsXhighcorrectly lets legacy aliases (gpt-5-mini/nano → gpt-5.4) keep xhigh; small ordering oddity (canonicalSupportsXhigh declared mid-block between model detection flags)String(error)in log.warn may surface Windows filesystem paths with usernames — needs explicit redaction per org policyFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[loadAccounts] -->|withStorageLock| B[loadAccountsInternal] B -->|fs.readFile project path| C{file exists?} C -->|yes| D[parse + normalizeAccountStorage] D --> E[return AccountStorageV3] C -->|ENOENT| F[migrateLegacyProjectStorageIfNeeded] F -->|migrated| E F -->|null| G[loadGlobalAccountsFallback] G -->|globalStoragePath == currentStoragePath| H[return null] G -->|read global file| I{valid schema?} I -->|no| H I -->|yes| J[normalizeAccountStorage] J -->|null| H J -->|valid| K{persistMigration?} K -->|no| L[return globalFallback] K -->|yes| M[fs.access seedPath] M -->|file exists| L M -->|ENOENT| N[persistMigration globalFallback] M -->|other error| L N -->|success| O[log Seeded project storage] N -->|EPERM/EBUSY| P[log warn, graceful degrade] O --> L P --> L C -->|other error| Q[log.error, return null]Comments Outside Diff (3)
lib/request/request-transformer.ts, line 474-536 (link)Reasoning config for rerouted legacy aliases mismatches the actual API model
after
gpt-5-mini,gpt-5-nano, andgpt-5are routed togpt-5.4via MODEL_MAP (lines 150–152),getReasoningConfigstill evaluates the original model name (lookupModel = originalModelon line 886, passed toresolveReasoningConfigon line 996–1000).for "gpt-5-mini":
normalizedName = "gpt-5-mini"(the input string, lowercased on line 428)normalizedName.startsWith("gpt-5-")→isGpt51General = truenormalizedName.includes("mini")→isLightweight = truesupportsXhigh = false(isGpt51General is not in the xhigh supporters list)defaultEffort = "minimal"(isLightweight branch)but the API call targets
gpt-5.4(line 899), which should have:supportsXhigh = true(gpt-5.4 is in line 490)defaultEffort = "high"(gpt-5.4 branches to line 513)impact:
model: "gpt-5-mini"andreasoningEffort: "xhigh"gets silently downgraded to "high" (line 534–536)the fix is to use the normalized model name for reasoning config when it differs from the original. after line 899 (where
body.model = normalizedModel), pass the normalized name to reasoning config on line 996 instead oflookupModel. keeplookupModelfor config lookup (line 887) but usenormalizedModelfor reasoning params:Prompt To Fix With AI
lib/request/request-transformer.ts, line 473-529 (link)legacy alias default reasoning effort is wrong
gpt-5-miniandgpt-5-nanoboth canonicalize togpt-5.4(viaMODEL_MAP), which has a default reasoning effort of"high". butgetReasoningConfigis called with the original model name, so for"gpt-5-mini":isGpt54General = false(raw name doesn't contain"gpt-5.4")supportsXhigh = falseisLightweight = true(contains"mini")defaultEffort = "minimal"← wrong; should be"high"same for
"gpt-5-nano"and"gpt-5".canonicalSupportsXhigh(line 460-463) was introduced to protect explicitxhighrequests but is never used in thedefaultEffortpath. fix: factorcanonicalSupportsXhighinto the default:this is a real regression: users with
gpt-5-miniin their opencode config will silently get near-zero reasoning whilegpt-5.4users get fullhigheffort, contradicting the PR's stated intent that these aliases are equivalent.lib/request/request-transformer.ts, line 473-530 (link)isLightweightfires on legacy aliases that now resolve togpt-5.4, causing wrong default effortgpt-5-miniandgpt-5-nanoboth now map togpt-5.4vianormalizeModel, butgetReasoningConfigstill evaluatesisLightweightagainst the pre-normalization model name string. becausenormalizedName.includes("mini")andnormalizedName.includes("nano")are true,isLightweight = true, anddefaultEffortresolves to"minimal"(line 527-528).the problem:
gpt-5.4called directly defaults to"high"(sinceisGpt54General = true→supportsXhigh = true→defaultEffort = "high"), butgpt-5-minicalled with no effort preference reaches the same api endpoint witheffort = "minimal". theDATED_ALIAS_EFFORT_SUFFIXESlist (none,low,medium,high,xhigh) does not includeminimalforgpt-5.4, so this mismatch may surface as api rejections for callers who omit an explicit effort.there's a parallel fix already present for xhigh (
canonicalSupportsXhighat line 460-463 ensures xhigh is not downgraded for canonical-gpt-5.4 aliases), but no equivalent guard forisLightweight:confirmed by the test at
test/request-transformer.test.ts:2103which assertseffort === "minimal"forgpt-5-mini— this test is encoding the wrong behavior ifgpt-5.4does not acceptminimalas a valid reasoning level.Last reviewed commit: 417fa71
Context used: