Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds dependency-review and invisible-character scanning to CI, applies least-privilege ChangesSecurity Hardening
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
edelauna
changed the title
edelauna
requested review from
JamesRobert20,
hannesrudolph,
navedmerchant and
taltas
as code owners
![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: 1
🧹 Nitpick comments (1)
packages/config-eslint/base.js (1)
42-50: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueRule only covers a subset of invisible/homoglyph characters — comment slightly overstates coverage.
no-irregular-whitespace's character set covers\u200B(ZWSP) and\uFEFF(BOM), but not zero-width joiner/non-joiner (\u200C/\u200D), word joiner (\u2060), soft hyphen (\u00AD), or the bidi-override/Trojan Source characters (\u202A–\u202E,\u2066–\u2069). So even withskipStrings: true, this local rule won't catch those in identifiers/comments/code — only the CIinvisible-charsbyte-scan job (per the PR description) covers that full set. Worth tightening the comment so contributors don't over-rely on the editor/lint layer for Trojan Source protection.Suggested comment tweak
- // Reject invisible/irregular whitespace (zero-width chars, etc.) in - // identifiers and between tokens. String literals are skipped to - // avoid noise in i18n locale files; the CI invisible-chars job in - // code-qa.yml is the authoritative defense for the string case - // (it scans raw bytes across strings, identifiers, and comments). + // Reject irregular whitespace (incl. zero-width space \u200B and BOM) + // in identifiers and between tokens. This rule does NOT catch + // bidi-override, ZWJ/ZWNJ, or word-joiner characters; the CI + // invisible-chars job in code-qa.yml is the authoritative defense + // for the full Trojan Source character set across all files.🤖 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 `@packages/config-eslint/base.js` around lines 42 - 50, Tighten the explanatory comment above the no-irregular-whitespace rule in base.js so it does not claim broader invisible/homoglyph coverage than the rule actually provides. Keep the note around the rule itself, but make clear that no-irregular-whitespace only handles its limited whitespace set and that the CI invisible-chars job in code-qa.yml is what covers the wider Trojan Source/invisible-character cases; update the wording near skipStrings and skipComments accordingly.
🤖 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/code-qa.yml:
- Around line 28-55: Add a pre-merge safety check for the new invisible-chars
gate and harden Checkout by setting persist-credentials: false in the
invisible-chars job. The grep in the run step currently scans for ZWJ/RLM/LRM
and bidi characters across source and workflow files with no exceptions, so
verify the current main branch yields zero matches before enabling it, or the
job will fail on existing legitimate i18n/emoji content. Update the
actions/checkout step and, if needed, narrow the grep pattern or exclusions in
the invisible-chars job to avoid false positives while keeping the scan
effective.
---
Nitpick comments:
In `@packages/config-eslint/base.js`:
- Around line 42-50: Tighten the explanatory comment above the
no-irregular-whitespace rule in base.js so it does not claim broader
invisible/homoglyph coverage than the rule actually provides. Keep the note
around the rule itself, but make clear that no-irregular-whitespace only handles
its limited whitespace set and that the CI invisible-chars job in code-qa.yml is
what covers the wider Trojan Source/invisible-character cases; update the
wording near skipStrings and skipComments accordingly.
🪄 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: 0983a405-70f8-418d-baf7-8749004b8703
📒 Files selected for processing (8)
.github/workflows/cli-release.yml.github/workflows/code-qa.yml.github/workflows/codeql.yml.github/workflows/e2e.yml.github/workflows/marketplace-publish.yml.github/workflows/release-validation.yml.vscode/settings.jsonpackages/config-eslint/base.js
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
added
awaiting-review
navedmerchant
approved these changes
Jul 2, 2026
github-actions
Bot
added
awaiting-review
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
2 participants
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.
Related GitHub Issue
Closes: #782
Description
Implements the in-repo supply-chain hardening from #782. Complements #781 (publish provenance); this PR does not touch publish workflows.
Changes:
code-qa.yml) — newdependency-reviewjob, gated onpull_requestevents, validates the dependency diff of a PR against GitHub's advisory database before merge.actions/dependency-review-action@v5.0.0, pinned by SHA.no-irregular-whitespace(error,skipStrings: true) added to the sharedpackages/config-eslint/base.js, so every workspace inherits it. Local fast feedback for identifier/token splitting;skipStrings: truekeeps it quiet in i18n locale files.invisible-charsCI job incode-qa.yml— the authoritative defense. Scans raw bytes (so it catches chars in strings, identifiers, and comments alike) for zero-width (U+200B–200F), word joiner (U+2060), BOM (U+FEFF), bidi-override / "Trojan Source" (U+202A–202E), and soft hyphen (U+00AD). Covers*.ts/.tsx/.js/.mjs/.cjs/.cts/.mts/.shand the executablerun:blocks inside.githubworkflow/action YAML.permissions: contents: readadded as a top-level default to the 6 workflows that were missing it (code-qa,cli-release,codeql,e2e,marketplace-publish,release-validation). Jobs that need more (e.g.release,publish-stable, CodeQLanalyze) keep their existing job-level escalation..vscode/settings.jsoncleanup — enablededitor.unicodeHighlight.invisibleCharacters+ambiguousCharacters; replaced the deprecatedtypescript.tsc.autoDetectwithjs/ts.tsc.autoDetect; removed the unresolvedvitest.disableWorkspaceWarning(the Vitest extension isn't in workspace recommendations, so it produced "unknown setting" noise for most contributors).Already good (not changed here): all third-party actions are SHA-pinned,
--frozen-lockfileis universal, and pnpm'sonlyBuiltDependenciesallowlist already blocks arbitrary install scripts.Test Procedure
pnpm --filter ./src lint,pnpm --filter ./webview-ui lint,pnpm --filter @roo-code/core lint— all pass with--max-warnings=0(the new ESLint rule produces zero violations against existing code).grepcommand locally against the expanded scope (src webview-ui packages apps .github, all listed extensions) → zero matches (no false positives)..shand a.yml; the CI grep correctly detected both, then removed the planted files (working tree verified clean).Pre-Submission Checklist
Screenshots / Videos
N/A — CI/config only, no UI changes.
Documentation Updates
Additional Notes
A code review flagged that the original invisible-char scan missed executable release scripts (
*.sh/*.cjs) and workflow/action YAML; this is addressed — the scan now covers both.One review suggestion (re-adding
vitest.disableWorkspaceWarning) was intentionally not applied: the Vitest extension is not in workspace recommendations, so the setting is unresolved for most contributors and only adds "unknown setting" noise. If the workspace-config warning becomes painful for contributors using the Vitest GUI extension, the cleaner fix is to addvitest.explorerto.vscode/extensions.json.Get in Touch
Summary by CodeRabbit