fix: improve generation failure diagnostics#315
Conversation
There was a problem hiding this comment.
Review mode: initial
Findings
-
[Minor] The
hasModelsPrefixregex/^models\//iis case-insensitive, which is fine. However, thementionsModelsPrefixregex\bmodels\/[-._:\/a-z0-9]+\buses a character class that includes:, but the forward slash in the pattern is literal (escaped). This works as expected, but note thatmodels/prefix detection viactx.messageis only triggered under 400/404/422 status codes. This is a reasonable heuristic and not a bug. -
[Nit] Spanish (
es.json) and Portuguese (pt-BR.json) locale files include additional string additions (e.g.,unsupportedRole,reasoningPolicy,normalizeModelId, etc.) that are not strictly part of this PR's scope. These are valuable completions and do not introduce any issues, but could be noted as bonus cleanup.
No blockers or major issues found. The changes are well-structured, properly tested, and follow project conventions.
Summary
- Adds
generation-timeoutdiagnostic category with awarningseverity andopenSettingsfix pointing to Advanced tab. - Expands
model-id-shapedetection to handle 400/404/422 errors withmodels/prefix in modelId or message. - Updates i18n for en, es, pt-BR, zh-CN with new keys for timeout and model-id-shape diagnostics.
- Adds unit tests for both new diagnostics in
packages/shared/src/diagnostics.test.ts. - Adds integration tests in
apps/desktop/src/renderer/src/store.test.tsfor toast action and store state changes. - No new dependencies added; license constraints respected.
- Linked issue #314 is referenced as "Part of #314" (not closing claim), so no closure validation needed.
Testing
packages/shared/src/diagnostics.test.tscovers both new diagnostic paths.apps/desktop/src/renderer/src/store.test.tscovers the store's handling of generation timeouts and model-id-shape errors viarunFailingGenerate.- Existing regression coverage for
diagnoseanddiagnoseGenerateFailureremains intact.
Open-CoDesign Bot
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
No new issues. The previous review noted only a minor observation about regex patterns and a nit about bonus locale strings — neither requires action.
The new change in pnpm-workspace.yaml adds onlyBuiltDependencies: - electron to allow the Electron native module to build in pnpm 10 CI. This is a safe, targeted fix that does not introduce any license, security, or behavioral concerns. It aligns with the project's build/CI tooling conventions (copyleft-safe because it only affects CI).
All previous findings are resolved or were non-blocking. The diagnostics logic, tests, and i18n updates remain solid.
Summary
- The PR correctly adds
generation-timeoutdiagnostic, improvesmodel-id-shapedetection for bare 400/404/422 errors withmodels/prefix, and updates all four locales. - Unit and integration tests cover both new diagnostic paths including store toast action routing.
- The new
pnpm-workspace.yamladdition enables Electron install script execution in pnpm 10 without side effects. - No new dependencies, no license conflicts, no hard-coded UI tokens, no silent fallbacks.
- The PR references #314 as "Part of" — no premature closure claim to validate.
Testing
packages/shared/src/diagnostics.test.ts— passes.apps/desktop/src/renderer/src/store.test.ts— passes.- No Playwright E2E tests added; the changes are diagnostic-only and adequately covered by unit tests.
Ready to merge.
Open-CoDesign Bot
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
No new issues. The previous review identified no blockers, and the latest diff addresses the only concern (test inter-test pollution) by extracting a shared registerIpcForTest() helper and adding timeouts to each test case. The helper properly resets shared state (registeredChannels, handlers) before each test, preventing stale state from one assertion leaking into another.
Summary
- The refactored test setup in
onboarding-ipc.test.tsnow callsregisterIpcForTest()per-test, which clears and re-initializes the shared channel/handler state, making the tests independent and robust. - 15 s timeout is generous but reasonable for mocked async imports.
- No other files changed, no new dependencies, no hard-coded UI tokens, no license issues.
- The PR remains marked as "Part of #314" — no premature closure claim.
Ready to merge.
Open-CoDesign Bot
Summary
generation-timeoutdiagnostic forGENERATION_TIMEOUTerrorsmodels/prefixed model IDs for 400/404/422 provider errors even when the gateway returns no useful bodyelectroninstall script in pnpm 10 so CI desktop tests can import Electron after a fresh installPart of #314.
Verification
pnpm exec biome check packages/shared/src/diagnostics.ts packages/shared/src/diagnostics.test.ts apps/desktop/src/renderer/src/store.test.ts packages/i18n/src/locales/en.json packages/i18n/src/locales/es.json packages/i18n/src/locales/pt-BR.json packages/i18n/src/locales/zh-CN.jsonpnpm exec biome check apps/desktop/src/main/onboarding-ipc.test.ts pnpm-workspace.yamlpnpm --filter @open-codesign/shared exec vitest run src/diagnostics.test.tspnpm --filter @open-codesign/desktop exec vitest run src/renderer/src/store.test.tspnpm --filter @open-codesign/desktop exec vitest run src/main/onboarding-ipc.test.tspnpm typecheckpnpm install --frozen-lockfile --config.confirmModulesPurge=false --reporter=append-only