fix(detection): C1 PR-3 — tighten batch detector (closes part 3 of #83)#109
Conversation
Eliminates 8 of 9 corpus batch false positives by gating both the TS AST sequential-batching detector and the Python detectSequentialBatching on enclosing function. Calls in different functions of the same file execute on independent code paths and cannot be batched together. TypeScript (src/ast/waste/batch-detector.ts): - detectSequential() now buckets by (provider, enclosingFunction) instead of provider alone. Module-level calls bucket to "<module>" so they cluster only with other module-level calls. - Within a bucket, dedupe by line — cross-file resolver expansion can produce multiple AstCallMatch entries at the same source line for one user-written call (e.g., bedrock-raw-fetch/src/index.ts where one await handleApi(...) resolves through 2 internal wrapper functions). - Bucket Map carries the provider as a struct field rather than encoding it in the key string — avoids a "::" delimiter footgun if a future provider id contains the separator. - Evidence string lists unique sorted lines. Python (src/scanner/python-waste-detector.ts): - detectSequentialBatching() applies the same (providerKey, enclosingFunction) bucketing. The cluster.length >= 3 proximity threshold is preserved (Python doesn't have explicit await sequencing as a clear N+1 indicator, so it warrants a stronger signal). Measurement (benchmark/baseline.json, docs/accuracy/findings.md): - batch: 9 FP / 0 TP / 1 FN → 1 FP / 0 TP / 1 FN - finding precision: 9.09% → 33.33% globally (+24.24pp) - All other per-detector metrics unchanged - No detection or recall regressions - Sample size for batch falls below the per-type gate's >=3 threshold, so the gate skips it (next regression on batch will need 3+ emissions before failing the build) Tests (src/test/c1-pr3-batch-tightening.test.ts, fixtures/c1-pr3/): - 4 new test cases pinning both regression directions for both languages: cross-function calls (-) and same-function calls (+). - Total: 357 PASS. Remaining 1 batch FP is bedrock-raw-fetch/src/index.ts:5 — two sequential `await handleApi(...)` calls in `main()`. Arguably a true positive (Promise.all would parallelize them) that the corpus didn't label. Fixing it cleanly requires either control-flow-branch awareness in the detector or a corpus annotation update; both are scoped to a follow-up PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Summary
Eliminates 8 of 9 corpus batch false positives (9 → 1) by gating the sequential-batching detectors on enclosing function. Calls in different functions of the same file execute on independent code paths and cannot be batched together.
Builds on #106 (per-detector measurement) and #108 (cache detector). Same investigative shape, same review pattern.
What changed
TypeScript detector (
src/ast/waste/batch-detector.ts)detectSequential()now buckets by(provider, enclosingFunction)instead ofprovideralone:\"<module>\"so they cluster only with other module-level calls.{provider, matches}) instead of encoding it in the key — avoids a\"::\"delimiter footgun if a future provider id contains the separator. Per CodeRabbit-style code-quality review.AstCallMatchentries at the same source line for one user-written call (e.g.,bedrock-raw-fetch/src/index.tswhere oneawait handleApi(...)resolves through 2 internal wrappers).Python detector (
src/scanner/python-waste-detector.ts)detectSequentialBatching()applies the same(providerKey, enclosingFunction)bucketing, with the same struct-field pattern. Thecluster.length >= 3proximity threshold is preserved (Python doesn't have explicit await sequencing as an N+1 indicator, so it warrants a stronger signal than TS's >= 2).Measurement (corpus v1, 7 fixtures)
Per-detector:
n_plus_onecachebatchrate_limitunbatched_parallelThe remaining batch FP is
bedrock-raw-fetch/src/index.ts:5— two sequentialawait handleApi(...)calls in the samemain()function (lines 5 and 11). Arguably a true positive (Promise.allwould parallelize them) that the corpus didn't label. Fixing it cleanly requires either control-flow-branch awareness or a corpus annotation update; out of scope for this PR.Sample size for batch (TP+FP = 1) is below the per-type gate's ≥3 threshold, so any future regression will need 3+ emissions before tripping the gate. This is an intentional gate design choice (PR-1) to avoid noise on small samples.
Tests
4 new TDD-style fixtures + tests in
src/test/c1-pr3-batch-tightening.test.tsandsrc/test/fixtures/c1-pr3/:ts_diff_functions.tsts_same_function.tspy_diff_functions.pypy_same_function.pyAll 357 tests pass.
Acceptance criteria for issue #83 (part 3)
detectSequential()emits ZERO findings onanswer.ts,bedrock-client.ts,raw-fetch-client.ts,summarize.ts,tts-service.tsdetectSequentialBatching()emits ZERO findings onanthropic_helper.pyandchat_completions_basic.pynpm testpasses (357/357)npm run benchmarkexits 0; per-type gate doesn't failfindingPrecisionrises (9.09% → 33.33%)benchmark/baseline.jsonupdated;batchcollapses to TP=0/FP=1/FN=1Out of scope (next steps)
routes/api.ts-style mutually-exclusive-branch pattern if branches are followed.rate_limitdetector — defer until corpus has rate-limit positive cases.Notes for reviewers
enclosingFunctionisstring | nullonAstCallMatch. Module-level calls (null) bucket to\"<module>\"rather than getting their own per-call buckets — desired behavior since two top-level awaits in a script DO live on the same execution path.ast-batch-detector.test.tssynthetic tests useenclosingFunction: null(module-level default) and don't assert on the exact line list, so they pass unchanged.Map<string, {provider: string; matches: AstCallMatch[]}>instead ofMap<string, AstCallMatch[]>+ key-split-on-"::". The structural pattern is what PR-2 review settled on for similar code; reused here.Test plan
npm test— 357 PASSnpm run benchmark— exit 0, no regressionsbedrock-raw-fetch/src/index.ts:5still emits🤖 Generated with Claude Code