fix(packages/bundler): correct autoload region marker and restructure tests#83
fix(packages/bundler): correct autoload region marker and restructure tests#83zrosenbauer merged 10 commits intomainfrom
Conversation
… tests The autoload plugin's transform hook searched for `//#region src/autoloader.ts` but core's dist emits `//#region src/autoload.ts`. The mismatch meant the runtime `readdir`-based autoloader was never replaced with the static version, causing ENOENT when running bundled CLIs (`dist/commands/` doesn't exist). Also restructures the test suite: - Move auth, HTTP chain, and typed middleware tests into `packages/core/src/` where they belong (they test core internals, not built examples) - Replace old integration tests with true E2E tests that build example CLIs and run the binaries as subprocesses, asserting on stdout - Remove `@` alias from `vitest.exports.config.ts` (no longer needed) Co-Authored-By: Claude <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 458a29d The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Co-Authored-By: Claude <noreply@anthropic.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR renames bundler autoloader region labels from "src/autoloader.ts" to "src/autoload.ts" (code and tests), switches several auth tests to import mock OAuth utilities from a centralized test module and removes those re-exports from tests/helpers index, removes multiple per-command integration test files and adds consolidated example-based integration suites plus a new tests/helpers.ts runner, updates Vitest config (removes the Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
…est:exports
- Extract `createExampleRunner()` into `tests/helpers.ts` for shared use
- Remove inline `execSync('pnpm build')` from integration tests (turbo handles it)
- Update `test:exports` script to run `turbo run build` before vitest
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/helpers.ts`:
- Around line 15-20: The test helper currently builds a shell-interpolated
command via execSync(`node dist/index.mjs ${args.join(' ')}`) which breaks
arguments with spaces and risks injection; change it to use an argv-based child
process call (e.g., execFileSync or spawnSync) that passes 'node' as the
executable and ['dist/index.mjs', ...args] as the args array so arguments are
not shell-interpolated, keeping the same options (cwd, encoding, timeout) and
return value behavior in the function that returns (...args: readonly string[]).
In `@tests/integration/advanced.test.ts`:
- Around line 1-39: The test file tests/integration/advanced.test.ts violates
the project testing standard; move it to the canonical location
test/integration/advanced.test.ts and update any relative imports accordingly
(e.g., adjust the import of createExampleRunner from '../helpers.js' so it
correctly resolves from the new file location). Ensure the test still imports
createExampleRunner and uses the same describe/it assertions as before (retain
the symbols createExampleRunner and the test name strings) and run the test
suite to verify paths are correct.
In `@tests/integration/simple.test.ts`:
- Around line 1-76: The test file simple.test.ts is placed under
tests/integration/ but must live in test/integration/ per the repo testing
standard; move this file (the suite that begins with describe('examples/simple
(built CLI)') and uses createExampleRunner) into the required directory
test/integration/, then update any relative imports (e.g., the
createExampleRunner import from '../helpers.js') so they resolve from the new
file location and run the suite from the new path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e9b18a1c-9793-432d-990b-fc8336b6fcc7
📒 Files selected for processing (4)
package.jsontests/helpers.tstests/integration/advanced.test.tstests/integration/simple.test.ts
Replace test:exports with test:integration using turbo's arbitrary task dependencies to ensure example builds complete before integration tests. Co-Authored-By: Claude <noreply@anthropic.com>
- Add E2E tests for icons, diagnostic-output, and authenticated-service - Support custom dist paths in test runner (for cli/ subdirectory) - Use //#test:integration root task so turbo runs integration tests - Add cli/package.json to authenticated-service to fix build warning - Wire all example builds as turbo dependencies for test:integration Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/helpers.ts (1)
1-1:⚠️ Potential issue | 🟠 MajorReplace shell-interpolated
execSyncwith argv-based process execution.Line 20 builds a shell command using
args.join(' '), which mis-parses args containing spaces and allows shell metacharacter injection.🔧 Proposed fix
-import { execSync } from 'node:child_process' +import { execFileSync } from 'node:child_process' @@ return (...args: readonly string[]): string => - execSync(`node ${distPath} ${args.join(' ')} 2>&1`, { + execFileSync('node', [distPath, ...args], { cwd, encoding: 'utf8', timeout: 10_000, })#!/bin/bash set -euo pipefail # Verify shell-interpolated execution is still present. rg -n "execSync\\(`node \\$\\{distPath\\} \\$\\{args\\.join\\(' '\\)\\} 2>&1`" tests/helpers.ts # Inspect current call sites that depend on this helper. rg -n "createExampleRunner\\(" tests/integration/*.test.ts tests/helpers.tsAlso applies to: 19-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers.ts` at line 1, The helper currently builds a shell-interpolated command using execSync with args.join(' '), which mis-parses spaced arguments and allows shell injection; in the createExampleRunner helper replace that execSync call with a argv-based API (e.g., child_process.execFileSync or spawnSync) by invoking 'node' with an array like [distPath, ...args] so each arg is preserved, capture stdout/stderr via options (encoding:'utf8' and stdio:'pipe' or combine stderr into the returned string), and remove the inline "2>&1" shell redirection; update any result handling to use the returned buffers/strings from execFileSync/spawnSync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/helpers.ts`:
- Around line 13-16: The exported function createExampleRunner currently uses
two positional parameters (example, distPath) which violates the repo standard;
change its signature to accept a single destructured object parameter ({
example, distPath = 'dist/index.mjs' }) and update all callers to pass an object
instead of positional args; keep the same default for distPath, preserve the
return type and behavior, and update any tests/imports that call
createExampleRunner to use the new object shape.
---
Duplicate comments:
In `@tests/helpers.ts`:
- Line 1: The helper currently builds a shell-interpolated command using
execSync with args.join(' '), which mis-parses spaced arguments and allows shell
injection; in the createExampleRunner helper replace that execSync call with a
argv-based API (e.g., child_process.execFileSync or spawnSync) by invoking
'node' with an array like [distPath, ...args] so each arg is preserved, capture
stdout/stderr via options (encoding:'utf8' and stdio:'pipe' or combine stderr
into the returned string), and remove the inline "2>&1" shell redirection;
update any result handling to use the returned buffers/strings from
execFileSync/spawnSync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72c579c8-025f-4b91-9aef-99900ddc56f9
📒 Files selected for processing (6)
examples/authenticated-service/cli/package.jsontests/helpers.tstests/integration/authenticated-service.test.tstests/integration/diagnostic-output.test.tstests/integration/icons.test.tsturbo.json
Examples use `kidd build` which requires @kidd-cli/cli dist to exist. Move the Build step before Test so packages are built first. Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
pnpm 10 on Linux does not create .bin entries for workspace packages when the bin target (dist/index.js) does not exist at install time. Re-running pnpm install after building packages recreates the links. Co-Authored-By: Claude <noreply@anthropic.com>
Per coding standard, exported functions with 2+ params must use object destructuring. Co-Authored-By: Claude <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Replace execSync with spawnSync in createExampleRunner to prevent command injection and argument splitting issues with shell-interpolated command strings. Co-Authored-By: Claude <noreply@anthropic.com>
Summary
transformhook searched for//#region src/autoloader.tsbut core's dist emits//#region src/autoload.ts(the source file isautoload.ts, notautoloader.ts). This meant the runtimereaddir-based autoloader was never replaced with the static version duringkidd build, causingENOENT: no such file or directory, scandir 'dist/commands'at runtime.packages/core/src/where they belong. Replaced them with true E2E tests that build example CLIs and run the binaries as subprocesses.Changes
Bug fix
packages/bundler/src/autoloader/autoload-plugin.ts— fixed region marker constant and replacement outputpackages/bundler/src/autoloader/generate-autoloader.ts— aligned(static)region labelsTest restructure
packages/core/src/: auth-http-chain, typed-middleware, device-code-e2e, oauth-e2e, mock-oauth-servertests/integration/):simple.test.tsandadvanced.test.ts— build example CLIs, runnode dist/index.mjs, assert stdoutvitest.exports.config.ts— removed@alias (no longer needed)Test plan
pnpm checkpasses (typecheck + lint + format)pnpm testpasses (740 core tests including moved tests)pnpm test:exportspasses (56 tests: exports + E2E simple + E2E advanced)--help,greet,list,deploy --help