Repository navigation
fix(analytics): the call scanner read JSDoc examples as live calls — 3 of 4 'Potential Bugs' were documentation (#17668) - #17670
Conversation
…17668) Three of the four findings on the live "Missing Endpoints — Potential Bugs" panel were documentation. Verified line by line before changing anything: useFileDownload.ts:11 inside the /** block opening at 6, closing at 17 FALSE useFileDownload.ts:16 same block, "Or inline:" example FALSE apiErrorHandler.ts:222 inside the ```typescript fence opening at 212 FALSE ThemeManagerView.vue:35 real call, and no backend route matches TRUE The scanner decided "is this a comment?" per line, by prefix: stripped = line.strip() if stripped.startswith("//") or stripped.startswith("/*"): That knows two spellings and the class has at least three. A JSDoc example lives on a `*` continuation line, which starts with neither -- all three false positives. A `//` trailing real code is scanned too, because the line does not *start* with it. And any line inside a `/* */` block that has no leading star. "Is this position inside a comment" is not a property of one line, so a per-line prefix test cannot answer it. `blank_comments` carries block state across lines and replaces comment bodies with **spaces**, so every `file:line` the scanner reports still points at the same place. It lives in its own module because the scanner is at its frozen size ceiling; the scanner shrinks 1445 -> 1444 and both baselines ratchet down. String awareness is the part that is not optional: `'https://example.test'` contains `//`, and a stripper without it blanks the rest of that line -- silencing a real call and reporting nothing, which is the failure direction that looks like success. Vue `<!-- -->` is handled too, since a commented-out template call is the same defect in a different syntax. Verified against the three real files rather than synthetic input: useFileDownload.ts /api/ visible before [11, 16] -> after [] apiErrorHandler.ts /api/ visible before [222] -> after [] ThemeManagerView.vue /api/ visible before [35, 49] -> after [35, 49] The false positives go, the true positive stays. `POST /api/themes` is a theme upload that can only 404 and was previously indistinguishable from noise. Tests are organised by failure *direction*, because only one direction is visible: a false positive shows up as a wrong finding, a false negative as a clean panel. A non-vacuity test fails on an identity implementation, since every "this call survives" assertion passes when nothing is blanked at all. Mutation-proved, each verified to have applied before its result was read: identity fails 6, deleting rather than blanking fails 5, dropping string awareness fails 1, not entering block state fails 4. Two of those first read as "not caught" because a shell heredoc mangled the replacement and the edit never landed -- a mutation that did not apply is not evidence that a test is weak, and the harness now asserts the file changed before running anything. `blank_comments` is split into small state consumers rather than one loop: the function-length gate caught it at 67 body lines, and the split is better anyway -- each consumer answers one question and the driver is a five-branch dispatch. One limit stated in the module rather than left to be rediscovered: a JS regex literal containing a slash pair is not distinguished from a line comment. The scanner's input is API-path string literals where that shape does not arise, and the fix would be a tokenizer rather than another special case. Closes #17668
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 33 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe frontend API scanner now blanks comments before scanning, while preserving source positions and quoted strings. Tests cover comment handling and retained API calls. The module registry test records a new measurement and raises its REACH floor. ChangesFrontend API scanning
Module registry floor
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to An unusual template comment can still produce a false API-call finding. The remaining issue is narrow and can be fixed with brace-aware comment handling. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Comment filtering should remove documentation-only findings, but a valid source-code pattern can also hide real calls from endpoint reports. The effect is limited to analysis results; no change to authorization or code execution is established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The changes to ✨ Finishing Touches 💡 1📝 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 |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
mrveiss
left a comment
mrveiss
left a comment
There was a problem hiding this comment.
Non-author review of 64103ecaa54f, on scope: does the fix cover the class or the instance?
It covers the class, and it covers the inverse hazard too
comment_blanking.py is not a JSDoc special case. It handles the three comment forms a Vue/TS tree actually contains — // (_LINE), /* … */ (_BLOCK_OPEN/_CLOSE, of which /** */ is a case) and <!-- … --> (_HTML_OPEN/_CLOSE) — and the tests exercise each: a // trailing real code (:59), a Vue template comment (:64), a block interior with no leading star (:55), a JSDoc continuation line (:43), and a multi-line block whose close restores scanning for the code after it (:68).
So the answer to "does it strip the specific form it found?" is no — a //-prefixed example scans as a comment as well, which is the case that would have made this the class repeating inside its own repair.
And it handles the hazard in the other direction, which is the part I would look at first in a comment stripper, because over-blanking corrupts scanning rather than merely under-blanking it:
| test | what it stops |
|---|---|
:79 a URL containing // does not start a comment |
"https://x" being eaten |
:92 a block marker inside a string is not a comment |
"/*" in a literal |
:96 an escaped quote does not end the string |
"a\"b" desynchronising the scan |
:100 a template literal is passed through |
backtick content |
:88 a plain call is untouched |
the base case |
_consume_string honouring backslash escapes is what makes those hold rather than a regex that would get them wrong.
Offset integrity is pinned, and it is the property this fix needed most. :121 length and line count unchanged (parametrised), :128 the line numbers of surviving calls unchanged. Blanking rather than deleting is the right choice for a scanner that reports line numbers, and it is asserted rather than assumed.
And :139 test_the_blanker_actually_blanks_something is the vacuity guard — without it a no-op blanker passes every test above. Present without anyone asking for it.
The fourth finding: proxied, not verified — and I cannot settle it by reading
Three of four findings were documentation; the fourth was presumably real, and "a fix that silences all four is worse than the defect" is the right test. What the PR gives is the structural proxy: code after a closed block is still scanned (:68) and a plain call is untouched (:88).
That is not the same as the fourth finding surviving. Settling it means running the scanner over the same tree before and after and diffing the finding set — a run, not a read, and I am not going to assert a result I did not produce. Whoever merges should ask for that diff, or for the one finding named so it can be looked for by hand. If it turns out the real finding was itself inside a comment, the defect was never a precision problem at all.
Precision on the panel: unchanged, and I could not locate where the label lives
The PR touches the scanner, the new module, its tests, and two ratchet entries. No consumer of the scanner's output is modified, so nothing about how findings are presented changes — this fixes the findings, not the framing.
I went looking for the label to say whether it is qualified and could not find it: no Potential Bugs or potential_bugs in api/codebase_analytics/ or autobot-frontend/src. The scanner's own language is "orphaned" (api_endpoint_scanner.py:149). So I am not claiming the label is unqualified — I am saying I could not establish where it is set, and that is a question for whoever knows the panel. The argument stands either way: at 25% precision a finding label that invites action costs investigation time on the false ones and discredits the true one, and that half is untouched here.
Ratchet banked, measured between revisions
api_endpoint_scanner.py 1445 → 1444 in both scripts/python_file_size_known_large.py and repo_tests/python_file_size_ratchet_baseline.py. Read from git diff origin/main 64103ecaa54f -- <path> so both sides name their revision — I got this wrong on #17647 by reading one side from the working tree, so it is worth saying how it was measured rather than just that it was.
…oxy (#17668) Review condition on #17670: the PR showed three findings silenced and argued the fourth survives from structural proxies -- "code after a closed block is still scanned", "a plain call is untouched". Those show the mechanism is *capable* of keeping a call, not that it kept *this* call. The corollary is what makes it non-optional: **a fix that silenced all four would look identical to a correct one** if only the three were checked. So this runs `FrontendAPICallScanner._parse_api_call` over the same three files with comments intact and with them blanked, and asserts both directions: useFileDownload.ts before [(11, GET /api/export), (16, GET /api/export)] after [] apiErrorHandler.ts before [(222, GET /api/users)] after [] ThemeManagerView.vue before [(35, POST /api/themes), after [(35, POST /api/themes), (49, DELETE /api/themes/${id})] (49, DELETE /api/themes/${id})] The real finding is named rather than implied: **`POST /api/themes` at `ThemeManagerView.vue:35`**, a theme upload with no matching backend route. A second real call at `:49` (`DELETE /api/themes/${id}`) survives too and was not in the panel's four -- worth noting, since it shares the missing route. Three tests, one per direction plus one against vacuity: if the pre-fix scan found nothing, every "absent after" assertion would be trivially satisfied, so the comparison asserts at least four pre-fix findings exist. Mutation-proved by the mutation that matters here -- making the blanker blank *code* as well as comments, i.e. the fix that silences everything -- fails 2 of 5. That is the assertion the proxies could not make. The pre-fix side is reconstructed by scanning with comments left intact, which is what the prefix test did for every shape not starting with a marker. That keeps both sides comparable without depending on the old revision, and both sides name how they were produced. Refs #17668
…on (#17668) CI red after merging main into this PR: the floor described a tree two merges old. [one-claim-registry] floor 2976 sits 51 below its live population of 3027 Python modules swept for claim-or-lock primitives, which exceeds the declared allowance of 50 (skips=0 + growth=50). Re-pinned to 3,002 -- mid-window, which is the convention this declaration documents for itself and explains: pinning at `population - growth` leaves zero headroom, so the next module anyone adds reds the meta-test (#17142). 3,002 leaves 25 before the next bump is due. The allowance is untouched. Only **one** of those 51 modules came from this PR. The sweep skips `_test.py`, so of the three files added here -- `comment_blanking.py` and two test modules -- one is swept. The other 50 arrived with base. The measurement is worth recording because I got it wrong first. I measured `len(REACH.discover())` in my worktree and got **3025** against CI's **3027**, and the two-module gap was not a discrepancy to explain: my worktree was still at `495c4811f4` and had never pulled the `update-branch` merge. I was measuring a tree two merges older than the one CI tested, which is the same shape as reading one side of a diff from the working tree and the other from a revision. Fast-forwarded to `b2f41ad3c694` and re-measured: **3027**, matching CI exactly. So both numbers in this commit name their revision: 3,027 live at `b2f41ad3c694`, by `len(REACH.discover())`, not by grep. Mutation-proved the meta-test still does its job: setting the floor to 2,900 fails `test_every_declared_floor_is_pinned_to_its_population`. A re-pin that quietly broke the check it satisfies would look identical to a correct one. 289 tests pass across `reach_declarations_test.py` and the registry guard. Refs #17668
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: f34822d9-b347-4ab3-bfdd-8afddeff75e5
⛔ Files ignored due to path filters (2)
repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (5)
autobot-backend/api/codebase_analytics/api_endpoint_scanner.pyautobot-backend/api/codebase_analytics/comment_blanking.pyautobot-backend/api/codebase_analytics/comment_blanking_test.pyautobot-backend/api/codebase_analytics/frontend_call_scanner_comments_test.pyrepo_tests/one_claim_registry_16653_test.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
…as code (#17668) Two review findings, batched into one push. The one rated Trivial is the serious one. **The test reimplemented what it verified.** `_scan` applied `blank_comments` and the call patterns itself rather than calling `FrontendAPICallScanner._scan_file`, so it would have passed with the production blanking removed entirely. Proved rather than argued -- deleting the `blank_comments` call from `_scan_file`: before this commit: both suites pass <- the hole after this commit: scanner test 2 failed, blanker test 17 passed The blanker test correctly stays green there, because the blanker is fine; only the wiring broke. That split is the signal the old arrangement could not produce. This is the same defect as verifying with a private regex instead of the scanner's own patterns -- which this PR already fixed once, one level down. I replaced the regex and kept a parallel copy of the loop around it. A verification that reimplements its subject tests its own copy, and the copy cannot drift from itself. The two sides are now named for what they are: `_scan_production` calls `_scan_file`, and `_scan_prefix_era` is an explicit reconstruction of the pre-fix behaviour, kept separate because it can no longer be obtained from the production path. **A `${...}` interpolation is code, not template text.** `_consume_string` copies a backtick literal verbatim so that a URL's slash pair survives -- correct for the text, wrong for interpolations, which can contain comments. Interpolations now go back through `blank_comments` recursively, with brace matching so a nested `{}` does not end one early. Length is preserved, so positions still hold. comment inside ${} hidden -> blanked nested braces in ${} hidden -> blanked `/api/items/${id}` kept <- template text is still text Mutation-proved: disabling interpolation handling fails 2. 25 tests across the two files. All functions under the 30-line gate; the scanner stays at its re-pinned 1444. Refs #17668
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 53236ccb-e941-448e-8a37-a5ed789b3803
⛔ Files ignored due to path filters (2)
repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (4)
autobot-backend/api/codebase_analytics/api_endpoint_scanner.pyautobot-backend/api/codebase_analytics/comment_blanking.pyautobot-backend/api/codebase_analytics/comment_blanking_test.pyautobot-backend/api/codebase_analytics/frontend_call_scanner_comments_test.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
`_matching_brace` counted raw braces, so a `}` inside a comment or a string ended the
interpolation early and everything after it was copied out as template text. Verified
before fixing -- both shapes leaked a path the scanner would then report:
const x = `${/* } api.get('/api/hidden') */ ''}` leaked /api/hidden
const y = `${ m['}'] /* api.get('/api/gone') */ }` leaked /api/gone
The review named the comment case; the string case has the identical cause and would
have survived a fix aimed only at comments. A brace inside either is not a delimiter.
The counting view comes from `blank_comments` itself, with an internal flag that
blanks string bodies as well, rather than from a second brace scanner. That is the
point: a separate scanner could disagree with this one about where a string ends, and
then the two would drift -- which is the defect this module was written to fix,
appearing one level down inside the fix. Whatever the real pass treats as a comment
or a string is exactly what the counter refuses to count. Blanking preserves length,
so indices in the view are indices in the source.
Recursion is broken by not processing interpolations in the counting view: a brace
inside a nested template is not a delimiter of the interpolation being measured, so
the whole literal blanks and `_matching_brace` cannot re-enter itself.
Two regression tests, one per shape. Mutation-proved: reverting the counter to raw
characters fails both. 27 tests across the two files, function-length gate clean.
This is the fourth finding on this PR that was the same class as the PR's own subject
-- private regex, parallel scan loop, interpolation contents, and now brace counting
-- each one a level further in. Every one was found by review, none by a tool.
Refs #17668
Provenance disclosure: the file-size figures in this PR came from invoking
|
1 participant
Thinking Path
Four findings on the live "Missing Endpoints — Potential Bugs" panel. I checked each against the source before touching anything:
GET /api/export@useFileDownload.ts:11/** … */opening at 6, closing at 17 — aUsage:exampleGET /api/export@useFileDownload.ts:16Or inline:exampleGET /api/users@apiErrorHandler.ts:222```typescriptfence opening at 212POST /api/themes@ThemeManagerView.vue:35The panel was wrong in both directions
Running the real scanner to settle the fourth finding surfaced something the panel never reported:
DELETE /api/themes/${id}atThemeManagerView.vue:49— a second genuine call, on the same missing route, absent from the panel's four.So this surface was over-reporting documentation and under-reporting real calls at the same time. That is a stronger and more useful statement than "3 of 4 were false": precision and recall were both wrong, in opposite directions, on one panel. It appeared only because the verification ran the scanner rather than reasoning about it — and it means "make the findings precise" is not the whole remedy, because the same panel is also missing things.
3 of 4 false, on a panel whose label invites action. That is worse than an empty panel: an empty one gets investigated, a wrong one gets acted on — and the one real finding gets discounted along with the noise.
Mechanism
api_endpoint_scanner.pydecided "is this a comment?" per line, by prefix:Two spellings; the class has at least three:
* const r = await ApiClient.get('/api/export')starts with neither marker. All three false positives./* … */.doThing() // apiClient.get('/api/legacy')does not start with the marker."Is this position inside a comment" is not a property of one line, so a per-line prefix test cannot answer it however many prefixes it learns.
Why string awareness is the load-bearing part
An
httpsscheme contains a slash pair. A comment stripper without string handling blanks the rest of that line — silencing a genuine call and reporting nothing, which is the failure direction that looks like success. The tests are grouped by failure direction for exactly this reason: a false positive shows up as a wrong finding, a false negative shows up as a clean panel.What Changed
api/codebase_analytics/comment_blanking.py(new) —blank_comments()carries block state across lines and replaces comment bodies with spaces, so everyfile:linethe scanner reports still points at the same place. Handles//,/* … */, and<!-- … -->for Vue templates; string literals pass through verbatim.api/codebase_analytics/api_endpoint_scanner.py— blanks before scanning instead of skipping by prefix. 1445 → 1444 lines, both baselines ratcheted down. The helper is a separate module because the scanner sat at its frozen ceiling.comment_blanking_test.py(new) — 17 tests.blank_commentsis four small state consumers plus a five-branch driver, not one loop: the function-length gate caught it at 67 body lines, and the split reads better — each consumer answers one question.Verification
Settled with the scanner, not a proxy — the first version of this verification used my own regex, which is a different instrument than
_API_CALL_PATTERNS:Pinned in
frontend_call_scanner_comments_test.py, which names the true positive rather than implying it. Mutation-proved with the mutation that matters: making the blanker blank code as well as comments — the fix that silences everything — fails 2 of 5. When a fix removes findings, the test must name a finding it keeps, or "fixed the false positives" and "broke the detector" have the same signature.Against the three real files rather than synthetic input:
The false positives go;
POST /api/themessurvives. The fix must not silence the true positive, and that is asserted rather than assumed.17 tests pass. Mutation-proved, each verified to have applied before its result was read:
_blank_likereturns input)Two of those first read as "not caught" — a shell heredoc mangled the replacement and the edit never landed. A mutation that did not apply is not evidence that a test is weak, and it points the wrong way: it invites weakening the code to satisfy a check that never ran. The harness now asserts the anchor appears exactly once and that the file changed before reading any result.
A non-vacuity test fails on an identity implementation, because every "this call survives" assertion passes when nothing is blanked at all.
Known limit, in the module rather than left to be rediscovered: a JS regex literal containing a slash pair is not distinguished from a line comment. The scanner's input is API-path string literals where that shape does not arise; the fix would be a tokenizer, not another special case.
Model Used
Claude Opus 5 (1M context)
Closes #17668
🤖 Generated with Claude Code
Summary by CodeRabbit