D1: benchmark corpus + CI accuracy gate (closes #86) - #101
Conversation
Two unconditional console.log statements in scan-results.ts and local-waste-detector.ts printed to stdout on every scan, which corrupts the --format json output stream and makes JSON.parse fail. Other diagnostic logs in this codebase are correctly guarded by RECOST_DEBUG_SCAN; these two were leftover debug traces. Deleting them rather than gating because there's nothing useful here to debug.
…ripts Adds the orchestrator that spawns the live CLI per fixture, parses its JSON output, computes per-fixture metrics, aggregates, prints a console report, optionally writes JSON and GITHUB_STEP_SUMMARY markdown, and gates against benchmark/baseline.json (when present). Notable implementation details: - REPO_ROOT climbs two levels from __dirname because tsc emits the runner at <repo>/dist-test/benchmark/runner.js under rootDir: "." + outDir: "dist-test". - detectedFromScan() dedupes scan results by file:line. The AST scanner emits one callSite per source location inside an endpoint group, but all callSites of a group share the group's methodSignature — including phantom callSites at the import line and the constructor line. Without dedupe the runner double-counts those phantoms and torches precision. Honest measurement of the phantoms remains (they show up as false positives), but each line is counted only once. - methodsEquivalent() in metrics.ts is extended with a dot-suffix match so fixture authors who write "chat.completions.create" still match the AST scanner's full receiver chain "client.chat.completions.create". - detectedFromScan() resolves callSite paths against the scan target dir (fixtureDir/src when present) so the resulting relative paths line up with the "src/..." paths used in expected.json. Smoke run against benchmark/_smoke yields detection P/R 50%/100% and finding P/R 100%/100% — the 50% precision reflects the two phantom detections at the import + constructor lines, which is the honest signal.
📝 WalkthroughWalkthroughThis PR implements a complete benchmark infrastructure for measuring and gating scanner accuracy. It adds metric computation (precision/recall/provider attribution), a benchmark runner that executes the scanner against labeled fixtures, CI workflow integration, report formatting, and a minimal smoke fixture for testing. ChangesBenchmark Infrastructure and CI Precision/Recall Gate
Sequence DiagramssequenceDiagram
participant GitHub as GitHub Actions
participant Checkout as Code Checkout
participant Build as npm run build:ext
participant Runner as benchmark/runner.ts
participant Scanner as dist/cli/scan.js
participant Fixtures as ./benchmark-fixtures
GitHub->>Checkout: Clone repo
Checkout->>Build: Setup + build extension
Build->>Runner: Workflow runs benchmark
Runner->>Runner: Discover fixture dirs
loop For each fixture
Runner->>Scanner: execFile with JSON output
Scanner->>Fixtures: Scan fixture code
Fixtures-->>Scanner: API calls detected
Scanner-->>Runner: JSON results
Runner->>Runner: Parse + deduplicate detections
end
Runner->>Runner: Load expected.json + computeMetrics
Runner->>Runner: Aggregate + compare to baseline
Runner->>GitHub: Upload report artifact
alt Metric drop > threshold
Runner->>GitHub: exit 1 (fail workflow)
else No regression
Runner->>GitHub: exit 0 (pass)
end
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 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)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/accuracy/measurement.md (1)
83-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate acceptance checklist state to reflect implemented D1 deliverables.
The checklist is still unchecked even though this section now includes measured baseline and CI gating details. Keeping it stale can mislead readers about completion status.
Suggested doc update
-- [ ] At least 5 repos in the corpus, hand-labeled. -- [ ] `npm run benchmark` produces a metrics report. -- [ ] CI workflow runs on every PR. -- [ ] Baseline committed; current metrics published in this doc once measured. -- [ ] Regression gate prevents merging PRs that drop precision/recall. +- [x] At least 5 repos in the corpus, hand-labeled. +- [x] `npm run benchmark` produces a metrics report. +- [x] CI workflow runs on every PR. +- [x] Baseline committed; current metrics published in this doc once measured. +- [x] Regression gate prevents merging PRs that drop precision/recall.🤖 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 `@docs/accuracy/measurement.md` around lines 83 - 87, Update the checklist so each completed D1 deliverable item is marked checked: change the list entries "- [ ] At least 5 repos in the corpus, hand-labeled.", "- [ ] `npm run benchmark` produces a metrics report.", "- [ ] CI workflow runs on every PR.", "- [ ] Baseline committed; current metrics published in this doc once measured.", and "- [ ] Regression gate prevents merging PRs that drop precision/recall." to checked form (e.g., "- [x] ...") and add a brief note or timestamp near the checklist (or under the same section heading) noting the measured baseline and CI gating are implemented to avoid stale state.
🧹 Nitpick comments (2)
benchmark/schema.ts (1)
84-90: ⚡ Quick winConsider validating that required string fields are non-empty.
The validator checks that
fixtureSlugis non-empty (line 65-66), but does not enforce the same constraint onfile,provider,method, andtypefields. Empty strings would pass validation but likely cause confusing downstream failures during metric computation or reporting.✨ Proposed additional validation
function validateEndpoint(value: unknown, fixturePath: string, index: number): ExpectedEndpoint { if (!value || typeof value !== "object") { throw new ExpectedJsonValidationError(fixturePath, `endpoints[${index}] must be an object`); } const e = value as Record<string, unknown>; - if (typeof e.file !== "string") throw new ExpectedJsonValidationError(fixturePath, `endpoints[${index}].file must be a string`); + if (typeof e.file !== "string" || e.file.length === 0) throw new ExpectedJsonValidationError(fixturePath, `endpoints[${index}].file must be a non-empty string`); if (typeof e.line !== "number" || !Number.isInteger(e.line) || e.line < 1) { throw new ExpectedJsonValidationError(fixturePath, `endpoints[${index}].line must be a 1-based integer`); } - if (typeof e.provider !== "string") throw new ExpectedJsonValidationError(fixturePath, `endpoints[${index}].provider must be a string`); - if (typeof e.method !== "string") throw new ExpectedJsonValidationError(fixturePath, `endpoints[${index}].method must be a string`); + if (typeof e.provider !== "string" || e.provider.length === 0) throw new ExpectedJsonValidationError(fixturePath, `endpoints[${index}].provider must be a non-empty string`); + if (typeof e.method !== "string" || e.method.length === 0) throw new ExpectedJsonValidationError(fixturePath, `endpoints[${index}].method must be a non-empty string`); if (e.must_detect !== true) throw new ExpectedJsonValidationError(fixturePath, `endpoints[${index}].must_detect must be true`);function validateFinding(value: unknown, fixturePath: string, index: number): ExpectedFinding { if (!value || typeof value !== "object") { throw new ExpectedJsonValidationError(fixturePath, `findings[${index}] must be an object`); } const f = value as Record<string, unknown>; - if (typeof f.file !== "string") throw new ExpectedJsonValidationError(fixturePath, `findings[${index}].file must be a string`); + if (typeof f.file !== "string" || f.file.length === 0) throw new ExpectedJsonValidationError(fixturePath, `findings[${index}].file must be a non-empty string`); if (typeof f.line !== "number" || !Number.isInteger(f.line) || f.line < 1) { throw new ExpectedJsonValidationError(fixturePath, `findings[${index}].line must be a 1-based integer`); } - if (typeof f.type !== "string") throw new ExpectedJsonValidationError(fixturePath, `findings[${index}].type must be a string`); + if (typeof f.type !== "string" || f.type.length === 0) throw new ExpectedJsonValidationError(fixturePath, `findings[${index}].type must be a non-empty string`); if (f.is_true_positive !== true) throw new ExpectedJsonValidationError(fixturePath, `findings[${index}].is_true_positive must be true`);Also applies to: 107-112
🤖 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 `@benchmark/schema.ts` around lines 84 - 90, The validator currently checks types for endpoint fields but allows empty strings; update the validation in the endpoint-checking block (where e.file, e.provider, e.method, e.type are validated and ExpectedJsonValidationError is thrown) to also require non-empty strings (e.g., typeof e.file === "string" && e.file.trim().length > 0), and mirror the same non-empty checks in the second identical block (around the 107-112 area) so that file, provider, method and type cannot be empty; keep error messages descriptive (use the existing fixturePath and `endpoints[${index}]` context) when throwing ExpectedJsonValidationError..github/workflows/benchmark.yml (1)
1-16: ⚡ Quick winSet explicit minimal workflow token permissions.
This workflow doesn’t declare
permissions, so it inherits repo defaults. Add least-privilege permissions (e.g.,contents: read) to reduce token blast radius.Suggested patch
name: benchmark +permissions: + contents: read + on: push:🤖 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 @.github/workflows/benchmark.yml around lines 1 - 16, The workflow currently lacks an explicit permissions block; add a least-privilege permissions section at the top-level of the workflow (e.g., under the root keys alongside name/on/concurrency) to restrict the GITHUB_TOKEN scope—for example add permissions: contents: read (and any other minimal permissions you need) to the workflow named "benchmark" so the benchmark job uses a reduced-scope token instead of inheriting repo defaults.
🤖 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 @.github/workflows/benchmark.yml:
- Around line 33-42: The workflow reads an unvalidated SHA from
.benchmark-fixtures-sha and injects it directly into shell git commands in the
steps "Read fixtures SHA" and "Clone fixtures repo at pinned SHA", which allows
command injection; fix by validating the value before use (ensure it matches
/^[0-9a-fA-F]{40}$/) and then use a safe interpolation method (either export the
validated value into an env: variable and reference $PINNED_SHA in the shell, or
quote the parameter when using ${{ steps.sha.outputs.value }}), and fail the job
if validation fails so only a proper 40‑char hex SHA is ever passed to git
fetch/checkout.
In `@benchmark/runner.ts`:
- Around line 67-72: The --threshold parsing currently allows negative numbers
which breaks the gate logic in computeDrops; update the parsing code (the branch
handling "--threshold" that sets args.thresholdPp) to validate that the parsed
Number n is finite AND non-negative (n >= 0), and if not, print an invalid
--threshold message and exit with code 2; apply the same validation change to
the other threshold parsing block referenced around the 237-249 region so both
places enforce non-negative values.
- Around line 145-157: The dedupe key used in the loop (constructed as
`${relFile}:${cs.line}`) is too coarse and can collapse distinct endpoints on
the same line; update the key creation in the block that builds endpoints
(inside the for-loops over result.endpoints and e.callSites where seenEndpoints,
endpoints, relFile, cs, and e are used) to include a more specific discriminator
such as the call-site column (cs.column or cs.col if your AST uses that name)
and/or the endpoint identity (e.methodSignature or e.method) — e.g. compose the
key with `${relFile}:${cs.line}:${cs.column ?? cs.col ?? (e.methodSignature ??
e.method ?? "")}` so distinct calls on the same line are not deduplicated
incorrectly.
---
Outside diff comments:
In `@docs/accuracy/measurement.md`:
- Around line 83-87: Update the checklist so each completed D1 deliverable item
is marked checked: change the list entries "- [ ] At least 5 repos in the
corpus, hand-labeled.", "- [ ] `npm run benchmark` produces a metrics report.",
"- [ ] CI workflow runs on every PR.", "- [ ] Baseline committed; current
metrics published in this doc once measured.", and "- [ ] Regression gate
prevents merging PRs that drop precision/recall." to checked form (e.g., "- [x]
...") and add a brief note or timestamp near the checklist (or under the same
section heading) noting the measured baseline and CI gating are implemented to
avoid stale state.
---
Nitpick comments:
In @.github/workflows/benchmark.yml:
- Around line 1-16: The workflow currently lacks an explicit permissions block;
add a least-privilege permissions section at the top-level of the workflow
(e.g., under the root keys alongside name/on/concurrency) to restrict the
GITHUB_TOKEN scope—for example add permissions: contents: read (and any other
minimal permissions you need) to the workflow named "benchmark" so the benchmark
job uses a reduced-scope token instead of inheriting repo defaults.
In `@benchmark/schema.ts`:
- Around line 84-90: The validator currently checks types for endpoint fields
but allows empty strings; update the validation in the endpoint-checking block
(where e.file, e.provider, e.method, e.type are validated and
ExpectedJsonValidationError is thrown) to also require non-empty strings (e.g.,
typeof e.file === "string" && e.file.trim().length > 0), and mirror the same
non-empty checks in the second identical block (around the 107-112 area) so that
file, provider, method and type cannot be empty; keep error messages descriptive
(use the existing fixturePath and `endpoints[${index}]` context) when throwing
ExpectedJsonValidationError.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 39008be2-64c4-4040-adcc-c15af1cbce02
📒 Files selected for processing (20)
.benchmark-fixtures-sha.github/workflows/benchmark.ymlbenchmark/README.mdbenchmark/_smoke/FIXTURE.mdbenchmark/_smoke/expected.jsonbenchmark/_smoke/src/openai-helper.tsbenchmark/baseline.jsonbenchmark/metrics.tsbenchmark/report.tsbenchmark/runner.tsbenchmark/schema.tsdocs/accuracy/measurement.mdpackage.jsonsrc/cli/filesystem-adapter.tssrc/scan-results.tssrc/scanner/local-waste-detector.tssrc/test/benchmark-metrics.test.tssrc/test/benchmark-schema.test.tstsconfig.benchmark.jsontsconfig.scanner-tests.json
💤 Files with no reviewable changes (2)
- src/scan-results.ts
- src/scanner/local-waste-detector.ts
| - name: Read fixtures SHA | ||
| id: sha | ||
| run: echo "value=$(cat .benchmark-fixtures-sha)" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Clone fixtures repo at pinned SHA | ||
| run: | | ||
| git clone --depth 1 https://github.com/recost-dev/extension_benchmark.git benchmark-fixtures | ||
| cd benchmark-fixtures | ||
| git fetch --depth 1 origin ${{ steps.sha.outputs.value }} | ||
| git checkout ${{ steps.sha.outputs.value }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(benchmark\.yml|\.benchmark-fixtures-sha)" | head -20Repository: recost-dev/extension
Length of output: 120
🏁 Script executed:
fd -t f "benchmark.yml" -o "\.benchmark-fixtures-sha"Repository: recost-dev/extension
Length of output: 232
🏁 Script executed:
cat -n .github/workflows/benchmark.yml | sed -n '30,45p'Repository: recost-dev/extension
Length of output: 810
🏁 Script executed:
cat -n .github/workflows/benchmark.yml | head -20Repository: recost-dev/extension
Length of output: 561
🏁 Script executed:
cat .benchmark-fixtures-shaRepository: recost-dev/extension
Length of output: 105
🏁 Script executed:
git log --oneline -n 5 -- .benchmark-fixtures-shaRepository: recost-dev/extension
Length of output: 110
🏁 Script executed:
cat -n .github/workflows/benchmark.yml | sed -n '37,42p'Repository: recost-dev/extension
Length of output: 426
🌐 Web query:
GitHub Actions variables shell injection escaping security
💡 Result:
GitHub Actions “variables” (contexts/expressions) can become a shell-injection risk when they’re interpolated directly into a run: (shell) script, because the ${{ ... }} expressions are evaluated by GitHub Actions first and substituted into the generated temporary shell script; untrusted input can then break out of quoting and execute attacker-controlled commands. [1][2] What to watch for 1) Inline ${{ ... }} inside run: (or script:) blocks Example pattern: run: some-command ${{ github.event.issue.title }}. The GitHub Docs explicitly show that a vulnerable workflow can be exploited when ${{ ... }} output is substituted into an inline shell script before execution. [1] 2) Untrusted sources GitHub advises treating attacker-influenced values such as issue/PR titles and bodies as untrusted input, and notes that expression evaluation can lead to script injections when used inside blocks like run. [2][3] Is escaping/quoting enough? - Naive escaping is error-prone because ${{ ... }} is replaced before the shell runs; the substituted value may still contain characters that break the script’s structure (e.g., quotes). GitHub recommends not using ${{ ... }} in run for this reason, and instead using safer patterns. [2][1] Defensive patterns that are recommended (with grounding) 1) Don’t use ${{ ... }} directly in run:; use environment variables instead GitHub’s blog recommends that ${{ }} in the run section is almost always dangerous because it macro-expands to arbitrary text in the shell command body. It recommends using ${{ }} in env: instead, then referencing the variable from the shell (example: "$TITLE" bash syntax). [2] 2) Prefer the “intermediate environment variable” approach (inline scripts) GitHub’s Secure use reference (security hardening) states that for inline scripts, the preferred approach is to set the expression result into an intermediate environment variable, so it “doesn’t interact with the script generation process,” and also advises double-quoting shell variables to avoid word-splitting. [3] 3) Consider using an action instead of inline shell script for parsing/processing GitHub states that using an action (e.g., JavaScript) that receives the context value as an argument is not vulnerable to the injection attack described, because the value is passed as an argument rather than used to generate a shell script. [3] 4) If you must serialize to JSON (to preserve structure), be careful with quoting boundaries Community guidance shows that placing ${{ toJSON(...) }} inside shell quoting can break parsing if the JSON contains characters that terminate or conflict with the surrounding quotes (e.g., single quotes or backticks/quotes depending on the quoting style). One workaround is to pass the serialized output via an environment variable, and then read/echo it from the shell using shell-safe quoting. [4] Practical “safe” template - Use env: to bring the untrusted value in, then reference it in run: as a normal shell variable (and quote it): - Set env: TITLE: ${{ github.event.pull_request.title }} - In run:, use echo "$TITLE" (double quotes) [2][3] - If you want, you can also validate/sanitize the value before using it as part of a command or argument. (GitHub’s hardening guidance emphasizes that you should ensure untrusted values do not flow into places where they could be interpreted as executable code.) [1][3] Bottom line - GitHub Actions “expression variables” are not automatically shell-escaped when interpolated into run:; they’re substituted into the shell script before execution, so untrusted content can lead to command/script injection. [1][2] - The most reliable mitigation is to move ${{ ... }} out of run: into env: (or into an action argument) and then handle the value as a quoted shell variable (double-quoted) rather than raw substituted text. [2][3]
Citations:
- 1: https://docs.github.com/en/actions/concepts/security/script-injections
- 2: https://github.blog/2023-08-09-four-tips-to-keep-your-github-actions-workflows-secure
- 3: https://docs.github.com/actions/learn-github-actions/security-hardening-for-github-actions
- 4: Cannot use ${{ toJson(github) }} when a ' appears in the commit message. actions/runner#1656
Validate and quote the pinned SHA before using it in shell commands.
At line 35, the SHA is read from .benchmark-fixtures-sha and interpolated into shell commands at lines 41–42 without validation or quoting. Since this workflow runs on pull requests, a malicious PR can modify the file to inject shell commands. GitHub Actions ${{ ... }} expressions are macro-expanded before shell execution, so untrusted input can break out and execute arbitrary code.
Restrict the SHA to the expected 40-character hex format and either quote the interpolation or move it to an env: block (GitHub's recommended pattern). The suggested patch below adds validation and quotes; consider alternatively using env: for stronger isolation.
Suggested hardening patch
- name: Read fixtures SHA
id: sha
- run: echo "value=$(cat .benchmark-fixtures-sha)" >> "$GITHUB_OUTPUT"
+ run: |
+ SHA="$(tr -d '\n\r' < .benchmark-fixtures-sha)"
+ if ! [[ "$SHA" =~ ^[0-9a-f]{40}$ ]]; then
+ echo "Invalid fixtures SHA: '$SHA'" >&2
+ exit 1
+ fi
+ echo "value=$SHA" >> "$GITHUB_OUTPUT"
@@
- name: Clone fixtures repo at pinned SHA
run: |
git clone --depth 1 https://github.com/recost-dev/extension_benchmark.git benchmark-fixtures
cd benchmark-fixtures
- git fetch --depth 1 origin ${{ steps.sha.outputs.value }}
- git checkout ${{ steps.sha.outputs.value }}
+ git fetch --depth 1 origin "${{ steps.sha.outputs.value }}"
+ git checkout "${{ steps.sha.outputs.value }}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Read fixtures SHA | |
| id: sha | |
| run: echo "value=$(cat .benchmark-fixtures-sha)" >> "$GITHUB_OUTPUT" | |
| - name: Clone fixtures repo at pinned SHA | |
| run: | | |
| git clone --depth 1 https://github.com/recost-dev/extension_benchmark.git benchmark-fixtures | |
| cd benchmark-fixtures | |
| git fetch --depth 1 origin ${{ steps.sha.outputs.value }} | |
| git checkout ${{ steps.sha.outputs.value }} | |
| - name: Read fixtures SHA | |
| id: sha | |
| run: | | |
| SHA="$(tr -d '\n\r' < .benchmark-fixtures-sha)" | |
| if ! [[ "$SHA" =~ ^[0-9a-f]{40}$ ]]; then | |
| echo "Invalid fixtures SHA: '$SHA'" >&2 | |
| exit 1 | |
| fi | |
| echo "value=$SHA" >> "$GITHUB_OUTPUT" | |
| - name: Clone fixtures repo at pinned SHA | |
| run: | | |
| git clone --depth 1 https://github.com/recost-dev/extension_benchmark.git benchmark-fixtures | |
| cd benchmark-fixtures | |
| git fetch --depth 1 origin "${{ steps.sha.outputs.value }}" | |
| git checkout "${{ steps.sha.outputs.value }}" |
🤖 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 @.github/workflows/benchmark.yml around lines 33 - 42, The workflow reads an
unvalidated SHA from .benchmark-fixtures-sha and injects it directly into shell
git commands in the steps "Read fixtures SHA" and "Clone fixtures repo at pinned
SHA", which allows command injection; fix by validating the value before use
(ensure it matches /^[0-9a-fA-F]{40}$/) and then use a safe interpolation method
(either export the validated value into an env: variable and reference
$PINNED_SHA in the shell, or quote the parameter when using ${{
steps.sha.outputs.value }}), and fail the job if validation fails so only a
proper 40‑char hex SHA is ever passed to git fetch/checkout.
| else if (a === "--threshold") { | ||
| const raw = requireValue(argv, ++i, a); | ||
| const n = Number(raw); | ||
| if (!Number.isFinite(n)) { console.error(`Invalid --threshold value: ${raw}`); process.exit(2); } | ||
| args.thresholdPp = n; | ||
| } |
There was a problem hiding this comment.
Validate --threshold as non-negative to keep gate semantics correct.
--threshold currently accepts negative numbers. With a negative value, the drop check in computeDrops becomes invalid and can fail/pass the gate incorrectly.
Suggested fix
else if (a === "--threshold") {
const raw = requireValue(argv, ++i, a);
const n = Number(raw);
- if (!Number.isFinite(n)) { console.error(`Invalid --threshold value: ${raw}`); process.exit(2); }
+ if (!Number.isFinite(n) || n < 0) {
+ console.error(`Invalid --threshold value: ${raw} (must be a non-negative number)`);
+ process.exit(2);
+ }
args.thresholdPp = n;
}Also applies to: 237-249
🤖 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 `@benchmark/runner.ts` around lines 67 - 72, The --threshold parsing currently
allows negative numbers which breaks the gate logic in computeDrops; update the
parsing code (the branch handling "--threshold" that sets args.thresholdPp) to
validate that the parsed Number n is finite AND non-negative (n >= 0), and if
not, print an invalid --threshold message and exit with code 2; apply the same
validation change to the other threshold parsing block referenced around the
237-249 region so both places enforce non-negative values.
| for (const e of result.endpoints) { | ||
| for (const cs of e.callSites) { | ||
| const relFile = path.relative(fixtureDir, path.resolve(scanRoot, cs.file)).replace(/\\/g, "/"); | ||
| const key = `${relFile}:${cs.line}`; | ||
| if (seenEndpoints.has(key)) continue; | ||
| seenEndpoints.add(key); | ||
| endpoints.push({ | ||
| file: relFile, | ||
| line: cs.line, | ||
| provider: e.provider ?? "unknown", | ||
| method: e.methodSignature ?? e.method ?? "", | ||
| }); | ||
| } |
There was a problem hiding this comment.
Endpoint dedupe key is too broad and can drop valid detections on same line.
Deduping by file:line alone can merge different endpoint calls that share a line, which undercounts detections and distorts benchmark metrics.
Suggested fix
- const key = `${relFile}:${cs.line}`;
+ const method = e.methodSignature ?? e.method ?? "";
+ const provider = e.provider ?? "unknown";
+ const key = `${relFile}:${cs.line}:${provider}:${method}`;
if (seenEndpoints.has(key)) continue;
seenEndpoints.add(key);
endpoints.push({
file: relFile,
line: cs.line,
- provider: e.provider ?? "unknown",
- method: e.methodSignature ?? e.method ?? "",
+ provider,
+ method,
});🤖 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 `@benchmark/runner.ts` around lines 145 - 157, The dedupe key used in the loop
(constructed as `${relFile}:${cs.line}`) is too coarse and can collapse distinct
endpoints on the same line; update the key creation in the block that builds
endpoints (inside the for-loops over result.endpoints and e.callSites where
seenEndpoints, endpoints, relFile, cs, and e are used) to include a more
specific discriminator such as the call-site column (cs.column or cs.col if your
AST uses that name) and/or the endpoint identity (e.methodSignature or e.method)
— e.g. compose the key with `${relFile}:${cs.line}:${cs.column ?? cs.col ??
(e.methodSignature ?? e.method ?? "")}` so distinct calls on the same line are
not deduplicated incorrectly.
- workflow: validate fixtures SHA as 40-char hex and pass through env var instead
of inline ${{ }} interpolation — prevents shell injection from a PR that edits
.benchmark-fixtures-sha
- workflow: add explicit `permissions: contents: read` (least privilege)
- runner: reject negative --threshold (a negative value silently disabled the
gate via `< -(-n)` always-false comparison)
- runner: dedupe endpoints by file:line:provider:method instead of file:line, so
distinct calls on the same line (e.g. Promise.all([a.create(), b.create()]))
aren't merged
- schema: require non-empty strings for file/provider/method/type — empty values
passed type-only validation but produced silent zero-match downstream
- docs/accuracy/measurement.md: flip the D1 acceptance checklist to checked now
that the corpus, runner, workflow, baseline, and gate are all live
1 participant
Summary
Stands up a labeled benchmark corpus + CI precision/recall gate for the scanner.
benchmark/runner.ts,metrics.ts,schema.ts,report.ts, and an in-repo_smoke/fixture..benchmark-fixtures-shatorecost-dev/extension_benchmarkata8d77b3(5 hand-labeled fixtures: langchain-openai, openai-cookbook, stripe-sample, bedrock-raw-fetch, flask-mixed-providers)..github/workflows/benchmark.yml— clones the fixtures repo at the pinned SHA and runs the gate on every PR and main push.benchmark/baseline.json. Initial baseline table populated indocs/accuracy/measurement.md.642539dand77d658f.Test plan
npm run benchmark -- --fixtures ../extension_benchmarkruns all 5 fixtures and reports per-fixture + aggregate metrics. Exit 0 against the committed baseline.src/scanner/endpoint-classification.ts(mis-classifying openai.com) trips the gate with exit 1 and a clear FAIL message.benchmark-metrics.test.tspass (8 spec-mandated + 2 covering themethodsEquivalentdot-suffix branch).benchmarkworkflow runs to completion green on this PR (will verify after push).Closes #86.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests