Conversation
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
Summary by CodeRabbit
WalkthroughThe PR eliminates all inline ESLint disable directives from the ChangesSettings Package ESLint Directive Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
LLxprt PR Review – PR #2149Issue AlignmentEvidence of success: On the Side Effects
Code Quality
Tests and CoverageCoverage impact: Increase New behavioral tests added in
SettingsService test was corrected: removed the The SettingsService tests pass (29 tests). The eslint-guard tests pass (15 tests, including the 3 new settings-specific ones). Failures observed in Verdict: ReadyThe PR achieves the issue goal: |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/settings/src/settings/SettingsService.ts (1)
10-21: 📐 Maintainability & Code Quality | 🟡 MinorRemove the stray
n?: unknownfield fromEphemeralSettings. It isn’t used anywhere inpackages/settings/srcand only adds noise to the settings contract.🤖 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/settings/src/settings/SettingsService.ts` around lines 10 - 21, Remove the stray n?: unknown field from the EphemeralSettings interface in SettingsService; it is not used anywhere in packages/settings/src. Update the settings contract definition only, keeping the existing providers, global, activeProvider, and tools shape intact.
🤖 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 `@packages/settings/src/profiles/ProfileManager.ts`:
- Around line 115-137: hasStandardProfileFields() is too permissive because it
only checks that modelParams and ephemeralSettings are non-null, allowing
primitives like numbers or booleans to pass into loadProfile() and be cast as
Profile. Tighten the guard in ProfileManager by validating that
record.modelParams and record.ephemeralSettings are object-shaped (and not
arrays) before returning true, and keep the existing version/provider/model
checks intact so the final Profile cast only happens for properly shaped
records.
- Around line 75-99: validateLoadBalancerShape() is missing a check for the
load-balancer discriminant before it returns a LoadBalancerProfile. Update this
validator in ProfileManager.ts to verify that the incoming record has type ===
'loadbalancer' in addition to version === 1 and a non-empty profiles array, and
throw the same kind of validation error when the type is missing or incorrect.
Make sure saveLoadBalancerProfile() relies on this strengthened validation so
only properly shaped load-balancer profiles can be persisted and later loaded by
loadProfile().
In `@packages/settings/src/settings/SettingsService.ts`:
- Around line 283-291: Empty-string activeProvider values should consistently
fall back to 'openai' instead of being exported as blank. Update the fallback
logic in SettingsService, specifically the activeProvider selection used by
exportForProfile and the matching branch in getDiagnosticsData, so the
settings.activeProvider path rejects empty strings the same way the
global.activeProvider path does. Keep the existing defaulting behavior intact by
only accepting non-empty strings before assigning activeProvider.
In `@scripts/tests/eslint-guard.test.js`:
- Around line 237-243: The ESLint guard test is flagging any line containing the
directive text, including strings, instead of only actual comment directives.
Update the directive matching logic in eslint-guard.test.js around directiveRe
and the offenders scan so it only matches inline ESLint comment syntax, and keep
using listTsFiles/readFileSync/lines.forEach to locate the affected lines. Make
the check ignore non-comment occurrences while still detecting eslint-disable
and eslint-enable directives in comments.
---
Outside diff comments:
In `@packages/settings/src/settings/SettingsService.ts`:
- Around line 10-21: Remove the stray n?: unknown field from the
EphemeralSettings interface in SettingsService; it is not used anywhere in
packages/settings/src. Update the settings contract definition only, keeping the
existing providers, global, activeProvider, and tools shape intact.
🪄 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: 67fe1291-954d-4daa-8b01-df4a9c2f77e4
📒 Files selected for processing (6)
eslint.config.jspackages/settings/src/__tests__/SettingsService.test.tspackages/settings/src/profiles/ProfileManager.tspackages/settings/src/profiles/__tests__/ProfileManager.test.tspackages/settings/src/settings/SettingsService.tsscripts/tests/eslint-guard.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: Lint (Javascript)
- GitHub Check: CodeQL
- GitHub Check: Run LLxprt review
- GitHub Check: Interactive UI (tmux)
🧰 Additional context used
🧠 Learnings (7)
📚 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:
packages/settings/src/__tests__/SettingsService.test.tspackages/settings/src/profiles/__tests__/ProfileManager.test.tspackages/settings/src/profiles/ProfileManager.tspackages/settings/src/settings/SettingsService.ts
📚 Learning: 2026-03-26T00:49:43.150Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/__tests__/auth-flow-orchestrator.spec.ts:309-324
Timestamp: 2026-03-26T00:49:43.150Z
Learning: In this repository’s Jest (or Jest-like) test files, it is acceptable to use `expect(promiseReturningFunction).resolves.not.toThrow()` when the function returns `Promise<void>`. Do not flag this as an incorrect or suboptimal matcher; for `Promise<void>` it is functionally equivalent to using `resolves.toBeUndefined()` to assert successful resolution.
Applied to files:
packages/settings/src/__tests__/SettingsService.test.tspackages/settings/src/profiles/__tests__/ProfileManager.test.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/settings/src/__tests__/SettingsService.test.tspackages/settings/src/profiles/__tests__/ProfileManager.test.tspackages/settings/src/profiles/ProfileManager.tspackages/settings/src/settings/SettingsService.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:
packages/settings/src/__tests__/SettingsService.test.tspackages/settings/src/profiles/__tests__/ProfileManager.test.tspackages/settings/src/profiles/ProfileManager.tspackages/settings/src/settings/SettingsService.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:
packages/settings/src/__tests__/SettingsService.test.tspackages/settings/src/profiles/__tests__/ProfileManager.test.tsscripts/tests/eslint-guard.test.jspackages/settings/src/profiles/ProfileManager.tseslint.config.jspackages/settings/src/settings/SettingsService.ts
📚 Learning: 2026-06-19T17:16:56.523Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2108
File: packages/agents/src/api/agentImpl.ts:1047-1079
Timestamp: 2026-06-19T17:16:56.523Z
Learning: When the fake-provider test seam is active in vybestack/llxprt-code, `process.env.LLXPRT_FAKE_RESPONSES` is set to a fixture file path ending in a `.jsonl` (not to the string `'1'` or any other boolean-like value). In code, detect the seam by checking `process.env.LLXPRT_FAKE_RESPONSES !== undefined` (and/or that it is a non-empty string), rather than using `process.env.LLXPRT_FAKE_RESPONSES === '1'`. Update any callers of the env var accordingly (see `packages/providers/src/composition/providerManagerInstance.ts` and harness usages).
Applied to files:
packages/settings/src/__tests__/SettingsService.test.tspackages/settings/src/profiles/__tests__/ProfileManager.test.tspackages/settings/src/profiles/ProfileManager.tspackages/settings/src/settings/SettingsService.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)
packages/settings/src/profiles/__tests__/ProfileManager.test.ts
[warning] 361-361: 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.writeFile(filePath, 'null', 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 370-370: 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.writeFile(filePath, '[1, 2, 3]', 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 379-379: 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.writeFile(filePath, '42', 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 388-388: 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.writeFile(filePath, '{}', 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 411-415: 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.writeFile(
filePath,
JSON.stringify({ type: 'loadbalancer', version: 2 }),
'utf8',
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 424-432: 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.writeFile(
filePath,
JSON.stringify({
type: 'loadbalancer',
version: 2,
profiles: ['p1'],
}),
'utf8',
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 441-445: 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.writeFile(
filePath,
JSON.stringify({ type: 'loadbalancer', version: 1 }),
'utf8',
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 454-462: 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.writeFile(
filePath,
JSON.stringify({
type: 'loadbalancer',
version: 1,
profiles: [],
}),
'utf8',
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 471-475: 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.writeFile(
filePath,
JSON.stringify({ type: 'loadbalancer', version: 2 }),
'utf8',
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🔇 Additional comments (6)
packages/settings/src/profiles/__tests__/ProfileManager.test.ts (1)
346-395: LGTM!Also applies to: 397-482
packages/settings/src/__tests__/SettingsService.test.ts (1)
44-44: LGTM!packages/settings/src/profiles/ProfileManager.ts (1)
151-153: LGTM!Also applies to: 219-219, 259-278, 283-287, 420-440, 459-460
packages/settings/src/settings/SettingsService.ts (1)
39-56: LGTM!Also applies to: 114-118, 166-179, 260-275, 360-369
eslint.config.js (1)
292-296: LGTM!scripts/tests/eslint-guard.test.js (1)
250-264: LGTM!
Comment on lines
+75
to
+99
| function validateLoadBalancerShape( | ||
| name: string, | ||
| profile: unknown, | ||
| ): LoadBalancerProfile { | ||
| if (!isPlainObject(profile)) { | ||
| throw new Error( | ||
| `LoadBalancer profile '${name}' must reference at least one profile`, | ||
| ); | ||
| } | ||
| const record = profile; | ||
| if (record.version !== 1) { | ||
| throw new Error('unsupported profile version'); | ||
| } | ||
| const profilesField = record.profiles; | ||
| if ( | ||
| profilesField === null || | ||
| profilesField === undefined || | ||
| !Array.isArray(profilesField) || | ||
| profilesField.length === 0 | ||
| ) { | ||
| throw new Error( | ||
| `LoadBalancer profile '${name}' must reference at least one profile`, | ||
| ); | ||
| } | ||
| return profile as unknown as LoadBalancerProfile; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the load-balancer discriminant before saving.
validateLoadBalancerShape() casts to LoadBalancerProfile without checking type === 'loadbalancer'. Since saveLoadBalancerProfile() persists after this check, a malformed object with version: 1 and profiles: [...] but no/incorrect type can be saved and later rejected by loadProfile() as a standard profile.
Proposed fix
const record = profile;
+ if (record.type !== 'loadbalancer') {
+ throw new Error(`LoadBalancer profile '${name}' has invalid type`);
+ }
if (record.version !== 1) {
throw new Error('unsupported profile version');
}Also applies to: 176-176
🤖 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/settings/src/profiles/ProfileManager.ts` around lines 75 - 99,
validateLoadBalancerShape() is missing a check for the load-balancer
discriminant before it returns a LoadBalancerProfile. Update this validator in
ProfileManager.ts to verify that the incoming record has type === 'loadbalancer'
in addition to version === 1 and a non-empty profiles array, and throw the same
kind of validation error when the type is missing or incorrect. Make sure
saveLoadBalancerProfile() relies on this strengthened validation so only
properly shaped load-balancer profiles can be persisted and later loaded by
loadProfile().
Comment on lines
+115
to
+137
| function hasStandardProfileFields(record: Record<string, unknown>): boolean { | ||
| if (isMissingVersion(record.version)) { | ||
| return false; | ||
| } | ||
| const provider = record.provider; | ||
| if (typeof provider !== 'string' || provider === '') { | ||
| return false; | ||
| } | ||
| const model = record.model; | ||
| if (typeof model !== 'string' || model === '') { | ||
| return false; | ||
| } | ||
| if (record.modelParams === null || record.modelParams === undefined) { | ||
| return false; | ||
| } | ||
| if ( | ||
| record.ephemeralSettings === null || | ||
| record.ephemeralSettings === undefined | ||
| ) { | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require object-shaped standard profile fields.
hasStandardProfileFields() accepts any non-null modelParams/ephemeralSettings, so JSON like "modelParams": 1 or "ephemeralSettings": false passes loadProfile() and is returned as Profile. Tighten the guard before the final cast.
Proposed fix
- if (record.modelParams === null || record.modelParams === undefined) {
+ if (!isPlainObject(record.modelParams)) {
return false;
}
- if (
- record.ephemeralSettings === null ||
- record.ephemeralSettings === undefined
- ) {
+ if (!isPlainObject(record.ephemeralSettings)) {
return false;
}Also applies to: 279-287
🤖 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/settings/src/profiles/ProfileManager.ts` around lines 115 - 137,
hasStandardProfileFields() is too permissive because it only checks that
modelParams and ephemeralSettings are non-null, allowing primitives like numbers
or booleans to pass into loadProfile() and be cast as Profile. Tighten the guard
in ProfileManager by validating that record.modelParams and
record.ephemeralSettings are object-shaped (and not arrays) before returning
true, and keep the existing version/provider/model checks intact so the final
Profile cast only happens for properly shaped records.
Comment on lines
+283
to
+291
| const globalActive = this.settings.global.activeProvider; | ||
| let activeProvider: string; | ||
| if (typeof globalActive === 'string' && globalActive !== '') { | ||
| activeProvider = globalActive; | ||
| } else if (this.settings.activeProvider !== null) { | ||
| activeProvider = this.settings.activeProvider; | ||
| } else { | ||
| activeProvider = 'openai'; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Empty-string activeProvider skips the 'openai' fallback.
The global branch (Line 285) explicitly rejects '', but the settings.activeProvider branch (Line 287) only checks !== null. If settings.activeProvider is an empty string, exportForProfile exports defaultProvider: '' instead of falling back to 'openai'. The same pattern exists in getDiagnosticsData (Line 385). Low risk today since it's normally set to non-empty values, but worth tightening for consistency.
🔧 Proposed fix
- } else if (this.settings.activeProvider !== null) {
+ } else if (
+ this.settings.activeProvider !== null &&
+ this.settings.activeProvider !== ''
+ ) {
activeProvider = this.settings.activeProvider;
} else {
activeProvider = 'openai';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const globalActive = this.settings.global.activeProvider; | |
| let activeProvider: string; | |
| if (typeof globalActive === 'string' && globalActive !== '') { | |
| activeProvider = globalActive; | |
| } else if (this.settings.activeProvider !== null) { | |
| activeProvider = this.settings.activeProvider; | |
| } else { | |
| activeProvider = 'openai'; | |
| } | |
| const globalActive = this.settings.global.activeProvider; | |
| let activeProvider: string; | |
| if (typeof globalActive === 'string' && globalActive !== '') { | |
| activeProvider = globalActive; | |
| } else if ( | |
| this.settings.activeProvider !== null && | |
| this.settings.activeProvider !== '' | |
| ) { | |
| activeProvider = this.settings.activeProvider; | |
| } else { | |
| activeProvider = 'openai'; | |
| } |
🤖 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/settings/src/settings/SettingsService.ts` around lines 283 - 291,
Empty-string activeProvider values should consistently fall back to 'openai'
instead of being exported as blank. Update the fallback logic in
SettingsService, specifically the activeProvider selection used by
exportForProfile and the matching branch in getDiagnosticsData, so the
settings.activeProvider path rejects empty strings the same way the
global.activeProvider path does. Keep the existing defaulting behavior intact by
only accepting non-empty strings before assigning activeProvider.
Comment on lines
+237
to
+243
| const directiveRe = /eslint-(?:disable|enable)(?:-next-line|-line)?\b/; | ||
| const offenders = []; | ||
| for (const file of listTsFiles(settingsSrcDir)) { | ||
| const lines = readFileSync(file, 'utf8').split('\n'); | ||
| lines.forEach((line, idx) => { | ||
| if (directiveRe.test(line)) { | ||
| offenders.push(file.replace(repoRoot + '/', '') + ':' + (idx + 1)); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Limit directive detection to comment syntax to avoid false positives.
On Line 237, the regex matches token text anywhere in a line, including strings. This test should only flag inline ESLint directive comments.
Suggested fix
- const directiveRe = /eslint-(?:disable|enable)(?:-next-line|-line)?\b/;
+ const directiveRe =
+ /(?:\/\/|\/\*)\s*eslint-(?:disable|enable)(?:-next-line|-line)?\b/;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const directiveRe = /eslint-(?:disable|enable)(?:-next-line|-line)?\b/; | |
| const offenders = []; | |
| for (const file of listTsFiles(settingsSrcDir)) { | |
| const lines = readFileSync(file, 'utf8').split('\n'); | |
| lines.forEach((line, idx) => { | |
| if (directiveRe.test(line)) { | |
| offenders.push(file.replace(repoRoot + '/', '') + ':' + (idx + 1)); | |
| const directiveRe = | |
| /(?:\/\/|\/\*)\s*eslint-(?:disable|enable)(?:-next-line|-line)?\b/; | |
| const offenders = []; | |
| for (const file of listTsFiles(settingsSrcDir)) { | |
| const lines = readFileSync(file, 'utf8').split('\n'); | |
| lines.forEach((line, idx) => { | |
| if (directiveRe.test(line)) { | |
| offenders.push(file.replace(repoRoot + '/', '') + ':' + (idx + 1)); |
🤖 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/tests/eslint-guard.test.js` around lines 237 - 243, The ESLint guard
test is flagging any line containing the directive text, including strings,
instead of only actual comment directives. Update the directive matching logic
in eslint-guard.test.js around directiveRe and the offenders scan so it only
matches inline ESLint comment syntax, and keep using
listTsFiles/readFileSync/lines.forEach to locate the affected lines. Make the
check ignore non-comment occurrences while still detecting eslint-disable and
eslint-enable directives in comments.
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. |
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.
TLDR
Eliminates all inline ESLint disable and enable directives from packages/settings/src and locks the settings package into the completed directive-cleanup guard scope. Adds behavioral tests around malformed profile loading and guard coverage so settings cannot regress to inline lint suppressions.
Dive Deeper
This PR removes the remaining settings-module ESLint directives by refactoring the affected code rather than moving suppressions elsewhere.
Key changes:
Reviewer Test Plan
Suggested validation:
Testing Matrix
Verification run on macOS:
Linked issues / bugs
Fixes #2120