CodeIssuesPull RequestsActionsSecurityInsights
✨ AI
More
Settings

fix(health,mcp): a security score from 3% of the repo, and the last of the fake totals #5611

MergedXSccantynz wants to mergefix/security-coverage-and-totalsmainopened 1h ago
ccantynzcommented 1h ago

Third wave from the cross-platform session. Both headline items are the same defect in different clothes: a number computed from partial input, presented with the confidence of a complete one.

Security scored 88/100 having read 3.2% of the source

The scanner ran one git show per file, so covering a 3,090-file repo meant 3,090 subprocesses. It was capped at 100 files — and then scored the result as a measurement of the repository.

The coverage block did disclose it (sourceFilesScanned: 100, truncated: true), which is better than silence and still not enough. As the GateTest session put it: a score computed from 3% of a repo is not a truncated measurement, it's a different measurement wearing the same name — and nobody discounts a number that says 88.

Fixed by making full coverage affordable rather than by better-labelling the absence. One git grep -n returns every candidate line in the whole tree in a single subprocess.

before after
files covered (this repo) 100 of 1,144 all 1,144
subprocesses 100 1 grep + 19 reads
wall time ~1s

Only files containing an innerHTML assignment get read in full, because those are judged as whole statements — 19 files here.

The token list is deliberately a superset of every rule, so a line it doesn't return cannot contain a finding. Leading letters are dropped where that buys case-insensitivity (assword matches both spellings; -i isn't on the git argument allowlist). Broad costs output; narrow costs a missed vulnerability.

When git grep can't answer it returns null, the bounded scan runs, and the response says method: "capped" with truncated: true. "No candidates" must never be arrived at by way of "the tool failed" — and those two cases were previously indistinguishable from the numbers alone.

The last of the fake totals

The previous wave fixed four listing tools and missed both repo searches. GateTest measured it:

search_repos q="a" limit=5   -> total: 5
search_repos q="a" limit=20  -> total: 20
actual matches               -> 36

Worse than the list_prs case it matches: total: 50 is a round number that invites suspicion, while total: 5 when you asked for 5 reads as a complete and precise answer. Nothing signals the 31 repos the caller can't see. Both now return the true count and page properly. No total: rows.length remains in either tool file.

Ancestor trees counted as contents

list_tree with a path + recursive: true included the ancestor trees leading to that path — totalCount: 10 for 8 actual entries (Vapron's #5). Verified against real git output: scoping to src/lib/selfcheck emits src, src/lib, src/lib/selfcheck ahead of 52 real entries. Exactly those three are dropped now, from the count as well as the list.

Degraded AI output was indistinguishable from real output

generate_commit_message fell back to a heuristic on three paths — no API key, empty model response, any network/API failure — returning a result shaped identically to a real generation. A caller couldn't tell chore(src): update 1 file from a model's considered summary. Now carries source: "ai" | "heuristic" and a degradedReason, and the failure path logs rather than swallowing.

A test heuristic that punished its own documentation

git-arg-injection collects every dash-prefixed literal in git/repository.ts and requires each to be an allowed git flag. It caught "--- " (a diff header matched with startsWith) and then `-i` inside a comment explaining why that flag is not on the allowlist — so documenting the guard failed the guard's own test.

Narrowed by context both times, not by exceptions: literals consumed by a string predicate, and literals on a comment line, are not arguments. Anything that could reach git as an argument still must be allowlisted. Detected by line position rather than by stripping comments, which would have to parse strings containing // correctly to avoid dropping a real flag.

Verification

  • 48 unit tests across the paging contract and the XSS predicate.
  • git grep coverage measured locally (13,034 lines / 1,144 files / 19 full reads).
  • Ancestor filter verified against real git ls-tree output.
  • Full CI-safe suite: 5,321 pass, 11 fail — identical to the pre-change baseline (local no-DB artifacts).
ccantynzAI Reviewcommented 1h ago

AI Triage

(no summary)

Priority: medium Risk area: mixed

Suggested labels: (no label suggestions) Suggested reviewers: (no reviewer suggestions)

Suggestions only — nothing has been applied. The PR author stays in control.

gluecron[bot]🤖 botAI Reviewcommented 1h ago

AI review unavailable

The platform's AI balance is exhausted, so AI generation is temporarily unavailable. Nothing was lost. You can queue this as a repair for the internal agent from the repository's Health page, or try again once the balance is restored. The PR is otherwise unchanged.

Cross-repo impact

See what breaks downstream if this PR merges.

Analyze →
⮌ Merged

This pull request was merged into main.

c comment · e edit title · m merge · a approve · r request changes · ? shortcuts