fix(availability): restore status_code IS NOT NULL terminal filter#1189
Conversation
可用性监控的 provider + 时间范围聚合改用 status_code IS NOT NULL 收敛 终态边界，与部分索引 idx_message_request_provider_created_at_finalized_active (deleted_at IS NULL AND status_code IS NOT NULL) 对齐，让热路径查询 能稳定命中索引；同时避免把仅写入 providerChain / errorMessage 片段但 statusCode 仍为 NULL 的"请求中"记录算入聚合（之前的内联终态谓词复刻 fn_is_message_request_finalized 语义后，会让分类函数把它们误算成 failure）。 终态记录的成功/失败/排除分类继续由 fn_compute_message_request_success_rate_outcome(...) 完成。 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough将 availability-service 中的“已终态(finalized)”判定由复杂的 providerChain/blockedBy/errorMessage 组合逻辑重写为仅以 message_request.status_code 非空为准，并同步更新相关单元测试的 SQL 断言以匹配该新边界。 变更终态判定逻辑简化
评估代码审查工作量🎯 4 (Complex) | ⏱️ ~45 minutes 可能相关的其他 PR
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 simplifies the 'finalized request' logic in the availability service by replacing a complex multi-field SQL condition with a single status_code IS NOT NULL check. This change optimizes database performance by aligning with the existing partial index and prevents in-progress requests from being misclassified as failures. Unit tests have been updated to verify the simplified SQL generation and ensure that outcome classification still functions correctly. I have no feedback to provide as there were no review comments to assess.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/lib/availability-service.test.ts (1)
312-325: ⚡ Quick win把
error_message也纳入负向断言。现在这些用例只排除了
blocked_by和provider_chain路径；如果 finalized 条件回归成"status_code" is not null or "error_message" is not null，测试依然会通过，但会重新把仅写入errorMessage、statusCode仍为NULL的记录纳入可用性统计。建议把这组断言提成一个 helper，并补上对"error_message" is not null的排除断言。可考虑这样收紧断言
+function expectStatusCodeOnlyFinalizedBoundary(sqlText: string) { + expect(sqlText).toContain(`"status_code" is not null`); + expect(sqlText).not.toContain(`"blocked_by" is not null`); + expect(sqlText).not.toContain(`"error_message" is not null`); + expect(sqlText).not.toContain(`"provider_chain" -> -1 ->> 'reason'`); +} + ... - expect(finalizedRequestsSql).toContain(`"status_code" is not null`); - expect(finalizedRequestsSql).not.toContain(`"blocked_by" is not null`); - expect(finalizedRequestsSql).not.toContain(`"provider_chain" -> -1 ->> 'reason'`); + expectStatusCodeOnlyFinalizedBoundary(finalizedRequestsSql);Also applies to: 494-499, 533-535, 568-575, 742-747
🤖 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 `@tests/unit/lib/availability-service.test.ts` around lines 312 - 325, The tests assert finalizedRequestsSql and queryText must exclude certain non-finalized paths but miss `"error_message" is not null`; update the assertions (and extract them into a reusable helper) so each check for finalizedRequestsSql includes expect(finalizedRequestsSql).not.toContain(`"error_message" is not null`) alongside the existing not.toContain checks for `"blocked_by" is not null` and `"provider_chain" -> -1 ->> 'reason'`, and reuse that helper across all affected cases (references: finalizedRequestsSql, queryText, fn_is_message_request_finalized, fn_compute_message_request_success_rate_outcome, and the `"status_code" is not null` positive assertion).
🤖 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.
Nitpick comments:
In `@tests/unit/lib/availability-service.test.ts`:
- Around line 312-325: The tests assert finalizedRequestsSql and queryText must
exclude certain non-finalized paths but miss `"error_message" is not null`;
update the assertions (and extract them into a reusable helper) so each check
for finalizedRequestsSql includes
expect(finalizedRequestsSql).not.toContain(`"error_message" is not null`)
alongside the existing not.toContain checks for `"blocked_by" is not null` and
`"provider_chain" -> -1 ->> 'reason'`, and reuse that helper across all affected
cases (references: finalizedRequestsSql, queryText,
fn_is_message_request_finalized,
fn_compute_message_request_success_rate_outcome, and the `"status_code" is not
null` positive assertion).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a4f203b-de64-4fd8-9615-6e05e5f09567
📒 Files selected for processing (2)
src/lib/availability/availability-service.tstests/unit/lib/availability-service.test.ts
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. The change correctly restores as the terminal-state filter, aligning the availability query with the partial index predicate and eliminating the CPU spike caused by planner-opaque inlined SQL. Tests were TDD'd and validate the new boundary behavior.
PR Size: S
- Lines changed: 138 (38 additions, 100 deletions)
- Files changed: 2
Review Coverage
- Logic and correctness - Clean. The simplified predicate aligns with and correctly excludes in-flight records that were previously misclassified as failures.
- Security (OWASP Top 10) - Clean. No user input reaches raw SQL; all parameters remain properly escaped through Drizzle ORM.
- Error handling - Clean. No new catch blocks or error paths introduced.
- Type safety - Clean. is a standard Drizzle type-safe operator, consistent with existing usage in the same file.
- Documentation accuracy - Clean. The new JSDoc accurately explains the rationale, known limitations, and why the function-call approach was abandoned.
- Test coverage - Adequate. Tests assert both the new predicate and the absence of the old complex SQL; edge cases (Gemini passthrough, intermediate persistence state) are covered.
- Code clarity - Good. Removing ~60 lines of opaque SQL and the constants dramatically improves maintainability.
Automated review by Claude AI
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. The change correctly restores status_code IS NOT NULL as the terminal-state filter, aligning the availability query with the partial index predicate and eliminating the CPU spike caused by planner-opaque inlined SQL. Tests were TDD'd and validate the new boundary behavior.
PR Size: S
- Lines changed: 138 (38 additions, 100 deletions)
- Files changed: 2
Review Coverage
- Logic and correctness - Clean. The simplified predicate aligns with
idx_message_request_provider_created_at_finalized_activeand correctly excludes in-flight records that were previously misclassified as failures. - Security (OWASP Top 10) - Clean. No user input reaches raw SQL; all parameters remain properly escaped through Drizzle ORM.
- Error handling - Clean. No new catch blocks or error paths introduced.
- Type safety - Clean.
isNotNullis a standard Drizzle type-safe operator, consistent with existingisNullusage in the same file. - Documentation accuracy - Clean. The new JSDoc accurately explains the rationale, known limitations, and why the function-call approach was abandoned.
- Test coverage - Adequate. Tests assert both the new predicate and the absence of the old complex SQL; edge cases (Gemini passthrough, intermediate persistence state) are covered.
- Code clarity - Good. Removing ~60 lines of opaque SQL and the
FINALIZED_PROVIDER_CHAIN_REASONSconstants dramatically improves maintainability.
Automated review by Claude AI
…ssage 把"终态边界只看 status_code IS NOT NULL"的 5 处断言抽到 expectStatusCodeOnlyFinalizedBoundary helper，并补上对 "error_message" is not null 的负向断言：之前只排除 blocked_by / provider_chain 路径，如果未来 finalized 回归成 "status_code" is not null or "error_message" is not null， 仅写入 errorMessage 的"请求中"记录会重新被纳入可用性统计而不被发现。 Addresses CodeRabbit nitpick on #1189. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR.
PR Size: S
- Lines changed: 138
- Files changed: 2
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Automated review by Codex AI
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
- Identified PR
#1189(fix(availability): restore status_code IS NOT NULL terminal filter) and reviewed the full diff for changed files:src/lib/availability/availability-service.tsandtests/unit/lib/availability-service.test.ts. - Computed PR size as S (38 additions, 100 deletions; 138 total lines changed across 2 files) and applied label
size/Sto the PR. - Ran a diff-scoped, multi-perspective review (comments, tests, error handling, types, logic/security/perf, simplification) and found no issues that met the reporting threshold (>= 80 confidence) after full-file/context validation.
- Posted the required PR review summary via
gh pr review --comment(no inline comments, since there were no validated reportable diff-line issues).
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Related Issues & PRs
status_code IS NOT NULL终态边界与部分索引。背景
可用性监控的 provider + 时间范围聚合查询加载很久。热路径原本依赖
message_request上的部分索引：近期的"已终态"判定被改成内联复刻
fn_is_message_request_finalized(...)的语义（status_code / blocked_by / errorMessage / providerChain 任一非空即视为终态）。这样做有两个问题：status_code IS NOT NULL作为可单独证明的谓词分支，Postgres 难以判定它蕴含索引谓词，常退化为大范围扫描；providerChain/errorMessage片段、但statusCode仍为 NULL 的"请求中"记录也判为终态。fn_compute_message_request_success_rate_outcome(...)会把这些请求中的中间状态算成failure，污染可用性数据。改动
buildAvailabilityFinalizedCondition恢复为status_code IS NOT NULL，重新对齐部分索引，同时去掉对内联 finalized 谓词的依赖（以及随之而来的 jsonb /CASE复杂度和FINALIZED_PROVIDER_CHAIN_REASONS_SQL常量）。fn_compute_message_request_success_rate_outcome(...)，逻辑不变。status_code IS NOT NULL、不再包含fn_is_message_request_finalized或blocked_by/provider_chain reason内联片段、且仍由 outcome 函数做分类"作为断言写到红，再回到生产代码上让测试转绿。Test plan
bun run typecheckbun run lintbun run buildbun run test— 5969/5982 passed (13 skipped)bunx vitest run --config tests/configs/{integration,my-usage,quota,proxy-guard-pipeline,public-status.integration,thinking-signature-rectifier,codex-session-id-completer,include-session-id-in-errors,logs-sessionid-time-filter,usage-logs-sessionid-search}.config.ts全部 exit 0bunx vitest run tests/unit/lib/availability-service.test.ts— 19/19 passedtests/configs/e2e.config.ts) 本地未跑通：依赖 13500 端口上的 dev server（pre-existing onorigin/dev同样如此），与本改动无关EXPLAIN大时间范围的 availability 聚合查询，确认 partial index 被命中🤖 Generated with Claude Code
Greptile Summary
This PR restores the
buildAvailabilityFinalizedConditionfunction to the simplestatus_code IS NOT NULLpredicate, replacing the previously inlined multi-branch OR expression that mirroredfn_is_message_request_finalized. The simplified predicate re-aligns with the partial indexidx_message_request_provider_created_at_finalized_activeand stops partially-written records (e.g. rows with onlyproviderChain/errorMessagebut nostatus_code) from being pulled into availability aggregation as false failures.buildAvailabilityFinalizedConditionis reduced to one line (isNotNull(messageRequest.statusCode)), removing ~50 lines of JSONB CASE guards and theFINALIZED_PROVIDER_CHAIN_REASONS_SQLconstant.expectStatusCodeOnlyFinalizedBoundaryhelper that asserts the four contract invariants (index-aligned predicate present;blocked_by,error_message, andprovider_chainpaths absent) and are applied consistently across all relevant test cases.Confidence Score: 5/5
Safe to merge — the change narrows the finalized-request filter to a single SARGable predicate that matches the existing partial index, and all 19 unit tests pass.
The production change is a straightforward simplification: a 50-line multi-branch SQL expression is replaced by a single Drizzle call, and every call site remains unchanged. The new predicate is a strict subset of the old one, which is intentional and well-documented. Tests cover the boundary contract with a shared helper applied consistently across four test cases.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Incoming message_request rows] --> B{deleted_at IS NULL?} B -- No --> SKIP1[Excluded — soft-deleted] B -- Yes --> C{status_code IS NOT NULL?\nbuildAvailabilityFinalizedCondition} C -- No --> SKIP2[Excluded — in-progress / partial write] C -- Yes --> D[Finalized requests CTE] D --> E[fn_compute_message_request_success_rate_outcome\nblocked_by, status_code, error_message, provider_chain] E --> F{outcome} F -- success --> GREEN[greenCount] F -- failure --> RED[redCount] F -- excluded --> EXCL[Excluded from counts] GREEN & RED --> AGG[Bucket aggregation\nprovider_bucket_stats] AGG --> RESULT[AvailabilityQueryResult]Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "test(availability): factor finalized-bou..." | Re-trigger Greptile