audit: close 8 correctness/robustness gaps across api, registry, and tooling - #521
Conversation
…tooling Findings from a full-code review (issue #520), each with a test that kills its mutant: - promote: `sources` now passes the same overlong() ceiling every sibling list-shaped input enforces — a request could previously buy CPU/memory proportional to an attacker-chosen id count. - registry/embeddings: embeddings_status, semantic_twins, semantic_resolve, and explain_semantic_resolve now hold the entry's tombstone fence over their sidecar loads — a lost race with delete answers None (the context endpoint's own 404), never a status built from unlinked or successor-generation sidecars. The fence is scoped: provider round trips and the O(N²) sweep run outside it. - import: the group-restore timeout arm now carries integrity/durable_batches machine-readably, like the batch loop's own timeout refusal. - benchmark/compare: wall_seconds drops end-before-start samples instead of sinking the latency distribution; a re-processed document's second `end` record supersedes the first (mirroring start's keep-the-earliest, not repeating it). - embedding: PassageAnnIndex::build on an empty store returns an empty index instead of panicking on an out-of-bounds slice. - evalset: case_id emptiness is trimmed like every other field's. - metrics: note_replica_shipped clears behind_since_epoch on a caught-up lane, the same three-way move note_replica_lane makes. Closes #520 Claude-Session: https://claude.ai/code/session_011NozdDS9JqgCpi9Z3wo4Pd
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAPIの復元タイムアウトとプロモーション入力を検証します。ベンチマーク計測とレプリケーション遅延を修正します。空データ、評価ケースID、埋め込み削除競合の処理と回帰テストを追加します。 ChangesAPIの検証と拒否応答
ベンチマークとメトリクスの正確性
入力と空データの処理
埋め込み読み取りの削除競合制御
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/api/import.rs`:
- Around line 421-433: 追加された RestoreGroupsError::Timeout の拒否応答形状を、/import の
golden wire fixture と tests/http_api/contract.rs の契約テストで検証してください。restore_refusal
に渡す total は、全バッチ成功後の group restore であるため durable batch 数と一致させ、integrity と
durable_batches も応答内容に含めてください。HTTP_CONTRACT と MCP_CONTRACT は変更しないでください。
In `@src/metrics.rs`:
- Around line 498-514: Extend the test
a_shipped_report_at_or_below_applied_clears_the_behind_age to also call
note_replica_shipped with shipped_seq 4 while applied_seq remains 5, then assert
replica_lag[&key].behind_since_epoch is 0. Keep the existing equality case and
verify the less-than case explicitly.
🪄 Autofix
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 Plus
Run ID: 2e7984ac-c962-4ab7-9b7d-bbea30de7334
📒 Files selected for processing (12)
src/api.rssrc/api/import.rssrc/api/promote.rssrc/benchmark/compare.rssrc/benchmark/compare/tests.rssrc/embedding.rssrc/evalset.rssrc/metrics.rssrc/metrics/record.rssrc/registry/embeddings.rssrc/registry/embeddings/gloss_tests.rstests/http_api/promote.rs
… test CodeRabbit on #521: - New golden wire fixture (import_refusal_durable_prefix) + contract test pinning the /import refusal envelope's integrity/durable_batches — recorded from the deterministic mid-stream rejection, since the group-restore Timeout arm only fires when the deadline dies in the unhittable window between batch loop and group phase; that arm's field set stays pinned by the api.rs unit test. `response.integrity` joins shapes.json's enums. - The replica-age test now also drives shipped_seq strictly below applied_seq, so a `>=` → `==` mutant in note_replica_shipped's caught-up arm no longer survives. Claude-Session: https://claude.ai/code/session_011NozdDS9JqgCpi9Z3wo4Pd
Summary
全 Rust コード監査(#520)で見つかった 8 件の修正。各修正にミュータントを殺すテスト付き。
sourcesに他のリスト入力と同じoverlong()上限(1000 件)を適用 — 従来は入力サイズ比例の CPU/メモリを無制限に消費し得たembeddings_status/semantic_twins/semantic_resolve/explain_semantic_resolveのサイドカー読みを tombstone フェンスで保護 — delete に負けた読みは context エンドポイントと同じ「存在しない」(None) を返し、unlink 済み・後継世代のサイドカーから状態を組み立てない。フェンスはスコープ限定で、プロバイダ呼び出しと O(N²) スイープはフェンス外(delete をネットワーク待ちにしない)integrity/durable_batchesを機械可読で付与(バッチループ側のタイムアウトと同じ契約)wall_secondsが end<start の壊れたサンプルを破棄。重複endレコードは最後の記録が勝つ(start の keep-earliest と対称)PassageAnnIndex::buildが空ストアで空インデックスを返す(従来は範囲外スライスの潜在パニック)case_idの空チェックを他フィールド同様 trim 付きにnote_replica_shippedがキャッチアップ時にbehind_since_epochをリセット(note_replica_laneと対称)Closes #520
Test plan
cargo fmt/cargo clippy --all-targets(警告ゼロ)cargo testフルスイート 3211 件パス(失敗 0)semantic_twins既存ロジック由来の 508 件は変更範囲外のため除外。CI の 60 件予算も超過する規模なので、必要なら mutants-sweep.yml のモジュールスイープで別途カバー)https://claude.ai/code/session_011NozdDS9JqgCpi9Z3wo4Pd
Summary by CodeRabbit
新機能
バグ修正