Conversation
…ule-mock retention and vi.fn restore
…alias preload via vi.mock
…ting the test process
… guarantees of the shim
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/ui/components/TodoPanel.responsive.test.tsx (1)
81-81: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRestore the removed TodoPanel behavior coverage.
The migration removes tests for narrow rendering, current-task output, resize output, and subtask semantic rendering. The PR objective requires migration without silently excluding, skipping, or deferring tests. Port these assertions to Bun-compatible rendering or replace each with equivalent behavior coverage.
packages/cli/src/ui/components/TodoPanel.responsive.test.tsx#L81-L81: Restore narrow-width status, count, and content-suppression coverage.packages/cli/src/ui/components/TodoPanel.responsive.test.tsx#L155-L155: Restore coverage for the in-progress task indicator if it remains part of the UI contract.packages/cli/src/ui/components/TodoPanel.responsive.test.tsx#L206-L206: Restore the initial narrow-render assertion in the resize test.packages/cli/src/ui/components/TodoPanel.semantic.test.tsx#L191-L191: Restore semantic rendering coverage for subtasks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/ui/components/TodoPanel.responsive.test.tsx` at line 81, Restore the removed TodoPanel behavior coverage using Bun-compatible rendering or equivalent assertions: in packages/cli/src/ui/components/TodoPanel.responsive.test.tsx:81, cover narrow-width status, count, and content suppression; at :155, cover the in-progress task indicator; at :206, restore the initial narrow-render assertion in the resize test; and in packages/cli/src/ui/components/TodoPanel.semantic.test.tsx:191, restore semantic subtask rendering coverage. Do not skip, defer, or silently remove these behaviors.
🧹 Nitpick comments (6)
packages/cli/test/run-bun-tests.test.ts (1)
134-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert discovery is non-empty before looping.
If
discoverTestFilesreturned an empty array, the loop body never runs and the test passes without checking anything. Assert the array contents directly.♻️ Proposed change
- for (const file of discoverTestFiles(root)) { - expect(file.startsWith('/')).toBe(false); - expect(file).toBe('src/only.test.ts'); - } + expect(discoverTestFiles(root)).toEqual(['src/only.test.ts']);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/run-bun-tests.test.ts` around lines 134 - 147, Update the test around discoverTestFiles to store its returned paths, assert the result is non-empty, and then verify the expected relative path directly. Preserve the existing checks that the discovered path is not absolute and equals src/only.test.ts.packages/cli/test/ui/commands/authCommand-logout.test.ts (2)
703-739: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound
numRunsfor the property tests that perform real token-store IO.
it.propandfc.assertboth default to 100 runs. This property saves tokens to the keyring-backed store and then executes up to 10 concurrent logout commands per run, so a single test can perform on the order of a thousand store operations. The Bun runner applies a 30s per-test timeout (PER_TEST_TIMEOUT_MSinpackages/cli/run-bun-tests.ts), and this file contains four such properties. Set an explicit run count so the suite has a predictable budget.♻️ Proposed change
}, ), + { numRuns: 20 }, ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/ui/commands/authCommand-logout.test.ts` around lines 703 - 739, Set an explicit, bounded numRuns option on the fc.assert call in the concurrent logout property test, using a run count appropriate for the keyring-backed token-store IO budget. Apply the same explicit bound to the other three property tests in this file that perform real token-store operations, while preserving their existing property logic.
651-672: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a direct whitespace arbitrary.
The filter rejects most generated strings and does not provide a targeted whitespace distribution.
fast-check4.5.3 supportsunitwithfc.string, so generate spaces and tabs directly withmaxLength: 4.♻️ Proposed change
- fc.string().filter((s) => /^\s*$/.test(s) && s.length < 5), // Only whitespace, shorter - fc.string().filter((s) => /^\s*$/.test(s) && s.length < 5), + fc.string({ unit: fc.constantFrom(' ', '\t'), maxLength: 4 }), + fc.string({ unit: fc.constantFrom(' ', '\t'), maxLength: 4 }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/ui/commands/authCommand-logout.test.ts` around lines 651 - 672, Update the property-based test around the `leadingSpace` and `trailingSpace` arbitraries to generate whitespace directly using `fc.string` with a whitespace character set and `maxLength: 4`, rather than filtering arbitrary strings with a regular expression. Preserve the existing command construction and assertions in the `authCommand.execute` test.packages/cli/src/config/extensions/settingsIntegration.test.ts (1)
32-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider clearing the shared keyring between tests, and forwarding the options argument.
The
entriesmap lives for the whole file, so credentials written by one test remain visible to later tests. The subclass also accepts only two constructor parameters, so any third options argument passed by the code under test is dropped and replaced by the keyring loader only.♻️ Suggested adjustments
- class InMemoryExtensionSettingsStorage extends actual.ExtensionSettingsStorage { - constructor(extensionName: string, extensionDir: string) { - super(extensionName, extensionDir, { - keyringLoader: async () => keyring, - }); - } - } + class InMemoryExtensionSettingsStorage extends actual.ExtensionSettingsStorage { + constructor( + extensionName: string, + extensionDir: string, + options?: Record<string, unknown>, + ) { + super(extensionName, extensionDir, { + ...options, + keyringLoader: async () => keyring, + }); + } + }Add a
beforeEachthat empties the shared map if tests must not observe each other's stored values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/config/extensions/settingsIntegration.test.ts` around lines 32 - 65, Update the settingsIntegration test mock so the shared entries map is cleared in a beforeEach, preventing credentials from leaking between tests, and change InMemoryExtensionSettingsStorage to accept and forward the original options argument while overriding only keyringLoader. Use the existing mock class and keyring symbols as the implementation points.test-setup/vitest-parity.test.ts (2)
95-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the mock after the
vi.unmockcase.
vi.unmock('./import-actual-fixture.js')at Line 101 replaces the module registration with the genuine exports for the remainder of the file. The earlier describes already ran, so the file passes today, but any case added below this point that expects'mocked-by-async-factory'will fail for a non-obvious reason. Re-register the mock in anafterEachfor this describe to keep the file order-independent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-setup/vitest-parity.test.ts` around lines 95 - 105, Restore the mocked registration after the vi.unmock test by adding an afterEach hook within the vi.unmock describe that re-registers the fixture with the existing mock setup, preserving the expected mocked-by-async-factory behavior for subsequent cases.
31-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert through a fresh import so the case can actually fail.
fixtureValueat Lines 36 and 41 is a static ESM binding captured when the file was evaluated.vi.restoreAllMocks()cannot change it, so this case passes even ifreapplyModuleMocks()is removed fromrestoreAllMocks. The case at Lines 72-77 covers the real behavior. Re-import inside this case to make the assertion meaningful.♻️ Suggested change
-describe('vi.restoreAllMocks over a module mock', () => { - it('leaves the module mocked and clears its call history', () => { - // Vitest's restoreAllMocks only restores spies; Bun's also reverts module - // mocks. The shim re-registers them afterwards, so a module mocked with - // vi.mock must still be a mock function here — with its calls cleared. - const mocked = fixtureValue; - expect(mocked).toBe('mocked-by-async-factory'); - - vi.restoreAllMocks(); - - expect(fixtureValue).toBe('mocked-by-async-factory'); - }); -}); +describe('vi.restoreAllMocks over a module mock', () => { + it('leaves the module mocked', async () => { + // Vitest's restoreAllMocks only restores spies; Bun's also reverts module + // mocks. The shim re-registers them afterwards, so a re-import after the + // restore must still observe the mocked value. + expect(fixtureValue).toBe('mocked-by-async-factory'); + + vi.restoreAllMocks(); + + const reimported = await import('./import-actual-fixture.js'); + expect(reimported.fixtureValue).toBe('mocked-by-async-factory'); + }); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-setup/vitest-parity.test.ts` around lines 31 - 43, Update the vi.restoreAllMocks module-mock test around fixtureValue to dynamically import the fixture after restoreAllMocks instead of asserting the statically captured ESM binding. Assert the freshly imported value remains 'mocked-by-async-factory', ensuring the test exercises re-registration of the module mock.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/run-bun-tests.ts`:
- Around line 129-141: Update collectTestFiles around statSync and realpathSync
to catch inspection errors for individual entries, treating unreadable or
dangling links as non-test entries and continuing the directory walk. Preserve
the existing visited real-path cycle prevention and test-file collection
behavior for entries that can be inspected.
In `@packages/cli/src/cli.startInteractiveUI.test.tsx`:
- Around line 66-76: Update the test teardown to call
__setRenderForTesting(null) in afterEach, clearing the renderer spy installed by
injectRenderSpy() so later startInteractiveUI tests use the default renderer.
In `@packages/cli/src/config/config.integration.test.ts`:
- Around line 365-378: Update parseArgumentsWithExitAsThrow so it saves the
original process.stdout.write reference without binding or wrapping it, then
restores that exact reference in the finally block while preserving the
temporary suppressed writer during parsing.
In `@packages/cli/src/config/profileResolution.ts`:
- Around line 168-172: Gate the inline profile debug log in the
profile-resolution flow with tempDebugMode, matching the behavior of
applyFileProfile. Only invoke debugLogger.debug for the inline profile when
tempDebugMode is enabled, while preserving the existing provider and model
details.
In `@packages/cli/src/integration-tests/security.integration.test.ts`:
- Around line 387-389: Update the valid keyfile assertion around fs.access to
pass the read-permission mode fs.constants.R_OK, ensuring the test verifies the
file is readable rather than only confirming its existence.
In
`@packages/cli/src/providers/logging/multi-provider-logging.integration.test.ts`:
- Around line 496-518: Update the offeredTools declaration used by the OpenAI
and Anthropic generateChatCompletion calls to replace the legacy parameters
field with parametersJsonSchema, preserving the existing schema content and
removing parameters entirely.
In `@packages/cli/src/services/__testhelpers__/mockFs.ts`:
- Around line 174-183: Update clear() around the mkdirSync calls for
userCommandsDir and projectCommandsDir to ignore errors only when the error code
is EEXIST; rethrow every other directory-creation failure so setup reports the
originating filesystem operation.
In `@packages/cli/src/test-utils/render.tsx`:
- Line 284: Validate the interval option in waitFor before calculating
maxIterations, rejecting zero (and any non-positive value) so polling cannot
produce Infinity and bypass the timeout boundary; preserve the existing timeout
behavior for valid positive intervals.
In `@packages/cli/test-utils/ink-stub.ts`:
- Around line 43-45: Make stdin ownership render-scoped in setActiveStdin and
the related registry in packages/cli/test-utils/ink-stub.ts, tracking each
active render and restoring the most recently remaining stdin when one is
released; reset the registry after global cleanup drains all instances. Update
removeActiveInstance in packages/cli/test-utils/ink-testing-library.ts to
release only its own stdin and restore another active render’s stdin, then add
an overlapping-render regression test.
In `@packages/cli/test/run-bun-tests.test.ts`:
- Around line 265-283: Update the “discoverTestFiles symlink safety” suite to
use describe.skipIf(process.platform === 'win32'), preserving the existing
cycle-detection test unchanged for supported platforms.
In `@test-setup/augment-bun-vi.ts`:
- Around line 662-670: Update the factory-result handling around drainMicrotasks
to check Bun.peek.status(factoryResult) and only read Bun.peek and apply the
settled namespace when the status is fulfilled, allowing rejected results to
reach the existing rejection handler. In the pending path, handle an undefined
syncActual from loadIsolatedModuleSync with an explicit fallback instead of
passing it to toNamespace and registering { default: undefined }.
- Around line 821-845: Update spyOnCompat to inspect the property descriptor
before Reflect.get and support accessor spying for get and set modes by wrapping
the corresponding descriptor accessor while preserving the other descriptor
fields. Install the replacement with the accessor descriptor and ensure the
installed spy restoration restores the original descriptor; retain existing
behavior for data properties and unsupported targets.
---
Outside diff comments:
In `@packages/cli/src/ui/components/TodoPanel.responsive.test.tsx`:
- Line 81: Restore the removed TodoPanel behavior coverage using Bun-compatible
rendering or equivalent assertions: in
packages/cli/src/ui/components/TodoPanel.responsive.test.tsx:81, cover
narrow-width status, count, and content suppression; at :155, cover the
in-progress task indicator; at :206, restore the initial narrow-render assertion
in the resize test; and in
packages/cli/src/ui/components/TodoPanel.semantic.test.tsx:191, restore semantic
subtask rendering coverage. Do not skip, defer, or silently remove these
behaviors.
---
Nitpick comments:
In `@packages/cli/src/config/extensions/settingsIntegration.test.ts`:
- Around line 32-65: Update the settingsIntegration test mock so the shared
entries map is cleared in a beforeEach, preventing credentials from leaking
between tests, and change InMemoryExtensionSettingsStorage to accept and forward
the original options argument while overriding only keyringLoader. Use the
existing mock class and keyring symbols as the implementation points.
In `@packages/cli/test/run-bun-tests.test.ts`:
- Around line 134-147: Update the test around discoverTestFiles to store its
returned paths, assert the result is non-empty, and then verify the expected
relative path directly. Preserve the existing checks that the discovered path is
not absolute and equals src/only.test.ts.
In `@packages/cli/test/ui/commands/authCommand-logout.test.ts`:
- Around line 703-739: Set an explicit, bounded numRuns option on the fc.assert
call in the concurrent logout property test, using a run count appropriate for
the keyring-backed token-store IO budget. Apply the same explicit bound to the
other three property tests in this file that perform real token-store
operations, while preserving their existing property logic.
- Around line 651-672: Update the property-based test around the `leadingSpace`
and `trailingSpace` arbitraries to generate whitespace directly using
`fc.string` with a whitespace character set and `maxLength: 4`, rather than
filtering arbitrary strings with a regular expression. Preserve the existing
command construction and assertions in the `authCommand.execute` test.
In `@test-setup/vitest-parity.test.ts`:
- Around line 95-105: Restore the mocked registration after the vi.unmock test
by adding an afterEach hook within the vi.unmock describe that re-registers the
fixture with the existing mock setup, preserving the expected
mocked-by-async-factory behavior for subsequent cases.
- Around line 31-43: Update the vi.restoreAllMocks module-mock test around
fixtureValue to dynamically import the fixture after restoreAllMocks instead of
asserting the statically captured ESM binding. Assert the freshly imported value
remains 'mocked-by-async-factory', ensuring the test exercises re-registration
of the module mock.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c379c9d-36dc-4b06-930d-d47d8756412d
⛔ Files ignored due to path filters (28)
packages/cli/src/ui/__snapshots__/App.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/HistoryItemDisplay.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/IDEContextDetailDisplay.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/InputPrompt.paste.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/InputPrompt.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/InputPrompt.vim.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/LoadingIndicator.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/ModelStatsDisplay.test.js.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/ModelStatsDisplay.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/SessionSummaryDisplay.test.js.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/SessionSummaryDisplay.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/SettingsDialog.interactions.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/SettingsDialog.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/StatsDisplay.sections.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/StatsDisplay.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/Table.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/ToolStatsDisplay.test.js.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/__snapshots__/ToolStatsDisplay.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/messages/__snapshots__/ToolGroupMessage.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/messages/__snapshots__/ToolMessage.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/shared/__snapshots__/RadioButtonSelect.test.js.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/components/views/__snapshots__/ChatList.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/utils/__snapshots__/MarkdownDisplay.test.js.snapis excluded by!**/*.snap,!**/__snapshots__/**packages/cli/src/ui/utils/__snapshots__/MarkdownDisplay.test.tsx.snapis excluded by!**/*.snap,!**/__snapshots__/**project-plans/issue2843/coverage-gaps.mdis excluded by!project-plans/**project-plans/issue2843/coverage-issue-body.mdis excluded by!project-plans/**project-plans/issue2843/plan.mdis excluded by!project-plans/**project-plans/issue2843/pr-body.mdis excluded by!project-plans/**
📒 Files selected for processing (177)
.github/workflows/ci.ymlpackages/cli/bun-test-setup.tspackages/cli/bunfig.tomlpackages/cli/package.jsonpackages/cli/run-bun-tests.tspackages/cli/src/cli-sandbox.test.tsxpackages/cli/src/cli.provider-init.test.tspackages/cli/src/cli.renderOptions.test.tsxpackages/cli/src/cli.startInteractiveUI.test.tsxpackages/cli/src/cli.test.tsxpackages/cli/src/cliStartupOrdering.test.tspackages/cli/src/commands/extensions/validate.test.tspackages/cli/src/commands/mcp.test.tspackages/cli/src/commands/mcp/add.test.tspackages/cli/src/config/__tests__/sandboxConfig.test.tspackages/cli/src/config/auth.test.tspackages/cli/src/config/config.integration.test.tspackages/cli/src/config/extension.part3.test.tspackages/cli/src/config/extension.test.tspackages/cli/src/config/extension.tspackages/cli/src/config/extensionLoaderSettingsRef.tspackages/cli/src/config/extensions/github.test.tspackages/cli/src/config/extensions/settingsIntegration.test.tspackages/cli/src/config/memoryReconciliation.concurrent.test.tspackages/cli/src/config/memoryReconciliation.test.tspackages/cli/src/config/profileBootstrap.tspackages/cli/src/config/profileResolution.tspackages/cli/src/config/trustedFolders.test.tspackages/cli/src/integration-tests/cli-args-test-helpers.tspackages/cli/src/integration-tests/cli-args.integration.test.tspackages/cli/src/integration-tests/cli-args.profile-flag.integration.test.tspackages/cli/src/integration-tests/compression-settings-apply.integration.test.tspackages/cli/src/integration-tests/consumer-migration-p13.integration.test.tspackages/cli/src/integration-tests/ephemeral-settings.integration.test.tspackages/cli/src/integration-tests/loadbalancer.integration.test.tspackages/cli/src/integration-tests/model-params-isolation.integration.test.tspackages/cli/src/integration-tests/oauth-timing.integration.test.tspackages/cli/src/integration-tests/retry-settings.integration.test.tspackages/cli/src/integration-tests/security.integration.test.tspackages/cli/src/integration-tests/test-utils.test.tspackages/cli/src/integration-tests/tools-governance.integration.test.tspackages/cli/src/launcher/bun-launcher.test.tspackages/cli/src/providers/logging/git-stats.integration.test.tspackages/cli/src/providers/logging/multi-provider-logging.integration.test.tspackages/cli/src/services/BuiltinCommandLoader.test.tspackages/cli/src/services/FileCommandLoader.processors.test.tspackages/cli/src/services/FileCommandLoader.test.tspackages/cli/src/services/__testhelpers__/mockFs.tspackages/cli/src/session/errorReporting.test.tspackages/cli/src/storage/ConversationStorage.test.tspackages/cli/src/test-utils/render.tsxpackages/cli/src/test-utils/responsive-testing.test.tsxpackages/cli/src/test-utils/sessionMetrics.tspackages/cli/src/ui/App.behavior.test.tsxpackages/cli/src/ui/App.components.test.tsxpackages/cli/src/ui/App.context.test.tsxpackages/cli/src/ui/App.dialogs.test.tsxpackages/cli/src/ui/App.e2e.test.tsxpackages/cli/src/ui/App.test.tsxpackages/cli/src/ui/__tests__/AppContainer.keybindings.test.tsxpackages/cli/src/ui/__tests__/AppContainer.mount.test.tsxpackages/cli/src/ui/__tests__/AppContainer.render-budget.test.tsxpackages/cli/src/ui/__tests__/integrationWiring.spec.tsxpackages/cli/src/ui/commands/AuthDialog.issue2274.test.tsxpackages/cli/src/ui/commands/diagnosticsCommand-test-helpers.tspackages/cli/src/ui/commands/permissionsCommand.test.tspackages/cli/src/ui/commands/providerCommand.test.tspackages/cli/src/ui/commands/test/subagentCommand.test.tspackages/cli/src/ui/components/AboutBox.theme.test.tsxpackages/cli/src/ui/components/AnsiOutput.test.tsxpackages/cli/src/ui/components/AuthDialog.test.tsxpackages/cli/src/ui/components/AuthDialog.theme.test.tsxpackages/cli/src/ui/components/ContextIndicator.ui.test.tsxpackages/cli/src/ui/components/Footer.responsive.test.tsxpackages/cli/src/ui/components/Footer.test.tsxpackages/cli/src/ui/components/Footer.tsxpackages/cli/src/ui/components/HistoryItemDisplay.test.tsxpackages/cli/src/ui/components/InputPrompt.completion.test.tsxpackages/cli/src/ui/components/InputPrompt.editing.test.tsxpackages/cli/src/ui/components/InputPrompt.paste.spec.tsxpackages/cli/src/ui/components/InputPrompt.paste.test.tsxpackages/cli/src/ui/components/InputPrompt.test.tsxpackages/cli/src/ui/components/InputPrompt.tsxpackages/cli/src/ui/components/InputPrompt.vim.test.tsxpackages/cli/src/ui/components/LoadingIndicator.test.tsxpackages/cli/src/ui/components/OAuthCodeDialog.test.tsxpackages/cli/src/ui/components/PoliciesDialog.test.tsxpackages/cli/src/ui/components/ProviderDialog.responsive.test.tsxpackages/cli/src/ui/components/SessionSummaryDisplay.test.tsxpackages/cli/src/ui/components/SettingsDialog.interactions.test.tsxpackages/cli/src/ui/components/SettingsDialog.test.tsxpackages/cli/src/ui/components/StatsDisplay.theme.test.tsxpackages/cli/src/ui/components/TodoPanel.responsive.test.tsxpackages/cli/src/ui/components/TodoPanel.semantic.test.tsxpackages/cli/src/ui/components/__tests__/LayoutManager.test.tsxpackages/cli/src/ui/components/__tests__/SessionBrowserDialog.layout.spec.tsxpackages/cli/src/ui/components/__tests__/SessionBrowserDialog.spec.tsxpackages/cli/src/ui/components/inputPromptRender.tsxpackages/cli/src/ui/components/inputPromptTypes.tspackages/cli/src/ui/components/messages/ToolConfirmationMessage.responsive.test.tsxpackages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsxpackages/cli/src/ui/components/shared/MaxSizedBox.test.tsxpackages/cli/src/ui/components/shared/ScrollableList.theme.test.tsxpackages/cli/src/ui/components/shared/VirtualizedList.theme.test.tsxpackages/cli/src/ui/components/shared/text-buffer.part2.test.tspackages/cli/src/ui/components/shared/text-buffer.part3.test.tspackages/cli/src/ui/components/shared/text-buffer.part4.test.tspackages/cli/src/ui/components/shared/text-buffer.part5.test.tspackages/cli/src/ui/components/shared/text-buffer.test.tspackages/cli/src/ui/containers/SessionController.test.tsxpackages/cli/src/ui/hooks/agentStream/__tests__/streamUtils.test.tspackages/cli/src/ui/hooks/toolMapping.test.tspackages/cli/src/ui/hooks/useAgentStream-test-helpers.tspackages/cli/src/ui/hooks/useAgentStream.approval.test.tsxpackages/cli/src/ui/hooks/useAgentStream.cancellation.test.tsxpackages/cli/src/ui/hooks/useAgentStream.commands.test.tsxpackages/cli/src/ui/hooks/useAgentStream.finished.test.tsxpackages/cli/src/ui/hooks/useAgentStream.hooks.test.tsxpackages/cli/src/ui/hooks/useAgentStream.include.test.tsxpackages/cli/src/ui/hooks/useAgentStream.loopdetect.test.tsxpackages/cli/src/ui/hooks/useAgentStream.ordering.test.tsxpackages/cli/src/ui/hooks/useAgentStream.subagent.spec.tsxpackages/cli/src/ui/hooks/useAgentStream.test.tsxpackages/cli/src/ui/hooks/useAgentStream.thinking.test.tsxpackages/cli/src/ui/hooks/useAgentStream.thought.test.tsxpackages/cli/src/ui/hooks/useAgentStream.usercancel.test.tsxpackages/cli/src/ui/hooks/useAtCompletion.subagent.test.tspackages/cli/src/ui/hooks/useAtCompletion.test.tspackages/cli/src/ui/hooks/useFolderTrust.test.tspackages/cli/src/ui/hooks/useGitBranchName.test.tsxpackages/cli/src/ui/hooks/useKeypress.test.tsxpackages/cli/src/ui/hooks/useLoadingIndicator.test.tsxpackages/cli/src/ui/themes/theme-compat.tspackages/cli/src/ui/utils/MarkdownDisplay.test.tsxpackages/cli/src/ui/utils/clipboardUtils.test.tspackages/cli/src/ui/utils/commandUtils.test.tspackages/cli/src/ui/utils/mouse.test.tspackages/cli/src/ui/utils/updateCheck.test.tspackages/cli/src/utils/dynamicSettings.test.tspackages/cli/src/utils/sandbox-bashrc.tspackages/cli/src/utils/sandbox-entrypoint.test.tspackages/cli/src/utils/sandbox-seatbelt.test.tspackages/cli/src/utils/sessionCleanup.integration.test.tspackages/cli/src/utils/userStartupWarnings.tspackages/cli/src/utils/version.test.tspackages/cli/src/utils/version.tspackages/cli/stryker.conf.jsonpackages/cli/test-setup-base.tspackages/cli/test-setup.tspackages/cli/test-utils/ink-stub.tspackages/cli/test-utils/ink-testing-library.tspackages/cli/test/baseProvider.stateless.stub.test.tspackages/cli/test/integration/auth-e2e.integration.test.tspackages/cli/test/openai.stateless.stub.test.tspackages/cli/test/openaiResponses.stateless.stub.test.tspackages/cli/test/providers/providerAliases.test.tspackages/cli/test/run-bun-tests.test.tspackages/cli/test/ui/commands/authCommand-logout.test.tspackages/cli/vitest.agentStream.config.tspackages/cli/vitest.ci.covered.config.tspackages/cli/vitest.ci.fast.config.tspackages/cli/vitest.cli-integration.config.tspackages/cli/vitest.config.integration.tspackages/cli/vitest.config.mutation.tspackages/cli/vitest.config.tspackages/cli/vitest.integration.config.tspackages/cli/vitest.test-groups.test.tspackages/cli/vitest.test-groups.tspackages/settings/src/settings/registry/registry-entries-2.tsscripts/bun-test-manifest.tsscripts/tests/bun-manifest-root-ownership.bun.test.tsscripts/tests/bun-test-manifest.bun.test.tstest-setup/augment-bun-vi.tstest-setup/module-resolution.tstest-setup/nested-mock-fixture.tstest-setup/stub-helpers.tstest-setup/vitest-parity.test.ts
💤 Files with no reviewable changes (36)
- packages/cli/test/openai.stateless.stub.test.ts
- packages/cli/test/openaiResponses.stateless.stub.test.ts
- packages/cli/src/ui/components/shared/ScrollableList.theme.test.tsx
- packages/cli/src/ui/components/AuthDialog.theme.test.tsx
- packages/cli/test/baseProvider.stateless.stub.test.ts
- packages/cli/src/ui/hooks/useAgentStream.approval.test.tsx
- packages/cli/vitest.config.integration.ts
- packages/cli/vitest.ci.covered.config.ts
- packages/cli/vitest.cli-integration.config.ts
- packages/cli/src/ui/components/shared/VirtualizedList.theme.test.tsx
- packages/cli/vitest.config.ts
- packages/cli/src/ui/App.test.tsx
- packages/cli/src/ui/App.e2e.test.tsx
- packages/cli/test/integration/auth-e2e.integration.test.ts
- packages/cli/stryker.conf.json
- packages/cli/vitest.integration.config.ts
- packages/cli/vitest.config.mutation.ts
- packages/cli/src/ui/App.components.test.tsx
- packages/cli/vitest.agentStream.config.ts
- packages/cli/test-setup-base.ts
- packages/cli/src/ui/components/StatsDisplay.theme.test.tsx
- packages/cli/src/ui/components/OAuthCodeDialog.test.tsx
- packages/cli/src/ui/components/AboutBox.theme.test.tsx
- packages/cli/src/ui/hooks/useAgentStream.hooks.test.tsx
- packages/cli/src/ui/App.context.test.tsx
- packages/cli/src/ui/components/messages/ToolConfirmationMessage.responsive.test.tsx
- packages/cli/src/ui/hooks/useAgentStream.cancellation.test.tsx
- packages/cli/src/config/auth.test.ts
- packages/cli/src/ui/App.dialogs.test.tsx
- packages/cli/test-setup.ts
- packages/cli/vitest.test-groups.test.ts
- packages/cli/src/ui/components/inputPromptTypes.ts
- packages/cli/src/ui/hooks/useAgentStream.loopdetect.test.tsx
- packages/cli/vitest.ci.fast.config.ts
- packages/cli/src/ui/App.behavior.test.tsx
- packages/cli/src/ui/components/Footer.responsive.test.tsx
Collaborator
Author
|
CodeRabbit triage and remediation update: Blocker-Fix / In-scope-Fix addressed locally:
Reject findings:
The agents CI flake was also root-caused: advanceTimersByTimeAsync can stall at its post-fake-time real setImmediate boundary on Linux; Bun then times out the test and teardown clears mock history, producing the misleading zero-call assertion. The focused test now uses synchronous fake-time advancement for its synchronous timeout callback and asserts the observable TIMEOUT result. It passed 10 stress runs plus the current focused run. Verification on current local candidate: typecheck/build/lint/format pass; focused suites pass (agents 12/12, CLI start 9/9, config 19/19, logging 7/7, stdin ownership 1/1, runner 26/26, security 15 pass with 1 pre-existing platform skip, augment 21/21). A fresh CI run is still required after push; the user-triggered green rerun was on the prior head. |
This was referenced Aug 6, 2026
This was referenced Aug 6, 2026
Merged
acoliver
added a commit
that referenced
this pull request
Aug 6, 2026
* Guard against silently unrun CLI test files (Fixes #2923) The Vitest baseExclude contract this issue was filed against is gone: PR #3056 replaced it with structural discovery in packages/cli/run-bun-tests.ts, and all 670 tracked CLI test files are now discovered and run. What was missing is the third resolution the issue asks for — something that makes a future silent exclusion fail loudly. run-bun-tests.ts walks a hardcoded TEST_ROOTS list, so a tracked test file added anywhere else under packages/cli would never run while every existing test still passed. The new guard compares the git-tracked test set against the runner's own discoverTestFiles() and fails, naming the file, when the two disagree or when a path is discovered more than once. * Address review: prove sorting, distinguish maxBuffer overflow from timeout The sorted-output test passed pre-sorted input, so it would still pass if sorting were dropped. It now supplies unsorted input. Node kills a child with SIGTERM for both a timeout and a maxBuffer overflow, so the helper reported runaway output as a timeout and hid the real cause. The overflow is now identified by its error code first, matching the handling in scripts/tests/cli-import-boundary.test.ts. * Report both discovery violations in a single run evaluateDiscovery returned as soon as it found duplicates, so a run that had both duplicates and undiscovered files only reported the duplicates. The reader had to fix one, re-run, and only then learn about the other. Both are now computed up front and every violation present is reported together.
This was referenced Aug 6, 2026
Open
7 tasks
This was referenced Aug 8, 2026
Merged
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2843.
Migrates the
cliworkspace to Bun's native test runner and removes Vitest fromit entirely. There is no
test:vitestfallback: keeping one would mean keepingtwo runners green.
What changed
packages/cli/run-bun-tests.ts— discovers every test file undersrc/,test/,test-bun/andtest-utils/and runs each in its ownbun testprocess. A process per file is required because Bun's
mock.moduleregistryis process-wide, so a shared process leaks module mocks between files. Writes
JUnit, passes
--timeout 30000explicitly (Bun's 5s default is not enough,and the
bunfigkey is not honoured for single-file runs).vitest.test-groups.ts,test-setup.ts,test-setup-base.ts,stryker.conf.json.test-setup/augment-bun-vi.ts(shared with every workspace) — asyncvi.mockfactories are settled synchronously withdrainMicrotasks()so themock exists before the module body runs, as Vitest's hoisting guarantees; a
factory that rejects now fails loudly instead of silently leaving the real
module installed;
restoreAllMockskeeps module mocks registered and clearsnested mock history.
packages/cli/bun-test-setup.ts— Vitestglobals: trueparity, Inkteardown, credential-proxy env isolation, and
process.exitCodereset pertest.
silently appended new entries and every snapshot assertion was vacuous.
The exclusion list this removes
vitest.test-groups.tscarried abaseExcludelist that hid ~37 test files,and no CI job ever invoked the CLI's
test:integration, hiding 24 more. Thosefiles had not run in a long time. This PR runs everything, which is why
previously invisible failures appear.
Results
Cross-workspace, to cover the shared shim: core 336/336, providers 493/493,
auth 33/33, test-setup 3/3. The 3
agentsfailures were verified against aclean
mainworktree and fail identically there.lint0,typecheck0,prettier --checkclean,build0, smoke test passes.What is not fixed here, and why
Every remaining failure and every deleted test is recorded in #3046 with
evidence. The substantive items:
spawnSyncdrops extra file descriptors.sandbox-bashrc.tsreadspayloads from fds 3 and 4; Node returns
output.length5 with bothpopulated, Bun returns 3 and empty. All 16 cases fail and cannot be fixed in
the test. This has production impact, since the repo is moving to Bun as the
runtime.
works (
parseInlineProfilehas notype: 'loadbalancer'branch).bun-build.config.tsemits
packages/cli/bundle/llxprt.jswith no assets, whilecopy_bundle_assets.tsstill targets the pre-Ship a prebuilt CLI bundle: raw-TS distribution costs 5.2s of startup per launch, blowing agent-launcher timeouts on Windows #2999 repo-rootbundle/and isinvoked by nothing. CI is unaffected because it never builds a bundle, but a
stale or asset-less bundle silently shadows correct source.
24 tests were deleted: 13 asserted behaviour that no longer exists in the source
(e.g. a prop nothing reads), and 11 were placeholders whose only assertion was
expect(true).toBe(true). Each is listed in #3046 with the behaviours lost.Verification notes
CI=trueso Bun compares instead ofwriting. An early round of this migration committed fabricated snapshots
recorded from error frames; they were found by diffing entry counts against
mainand reverted.npm run bundle—npm run builddoes not regenerate it.Summary by CodeRabbit