Conversation
Extend the SonarJS maintainability, regex-correctness, and size/complexity guardrails that protect packages/*/src to the scripts/** tree and scripts/tests/**, which previously received only the base ESLint recommended layer. Build/dev scripts are now held to the same quality bar as application source. - eslint.config.js: add a scripts quality block with the curated rule set (non-type-aware parity with packages). no-console, type-aware TS rules, import/*, and React rules are intentionally excluded with documented rationale (scripts have no tsconfig and legitimately log to stdout/stderr). - Three centralized, tagged carve-outs (all eslint-policy-allow-off: #2282): scripts/tests max-lines-per-function (test parity), check-eslint-guard.js (inherent state-machine parser complexity), eslint-guard.test.js (exhaustive fixture max-lines). - Fix all ~117 newly-exposed violations at root cause across ~45 scripts: decompose oversized functions, extract helpers, reduce nesting, and rewrite catastrophic-backtracking regexes. No inline suppressions added. - scripts/start.js: replace backtracking regex with an equivalent manual scanner that preserves the original removal semantics (whitespace around equals, tab separators, value-not-starting-with-dash). - tmux-harness.js: split into helpers/io/steps modules (preserves public API) to get under the 800-line limit. - Add behavioral regression test (scripts-quality-coverage.test.ts) verifying scripts files receive the quality rules and the carve-outs are in place.
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
WalkthroughExtends strict ESLint quality rules to ChangesIssue
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
LLxprt PR Review – PR #2284Issue Alignment: Strong. The PR extends the strict SonarJS/regex/size ESLint layer to Side Effects: The new rules could fail CI for unmodified scripts, but this PR includes the fixes. Carve-outs for Code Quality: The guard decomposition is a significant improvement— Tests and Coverage: Increase. Verdict: Ready. The implementation fully resolves #2282 with meaningful automated tests, documented policy exceptions, and systematic code-quality improvements. The large surface area is inherent to applying lint rules across an entire tree; the changes are consistent and well-contained. |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/update-homebrew-formula.js (1)
220-252: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winOnly swallow the expected
git diff --cached --quietexit code.This catch currently treats every
git diff --cached --quietfailure as “there are staged changes”, but Git uses nonzero exits for real errors too. On a genuine git failure, the script will continue into commit/push and mask the root cause.Suggested fix
- } catch { + } catch (error) { + if (error?.status !== 1) { + throw error; + } runCommand(`git commit -m "Update llxprt-code to ${version}"`, { cwd: tapDir, stdio: 'ignore', }); console.log('Committed changes');🤖 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/update-homebrew-formula.js` around lines 220 - 252, The nested try/catch in commitAndPush currently treats any failure from runCommand('git diff --cached --quiet') as “there are staged changes,” which can hide real Git errors. Update the git diff check in commitAndPush to only ignore the expected nonzero exit code for no staged differences, and rethrow or surface any other failure before attempting git commit and git push. Use the existing runCommand calls and the commitAndPush function as the place to apply the fix.
🤖 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/check-lockfile.js`:
- Around line 58-63: The skip logic in check-lockfile is too broad because the
location check in the main filter also excludes ordinary dependencies nested
under workspace packages. Update the conditional around details.link and the
location checks so only linked workspace packages are skipped, and remove or
narrow the packages/*/node_modules/* exemption while keeping the existing
node_modules validation path for third-party packages such as
packages/foo/node_modules/bar.
In `@scripts/check-settings-boundary.js`:
- Around line 744-751: The export scan in the MOVED_SYMBOLS check stops at the
first match because it uses find(), so barrels that re-export multiple moved
symbols only report one violation. Update the logic around the MOVED_SYMBOLS
lookup to iterate over all symbols and collect every matching export line, using
collectExportLinesForSymbol for each match, then append all findings to
violations instead of selecting a single matchedSym.
In `@scripts/check-storage-import-boundary.mjs`:
- Around line 173-188: Restore ancestor-aware matching in
collectDynamicImportViolations() so dynamic imports are checked the same way as
scripts/preflight-import-inventory.mjs. Instead of only inspecting node.parent,
walk up through the enclosing ancestors (including the VariableDeclaration and
binding pattern around AwaitExpression) and collect identifiers from the full
containing structure before comparing against ctx.movedSymbols. Keep the
violation recording logic the same, but ensure destructured forms like const {
Storage } = await import('…') are detected consistently.
In `@scripts/codemod-import-type-annotations.mjs`:
- Around line 85-103: The import-extension logic in the codemod can incorrectly
try to merge named type imports into a namespace import declaration, which
`addNamedImport` cannot handle. Update the existing-declaration lookup in the
codemod entry point to ignore `import * as ns from ...` shapes, or detect that
the only matching declaration is a namespace import and then emit a separate
type-only import instead. Keep the fix localized around the
`existing`/`addNamedImport` flow so `import type` additions only target
compatible declarations.
In `@scripts/codemods/apply-suggestions.mjs`:
- Around line 61-69: The current apply-suggestions flow only looks at
suggestions[0], so it can miss later fixes and choose an overlap-prone edit
based on the first suggestion alone. Update the selection logic in
apply-suggestions.mjs to consider all suggestion entries from each ESLint
message, flatten them before sorting by fix.range[0], and then apply the
highest-range fix using the existing readFileSync/writeFileSync replacement
flow.
In `@scripts/codemods/nce-try-unreachable.mjs`:
- Around line 93-107: The rewrite in nce-try-unreachable.mjs is reusing
catchName as a top-level let binding, which can collide with an existing name in
the surrounding scope and break parsing. Update the codemod logic around the
replacement string to capture the thrown value in a fresh temporary variable,
then wrap the preserved catch body in a new block that reintroduces the original
catchName only inside that block. Keep the fix localized to the replacement
generation near keptTryStmts, keptCatchStmts, and catchName.
In `@scripts/lint.js`:
- Around line 214-217: The fallback in the changed-files retrieval logic still
exits before returning the empty array, so adjust the catch path in the function
handling the HEAD~1 fallback to degrade gracefully instead of hard-failing. In
the block around the second catch in scripts/lint.js, remove the premature
process.exit(1) and let the function return [] when both diff attempts fail,
keeping the existing console.error context in place.
In `@scripts/local_telemetry.js`:
- Around line 129-132: The child-process error handlers are being attached too
late in local telemetry startup, so an immediate spawn failure can fire before
the listeners exist. Move the call to registerProcessErrorHandlers(processes)
earlier in the startup flow in local_telemetry.js, before any waitForPort(...)
awaits and ideally right after starting the Jaeger/Collector processes in
startJaeger/startCollector, so ENOENT/EACCES and similar failures are captured
by the intended diagnostics.
In `@scripts/preflight-import-inventory.mjs`:
- Around line 177-182: Namespace imports are being recorded as a wildcard
placeholder in preflight-import-inventory, which later gets dropped and causes
namespace-member consumers to disappear from the inventory. Update the
namespace-import branch in the import scanning logic to record the actual
accessed moved symbols for importClause.namedBindings instead of only adding '*'
via ctx.addImport, so deduplicateImports() retains ns.Member-style consumers and
stays aligned with scripts/check-storage-import-boundary.mjs.
In `@scripts/telemetry_gcp.js`:
- Around line 103-108: The collector process `error` listener is being attached
too late in `startCollector()`-related flow, so an immediate `spawn()` failure
can fire before the handler exists. Move registration of the
`state.collectorProcess.on('error', ...)` handler into `startCollector()`
itself, alongside the process creation logic, so failures are always caught and
logged with collector diagnostics. Apply the same change anywhere else the
collector is started, including the other start/teardown path referenced by the
telemetry GCP code.
In `@scripts/tests/scripts-quality-coverage.test.ts`:
- Around line 47-116: The regression tests in
scripts/tests/scripts-quality-coverage.test.ts only assert severities, so they
can miss policy drift in the actual rule configuration. Update the affected
cases around effectiveRulesFor to also verify the configured options/off states
for the rules named in the suite, especially complexity, max-lines,
max-lines-per-function, and sonarjs/cognitive-complexity, rather than just
checking for 2/0. In the guard-parser coverage for
scripts/check-eslint-guard.js, add assertions that the documented carve-outs for
sonarjs/slow-regex and sonarjs/too-many-break-or-continue-in-loop are explicitly
disabled while the non-structural rules still apply.
In `@scripts/tmux-harness-io.mjs`:
- Around line 28-46: `runTmux()` is still using the default `spawnSync` buffer,
which can overflow on large pane captures and trigger `ENOBUFS`; update the
`spawnSync` call in `runTmux` to explicitly allow a much larger buffer or switch
to a non-buffered capture approach. Keep the change localized to the `runTmux`
helper in `scripts/tmux-harness-io.mjs`, and make sure the
`capture-pane`/`captureScrollback` path can handle the large history sizes
already enabled by `scripts/tmux-harness.js`.
In `@scripts/tmux-harness-steps.mjs`:
- Around line 332-343: The "always" branch in sendApprovalChoice is too fragile
because it assumes the desired option is always one row below the current
selection after only checking visibility. Update this logic to select "Yes,
allow always" by label the same way executeSelectToolOptionStep() does, using
the visible screen text from captureScreenWithFallback/sessionName and sendKeys
to move directly to the matching option instead of hardcoding a Down press. Keep
the existing visibility guard, but ensure the navigation targets the labeled
option rather than relying on dialog ordering.
In `@scripts/update-homebrew-formula.js`:
- Around line 125-151: The version check in resolveVersion is too permissive
because it only matches a prefix, so malformed values like 1.2.3foo can pass.
Tighten the validation in resolveVersion so the non-prerelease path requires the
entire string to match X.Y.Z exactly, while still preserving the existing
prerelease skip when version.includes('-') is true. Update the regex or
equivalent check in resolveVersion to anchor both ends before returning the
version.
In `@scripts/version.js`:
- Around line 84-87: The sandboxImageUri tag rewrite is replacing everything
after the first colon, which breaks registry host:port values like in
rootPackageJson and the manifest update path. Update the version rewrite logic
in scripts/version.js to target only the final image tag separator, so host
ports are preserved while the tag is changed to newVersion. Make the same fix in
both manifest update spots that feed config.sandboxImageUri, and ensure the
resulting value is still what packages/cli/src/config/sandboxConfig.ts expects
at runtime.
---
Outside diff comments:
In `@scripts/update-homebrew-formula.js`:
- Around line 220-252: The nested try/catch in commitAndPush currently treats
any failure from runCommand('git diff --cached --quiet') as “there are staged
changes,” which can hide real Git errors. Update the git diff check in
commitAndPush to only ignore the expected nonzero exit code for no staged
differences, and rethrow or surface any other failure before attempting git
commit and git push. Use the existing runCommand calls and the commitAndPush
function as the place to apply the fix.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ffa07e29-e90f-4d79-8192-f75fbbc41be4
📒 Files selected for processing (50)
eslint.config.jsscripts/aggregate_evals.jsscripts/benchmark/responses_vs_chat.tsscripts/bind-release-deps.jsscripts/check-build-status.jsscripts/check-eslint-guard.jsscripts/check-lockfile.jsscripts/check-settings-boundary.jsscripts/check-storage-import-boundary.mjsscripts/check-storage-package-cycle.mjsscripts/clean.jsscripts/codemod-import-type-annotations.mjsscripts/codemods/apply-suggestions.mjsscripts/codemods/nce-try-unreachable.mjsscripts/codemods/no-conditional-expect.mjsscripts/codemods/pse-disable.mjsscripts/codemods/pse-fix.mjsscripts/deflake.jsscripts/generate-keybindings-doc.tsscripts/generate-settings-doc.tsscripts/generate-settings-schema.tsscripts/get-release-version.jsscripts/issue2208-tui-repro.mjsscripts/lint.jsscripts/local_telemetry.jsscripts/ollama-logging-proxy.cjsscripts/preflight-import-inventory.mjsscripts/preinstall.cjsscripts/sandbox_command.jsscripts/start.jsscripts/telemetry_gcp.jsscripts/telemetry_utils.jsscripts/test-acp-integration.mjsscripts/test-mcp-server.jsscripts/tests/bun-workspaces.test.tsscripts/tests/eslint-guard.test.jsscripts/tests/interactive-ui.test.tsscripts/tests/loading-indicator-nowrap.test.jsscripts/tests/providers-directive-guard.test.jsscripts/tests/publish-integrity.test.tsscripts/tests/scripts-quality-coverage.test.tsscripts/tests/scrollback-regression.test.jsscripts/tests/ui-image-harness.test.jsscripts/tmux-harness-helpers.mjsscripts/tmux-harness-io.mjsscripts/tmux-harness-steps.mjsscripts/tmux-harness.jsscripts/update-homebrew-formula.jsscripts/verify-bun-workspace-links.mjsscripts/version.js
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: LLxprt Code CI / 7_Lint (GitHub Actions).txt: Apply strict code-quality lint rules to scripts tree (Fixes #2282)
Conclusion: failure
##[group]Run actionlint \
�[36;1mactionlint \�[0m
�[36;1m -color \�[0m
�[36;1m -format "{{range \$err := .}}::error file={{\$err.Filepath}},line={{\$err.Line}},col={{\$err.Column}}::{{\$err.Filepath}}@{{\$err.Line}} {{\$err.Message}}%0A\`\`\`%0A{{replace \$err.Snippet \"\\\\n\" \"%0A\"}}%0A\`\`\`\\n{{end}}" \�[0m
GitHub Actions: LLxprt Code CI / Lint (GitHub Actions): Apply strict code-quality lint rules to scripts tree (Fixes #2282)
Conclusion: failure
##[group]Run actionlint \
�[36;1mactionlint \�[0m
�[36;1m -color \�[0m
�[36;1m -format "{{range \$err := .}}::error file={{\$err.Filepath}},line={{\$err.Line}},col={{\$err.Column}}::{{\$err.Filepath}}@{{\$err.Line}} {{\$err.Message}}%0A\`\`\`%0A{{replace \$err.Snippet \"\\\\n\" \"%0A\"}}%0A\`\`\`\\n{{end}}" \�[0m
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
scripts/check-build-status.jsscripts/tests/interactive-ui.test.tsscripts/test-mcp-server.jsscripts/tests/ui-image-harness.test.jsscripts/tests/loading-indicator-nowrap.test.jsscripts/tests/scripts-quality-coverage.test.tsscripts/tests/providers-directive-guard.test.jsscripts/clean.jsscripts/deflake.jsscripts/generate-keybindings-doc.tsscripts/get-release-version.jsscripts/start.jsscripts/check-lockfile.jsscripts/tests/scrollback-regression.test.jsscripts/check-eslint-guard.jsscripts/generate-settings-doc.tsscripts/benchmark/responses_vs_chat.tsscripts/sandbox_command.jsscripts/telemetry_utils.jsscripts/version.jseslint.config.jsscripts/generate-settings-schema.tsscripts/tests/publish-integrity.test.tsscripts/aggregate_evals.jsscripts/lint.jsscripts/local_telemetry.jsscripts/tests/eslint-guard.test.jsscripts/tests/bun-workspaces.test.tsscripts/bind-release-deps.jsscripts/telemetry_gcp.jsscripts/update-homebrew-formula.jsscripts/tmux-harness.jsscripts/check-settings-boundary.js
📚 Learning: 2026-06-30T06:12:11.602Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/publish-integrity.test.ts:124-166
Timestamp: 2026-06-30T06:12:11.602Z
Learning: Do not add (or recommend adding) inline ESLint suppression directives such as `/* eslint-disable */`, `/* eslint-enable */`, or similar comment-based suppressions. This repository’s policy (`#2079/`#2080) bans inline ESLint suppressions and it is mechanically enforced by `scripts/check-eslint-guard.js` in the `lint:eslint-guard` CI job. During review, only suggest inline ESLint suppression if the author explicitly states the policy has changed (i.e., the CI guard would no longer block it).
Applied to files:
scripts/check-build-status.jsscripts/tests/interactive-ui.test.tsscripts/test-mcp-server.jsscripts/tests/ui-image-harness.test.jsscripts/tests/loading-indicator-nowrap.test.jsscripts/tests/scripts-quality-coverage.test.tsscripts/tests/providers-directive-guard.test.jsscripts/clean.jsscripts/deflake.jsscripts/generate-keybindings-doc.tsscripts/get-release-version.jsscripts/start.jsscripts/check-lockfile.jsscripts/tests/scrollback-regression.test.jsscripts/check-eslint-guard.jsscripts/generate-settings-doc.tsscripts/benchmark/responses_vs_chat.tsscripts/sandbox_command.jsscripts/telemetry_utils.jsscripts/version.jseslint.config.jsscripts/generate-settings-schema.tsscripts/tests/publish-integrity.test.tsscripts/aggregate_evals.jsscripts/lint.jsscripts/local_telemetry.jsscripts/tests/eslint-guard.test.jsscripts/tests/bun-workspaces.test.tsscripts/bind-release-deps.jsscripts/telemetry_gcp.jsscripts/update-homebrew-formula.jsscripts/tmux-harness.jsscripts/check-settings-boundary.js
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
scripts/tests/interactive-ui.test.tsscripts/tests/scripts-quality-coverage.test.tsscripts/generate-keybindings-doc.tsscripts/generate-settings-doc.tsscripts/benchmark/responses_vs_chat.tsscripts/generate-settings-schema.tsscripts/tests/publish-integrity.test.tsscripts/tests/bun-workspaces.test.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
scripts/tests/interactive-ui.test.tsscripts/tests/scripts-quality-coverage.test.tsscripts/generate-keybindings-doc.tsscripts/generate-settings-doc.tsscripts/benchmark/responses_vs_chat.tsscripts/generate-settings-schema.tsscripts/tests/publish-integrity.test.tsscripts/tests/bun-workspaces.test.ts
📚 Learning: 2026-06-30T04:44:54.618Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/detect-installer.test.ts:11-19
Timestamp: 2026-06-30T04:44:54.618Z
Learning: For files under `scripts/tests/` (including `scripts/tests/vitest.config.ts`), note that the Vitest ESM test setup shims `__dirname` and `__filename`. Therefore, `resolve(__dirname, ...)` (and similar use of `__dirname`) should not be flagged as an ESM incompatibility by itself during code review.
Applied to files:
scripts/tests/interactive-ui.test.tsscripts/tests/scripts-quality-coverage.test.tsscripts/tests/publish-integrity.test.tsscripts/tests/bun-workspaces.test.ts
📚 Learning: 2026-06-30T06:11:21.810Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/publish-integrity.test.ts:124-166
Timestamp: 2026-06-30T06:11:21.810Z
Learning: When reviewing files under scripts/tests/**/*.ts (e.g., scripts/tests/publish-integrity.test.ts), don’t suggest SonarJS inline disables based solely on the fact that the SonarJS rule is present in the repo’s root ESLint flat config. In vybestack/llxprt-code, the SonarJS TypeScript rule block is scoped to packages/*/src/**/*.{ts,tsx}, and the Vitest override to packages/*/src/**/*.{test,spec}.{ts,tsx}; scripts/tests/**/*.ts files don’t receive those blocks by default. Only recommend inline disables if SonarJS rules are actually enabled for that specific file and a violation is reported there (e.g., via the ESLint run/diagnostics).
Applied to files:
scripts/tests/interactive-ui.test.tsscripts/tests/scripts-quality-coverage.test.tsscripts/tests/publish-integrity.test.tsscripts/tests/bun-workspaces.test.ts
📚 Learning: 2026-06-30T04:44:57.052Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/verify-bun-workspace-links.test.ts:93-96
Timestamp: 2026-06-30T04:44:57.052Z
Learning: For test files under `scripts/tests` that use real filesystem symlinks (e.g., `symlinkSync` into `node_modules/<name>` or other symlink-based fixtures), avoid running them on unprivileged Windows because they commonly throw `EPERM`. Guard these tests with `describe.skipIf(process.platform === 'win32')` (using the repo’s existing `describe.skipIf` helper) to keep local Windows developer runs consistent with the sibling script-harness suites.
Applied to files:
scripts/tests/interactive-ui.test.tsscripts/tests/scripts-quality-coverage.test.tsscripts/tests/publish-integrity.test.tsscripts/tests/bun-workspaces.test.ts
📚 Learning: 2026-06-30T04:44:57.052Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/verify-bun-workspace-links.test.ts:93-96
Timestamp: 2026-06-30T04:44:57.052Z
Learning: When reviewing tests under `scripts/tests`, don’t treat Windows-specific skips/failures in `npm run test:scripts` as a CI requirement. The repo’s CI (`.github/workflows/ci.yml` and `.github/workflows/nightly.yml`) only runs the relevant script-test invocation on `macos-latest` (`if: matrix.os == 'macos-latest'`), so Windows compatibility measures should be interpreted as local-dev support rather than something that must be fixed to satisfy CI.
Applied to files:
scripts/tests/interactive-ui.test.tsscripts/tests/scripts-quality-coverage.test.tsscripts/tests/publish-integrity.test.tsscripts/tests/bun-workspaces.test.ts
📚 Learning: 2026-03-16T20:36:45.254Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1733
File: eslint.config.js:0-0
Timestamp: 2026-03-16T20:36:45.254Z
Learning: In vybestack/llxprt-code, the global baseline for no-console is intentionally set to 'warn' across core/cli packages. Do not flag this in reviews or suggest removing it. Entry points or CLI-specific configurations may override this to 'off' in a later pass when the rule is tightened to 'error'. Treat this as a deliberate, repository-wide baseline, and only flag console usage if there is an explicit deviation from the stated intent or an automation gate indicates improper override.
Applied to files:
eslint.config.js
🪛 ast-grep (0.44.0)
scripts/tests/ui-image-harness.test.js
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
scripts/tests/scripts-quality-coverage.test.ts
[warning] 7-7: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
scripts/local_telemetry.js
[warning] 125-125: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(OTEL_CONFIG_FILE, OTEL_CONFIG_CONTENT)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 155-155: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(JAEGER_LOG_FILE, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 181-181: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(OTEL_LOG_FILE, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
scripts/telemetry_gcp.js
[warning] 99-99: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(OTEL_CONFIG_FILE, getOtelConfigContent(projectId))
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 176-176: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(OTEL_LOG_FILE, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
scripts/check-settings-boundary.js
[warning] 408-408: Detects non-literal values in regular expressions
Context: new RegExp(\\b${sym}\\b)
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
[warning] 721-721: Detects non-literal values in regular expressions
Context: new RegExp(\\b${sym}\\b)
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
[warning] 744-744: Detects non-literal values in regular expressions
Context: new RegExp(export.*\\b${sym}\\b)
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
🪛 OpenGrep (1.23.0)
scripts/telemetry_utils.js
[ERROR] 234-234: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 236-236: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
scripts/lint.js
[ERROR] 264-264: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (36)
eslint.config.js (1)
499-649: LGTM!scripts/tmux-harness-helpers.mjs (1)
14-209: LGTM!scripts/tmux-harness-io.mjs (1)
49-268: LGTM!scripts/tmux-harness-steps.mjs (1)
44-331: LGTM!Also applies to: 346-464
scripts/tmux-harness.js (1)
20-82: LGTM!scripts/check-build-status.js (1)
32-32: LGTM!scripts/check-storage-package-cycle.mjs (1)
44-45: LGTM!scripts/tests/scrollback-regression.test.js (1)
24-25: LGTM!scripts/tests/providers-directive-guard.test.js (1)
65-89: LGTM!Also applies to: 106-106
scripts/tests/publish-integrity.test.ts (1)
109-166: LGTM!Also applies to: 183-186, 270-322, 355-361
scripts/tests/ui-image-harness.test.js (1)
15-21: LGTM!scripts/clean.js (1)
40-51: LGTM!scripts/generate-keybindings-doc.ts (1)
155-171: LGTM!scripts/generate-settings-doc.ts (1)
82-82: LGTM!Also applies to: 130-133
scripts/generate-settings-schema.ts (1)
157-175: LGTM!Also applies to: 213-239
scripts/get-release-version.js (1)
62-62: LGTM!scripts/issue2208-tui-repro.mjs (1)
54-56: LGTM!scripts/test-mcp-server.js (1)
86-88: LGTM!scripts/version.js (1)
35-37: LGTM!scripts/tests/bun-workspaces.test.ts (1)
353-368: LGTM!Also applies to: 392-428, 530-534, 547-549, 568-570, 594-594
scripts/tests/eslint-guard.test.js (1)
45-72: LGTM!scripts/tests/interactive-ui.test.ts (1)
48-49: LGTM!scripts/tests/loading-indicator-nowrap.test.js (1)
30-31: LGTM!scripts/check-eslint-guard.js (1)
2055-2055: LGTM!Also applies to: 2596-2603, 2934-2941
scripts/aggregate_evals.js (1)
38-41: LGTM!Also applies to: 50-84, 109-113, 121-170, 214-217, 236-256, 313-316, 357-361
scripts/bind-release-deps.js (1)
77-82: LGTM!Also applies to: 94-161, 176-177, 188-205, 217-226
scripts/update-homebrew-formula.js (1)
156-179: LGTM!Also applies to: 184-215, 257-282
scripts/verify-bun-workspace-links.mjs (1)
95-97: LGTM!Also applies to: 99-153, 155-227
scripts/deflake.js (1)
53-67: LGTM!Also applies to: 106-107
scripts/preinstall.cjs (1)
64-80: LGTM!Also applies to: 105-108
scripts/test-acp-integration.mjs (1)
28-28: LGTM!Also applies to: 69-125
scripts/telemetry_utils.js (1)
162-239: LGTM!Also applies to: 266-286
scripts/ollama-logging-proxy.cjs (1)
21-97: LGTM!Also applies to: 100-112, 123-123, 136-146, 155-155
scripts/start.js (1)
39-98: LGTM!scripts/sandbox_command.js (1)
62-94: LGTM!Also applies to: 138-148
scripts/benchmark/responses_vs_chat.ts (1)
72-122: LGTM!Also applies to: 147-250
) The durable root TS-suppression scan (scanRootTypeScriptSuppressions) walked the entire repo via listCheckedSourceFiles, which only skips generated directories. Local vendored upstream repos under research/ (gitignored) produced 53 false-positive violations in dirty worktrees, causing lint:eslint-guard and its test to fail locally. Add listGitTrackedCheckedSourceFiles: it runs git ls-files scoped to rootDir and filters the enumerated checked-source set to tracked files. When rootDir is not a git repository (temporary test fixtures via mkdtempSync) git exits non-zero and the function falls back to listCheckedSourceFiles, preserving all existing temp-dir test behaviour. Also harden findings from the correlated review of #2282: - Narrow processChar's data contract in publish-integrity.test.ts to return only the emitted chunk instead of threading the full accumulated output through every per-character call. - Assert the motivating rule (sonarjs/too-many-break-or-continue-in-loop) and the intentional type-aware gap (no-floating-promises) in the scripts quality-coverage test. - Add two guard tests proving untracked files are ignored while tracked files are still flagged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/check-eslint-guard.js`:
- Around line 4111-4124: Stop falling back to listCheckedSourceFiles(rootDir)
when git(['ls-files'], rootDir) succeeds but produces no tracked files; in
listGitTrackedCheckedSourceFiles, return the tracked-filtered result directly
and only use the fallback in the catch path. Add a regression in
scripts/tests/eslint-guard.test.js around scanRootTypeScriptSuppressions() that
covers an initialized repo/root with zero tracked checked sources (or only
untracked ones) and asserts the result is empty.
In `@scripts/tests/scripts-quality-coverage.test.ts`:
- Around line 100-110: The test currently checks a .js scripts path, so it can
miss a regression where type-aware `@typescript-eslint` rules are enabled only for
scripts/**/*.ts. Update the assertion in the scripts-quality-coverage test to
use the representative .ts scripts file path already used elsewhere in the
suite, and keep the severity checks on effectiveRulesFor and severity so the
contract for type-aware rules is actually enforced.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0a01dc27-10b9-4b0b-b7dc-0ac453546fd8
📒 Files selected for processing (4)
scripts/check-eslint-guard.jsscripts/tests/eslint-guard.test.jsscripts/tests/publish-integrity.test.tsscripts/tests/scripts-quality-coverage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
- GitHub Check: CodeQL
- GitHub Check: Lint (Javascript)
- GitHub Check: Interactive UI (tmux)
- GitHub Check: Run LLxprt review
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
scripts/tests/scripts-quality-coverage.test.tsscripts/tests/publish-integrity.test.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
scripts/tests/scripts-quality-coverage.test.tsscripts/tests/publish-integrity.test.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
scripts/tests/scripts-quality-coverage.test.tsscripts/tests/publish-integrity.test.tsscripts/check-eslint-guard.jsscripts/tests/eslint-guard.test.js
📚 Learning: 2026-06-30T06:12:11.602Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/publish-integrity.test.ts:124-166
Timestamp: 2026-06-30T06:12:11.602Z
Learning: Do not add (or recommend adding) inline ESLint suppression directives such as `/* eslint-disable */`, `/* eslint-enable */`, or similar comment-based suppressions. This repository’s policy (`#2079/`#2080) bans inline ESLint suppressions and it is mechanically enforced by `scripts/check-eslint-guard.js` in the `lint:eslint-guard` CI job. During review, only suggest inline ESLint suppression if the author explicitly states the policy has changed (i.e., the CI guard would no longer block it).
Applied to files:
scripts/tests/scripts-quality-coverage.test.tsscripts/tests/publish-integrity.test.tsscripts/check-eslint-guard.jsscripts/tests/eslint-guard.test.js
📚 Learning: 2026-06-30T04:44:54.618Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/detect-installer.test.ts:11-19
Timestamp: 2026-06-30T04:44:54.618Z
Learning: For files under `scripts/tests/` (including `scripts/tests/vitest.config.ts`), note that the Vitest ESM test setup shims `__dirname` and `__filename`. Therefore, `resolve(__dirname, ...)` (and similar use of `__dirname`) should not be flagged as an ESM incompatibility by itself during code review.
Applied to files:
scripts/tests/scripts-quality-coverage.test.tsscripts/tests/publish-integrity.test.ts
📚 Learning: 2026-06-30T06:11:21.810Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/publish-integrity.test.ts:124-166
Timestamp: 2026-06-30T06:11:21.810Z
Learning: When reviewing files under scripts/tests/**/*.ts (e.g., scripts/tests/publish-integrity.test.ts), don’t suggest SonarJS inline disables based solely on the fact that the SonarJS rule is present in the repo’s root ESLint flat config. In vybestack/llxprt-code, the SonarJS TypeScript rule block is scoped to packages/*/src/**/*.{ts,tsx}, and the Vitest override to packages/*/src/**/*.{test,spec}.{ts,tsx}; scripts/tests/**/*.ts files don’t receive those blocks by default. Only recommend inline disables if SonarJS rules are actually enabled for that specific file and a violation is reported there (e.g., via the ESLint run/diagnostics).
Applied to files:
scripts/tests/scripts-quality-coverage.test.tsscripts/tests/publish-integrity.test.ts
📚 Learning: 2026-06-30T04:44:57.052Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/verify-bun-workspace-links.test.ts:93-96
Timestamp: 2026-06-30T04:44:57.052Z
Learning: For test files under `scripts/tests` that use real filesystem symlinks (e.g., `symlinkSync` into `node_modules/<name>` or other symlink-based fixtures), avoid running them on unprivileged Windows because they commonly throw `EPERM`. Guard these tests with `describe.skipIf(process.platform === 'win32')` (using the repo’s existing `describe.skipIf` helper) to keep local Windows developer runs consistent with the sibling script-harness suites.
Applied to files:
scripts/tests/scripts-quality-coverage.test.tsscripts/tests/publish-integrity.test.ts
📚 Learning: 2026-06-30T04:44:57.052Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/verify-bun-workspace-links.test.ts:93-96
Timestamp: 2026-06-30T04:44:57.052Z
Learning: When reviewing tests under `scripts/tests`, don’t treat Windows-specific skips/failures in `npm run test:scripts` as a CI requirement. The repo’s CI (`.github/workflows/ci.yml` and `.github/workflows/nightly.yml`) only runs the relevant script-test invocation on `macos-latest` (`if: matrix.os == 'macos-latest'`), so Windows compatibility measures should be interpreted as local-dev support rather than something that must be fixed to satisfy CI.
Applied to files:
scripts/tests/scripts-quality-coverage.test.tsscripts/tests/publish-integrity.test.ts
The scripts-quality-coverage behavioral test spawns eslint --print-config to inspect effective rule sets. On macOS CI runners the ESLint startup + flat-config load exceeded vitest's 5s default per-test timeout, causing a spurious failure. Memoize effectiveRulesFor so each unique path is resolved at most once, and add a 30s per-test timeout (CONFIG_TIMEOUT) to every case since these are integration tests that spawn an external process.
The new scripts/check-cli-import-boundary.mjs (added to main by PR #2265) contained 9 quality-rule violations under the strict scripts lint layer introduced by this PR. Fix at root cause, no suppressions: - Decompose 174-line main() into 4 phase functions (runDeepImportScan, runGetConfigScan, runAllowlistFreshness, runThinEntryGuard) plus extracted helpers (formatViolationLine, collectStaleEntries, etc.). - Fix expression-complexity in isViMockCall via guard clause. - Fix too-many-break-or-continue in walk loop via extracted processEntry. - Auto-fix arrow-body-style in cli-import-boundary.test.js. All 28 cli-import-boundary tests pass.
…2282) CodeRabbit findings (2 valid bugs fixed): 1. check-eslint-guard.js listGitTrackedCheckedSourceFiles: when git ls-files succeeds but returns zero tracked files (fresh repo / all gitignored), the code incorrectly fell back to the unfiltered filesystem walk, reintroducing the exact false positives the git-tracking filter was designed to prevent. Now returns an empty list; only falls back on an actual git failure (catch block). 2. scripts-quality-coverage.test.ts: the type-aware gap assertion used scripts/start.js (.js). Type-aware @typescript-eslint rules only activate for files the TS parser processes, so the assertion would pass vacuously if those rules were enabled only for .ts. Now uses a .ts file to lock the real contract.
) The check-eslint-guard.js was exempted from 8 structural quality rules (complexity, nested-control-flow, cognitive-complexity, etc.) via an eslint.config.js carve-out. This undermined the entire purpose of issue #2282 — extending quality coverage to scripts. Decomposed the 5193-line monolith into 13 focused modules under scripts/eslint-guard/, each independently passing all structural quality rules. The entry point scripts/check-eslint-guard.js is now a 131-line thin re-exporter with main(). Modules: constants, git, directive-scanner, rule-config, diff-context, check-diff, diff-state-tracking, added-config-checks, scanners, cli-scanner, config-scanner, bypass-detector, violations. Removed the carve-out from eslint.config.js. All 454 guard tests pass with zero behavior changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/eslint-guard/added-config-checks.mjs`:
- Around line 215-222: Aggregate multiline removed state before keyed comparison
so cross-form checks use the full removed rule, not just the first pending
entry. Update findMatchingRemovedKeyed and the related logic in the keyed
comparison flow to combine severity and threshold parts of removed multiline
ceiling rules before comparing against added single-line keyed rules. Make sure
the match/search path considers the buffered multiline pieces together, so
additions aren’t misreported and real increases/decreases are evaluated against
the complete removed state.
- Around line 45-49: The inline-rules detection in checkInlineRulesEntries is
too broad and can treat nested config objects as ESLint rules. Update the guard
so extractInlineRulesEntries only runs when the current object is actually a
rules container, mirroring the structural check used by openRulesBlockIfNeeded,
and skip parsing whenever state.arbitraryObjectDepth indicates a non-rule
container such as settings or other nested objects.
In `@scripts/eslint-guard/bypass-detector.mjs`:
- Around line 18-35: `stripInlineComment()` only strips `//`, so block comments
can still influence `isModulePathLine()`,
`checkModuleDirectiveScopesInConfig()`, and the brace/module depth tracking in
`bypass-detector.mjs`. Update the parsing logic to recognize and ignore `/* ...
*/` comments before module-path and brace analysis, and ensure the same
block-comment-aware handling is applied in the module-scope detection paths so
commented-out paths cannot seed `ctx.moduleObjectDepth` or alter
`ctx.braceDepth`.
In `@scripts/eslint-guard/diff-state-tracking.mjs`:
- Around line 391-395: `bufferInlineRules` is still collecting inline rules from
removed content even when it is inside a removed non-rule container. Update
`bufferInlineRules` in `diff-state-tracking.mjs` to also bail out when
`state.removedNonRuleContainerDepth` is set, alongside the existing
`removedArbitraryObjectDepth` check, so `pendingRemovedInlineRules` is not
seeded from `settings: { ... rules: { ... } }`-style blocks.
In `@scripts/eslint-guard/directive-scanner.mjs`:
- Around line 447-454: The brace-tracking logic in scanTemplateTextChar is
counting `${` twice, which leaves exprDepth too high and can make
scanTemplateLiteralState miss trailing line comments after template expressions.
Update the `${` branch in scanTemplateTextChar so it consumes the sequence as
one transition, matching the behavior already used by scanTemplateLiteralState,
and ensure the scanner only increments exprDepth once for the opening
interpolation.
- Around line 395-399: The fast-path in directive scanning is skipping regex
literals only outside templates, but template expressions inside ${...} still
contain executable JS and can include regexes like in directive-scanner.mjs.
Update the regex-literal handling around canStartRegex/skipRegex so it also
applies while scanning inside template expressions, using the existing
state.inTemplate tracking (or a template-expression equivalent) to distinguish
raw template text from embedded JS. This should prevent // inside a regex from
being misread as a comment before the real trailing directive comment.
In `@scripts/eslint-guard/git.mjs`:
- Around line 21-27: Update the argument parsing in git.mjs so the --base and
--head handling in the argv loop rejects missing values instead of assigning
undefined; if argv[++i] is absent or looks like another flag, stop parsing and
emit the same usage/help error path used by the --help case. Make the check in
the argument loop that sets args.base and args.head, so execFileSync('git', ...)
only runs with validated values.
In `@scripts/eslint-guard/rule-config.mjs`:
- Around line 60-63: The extractThresholdValue helper only matches unquoted max
keys, so single-line rule configs like quoted max thresholds are missed. Update
extractThresholdValue in rule-config.mjs to recognize both quoted and unquoted
max keys, using the same pattern support already present in the standalone max
helpers. Keep the existing return shape with value and form so the guard
continues to work for ceiling-rule threshold changes.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7fba285f-0cc7-4b4f-af69-f2fef7c9955e
📒 Files selected for processing (16)
eslint.config.jsscripts/check-eslint-guard.jsscripts/eslint-guard/added-config-checks.mjsscripts/eslint-guard/bypass-detector.mjsscripts/eslint-guard/check-diff.mjsscripts/eslint-guard/cli-scanner.mjsscripts/eslint-guard/config-scanner.mjsscripts/eslint-guard/constants.mjsscripts/eslint-guard/diff-context.mjsscripts/eslint-guard/diff-state-tracking.mjsscripts/eslint-guard/directive-scanner.mjsscripts/eslint-guard/git.mjsscripts/eslint-guard/rule-config.mjsscripts/eslint-guard/scanners.mjsscripts/eslint-guard/violations.mjsscripts/tests/scripts-quality-coverage.test.ts
💤 Files with no reviewable changes (1)
- eslint.config.js
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: CodeQL
- GitHub Check: Lint (Javascript)
- GitHub Check: Interactive UI (tmux)
- GitHub Check: Run LLxprt review
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
scripts/tests/scripts-quality-coverage.test.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
scripts/tests/scripts-quality-coverage.test.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
scripts/tests/scripts-quality-coverage.test.ts
📚 Learning: 2026-06-30T06:12:11.602Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/publish-integrity.test.ts:124-166
Timestamp: 2026-06-30T06:12:11.602Z
Learning: Do not add (or recommend adding) inline ESLint suppression directives such as `/* eslint-disable */`, `/* eslint-enable */`, or similar comment-based suppressions. This repository’s policy (`#2079/`#2080) bans inline ESLint suppressions and it is mechanically enforced by `scripts/check-eslint-guard.js` in the `lint:eslint-guard` CI job. During review, only suggest inline ESLint suppression if the author explicitly states the policy has changed (i.e., the CI guard would no longer block it).
Applied to files:
scripts/tests/scripts-quality-coverage.test.ts
📚 Learning: 2026-06-30T04:44:54.618Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/detect-installer.test.ts:11-19
Timestamp: 2026-06-30T04:44:54.618Z
Learning: For files under `scripts/tests/` (including `scripts/tests/vitest.config.ts`), note that the Vitest ESM test setup shims `__dirname` and `__filename`. Therefore, `resolve(__dirname, ...)` (and similar use of `__dirname`) should not be flagged as an ESM incompatibility by itself during code review.
Applied to files:
scripts/tests/scripts-quality-coverage.test.ts
📚 Learning: 2026-06-30T06:11:21.810Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/publish-integrity.test.ts:124-166
Timestamp: 2026-06-30T06:11:21.810Z
Learning: When reviewing files under scripts/tests/**/*.ts (e.g., scripts/tests/publish-integrity.test.ts), don’t suggest SonarJS inline disables based solely on the fact that the SonarJS rule is present in the repo’s root ESLint flat config. In vybestack/llxprt-code, the SonarJS TypeScript rule block is scoped to packages/*/src/**/*.{ts,tsx}, and the Vitest override to packages/*/src/**/*.{test,spec}.{ts,tsx}; scripts/tests/**/*.ts files don’t receive those blocks by default. Only recommend inline disables if SonarJS rules are actually enabled for that specific file and a violation is reported there (e.g., via the ESLint run/diagnostics).
Applied to files:
scripts/tests/scripts-quality-coverage.test.ts
📚 Learning: 2026-06-30T04:44:57.052Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/verify-bun-workspace-links.test.ts:93-96
Timestamp: 2026-06-30T04:44:57.052Z
Learning: For test files under `scripts/tests` that use real filesystem symlinks (e.g., `symlinkSync` into `node_modules/<name>` or other symlink-based fixtures), avoid running them on unprivileged Windows because they commonly throw `EPERM`. Guard these tests with `describe.skipIf(process.platform === 'win32')` (using the repo’s existing `describe.skipIf` helper) to keep local Windows developer runs consistent with the sibling script-harness suites.
Applied to files:
scripts/tests/scripts-quality-coverage.test.ts
📚 Learning: 2026-06-30T04:44:57.052Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/verify-bun-workspace-links.test.ts:93-96
Timestamp: 2026-06-30T04:44:57.052Z
Learning: When reviewing tests under `scripts/tests`, don’t treat Windows-specific skips/failures in `npm run test:scripts` as a CI requirement. The repo’s CI (`.github/workflows/ci.yml` and `.github/workflows/nightly.yml`) only runs the relevant script-test invocation on `macos-latest` (`if: matrix.os == 'macos-latest'`), so Windows compatibility measures should be interpreted as local-dev support rather than something that must be fixed to satisfy CI.
Applied to files:
scripts/tests/scripts-quality-coverage.test.ts
📚 Learning: 2026-06-30T16:45:45.666Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2284
File: scripts/tests/scripts-quality-coverage.test.ts:0-0
Timestamp: 2026-06-30T16:45:45.666Z
Learning: For ESLint-config regression tests under scripts/tests (e.g., scripts-quality-coverage.test.ts), avoid adding duplicate, option-level assertions. These tests should only verify that the intended ESLint rules are applied to the target file set at error severity. Since the lint:eslint-guard CI job in vybestack/llxprt-code mechanically rejects ESLint ceiling-threshold increases and severity downgrades, reviewers should not request redundant checks that merely restate option-level guard behavior.
Applied to files:
scripts/tests/scripts-quality-coverage.test.ts
📚 Learning: 2026-06-30T20:03:19.860Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2290
File: packages/cli/src/launcher/bun-launcher.ts:209-226
Timestamp: 2026-06-30T20:03:19.860Z
Learning: If a change introduces large test blocks that would exceed the repository’s max-lines lint limit, split the test into a dedicated test file (e.g., move related coverage into a new {name}.test.ts file) rather than keeping everything in one test module. Ensure the extracted tests still cover the same behavior and are correctly imported/exercised by the test runner.
Applied to files:
scripts/tests/scripts-quality-coverage.test.ts
🔇 Additional comments (6)
scripts/eslint-guard/violations.mjs (1)
7-16: LGTM!scripts/tests/scripts-quality-coverage.test.ts (1)
147-151: LGTM!Also applies to: 166-185
scripts/eslint-guard/cli-scanner.mjs (1)
17-142: LGTM!scripts/eslint-guard/check-diff.mjs (1)
55-672: LGTM!scripts/eslint-guard/diff-context.mjs (1)
40-408: LGTM!scripts/eslint-guard/scanners.mjs (1)
22-384: LGTM!
- local_telemetry.js + telemetry_gcp.js: attach child-process error handlers immediately after spawn (before waitForPort) so immediate spawn failures (ENOENT/EACCES) are caught by the intended diagnostics instead of crashing via uncaught-exception. - version.js: replace regex tag-rewrite with lastIndexOf+slice so registry host:port segments are preserved when the sandbox image URI contains a port (e.g. ghcr.io:443/org/image:tag).
- rule-config.mjs: detect quoted max keys in extractThresholdValue so threshold increases in single-line ceiling rules cannot bypass the guard. - git.mjs: reject --base/--head when the value is missing instead of dying with a raw execFileSync argument error. - bypass-detector.mjs: strip block comments (not just line comments) so module paths inside comments cannot seed bogus bypass detections. - added-config-checks.mjs: skip inline rules inside non-rule containers; aggregate removed multiline severity+threshold entries before cross-form keyed comparisons to avoid false threshold-addition reports. - diff-state-tracking.mjs: skip removed inline rules inside non-rule containers (parity with the added-side fix). - directive-scanner.mjs: skip regex literals inside template expressions; consume dollar-brace as a single transition in the brace-tracking scanner to avoid double-counting exprDepth.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This was referenced Jul 1, 2026
Bun CI run-path blocked: type-aware ESLint + incomplete hoisted-linker package materialization
#2308
Closed
Closed
This was referenced Jul 15, 2026
This was referenced Jul 23, 2026
This was referenced Jul 25, 2026
Closed
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.
Summary
Extends the repository's strict code-quality ESLint layer (SonarJS maintainability, regex correctness, and size/complexity limits) to the scripts/** tree and scripts/tests/**, which previously received only the base ESLint recommended layer. Build/dev scripts are now held to the same maintainability bar as packages/*/src, without adding any inline suppressions or weakening existing guardrails.
Fixes #2282
What changed
eslint.config.js — new scripts quality block
Adds a config block targeting scripts/**/*.{ts,tsx,js,mjs,cjs} with the curated quality rule set, applying non-type-aware parity with the packages source block:
Intentionally excluded (documented rationale in config): no-console (scripts legitimately log to stdout/stderr), type-aware TS rules like no-floating-promises/strict-boolean-expressions (scripts have no tsconfig project, so type-aware analysis is unavailable), import/* rules (package-resolution policies specific to application packages), and React rules (no UI in scripts).
Three centralized carve-outs (all tagged // eslint-policy-allow-off: #2282)
Violation fixes (~117 across ~45 scripts)
All newly-exposed violations fixed at root cause — no inline suppressions, no severity downgrades, no threshold increases:
tmux-harness.js split
Split tmux-harness.js (1273 lines) into tmux-harness.js + tmux-harness-helpers.mjs + tmux-harness-io.mjs + tmux-harness-steps.mjs to get under the 800-line max-lines limit, preserving the public API (all 63 existing tests pass).
Behavioral regression test
New scripts/tests/scripts-quality-coverage.test.ts uses eslint --print-config to assert:
Verification
Note: npm run typecheck has pre-existing failures in packages/cli and packages/providers (LoadBalancerSelectionChanged) that are unrelated to this PR — those files are not in this diff.
Acceptance criteria