fix(api): address PR #972 review findings for release readiness#974
Conversation
Correct migration FK targets (`user` not `users`), unblock BYOK session creation, and harden compose projection, SSE mapping, and resume validation. Co-authored-by: Cursor <cursoragent@cursor.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Your plan includes 5 reviews of capacity. Refill in 48 minutes and 26 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors compose backend pre-flight checks to use stored BYOK credentials rather than per-model provider validation; allows compose-phase ChangesCompose Validation & Event Handling
Sequence DiagramsequenceDiagram
participant Client
participant assertComposeBackendReady
participant FetchModelIDs
participant UserAIStore
Client->>assertComposeBackendReady: request pre-flight check for graphId
assertComposeBackendReady->>FetchModelIDs: fetch modelIds for graphId
alt No models configured
FetchModelIDs-->>assertComposeBackendReady: empty -> return early
else Models present
assertComposeBackendReady->>UserAIStore: derive expectedProvider & fetch credential for user
UserAIStore-->>assertComposeBackendReady: credential or null
alt Credential missing
UserAIStore-->>assertComposeBackendReady: respond with HTTPException(400)
else Credential found
UserAIStore-->>assertComposeBackendReady: success -> allow compose
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies database foreign keys, refines BYOK backend validation, and wraps chat model streaming in a try...finally block for reliable usage recording. It also improves web search model selection, adds schema validation for research sources, and updates SSE mapping. Feedback highlights a risk of error masking in the usage recording logic and identifies redundant conditional checks in model resolution and session projection.
| const usage = await recordZediUsage({ | ||
| db: this.db, | ||
| userId: this.userId, | ||
| modelId: this.modelRowId, | ||
| feature: this.feature, | ||
| usage: { inputTokens, outputTokens }, | ||
| inputCostUnits: this.inputCostUnits, | ||
| outputCostUnits: this.outputCostUnits, | ||
| apiMode: this.apiMode, | ||
| }); |
There was a problem hiding this comment.
finally ブロック内で recordZediUsage を await していますが、この処理が失敗した場合に try ブロックで発生した元のエラーが上書き（マスク）されてしまう可能性があります。利用状況の記録は重要ですが、ここでの例外が原因で最終的なチャンクの yield や後続のクリーンアップが阻害されるのを防ぐため、recordZediUsage を try-catch で囲んでエラーをハンドル（ログ出力など）することを検討してください。
There was a problem hiding this comment.
対応しました。finally を廃止し、done === true 時のみ recordZediUsage を呼びます。recordZediUsage 自体は try/catch で囲み、失敗時は console.error のうえ推定トークンで最終チャンクを返します（成功ストリームの例外マスクを防止）。
| const first = rows[0]; | ||
| if (!first) return null; |
There was a problem hiding this comment.
直前の 94 行目で rows.length === 0 の場合に null を返しているため、この if (!first) チェックは冗長です。コードを簡潔にするために削除を検討してください。
References
- 簡潔で意図が明確なコードを推奨します。 (link)
- 不要な条件分岐や到達不能なコードを削除することで、コードの可読性と保守性を向上させます。
There was a problem hiding this comment.
対応しました。rows.length === 0 の直後に rows[0] を直接参照する形に整理し、冗長な if (!first) を削除しました。
| if (typeof state.phase === "string" && projection.phase === undefined) { | ||
| projection.phase = phaseFromSessionRow(state.phase, "interrupted"); | ||
| projection.phase = | ||
| state.phase.startsWith("completed") || state.phase === "completed" |
There was a problem hiding this comment.
state.phase.startsWith("completed") が真であれば state.phase === "completed" の条件も満たされるため、後者のチェックは冗長です。コードを簡潔にするために削除を検討してください。
References
- 冗長な論理演算を避けることを推奨します。 (link)
- 論理的に冗長な条件式を整理することで、コードの意図をより明確にします。
There was a problem hiding this comment.
対応しました。startsWith("completed") のみに簡略化しました。
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e422fe50b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const usage = await recordZediUsage({ | ||
| db: this.db, | ||
| userId: this.userId, | ||
| modelId: this.modelRowId, | ||
| feature: this.feature, |
There was a problem hiding this comment.
recordZediUsage now runs unconditionally in finally, so it executes even when the provider stream throws before completion; this is reachable because streamOpenAI/streamAnthropic/streamGoogle throw on non-2xx responses in server/api/src/services/aiProviders.ts. In those failure paths the request fails but we still charge estimated prompt tokens, which can inflate usage/budget accounting and bill failed calls; billing should be limited to successful completion (or a narrowly-detected abort case).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
対応しました。finally による無条件課金をやめ、routes/ai/chat.ts と同様に done === true のときだけ recordZediUsage を呼ぶようにしました。プロバイダ例外は for await からそのまま伝播するため課金コードに到達しません。incomplete 終了は usage メタデータのみ返し DB 課金しません。テスト does not record usage when the provider stream throws を追加済みです。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
server/api/src/__tests__/routes/composeSessionProjection.test.ts (1)
66-85: ⚡ Quick winAdd a spec case for prefixed completed phases.
The implementation now treats
completed*values ascompleted, but this test only locks in the exact"completed"string. Add one case for a prefixed phase so that behavior is part of the spec too.🧪 Suggested test
it("projects completion markdown from checkpoint values", () => { const projection = projectComposeStateValues({ phase: "completed", completion: { markdown: "## A\n\nBody", @@ expect(projection.completedMarkdown).toBe("## A\n\nBody"); expect(projection.draftedSections).toHaveLength(1); expect(projection.phase).toBe("completed"); }); + + it("maps prefixed completed phases to completed", () => { + const projection = projectComposeStateValues({ + phase: "completed:await_persist", + }); + expect(projection.phase).toBe("completed"); + });As per coding guidelines,
**/*.{test,spec}.{ts,tsx,js,jsx}: Tests serve as a source of truth for specifications alongside implementation code TSDoc/JSDoc.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/src/__tests__/routes/composeSessionProjection.test.ts` around lines 66 - 85, Add a test asserting that phases that start with "completed" are treated as completed: update the test using projectComposeStateValues by adding a case where the input phase is a prefixed string (e.g., "completed-foo" or "completedWithNotes") with the same completion object and assert projection.completedMarkdown, projection.draftedSections.length, and projection.phase are the expected completed values (use the same assertions as the existing "projects completion markdown from checkpoint values" test but with the prefixed phase).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/api/src/agents/core/composeBackendValidation.ts`:
- Around line 29-31: Update the TSDoc/JSDoc in composeBackendValidation.ts (the
top-level doc for the composeBackendValidation function / createZediChatModel
related functions) so it no longer claims static model-provider compatibility is
validated here; instead state in English and Japanese that this module only
checks credential existence and that provider matching for BYOK runs is enforced
at runtime via resolveComposeModelId, and add a Japanese translation of the
nearby inline comment ("Model-less graphs... Provider matching...") so both
languages mirror the inline note.
In `@server/api/src/agents/core/llm/zediChatModel.ts`:
- Around line 294-296: The new inline comments (e.g., the one starting "Surface
incremental tokens to LangChain callback consumers..." and the other comments
around the same block referenced at lines ~316 and ~334) are English-only;
update them to be bilingual by adding equivalent Japanese translations alongside
the existing English text while preserving meaning and tone and keeping the
comments adjacent to the same code (references: the comment that mentions
`streamEvents` and "LangChain callback consumers" and the nearby comment
blocks). Ensure formatting matches existing TS comment style used across the
repo (both languages in the same comment block).
- Around line 315-332: The finally block currently awaits recordZediUsage(...)
which can reject and interfere with the primary stream error; wrap the
recordZediUsage call in its own try/catch inside the finally so any rejection is
caught and only logged (e.g., this.logger.error(...) or console.error) and not
re-thrown, ensuring the original generator error propagates; keep using
promptLength/zediMessages/accumulated/inputTokens/outputTokens as computed but
move the await into the guarded try/catch. Also update the inline comments
around the finally/usage calculation (references to lines near zediMessages,
promptLength and recordZediUsage) to include both English and Japanese
translations per the bilingual guideline.
---
Nitpick comments:
In `@server/api/src/__tests__/routes/composeSessionProjection.test.ts`:
- Around line 66-85: Add a test asserting that phases that start with
"completed" are treated as completed: update the test using
projectComposeStateValues by adding a case where the input phase is a prefixed
string (e.g., "completed-foo" or "completedWithNotes") with the same completion
object and assert projection.completedMarkdown,
projection.draftedSections.length, and projection.phase are the expected
completed values (use the same assertions as the existing "projects completion
markdown from checkpoint values" test but with the prefixed phase).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dcfcf33d-df4b-4a51-9c7e-32dabcd47fbf
📒 Files selected for processing (14)
server/api/drizzle/0031_add_wiki_compose_sessions.sqlserver/api/drizzle/0032_add_user_ai_credentials.sqlserver/api/src/__tests__/agents/core/composeBackendValidation.test.tsserver/api/src/__tests__/agents/runner/sseMapper.test.tsserver/api/src/__tests__/routes/composeSessionProjection.test.tsserver/api/src/agents/core/checkpoint/postgresCheckpointer.tsserver/api/src/agents/core/composeBackendValidation.tsserver/api/src/agents/core/llm/zediChatModel.tsserver/api/src/agents/core/tools/resolveWebSearchModel.tsserver/api/src/agents/core/types/index.tsserver/api/src/agents/runner/sseMapper.tsserver/api/src/agents/subgraphs/research/resumeSchema.tsserver/api/src/routes/composeSessionProjection.tsserver/api/src/services/wikiSearchService.ts
Bill streaming usage only on successful done chunks, skip DB writes on provider errors and incomplete streams, and sync bilingual TSDoc. Co-authored-by: Cursor <cursoragent@cursor.com>
|
レビューコメントへの対応をコミットしました（d7e7488a）。
@coderabbitai review |
|
PR全体のレビューを実施します。 🐇 ✨ ✅ Actions performedReview triggered.
|
Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
概要
PR #972（develop → main リリース）のレビュー・CI で検出された正当な問題を修正します。主因はマイグレーション FK が存在しない
"users"テーブルを参照していたことによる deploy-dev 失敗です。変更点
server/api/drizzle/0031/0032の FK を"users"("id")→"user"("id")に修正（Better Auth の実テーブル名に合わせる）server/api/src/agents/core/setup()失敗時にsetupOnceをリセットして再試行可能にtry/finally化SseComposePhaseEvent/SseComposeSectionEventを barrel export に追加server/api/src/agents/runner/mapComposePhaseに"conflict"フェーズを追加server/api/src/routes/composeSessionProjection: checkpointphase: "completed"を正しく"completed"に投影server/api/src/agents/subgraphs/research/researchResumeSchema:approvedSourceIdsを必須化、approved/rejected の重複を拒否server/api/src/services/wikiSearchService:limitの NaN ガード、updated_atの Date 正規化変更の種類
テスト方法
cd server/api && bun run test:run -- src/__tests__/agents/core/composeBackendValidation.test.ts src/__tests__/routes/composeSessionProjection.test.ts src/__tests__/agents/runner/sseMapper.test.ts src/__tests__/services/wikiSearchService.test.ts src/__tests__/agents/subgraphs/research/researchGraph.resume.test.ts src/__tests__/agents/core/llm/zediChatModel.test.tsチェックリスト
関連 Issue / PR
Related to #972
レビューコメント対応メモ（対応しないもの）
waitForTimeout置換SECURITY.ja.mdplaceholderMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests