Repository navigation
Fix all open CodeQL alerts, clear report-ui typecheck debt, refresh README screenshots - #96
Merged
Merged
Conversation
…creenshots CodeQL alerts (all 8 pre-existing open alerts addressed): - js/file-system-race x3: readInstallScriptFile and readInspectablePackageFile now stat and read through a single FileHandle, and build-report.ts reads built assets directly with try/catch instead of exists-then-read. - js/log-injection (build-spdx.ts): error messages are logged with line breaks stripped, matching the sanitizer form the query recognizes. - js/shell-command-injection-from-environment + js/indirect-command-line-injection (cli.ts --open on Windows): replaced `cmd /c start` with `rundll32 url.dll,FileProtocolHandler` so the report path is never parsed by a shell. - js/xss-through-dom (report-ui): the results summary is built with DOM nodes instead of innerHTML, removing the sink. - js/http-to-file-access (build-spdx.ts): vendored SPDX ids are now validated against the strict SPDX charset before writing; the alert itself is dismissed as won't-fix since writing pinned upstream data to the generated file is the script's purpose. Maintenance fix found along the way: abbreviated packuments over 4 MiB (typescript's is ~9 MiB) failed the registry lookup and marked the whole maintenance collector partial for any project using TypeScript. Packument fetches now allow 16 MiB. Refactoring/quality: - report-ui now typechecks cleanly (css module declaration, null narrowing in graphView, structural param for buildWorkspaceFilterOptions) and a new `npm run typecheck` script covering both tsconfigs runs in CI. Docs: README screenshots regenerated from a current report - the old ones showed the retired SaaS upsell banner; the new ones show the maintenance column with drift tiers, the header stat chips, and replacement counts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds CI typechecking for the CLI and report UI, updates report UI behavior and generated assets, and hardens artifact reads, SPDX validation, filesystem inspection, network payload limits, and Windows report launching. ChangesReport runtime and hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@scripts/build-report.ts`:
- Around line 50-70: Update the CSS and JS file handling around cssPath and
jsPath so each catch only treats an ENOENT filesystem error as the missing-asset
case. Rethrow all other errors, including permission, directory, disk, and write
failures, while preserving the existing warnings for genuinely missing
report.css or report.iife.js files.
In `@src/aggregator.ts`:
- Around line 2166-2176: Update the file-reading helper around handle, fs.open,
and handle.readFile to enforce the byte limit during reading rather than relying
only on stat.size: read at most maxBytes + 1 bytes (or use an equivalent hard
cap), reject the result when it exceeds the limit, and preserve the existing
undefined behavior for non-files and read errors. Apply this enforcement
consistently at both call sites using INSTALL_SCRIPT_MAX_BYTES and maxBytes.
In `@src/cli.ts`:
- Around line 1434-1438: Update the rundll32 invocation in the report-opening
flow to pass pathToFileURL(filePath).href instead of the raw normalizedPath,
ensuring Windows handles spaces, special characters, and UNC paths as a
canonical file URL. Add a Windows regression test covering a path with spaces
and special characters.
🪄 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
Run ID: 7a7b3a40-f323-4f7a-b956-40d65e2ad014
⛔ Files ignored due to path filters (8)
dist/aggregator.jsis excluded by!**/dist/**dist/cli.jsis excluded by!**/dist/**dist/report-assets.jsis excluded by!**/dist/**dist/runners/maintenanceSignals.jsis excluded by!**/dist/**docs/screenshot-01.jpgis excluded by!**/*.jpgdocs/screenshot-02.jpgis excluded by!**/*.jpgdocs/screenshot-03.jpgis excluded by!**/*.jpgreport-ui/dist/report.iife.jsis excluded by!**/dist/**
📒 Files selected for processing (12)
.github/workflows/ci.ymlpackage.jsonreport-ui/env.d.tsreport-ui/graphView.tsreport-ui/main.tsscripts/build-report.tsscripts/build-spdx.tssrc/aggregator.tssrc/cli.tssrc/report-assets.tssrc/runners/maintenanceSignals.tssrc/workspaceFilter.ts
…e URL for rundll32 - build-report.ts rethrows any read/write error that is not ENOENT, so permission or disk failures fail the build instead of silently emitting empty assets. - readHandleCapped enforces the per-file byte limit during the read (maxBytes + 1 hard cap), so a file growing after fstat cannot exceed it; both aggregator call sites use it. - The Windows --open flow passes pathToFileURL(...).href to rundll32's FileProtocolHandler so spaces, special characters, and UNC paths survive its argument handling. No unit test added: the conversion is Node's own pathToFileURL and openInBrowser's only behavior is spawning a real process. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/build-report.ts (1)
69-75: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not treat write failures as missing JavaScript.
This catch covers both
readFileSyncandwriteFileSync. If the read succeeds but the output directory disappears before the write,writeFileSynccan throwENOENT; the code then silently emits a report with empty JavaScript. Handle the read and write in separate blocks so only read-timeENOENTproduces the missing-artifact warning.🛠️ Proposed fix
let jsContent = ''; +let jsReadSucceeded = false; try { jsContent = fs.readFileSync(jsPath, 'utf8'); - jsContent = sanitizeMinifiedJs(jsContent); - fs.writeFileSync(jsPath, jsContent, 'utf8'); - console.log(`✓ Read JS: ${jsPath} (${jsContent.length} bytes)`); + jsReadSucceeded = true; } catch (err) { if (!isMissingFile(err)) throw err; console.warn('Warning: report.iife.js not found, JS will be empty'); } + +if (jsReadSucceeded) { + jsContent = sanitizeMinifiedJs(jsContent); + fs.writeFileSync(jsPath, jsContent, 'utf8'); + console.log(`✓ Read JS: ${jsPath} (${jsContent.length} bytes)`); +}🤖 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 `@scripts/build-report.ts` around lines 69 - 75, Separate the read and write operations in the JavaScript processing flow so the catch around fs.readFileSync only suppresses read-time ENOENT via isMissingFile(err), while failures from fs.writeFileSync propagate normally. Preserve the existing missing-artifact warning behavior for absent input files and ensure successful reads are still sanitized, written, and reported.
🤖 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.
Outside diff comments:
In `@scripts/build-report.ts`:
- Around line 69-75: Separate the read and write operations in the JavaScript
processing flow so the catch around fs.readFileSync only suppresses read-time
ENOENT via isMissingFile(err), while failures from fs.writeFileSync propagate
normally. Preserve the existing missing-artifact warning behavior for absent
input files and ensure successful reads are still sanitized, written, and
reported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d344d9fd-eaaf-49fa-ab4a-2593831f7f0b
⛔ Files ignored due to path filters (2)
dist/aggregator.jsis excluded by!**/dist/**dist/cli.jsis excluded by!**/dist/**
📒 Files selected for processing (3)
scripts/build-report.tssrc/aggregator.tssrc/cli.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cli.ts
- src/aggregator.ts
A write-time failure inside the same try was misreported as a missing input artifact; sanitize/write errors now propagate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes out all 8 pre-existing open CodeQL alerts on master, plus batched quality work while we're spending a CodeRabbit review anyway.
CodeQL alerts
js/file-system-race(high)readInstallScriptFile/readInspectablePackageFilestat and read through oneFileHandle, so the checked file and the read file cannot differjs/file-system-race(high)build-report.tsreads built assets with try/catch instead of exists-then-readjs/log-injection(error-level)--openon Windows now usesrundll32 url.dll,FileProtocolHandlerinstead ofcmd /c start, so the report path is never shell-parsedjs/xss-through-dom(high)innerHTMLsinkjs/http-to-file-access(medium)Genuine bug found along the way
Abbreviated packuments larger than 4 MiB failed the maintenance lookup — and
typescript's is ~9 MiB, so any project depending on TypeScript got a "maintenance collection is partial" banner and an unknown maintenance chip. Packument fetches now allow 16 MiB.Refactoring
buildWorkspaceFilterOptions).npm run typecheckcovers both tsconfigs and runs as a CI step, so the debt can't come back.Docs
README screenshots regenerated from a current report — the old ones still showed the retired SaaS "Upgrade this scan" banner. The new ones show the header stat chips (including the e18e Replacements count), the Maintenance column with the new drift tiers, and the current graph and detail views.
Testing
🤖 Generated with Claude Code
Summary by CodeRabbit
file://URL.typecheckscript).