Conversation
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR removes inline ESLint disable directives from Changespackages/core ESLint directive elimination
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
LLxprt PR Review – PR #2146Issue AlignmentIssue #2115 requires eliminating every inline ESLint disable/enable directive from
Side Effects
Code QualityHigh. The refactored code is consistently cleaner than what it replaced:
Tests and CoverageCoverage impact: increase. The new guard script (
Test files (e.g., VerdictReady. The PR fully addresses Issue #2115: |
![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: 35
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/core/src/tools-adapters/coreSubagentServiceHelpers.ts (1)
230-255: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winKeep
emitted_varsnull-safe when formatting results.Line 230 will throw on
Object.entries(output.emitted_vars)when a real subagent output omitsemitted_vars. Keep the previous empty-object fallback here and in the JSON payload so successful outputs without emitted variables still format cleanly.🐛 Proposed fix
- const emittedVars = Object.entries(output.emitted_vars); + const emittedVarsObject = output.emitted_vars ?? {}; + const emittedVars = Object.entries(emittedVarsObject); @@ - emitted_vars: output.emitted_vars, + emitted_vars: output.emitted_vars ?? {},🤖 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/core/src/tools-adapters/coreSubagentServiceHelpers.ts` around lines 230 - 255, The code at line 230 calls Object.entries(output.emitted_vars) without null-safety, which will throw if output.emitted_vars is undefined or null from the subagent output. Add a null-safe fallback by using Object.entries(output.emitted_vars || {}) to handle cases where emitted_vars is missing. Apply the same empty-object fallback to the emitted_vars property in the payload object within the formatSuccessContent function to ensure consistent handling across both the formatted message and the JSON payload.packages/core/src/runtime/AgentRuntimeLoader.ts (1)
214-230: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReintroduce explicit
profileboundary validation before dereferencing.Without the guard, malformed invocations fail later with an unhelpful TypeError instead of a deterministic boundary error.
Suggested fix
export async function loadAgentRuntime( options: AgentRuntimeLoaderOptions, ): Promise<AgentRuntimeLoaderResult> { const { profile, overrides = {}, signal } = options; + if (profile == null) { + throw new Error( + 'AgentRuntimeLoader requires a profile snapshot. Supply options.profile.', + ); + } if (signal?.aborted === true) {🤖 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/core/src/runtime/AgentRuntimeLoader.ts` around lines 214 - 230, The loadAgentRuntime function dereferences the profile parameter (accessing profile.providerManager and profile.config) without first validating that profile is defined and valid. Add explicit boundary validation for the profile parameter at the beginning of the function, similar to the existing signal?.aborted check, to throw a clear and deterministic error if profile is null, undefined, or invalid, rather than allowing a confusing TypeError to occur later when accessing its properties.packages/core/src/utils/googleErrors.ts (2)
282-299: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
nullbefore using theinoperator on parsed response data.At Line 297,
typeof data === 'object'is true fornull, so'error' in datacan throw. This turns malformed API payloads into runtime exceptions.Suggested fix
- if (typeof data === 'object' && 'error' in data) { + if (data !== null && typeof data === 'object' && 'error' in data) { outerError = (data as { error?: ErrorShape }).error; }🤖 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/core/src/utils/googleErrors.ts` around lines 282 - 299, The code at line 297 checks typeof data === 'object' before using the 'in' operator, but this check alone is insufficient because typeof null also returns 'object' in JavaScript, which will cause a runtime error when attempting 'error' in null. Add an additional guard to check that data is not null in the condition before using the 'in' operator to safely handle malformed API payloads.
325-342: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
fromApiErrornow admitsnullmessage and can crash on'error' in data.At Line 325,
nullpasses the new guard (!== undefined && !== ''), and then Line 341 can evaluate'error' in datawheredataisnull, throwing at runtime.Suggested fix
- if (apiError.message !== undefined && apiError.message !== '') { + if ( + apiError.message !== undefined && + apiError.message !== null && + apiError.message !== '' + ) { let data = apiError.message; @@ - if (typeof data === 'object' && 'error' in data) { + if (data !== null && typeof data === 'object' && 'error' in data) { outerError = (data as { error?: ErrorShape }).error; } }🤖 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/core/src/utils/googleErrors.ts` around lines 325 - 342, The guard condition at line 325 in the fromApiError function checks that apiError.message is not undefined and not an empty string, but this allows null to pass through. When null is passed, the code at line 341 attempts to use the in operator on data which could be null, causing a runtime error. Update the guard condition to also check that apiError.message is not null, ensuring null values are rejected along with undefined and empty strings.
🤖 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/core/src/code_assist/setup.ts`:
- Around line 43-46: The nullish coalescing operator ?? only treats null and
undefined as falsy, so an empty string from GOOGLE_CLOUD_PROJECT will be kept as
a valid value and prevent fallback to GOOGLE_CLOUD_PROJECT_ID. Modify the
projectId assignment to check that GOOGLE_CLOUD_PROJECT is not only defined but
also not an empty string before using it. Consider adding a truthiness check
(like trim() or logical OR) to ensure empty strings are treated as invalid and
allow the fallback to GOOGLE_CLOUD_PROJECT_ID to work properly.
In `@packages/core/src/config/lspIntegration.ts`:
- Around line 102-105: The normalizeLspConfig function only checks if servers is
undefined but does not validate that servers is actually an array when it is
defined. This allows invalid non-array values like null or strings to pass
through and cause runtime errors during array operations. Update the condition
in the ternary operator to check not only if servers is undefined but also if it
is not an array using Array.isArray, ensuring that both undefined and non-array
values are normalized to an empty array in the returned normalized LspConfig
object.
In `@packages/core/src/core/contentGenerator.ts`:
- Around line 86-89: The nullish coalescing operator `??` used in the
googleCloudProject variable assignment only falls back when the left operand is
null or undefined, not when it is an empty string. If GOOGLE_CLOUD_PROJECT is
set to an empty string, it will block the fallback to GOOGLE_CLOUD_PROJECT_ID.
Replace the `??` operators with `||` (logical OR) operators so that empty
strings are also treated as falsy and allow the fallback chain to continue to
the next environment variable option.
In `@packages/core/src/filters/EmojiFilter.ts`:
- Around line 72-73: The variation selectors (U+FE0E/U+FE0F) and zero-width
joiner (U+200D) code point ranges on lines 72-73 should not be included in the
base emoji ranges as they are modifiers, not standalone emojis. Remove these two
ranges from the base emoji detection array in the relevant section, and ensure
they are instead handled as part of emoji modifier/sequence detection logic
elsewhere in the code (also check lines 92-111 as mentioned in the comment for
similar occurrences that need the same treatment). This prevents detectEmojis()
from incorrectly flagging non-emoji text that uses these format characters.
In `@packages/core/src/hooks/__tests__/hookEventHandler-messagebus.test.ts`:
- Around line 45-64: The isValidUuid function validates only the 8-4-4-4-12 hex
format but does not enforce actual UUID v4 semantic constraints. Add validation
to check that the version nibble (first character of the third group, groups[2])
is exactly '4', and the variant nibble (first character of the fourth group,
groups[3]) is one of the characters '8', '9', 'a', 'b', 'A', or 'B'. Insert
these checks after validating the group lengths and hex characters to ensure the
function enforces true UUID v4 compliance as stated in its JSDoc comment.
In `@packages/core/src/hooks/hookRegistry.ts`:
- Around line 118-125: The getHookName method directly accesses the .length
property on entry.config.command without first checking if command exists or is
a string, which causes a runtime crash when plugin-type hooks are registered
without a command property. Add a guard condition to verify that
entry.config.command exists and is a string before accessing its .length
property, and return 'unknown-command' as the fallback when command is missing,
empty, or not a string.
In `@packages/core/src/hooks/types.ts`:
- Around line 313-314: The code uses the `in` operator to check for key presence
in hook payloads (checking for 'llm_response', 'llm_request', 'toolConfig'), but
this only confirms the key exists, not that the value is a non-null object. Add
nullish and type-shape guards in addition to the key presence checks at all
affected locations (around lines 313-314, 329-337, 364-375, and 393) to ensure
values are non-null and are objects before passing them to translator methods
like fromHookLLMResponse, fromHookLLMRequest, and accessing properties like
null.candidates. Replace simple key existence checks with checks that verify
both the key exists AND the value is a truthy object.
In `@packages/core/src/parsers/text-tool-call-arg-parsing.ts`:
- Around line 112-119: The current logic checks for XML patterns before
attempting JSON parsing, which causes valid JSON strings containing angle
brackets (like {"query":"<div>weather</div>"}) to be incorrectly treated as XML.
Reverse the order of operations in the conditional block: first attempt to parse
args as JSON using JSON.parse(args) and apply applyToolSpecificNormalizations if
successful, then only if JSON parsing fails (wrap in try-catch), fall back to
checking for XML patterns with the includes checks for `<parameter` and angle
brackets, and call parseXMLParameters as the fallback option.
In `@packages/core/src/parsers/TextToolCallParser.ts`:
- Around line 584-590: The issue is that the generic `<use>` parser is matching
`<use_foo>` tags because it only searches for the `<use` prefix without checking
what follows it. After the indexOf call finds the prefix at line 585, add a
validation check to ensure the character immediately following the matched
prefix is either a closing angle bracket or whitespace. If it's an underscore or
other character, skip that match and continue searching from the next position.
This prevents tryParseUseRequest from processing underscore-specific variants
like `<use_foo>` that should be handled by a more specific parser.
- Around line 751-758: The word boundary check for the 'for' separator in the
condition at line 751 is insufficient because it only verifies that the content
starts with 'for' but does not ensure that 'for' is a complete word token. Add
an additional check after verifying content.startsWith('for', afterName) to
ensure that the character immediately following 'for' (at position afterName +
'for'.length) is either whitespace using isWhitespaceChar() or is at the end of
content (content.length). If this word boundary condition is not met, return a
no-match result similar to the existing error case.
In `@packages/core/src/policy/utils.test.ts`:
- Around line 10-15: The buildDirPathTestRegExp function is a lint-evasion
helper that doesn't actually prevent the static analysis warning from triggering
on the new RegExp constructor inside it. Remove this helper function and instead
address the lint rule directly at the source by either adding an explicit lint
suppression comment (like eslint-disable-next-line) immediately before the new
RegExp call within the test, or refactor to use a static string literal pattern
instead of dynamically building the regex with parts.join().
In `@packages/core/src/prompt-config/prompt-installer.test.ts`:
- Around line 26-27: The backup path validator currently uses a minimum length
check (suffix.length < 15) which allows malformed timestamps with extra
characters to pass validation. Change this condition to enforce an exact
timestamp length by checking if the suffix length equals exactly 15 (the
expected length of YYYYMMDD_HHMMSS format). Apply the same fix to the other
validation checks mentioned in lines 29-37 and 638-639 to ensure consistency
across all backup path validators.
In `@packages/core/src/recording/ReplayEngine.ts`:
- Around line 264-273: The code currently casts the severity variable to
SessionEventPayload['severity'] without validating that it matches one of the
allowed enum values, which could allow invalid severity values like "fatal" to
be persisted. Replace the type check for severity (the typeof severity ===
'string' check) with a validation that explicitly checks if the severity value
is one of the allowed enum values for SessionEventPayload['severity'], ensuring
only valid enum members are accepted before the cast operation in the
sessionEvents push.
- Around line 287-290: The current code at the workspaceDirs assignment casts
the directories array to string[] without validating that each element is
actually a string, allowing non-string values to corrupt the metadata. Before
assigning to acc.metadata.workspaceDirs in the condition where
Array.isArray(directories) is true, add validation to ensure every element in
the directories array is a string type, either by filtering to include only
string entries or by rejecting the entire array if any non-string values are
detected. This prevents malformed values from being written to the metadata.
In `@packages/core/src/recording/SessionDiscovery.ts`:
- Around line 367-370: In the SessionDiscovery.ts file, the filter and map chain
that extracts text from blocks is using a type assertion (as string) which can
coerce invalid payloads. Add an additional type guard to the existing filter
condition to ensure block.text is actually a string type before mapping.
Specifically, extend the filter to check typeof block.text === 'string' in
addition to the existing block.type === 'text' check, and remove the as string
type assertion from the map operation since the type will now be guaranteed by
the filter.
In `@packages/core/src/runtime/AgentRuntimeState.ts`:
- Around line 234-235: The nullish coalescing operator `??` in the session ID
assignment only checks for null and undefined, allowing empty strings to bypass
the ID generation logic. Replace the `??` operator with the logical OR operator
`||` to ensure that both null, undefined, and empty string values for
params.sessionId trigger generation of a new session identifier using the
Date.now() and Math.random() pattern, preventing weak or invalid session
identities from being persisted.
In `@packages/core/src/runtime/contracts/boundary-guards.test.ts`:
- Around line 89-103: The checkLineForProviderImport function does not account
for multi-line block comments, causing it to incorrectly flag provider imports
that appear inside `/* ... */` comment blocks as real imports. Modify the
function to detect and skip lines that are inside multi-line block comments by
checking for `/*` and `*/` markers on the current line, or refactor the calling
code to track block-comment state across multiple lines and pass that state
information to checkLineForProviderImport so it can properly ignore content
within comment blocks.
- Around line 285-287: The empty catch block at line 285-287 is silently
swallowing all errors when parsing tsconfig.json, which hides real test failures
and regressions. Instead of the empty catch block, log the error being caught so
that any unexpected parsing failures are visible during test execution. This
will help surface real provider-reference regressions instead of allowing the
test to silently pass when the tsconfig cannot be parsed. Consider using
console.warn or a similar logging mechanism to record what error occurred while
still allowing the test to continue gracefully for expected cases like TS
extends format issues.
In `@packages/core/src/services/loopDetectionService.ts`:
- Around line 84-97: The markdown marker detection in the
isMarkdownListOrHeading function is checking for literal space characters when
validating list markers and headings, but it should accept any whitespace
character including tabs. Replace the literal space comparisons (first === ' '
for unordered lists and line[i + 1] === ' ' for ordered lists) with a whitespace
check that recognizes tabs and other whitespace characters. Apply this same
whitespace check to all similar marker validation patterns throughout the
function to ensure markdown syntax with tabs is properly detected.
- Around line 293-300: The range check `char >= '+' && char <= '_'` in the
isDivider logic is too broad and incorrectly includes digits (0-9), uppercase
letters (A-Z), and other punctuation characters, causing false positives where
chunks like "ERROR" or "12345" are treated as dividers. Replace the range-based
condition with explicit character checks that only allow the specific divider
characters intended (such as plus, minus, equals, dot, and underscore) rather
than the entire ASCII range between these boundaries.
In `@packages/core/src/skills/skillManager.ts`:
- Around line 229-232: In the skillManager.ts file where the SkillDefinition
object is being returned with the skillName, description, and location
properties, the description field assignment using config.description ?? '' only
handles null/undefined cases but allows non-string values like false, 0, or
objects to pass through, violating the string type contract. Replace the
description assignment to explicitly check whether config.description is a
string using typeof or an instanceof check, and only use it if valid; otherwise
default to an empty string to ensure SkillDefinition.description always receives
a string value.
- Around line 47-55: The frontmatter close marker detection in the content
parsing logic is not validating that the `---` delimiter is on its own line.
After finding the closeMarker position using indexOf with FRONTMATTER_CLOSE, you
need to verify that what immediately follows the `---` marker is either a
newline character or the end of the content string to ensure it's a standalone
delimiter and not part of text like `---foo`. Add a validation check after the
closeMarker assignment to confirm the marker is truly standalone before
proceeding with extracting the body content.
In `@packages/core/src/tools-adapters/CoreSubagentServiceAdapter.ts`:
- Around line 299-300: The method at line 300 needs to check if loadSubagent
returns a valid result before passing it to toToolsSubagentConfig. Currently,
when loadSubagent returns undefined for a missing subagent, passing it directly
to toToolsSubagentConfig will cause the function to throw an error when
dereferencing the config object. Add a guard condition to check if loaded is
defined or not. If it is undefined, return undefined to maintain the promised
return type of ToolsSubagentConfig | undefined. Only call toToolsSubagentConfig
with loaded if the loaded variable contains a valid value.
In `@packages/core/src/utils/errorReporting.ts`:
- Around line 12-35: In the reportToStderr function, the line that constructs
the output variable by mapping and formatting parts happens before the try-catch
block, so any errors thrown during the formatPart calls or join operation will
escape the error handler and interrupt reporting. Move the entire output
variable declaration (the line with parts.map((part) => formatPart(part)).join('
') + '\n') inside the try block so that formatting errors are also caught and
swallowed along with write failures.
In `@packages/core/src/utils/events.ts`:
- Around line 171-176: The fallback overloads for the on() and off() methods use
never[] as the listener parameter type, which rejects any listener with actual
arguments and prevents valid listeners when using arbitrary string or symbol
events. Replace never[] with unknown[] in both the on() method override (around
lines 171-176) and the off() method override (around lines 207-212) to align
with the base EventEmitter contract and allow listeners with any argument
signatures.
In `@packages/core/src/utils/googleErrors.ts`:
- Around line 303-305: The outer error fallback condition at line 303 in the
googleErrors.ts file only checks for undefined with `outerError === undefined`,
but fails to account for the null case, which can cause crashes when later code
attempts to access properties like `currentError.message` on a null value.
Update the condition in the outer error fallback block to check for both null
and undefined values instead of just undefined, ensuring null values are
properly handled before proceeding with nested property access.
In `@packages/core/src/utils/googleQuotaErrors.ts`:
- Line 199: The code at line 199 in googleQuotaErrors.ts uses optional chaining
for errorInfo but not for the metadata property access, which causes an error
when metadata is undefined. Add optional chaining to the metadata access by
changing errorInfo?.metadata['quota_limit'] to
errorInfo?.metadata?.['quota_limit'] to ensure the code safely handles
partially-populated ErrorInfo payloads where metadata may be missing.
In `@packages/core/src/utils/quotaErrorDetection.ts`:
- Line 90: The return statement in the function is passing a potentially
non-string value to the checkMessage function without proper type validation.
The optional chaining and nullish coalescing operator only protect against null
and undefined, but not other falsy values like 0 or false that could arrive from
untyped response payloads. Add a typeof check to validate that
errorData.error?.message is actually a string before passing it to checkMessage.
If the message is not a string type, fall back to an empty string to prevent
runtime crashes when checkMessage calls methods like .includes() on non-string
values.
- Around line 19-23: The `isApiError` function currently only checks that the
`message` property exists on the inner error object, but does not verify that it
is a string. This causes runtime errors when downstream code (around lines 55
and 105) calls `.includes()` on the message value. Strengthen the type guard in
the return statement of `isApiError` to explicitly check that `inner.message` is
typeof 'string', similar to how `isStructuredError` validates its message field
at lines 30–31.
In `@packages/core/src/utils/schemaValidator.ts`:
- Around line 242-258: The isIsoDate function currently only validates that
months fall between 1-12 and days between 1-31, but does not check actual
calendar validity, allowing invalid dates like 2024-02-31 or 2023-02-29 to pass
validation. After validating the format and digit requirements for year, month,
and day variables using isAllDigits, add a final validation step using the
JavaScript Date object to ensure the parsed date actually exists: construct a
Date object from the year, month (adjusting for zero-based indexing), and day
values, then verify that the resulting date's month, day, and year match the
input values to catch impossible dates and properly handle leap year validation.
- Around line 25-30: The code assumes the `.default` property exists when
extracting Ajv constructors, but the type annotation marks it as optional,
causing instantiation failures at lines 202 and 207 when `.default` is
undefined. For both Ajv2020Class and AjvClass assignments, instead of directly
casting to `.default`, use a fallback pattern (such as logical OR) that checks
if `.default` exists and falls back to using the module itself as the
constructor when it doesn't, ensuring safe resolution in both ESM and CJS
interop scenarios.
In `@packages/core/src/utils/shell-utils.ts`:
- Around line 396-435: The matchShellWrapperPrefix function currently uses
literal space characters to match shell wrapper prefixes, but shell parsing
accepts tabs and newlines as separators too, causing commands with alternative
whitespace to not be properly unwrapped. Replace the literal space matching in
the function with whitespace-aware matching that handles spaces, tabs, and
newlines. This should be applied to all shell prefix checks including the shell
wrappers loop, the cmd.exe section, and the PowerShell wrappers section - modify
the string comparisons that check for patterns like shell + ' ', cmd.exe + ' ',
-c ' ', /c ' ', -noprofile ' ', and -command ' ' to use cursor-based whitespace
navigation or regex that accepts any whitespace character.
In `@packages/core/src/utils/systemEncoding.ts`:
- Around line 93-103: The encoding extraction logic in systemEncoding.ts does
not remove locale modifiers like `@euro` from the encoding string, resulting in
invalid encoding labels such as utf-8@euro. Modify the code to strip any @
character and everything after it from the encoding variable before returning
it. This normalization should be applied in both the explicit encoding path
(where encoding is extracted from the dotIndex) and the fallback path (where
locale itself is returned as encoding), ensuring that locale modifiers are
removed from the final encoding token before it is converted to lowercase and
returned.
In `@scripts/check-eslint-guard.js`:
- Around line 120-122: The isStandaloneOffRuleValue function's regex pattern
does not account for inline comments following the off value. The current
pattern `/^\s*(?:['"]off['"]|0),?\s*$/` requires the line to end after optional
whitespace, but it should allow for trailing comments like `'off', // comment`.
Modify the regex to optionally match comments after the off value and comma by
updating the end of the pattern to allow for optional comment syntax before the
line terminator.
In `@scripts/tests/eslint-guard.test.js`:
- Around line 238-257: The test `flags packages/core multiline rule-off values`
currently covers multiline rule-off values but lacks regression coverage for
inline-commented variants. Add two additional test cases within the config
string to cover scenarios where `off` and `0` values have inline comments (like
`'off', // comment` and `0, // comment`), ensuring the
`checkCoreCentralBypassesInConfig` function properly detects these patterns as
violations and update the expected violation count accordingly.
---
Outside diff comments:
In `@packages/core/src/runtime/AgentRuntimeLoader.ts`:
- Around line 214-230: The loadAgentRuntime function dereferences the profile
parameter (accessing profile.providerManager and profile.config) without first
validating that profile is defined and valid. Add explicit boundary validation
for the profile parameter at the beginning of the function, similar to the
existing signal?.aborted check, to throw a clear and deterministic error if
profile is null, undefined, or invalid, rather than allowing a confusing
TypeError to occur later when accessing its properties.
In `@packages/core/src/tools-adapters/coreSubagentServiceHelpers.ts`:
- Around line 230-255: The code at line 230 calls
Object.entries(output.emitted_vars) without null-safety, which will throw if
output.emitted_vars is undefined or null from the subagent output. Add a
null-safe fallback by using Object.entries(output.emitted_vars || {}) to handle
cases where emitted_vars is missing. Apply the same empty-object fallback to the
emitted_vars property in the payload object within the formatSuccessContent
function to ensure consistent handling across both the formatted message and the
JSON payload.
In `@packages/core/src/utils/googleErrors.ts`:
- Around line 282-299: The code at line 297 checks typeof data === 'object'
before using the 'in' operator, but this check alone is insufficient because
typeof null also returns 'object' in JavaScript, which will cause a runtime
error when attempting 'error' in null. Add an additional guard to check that
data is not null in the condition before using the 'in' operator to safely
handle malformed API payloads.
- Around line 325-342: The guard condition at line 325 in the fromApiError
function checks that apiError.message is not undefined and not an empty string,
but this allows null to pass through. When null is passed, the code at line 341
attempts to use the in operator on data which could be null, causing a runtime
error. Update the guard condition to also check that apiError.message is not
null, ensuring null values are rejected along with undefined and empty strings.
🪄 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: 0151d5c0-061f-4c37-af4c-9d19438631df
📒 Files selected for processing (131)
eslint.config.jspackages/core/src/code_assist/oauth-credential-storage.tspackages/core/src/code_assist/setup.tspackages/core/src/config/config-lsp-integration.test.tspackages/core/src/config/configBaseCore.tspackages/core/src/config/endpoints.tspackages/core/src/config/lspIntegration.tspackages/core/src/config/subagentManager.tspackages/core/src/config/toolRegistryFactory.tspackages/core/src/core/contentGenerator.tspackages/core/src/core/coreToolHookTriggers.tspackages/core/src/core/lifecycleHookTriggers.tspackages/core/src/core/logger.tspackages/core/src/core/subagentTypes.test.tspackages/core/src/core/subagentTypes.tspackages/core/src/debug/DebugLogger.test.tspackages/core/src/debug/FileOutput.test.tspackages/core/src/filters/EmojiFilter.tspackages/core/src/hooks/__tests__/hookEventHandler-messagebus.test.tspackages/core/src/hooks/__tests__/hookSemantics.test.tspackages/core/src/hooks/__tests__/hookSystem-lifecycle.test.tspackages/core/src/hooks/__tests__/hookValidators.test.tspackages/core/src/hooks/hookEventHandler.tspackages/core/src/hooks/hookRegistry.tspackages/core/src/hooks/hookRunner.test.tspackages/core/src/hooks/types.tspackages/core/src/integration/compression-duplicate-ids.test.tspackages/core/src/models/hydration.tspackages/core/src/models/registry.tspackages/core/src/parsers/TextToolCallParser.test.tspackages/core/src/parsers/TextToolCallParser.tspackages/core/src/parsers/text-tool-call-arg-parsing.tspackages/core/src/parsers/text-tool-call-helpers.tspackages/core/src/parsers/tool-call-parser-utils.tspackages/core/src/policy/utils.test.tspackages/core/src/prompt-config/TemplateEngine.tspackages/core/src/prompt-config/prompt-cache.tspackages/core/src/prompt-config/prompt-installer.test.tspackages/core/src/prompt-config/prompt-loader.test.tspackages/core/src/prompt-config/prompt-service.tspackages/core/src/recording/ReplayEngine.tspackages/core/src/recording/SessionDiscovery.test.tspackages/core/src/recording/SessionDiscovery.tspackages/core/src/recording/SessionLockManager.test.tspackages/core/src/recording/SessionLockManager.tspackages/core/src/recording/SessionRecordingService.test.tspackages/core/src/recording/__tests__/SessionDiscovery.extensions.spec.tspackages/core/src/recording/resumeSession.test.tspackages/core/src/recording/sessionCleanupUtils.test.tspackages/core/src/recording/sessionManagement.test.tspackages/core/src/runtime/AgentRuntimeLoader.tspackages/core/src/runtime/AgentRuntimeState.tspackages/core/src/runtime/RuntimeInvocationContext.tspackages/core/src/runtime/contracts/boundary-guards.test.tspackages/core/src/runtime/createAgentRuntimeContext.tspackages/core/src/runtime/errors/MissingRuntimeProviderError.test.tspackages/core/src/services/environmentSanitization.tspackages/core/src/services/gitService.tspackages/core/src/services/history/__tests__/density-history.test.tspackages/core/src/services/history/canonicalToolIds.tspackages/core/src/services/loopDetectionService.tspackages/core/src/skills/skillLoader.tspackages/core/src/skills/skillManager.tspackages/core/src/storage/SessionPersistenceService.tspackages/core/src/telemetry/loggers.test.circular.tspackages/core/src/test-utils/runtime.tspackages/core/src/test-utils/tools.tspackages/core/src/todo/todoFormatter.tspackages/core/src/tools-adapters/CoreMessageBusAdapter.tspackages/core/src/tools-adapters/CoreShellToolHostAdapter.tspackages/core/src/tools-adapters/CoreSubagentServiceAdapter.tspackages/core/src/tools-adapters/coreSubagentServiceHelpers.tspackages/core/src/tools/tool-key-storage.test.tspackages/core/src/utils/__tests__/resolveTextSearchTarget.test.tspackages/core/src/utils/ast-grep-utils.tspackages/core/src/utils/asyncIterator.test.tspackages/core/src/utils/bfsFileSearch.test.tspackages/core/src/utils/checkpointUtils.test.tspackages/core/src/utils/checkpointUtils.tspackages/core/src/utils/editor.tspackages/core/src/utils/errorParsing.tspackages/core/src/utils/errorReporting.test.tspackages/core/src/utils/errorReporting.tspackages/core/src/utils/events.tspackages/core/src/utils/fileDiffUtils.tspackages/core/src/utils/fileUtils.test.tspackages/core/src/utils/fileUtils.tspackages/core/src/utils/filesearch/crawler.tspackages/core/src/utils/filesearch/fileSearch.tspackages/core/src/utils/generateContentResponseUtilities.tspackages/core/src/utils/getFolderStructure.tspackages/core/src/utils/getPty.tspackages/core/src/utils/gitIgnoreParser.tspackages/core/src/utils/gitLineChanges.tspackages/core/src/utils/googleErrors.tspackages/core/src/utils/googleQuotaErrors.test.tspackages/core/src/utils/googleQuotaErrors.tspackages/core/src/utils/ignorePatterns.tspackages/core/src/utils/memoryDiscovery.test.tspackages/core/src/utils/memoryDiscovery.tspackages/core/src/utils/memoryImportProcessor.test.tspackages/core/src/utils/memoryImportProcessor.tspackages/core/src/utils/parameterCoercion.tspackages/core/src/utils/partUtils.tspackages/core/src/utils/paths.tspackages/core/src/utils/quotaErrorDetection.tspackages/core/src/utils/retry.quota.test.tspackages/core/src/utils/retry.test.tspackages/core/src/utils/retry.tspackages/core/src/utils/safeJsonStringify.test.tspackages/core/src/utils/sanitization.tspackages/core/src/utils/schemaValidator.tspackages/core/src/utils/secure-browser-launcher.tspackages/core/src/utils/shell-parser.test.tspackages/core/src/utils/shell-parser.tspackages/core/src/utils/shell-utils.shellReplacement.test.tspackages/core/src/utils/shell-utils.test.tspackages/core/src/utils/shell-utils.tspackages/core/src/utils/shellPathCompletion.tspackages/core/src/utils/stdio.test.tspackages/core/src/utils/stdio.tspackages/core/src/utils/streamIdleTimeout.tspackages/core/src/utils/summarizer.tspackages/core/src/utils/systemEncoding.tspackages/core/src/utils/terminalSerializer.test.tspackages/core/src/utils/terminalSerializer.tspackages/core/src/utils/tool-utils.tspackages/core/src/utils/userAccountManager.tspackages/core/test/models/schema.test.tsscripts/check-eslint-guard.jsscripts/tests/eslint-guard.test.js
💤 Files with no reviewable changes (6)
- packages/core/src/integration/compression-duplicate-ids.test.ts
- packages/core/src/services/gitService.ts
- packages/core/src/runtime/RuntimeInvocationContext.ts
- packages/core/src/recording/SessionRecordingService.test.ts
- packages/core/src/hooks/tests/hookSemantics.test.ts
- packages/core/src/hooks/tests/hookValidators.test.ts
# Conflicts: # eslint.config.js # packages/core/src/config/config-lsp-integration.test.ts # packages/core/src/config/endpoints.ts # packages/core/src/core/subagentTypes.ts # packages/core/src/debug/DebugLogger.test.ts # packages/core/src/debug/FileOutput.test.ts # packages/core/src/filters/EmojiFilter.ts # packages/core/src/hooks/__tests__/hookEventHandler-messagebus.test.ts # packages/core/src/hooks/hookRunner.test.ts # packages/core/src/parsers/TextToolCallParser.test.ts # packages/core/src/parsers/TextToolCallParser.ts # packages/core/src/parsers/tool-call-parser-utils.ts # packages/core/src/policy/utils.test.ts # packages/core/src/prompt-config/prompt-installer.test.ts # packages/core/src/runtime/contracts/boundary-guards.test.ts # packages/core/src/services/loopDetectionService.ts # packages/core/src/skills/skillLoader.ts # packages/core/src/skills/skillManager.ts # packages/core/src/tools/tool-key-storage.test.ts # packages/core/src/utils/gitLineChanges.ts # packages/core/src/utils/memoryImportProcessor.test.ts # packages/core/src/utils/parameterCoercion.ts # packages/core/src/utils/paths.ts # packages/core/src/utils/schemaValidator.ts # packages/core/src/utils/secure-browser-launcher.ts # packages/core/src/utils/shell-utils.ts # packages/core/src/utils/systemEncoding.ts # scripts/tests/eslint-guard.test.js
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/core/src/config/toolRegistryFactory.ts (1)
526-562: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
createToolRegistry()still requires a fullConfig, notConfigBaseCore.The new signature accepts any
ConfigBaseCore, but Lines 531-562 immediately downcast it toConfigand pass it into adapters that rely on the broaderConfigAPI. That makes the exported type unsound: callers can now compile againstConfigBaseCoreand still fail at runtime when thoseConfig-only methods are missing. Either restore the parameter type toConfigor introduce a dedicated boundary interface that exactly matches what these adapters consume.🤖 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/core/src/config/toolRegistryFactory.ts` around lines 526 - 562, createToolRegistry() is typed too broadly with ConfigBaseCore even though it immediately casts to Config and passes that value into CoreToolRegistryHostAdapter, registerStandardTools, and registerAgentTools, all of which depend on Config-only APIs. Tighten the function boundary by changing createToolRegistry and its related call sites/helpers to accept Config, or define a narrower interface that exactly covers the properties/methods these adapters actually use, so the exported signature matches runtime expectations.packages/core/src/tools-adapters/CoreSubagentServiceAdapter.ts (1)
785-803: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep missing-config async calls on the structured error path.
checkAsyncSettings()now throws beforeexecuteAsyncSubagent()reaches anytry/catch. Because this adapter still has a constructor path with noconfig, an async call can now escape as an uncaught exception instead of returning the normal execution error result.💡 Suggested fix
private checkAsyncSettings(): SubagentResult | undefined { - const settingsService = this.requireConfig().getSettingsService(); - const globalSettings = settingsService.getAllGlobalSettings(); + const settingsService = this.config?.getSettingsService(); + const globalSettings = settingsService?.getAllGlobalSettings() ?? {}; const subagentsSettings = globalSettings['subagents'] as | { asyncEnabled?: boolean } | undefined; @@ - const ephemeralSettings = this.requireConfig().getEphemeralSettings(); + const ephemeralSettings = this.config?.getEphemeralSettings() ?? {}; if (ephemeralSettings['subagents.async.enabled'] === false) { return {🤖 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/core/src/tools-adapters/CoreSubagentServiceAdapter.ts` around lines 785 - 803, The async settings guard in CoreSubagentServiceAdapter is throwing before executeAsyncSubagent() can convert missing-config cases into the standard structured tool error response. Update checkAsyncSettings() and its call site so the no-config path is handled without throwing, and ensure executeAsyncSubagent() still returns the normal error object with ToolErrorType.EXECUTION_FAILED when config is missing. Use the existing symbols checkAsyncSettings, executeAsyncSubagent, requireConfig, and ToolErrorType to keep the async call on the structured error path.packages/core/src/skills/skillLoader.ts (1)
283-306: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep sync skill loading on the same parser path.
The async loader uses
parseFrontmatter()with fallback parsing, but the sync loader callsyaml.load()directly. Directory discovery can therefore reject skills thatloadSkillFromFile()accepts.Proposed fix
- const frontmatter = yaml.load(parts[0]); - if ( - frontmatter === null || - frontmatter === undefined || - typeof frontmatter !== 'object' - ) { - return null; - } - - const { name, description } = frontmatter as Record<string, unknown>; - if (typeof name !== 'string' || typeof description !== 'string') { + const frontmatter = parseFrontmatter(parts[0]); + if (!frontmatter) { return null; } return { - name, - description, + name: frontmatter.name, + description: frontmatter.description, location: filePath, body: parts[1].trim(), source,🤖 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/core/src/skills/skillLoader.ts` around lines 283 - 306, The sync skill loading path in skillLoader should use the same frontmatter parsing logic as the async path, since calling yaml.load() directly makes directory discovery reject skills that loadSkillFromFile() would accept. Update the frontmatter handling in the skill-loading flow to reuse parseFrontmatter() (and its fallback parsing behavior) or otherwise route both paths through the same parser, preserving the existing name/description validation and using the same symbols like extractFrontmatter, parseFrontmatter, and loadSkillFromFile for consistency.
♻️ Duplicate comments (5)
packages/core/src/code_assist/setup.ts (1)
43-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWhitespace-only
GOOGLE_CLOUD_PROJECTstill suppresses the fallback env var.Line 46 only converts
''toundefined. IfGOOGLE_CLOUD_PROJECT=' ', the??chain stops there andGOOGLE_CLOUD_PROJECT_IDis ignored, so an invalid blank project ID is passed downstream. Normalize each env var before the fallback chain.Suggested fix
+function normalizeProjectEnv(value: string | undefined): string | undefined { + return value == null || value.trim() === '' ? undefined : value; +} + - const rawProjectId = - process.env['GOOGLE_CLOUD_PROJECT'] ?? - process.env['GOOGLE_CLOUD_PROJECT_ID']; - const projectId = rawProjectId === '' ? undefined : rawProjectId; + const projectId = + normalizeProjectEnv(process.env['GOOGLE_CLOUD_PROJECT']) ?? + normalizeProjectEnv(process.env['GOOGLE_CLOUD_PROJECT_ID']);🤖 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/core/src/code_assist/setup.ts` around lines 43 - 46, The project ID selection in setup.ts still treats whitespace-only GOOGLE_CLOUD_PROJECT as a valid value, preventing fallback to GOOGLE_CLOUD_PROJECT_ID. Update the logic around rawProjectId/projectId to trim and normalize each environment value before choosing the fallback so blank or whitespace-only strings become undefined. Use the setup() projectId assignment and the env lookups for GOOGLE_CLOUD_PROJECT and GOOGLE_CLOUD_PROJECT_ID as the places to adjust.packages/core/src/runtime/AgentRuntimeState.ts (1)
236-238: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWhitespace-only
sessionIdvalues still bypass regeneration.This only rejects
'', so values like' 'are persisted as real session identifiers. That reopens the invalid-ID case the earlier normalization was meant to close.Suggested fix
- const sessionId = - params.sessionId !== undefined && params.sessionId !== '' - ? params.sessionId - : `session-${randomUUID()}`; + const normalizedSessionId = + typeof params.sessionId === 'string' && params.sessionId.trim() !== '' + ? params.sessionId + : undefined; + const sessionId = normalizedSessionId ?? `session-${randomUUID()}`;🤖 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/core/src/runtime/AgentRuntimeState.ts` around lines 236 - 238, The session ID normalization in AgentRuntimeState should treat whitespace-only values as invalid instead of only checking for an empty string. Update the logic around params.sessionId so the generated `session-${randomUUID()}` fallback is used when the provided value is undefined, empty, or only whitespace, preserving the intended validation behavior.packages/core/src/core/contentGenerator.ts (1)
86-90: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBlank
GOOGLE_CLOUD_PROJECTstill blocks the ID fallback.
??runs before the empty-string normalization, soGOOGLE_CLOUD_PROJECT=''preventsGOOGLE_CLOUD_PROJECT_IDfrom ever being consulted. That regresses the earlier non-empty-env fix and can silently disable Vertex config again.Suggested fix
- const rawGoogleCloudProject = - process.env['GOOGLE_CLOUD_PROJECT'] ?? - process.env['GOOGLE_CLOUD_PROJECT_ID']; - const googleCloudProject = - rawGoogleCloudProject === '' ? undefined : rawGoogleCloudProject; + const readNonEmptyEnv = (name: string): string | undefined => { + const value = process.env[name]; + return value === undefined || value === '' ? undefined : value; + }; + const googleCloudProject = + readNonEmptyEnv('GOOGLE_CLOUD_PROJECT') ?? + readNonEmptyEnv('GOOGLE_CLOUD_PROJECT_ID');🤖 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/core/src/core/contentGenerator.ts` around lines 86 - 90, The Vertex project env lookup in contentGenerator is still prioritizing GOOGLE_CLOUD_PROJECT before empty-string normalization, so an empty value can block fallback to GOOGLE_CLOUD_PROJECT_ID. Update the project resolution logic in contentGenerator to treat blank GOOGLE_CLOUD_PROJECT as unset before choosing the fallback, and ensure the resulting googleCloudProject selection still comes from the first non-empty env value.packages/core/src/runtime/contracts/boundary-guards.test.ts (1)
292-301: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
tsconfig.jsonparse errors still let the guard test pass.Writing to
stderrhere preserves the same bypass as the old emptycatch: any unhandledtsconfig.jsonformat causes the providers-reference assertion to be skipped instead of failing the test.Suggested fix
- } catch (parseError) { - // Surface unexpected tsconfig parse failures so real regressions - // are not silently masked. TS extends-format issues still skip. - process.stderr.write( - 'tsconfig.json could not be parsed for provider-reference check: ' + - (parseError instanceof Error - ? parseError.message - : String(parseError)) + - '\n', - ); - } + } catch (parseError) { + throw new Error( + 'tsconfig.json could not be parsed for provider-reference check: ' + + (parseError instanceof Error + ? parseError.message + : String(parseError)), + ); + }🤖 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/core/src/runtime/contracts/boundary-guards.test.ts` around lines 292 - 301, The provider-reference guard in boundary-guards.test.ts is still swallowing tsconfig parse failures, so the test can pass after an unexpected parse error. Update the catch path in the tsconfig parsing logic around the provider-reference check to fail the test explicitly instead of only writing to stderr, and keep the TS extends-format skip behavior separate from real parse errors. Use the existing provider-reference assertion flow in the boundary-guards test to locate the fix.packages/core/src/services/loopDetectionService.ts (1)
139-147: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the markdown whitespace helper for blockquotes too.
Line 143 still checks only a literal space, so
>\tquotedoes not reset content tracking.Proposed fix
- if (line.length >= 2 && line[0] === '>' && line[1] === ' ') { + if ( + line.length >= 2 && + line[0] === '>' && + isMarkdownWhitespace(line[1]) + ) { return true; }🤖 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/core/src/services/loopDetectionService.ts` around lines 139 - 147, The blockquote detector in detectMarkdownBlockquote still hardcodes a literal space after the markdown `>` marker, so it misses valid blockquotes like `>\tquote`. Update the whitespace check to use the same markdown whitespace helper used elsewhere in LoopDetectionService instead of testing only `line[1] === ' '`, and keep the logic inside detectMarkdownBlockquote aligned with the existing markdown parsing behavior.
🤖 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/core/src/code_assist/oauth-credential-storage.ts`:
- Around line 28-31: The credential normalization in normalizeOptionalString and
the saveCredentials path is only filtering nullish/empty values, so malformed
legacy data can still persist non-string values into refreshToken, tokenType, or
scope. Update the boundary in saveCredentials so each credential field is
validated by type before persistence, treating any non-string as absent and
applying the existing Bearer fallback for tokenType, instead of casting with as
string. Keep the fix localized to the credential handling flow used by
migrateFromFileStorage and saveCredentials.
In `@packages/core/src/config/lspIntegration.ts`:
- Around line 115-135: The timeout handling in lspIntegration’s MCP navigation
registration only rejects the waiting Promise and leaves
registerMcpNavigationTools() running, so add an explicit cancellation path that
the registration flow can observe and stop on timeout. Update the Promise.race
around registerMcpNavigationTools() to propagate an abort signal or equivalent
cancellation token into that function, and ensure any partially created
transport/client resources are explicitly closed when the timeout fires instead
of only clearing state.lspMcpClient and state.lspMcpTransport in the catch
block. Keep the fix centered on registerMcpNavigationTools(), the timeout setup,
and the cleanup path in lspIntegration.
In `@packages/core/src/core/logger.ts`:
- Around line 491-497: The checkpoint text filtering path in filterPartText
currently casts EmojiFilter.filterText().filtered to a string, which can
propagate null into Part.text when the filter runs in error mode. Update
filterPartText (and any caller like loadCheckpoint that relies on it) to guard
against a null filtered value and preserve a valid text string or otherwise skip
writing the text field, so checkpoint parts never end up with text: null.
In `@packages/core/src/hooks/hookRegistry.ts`:
- Line 194: The trust warning currently builds the display list in hookRegistry
using untrusted.map on name ?? command, which can still render an empty string
when a hook has name: ''. Update the warning display to use getHookName() for
each untrusted hook instead of directly reading name/command so blank names fall
back correctly to the command. Keep the change localized to the hookRegistry
trust-warning logic and preserve the existing join(', ') formatting.
In `@packages/core/src/parsers/text-tool-call-arg-parsing.ts`:
- Around line 170-171: The normalization logic in
applyToolSpecificNormalizations contains a redundant typeof toolName check even
though the function only accepts a string and all callers pass strings. Simplify
the normalizedTool assignment by removing the dead fallback branch and using the
string value directly so the code no longer triggers
`@typescript-eslint/no-unnecessary-condition`.
In `@packages/core/src/parsers/text-tool-call-helpers.ts`:
- Around line 104-107: Update the truncation logic in text-tool-call-helpers so
it verifies the top-level object is actually closed before returning the
original content; the current checks in the helper around the inner object scan
and the related path at parse-cleanup can incorrectly keep trailing partial
tool-call text. Adjust the logic in the relevant function(s) that process the
objectStart/tagIdx range to distinguish a closing brace for the outer tool-call
object from one inside nested arguments, and only preserve content when the full
top-level object is complete; otherwise trim the trailing fragment from
cleanedContent.
- Around line 239-248: The early tool-call marker scan in TOOL_CALL_MARKERS is
missing the `<use_` prefix, so parse() can return too early and skip valid
underscore-style tool requests before findUseUnderscoreToolRequests() runs.
Update the marker list used by hasAnyToolCallMarker() to recognize `<use_`
alongside `<use `, keeping the existing parsing flow in parse() and the
underscore-specific handler intact.
In `@packages/core/src/parsers/TextToolCallParser.ts`:
- Around line 304-306: The TextToolCallParser logic currently accepts any text
between the JSON payload and the [END_TOOL_REQUEST] marker, which can wrongly
absorb explanatory content into a tool call. Update the parsing in
TextToolCallParser so the end marker is only matched if the characters between
jsonSegment.endIndex and endMarker are whitespace-only, using the existing
toolName/jsonSegment/endMarker flow. Keep the cleanedContent replacement aligned
with the downstream contract so only the tool request and surrounding whitespace
are removed, not adjacent text.
In `@packages/core/src/recording/ReplayEngine.ts`:
- Around line 191-200: The `isSpeakerContent` type guard in `ReplayEngine.ts` is
too permissive and should validate the full `IContent` shape before values are
added to `acc.history`. Update `isSpeakerContent` to require both `speaker` and
`blocks`, and ensure `speaker` is one of the allowed `IContent` speaker values
rather than just truthy. Then make the `acc.history` append path use this
stricter guard so malformed payloads are rejected before being treated as
`IContent[]`.
- Around line 327-333: Guard malformed events in ReplayEngine before calling
dispatchEvent: parsed.payload is currently cast directly to Record<string,
unknown> in the replay flow, which can let missing or non-object payloads crash
downstream handlers. Update the event handling logic around the parsed.type /
parsed.payload path to validate that parsed.payload exists and is a plain object
before dispatching, and skip or safely reject the event when it is invalid so
ReplayEngine does not fail on malformed input.
In `@packages/core/src/recording/SessionDiscovery.ts`:
- Around line 227-234: The skipped/corrupt count in SessionDiscovery is being
incremented for every null summary, but readSessionSummary() also returns null
for project-hash mismatches, so valid sessions from other projects are counted
incorrectly. Update the loop that processes sessionFiles in SessionDiscovery to
distinguish mismatch results from true corruption, and adjust
readSessionSummary() so the projectHash check returns a separate signal or
metadata that the caller can exclude from skippedCount while still counting
actual parse/read failures.
In `@packages/core/src/recording/SessionLockManager.ts`:
- Around line 133-137: The stale-lock reacquire path in SessionLockManager is
masking all write failures as contention by turning every fs.writeFile error
into “Session is in use by another process.” Update the try/catch around
fs.writeFile in the lock acquisition logic so only actual contention-like
failures are mapped to that message, and preserve or rethrow non-contention I/O
errors such as ENOSPC or EACCES with their original details. Use the
SessionLockManager reacquire code path and the fs.writeFile call as the anchor
points when making the change.
In `@packages/core/src/runtime/contracts/boundary-guards.test.ts`:
- Around line 79-109: The provider-boundary scan in
detectProviderImport/checkLineForProviderImport only inspects each line
independently, so multiline imports can evade the guard when the forbidden
module path appears on a continuation line. Update detectProviderImport to
recognize import statements across line breaks by buffering/normalizing the full
import/export statement before matching against PROVIDER_IMPORT_SUBSTRINGS,
while preserving the existing comment filtering in stripCommentsFromLine. Ensure
the fix still uses checkLineForProviderImport for the final match logic but can
detect split import forms reliably.
In `@packages/core/src/skills/skillManager.ts`:
- Around line 53-59: Update stripFrontmatter in skillManager so the closing
frontmatter delimiter check and body parsing accept CRLF as well as LF; the
current standalone-close logic only recognizes NEWLINE and fails on ---\r\n.
Adjust the afterClose validation and the subsequent bodyStart lookup to handle
an optional \r before the newline, keeping the behavior intact for valid
frontmatter while still rejecting inline delimiters.
In `@packages/core/src/utils/checkpointUtils.ts`:
- Around line 179-181: The checkpoint ID derivation in checkpointUtils should
only strip the trailing file extension, because `replace('.json', '')` in the
`toolCallToCheckpointMap.set(...)` path can remove an earlier `.json` substring
from `result.checkpointFileName`. Update the logic in that mapping flow to
derive the key from the filename suffix only, preserving any internal `.json`
text and keeping `toolCall.callId` to checkpoint mapping stable.
In `@packages/core/src/utils/errorReporting.ts`:
- Around line 138-148: logContextFallback currently wraps reportToStderr in
try/catch, but reportToStderr already absorbs its own failures so the fallback
branches are unreachable. Update logContextFallback so it first attempts to
stringify or otherwise prepare the context in the function itself, then call
reportToStderr only once per fallback path; use the existing logContextFallback
and reportToStderr symbols to preserve the intended sequence of original,
truncated JSON, and final “could not be logged” messages.
In `@packages/core/src/utils/paths.ts`:
- Around line 52-58: The deprecated SHELL_SPECIAL_CHARS export has lost the old
RegExp.test semantics because its test() method now only accepts a
single-character input; restore backward-compatible behavior so it reports true
when any shell special character appears anywhere in the provided string. Update
the SHELL_SPECIAL_CHARS.test implementation in paths.ts to mirror the legacy
RegExp-style matching while keeping the existing source getter and deprecated
export intact.
In `@packages/core/src/utils/schemaValidator.ts`:
- Around line 262-268: The date validation in schemaValidator’s year/month/day
check is using local date construction, which remaps years 0000–0099 and breaks
valid inputs like 0001-01-01. Update the logic around the yearNum/constructed
validation to create the Date in UTC and compare using UTC getters instead of
local ones, so the check works correctly for all year values.
In `@packages/core/src/utils/shell-parser.test.ts`:
- Around line 350-357: The empty-input test in shell-parser.test is asserting
the wrong behavior for parseShellCommand, since parseShellCommand('')
intentionally returns null when the input is blank. Update the test case named
"should handle empty input gracefully" to expect a null result from
parseShellCommand for empty strings, and remove the downstream
splitCommandsWithTree/tree assertions that assume a non-null tree; keep the test
aligned with the short-circuit behavior in parseShellCommand from
shell-parser.ts.
- Line 44: The availability gating in shell-parser tests is using
it.skipIf(!parserInitialized), but that value is decided before beforeAll can
refresh it, so the collected tests won’t reflect the current parser state.
Update shell-parser.test.ts to remove the collection-time skip logic and instead
check parserInitialized inside a setup hook or at the start of the affected test
cases, using the existing beforeAll and test blocks so the current parser state
determines whether the tests run.
In `@packages/core/src/utils/shell-utils.test.ts`:
- Around line 50-53: The parser setup in shell-utils.test.ts happens too late
for the `it.skipIf(!isParserAvailable())` checks because those are evaluated
when the test file is loaded, not inside `beforeAll`. Move the
`initializeShellParsers()` call to module initialization before the
`it.skipIf(...)` registrations, or change the parser-dependent cases to check
availability inside each test body so the `isParserAvailable()` guard sees the
initialized state.
In `@scripts/check-eslint-guard.js`:
- Around line 492-557: The core override scan in the main loop misses one-line
objects because `coreObjectDepth` and `rulesObjectDepth` are set after the line
has already been evaluated and `braceDepth` is only updated later. Update the
detection flow around `extractArrayStart`, `isCorePathLine`,
`enclosingObjectDepth`, and the `rules`/`ignores` checks so same-line
`files`/`rules`/`ignores` blocks are analyzed with the correct depth
immediately, and ensure `isNewOffRule` and `isStandaloneOffRuleValue` still
trigger violations for inline core overrides.
- Around line 349-379: The cleanup-scope scanner in
checkCoreDirectiveScopesInConfig currently skips same-line array declarations
because it sets currentScope and immediately continues, so single-line
legacyDirectiveCleanupScopes or completedDirectiveCleanupScopes entries
containing packages/core are missed. Update the parsing logic to also inspect
the remainder of the declaration line after matching the scope assignment, or
otherwise handle inline arrays before continuing, while keeping the existing
scope tracking and violation reporting in checkCoreDirectiveScopesInConfig.
---
Outside diff comments:
In `@packages/core/src/config/toolRegistryFactory.ts`:
- Around line 526-562: createToolRegistry() is typed too broadly with
ConfigBaseCore even though it immediately casts to Config and passes that value
into CoreToolRegistryHostAdapter, registerStandardTools, and registerAgentTools,
all of which depend on Config-only APIs. Tighten the function boundary by
changing createToolRegistry and its related call sites/helpers to accept Config,
or define a narrower interface that exactly covers the properties/methods these
adapters actually use, so the exported signature matches runtime expectations.
In `@packages/core/src/skills/skillLoader.ts`:
- Around line 283-306: The sync skill loading path in skillLoader should use the
same frontmatter parsing logic as the async path, since calling yaml.load()
directly makes directory discovery reject skills that loadSkillFromFile() would
accept. Update the frontmatter handling in the skill-loading flow to reuse
parseFrontmatter() (and its fallback parsing behavior) or otherwise route both
paths through the same parser, preserving the existing name/description
validation and using the same symbols like extractFrontmatter, parseFrontmatter,
and loadSkillFromFile for consistency.
In `@packages/core/src/tools-adapters/CoreSubagentServiceAdapter.ts`:
- Around line 785-803: The async settings guard in CoreSubagentServiceAdapter is
throwing before executeAsyncSubagent() can convert missing-config cases into the
standard structured tool error response. Update checkAsyncSettings() and its
call site so the no-config path is handled without throwing, and ensure
executeAsyncSubagent() still returns the normal error object with
ToolErrorType.EXECUTION_FAILED when config is missing. Use the existing symbols
checkAsyncSettings, executeAsyncSubagent, requireConfig, and ToolErrorType to
keep the async call on the structured error path.
---
Duplicate comments:
In `@packages/core/src/code_assist/setup.ts`:
- Around line 43-46: The project ID selection in setup.ts still treats
whitespace-only GOOGLE_CLOUD_PROJECT as a valid value, preventing fallback to
GOOGLE_CLOUD_PROJECT_ID. Update the logic around rawProjectId/projectId to trim
and normalize each environment value before choosing the fallback so blank or
whitespace-only strings become undefined. Use the setup() projectId assignment
and the env lookups for GOOGLE_CLOUD_PROJECT and GOOGLE_CLOUD_PROJECT_ID as the
places to adjust.
In `@packages/core/src/core/contentGenerator.ts`:
- Around line 86-90: The Vertex project env lookup in contentGenerator is still
prioritizing GOOGLE_CLOUD_PROJECT before empty-string normalization, so an empty
value can block fallback to GOOGLE_CLOUD_PROJECT_ID. Update the project
resolution logic in contentGenerator to treat blank GOOGLE_CLOUD_PROJECT as
unset before choosing the fallback, and ensure the resulting googleCloudProject
selection still comes from the first non-empty env value.
In `@packages/core/src/runtime/AgentRuntimeState.ts`:
- Around line 236-238: The session ID normalization in AgentRuntimeState should
treat whitespace-only values as invalid instead of only checking for an empty
string. Update the logic around params.sessionId so the generated
`session-${randomUUID()}` fallback is used when the provided value is undefined,
empty, or only whitespace, preserving the intended validation behavior.
In `@packages/core/src/runtime/contracts/boundary-guards.test.ts`:
- Around line 292-301: The provider-reference guard in boundary-guards.test.ts
is still swallowing tsconfig parse failures, so the test can pass after an
unexpected parse error. Update the catch path in the tsconfig parsing logic
around the provider-reference check to fail the test explicitly instead of only
writing to stderr, and keep the TS extends-format skip behavior separate from
real parse errors. Use the existing provider-reference assertion flow in the
boundary-guards test to locate the fix.
In `@packages/core/src/services/loopDetectionService.ts`:
- Around line 139-147: The blockquote detector in detectMarkdownBlockquote still
hardcodes a literal space after the markdown `>` marker, so it misses valid
blockquotes like `>\tquote`. Update the whitespace check to use the same
markdown whitespace helper used elsewhere in LoopDetectionService instead of
testing only `line[1] === ' '`, and keep the logic inside
detectMarkdownBlockquote aligned with the existing markdown parsing behavior.
🪄 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: e8bd742f-7c1b-4722-a3ee-0df7fa60ab11
⛔ Files ignored due to path filters (50)
project-plans/issue1594/.completed/P09a.mdis excluded by!project-plans/**project-plans/issue1594/.completed/P29.mdis excluded by!project-plans/**project-plans/issue1594/plan/00a-preflight-verification.mdis excluded by!project-plans/**project-plans/issue1594/plan/01a-analysis-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/00a-preflight-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/01-analysis.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/01a-analysis-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/02-pseudocode.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/02a-pseudocode-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/03-provider-manager-seam-stub.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/03a-provider-manager-seam-stub-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/04-provider-manager-seam-tdd.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/04a-provider-manager-seam-tdd-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/05-provider-manager-seam-impl.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/05a-provider-manager-seam-impl-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/06-fromconfig-stub.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/06a-fromconfig-stub-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/07a-early-parity-red-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/08-fromconfig-tdd.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/08a-fromconfig-tdd-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/09-fromconfig-impl.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/09a-fromconfig-impl-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/10-settings-surface-stub.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/10a-settings-surface-stub-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/11-settings-surface-tdd.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/11a-settings-surface-tdd-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/12-settings-surface-impl.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/12a-settings-surface-impl-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/13-seqmodel-tdd.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/13a-seqmodel-tdd-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/14-seqmodel-impl.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/14a-seqmodel-impl-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/15-contract-promotion-tdd.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/15a-contract-promotion-tdd-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/16-contract-promotion-impl.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/16a-contract-promotion-impl-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/17-runtime-seam-tdd.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/17a-runtime-seam-tdd-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/18-runtime-seam-impl.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/18a-runtime-seam-impl-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/19a-parity-harness-tdd-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/20-parity-harness-green.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/20a-parity-harness-green-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/21-boundary-and-nonbreaking.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/21a-boundary-and-nonbreaking-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/22-docs.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/22a-docs-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/23-quality-gates.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/23a-quality-gates-verification.mdis excluded by!project-plans/**project-plans/issue1594remediate/plan/24-final-plan-quality-eval.mdis excluded by!project-plans/**
📒 Files selected for processing (131)
eslint.config.jspackages/core/src/code_assist/oauth-credential-storage.tspackages/core/src/code_assist/setup.tspackages/core/src/config/config-lsp-integration.test.tspackages/core/src/config/configBaseCore.tspackages/core/src/config/endpoints.tspackages/core/src/config/lspIntegration.tspackages/core/src/config/subagentManager.tspackages/core/src/config/toolRegistryFactory.tspackages/core/src/core/contentGenerator.tspackages/core/src/core/coreToolHookTriggers.tspackages/core/src/core/lifecycleHookTriggers.tspackages/core/src/core/logger.tspackages/core/src/core/subagentTypes.test.tspackages/core/src/core/subagentTypes.tspackages/core/src/debug/DebugLogger.test.tspackages/core/src/debug/FileOutput.test.tspackages/core/src/filters/EmojiFilter.tspackages/core/src/hooks/__tests__/hookEventHandler-messagebus.test.tspackages/core/src/hooks/__tests__/hookSemantics.test.tspackages/core/src/hooks/__tests__/hookSystem-lifecycle.test.tspackages/core/src/hooks/__tests__/hookValidators.test.tspackages/core/src/hooks/hookEventHandler.tspackages/core/src/hooks/hookRegistry.tspackages/core/src/hooks/hookRunner.test.tspackages/core/src/hooks/types.tspackages/core/src/integration/compression-duplicate-ids.test.tspackages/core/src/models/hydration.tspackages/core/src/models/registry.tspackages/core/src/parsers/TextToolCallParser.test.tspackages/core/src/parsers/TextToolCallParser.tspackages/core/src/parsers/text-tool-call-arg-parsing.tspackages/core/src/parsers/text-tool-call-helpers.tspackages/core/src/parsers/tool-call-parser-utils.tspackages/core/src/policy/utils.test.tspackages/core/src/prompt-config/TemplateEngine.tspackages/core/src/prompt-config/prompt-cache.tspackages/core/src/prompt-config/prompt-installer.test.tspackages/core/src/prompt-config/prompt-loader.test.tspackages/core/src/prompt-config/prompt-service.tspackages/core/src/recording/ReplayEngine.tspackages/core/src/recording/SessionDiscovery.test.tspackages/core/src/recording/SessionDiscovery.tspackages/core/src/recording/SessionLockManager.test.tspackages/core/src/recording/SessionLockManager.tspackages/core/src/recording/SessionRecordingService.test.tspackages/core/src/recording/__tests__/SessionDiscovery.extensions.spec.tspackages/core/src/recording/resumeSession.test.tspackages/core/src/recording/sessionCleanupUtils.test.tspackages/core/src/recording/sessionManagement.test.tspackages/core/src/runtime/AgentRuntimeLoader.tspackages/core/src/runtime/AgentRuntimeState.tspackages/core/src/runtime/RuntimeInvocationContext.tspackages/core/src/runtime/contracts/boundary-guards.test.tspackages/core/src/runtime/createAgentRuntimeContext.tspackages/core/src/runtime/errors/MissingRuntimeProviderError.test.tspackages/core/src/services/environmentSanitization.tspackages/core/src/services/gitService.tspackages/core/src/services/history/__tests__/density-history.test.tspackages/core/src/services/history/canonicalToolIds.tspackages/core/src/services/loopDetectionService.tspackages/core/src/skills/skillLoader.tspackages/core/src/skills/skillManager.tspackages/core/src/storage/SessionPersistenceService.tspackages/core/src/telemetry/loggers.test.circular.tspackages/core/src/test-utils/runtime.tspackages/core/src/test-utils/tools.tspackages/core/src/todo/todoFormatter.tspackages/core/src/tools-adapters/CoreMessageBusAdapter.tspackages/core/src/tools-adapters/CoreShellToolHostAdapter.tspackages/core/src/tools-adapters/CoreSubagentServiceAdapter.tspackages/core/src/tools-adapters/coreSubagentServiceHelpers.tspackages/core/src/tools/tool-key-storage.test.tspackages/core/src/utils/__tests__/resolveTextSearchTarget.test.tspackages/core/src/utils/ast-grep-utils.tspackages/core/src/utils/asyncIterator.test.tspackages/core/src/utils/bfsFileSearch.test.tspackages/core/src/utils/checkpointUtils.test.tspackages/core/src/utils/checkpointUtils.tspackages/core/src/utils/editor.tspackages/core/src/utils/errorParsing.tspackages/core/src/utils/errorReporting.test.tspackages/core/src/utils/errorReporting.tspackages/core/src/utils/events.tspackages/core/src/utils/fileDiffUtils.tspackages/core/src/utils/fileUtils.test.tspackages/core/src/utils/fileUtils.tspackages/core/src/utils/filesearch/crawler.tspackages/core/src/utils/filesearch/fileSearch.tspackages/core/src/utils/generateContentResponseUtilities.tspackages/core/src/utils/getFolderStructure.tspackages/core/src/utils/getPty.tspackages/core/src/utils/gitIgnoreParser.tspackages/core/src/utils/gitLineChanges.tspackages/core/src/utils/googleErrors.tspackages/core/src/utils/googleQuotaErrors.test.tspackages/core/src/utils/googleQuotaErrors.tspackages/core/src/utils/ignorePatterns.tspackages/core/src/utils/memoryDiscovery.test.tspackages/core/src/utils/memoryDiscovery.tspackages/core/src/utils/memoryImportProcessor.test.tspackages/core/src/utils/memoryImportProcessor.tspackages/core/src/utils/parameterCoercion.tspackages/core/src/utils/partUtils.tspackages/core/src/utils/paths.tspackages/core/src/utils/quotaErrorDetection.tspackages/core/src/utils/retry.quota.test.tspackages/core/src/utils/retry.test.tspackages/core/src/utils/retry.tspackages/core/src/utils/safeJsonStringify.test.tspackages/core/src/utils/sanitization.tspackages/core/src/utils/schemaValidator.tspackages/core/src/utils/secure-browser-launcher.tspackages/core/src/utils/shell-parser.test.tspackages/core/src/utils/shell-parser.tspackages/core/src/utils/shell-utils.shellReplacement.test.tspackages/core/src/utils/shell-utils.test.tspackages/core/src/utils/shell-utils.tspackages/core/src/utils/shellPathCompletion.tspackages/core/src/utils/stdio.test.tspackages/core/src/utils/stdio.tspackages/core/src/utils/streamIdleTimeout.tspackages/core/src/utils/summarizer.tspackages/core/src/utils/systemEncoding.tspackages/core/src/utils/terminalSerializer.test.tspackages/core/src/utils/terminalSerializer.tspackages/core/src/utils/tool-utils.tspackages/core/src/utils/userAccountManager.tspackages/core/test/models/schema.test.tsscripts/check-eslint-guard.jsscripts/tests/eslint-guard.test.js
💤 Files with no reviewable changes (6)
- packages/core/src/runtime/RuntimeInvocationContext.ts
- packages/core/src/integration/compression-duplicate-ids.test.ts
- packages/core/src/services/gitService.ts
- packages/core/src/recording/SessionRecordingService.test.ts
- packages/core/src/hooks/tests/hookValidators.test.ts
- packages/core/src/hooks/tests/hookSemantics.test.ts
Comment on lines
+28
to
+31
| function normalizeOptionalString(value: unknown): string | undefined { | ||
| return value === null || value === undefined || value === '' | ||
| ? undefined | ||
| : (value as string); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Validate these credential fields by type before persisting them.
The new checks only reject null/undefined/''. migrateFromFileStorage() still feeds saveCredentials() with JSON.parse(...) as Credentials, so a malformed legacy file can persist objects or numbers into refreshToken, tokenType, or scope. Please treat non-strings as absent at this boundary (with 'Bearer' as the tokenType fallback) instead of asserting them with as string.
Suggested fix
function normalizeOptionalString(value: unknown): string | undefined {
- return value === null || value === undefined || value === ''
- ? undefined
- : (value as string);
+ return typeof value === 'string' && value !== '' ? value : undefined;
}
@@
refreshToken:
- credentials.refresh_token === undefined ||
- credentials.refresh_token === null ||
- credentials.refresh_token === ''
- ? undefined
- : credentials.refresh_token,
+ normalizeOptionalString(credentials.refresh_token),
tokenType:
- credentials.token_type === undefined ||
- credentials.token_type === null ||
- credentials.token_type === ''
- ? 'Bearer'
- : credentials.token_type,
+ normalizeOptionalString(credentials.token_type) ?? 'Bearer',
scope: normalizeOptionalString(credentials.scope),Also applies to: 105-117
🤖 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/core/src/code_assist/oauth-credential-storage.ts` around lines 28 -
31, The credential normalization in normalizeOptionalString and the
saveCredentials path is only filtering nullish/empty values, so malformed legacy
data can still persist non-string values into refreshToken, tokenType, or scope.
Update the boundary in saveCredentials so each credential field is validated by
type before persistence, treating any non-string as absent and applying the
existing Bearer fallback for tokenType, instead of casting with as string. Keep
the fix localized to the credential handling flow used by migrateFromFileStorage
and saveCredentials.
Comment on lines
+115
to
+135
| try { | ||
| await Promise.race([ | ||
| registerMcpNavigationTools(state, host, streams), | ||
| new Promise<void>((_, reject) => { | ||
| const signal = AbortSignal.timeout( | ||
| MCP_NAVIGATION_REGISTRATION_TIMEOUT_MS, | ||
| ); | ||
| signal.addEventListener( | ||
| 'abort', | ||
| () => | ||
| reject( | ||
| signal.reason ?? new Error('MCP navigation registration timeout'), | ||
| ), | ||
| { once: true }, | ||
| ); | ||
| }), | ||
| ]); | ||
| } catch { | ||
| state.lspMcpClient = undefined; | ||
| state.lspMcpTransport = undefined; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Timeout does not actually stop MCP navigation registration.
Promise.race() only rejects the waiter here. registerMcpNavigationTools() keeps running after the timeout, so it can still connect, mutate state, or register tools after this function has already treated the attempt as failed. Clearing the two state fields in the catch block does not cancel that in-flight work or close any partially opened resources. Pass an explicit cancellation path into registerMcpNavigationTools() and close the transport/client on timeout instead of only nulling the state.
🤖 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/core/src/config/lspIntegration.ts` around lines 115 - 135, The
timeout handling in lspIntegration’s MCP navigation registration only rejects
the waiting Promise and leaves registerMcpNavigationTools() running, so add an
explicit cancellation path that the registration flow can observe and stop on
timeout. Update the Promise.race around registerMcpNavigationTools() to
propagate an abort signal or equivalent cancellation token into that function,
and ensure any partially created transport/client resources are explicitly
closed when the timeout fires instead of only clearing state.lspMcpClient and
state.lspMcpTransport in the catch block. Keep the fix centered on
registerMcpNavigationTools(), the timeout setup, and the cleanup path in
lspIntegration.
Comment on lines
+491
to
+497
| function filterPartText(part: Part, emojiFilter: EmojiFilter): Part { | ||
| if (!part.text) { | ||
| return part; | ||
| } | ||
| const filterResult = emojiFilter.filterText(part.text); | ||
| return { ...part, text: filterResult.filtered as string }; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not write null into checkpoint text parts.
EmojiFilter.filterText() can return filtered: null in error mode; Line 496 then casts that into Part.text, so loadCheckpoint() can return content with text: null.
Proposed fix
function filterPartText(part: Part, emojiFilter: EmojiFilter): Part {
if (!part.text) {
return part;
}
const filterResult = emojiFilter.filterText(part.text);
- return { ...part, text: filterResult.filtered as string };
+ if (typeof filterResult.filtered !== 'string') {
+ return part;
+ }
+ return { ...part, text: filterResult.filtered };
}📝 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
| function filterPartText(part: Part, emojiFilter: EmojiFilter): Part { | |
| if (!part.text) { | |
| return part; | |
| } | |
| const filterResult = emojiFilter.filterText(part.text); | |
| return { ...part, text: filterResult.filtered as string }; | |
| } | |
| function filterPartText(part: Part, emojiFilter: EmojiFilter): Part { | |
| if (!part.text) { | |
| return part; | |
| } | |
| const filterResult = emojiFilter.filterText(part.text); | |
| if (typeof filterResult.filtered !== 'string') { | |
| return part; | |
| } | |
| return { ...part, text: filterResult.filtered }; | |
| } |
🤖 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/core/src/core/logger.ts` around lines 491 - 497, The checkpoint text
filtering path in filterPartText currently casts
EmojiFilter.filterText().filtered to a string, which can propagate null into
Part.text when the filter runs in error mode. Update filterPartText (and any
caller like loadCheckpoint that relies on it) to guard against a null filtered
value and preserve a valid text string or otherwise skip writing the text field,
so checkpoint parts never end up with text: null.
| if (untrusted.length > 0) { | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- intentional falsy coalescing: empty string hook name/command should fall through | ||
| const hookNames = untrusted.map((h) => h.name || h.command).join(', '); | ||
| const hookNames = untrusted.map((h) => h.name ?? h.command).join(', '); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use getHookName() for the trust warning display.
Line 194 preserves name: '', so an untrusted hook with an empty name can render as a blank entry instead of showing its command.
Proposed fix
- const hookNames = untrusted.map((h) => h.name ?? h.command).join(', ');
+ const hookNames = untrusted
+ .map((h) => this.getHookName({ config: h }))
+ .join(', ');📝 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 hookNames = untrusted.map((h) => h.name ?? h.command).join(', '); | |
| const hookNames = untrusted | |
| .map((h) => this.getHookName({ config: h })) | |
| .join(', '); |
🤖 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/core/src/hooks/hookRegistry.ts` at line 194, The trust warning
currently builds the display list in hookRegistry using untrusted.map on name ??
command, which can still render an empty string when a hook has name: ''. Update
the warning display to use getHookName() for each untrusted hook instead of
directly reading name/command so blank names fall back correctly to the command.
Keep the change localized to the hookRegistry trust-warning logic and preserve
the existing join(', ') formatting.
Comment on lines
+170
to
+171
| const normalizedTool = | ||
| typeof toolName === 'string' ? toolName.trim().toLowerCase() : ''; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the lint policy reference and locate this redundant guard.
rg -n 'unnecessary-condition|typeof toolName === .string.' eslint.config.js packages/core/src/parsers/text-tool-call-arg-parsing.tsRepository: vybestack/llxprt-code
Length of output: 545
🏁 Script executed:
#!/bin/bash
set -e
sed -n '130,190p' packages/core/src/parsers/text-tool-call-arg-parsing.ts
printf '\n---\n'
rg -n "applyToolSpecificNormalizations|toolName:" packages/core/src/parsers/text-tool-call-arg-parsing.tsRepository: vybestack/llxprt-code
Length of output: 2097
🏁 Script executed:
#!/bin/bash
set -e
ast-grep outline packages/core/src/parsers/text-tool-call-arg-parsing.ts --view expandedRepository: vybestack/llxprt-code
Length of output: 1366
🏁 Script executed:
#!/bin/bash
set -e
cat -n packages/core/src/parsers/text-tool-call-arg-parsing.ts | sed -n '150,190p'Repository: vybestack/llxprt-code
Length of output: 1525
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,220p' packages/core/src/parsers/text-tool-call-arg-parsing.ts
printf '\n--- CALL SITES ---\n'
rg -n "applyToolSpecificNormalizations\(" packages/core/src -g '*.ts'Repository: vybestack/llxprt-code
Length of output: 6790
Remove the redundant typeof toolName guard. applyToolSpecificNormalizations only receives a string, and every call site passes a string, so the fallback branch is dead code and trips @typescript-eslint/no-unnecessary-condition.
🤖 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/core/src/parsers/text-tool-call-arg-parsing.ts` around lines 170 -
171, The normalization logic in applyToolSpecificNormalizations contains a
redundant typeof toolName check even though the function only accepts a string
and all callers pass strings. Simplify the normalizedTool assignment by removing
the dead fallback branch and using the string value directly so the code no
longer triggers `@typescript-eslint/no-unnecessary-condition`.
Source: Learnings
|
|
||
| beforeAll(async () => { | ||
| // Try to initialize - may fail in test environment | ||
| // Refresh initialization state in case another test reset the parser. |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
In Vitest 3.1.1, is the predicate passed to it.skipIf(...)evaluated when the test is declared, or is it re-evaluated afterbeforeAll hooks run?
💡 Result:
In Vitest 3.1.1, the predicate passed to it.skipIf is evaluated during the test gathering (declaration) phase, not during test execution after beforeAll hooks [1]. Because it.skipIf is evaluated when the test file is loaded and parsed, it cannot rely on the state set by lifecycle hooks like beforeAll or beforeEach [1]. If you need to skip a test based on conditions determined during test execution (such as after an async setup or a hook has run), you should use t.skip inside the test body or the hook itself instead [1].
Citations:
🏁 Script executed:
sed -n '1,120p' packages/core/src/utils/shell-parser.test.tsRepository: vybestack/llxprt-code
Length of output: 3689
🏁 Script executed:
sed -n '1,120p' packages/core/src/utils/shell-parser.test.tsRepository: vybestack/llxprt-code
Length of output: 3689
Don't use it.skipIf(!parserInitialized) with a value set before beforeAll. Vitest evaluates skipIf during test collection, so the later beforeAll refresh of parserInitialized won't change which tests run. Move the availability check to a hook or test body so the current parser state is used.
🤖 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/core/src/utils/shell-parser.test.ts` at line 44, The availability
gating in shell-parser tests is using it.skipIf(!parserInitialized), but that
value is decided before beforeAll can refresh it, so the collected tests won’t
reflect the current parser state. Update shell-parser.test.ts to remove the
collection-time skip logic and instead check parserInitialized inside a setup
hook or at the start of the affected test cases, using the existing beforeAll
and test blocks so the current parser state determines whether the tests run.
Comment on lines
+350
to
+357
| it.skipIf(!parserInitialized)( | ||
| 'should handle empty input gracefully', | ||
| () => { | ||
| // Empty string should produce an empty command list | ||
| const tree = parseShellCommand(''); | ||
| expect(tree).not.toBeNull(); | ||
| const commands = splitCommandsWithTree(tree!); | ||
| expect(commands.length).toBe(0); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the empty-input expectation.
parseShellCommand('') returns null by design (!command.trim() short-circuits in packages/core/src/utils/shell-parser.ts), so this assertion fails whenever the parser-backed path runs.
Suggested fix
- const tree = parseShellCommand('');
- expect(tree).not.toBeNull();
- const commands = splitCommandsWithTree(tree!);
- expect(commands.length).toBe(0);
+ expect(parseShellCommand('')).toBeNull();📝 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
| it.skipIf(!parserInitialized)( | |
| 'should handle empty input gracefully', | |
| () => { | |
| // Empty string should produce an empty command list | |
| const tree = parseShellCommand(''); | |
| expect(tree).not.toBeNull(); | |
| const commands = splitCommandsWithTree(tree!); | |
| expect(commands.length).toBe(0); | |
| it.skipIf(!parserInitialized)( | |
| 'should handle empty input gracefully', | |
| () => { | |
| // Empty string should produce an empty command list | |
| expect(parseShellCommand('')).toBeNull(); | |
| } | |
| ); |
🤖 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/core/src/utils/shell-parser.test.ts` around lines 350 - 357, The
empty-input test in shell-parser.test is asserting the wrong behavior for
parseShellCommand, since parseShellCommand('') intentionally returns null when
the input is blank. Update the test case named "should handle empty input
gracefully" to expect a null result from parseShellCommand for empty strings,
and remove the downstream splitCommandsWithTree/tree assertions that assume a
non-null tree; keep the test aligned with the short-circuit behavior in
parseShellCommand from shell-parser.ts.
Comment on lines
+50
to
53
| beforeAll(async () => { | ||
| mockPlatform.mockReturnValue('linux'); | ||
| await initializeShellParsers(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and identify the relevant test constructs.
ast-grep outline packages/core/src/utils/shell-utils.test.ts --view expanded || true
printf '\n--- file excerpt ---\n'
sed -n '1,220p' packages/core/src/utils/shell-utils.test.ts
printf '\n--- search skipIf usage ---\n'
rg -n "skipIf|isParserAvailable|initializeShellParsers|beforeAll" packages/core/src/utils/shell-utils.test.ts packages/core/src/utils -g '!**/dist/**' || true
printf '\n--- search test framework implementation ---\n'
rg -n "skipIf\\(|\\.skipIf|describe\\.skipIf|it\\.skipIf" packages -g '!**/dist/**' || trueRepository: vybestack/llxprt-code
Length of output: 31161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the test helper/framework definition if it exists in the repository.
rg -n "skipIf" packages -g '!**/dist/**' -g '!**/build/**' -g '!**/coverage/**' || trueRepository: vybestack/llxprt-code
Length of output: 13405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant portions of the file with line numbers.
nl -ba packages/core/src/utils/shell-utils.test.ts | sed -n '1,240p'Repository: vybestack/llxprt-code
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect parser availability/initialization behavior and the related test file sections.
printf '\n--- shell-parser.ts ---\n'
sed -n '1,240p' packages/core/src/utils/shell-parser.ts
printf '\n--- shell-utils.shellReplacement.test.ts excerpt ---\n'
sed -n '1,80p' packages/core/src/utils/shell-utils.shellReplacement.test.ts
printf '\n--- shell-utils.test.ts relevant ranges ---\n'
sed -n '140,220p' packages/core/src/utils/shell-utils.test.ts
printf '\n--- more relevant ranges ---\n'
sed -n '500,610p' packages/core/src/utils/shell-utils.test.tsRepository: vybestack/llxprt-code
Length of output: 15691
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/core/src/utils/shell-parser.ts --view expanded || trueRepository: vybestack/llxprt-code
Length of output: 1829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the parser module and Vitest helper semantics from local source if present.
sed -n '1,240p' packages/core/src/utils/shell-parser.ts
printf '\n--- if helper is defined locally ---\n'
rg -n "skipIf\\s*\\(" packages -g '!**/dist/**' -g '!**/build/**' | head -n 40Repository: vybestack/llxprt-code
Length of output: 11008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the parser module and the shell-utils test around the affected lines.
sed -n '1,220p' packages/core/src/utils/shell-parser.ts
printf '\n--- shell-utils.test.ts around parser-dependent tests ---\n'
sed -n '150,210p' packages/core/src/utils/shell-utils.test.ts
printf '\n--- shell-utils.test.ts around later parser-dependent tests ---\n'
sed -n '510,595p' packages/core/src/utils/shell-utils.test.tsRepository: vybestack/llxprt-code
Length of output: 11080
Initialize the parser before registering it.skipIf(...) tests
beforeAll runs too late here: it.skipIf(!isParserAvailable()) is evaluated while the file is loaded, so these parser-dependent cases can be skipped before initializeShellParsers() runs. That leaves the new coverage inactive in a normal run. Move parser initialization to module load, or keep the availability check inside each test body.
🤖 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/core/src/utils/shell-utils.test.ts` around lines 50 - 53, The parser
setup in shell-utils.test.ts happens too late for the
`it.skipIf(!isParserAvailable())` checks because those are evaluated when the
test file is loaded, not inside `beforeAll`. Move the `initializeShellParsers()`
call to module initialization before the `it.skipIf(...)` registrations, or
change the parser-dependent cases to check availability inside each test body so
the `isParserAvailable()` guard sees the initialized state.
Comment on lines
+349
to
+379
| export function checkCoreDirectiveScopesInConfig(configSource) { | ||
| const violations = []; | ||
| const lines = configSource.split('\n'); | ||
| let currentScope = null; | ||
| for (let i = 0; i < lines.length; i++) { | ||
| const line = lines[i]; | ||
| const scopeMatch = | ||
| /^const\s+(legacyDirectiveCleanupScopes|completedDirectiveCleanupScopes)\s*=/.exec( | ||
| line, | ||
| ); | ||
| if (scopeMatch) { | ||
| currentScope = scopeMatch[1]; | ||
| continue; | ||
| } | ||
| if (currentScope !== null) { | ||
| if (/^\s*\];/.test(line)) { | ||
| currentScope = null; | ||
| continue; | ||
| } | ||
| const match = /^\s*['"]([^'"]+)['"]/.exec(line); | ||
| if (match && match[1].includes('packages/core')) { | ||
| violations.push({ | ||
| file: 'eslint.config.js', | ||
| lineNumber: i + 1, | ||
| message: `packages/core must not remain in ${currentScope} (#2115).`, | ||
| content: line, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| return violations; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Single-line cleanup-scope entries still bypass the guard.
Line 360 sets currentScope and immediately continues, so a declaration like const legacyDirectiveCleanupScopes = ['packages/core/src/x.ts']; never reaches the matcher on Line 368. That makes the #2115 check formatting-dependent.
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 356-358: 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] 368-368: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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/check-eslint-guard.js` around lines 349 - 379, The cleanup-scope
scanner in checkCoreDirectiveScopesInConfig currently skips same-line array
declarations because it sets currentScope and immediately continues, so
single-line legacyDirectiveCleanupScopes or completedDirectiveCleanupScopes
entries containing packages/core are missed. Update the parsing logic to also
inspect the remainder of the declaration line after matching the scope
assignment, or otherwise handle inline arrays before continuing, while keeping
the existing scope tracking and violation reporting in
checkCoreDirectiveScopesInConfig.
Comment on lines
+492
to
+557
| for (let i = 0; i < lines.length; i++) { | ||
| const line = lines[i]; | ||
| const lineNumber = i + 1; | ||
| const arrayStart = extractArrayStart(line); | ||
| if (arrayStart !== null) { | ||
| currentArray = arrayStart; | ||
| } | ||
|
|
||
| if (currentArray !== null && /^\s*\]/.test(line)) { | ||
| currentArray = null; | ||
| } | ||
|
|
||
| if (isCorePathLine(line)) { | ||
| if (currentArray === 'ignores') { | ||
| violations.push({ | ||
| file: 'eslint.config.js', | ||
| lineNumber, | ||
| message: coreCentralBypassMessage('ignore'), | ||
| content: line, | ||
| }); | ||
| } else if (currentArray === 'allow') { | ||
| violations.push({ | ||
| file: 'eslint.config.js', | ||
| lineNumber, | ||
| message: coreCentralBypassMessage('allow-list'), | ||
| content: line, | ||
| }); | ||
| } | ||
| coreObjectDepth = enclosingObjectDepth(line, braceDepth); | ||
| } | ||
|
|
||
| const inCoreObject = | ||
| coreObjectDepth !== null && braceDepth >= coreObjectDepth; | ||
| if (inCoreObject) { | ||
| if (/^\s*ignores\s*:/.test(line)) { | ||
| violations.push({ | ||
| file: 'eslint.config.js', | ||
| lineNumber, | ||
| message: coreCentralBypassMessage('scoped ignore'), | ||
| content: line, | ||
| }); | ||
| } | ||
| if (/^\s*rules\s*:/.test(line)) { | ||
| rulesObjectDepth = braceDepth + countOpeningBraces(line); | ||
| } | ||
| if ( | ||
| rulesObjectDepth !== null && | ||
| (isNewOffRule(line) || isStandaloneOffRuleValue(line)) | ||
| ) { | ||
| violations.push({ | ||
| file: 'eslint.config.js', | ||
| lineNumber, | ||
| message: coreCentralBypassMessage('rule-off'), | ||
| content: line, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| braceDepth += countBraceDelta(line); | ||
| if (rulesObjectDepth !== null && braceDepth < rulesObjectDepth) { | ||
| rulesObjectDepth = null; | ||
| } | ||
| if (coreObjectDepth !== null && braceDepth < coreObjectDepth) { | ||
| coreObjectDepth = null; | ||
| rulesObjectDepth = null; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
One-line core override objects can hide off and ignores bypasses.
If files, rules, and/or ignores share one line, coreObjectDepth is recorded only after the line has already been checked, and braceDepth is updated afterward. A config like { files: ['packages/core/src/x.ts'], rules: { 'no-console': 'off' } } therefore slips through without any violation.
🤖 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/check-eslint-guard.js` around lines 492 - 557, The core override scan
in the main loop misses one-line objects because `coreObjectDepth` and
`rulesObjectDepth` are set after the line has already been evaluated and
`braceDepth` is only updated later. Update the detection flow around
`extractArrayStart`, `isCorePathLine`, `enclosingObjectDepth`, and the
`rules`/`ignores` checks so same-line `files`/`rules`/`ignores` blocks are
analyzed with the correct depth immediately, and ensure `isNewOffRule` and
`isStandaloneOffRuleValue` still trigger violations for inline core overrides.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
Removes all inline ESLint disable and enable directives from packages/core, removes packages/core from central directive-cleanup bypass lists, and adds guard coverage so core directive or central-bypass regressions fail automatically.
Dive Deeper
This PR completes the packages/core cleanup for issue #2115 by:
Reviewer Test Plan
A reviewer can validate by running:
Useful focused review areas:
Testing Matrix
Linked issues / bugs
Fixes #2115