Conversation
GitHub Actions emits Node 20 deprecation warnings and actions/setup-node resolved the project runtime from .nvmrc as Node 20. Move every CI job, the sandbox/builder images, the declared engine baseline, first-party type declarations, and contributor docs to Node 24 so CI matches GitHub's current JavaScript-action runtime direction. - .nvmrc: 20 -> 24 (drives all node-version-file jobs) - Dockerfile: node:20-slim -> node:24-slim - .gcp/Dockerfile.gemini-code-builder: NodeSource node_20.x -> node_24.x - actions/setup-node v4 -> v6 (node24 runtime), pinned SHA + ratchet ref - Hardcoded node-version '20'/'20.x' -> '24'/'24.x' in e2e, nightly, evals-nightly, build-sandbox workflows - engines.node >=20 -> >=24 across root + all workspace packages - @types/node ^20 -> ^24 in cli and vscode-ide-companion (other packages and root already on ^24) - Sync package-lock.json and bun.lock - Update package-metadata test assertion to expect >=24 - Docs (README, README_CN, CONTRIBUTING, getting-started, sandbox-setup, vscode companion, dev-docs/PLAN) reference Node 24
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughThis PR upgrades the repository's Node.js baseline from version 20 to version 24. Changes span the Dockerfiles, GitHub Actions workflows (updating actions/setup-node to v6 and node-version to 24), .nvmrc, documentation, and engines.node constraints across all package.json files, plus a corresponding test update. ChangesNode.js 24 Baseline Upgrade
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
LLxprt PR Review – PR #2318Issue AlignmentThe PR updates CI workflows, Dockerfiles, Side Effects
Code Quality
Tests and Coverage
VerdictNeeds Work
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
package-lock.json retained stale nested @types/node@20.19.33 installs for packages/cli and packages/vscode-ide-companion even though their manifests declare ^24.2.1. This caused npm ci to silently install Node 20 type definitions for those workspaces while the root hoisted entry was already 24.13.2 — so typecheck/build were validating against the wrong @types/node in both local and CI environments. Remove the four stale nested lockfile entries (2x @types/node + their undici-types deps). Both workspaces now hoist to the root @types/node@24.13.2, which satisfies ^24.2.1. Also align the root devDependency range from ^24.0.13 to ^24.2.1 for consistency with all workspace packages. Verified: check:lockfile passes, typecheck + build pass against @types/node@24.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-sandbox.yml:
- Around line 48-51: Disable the implicit npm cache in the build-sandbox publish
job’s actions/setup-node step. Because package.json declares packageManager as
npm@11.6.2, setup-node will auto-enable caching unless explicitly turned off;
update the Set up Node.js configuration to set package-manager-cache to false so
this workflow does not introduce a cache-poisoning path.
In @.github/workflows/nightly.yml:
- Around line 156-172: The nightly workflow’s Checkout step is pinned to a
different actions/checkout commit than the other v5 usage, so the ratchet label
and code are inconsistent. Update the Checkout reference in the workflow to the
same actions/checkout@v5 pin used elsewhere, keeping the existing ratchet
annotation aligned with that pinned version.
🪄 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: ec64950d-92f2-409f-809b-ca9c4bcbac14
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
.gcp/Dockerfile.gemini-code-builder.github/workflows/build-sandbox.yml.github/workflows/ci.yml.github/workflows/e2e.yml.github/workflows/evals-nightly.yml.github/workflows/interactive-ui.yml.github/workflows/luther.yml.github/workflows/nightly.yml.github/workflows/release.yml.github/workflows/upstream-sync.yml.nvmrcCONTRIBUTING.mdDockerfileREADME.mdREADME_CN.mddev-docs/PLAN.mddocs/getting-started.mddocs/tutorials/sandbox-setup.mdpackage.jsonpackages/a2a-server/package.jsonpackages/agents/package.jsonpackages/auth/package.jsonpackages/cli/package.jsonpackages/core/package.jsonpackages/ide-integration/package.jsonpackages/lsp/package.jsonpackages/mcp/package.jsonpackages/policy/package.jsonpackages/providers/package.jsonpackages/settings/package.jsonpackages/storage/package.jsonpackages/telemetry/package.jsonpackages/test-utils/package.jsonpackages/tools/package.jsonpackages/tools/src/__tests__/package-metadata.test.tspackages/vscode-ide-companion/README.mdpackages/vscode-ide-companion/package.json
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: Run LLxprt review
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: Bun Native Modules Smoke
- GitHub Check: Node Consumer Smoke
- GitHub Check: CodeQL
- GitHub Check: Lint (Javascript)
- GitHub Check: Interactive UI (tmux)
⚠️ CI failures not shown inline (2)
GitHub Actions: LLxprt PR Review / 0_Run LLxprt review.txt: Upgrade CI workflows and runtime baseline to Node 24 (Fixes #2316)
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mllxprt_log="review/llxprt.log"�[0m
�[36;1mcontext_limit="${LLXPRT_CONTEXT_LIMIT:-200000}"�[0m
�[36;1mecho "LLXPRT_CONTEXT_LIMIT raw: '${LLXPRT_CONTEXT_LIMIT-<unset>}'" >&2�[0m
�[36;1mecho "LLXPRT_CONTEXT_LIMIT evaluated: '${context_limit}'" >&2�[0m
�[36;1m�[0m
�[36;1m# Initial instruction for agentic review - agent will explore artifacts using tools�[0m
�[36;1minitial_prompt="You are reviewing PR #${PR_NUMBER}. Start by reading review/instructions.md for your mission and review/context.md for PR details. Use file tools to explore the changes incrementally. Write your final verdict to review/verdict.md."�[0m
�[36;1m�[0m
�[36;1mset +e�[0m
�[36;1m# Agentic mode: use --prompt flag to pass initial instruction�[0m
�[36;1m# Use --baseurl CLI flag instead of --set base-url to ensure proper timing�[0m
�[36;1mllxprt \�[0m
�[36;1m --provider "${LLXPRT_DEFAULT_PROVIDER}" \�[0m
�[36;1m --model "${LLXPRT_DEFAULT_MODEL}" \�[0m
�[36;1m --yolo \�[0m
�[36;1m --key "${OPENAI_API_KEY}" \�[0m
�[36;1m --baseurl "${OPENAI_BASE_URL}" \�[0m
�[36;1m --set modelparam.temperature=0.7 \�[0m
�[36;1m --set modelparam.max_tokens=16000 \�[0m
�[36;1m --set context-limit="${context_limit}" \�[0m
�[36;1m --set shell-replacement=false \�[0m
�[36;1m --prompt "${initial_prompt}" 2>&1 | tee "${llxprt_log}"�[0m
�[36;1mllxprt_status=${PIPESTATUS[0]}�[0m
�[36;1mset -e�[0m
�[36;1m�[0m
�[36;1mif [[ ${llxprt_status} -ne 0 ]]; then�[0m
�[36;1m {�[0m
�[36;1m echo "<!-- llxprt-pr-review -->"�[0m
�[36;1m echo "## WARNING: LLxprt PR Review infrastructure failure"�[0m
�[36;1m echo�[0m
�[36;1m echo "The automated reviewer failed with exit code ${llxprt_status}. Please inspect the workflow logs (LLxprt section) and re-run once resolved."�[0m
�[36;1m } > review/comment.md�[0m
�[36;1m exit "${llxprt_status}"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1m# Use verdict.md if agent wrote it, otherwise f...
GitHub Actions: LLxprt PR Review / Run LLxprt review: Upgrade CI workflows and runtime baseline to Node 24 (Fixes #2316)
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mllxprt_log="review/llxprt.log"�[0m
�[36;1mcontext_limit="${LLXPRT_CONTEXT_LIMIT:-200000}"�[0m
�[36;1mecho "LLXPRT_CONTEXT_LIMIT raw: '${LLXPRT_CONTEXT_LIMIT-<unset>}'" >&2�[0m
�[36;1mecho "LLXPRT_CONTEXT_LIMIT evaluated: '${context_limit}'" >&2�[0m
�[36;1m�[0m
�[36;1m# Initial instruction for agentic review - agent will explore artifacts using tools�[0m
�[36;1minitial_prompt="You are reviewing PR #${PR_NUMBER}. Start by reading review/instructions.md for your mission and review/context.md for PR details. Use file tools to explore the changes incrementally. Write your final verdict to review/verdict.md."�[0m
�[36;1m�[0m
�[36;1mset +e�[0m
�[36;1m# Agentic mode: use --prompt flag to pass initial instruction�[0m
�[36;1m# Use --baseurl CLI flag instead of --set base-url to ensure proper timing�[0m
�[36;1mllxprt \�[0m
�[36;1m --provider "${LLXPRT_DEFAULT_PROVIDER}" \�[0m
�[36;1m --model "${LLXPRT_DEFAULT_MODEL}" \�[0m
�[36;1m --yolo \�[0m
�[36;1m --key "${OPENAI_API_KEY}" \�[0m
�[36;1m --baseurl "${OPENAI_BASE_URL}" \�[0m
�[36;1m --set modelparam.temperature=0.7 \�[0m
�[36;1m --set modelparam.max_tokens=16000 \�[0m
�[36;1m --set context-limit="${context_limit}" \�[0m
�[36;1m --set shell-replacement=false \�[0m
�[36;1m --prompt "${initial_prompt}" 2>&1 | tee "${llxprt_log}"�[0m
�[36;1mllxprt_status=${PIPESTATUS[0]}�[0m
�[36;1mset -e�[0m
�[36;1m�[0m
�[36;1mif [[ ${llxprt_status} -ne 0 ]]; then�[0m
�[36;1m {�[0m
�[36;1m echo "<!-- llxprt-pr-review -->"�[0m
�[36;1m echo "## WARNING: LLxprt PR Review infrastructure failure"�[0m
�[36;1m echo�[0m
�[36;1m echo "The automated reviewer failed with exit code ${llxprt_status}. Please inspect the workflow logs (LLxprt section) and re-run once resolved."�[0m
�[36;1m } > review/comment.md�[0m
�[36;1m exit "${llxprt_status}"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1m# Use verdict.md if agent wrote it, otherwise f...
🧰 Additional context used
🧠 Learnings (22)
📚 Learning: 2026-07-01T06:58:31.777Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2307
File: packages/cli/package.json:34-36
Timestamp: 2026-07-01T06:58:31.777Z
Learning: When reviewing a package's `package.json` under `packages/**/`, ensure the `files` array does not accidentally include compiled test sources (e.g., test artifacts under `dist`). If `dist` contains compiled tests, fix it by updating `tsconfig.build.json` to exclude test sources from the build output and then rewrite the `package.json` `files` entries accordingly (so published files cover only the intended build outputs and not `dist/src/*.test.*`-style artifacts).
Applied to files:
packages/lsp/package.jsonpackages/settings/package.jsonpackages/a2a-server/package.jsonpackages/auth/package.jsonpackages/tools/package.jsonpackages/vscode-ide-companion/package.jsonpackages/telemetry/package.jsonpackages/core/package.jsonpackages/agents/package.jsonpackages/cli/package.jsonpackages/storage/package.jsonpackages/providers/package.jsonpackages/policy/package.jsonpackages/test-utils/package.jsonpackages/mcp/package.jsonpackages/ide-integration/package.json
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/tools/src/__tests__/package-metadata.test.ts
📚 Learning: 2026-03-26T00:49:43.150Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/__tests__/auth-flow-orchestrator.spec.ts:309-324
Timestamp: 2026-03-26T00:49:43.150Z
Learning: In this repository’s Jest (or Jest-like) test files, it is acceptable to use `expect(promiseReturningFunction).resolves.not.toThrow()` when the function returns `Promise<void>`. Do not flag this as an incorrect or suboptimal matcher; for `Promise<void>` it is functionally equivalent to using `resolves.toBeUndefined()` to assert successful resolution.
Applied to files:
packages/tools/src/__tests__/package-metadata.test.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/tools/src/__tests__/package-metadata.test.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
packages/tools/src/__tests__/package-metadata.test.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
packages/tools/src/__tests__/package-metadata.test.ts
📚 Learning: 2026-06-30T06:12:11.602Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2260
File: scripts/tests/publish-integrity.test.ts:124-166
Timestamp: 2026-06-30T06:12:11.602Z
Learning: Do not add (or recommend adding) inline ESLint suppression directives such as `/* eslint-disable */`, `/* eslint-enable */`, or similar comment-based suppressions. This repository’s policy (`#2079/`#2080) bans inline ESLint suppressions and it is mechanically enforced by `scripts/check-eslint-guard.js` in the `lint:eslint-guard` CI job. During review, only suggest inline ESLint suppression if the author explicitly states the policy has changed (i.e., the CI guard would no longer block it).
Applied to files:
packages/tools/src/__tests__/package-metadata.test.ts
📚 Learning: 2026-06-19T17:16:56.523Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2108
File: packages/agents/src/api/agentImpl.ts:1047-1079
Timestamp: 2026-06-19T17:16:56.523Z
Learning: When the fake-provider test seam is active in vybestack/llxprt-code, `process.env.LLXPRT_FAKE_RESPONSES` is set to a fixture file path ending in a `.jsonl` (not to the string `'1'` or any other boolean-like value). In code, detect the seam by checking `process.env.LLXPRT_FAKE_RESPONSES !== undefined` (and/or that it is a non-empty string), rather than using `process.env.LLXPRT_FAKE_RESPONSES === '1'`. Update any callers of the env var accordingly (see `packages/providers/src/composition/providerManagerInstance.ts` and harness usages).
Applied to files:
packages/tools/src/__tests__/package-metadata.test.ts
📚 Learning: 2026-06-26T16:23:45.527Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2188
File: packages/mcp/src/auth/token-storage/file-token-storage.test.ts:0-0
Timestamp: 2026-06-26T16:23:45.527Z
Learning: For TypeScript unit/integration tests that intentionally skip behavior on Windows, prefer platform-gating via `it.skipIf(process.platform === 'win32')` instead of doing an early return inside the test body. This matches existing repo conventions for Windows-specific test skips (e.g., tool/platform tests) and keeps test intent explicit.
Applied to files:
packages/tools/src/__tests__/package-metadata.test.ts
📚 Learning: 2026-06-26T20:36:11.918Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2214
File: packages/tools/src/tools/tools.test.ts:88-95
Timestamp: 2026-06-26T20:36:11.918Z
Learning: When reviewing tool message-bus code in this repo, ensure you use the correct `subscribe` API. `PublishSubscribeCapable.subscribe` has the 2-argument signature `subscribe(event: string, handler: (response: unknown) => void): void | Unsubscribe` (i.e., `(event, handler)`), and call sites (e.g., `BaseToolInvocation` confirmation flow in `packages/tools/src/tools/tools.ts` and related tests) should follow that shape. Do not confuse it with the separate optional `IToolMessageBus.subscribe`, which may have a different contract; using the wrong `subscribe` overload/signature should be flagged during review.
Applied to files:
packages/tools/src/__tests__/package-metadata.test.ts
📚 Learning: 2026-06-30T20:03:19.860Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2290
File: packages/cli/src/launcher/bun-launcher.ts:209-226
Timestamp: 2026-06-30T20:03:19.860Z
Learning: If a change introduces large test blocks that would exceed the repository’s max-lines lint limit, split the test into a dedicated test file (e.g., move related coverage into a new {name}.test.ts file) rather than keeping everything in one test module. Ensure the extracted tests still cover the same behavior and are correctly imported/exercised by the test runner.
Applied to files:
packages/tools/src/__tests__/package-metadata.test.ts
📚 Learning: 2026-03-19T22:49:57.979Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/OpenAIRequestBuilder.test.ts:316-395
Timestamp: 2026-03-19T22:49:57.979Z
Learning: In `packages/core/src/providers/openai/OpenAIRequestBuilder.ts`, do not add logic to deduplicate tool messages by `tool_call_id` inside `validateToolMessageSequence`. Contiguous tool messages with the same `tool_call_id` are valid parts of the conversation history (e.g., replay/adaptor splitting). The only filtering/dropping behavior should be removing *orphaned* tool messages—those whose `tool_call_id` does not match any declared tool call in the immediately preceding assistant message. Eager deduplication of same-id tool messages has previously caused strict OpenAI-provider HTTP 400 errors, so avoid introducing it.
Applied to files:
.nvmrc
📚 Learning: 2026-03-20T01:26:20.297Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1736
File: packages/core/src/providers/openai/OpenAIClientFactory.test.ts:241-246
Timestamp: 2026-03-20T01:26:20.297Z
Learning: For `packages/core/src/providers/openai/OpenAIClientFactory.test.ts`, it’s acceptable for the `instantiateClient` tests to inspect the OpenAI SDK client instance’s internal `_options` field (e.g., via a type cast to `Record<string, unknown>`) to assert `defaultHeaders` and HTTP agent propagation. Do not flag `_options` inspection in this specific test file as relying on unstable internals, since this is a deliberate testing tradeoff versus heavier constructor mocking.
Applied to files:
.nvmrc
📚 Learning: 2026-03-21T00:08:43.988Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1741
File: packages/core/src/config/lspIntegration.ts:75-101
Timestamp: 2026-03-21T00:08:43.988Z
Learning: In packages/core/src/config/lspIntegration.ts, keep the existing timeout-bounded MCP navigation tool registration implementation that uses `Promise.race([registerMcpNavigationTools(...), timeoutPromise]).then(result => { ... })`. Do not refactor it into an `await` + inline `if`/fallback structure; treat this pattern as intentionally preserved from the original config.ts implementation and avoid restructuring refactors in this context.
Applied to files:
.nvmrc
📚 Learning: 2026-03-21T15:59:42.173Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1740
File: packages/core/src/providers/utils/toolIdNormalization.ts:98-121
Timestamp: 2026-03-21T15:59:42.173Z
Learning: In `packages/core/src/providers/utils/toolIdNormalization.ts`, keep `normalizeToAnthropicToolId`’s handling of empty/falsy tool IDs deterministic: return the constant `'toolu_empty'` and use `debugLogger.error(...)` for visibility. Do not replace this with a random/hash-based fallback; the prior hash fallback caused a pairing hazard where `tool_use` and `tool_result` could receive different IDs, breaking correlation. (Although this path is typically unreachable because upstream normalization maps empty IDs via `normalizeToHistoryToolId` to a value handled by the `hist_tool_` prefix logic, the error log should remain for the case where a provider returns an empty tool call ID.)
Applied to files:
.nvmrc
📚 Learning: 2026-03-21T17:07:17.141Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1743
File: packages/core/src/core/DirectMessageProcessor.ts:322-336
Timestamp: 2026-03-21T17:07:17.141Z
Learning: In `packages/core/src/core/DirectMessageProcessor.ts`, preserve the semantics in `_applyToolSelectionHook`: the guard `if (allowedFunctions?.length)` is intentionally keeping upstream behavior from `geminiChat.ts`. Treat an empty `allowedFunctionNames`/`allowedFunctions` array (`[]`) as “no restriction,” not “deny all,” and do not flag any decomposition/refactor PRs as a bug due to this empty-list handling. If someone proposes changing `[]` to mean deny-all, treat it as a feature/behavior modification (e.g., separate issue) rather than a refactor; upstream fix is tracked separately (e.g., `#1748`).
Applied to files:
.nvmrc
📚 Learning: 2026-03-26T02:22:16.930Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/streamUtils.ts:100-116
Timestamp: 2026-03-26T02:22:16.930Z
Learning: In `packages/cli/src/ui/hooks/geminiStream/streamUtils.ts`, do not flag `mergePendingToolGroupsForDisplay` as a deduplication bug when `filteredPendingTools` only removes overlapping shell entries and may leave overlapping non-shell tool `callId`s potentially duplicated. This behavior is pre-existing and was faithfully extracted from `useGeminiStream.ts`; any change to deduplicate non-shell overlapping `callId`s should be treated as a separate enhancement, not a structural refactor fix.
Applied to files:
.nvmrc
📚 Learning: 2026-03-26T02:47:16.208Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1779
File: packages/core/src/core/subagentExecution.test.ts:142-143
Timestamp: 2026-03-26T02:47:16.208Z
Learning: In `packages/core/src/core/subagentExecution.test.ts`, for the `checkGoalCompletion` tests, treat the return type as `Promise<Content[] | null>` (not `Promise<Part[] | null>`). Since `Content` is shaped like `{ role: string, parts: Part[] }`, assertions like `result![0].parts[0]` are correct for accessing the first `Part` inside the first `Content` element. Do not flag `result![0].parts[0]` as an invalid shape/index access in these tests for this function.
Applied to files:
.nvmrc
📚 Learning: 2026-03-26T20:52:05.558Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:140-149
Timestamp: 2026-03-26T20:52:05.558Z
Learning: In packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts, when reviewing decomposition/refactoring PRs, do not flag the applyThoughtToState reducer’s setPendingHistoryItem updater that casts item?.type as 'gemini' | 'gemini_content' and uses item?.text || '' as a bug. This behavior is intentional and was preserved from the original useGeminiStream.ts; changing it is a behavioral change that could affect tool_group pending item handling and requires careful end-to-end testing across all thinking-block rendering paths.
Applied to files:
.nvmrc
📚 Learning: 2026-03-27T01:00:24.588Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1785
File: packages/cli/src/config/__tests__/toolGovernanceParity.test.ts:458-472
Timestamp: 2026-03-27T01:00:24.588Z
Learning: In `packages/cli/src/config/__tests__/toolGovernanceParity.test.ts`, when reviewing this test suite’s parity/refactor checks (including cases using “non-interactive DEFAULT with explicit --allowed-tools” and corresponding YOLO test cases), don’t flag `read_file` as an insufficient or incorrect `--allowed-tools` value just because it’s in `READ_ONLY_TOOL_NAMES`. These tests are intentionally scoped to preserving existing allowlist behavior across the refactor, not to proving union/exclusion semantics for non-read-only tools; treat the choice as deliberate test design unless the assertions themselves are incorrect.
Applied to files:
.nvmrc
📚 Learning: 2026-04-22T08:29:35.103Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1907
File: packages/core/src/utils/googleQuotaErrors.test.ts:344-347
Timestamp: 2026-04-22T08:29:35.103Z
Learning: In `packages/core/src/utils/googleQuotaErrors.test.ts`, treat `if (result instanceof RetryableQuotaError) { ... }` blocks (with paired `// eslint-disable-next-line vitest/no-conditional-in-test` and `// eslint-disable-next-line vitest/no-conditional-expect`) as an intentional type-narrowing pattern for this lint-enforcement/test-validation work. In future reviews, don’t recommend consolidating these into a helper (e.g., `asRetryable()`) or removing the `eslint-disable-next-line` comments as part of lint-promotion/refactoring; keep the structure unchanged unless the PR explicitly changes the lint/enforcement scope.
Applied to files:
.nvmrc
📚 Learning: 2026-04-23T23:33:00.076Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1907
File: packages/cli/src/ui/components/Footer.test.tsx:198-214
Timestamp: 2026-04-23T23:33:00.076Z
Learning: In `packages/cli/src/ui/components/Footer.test.tsx`, the responsive truncation test (`"should adapt branch truncation length based on breakpoint"`) intentionally uses a regex contract (`/feature\/.+\.\.\..+/`) to validate truncation behavior rather than asserting per-breakpoint maximum lengths. During code review, do not flag the `expectedMaxLength` field as unused and do not suggest adding per-breakpoint `expectedMaxLength` assertions, since those would be a test-coverage enhancement beyond what should be enforced by lint/PR-scope review.
Applied to files:
.nvmrc
🪛 Checkov (3.3.2)
Dockerfile
[low] 1-101: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 Trivy (0.69.3)
Dockerfile
[info] 1-1: No HEALTHCHECK defined
Add HEALTHCHECK instruction in your Dockerfile
Rule: DS-0026
(IaC/Dockerfile)
🪛 zizmor (1.26.1)
.github/workflows/build-sandbox.yml
[error] 49-49: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
.github/workflows/release.yml
[error] 144-144: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
.github/workflows/nightly.yml
[warning] 168-169: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
.github/workflows/ci.yml
[error] 203-203: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
[error] 475-475: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
[error] 520-520: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
[error] 589-589: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step
(cache-poisoning)
🔇 Additional comments (37)
Dockerfile (1)
1-1: LGTM!.gcp/Dockerfile.gemini-code-builder (1)
7-9: LGTM!Also applies to: 38-49
.github/workflows/ci.yml (1)
187-189: LGTM! Theactions/setup-node@v6bump and node-version 24 changes are consistent with the repo's declared Node 24 baseline. The pre-existingcache: 'npm'inputs mean caching behavior here isn't newly changed by this bump (unlike the no-cache-input case inbuild-sandbox.yml).Also applies to: 203-206, 475-478, 520-523, 589-592
.github/workflows/e2e.yml (1)
146-169: LGTM!Also applies to: 272-276
.github/workflows/evals-nightly.yml (1)
24-28: LGTM!Also applies to: 60-64
.github/workflows/interactive-ui.yml (1)
71-75: LGTM!.github/workflows/luther.yml (1)
348-353: LGTM!.github/workflows/nightly.yml (2)
33-33: LGTM!
271-271: LGTM!.github/workflows/release.yml (1)
144-147: 🔒 Security & Privacy | ⚖️ Poor tradeoffReview cache use in privileged release job.
This step enables
cache: 'npm'inside thereleasejob, which holdscontents: write,packages: write, andid-token: writepermissions and publishes to npm/ghcr. Static analysis flags this as a potential cache-poisoning vector — a cache populated during a less-trusted run could be restored here and affect what gets built/published.Consider whether this job should use an isolated/dedicated cache key or disable caching entirely, given its elevated permissions and production-release scope.
Source: Linters/SAST tools
.github/workflows/upstream-sync.yml (1)
37-37: LGTM!.nvmrc (1)
1-1: LGTM!package.json (1)
5-5: LGTM!Also applies to: 158-158
packages/a2a-server/package.json (1)
52-52: LGTM!packages/agents/package.json (1)
64-64: LGTM!packages/auth/package.json (1)
40-40: LGTM!CONTRIBUTING.md (1)
103-104: LGTM!README.md (1)
83-83: LGTM!README_CN.md (1)
65-65: LGTM!packages/cli/package.json (1)
103-103: LGTM!Also applies to: 121-121
packages/core/package.json (1)
268-268: LGTM!packages/ide-integration/package.json (1)
44-44: LGTM!packages/lsp/package.json (1)
33-33: LGTM!dev-docs/PLAN.md (1)
251-251: LGTM!docs/getting-started.md (1)
7-7: LGTM!docs/tutorials/sandbox-setup.md (1)
24-24: LGTM!packages/mcp/package.json (1)
50-50: LGTM!packages/policy/package.json (1)
50-50: LGTM!packages/providers/package.json (1)
169-169: LGTM!packages/settings/package.json (1)
64-64: LGTM!packages/vscode-ide-companion/README.md (1)
46-46: LGTM!packages/storage/package.json (1)
83-83: LGTM!packages/telemetry/package.json (1)
78-78: LGTM!packages/test-utils/package.json (1)
24-24: LGTM!packages/tools/package.json (1)
73-73: LGTM!packages/tools/src/__tests__/package-metadata.test.ts (1)
130-133: LGTM!packages/vscode-ide-companion/package.json (1)
138-138: LGTM!
The root @types/node range was bumped from ^24.0.13 to ^24.2.1 in package.json but bun.lock still recorded ^24.0.13. The bun-workspaces test (macOS-only script harness) verifies bun.lock ranges match package.json, causing Test (macos-latest, keyring) to fail in CI.
acoliver
added a commit
that referenced
this pull request
Jul 2, 2026
# Conflicts: # .github/workflows/ci.yml # CONTRIBUTING.md # README.md # docs/getting-started.md
build-sandbox.yml: disable setup-node v6 package-manager-cache to prevent cache-poisoning in this privileged Docker publish job (packages:write). setup-node v6 auto-enables caching when package.json declares packageManager, unlike v4 which required an explicit cache input. nightly.yml: align e2e_full checkout pin to actions/checkout@v5 SHA (08c6903) used consistently across all other workflows, fixing the ratchet label/code inconsistency flagged by CodeRabbit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GitHub Actions emits Node 20 deprecation warnings, and actions/setup-node resolved the project runtime from .nvmrc as Node 20. This moves repository CI workflows, the sandbox/builder images, the declared engine baseline, first-party type declarations, and contributor docs to Node 24 so the warnings disappear and CI matches GitHub's current JavaScript-action runtime direction.
Closes #2316.
Changes
Runtime / images
CI workflows (9 files)
Package metadata
Lockfile integrity fix (follow-up commit)
Tests
Docs
Notes / decisions
Breaking support change (engines.node >=24)
This raises the declared minimum Node version from >=20 to >=24, which drops Node 20 and Node 22 (Node 22 is active LTS until April 2027). This is intentional and honest: the ci.yml test matrix was already 24.x-only on main before this PR -- Node 20 and 22 were declared in engines.node but were never CI-tested. Declaring >=24 aligns the published support floor with the version actually verified in CI. If broader runtime support is desired later, a dedicated matrix leg (e.g., Node 22) must be added to CI before widening engines.node again.
Other notes
Verification
Acceptance criteria (#2316)