Make folder trust updates live (Fixes #637) by acoliver · Pull Request #2547 · vybestack/llxprt-code

acoliver · 2026-07-13T03:39:35Z

TLDR

Make folder-trust changes effective immediately and safely across the active Config, MCP capabilities, policies, hooks, commands, approvals, IDE trust, and trust UI without requiring a restart.

The latest remediation also prevents delayed exits and post-unmount UI updates, catches synchronous trust-selection failures, refreshes direct trust provenance after persistence, and makes fake MCP latency honor pre-aborted signals.

Dive Deeper

Reconcile live trust gain/revocation across MCP clients, tools, prompts, resources, trusted policy rules, hooks, file commands, approvals, and IDE trust.
Preserve fail-closed behavior during canonicalization, connection, discovery, refresh, and publication races.
Centralize workspace trust resolution and harden trusted-folder persistence with canonical paths, atomic saves, and restrictive permissions.
Keep saved local fallback and effective trust distinct in permissions/startup UI.
Guard asynchronous trust dialogs/hooks after unmount and clear delayed fatal-exit timers.
Add behavioral coverage across CLI, core, MCP, policy, and IDE integration.
Evaluate all OCR and CodeRabbit feedback; fix valid correctness findings while source-dismissing duplicate, factual, security-regressing, and style-only churn.

Reviewer Test Plan

Start in an untrusted workspace and change trust through the startup and /permissions dialogs; verify effective trust changes without restart.
Revoke trust while MCP discovery/refresh is in flight; verify stale tools, prompts, and resources are not published and the client disconnects.
Unmount a trust dialog while a live transition is pending; verify no completion message, exit callback, state update, or delayed process exit occurs.
Make a trust-selection callback throw synchronously; verify an actionable error is displayed rather than an unhandled exception.
Run focused suites:
- cd packages/cli && npx vitest run src/ui/hooks/permissionsModifyTrustDialog.behavior.test.tsx
- cd packages/core && npx vitest run src/config/config.ideTrustLive.test.ts
- cd packages/mcp && npx vitest run src/fake/fakeMcpDiscovery.authorization.test.ts
Run npm run typecheck, npm run format, npm run build, changed-file ESLint, and the ollamakimi smoke command.

Testing Matrix

	🍏	🪟	🐧
npm run	✅	❓	CI
npx	✅	❓	CI
Docker	❓	❓	❓
Podman	❓	-	-
Seatbelt	❓	-	-

macOS verification completed locally. Linux validation is delegated to PR CI; Windows and container/sandbox variants were not run locally.

Linked issues / bugs

Fixes #637

Summary by CodeRabbit

New Features
- Folder trust changes apply immediately (including IDE trust transitions) without requiring a restart.
- File-based commands are now gated by live folder trust, with a clear untrusted message when blocked.
- Permissions and trust dialogs now consistently use the configured working directory, showing both saved vs effective trust.
Bug Fixes
- Trusted-folder evaluation and persistence are more reliable: most-specific matching wins, distrust overrides trust, and trusted-folder saves are atomic with restrictive permission repair.
- Policy listings now support tool-name prefixes (including correct wildcard handling), improving rule matching and display.
- Configuration-loading errors now provide clearer, fix-focused guidance.

coderabbitai · 2026-07-13T03:39:41Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 97e986c8-5593-43d8-b933-a4f5c4fe14cb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Changes

This PR makes folder trust dynamic across CLI configuration, UI flows, core configuration, policy evaluation, MCP lifecycle management, IDE integration, and file-command loading. Trust changes are persisted atomically, propagated through typed events, and applied without restarting.

Dynamic trust and policy enforcement

Layer / File(s)	Summary
Trust resolution and persistence `packages/cli/src/config/*`	Trust rules resolve by specificity, use explicit working directories, format configuration errors consistently, and save atomically with permission validation.
Live trust propagation `packages/core/src/config/*`, `packages/core/src/utils/events.ts`, `packages/ide-integration/src/ide/ide-client.ts`	Config tracks local and IDE trust changes, updates approval and policy state, serializes transition effects, and emits typed trust-change events.
CLI and UI trust flows `packages/cli/src/ui/commands/`, `packages/cli/src/ui/components/`, `packages/cli/src/ui/hooks/*`	Trust dialogs and permissions commands use configured working directories, persist changes, update live state, and replace restart prompts with immediate status displays.
MCP authorization and lifecycle `packages/mcp/src/client/*`	MCP discovery, execution, refresh, connection, and server cleanup enforce current trust and handle revocation races, cancellation, and stale capabilities.
Policy matcher updates `packages/core/src/policy/`, `packages/policy/src/`	Policy rules support explicit tool prefixes, literal wildcard names, source-based removal, and updated MCP trusted-rule generation.
Command and UI integration `packages/cli/src/services/FileCommandLoader.ts`, `packages/cli/src/ui/hooks/slashCommandProcessorSupport.ts`, `packages/cli/src/ui/hooks/useAutoAcceptIndicator.ts`	File commands and UI indicators react immediately to trust changes, while command registries reload when trust events are emitted.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

vybestack/llxprt-code#639: Related /permissions trust-flow and live trust update wiring.
vybestack/llxprt-code#1785: Related CLI config trust-evaluation flow and working-directory forwarding.
vybestack/llxprt-code#2384: Related CLI folder-trust UI and runtime configuration flow.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes implement immediate trust propagation and live updates across CLI, core, MCP, and UI, matching `#637`'s no-restart requirement.
Out of Scope Changes check	✅ Passed	The added changes are all supporting the live trust workflow; no clearly unrelated code changes stand out.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: making folder trust updates live.
Description check	✅ Passed	The description follows the required template and includes TLDR, Dive Deeper, Reviewer Test Plan, Testing Matrix, and linked issue details.

📋 Issue Planner

Built with CodeRabbit's Coding Plans for faster development and fewer bugs.

View plan used: #637

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch issue637

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-07-13T03:45:29Z

LLxprt PR Review – PR #2547

Issue Alignment

The PR directly addresses issue #637: folder trust updates now propagate live across Config, MCP clients, policy, hooks, commands, approvals, IDE trust, and UI without requiring a restart. The review artifacts include dedicated trust-lifecycle wiring and new trust-related tests in core, CLI, MCP, and policy packages.

Side Effects

Broad blast radius: 371 files changed, including unrelated provider retry/load-balancing rewrites, new genai-enclave scripts, and docs/CI tweaks.
Trust state is now effectively mutable at runtime; any subsystem caching trust at startup must consume the new reactive path or risk stale decisions.
Persistence/UI separation is introduced; if hooks or commands read the wrong layer, behavior could diverge between effective trust and saved trust.

Code Quality

Architecture appears sound: centralized trust events, atomic persistence, canonical paths, and restricted permissions reduce race/restart risks.
Risk areas without full source review: synchronous trust-selection failure handling, unmount guards for async trust dialogs, fake-MCP latency honoring pre-aborted signals, and fail-closed behavior during canonicalization/discovery races.
Scope discipline is weak for a single-issue PR; large unrelated refactors increase review burden and merge risk.

Tests and Coverage

Coverage impact: likely increase.
Justification: many new trust-specific test files were added across packages/core, packages/cli, packages/mcp, and packages/policy, plus expanded MCP client-manager trust/discovery tests. However, because the full diff and source could not be inspected, I cannot confirm whether new trust paths are fully exercised or if some added tests rely on implementation details.

Verdict

Needs Work. The trust-live implementation appears aligned with #637 and is backed by extensive new tests, but the PR bundles massive unrelated changes, and key trust-related behaviors cannot be fully verified from the available review artifacts. Please split non-trust changes into a separate PR and provide a focused diff/source review for the trust lifecycle, persistence, and MCP wiring paths.

github-actions · 2026-07-13T03:49:01Z

OpenCodeReview — PR #2547

Reviewed head SHA: 7b7887048ef7b26ea749381680c2f4d4a95e429b
Merge base: 5146e50bce083424b7f56cfdbb4e5b26d4331d9a
OCR version: open-code-review v1.6.1 (034d512) linux/amd64 built at: 2026-06-25T12:11:53Z https://github.com/alibaba/open-code-review
Phase: review
Exit code: 0
Run: https://github.com/vybestack/llxprt-code/actions/runs/29416628446
181 finding(s) (0 posted inline).
Artifacts: ocr-review-output contains raw JSON, stdout, stderr, preview, phase, and exit-code diagnostics.

Findings without a resolvable position

packages/cli/src/config/extensions/hookSchema.test.ts: > This test expects the error to contain 'command', but the input { hooks: [{ command: 'echo' }] } is missing the type field, not command. Based on the Zod schema in hookSchema.ts, missing type should produce an error referencing type, not command. This appears to be a copy-paste error from the previous test. Update the assertion to toThrow('type') (or the actual Zod error message for a missing type field).
packages/cli/src/ui/commands/restoreCommand.test.ts: > The variable agentTempDir was renamed from geminiTempDir, but it still points to a .gemini directory path. This creates a misleading abstraction where the name suggests agent-agnostic behavior but the value remains gemini-specific. Either update the directory name to match the new variable intent (e.g., .agent) or retain the original geminiTempDir name to accurately reflect the actual path.
packages/core/src/utils/filesearch/fileSearch.ts: > The provider-neutral naming test still blocks useGeminiignore, but the codebase now uses useExtensionIgnore. This stale assertion will fail as soon as the lint/test rule is re-enabled or another contributor adds the old name, because the canonical API surface has already moved on.
packages/providers/src/__tests__/RetryOrchestrator.failover-budget.test.ts: > The failingProvider mock only implements one overload of generateChatCompletion, while IProvider defines two. If overload-specific behavior is ever needed, this mock could silently hide contract mismatches. Consider annotating the mock to indicate which overload is being satisfied, or use the existing test helpers if they provide partial mock support.
packages/providers/src/__tests__/LoadBalancingProvider.timeout.test.ts: > The input content format was updated to use speaker/blocks, but this output assertion still checks the legacy parts property. If the provider output has also migrated to the blocks format (as seen in the new test providers), chunks[0].parts will be undefined and this assertion will fail. Update to check chunks[0].blocks?.[0] instead.
packages/a2a-server/src/config/config.ts: > The removal of GEMINI_YOLO_MODE fallback and the change from settings.folderTrust === true to settings.folderTrust both appear intentional based on the changelog and test updates. No issues found in this diff. (line 99-111)
packages/a2a-server/src/config/settings.ts: > The deprecation comment for USER_SETTINGS_DIR and USER_SETTINGS_PATH contains a misleading statement: 'computed at load time for testability'. These constants are actually computed at module load time, which is precisely why they are problematic for testability — they capture homedir() before test mocks can override it. The new getUserSettingsPath() function fixes this by computing lazily. Consider updating the deprecation comment to accurately explain why the constants are deprecated: because they are computed at module load time, making them incompatible with mocked homedir() in tests. (line 31-34)
packages/a2a-server/src/config/settings.test.ts: > Consider strengthening the assertion in this test from .not.toBe(true) to .toBeUndefined(). The current assertion passes for both undefined and false, which means a regression where folderTrust incorrectly becomes false would not be caught. Using .toBeUndefined() makes the expected behavior explicit: when neither user nor workspace defines folderTrust, the effective value should remain absent, not coerced to false. (line 77-79)
packages/a2a-server/src/config/config.test.ts: > The environment variable cleanup and mock setup logic is duplicated across all three test cases. Consider extracting this into a shared beforeEach or helper function to follow DRY principles and reduce maintenance overhead when new env vars need to be cleared in the future. (line 73-85)
packages/a2a-server/src/config/mcpServerConfig.ts: > The type field validation here is incomplete: it only accepts 'http' or 'sse', but MCPServerConfig.type supports 'sse' | 'http' | 'streamable-http' (see packages/core/src/config/configTypes.ts). Valid 'streamable-http' values are silently dropped, which can change transport selection downstream. Update the check to accept all declared union values. (line 98-99)
packages/a2a-server/src/config/mcpServerConfig.ts: > This parser uses readStringRecord, which enumerates inherited enumerable properties via Object.entries. For config objects this is usually safe, but if the input inherits enumerable prototype properties, they would be included unexpectedly. Prefer filtering to own properties to avoid surprising inclusion of prototype pollution or inherited fields. (line 14-22)
packages/a2a-server/src/config/mcpServerConfig.ts: > A similar runtime MCP server config parser already exists in packages/cli/src/config/mcpServerConfig.ts, suggesting duplicated parsing logic across packages. If both implementations are needed, consider extracting a shared helper to avoid divergent behavior and duplicated maintenance. (line 131-133)
packages/a2a-server/src/config/extension.test.ts: > The afterEach cleanup assumes beforeEach completed successfully. If createHarness() or vi.mock setup throws, afterEach will attempt fs.rmSync on undefined/stale paths (harness / fakeHome), causing a TypeError that masks the original failure. Consider guarding the cleanup so it only runs when setup succeeded, or initialize these variables to safe defaults. (line 96-100)
packages/a2a-server/src/config/extension.compat.test.ts: > Using fs.symlinkSync with type 'dir' requires elevated privileges or Developer Mode on Windows, which can cause this test to fail in Windows CI environments. Consider guarding with a platform check (e.g., skip on process.platform === 'win32') or using 'junction' type on Windows, which does not require elevated privileges. (line 404-408)
packages/a2a-server/src/config/extension.ts: > Unbounded recursion in hydrateHookValue creates a stack overflow risk. This function recursively processes strings, arrays, and objects without any depth limit. A malicious extension can provide a hooks/hooks.json with deeply nested structures that will exhaust the call stack. Consider adding a depth parameter and rejecting inputs beyond a reasonable threshold (e.g., 100 levels). (line 649-675)
packages/a2a-server/src/config/extension.ts: > logger.error is misused for recoverable warning conditions (broken symlinks, directory enumeration failures, missing config files, parsing errors). These are not errors but warnings. Using error-level logging for warnings will trigger false alerts in monitoring systems. Use logger.warn instead, which is already available on the logger instance. (line 530-532)
packages/a2a-server/src/config/extension.ts: > logger.error is misused for recoverable warning conditions. Use logger.warn for permission-denied directory enumeration failures rather than logger.error with a 'Warning:' prefix. (line 506-508)
packages/a2a-server/src/config/extension.ts: > logger.error is misused for a recoverable condition (missing extension manifest). This should be logger.warn since the extension scan continues with other entries. (line 635-637)
packages/a2a-server/src/config/extension.ts: > logger.error is misused for a recoverable parsing error. Use logger.warn since the extension is skipped but the overall loading process continues. (line 699)
packages/a2a-server/src/config/extension.ts: > logger.error is misused for invalid extension hooks. This is a validation failure for one extension, not a system error. Use logger.warn to avoid false error alerts. (line 761-763)
packages/a2a-server/src/config/extension.ts: > logger.error is misused for a recoverable config parsing error. Use logger.warn since the extension is skipped but loading continues. (line 786-788)
packages/a2a-server/src/config/extension.ts: > logger.error is misused for malformed install metadata. This is a validation failure that causes the extension to be skipped, not a system error. Use logger.warn. (line 860-862)
packages/a2a-server/src/config/extension.ts: > logger.error is misused for malformed fallback install metadata. Use logger.warn since this is a recoverable validation failure for one extension. (line 887-889)
packages/a2a-server/src/config/extension.ts: > The ENOENT race condition is silently swallowed without any diagnostic. While falling back is correct, logging a debug-level message would help diagnose intermittent filesystem issues without producing false error alerts. (line 858-865)
packages/a2a-server/src/config/extension.ts: > Both ${pathSeparator} and ${/} are replaced with identical values (path.sep). Having two different placeholder names for the same concept is confusing and error-prone for extension authors. Consider deprecating one or documenting that they are equivalent. (line 654-660)
packages/agents/src/core/__tests__/subagentOrchestrator-test-helpers.ts: > The repeated as unknown as ... casts bypass TypeScript checking and can hide interface mismatches as the stubbed types evolve. Consider reusing the typed fake objects already present in the repo (e.g., fake scopes/runtime bundles from subagentOrchestrator-runtime.test.ts) or extracting a small typed helper/factory instead of casting inline. (line 38-44)
packages/agents/src/core/__tests__/providerAgnosticNamingAllowlist.ts: > The allowlist paths are already aligned with the scanner's relPath format (e.g. core/src/..., providers/src/...) because providerAgnosticNaming.test.ts derives paths via relative(PACKAGES_DIR, absPath) and strips the packages/ prefix. So the path-base concern is not a current bug, but this coupling is implicit and brittle. To prevent future drift, add a small self-validation test that asserts a representative sample of every allowlist category (GENUINE_GEMINI_TREES, GENUINE_GEMINI_FILES, ALLOWED_GEMINI_PAIRS, etc.) resolves to real files/identifiers in the workspace. Without this, allowlist staleness will only surface as CI failures and will be hard to debug. (line 18)
packages/agents/src/core/chatSession.runtime.timeout.test.ts: > The return() method of the async iterator returns pendingResult, a promise that never resolves. When the stream idle timeout fires and the runtime calls iterator.return() to cancel the stream, it will await this unresolved promise indefinitely. This blocks the iterator cleanup path and can cause the runtime to hang or leak resources. return() should resolve promptly, typically by resolving with { done: true }. (line 72-74)
packages/agents/src/core/turn.abort-timeout.test.ts: > The entire test case verifying post-idle-timeout deadlock recovery ('should allow subsequent calls after idle timeout (sendPromise deadlock prevention)') was removed. While iteratorCleanup.ts now bounds iterator cleanup, this specific deadlock-regression scenario is no longer covered. Please migrate this coverage into another test (e.g., iteratorCleanup.test.ts) so future refactors don't silently reintroduce sendPromise deadlocks. (line 362)
packages/agents/src/core/turn.ts: > Race condition: observedProviderError is unconditionally cleared in onStreamProgress whenever any stream chunk is processed. If the provider emits an error and then a late chunk arrives (or progress continues), the recorded provider error is lost. This can cause the system to report a generic StreamIdleTimeout instead of the underlying provider error, reducing debuggability. Consider only clearing observedProviderError when the stream completes successfully or the error is known to be transient. (line 520-521)
packages/agents/src/core/subagentOrchestrator.test.ts: > The expected default fallback value 1000 is hardcoded in 7 places across this test file. Production code exports this as DEFAULT_UNCONFIGURED_MAX_TURNS from ./subagentOrchestrator.js. Import and use that constant instead of the magic number to prevent test drift if the default ever changes.

import { DEFAULT_UNCONFIGURED_MAX_TURNS } from './subagentOrchestrator.js';
// ...
expect(runConfigArg.max_turns).toBe(DEFAULT_UNCONFIGURED_MAX_TURNS);
``` (line 318)

packages/agents/src/core/turn.ts: > Silent error swallowing: .catch(() =&gt; undefined) after the late-iterator cleanup promise discards any errors from closeIteratorBounded. If cleanup fails, the error is lost without logging or propagation, potentially masking resource leaks. Consider logging cleanup failures or allowing them to surface. (line 888-894)
packages/cli/src/commands/extensions/validate.test.ts: > Test assertions hardcode manifest filenames 'llxprt-extension.json' and 'gemini-extension.json' instead of importing the exported constants EXTENSIONS_CONFIG_FILENAME and EXTENSIONS_CONFIG_FILENAME_FALLBACK from ../../config/extension.js. These constants are already used in the implementation (validate.ts) and are the single source of truth. Using hardcoded strings creates maintenance drift risk — if the manifest names ever change, tests will still pass with stale strings while the implementation changes. Replace hardcoded manifest names in assertions with the imported constants. (line 125-129)
packages/cli/src/commands/extensions/validate.test.ts: > Test assertions hardcode manifest filenames 'llxprt-extension.json' and 'gemini-extension.json' instead of importing the exported constants EXTENSIONS_CONFIG_FILENAME and EXTENSIONS_CONFIG_FILENAME_FALLBACK from ../../config/extension.js. These constants are already used in the implementation (validate.ts) and are the single source of truth. Using hardcoded strings creates maintenance drift risk — if the manifest names ever change, tests will still pass with stale strings while the implementation changes. Replace hardcoded manifest names in assertions with the imported constants. (line 167-171)
packages/cli/src/config/__tests__/folderTrustOriginalSettingsParity.test.ts: > The new test mocks process.cwd() via vi.spyOn but never explicitly restores it. While the outer afterEach calls vi.restoreAllMocks(), relying on that for cleanup is fragile: if this test is later refactored into multiple assertions or if another test is added after it that depends on the real process.cwd(), the mock can leak and cause flaky behavior. Please restore the spy explicitly after the assertions, e.g. with a try/finally or by calling mockRestore() on the spy. (line 355-357)
packages/cli/src/config/extension.ts: > Misleading comment: loadExtensionsFromDir does not deduplicate extensions by name. The comment claims "Extensions are deduplicated by name, with the first occurrence (LLxprt) winning", but this function simply returns all loaded extensions from both roots. Deduplication actually occurs in loadUserExtensions via the uniqueExtensions Map. Since loadExtensionsFromDir is also called directly by annotateActiveExtensions (line 214) without deduplication, this comment could mislead future maintainers into assuming deduplication happens here, potentially causing bugs where duplicate extensions are processed unexpectedly. (line 273-276)
packages/cli/src/config/extension.ts: > The first validation branch for local installations (line 543-547) produces an error message mentioning --autoUpdate even when the user only provided an invalid --ref flag. The message should only mention the flags that were actually provided, or list both --ref and --autoUpdate without implying both were used. Consider splitting into separate validation branches with targeted messages, or changing to a generic message like "The --ref and --autoUpdate flags are only applicable for git-based installations." that accurately describes the restriction regardless of which flag triggered it. (line 543-548)
packages/cli/src/config/extension.ts: > The second validation branch for local installations (line 548-552) includes --allow-pre-release in the error message unconditionally, even when the user only passed --autoUpdate without --allow-pre-release. This produces an inconsistent and confusing error message. The message should reflect the actual invalid flags provided, or use a consistent message that covers all three flags without implying all were used. (line 548-552)
packages/cli/src/config/extensions/hookSchema.test.ts: > These tests only assert that result?.BeforeTool is defined, but do not verify that the optional name and timeout values are actually preserved in the parsed output. A schema that silently drops these fields would still pass these tests. Strengthen the assertions to check the actual values, e.g., expect(result?.BeforeTool?.[0]?.hooks?.[0]?.name).toBe('my-hook') and expect(result?.BeforeTool?.[0]?.hooks?.[0]?.timeout).toBe(5000). (line 89-111)
packages/cli/src/config/extensions/hookSchema.test.ts: > This test name is misleading: JSON.parse creates a regular object with __proto__ as a data property, not actual prototype pollution. The test actually verifies that __proto__ is rejected as a reserved hook name. Consider renaming it to something like 'rejects proto as a hook name from JSON input' to more accurately describe what is being tested. (line 199-204)
packages/cli/src/config/extensions/rootAwareUninstallIdentity.test.ts: > Missing test coverage for non-existent extension uninstall. The test suite verifies behavior for exact matches, case-insensitive fallbacks, and ambiguous cases, but does not test what happens when uninstallExtension is called with a name or source that doesn't match any registered extension. This could hide bugs in error handling or unexpected behavior like silent no-ops. Consider adding a test case that verifies the function throws an appropriate error (or handles gracefully) when given a non-existent extension name/source. (line 69)
packages/cli/src/config/extensions/rootAwareUninstallIdentity.test.ts: > The error message regex /ambiguous|multiple/i is overly broad and could match unintended error messages, leading to false-positive test passes. For example, it would match an error like "Multiple attempts failed" which is unrelated to ambiguity. Consider using a more specific pattern (e.g., /ambiguous.*extension|multiple.*registrations/i) or, preferably, checking for a specific error code or error type if the implementation provides one. (line 137-139)
packages/cli/src/config/extensions/rootAwareUninstallIdentity.test.ts: > The fs.rmSync(tempWorkspaceDir) call in afterEach is redundant because tempWorkspaceDir is a subdirectory of tempHomeDir, which is already being removed recursively. While harmless, this redundant call could be removed for clarity and to avoid potential confusion about cleanup order. (line 63-67)
packages/cli/src/config/extensions/rootAwareUninstallIdentity.test.ts: > The first test is labeled "cross-root" but only creates an extension in the llxprt root, not in the compat root. This makes the "cross-root" label misleading since there's no actual cross-root interaction being tested. Consider either updating the test name/comment to reflect that it's a single-root exact match, or adding a compat root extension to make it a true cross-root test. (line 69)
packages/cli/src/config/extensionsDiscovery.test.ts: > The manual cleanup fs.rmSync(llxprtRoot, { force: true }) at the end of this test case is redundant. The afterEach hook already recursively removes the entire tempRoot directory with force: true, which will clean up all test artifacts including the file created at llxprtRoot. Removing this manual cleanup would reduce noise and avoid implying that special cleanup is required for this test scenario. (line 256-260)
packages/cli/src/config/extensions/hookSchema.test.ts: > The test name suggests it tests prototype pollution, but JSON.parse('{"__proto__":{"command":"pollute"}}') only creates a regular object with a __proto__ data property — it does not actually pollute Object.prototype. The implementation's prototype-chain check (Object.getPrototypeOf) is not exercised by this input. Consider renaming the test to accurately describe what it verifies, e.g., 'rejects proto as a legacy hook name from JSON input', or use an object whose prototype has actually been tampered with if you intend to test prototype pollution protection. (line 199-204)
packages/cli/src/config/extensions/hookSchema.ts: > Missing array input guard: validateHooks accepts arrays because typeof [] === 'object' is true, and Object.entries([]) returns an empty array. This causes the function to silently return an empty hooks object instead of throwing a validation error. Add an explicit Array.isArray(hooks) check at the top of the function to reject arrays and restore the stricter behavior of the previous Zod schema. (line 142-148)
packages/cli/src/config/extensions/rootAwareManagement.test.ts: > The afterEach cleanup deletes tempWorkspaceDir after its parent tempHomeDir has already been recursively deleted. Since tempWorkspaceDir is created as a subdirectory of tempHomeDir (fs.mkdtempSync(path.join(tempHomeDir, 'root-aware-workspace-'))), the second fs.rmSync(tempWorkspaceDir, ...) is redundant. While force: true prevents crashes, this pattern could mask cleanup issues if the directory structure changes in the future. (line 73-77)
packages/cli/src/config/extensions/rootAwareManagement.test.ts: > The vi.mock('os', ...) factory sets homedir: vi.fn(), but this is immediately superseded by vi.spyOn(os, 'homedir').mockReturnValue(tempHomeDir) in beforeEach. The initial vi.fn() is unnecessary and makes the mock setup harder to understand. You can simplify by omitting homedir from the factory mock and letting vi.spyOn create the spy directly. (line 35-41)
packages/cli/src/config/extensions/rootAwareResolver.ts: > resolvePhysicalRegistrationDir does not accept or propagate a workspaceDir parameter. When it delegates to resolvePhysicalRegistrationDirByIdentifier, the fallback process.cwd() is used instead of the workspace context from the originally loaded extension. If this function is called while the process is running from a different directory than the extension's workspace, manifest loading and resolution will be inconsistent, potentially returning the wrong physical registration directory. Consider accepting an optional workspaceDir parameter and passing it through to the identifier resolver. (line 132-139)
packages/cli/src/config/extensions/rootAwareResolver.ts: > Redundant fs.existsSync checks before fs.readdirSync calls add unnecessary filesystem overhead. Since readdirSync already throws ENOENT for missing directories, and the subsequent try-catch handles that error, the existsSync guard is superfluous. Removing these checks simplifies the code and reduces filesystem calls. (line 66-79)
packages/cli/src/config/extensions/rootAwareResolver.ts: > tryLoadExtension silently swallows all errors and returns null without any logging. While the comments explain this is intentional to prevent one broken extension from aborting the scan, the complete lack of error reporting makes it nearly impossible for users to diagnose why an extension isn't being found or why it's being skipped. Consider adding at least debug-level logging so troubleshooting is possible without affecting normal operation. (line 48-57)
packages/cli/src/config/extensions/rootAwareManagement.test.ts: > The afterEach cleanup deletes tempWorkspaceDir after its parent tempHomeDir has already been recursively deleted. Since tempWorkspaceDir is created as a subdirectory of tempHomeDir (fs.mkdtempSync(path.join(tempHomeDir, 'root-aware-workspace-'))), the second fs.rmSync(tempWorkspaceDir, ...) is redundant. While force: true prevents crashes, this pattern could mask issues if the directory structure changes in the future. Consider removing the redundant deletion. (line 73-77)
packages/cli/src/config/extensions/rootAwareManagement.test.ts: > The vi.mock('os', ...) factory sets homedir: vi.fn(), but this is immediately superseded by vi.spyOn(os, 'homedir').mockReturnValue(tempHomeDir) in beforeEach. The initial vi.fn() in the factory is unnecessary and makes the mock setup harder to understand. You can simplify by omitting homedir from the factory mock and letting vi.spyOn create the spy directly. (line 35-41)
packages/cli/src/services/FileCommandLoader.test.ts: > Unused imports: afterEach and beforeEach are imported from 'vitest' but never used in the test file. This will likely cause lint warnings. Please remove these unused imports to keep the code clean. (line 16-24)
packages/cli/src/config/trustedFolders.test.ts: > Multiple tests replace debugLogger.warn with a no-op via vi.spyOn(debugLogger, 'warn').mockImplementation(() =&gt; {}), but they don’t restore or clear the spy afterward. If other tests in this suite (or in vitest worker threads) also rely on debugLogger.warn, this can cause cross-test pollution and flaky expectations. Consider restoring the original implementation in afterEach, or using mockReturnValue/mockClear to keep warnings observable outside the exact assertions that need them. (line 356)
packages/cli/src/config/settingsLoader.trust.test.ts: > The variable osActual is misleadingly named. Because Vitest hoists vi.mock calls above imports, osActual actually references the mocked os module, not the real one. The real os module is correctly obtained via vi.importActual as realOs. Consider renaming osActual to mockOs to avoid confusion for maintainers who might assume they are mutating the real os.homedir. (line 45-46)
packages/cli/src/config/settingsLoader.trust.test.ts: > In exposeTrustRules, fs.PathLike values from fs.existsSync and fs.PathOrFileDescriptor from fs.readFileSync are compared with string constants using strict equality (===). While this works because the production code always passes strings, the comparison is type-unsafe and would silently return '{}' if a Buffer or URL were ever passed. Consider normalizing the path parameter with String(path) before comparison to make the mock resilient to type changes. (line 91-110)
packages/cli/src/nonInteractiveCliSupport.ts: > return before a throw is dead code because throw never returns. In the switch cases below, remove the unnecessary return to keep the control flow clear and avoid misleading readers. (line 579)
packages/cli/src/ui/components/DialogManager.tsx: > The isRestarting prop is no longer passed to FolderTrustDialog after this change. Before removing it, please verify that FolderTrustDialog still receives restart state through another mechanism (e.g., context, config, or a different prop). If isRestarting is truly unused, remove it from FolderTrustDialog's props to avoid dead code and type mismatches. (line 242-245)
packages/cli/src/ui/commands/restoreCommand.test.ts: > Inconsistent variable naming: agentTempDir suggests an agent-agnostic directory, but it still points to the hardcoded .gemini path. This creates a misleading abstraction. Either update the directory name to match the new variable intent (e.g., .agent) or retain the original geminiTempDir name to accurately reflect the actual path. (line 31)
packages/cli/src/ui/commands/permissionsCommand.test.ts: > The global vi.mock('node:process', ...) for cwd is broader than the verified usage here. Neither permissionsCommand.ts nor the adjacent command modules in this directory call process.cwd() at module scope, so this mock can leak into unrelated tests if any transitive dependency calls process.cwd() inside functions. Consider limiting the mock scope to beforeEach/afterEach, or restoring it explicitly, to reduce cross-test contamination. (line 27-33)
packages/cli/src/config/trustedFolders.ts: > resolvePathTrust performs resolveCanonicalPath() on every rule path on every call, and aliasesFor() similarly re-resolves every config key. This causes repeated synchronous filesystem operations in hot paths. Consider memoizing/caching the canonical paths when LoadedTrustedFolders is constructed, since rules are loaded once and then reused. (line 121-170)
packages/cli/src/config/trustedFolders.ts: > resolveLocalWorkspaceTrust does not validate trustedFolders.errors, unlike the previous getWorkspaceTrustFromLocalConfig which threw on malformed configuration. If callers pass a LoadedTrustedFolders with errors, this will silently return undefined instead of surfacing the configuration problem, which can lead to incorrect trust decisions. (line 420-429)
packages/cli/src/ui/components/FolderTrustDialog.tsx: > Consider replacing the void Promise.resolve().then().catch().finally() pattern in handleSelect with an explicit async function using try/catch/finally. It preserves the same behavior while improving readability and debugging. (line 104-128)
packages/cli/src/ui/hooks/agentStream/__tests__/streamRuntimeTestHelper.ts: > This renames the mock runtime property from getGeminiDir to getLlxprtDir. If this reflects a real interface/contract rename, please verify the matching type/interface, the mock source contract, and all consumer call sites are updated consistently; otherwise this will become a runtime or type mismatch in tests. (line 168)
packages/cli/src/ui/hooks/permissionsModifyTrustDialog.behavior.test.tsx: > Found a non-standard Vitest matcher: toHaveBeenCalledExactlyOnceWith is not a built-in assertion in Vitest or Jest. This will throw TypeError: expect(...).toHaveBeenCalledExactlyOnceWith is not a function at runtime and cause the test to fail. Replace it with the standard toHaveBeenCalledOnceWith matcher. (line 399-402)
packages/cli/src/ui/hooks/useAutoAcceptIndicator.test.ts: > The agentStub.setApprovalMode(ApprovalMode.DEFAULT) call here is a no-op: its mock implementation would set getApprovalMode to return DEFAULT, but the very next line immediately overrides it to return YOLO. This makes the test harder to read and understand. You can remove the setApprovalMode call since it has no effect on the test outcome. (line 248-249)
packages/cli/src/ui/hooks/useAutoAcceptIndicator.test.ts: > This test name is misleading. The hook does not call agent.setApprovalMode() to restore the agent's approval mode; it only updates the local indicator state by reading agent.getApprovalMode(). Consider renaming it to something like 'restores the indicator when folder trust is regained' to accurately reflect the behavior being tested. (line 257)
packages/cli/src/ui/trustDialogHelpers.test.ts: > The new test file imports TrustLevel from ../config/trustedFolders.js, so ensure that module's export path/resolution remains stable after the refactor. Also note that this test file currently hardcodes the same user-facing strings as the implementation; if these labels are later localized, the tests will need adjustment or a shared constants module. (line 8-21)
packages/core/src/config/config.folderTrustLive.test.ts: > This InitializedConfig subclass directly sets this.initialized = true to bypass initialization checks. Because initialized is an internal/implementation detail, this couples the test to a protected flag and makes it fragile if the initialization mechanism changes. Consider using a factory helper or public initialize()/ensureInitialized() path in test setup instead of mutating internal state in a test-only subclass. (line 22-27)
packages/cli/src/ui/hooks/usePermissionsModifyTrust.ts: > Race condition: when an async commit succeeds, this unconditionally overwrites pendingTrustLevel with the committed value. If the user changed the selection while the commit was in flight, the UI will jump back to the older value. Capture the current pending level at the start of runCommit and only apply the committed value if it still matches what the user last selected. (line 226-245)
packages/cli/src/ui/hooks/useFolderTrust.test.ts: > The 'does not report or exit for a stale selection after unmount' test awaits the promise returned by handleFolderTrustSelect without explicitly asserting it resolves. If the hook implementation were changed to propagate errors post-unmount instead of swallowing them, this test would fail with an unhandled rejection at await selection before reaching the final assertions that verify no error reporting and no process.exit occur. Wrap the await with an explicit assertion to make the test's intent robust against implementation changes: await expect(selection).resolves.toBeUndefined(); (line 431-435)
packages/core/src/config/config.ideTrustLive.test.ts: > Remove the unnecessary optional chaining on resolveIdeClient and add a comment explaining that the empty object simulates a listener-less IDE client. After vi.waitFor(() =&gt; expect(getIdeClient).toHaveBeenCalledOnce()), resolveIdeClient is guaranteed to be defined, so using resolveIdeClient({}) provides clearer failure semantics if the expectation is violated for any reason. (line 79-82)
packages/core/src/config/config.ideTrustLive.test.ts: > Add expect(listener).toBeDefined() before invoking the listener. Without this assertion, listener?.(false) would silently do nothing if listener is undefined, causing expect(() =&gt; listener?.(false)).not.toThrow() to pass incorrectly and potentially masking listener registration failures. (line 142-148)
packages/core/src/config/config.ts: > Removing GeminiCLIExtension from the module exports is a breaking API change. I searched the codebase and found that packages/agents/src/core/__tests__/providerAgnosticNaming.test.ts explicitly verifies this type no longer exists, but before removing it from the public interface you should confirm no external consumers import it from this module. If there are downstream packages relying on this export, this change will cause compilation failures. Consider keeping the export as a deprecated alias, or verifying there are no external consumers first. (line 75-77)
packages/core/src/config/config.folderTrustMcpWiring.test.ts: > The HookSystem mock is incomplete. The test only implements initialize, dispose, isInitialized, getRegistry, and getEventHandler, but HookSystem exposes additional public methods such as register, unregister, trigger, and event-specific trigger methods. If the trust transition or disposal path calls any of these methods, the mock will throw 'not a function', causing test failures or masking real behavior. Please add no-op stubs for all public HookSystem methods that the production code might invoke during trust changes. (line 46-52)
packages/core/src/config/config.folderTrustMcpWiring.test.ts: > This test asserts on internal implementation details (mock.invocationCallOrder) to verify serialization order. This is brittle because internal ordering logic can change during refactoring without affecting the external contract, leading to unnecessary test maintenance and false failures. Consider verifying the observable outcome (e.g., the final trust state and side-effect counts) rather than the exact call sequence. (line 267-269)
packages/core/src/config/config.folderTrustMcpWiring.test.ts: > The HookSystem mock is incomplete. The test only implements initialize, dispose, isInitialized, getRegistry, and getEventHandler, but HookSystem exposes additional public methods such as setHookEnabled, getAllHooks, and multiple fire* event methods. If the trust transition or disposal path calls any of these methods, the mock will throw 'not a function', causing test failures or masking real behavior. Please add no-op stubs for all public HookSystem methods that the production code might invoke during trust changes. (line 46-52)
packages/core/src/config/config.folderTrustMcpWiring.test.ts: > This test asserts on internal implementation details (mock.invocationCallOrder) to verify serialization order. This is brittle because internal ordering logic can change during refactoring without affecting the external contract, leading to unnecessary test maintenance and false failures. Consider verifying the observable outcome (e.g., the final trust state and side-effect counts) rather than the exact call sequence. (line 267-269)
packages/core/src/policy/config.test.ts: > In the filter predicate, rule.source?.startsWith("Settings (MCP") ?? false contains redundant nullish coalescing. Array.prototype.filter already treats undefined as falsy, so ?? false does nothing and reduces readability. Prefer Boolean(rule.source?.startsWith("Settings (MCP")) or simply rule.source?.startsWith("Settings (MCP") since filter will drop the undefined results anyway. (line 241)
packages/core/src/utils/filesearch/ignore.ts: > This diff renames the ignore option from useGeminiignore to useExtensionIgnore, but this reference in providerAgnosticNaming.test.ts was missed. That will cause a compile/test error unless it is updated to useExtensionIgnore to match the new option name. (line 23)
packages/core/src/policy/config.ts: > Security-critical conditional gating: addMcpTrustedRules is now skipped entirely when trustedFolder is false. This means an untrusted folder silently loses MCP trusted allow rules rather than explicitly denying them, which may be acceptable, but it also means any bug in computing trustedFolder (e.g., undefined or stale context state) will silently downgrade the security posture. Ensure every caller computes this flag correctly, document that undefined defaults to false, and add a log/telemetry event when trusted MCP rules are suppressed so the decision is auditable. (line 171-173)
packages/core/src/policy/config.test.ts: > This test asserts an exact array order (excluded, trusted, allowed) via toStrictEqual. While the current buildSettingsRules implementation happens to push rules in that sequence, rule ordering is still an implementation detail rather than a stable contract. If future changes insert another rule category between excluded and trusted/allowed, this test will break even if the generated rules are semantically correct. Consider using order-independent assertions (e.g., sorting both arrays before comparison, or checking membership with toContainEqual) unless rule order is explicitly part of the public contract. (line 239-256)
packages/mcp/src/client/mcp-client-manager-helpers.ts: > The removeMcpServerState function mutates the failures array parameter via appendFailures as a side effect, which is not evident from the function signature. This could lead to unexpected behavior if callers don't anticipate the array modification. Consider returning the failures or documenting the mutation clearly. Additionally, removeMcpServerArtifacts uses a different error handling pattern (throwing AggregateError directly) creating inconsistency between the two related cleanup functions. (line 111-124)
packages/mcp/src/client/mcp-client-manager-helpers.ts: > The waitForMcpRefreshDebounce function contains a hardcoded 300ms debounce duration. Extracting this to a named constant (e.g., MCP_REFRESH_DEBOUNCE_MS) would improve maintainability and make the debounce behavior more discoverable and configurable. (line 58-73)
packages/core/src/hooks/hookSystem.ts: > runInitializationGenerations() can enter an infinite loop if dispose() is called while initialization is still in flight or queued. The while loop advances completedGeneration only on success, but dispose() increments initializationGeneration, so the loop will keep retrying a generation that immediately fails the disposed check and never reaches the exit condition. If dispose() is invoked from another callback during initialization, this Promise will never settle and can hang the process. Consider breaking out when lifecycleGeneration changes, e.g., by checking whether the current generation is still the latest before each retry, or by guarding the loop with a disposed flag. (line 129-144)
packages/core/src/hooks/hookSystem.ts: > The old explanatory comment about LLxprt-specific MessageBus subscription leaks was removed from the new initialize() path. Because disposeEventHandler() is now called here before registry.initialize(), that context is still relevant for future maintainers. Please restore or relocate the comment so the rationale for disposing the previous handler is not lost. (line 154-162)
packages/core/src/utils/output-format.ts: > The formatError method accepts error: Error, but standard JavaScript Error objects do not have status, category, or reason properties. The runtime checks in getSafeStatus/getSafeCategory/getSafeReason safely handle missing properties, but the TypeScript signature is misleading and may cause type confusion for callers. Consider updating the signature to accept a broader error type, such as unknown or a dedicated structured error interface, to accurately reflect the method's capabilities. (line 180)
packages/core/src/utils/output-format.ts: > The safe property extraction pattern (typeof error !== 'object' || error === null || !('prop' in error)) is duplicated across getSafeStatus, getSafeCategory, and getSafeReason. Consider extracting this into a single generic helper function (e.g., getSafeProperty&lt;T&gt;(obj: unknown, key: string, validator: (val: unknown) =&gt; val is T)) to reduce duplication and make future property additions easier. (line 126-168)
packages/mcp/src/client/mcp-client-manager.partial-failure.test.ts: > This new test relies on McpClient being called twice in a specific sequential order via chained mockReturnValueOnce, but startConfiguredMcpServers() launches discovery for all configured servers concurrently with Promise.all. Concurrent constructor calls mean the mock returns are consumed in nondeterministic order depending on runtime scheduling/object enumeration, so the test is fragile and can silently pass with swapped behaviors or fail spuriously. Make the test order-independent by separating the mock instances by server name (e.g., mockImplementation((name) =&gt; goodClient/badClient)) or use distinct mock constructors per server. (line 43-45)
packages/mcp/src/client/mcp-client-manager.test.ts: > The previous test case covering partial failure scenarios (mixed success/failure where discovery reaches COMPLETED despite individual server failures) was removed and replaced with this timeout-recovery test. These are different behaviors: this new test validates that a timeout failure clears when discovery later succeeds, but it does not cover the scenario where some servers fail permanently while others succeed. Consider retaining or adding a test for partial failure to ensure getDiscoveryState() still returns COMPLETED and successful servers remain usable when others fail. (line 848)
packages/mcp/src/client/mcp-client-manager.test.ts: > The rejectFirstRefresh variable is initialized with a no-op function () =&gt; {} but is immediately overwritten in the Promise constructor. Additionally, the type annotation (error: Error) =&gt; void does not match the zero-argument default. This is harmless but confusing; consider using let rejectFirstRefresh!: (error: Error) =&gt; void; to defer initialization without a mismatched default. (line 240)
packages/mcp/src/client/mcp-client.resource-refresh.test.ts: > The mock setup for Client and StdioClientTransport is duplicated across all four test cases. Extract this into a shared helper or beforeEach block to reduce repetition and ensure consistent mocking. (line 106-111)
packages/mcp/src/client/mcp-client.discovery.test.ts: > The new test 'aborts initial resource discovery when disconnected' uses a mocked ClientLib.Client that only implements request, but discover() also calls listTools (and listPrompts when enabled) before resource discovery. Because listTools is not mocked here, the test will throw TypeError: mockedClient.listTools is not a function before the abort behavior can be asserted. Either mock listTools/listPrompts or configure the mock server capabilities so those discovery steps are skipped. (line 120-137)
packages/mcp/src/client/mcp-client.stale-error.test.ts: > The second test ('ignores errors emitted by a stale SDK client after reconnect') does not actually simulate a stale SDK client error because staleSdkClient.onerror is undefined (initialized as undefined in createSdkClient()). The line staleErrorHandler?.(new Error('stale connection lost')) is a no-op due to optional chaining, meaning the test passes without exercising any error handling logic. This creates a false positive that gives incorrect confidence in the stale error handling behavior. (line 101-105)
packages/mcp/src/client/mcp-client.stale-error.test.ts: > The second test ('ignores errors emitted by a stale SDK client after reconnect') does not actually simulate a stale SDK client error because staleSdkClient.onerror is undefined (initialized in createSdkClient()). The line staleErrorHandler?.(new Error('stale connection lost')) is a no-op due to optional chaining, meaning the test passes without exercising any error handling logic. This creates a false positive that gives incorrect confidence in the stale error handling behavior. (line 101-105)
packages/mcp/src/client/mcp-client.stale-error.test.ts: > The first test ('closes and forgets an SDK client when handler registration fails') creates mocks for removeMcpToolsByServer, removePromptsByServer, and removeResourcesByServer but never asserts they were called. If the implementation forgets to call these cleanup methods during error handling, the test would not detect resource leaks. Consider adding assertions like expect(toolRegistry.removeMcpToolsByServer).toHaveBeenCalledWith('test-server') to verify proper cleanup. (line 90-94)
packages/mcp/src/client/mcp-client-manager.ts: > In the catch block, coreEvents.emitFeedback(...) can throw synchronously if a listener throws, which would prevent resolve() from executing and leak the promise in discoveringServers. This breaks the whenDiscoverySettled() contract and can cause stale discovery state during shutdown or trust transitions.

Fix: Wrap emitFeedback in a try-catch so the promise always resolves. (line 246-260)

packages/mcp/src/client/mcp-client-manager.ts: > After configured and extension discovery completes, discoveryState is never set to COMPLETED and no final CoreEvent.McpClientUpdate event is emitted. Consumers such as slashCommandHandlers.ts check getDiscoveryState() === MCPDiscoveryState.IN_PROGRESS, so the UI will remain in a loading state even after discovery finishes.

Fix: Set this.discoveryState = MCPDiscoveryState.COMPLETED and emit a final update event before returning. (line 666-675)

packages/mcp/src/client/mcp-client.ts: > Use closeClientWithTimeout(failedClient, this.serverName) instead of failedClient.close().catch(() =&gt; {}). The current fire-and-forget close() is inconsistent with disconnect() and risks process hangs if the SDK client stalls during shutdown. (line 181)
packages/mcp/src/client/mcp-discovery.latency.test.ts: > These tests rely on hardcoded call counts (9, 12, 1) to the isTrustedFolder mock to trigger revocation at specific points in connectAndDiscover. This is tightly coupled to the current internal call sequence of connectAndDiscover and will become brittle if that sequence changes. Consider modeling trust revocation by explicit phase/state transitions (e.g., beforePublish, duringToolRegistration, afterConnect) or by controlling the mock based on invocation order/naming, so the tests remain stable and self-documenting. (line 43-55)
packages/mcp/src/client/mcp-discovery.authorization.test.ts: > Inconsistent assertion: this test uses the raw string literal 'disconnected' instead of the MCPServerStatus.DISCONNECTED enum constant. All other tests in this file correctly use the enum constant, and this inconsistency creates maintenance risk if the enum value ever changes. Please replace 'disconnected' with MCPServerStatus.DISCONNECTED for consistency. (line 786-789)
packages/mcp/src/client/mcp-discovery.authorization.test.ts: > Inconsistent assertion: this test uses the raw string literal 'disconnected' instead of the MCPServerStatus.DISCONNECTED enum constant. All other tests in this file correctly use the enum constant, and this inconsistency creates maintenance risk if the enum value ever changes. Please replace 'disconnected' with MCPServerStatus.DISCONNECTED for consistency. (line 786-789)
packages/mcp/src/client/retryable-client-disconnections.ts: > activate is currently a redundant wrapper around retire—both methods execute identical logic. This is misleading because the name activate implies distinct behavior (e.g., clearing failed status while preserving pending state). Either remove activate and call retire directly, or implement it with different semantics if it is intended to selectively clear only the failed state. (line 18-20)
packages/mcp/src/client/mcp-status.ts: > Consider adding a brief JSDoc for the new failures parameter explaining that listener errors are collected into it and that when omitted, errors still propagate. (line 78-85)
packages/mcp/src/client/retryable-client-disconnections.ts: > activate currently has identical behavior to retire—both clear pending and failed for the given client. This makes activate either redundant dead code or a misleading API, since its name suggests distinct semantics (e.g., selectively clearing only the failed state). If activate is intended to have different behavior from retire, implement it accordingly; otherwise, remove it and call retire directly. (line 18-20)
packages/mcp/src/client/retryable-client-disconnections.ts: > activate is identical to retire—both clear pending and failed for the same client. This makes activate either dead code or misleading. If it's intended to have distinct behavior (e.g., only clearing the failed state), implement that separately; otherwise, remove it and call retire directly. (line 18-20)
packages/mcp/src/client/retryable-client-disconnections.ts: > A successful disconnect does not remove the client from pending. Once disconnect() resolves, the WeakMap entry is never cleaned up. Add this.pending.delete(client); in the success handler so completed disconnections are cleaned up, preventing unbounded growth of the pending map. (line 31-34)
packages/providers/src/BaseProviderNormalization.ts: > Add a brief comment documenting the signal-precedence rule here. The change introduces metadata?.abortSignal as a fallback for invocation.signal, which is a meaningful semantic shift. Future readers should not have to infer whether this fallback is intentional. (line 263-267)
packages/policy/src/toml-loader.ts: > The extracted resolveToolMatcher correctly preserves the previous tool-matching behavior: when neither mcpName nor toolName is provided, it omits both matcher properties, and the updated policy engine already handles that case with matchesAllTools. The refactor is consistent with the downstream matching changes in policy-engine.ts, so this appears safe. (line 356-369)
packages/policy/src/policy-engine.test.ts: > These new tests accurately reflect the current matching semantics: toolName is exact-match only, toolNamePrefix uses startsWith, and * in toolName is treated literally. No issue here; this is good behavioral coverage. (line 220)
packages/mcp/src/fake/fakeMcpDiscovery.ts: > The disconnectFakeServer helper uses a bare try/finally around toolRegistry.removeMcpToolsByServer(name) without catching potential errors. If tool removal throws (e.g., due to registry corruption, concurrent modification, or a missing server entry), the error propagates to the caller, but the server status is still updated to DISCONNECTED in the finally block. This creates an inconsistent state where stale tools may remain registered while the status indicates disconnection. Consider adding a catch block that logs the error and still proceeds with status cleanup, or decide whether tool removal failures should be treated as fatal discovery failures. (line 218-227)
packages/policy/src/policy-engine.test.ts: > Tests for removeRulesBySource only assert evaluation results or total rule counts, not the exact remaining rules. For example, after removing 'Settings (MCP Trusted)', this test checks that allowed-tool still evaluates to ALLOW, but does not verify that trusted-server__tool was actually removed and not replaced by another rule. Consider asserting engine.getRules() contents directly to catch accidental removal of wrong rules. (line 730)
packages/providers/src/__tests__/RetryOrchestrator.invocation.test.ts: > The assertions expect(normalized!.invocation.signal?.aborted).toBe(true) appear incorrect for normal execution flow. A freshly created linked AbortController is not aborted until its parent signal fires. If the intent is to verify signal cloning/linkage, this should either trigger abortController.abort() before the provider call, or assert the relationship/type instead of the aborted state. (line 153-154)
packages/providers/src/__tests__/RetryOrchestrator.invocation.test.ts: > Same issue: asserting aborted === true without first aborting the parent controller will likely cause this test to fail. If this is meant to verify that the new signal reacts to parent aborts, the parent should be aborted before this expectation, or the test should assert the cloning/linking contract instead. (line 270-271)
packages/providers/src/__tests__/LoadBalancingProvider.retryBoundary.integration.test.ts: > Scenario B asserts counter.value === maxAttempts, but with the new architecture RetryOrchestrator bounds transport attempts at the outer level while LoadBalancingProvider still rotates through multiple backends on each transport attempt. Each RetryOrchestrator attempt can therefore invoke up to NUM_BACKENDS delegate calls, so the total delegate invocations should still be maxAttempts * NUM_BACKENDS, not maxAttempts. Either the expected assertion needs to remain maxAttempts * NUM_BACKENDS, or RetryOrchestrator must be changed to suppress internal backend rotation; as written this assertion will fail on every run. (line 260)
packages/providers/src/__tests__/LoadBalancingProvider.retryBoundary.integration.test.ts: > This assertion will fail. LoadBalancingProvider.executeWithFailover still iterates through backends on each transport attempt (while (visitedCount &lt; numProfiles &amp;&amp; hasTransportAttemptRemaining(options))), and each backend iteration calls the delegate provider once. RetryOrchestrator bounds the total transport attempts, but each attempt still performs a full backend rotation. For maxAttempts = 3 and NUM_BACKENDS = 3, the expected delegate invocation count should remain maxAttempts * NUM_BACKENDS, not maxAttempts. Update the assertion to expect counter.value === maxAttempts * NUM_BACKENDS, or confirm that RetryOrchestrator is intended to suppress backend rotation entirely. (line 260)
packages/providers/src/__tests__/RetryOrchestrator.timeoutCleanup.test.ts: > This test uses a streamingTimeoutMs: 50 timeout, which is quite aggressive. In RetryOrchestrator.streamWithTimeout, timeout enforcement depends on Promise.race against delay(timeoutMs, ...), and because timeout only applies to the first chunk, the test’s 50ms window has very little buffer. On slow CI runners, under heavy event-loop load, or if iterator.next() scheduling is delayed, the provider generator might not register the abort before the test asserts, causing flaky failures. Consider increasing the timeout to something more forgiving (e.g., 200–500ms) to make the test robust without changing behavior. (line 64-68)
packages/providers/src/__tests__/RetryOrchestrator.timeoutCleanup.test.ts: > This test is named as if it verifies timeout behavior, but streamingTimeoutMs: 1_000 never actually fires because the provider throws immediately after yielding its only chunk. The test really validates “stream error after partial yield should not retry”, not timeout-specific cleanup. The name is misleading for future maintainers. If the intent is to test the timeout-then-cleanup path, the provider should hang on the first chunk until the timeout expires. If the intent is to test post-yield error handling, rename the test to something like does not retry after a stream yields partial content and then errors to accurately reflect what is being asserted. (line 95)
packages/providers/src/__tests__/LoadBalancingProvider.settings-merge.test.ts: > Consider moving this signal preservation test to a more appropriately named file (e.g., LoadBalancingProvider.abort.test.ts or alongside timeout tests in LoadBalancingProvider.timeout.test.ts) since this file is dedicated to settings merge behavior. (line 419)
packages/providers/src/__tests__/LoadBalancingProvider.settings-merge.test.ts: > Add an assertion on the streamed response content after abort to ensure the delegate's streaming path still functions correctly end-to-end, not just the signal state. (line 477-482)
packages/providers/src/anthropic/AnthropicProvider.caching.test.ts: > The mocked ToolFormatter keys no longer match the module’s actual exports. The old Gemini-specific names were replaced with generic ones here, but this test (and likely others) still needs to mock the real exported functions. If ToolFormatter.js does not export convertToolDeclarationsToAnthropic/convertToolDeclarationsToFormat, this mock will be ignored and can lead to real implementations being called during tests. Please align these mock keys with the actual exported function names from ToolFormatter.js, and update any other tests that still reference the old names. (line 67-68)
packages/providers/src/anthropic/AnthropicProvider.messaging.test.ts: > The test mocks convertToolDeclarationsToAnthropic and convertToolDeclarationsToFormat, but schemaConverter.ts currently exports convertToolsToAnthropic and no convertGeminiToFormat/convertToolDeclarationsToFormat. Because the mock names do not match the module's actual exports, this mock becomes a no-op and the real implementation runs during this test. Please update the mock names to the actual exported identifiers (and remove any mismatched mock entries) so the test behavior is predictable. (line 65-66)
packages/providers/src/__tests__/retryInfrastructure.behavior.test.ts: > The test 'bounds detached primitive error deduplication' hardcodes 64 as the deduplication limit, but the implementation uses MAX_HANDLED_PRIMITIVE_ERRORS = 32. This will cause the test expectation to diverge from actual behavior. Update the test to import and use the same constant so the assertion remains aligned with implementation changes. (line 335-339)
packages/providers/src/anthropic/AnthropicProvider.messaging.test.ts: > This test mocks convertToolDeclarationsToAnthropic and convertToolDeclarationsToFormat, but the current schema converter module exports convertToolsToAnthropic and does not expose a convertGeminiToFormat/convertToolDeclarationsToFormat alias. If the mocked consumer imports these symbols directly from the schema module, the mock is effectively a no-op and the real implementation still runs. Please align the mock names with the actual exported identifiers, or verify these symbols are re-exported elsewhere. (line 65-66)
packages/providers/src/anthropic/AnthropicStreamProcessor.ts: > The previous hasYieldedContent guard prevented retrying a stream after partial content had already been yielded. After removing that guard from processStreamEvents and handleMessageDelta, this file no longer protects against partial-yield retries. If the central retry layer retries Anthropic streaming after an error, consumers can receive duplicated or inconsistent content. Please ensure the external retry path enforces an equivalent boundary before re-entering processAnthropicStream on a failed stream. (line 98-101)
packages/providers/src/loadBalancing/delegateAttempt.ts: > In cleanupDelegateAttempt, when the stream throws AND dispose() also throws, the cleanup error is silently dropped because requestFailed is checked first. This masks resource leaks or incomplete cleanup, which is especially important for MCP client lifecycle management.

Fix: check cleanupFailure first so cleanup errors are never swallowed.
if (cleanupFailure !== undefined) throw cleanupFailure;
if (requestFailed) throw requestFailure;
``` (line 61-62)

packages/providers/src/loadBalancing/failoverSettings.ts: > This comment was originally for shouldFailover but is now positioned above permitsLoadBalancerFailover. Move it back above shouldFailover and add a new comment for this function explaining its specific logic. (line 67-70)
packages/providers/src/loadBalancing/failoverSettings.ts: > Extract the magic string 'retries_exhausted' to a named constant for maintainability. (line 75)
packages/providers/src/loadBalancing/failoverSettings.ts: > Add a comment explaining why non-retries_exhausted errors always permit failover, while retries_exhausted errors require isRetryable: true. Also document that missing isRetryable defaults to false (no failover). (line 70)
packages/providers/src/auth/BucketFailoverHandlerImpl.ts: > tryFailover rejects instead of returning false when already aborted. When signal.aborted is true, raceWithAbort(Promise.resolve(false), signal) rejects with an abort error, but the method signature promises Promise&lt;boolean&gt;. This changes the observable behavior: callers expecting a boolean result will instead receive a rejection.

Fix: return Promise.resolve(false) directly when already aborted, or wrap the rejection into a resolved false.

async tryFailover(context?: FailoverContext): Promise&amp;lt;boolean&amp;gt; {
  const signal = context?.signal;
  if (isAborted(signal)) {
    return Promise.resolve(false);
  }
  return raceWithAbort(this.tryFailoverInternal(context), signal);
}
``` (line 153-159)

packages/providers/src/auth/BucketFailoverHandlerImpl.ts: > waitForEagerAuth does not accept the abort signal, so the failover handler remains blocked on eager-auth waits even after the overall operation has been cancelled. If ensureBucketsAuthInFlight is a long-running promise, the handler will not return until it settles.

Fix: pass signal into waitForEagerAuth and race the await against it, so the wait can be interrupted on abort.

private async waitForEagerAuth(
  candidateBucket: string,
  waitingMessage: string,
  failureMessage: string,
  signal: AbortSignal | undefined,
): Promise&amp;lt;void&amp;gt; {
  const eagerAuthInFlight = this.ensureBucketsAuthInFlight;
  if (!eagerAuthInFlight) return;

  logger.debug(
    `${waitingMessage} (provider=${this.provider}, bucket=${candidateBucket})`,
  );
  try {
    await raceWithAbort(eagerAuthInFlight, signal);
  } catch (error) {
    if (isAbortError(error)) return;
    logger.debug(`${failureMessage} for ${candidateBucket}:`, error);
  }
}

Also update the three call sites in tryPassThreeForegroundReauth to pass signal. (line 539-555)

packages/providers/src/errors.ts: > The status property was added to LoadBalancerFailoverError, which contradicts the previous design decision documented in the removed JSDoc comment. The old comment explicitly warned that exposing an HTTP status on the aggregate error would cause retryWithBackoff to mis-route into bucket-failover paths. Please verify that all retry/failover consumers of this error correctly handle the new status property and do not incorrectly treat the aggregate as a 429 response. (line 219-221)
packages/providers/src/errors.ts: > Error message formatting for LoadBalancerFailoverError and AllBucketsExhaustedError now uses summarizeProviderLabels and truncates failure details to 3 entries (MAX_DISPLAYED_FAILURES = 3). This will break any tests asserting on exact error message strings and may hide diagnostic information in logs. Please ensure all related tests are updated to match the new format, and verify that the truncation limit is appropriate for production diagnostics. (line 311-323)
packages/providers/src/errors.ts: > AllBucketsExhaustedError now calculates status via getEffectiveProviderStatus instead of direct extraction. This function can derive a synthetic status (e.g., 429 for rate_limit category) when the original error lacks one, which may change downstream retry/failover behavior compared to the previous undefined status. Please confirm this behavioral change is intended and covered by tests. (line 530-538)
packages/providers/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts: > The second test case has a race condition that makes the assertion expect(oauthManager.getTokenStore).not.toHaveBeenCalled() unreliable. Inside retryWithBackoff, when the thrown error is classified as a 429, attemptFailover synchronously invokes handler.tryFailover(...) before the microtask queue runs. BucketFailoverHandlerImpl.tryFailoverInternal then calls classifyTriggeringBucket → readStoredToken, which calls oauthManager.getTokenStore() synchronously before any await that would allow the queued controller.abort() to take effect. As a result, getTokenStore is invoked before the AbortError is thrown, so the assertion will fail in practice. Consider asserting the intended outcome instead—e.g., that the promise rejects with AbortError and that failover state did not change—rather than asserting that a synchronous setup call was skipped. (line 75-95)
packages/providers/src/retryErrorClassification.ts: > isObjectLike treats functions as object-like values (typeof value === 'function'), which is type-incorrect and semantically risky here. If a function is ever passed as an error (e.g., a callback/arrow function thrown or rejected), markErrorAfterStreamOutput will add it to errorsAfterStreamOutput, and hasErrorName will inspect its .name. This can misclassify non-error functions as terminal retry errors or as named errors like AbortError, potentially suppressing legitimate retries. Restrict isObjectLike to actual objects. (line 31-35)
packages/providers/src/auth/__tests__/BucketFailoverHandlerImpl.invalidateAuthCache.test.ts: > The second test case has a race condition that makes the assertion expect(oauthManager.getTokenStore).not.toHaveBeenCalled() unreliable. Inside retryWithBackoff, when the thrown error is classified as a 429, attemptFailover synchronously invokes handler.tryFailover(...) before the microtask queue runs. BucketFailoverHandlerImpl.tryFailoverInternal then calls classifyTriggeringBucket → readStoredToken, which calls oauthManager.getTokenStore() synchronously before any await that would allow the queued controller.abort() to take effect. As a result, getTokenStore is invoked before the AbortError is thrown, so the assertion will fail in practice. Consider asserting the intended outcome instead—e.g., that the promise rejects with AbortError and that failover state did not change—rather than asserting that a synchronous setup call was skipped. (line 75-95)
packages/providers/src/loadBalancing/streamTimeout.ts: > The pattern for await (const chunk of { [Symbol.asyncIterator]: () =&gt; iterator }) is non-idiomatic and harder to read than the standard async iteration syntax. Since AsyncIterableIterator&lt;IContent&gt; already implements AsyncIterable&lt;IContent&gt;, this should be simplified to yield* iterator (or for await (const chunk of iterator)). (line 79-81)
packages/providers/src/providerErrorObservation.ts: > Runtime crash risk: options.metadata is optional per GenerateChatOptions, but ...options.metadata will throw TypeError: Cannot convert undefined or null to object whenever metadata is omitted. Any call site that does not supply metadata will crash the moment this observation system is activated. Fix by defaulting to an empty object: metadata: { ...(options.metadata ?? {}), [PROVIDER_ERROR_OBSERVATION_CONTEXT_KEY]: context }. (line 117-123)
packages/providers/src/providerErrorObservation.ts: > Potential XSS vector: normalizePublicProviderText/getSafeProviderMessage/formatPublicProviderMessage strip control characters but do not HTML-escape &lt;, &gt;, &amp;, ", or '. If these strings are rendered into web UI components via dangerouslySetInnerHTML or similar, crafted provider error messages could inject markup/scripts. Add HTML entity escaping before returning public-facing text. (line 220-224)
packages/providers/src/providerErrorObservation.test.ts: > These two test names use the ambiguous verb 'contains', which doesn't clearly describe the behavior being verified. Consider renaming to 'handles synchronous observer failure' and 'propagates asynchronous observer rejection' to make the test intent explicit. (line 12-27)
packages/providers/src/utils/abortSignal.ts: > Consider adding a brief note that callers must invoke dispose() to avoid leaking listeners, since this utility is exported and may be reused outside the current retry loops. (line 39-41)
packages/tools/src/formatters/ToolFormatter.test.ts: > The test uses import.meta.dirname (line 378) to resolve the path to ToolFormatter.ts. This is a Node.js-specific API added in v20.7.0 and is not available in older Node.js versions. The test will fail with a ReferenceError in environments running Node.js < 20.7.0. Use the standard ESM pattern new URL('./ToolFormatter.ts', import.meta.url) instead, which is universally supported and does not depend on Node.js version. (line 353-356)
packages/tools/src/formatters/ToolFormatter.test.ts: > The test name 'falls back description to empty string when absent' (line 93) is misleading. The test passes an empty string '' to makeDeclarations, but the helper uses the nullish coalescing operator (??) which treats '' as a defined value, not absent. This test actually verifies that an empty string is preserved, not that an absent description falls back to ''. Rename the test to accurately reflect the behavior, e.g., 'preserves empty string description'. (line 92-97)
packages/tools/src/formatters/ToolFormatter.test.ts: > In the 'converts to hermes/xml shape with parameters field' test (line 204), both 'hermes' and 'xml' formats are tested in a single loop. If either format fails, the error message will not indicate which specific format caused the failure. Split these into separate test cases for clearer diagnostics. (line 267-276)
scripts/genai-enclave/ast-helpers.ts: > ts.isTypeAssertionExpression is used here as a runtime type guard, but it is not available in all supported TypeScript versions. If this project targets older TS versions, this will throw at runtime. Consider guarding with a version check or avoiding this guard to maintain compatibility. (line 35-38)
scripts/genai-enclave/export-provenance.ts: > computeVarRange incorrectly treats top-level for-loop let/const initializers as module-scoped. When a for loop is at the top level of a source file, variableStatement.parent is the ForStatement, not the SourceFile, so ts.isSourceFile(variableStatement.parent) is false and it falls through to blockScopedRange. However, if the parent chain somehow makes variableStatement.parent appear as the SourceFile (or if a future refactor changes the traversal), this logic could still misclassify for-loop variables. More importantly, the current traversal never visits for-loop variable declarations at all, so these bindings are silently ignored. Consider handling VariableDeclarationList children inside ForStatement, ForInStatement, and ForOfStatement explicitly, and ensure computeVarRange checks whether the declaration is truly inside a loop rather than assuming any non-SourceFile parent is block-scoped. (line 101-119)
scripts/genai-enclave/helper-return-analysis.ts: > The catch-all return here treats every unhandled statement as fallthrough with unproven returns, but several TypeScript control-flow nodes should be analyzed for return provenance. TryStatement, SwitchStatement, IterationStatement (for/while/do-while), LabeledStatement, BreakStatement, and ContinueStatement are not handled. For example, a switch where every case returns a proven value, or a try/catch where both branches return proven values, will be conservatively flagged as fallthrough/unproven. This produces false negatives that undermine the security analysis. Consider adding explicit handling for these node types (e.g., treat TryStatement like IfStatement with try/catch flows, SwitchStatement by checking all cases and default, etc.). (line 87-92)
scripts/genai-enclave/helper-return-analysis.ts: > FALLTHROUGH is a single mutable object reused across all invocations. Even though the ReturnFlow interface is readonly, the object itself can still be mutated at runtime (e.g., FALLTHROUGH.canFallThrough = false). If any current or future consumer accidentally mutates it, the return-flow analysis of every subsequently analyzed function will be silently corrupted. Use Object.freeze({...}) or create a factory function to return a fresh frozen object per use. (line 17-21)
scripts/genai-enclave/config.ts: > The hardcoded SANCTIONED_GENAI_VERSION ('1.30.0') is currently consistent with the three actual package.json declarations (root, packages/core, packages/providers), but this is a maintenance hazard. When @google/genai is bumped in any manifest, this config will silently diverge and the guard will either reject valid manifests or permit stale versions. Consider deriving the version dynamically from the manifests, or at minimum adding a test that asserts the hardcoded value matches the declared versions. (line 89)
scripts/genai-enclave/import-context.ts: > The string literals 'createRequire' and 'module' are hardcoded in multiple places (isCreateRequireFactoryCallee, getModuleBaseExpression, isModuleRequireRef). Since REQUIRE_IDENTIFIER is already imported from ast-helpers.ts for the require identifier, consider extracting CREATE_REQUIRE_IDENTIFIER and MODULE_IDENTIFIER as exported constants for consistency and to reduce typo risk during future refactoring. (line 113-126)
scripts/genai-enclave/scanner.ts: > parseSourceFile calls ts.transpileModule without any error handling. If the transpiler throws (for example, on pathological input, large files, or TypeScript version differences), this will crash the scanner. Since this is a tool that processes arbitrary user source files, wrap the transpilation in try/catch and either return an empty diagnostic list on failure or propagate a controlled scanner-level error instead of letting a raw TS exception bubble up. (line 58-69)
scripts/genai-enclave/export-detection.ts: > The comment suggests falling through to inspect object-literal bindings for export default assignments, but the implementation correctly limits this behavior to export = (CommonJS-style) via the node.isExportEquals guard below. This mismatch could mislead future maintainers into incorrectly extending object-literal inspection to ESM default exports, which only expose the default binding—not nested properties. Please update the comment to clarify that object-literal binding resolution applies only to export =, not export default. (line 208-213)
scripts/genai-enclave/manifest-enforcement.ts: > Version exactness enforcement can silently degrade if getAllowedGenaiVersion returns undefined for a sanctioned workspace. Here, allowed !== undefined bypasses the version check, allowing any @google/genai version to pass in a sanctioned workspace if the config map is missing that entry. Since SANCTIONED_DIRS and DEPENDENCY_MANIFEST_MAP are both derived from GENAI_DEPENDENCY_MANIFESTS, they should be in sync, but this guard creates a latent enforcement gap on configuration drift. Consider asserting that sanctioned workspaces always have an allowed version, or treating undefined as a violation/internal error. (line 346-367)
scripts/tests/genai-enclave-guard-allowlist.test.ts: > The parameter section: object is overly broad. TypeScript's object type represents any non-primitive but does not guarantee the value is a dictionary with string keys, which weakens type safety for Object.entries and bracket notation access. Change the type to Record&lt;string, unknown&gt; to accurately reflect the expected manifest section structure and enable proper compile-time checking. (line 21-28)
scripts/tests/genai-enclave-guard-helpers.ts: > The SANCTIONED_VERSION alias on line 164 is a redundant pass-through of the imported SANCTIONED_GENAI_VERSION. This unnecessary indirection adds cognitive overhead and creates a maintenance risk if the two ever drift. Use the imported constant directly in writeRequiredManifests and remove the alias. (line 162)
scripts/tests/genai-enclave-guard-helpers.ts: > The .toString() calls on lines 139-140 are redundant. Since execFileSync is invoked with encoding: 'utf-8' on line 118, Node.js already returns strings for stdout and stderr (even in the error case). These calls can be removed for clarity. (line 137-138)
scripts/genai-enclave/import-detection.ts: > Security: isNodeModuleRequireCallExpression checks expr.expression without unwrapping transparent expressions. If the require call is wrapped in parentheses or type casts (e.g., (require)('node:module') or (0, require)('node:module')), the inline factory chain ...createRequire(...)(...) will not be detected, allowing a bypass of the enclave guard. Unwrap expr.expression before matching. (line 260-275)
scripts/genai-enclave/import-detection.ts: > Security: isModuleRequireCall compares against the identifier module directly without unwrapping transparent expressions. Wrapped forms like (0, module).require(...) will not be recognized. This should unwrap the expression before matching to prevent bypass. (line 157-167)
scripts/genai-enclave/import-detection.ts: > Security: getModuleBaseExpression inspects expr.expression without unwrapping transparent expressions. A wrapped module reference such as (0, module).require(...) would not match. Since this helper feeds isModuleRequireRef/isModuleRequireCall, unwrapping here would harden multiple call sites against parenthesized bypasses. (line 157-190)
scripts/tests/genai-enclave-guard.test.ts: > Inconsistent helper invocation: this test calls runScript(root) without the expected exit code, while every other test in this file explicitly passes runScript(root, 0) or runScript(root, 1). Even though runScript accepts a default parameter, omitting it here reduces readability and could mask issues if the helper’s default ever changes. Please pass 0 explicitly for consistency. (line 677-687)
scripts/tests/genai-enclave-scanner-exports.test.ts: > These tests use full-object toEqual matching on violation results. If the scanner later adds fields (for example, column, suggestion, or severity), these assertions will break even when behavior is unchanged. Consider asserting only the fields relevant to the behavior under test, such as exportName and exportForm (and kind if needed), to reduce maintenance brittleness. (line 104-112)
scripts/tests/genai-enclave-scanner-exports.test.ts: > Same brittleness risk here: full-object toEqual will break if additional violation fields are added later. Prefer partial shape assertions (for example, arrayContaining / objectContaining) so this test remains stable as the violation model evolves. (line 99-113)
scripts/tests/genai-enclave-scanner-exports.test.ts: > Requirement-reference prefixes are inconsistent across tests (F2:, A2/A3:, A6:, #2:, #2352). This makes it harder to trace failing tests back to requirements or tickets. Standardize on one prefix style to improve maintainability and auditability. (line 134)
scripts/tests/genai-enclave-scanner-exports.test.ts: > Requirement-reference prefixes are inconsistent across tests (F2:, A2/A3:, A6:, #2:, #2352). This makes it harder to trace failing tests back to requirements or tickets. Standardize on one prefix style to improve maintainability and auditability. (line 421)
scripts/tests/genai-enclave-scanner-regressions.test.ts: > The file header comment is stale: it claims to cover only findings 1, 2, and 7 and that findings 3-6 are covered elsewhere, but the file also contains test suites for findings 3, 4, A1, A4, A5, A7, and A8 plus several Critical regressions. Please update the header so it accurately reflects the current scope. (line 7-20)
scripts/tests/genai-published-root.test.ts: > The getGenaiVersion function returns the first occurrence of @google/genai across dependency sections without validating that all occurrences use the same version. If a manifest declares @google/genai in multiple sections with different versions, the test could pass even though the manifest contains an internal version conflict. Consider adding a check that all occurrences across dependencies, devDependencies, peerDependencies, and optionalDependencies use identical versions. (line 103-116)
scripts/tests/genai-published-root.test.ts: > Object.fromEntries(Object.entries(value)) creates an unnecessary shallow copy of the parsed manifest. Since isDependencyManifest only reads properties and does not modify the object, this copy is redundant and adds minor overhead. The function can operate directly on the input value after the type guard. (line 72-74)
scripts/tests/published-closure-regressions.test.ts: > Cleanup functions in test finally blocks lack error handling. If rmSync fails (e.g., on Windows due to file locks held by the process under test, or permission issues), the cleanup error will propagate and mask the actual test failure. Wrap cleanup in a try-catch or use a testing utility that ignores cleanup errors to prevent flaky behavior.

Example:

finally {
  try {
    cleanup();
  } catch { /* ignore cleanup errors in tests */ }
}
``` (line 82-84)

scripts/tests/publish-integrity.test.ts: > Dead code: after packageNameToWorkspace.has(name) returns true, packageNameToWorkspace.get(dep) cannot return undefined for the same key. These branches are unreachable and can be removed. (line 592-604)
scripts/tests/published-closure-regressions.test.ts: > Cleanup functions in test finally blocks lack error handling. If rmSync fails (e.g., on Windows due to file locks held by the process under test, or permission issues), the cleanup error will propagate and mask the actual test failure. Wrap cleanup in a try-catch or use a testing utility that ignores cleanup errors to prevent flaky behavior.

Example:

finally {
  try {
    cleanup();
  } catch { /* ignore cleanup errors in tests */ }
}
``` (line 81-84)

scripts/tests/publish-integrity.test.ts: > Dead code: after packageNameToWorkspace.has(name) returns true, packageNameToWorkspace.get(dep) cannot return undefined for the same key. These branches are unreachable and can be removed. (line 671-685)
scripts/tests/publish-integrity.test.ts: > Dead code: after packageNameToWorkspace.has(name) returns true, packageNameToWorkspace.get(dep) cannot return undefined for the same key. These branches are unreachable and can be removed. (line 724-737)
scripts/tests/publish-integrity.test.ts: > Duplication: these three blocks repeat the same setup logic (rootPackage parse, packed, buildPackageNameToWorkspaceMap, collectShippedWorkspacePackagePaths). Extract a shared setupWorkspaceIntegrityContext() helper to keep them in sync and reduce boilerplate. (line 651-662)
scripts/tests/workspace-source-helpers.ts: > Performance issue: Using Array.shift() in a while loop causes O(n²) complexity because each shift operation moves all remaining elements. For large dependency graphs, this could significantly slow down test execution. Consider using an index pointer instead:

let queueIndex = 0;
while (queueIndex &amp;lt; queue.length) {
  const current = queue[queueIndex++];
  // ... rest of loop body
}
``` (line 641-642)

scripts/tests/workspace-source-helpers.ts: > Misleading comment: The comment mentions 'Cycle check: resolve the real path and verify it doesn't loop back to a path we've already resolved' but the code only checks if the resolved path is a directory using statSync. It does not track previously seen paths or implement actual cycle detection. Please update the comment to accurately describe what the code does, or implement proper cycle detection if intended. (line 315-322)
scripts/tests/workspace-source-helpers.ts: > Comment/code discrepancy: The comment states 'Priority: bun → import → require → default → any string leaf' suggesting ordered preference, but the implementation simply collects ALL string leaf paths without any priority ordering. Either update the comment to reflect the actual behavior or implement the described priority if that was the intent. (line 339-342)
scripts/tests/workspace-source-helpers.ts: > Redundant code: The else branch and the fallback at the bottom both contain nearly identical logic for extracting the bun condition or falling back to manifest.main. The else branch is redundant because the fallback handles the same cases. Consider refactoring to remove the duplication: (line 406-417)
scripts/tests/workspace-source-helpers.ts: > Potential path traversal: The function resolves relative specifiers using path.resolve() but does not validate that the resolved path stays within repoRoot. A specifier with excessive .. sequences could resolve to files outside the repository. Consider adding a check after resolving the target to ensure it remains within the repo root:

const rel = relative(repoRoot, target);
if (rel.startsWith('..') || rel === '') {
  return undefined;
}
``` (line 465-471)

acoliver · 2026-07-13T04:06:11Z

The two OCR findings without inline positions were both addressed in 277ae02:

Disconnect cleanup: McpClient.disconnect now detaches the transport and client references first, attempts both closes even if transport close fails, always transitions status to DISCONNECTED in a finally block, and then rethrows the first close error.
createClient reuse: connectAndDiscoverFake now delegates new client construction to this.createClient(name, config), eliminating the duplicate constructor argument list and preventing parameter drift.

acoliver · 2026-07-15T09:18:08Z

OCR head `a0f016fc3ecfa9fb1d0dcff051166f19c22a5f7e` — 7 unpositioned finding dispositions

Dismissed — false positive. PermissionsModifyTrustDialog.test.tsx imports the real ideContext; setIdeContext, getIdeContext, and clearIdeContext therefore share the production context state. The mock only overrides getIdeTrust, not the context methods.
Accepted and fixed after behavioral reproduction. A direct McpClient.disconnect() with a throwing DISCONNECTING status listener exited before aborting work or closing the SDK client. A test-first regression reproduced sdkClient.close being called zero times. The fix catches both DISCONNECTING and DISCONNECTED notification failures, continues cleanup, and includes those failures in the final disconnect error; the regression now verifies close, final disconnected state, and both retained listener errors.
Dismissed — intentional adapter state. markConnectedForFakeDiscovery is invoked only by the production fake-discovery adapter, guards that there is no SDK client and that the state is disconnected, and enables the same manager lifecycle/quarantine behavior for fixture-backed MCP servers. Making it private would prevent its manager caller; moving it to tests would break the production fake adapter.
Dismissed — already covered. fakeMcpDiscovery.authorization.test.ts already spies on signal.removeEventListener and verifies the abort listener is removed when authorization is revoked during latency. No coverage gap remains.
Dismissed — coverage expansion without a reproduced defect. The production discovery flow checks authorization after resource discovery and again throughout publication, and existing authorization/publication tests verify rollback and disconnect when authorization changes. No stale-resource publication reproducer was provided.
Dismissed — deliberate defense at the capability boundary. FakeMcpToolInvocation retains a local fail-closed authorization guard even if the current closure already catches failures. The invocation accepts any () => boolean at its own contract boundary, so removing the guard would make correctness depend on one present constructor caller.
Dismissed — formatter-controlled style only. Prettier is authoritative for this file and formatting verification passes; adding hand-maintained whitespace does not address correctness or security.

Other latest feedback

CodeRabbit: the only inline concern was withdrawn by CodeRabbit in follow-up comment 3569089285, which agreed the existing listener-count black-box test is stronger. Its current summary has only the repository-wide docstring-coverage warning (8.33% versus 80%), a policy/style request across this very large PR rather than a correctness or security defect; no suppressions, ignores, or rule changes were made.
Ordinary review: no current ordinary reviewer requested a code change; the latest prior LLxprt disposition records the review as Ready.
OCR inline: all 50 designated inline findings received individual evidence-based replies and were explicitly resolved.

acoliver · 2026-07-15T10:31:04Z

OCR head `82ef6e19c4f785d8a397b4cbb1c70b90a03d46f0` — 4 unpositioned finding dispositions

Dismissed — equivalent mock value. Returning {} and returning { isIdeTrusted: undefined } are observationally equivalent for destructuring and property access in the code under review. The App context behavior remains covered; no undefined-property defect exists.
Dismissed — matcher exists. toHaveBeenCalledExactlyOnceWith is supported by this repository's Vitest matcher setup and is used throughout the codebase (40 matches). The implicated behavior suite passes, so the predicted runtime failure is false.
Dismissed — naming/style churn. The behavior test filename is descriptive and valid under the configured test discovery. Renaming it would not correct behavior or security.
Dismissed — unused test-double surface only. getMcPInstructions mirrors the manager contract but is not material to the behavior under test. Removing a harmless mock property is cleanup churn, not a reproducible defect.

All 45 findings were evaluated. Only inline finding 3 reproduced a security-relevant defect; it is fixed fail-closed with a behavioral regression in a6b0ef9. No suppressions, ignores, or rule changes were introduced.

acoliver · 2026-07-15T11:07:27Z

OCR head `a6b0ef9674c2093b0227f9f5bde896ff752138bf` — 18 unpositioned finding dispositions

Evaluated all 18 findings from OCR comment 4954250320 against the current code and behavioral tests. None is a demonstrably reproducible correctness or security defect, so no source change or push was made.

slashCommandProcessorSupport.test.tsx — mcpCommands / McpPromptLoader dead code: Dismissed (required orchestration fixture). useCommandReload constructs and executes McpPromptLoader as part of each real command-registry load. Keeping its result explicitly empty isolates the folder-trust behavior while exercising the actual loader composition; removing the mock would expose unrelated MCP state.
hook-reinit.test.ts — agent.dispose() rejection can crash cleanup: Dismissed (factually false). runExitCleanup() wraps every registered async cleanup in its own try/catch and intentionally continues. The registerCleanup(async () => await agent.dispose()) rejection is consumed by that runner, so partial teardown does not escape shutdown.
mcp-client.stale-error.test.ts — use mockReturnValueOnce: Dismissed (test style). Each test resets mocks and creates exactly one SDK client. The current mockReturnValue accurately supports that behavior; hypothetical extra constructions are not a current defect.
mcp-client.lifecycle.test.ts — throwing onerror contradicts not.toThrow: Dismissed (factually false). McpClient.connect() captures the original throwing handler and replaces mockedClient.onerror with its guarded wrapper. The test invokes that installed wrapper, verifies the original exception is contained, and passes in the focused suite.
mcp-client.tools.test.ts — assert cleanup server arguments: Dismissed (assertion preference). The test already verifies the cleanup count and surrounding behavior; the client under test has one fixed test-server identity. Additional nth-call argument assertions do not reveal a reproducible defect.
mcp-client.ts — AggregateError compatibility: Dismissed (standard error contract). AggregateError extends Error; existing callers catch unknown/Error, retryable disconnections preserve the rejection, and behavioral tests verify single and multiple cleanup failure contracts. No incompatible external caller was identified.
mcp-status.ts — optional failure collector makes contract inconsistent: Dismissed (intentional cleanup contract). The ordinary two-argument public call preserves legacy fail-fast listener behavior. The sole three-argument internal call is McpClient.updateStatus, where teardown must continue and aggregate listener failures with other cleanup failures. Removing that distinction would either hide public errors or abort cleanup.
retryable-client-disconnections.ts — shared rejecting promise: Dismissed (confirmed intentional). Concurrent requests for the same client deduplicate one physical disconnect; every waiter must observe that same operation's success or failure. Wrapping the promise would not change its semantic outcome and could obscure exactly-once behavior.
mcp-connection.ts — abort/rejection cause race: Dismissed (already behaviorally covered). The OAuth suite aborts, rejects the connect while cleanup is pending, and verifies one AbortError whose cause is the connect failure. If abort cleanup fully settles before a later underlying rejection exists, Promise-race semantics correctly report cancellation without a future cause.
mcp-discovery.latency.test.ts — hard-coded trust-check thresholds: Dismissed (named contract checkpoints). The values are named REVOKE_BEFORE_PUBLICATION_CHECK, REVOKE_DURING_TOOL_PUBLICATION_CHECK, and REVOKE_AFTER_CONNECT_CHECK; they intentionally pin the security checkpoints. A changed check sequence should break these tests rather than silently weaken revocation coverage.
mcp-discovery.latency.test.ts — toHaveBeenCalled consistency: Dismissed (style-only). The assertion establishes the required close behavior; exact invocation count is not the contract of this scenario and no correctness issue follows.
toml-loader.test.ts — missing blank line: Dismissed (format-only). This is whitespace with no behavior impact; the focused 29-test suite passes.
fakeMcpDiscovery.ts — top-level .strict() may reject extra fields: Dismissed (intentional fixture contract). This is a newly shipped test seam with a defined schema, not a compatibility parser for pre-existing fixtures. Rejecting unknown keys catches misspelled fixture controls instead of silently producing misleading integration behavior.
fakeMcpDiscovery.ts — nested .strict() risk: Dismissed (duplicate of 13). Strict nested tool/server validation is deliberate for the same reason: unknown latency, failure, or tool fields should fail fixture loading rather than be ignored. No legacy fixture containing extras exists in the repository.
fakeMcpDiscovery.ts — rename hasAuthorization: Dismissed (naming-only). The local function's call syntax and boolean return type are unambiguous; renaming has no correctness or security benefit.
fakeMcpDiscovery.ts — redundant safe authorization wrapper: Dismissed (harmless layered fail-closed defense). Discovery passes a current authorization closure into each stale tool handle, and invocation independently treats any callback exception as denial. The focused test verifies stale invocation is denied after revocation; removing a layer does not fix a defect.
fakeMcpDiscovery.ts — swallowed authorization callback errors: Dismissed (intentional fail-closed behavior, already covered). Authorization is a security predicate: an exception must become false, never permit discovery/invocation. The test treats authorization callback failures as revoked authorization proves this. Logging here would add noisy/potentially sensitive test-seam output without changing the contract.
fakeMcpDiscovery.ts — synchronous partial-publication window: Dismissed (speculative and already rolled back). No synchronous observer side effect is identified in ToolRegistry.registerTool; JavaScript does not interleave unrelated work inside the loop. Authorization is checked before and after every registration, and the behavioral test revokes during registration and verifies the just-published tool is removed before discovery resolves.

Other feedback state

CodeRabbit: its only inline finding is resolved and CodeRabbit explicitly withdrew it after accepting the behavioral listener-count proof. The current CodeRabbit check is successful; its remaining summary note is a non-blocking docstring-coverage warning, not a correctness/security defect.
Ordinary feedback: no unresolved human review threads exist. The LLxprt review summary reports missing checkout artifacts rather than a code finding, and the coverage comment is informational.

Verification evidence

CLI focused trust/dialog suites: 8 files, 141 tests passed
Core lifecycle/hook/event suites: 4 files, 45 tests passed, no type errors
MCP lifecycle/cancellation/authorization suites: 13 files, 109 tests passed, no type errors
Policy TOML suite: 1 file, 29 tests passed
Full npm run typecheck: passed for all workspaces and scripts

Conclusion: 0 actionable defects among these 18; no code change, commit, or push.

Preserve live folder-trust transitions while integrating the release branch.

Make folder trust updates live (Fixes #637)

1684f18

github-actions Bot added the maintainer:e2e:ok Trusted contributor; maintainer-approved E2E run label Jul 13, 2026