test: add bin-entries regression guard#40
Conversation
Parameterised test (tests/bin-entries.test.ts) that iterates over every pkg.bin entry and asserts: - target file exists on disk - target starts with a node shebang - target lives under a directory declared in pkg.files This prevents the regression class that shipped in @forgespace/core v1.4.0-v1.14.0, where bin.forge-patterns pointed at a non-existent dist/cli.js for over a year. See Forge-Space/core#202 for the original incident. Mutation-tested locally: reverting bin.forge-ai-init to point at a non-existent path makes the test fail on the existence assertion, proving the guard catches real breakage.
Forge AI Quality Gate✅ Passed — Score 0/100 (F) · Delta +0 · 0 new findings Category scores
Powered by Forge AI Action · forgespace.co |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdded a test that validates package.json Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.17.0)package.json┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/bin-entries.test.ts (1)
4-9: Use temp-directory fixtures for this filesystem-heavy test suite.The suite currently reads real repo state (
package.json,dist/*) directly, which makes it environment/order dependent. Prefer temp-dir fixtures for deterministic behavior.As per coding guidelines
**/*.test.ts: "Use temp directories for filesystem-heavy tests".Also applies to: 26-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bin-entries.test.ts` around lines 4 - 9, The test reads the real repository via process.cwd()/repoRoot and package.json (pkg), making it order- and environment-dependent; change the tests to create an isolated temp directory fixture for filesystem operations (use a temp dir, write a minimal package.json with the expected bin/files entries and create any required dist/* files), then point repoRoot or mock process.cwd() to that temp directory (so the code under test reads from the fixture), and apply the same pattern to the other filesystem-heavy blocks referenced around lines 26-49; update any cleanup to remove the temp fixture after each test.
🤖 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/bin-entries.test.ts`:
- Around line 17-23: The pkgFilesCoversPath function is too strict by only
comparing the top-level segment; update pkgFilesCoversPath to treat package
"files" entries as covering a path if any entry (1) exactly equals the cleaned
relPath, (2) equals the top-level segment (current behavior), (3) is a directory
prefix that matches the start of the cleaned relPath (e.g., entry "dist/" covers
"dist/index.js"), or (4) matches common glob patterns—use a glob/match library
(e.g., minimatch) or simple suffix/prefix checks to handle entries like
"dist/*.js" and explicit file paths so valid package configs are not rejected.
- Around line 39-44: The test currently early-returns when the computed bin path
(abs) is missing, masking failures; instead assert the file exists inside the
test by replacing the early return with an explicit assertion (e.g.,
expect(existsSync(abs)).toBeTruthy()/toBe(true)) before reading firstLine, then
proceed to read the file and assert the shebang; update the test in the
it.each(entries) block that computes abs from repoRoot/rel and uses firstLine so
missing bin targets cause a test failure rather than being skipped.
- Around line 34-37: The test relies on prebuilt dist artifacts and should be
made self-contained: in tests/bin-entries.test.ts update the suite that uses
it.each(entries) (and the repoRoot/existsSync check) to create a temporary test
fixture directory in a beforeAll (using fs.mkdtempSync or fs.promises.mkdtemp)
and write dummy files for each rel from entries into that temp dir (preserving
the same relative paths), then point repoRoot to that temp dir for the
assertions; remove dependence on global build artifacts and clean up the temp
directory in afterAll. Ensure references to entries, repoRoot, and the it.each
test remain but change the path resolution to use the new temp fixture
directory.
---
Nitpick comments:
In `@tests/bin-entries.test.ts`:
- Around line 4-9: The test reads the real repository via process.cwd()/repoRoot
and package.json (pkg), making it order- and environment-dependent; change the
tests to create an isolated temp directory fixture for filesystem operations
(use a temp dir, write a minimal package.json with the expected bin/files
entries and create any required dist/* files), then point repoRoot or mock
process.cwd() to that temp directory (so the code under test reads from the
fixture), and apply the same pattern to the other filesystem-heavy blocks
referenced around lines 26-49; update any cleanup to remove the temp fixture
after each test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| function pkgFilesCoversPath(files: string[], relPath: string): boolean { | ||
| const clean = relPath.replace(/^\.\//, ''); | ||
| const top = clean.split('/')[0]; | ||
| return files.some((f) => { | ||
| const c = f.replace(/\/$/, '').replace(/^\.\//, ''); | ||
| return c === top; | ||
| }); |
There was a problem hiding this comment.
pkg.files coverage check is too narrow and can reject valid package configs.
Matching only the top-level directory misses valid files entries like explicit file paths (e.g., dist/index.js) and common pattern forms. That can create false failures when this guard is rolled out across packages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/bin-entries.test.ts` around lines 17 - 23, The pkgFilesCoversPath
function is too strict by only comparing the top-level segment; update
pkgFilesCoversPath to treat package "files" entries as covering a path if any
entry (1) exactly equals the cleaned relPath, (2) equals the top-level segment
(current behavior), (3) is a directory prefix that matches the start of the
cleaned relPath (e.g., entry "dist/" covers "dist/index.js"), or (4) matches
common glob patterns—use a glob/match library (e.g., minimatch) or simple
suffix/prefix checks to handle entries like "dist/*.js" and explicit file paths
so valid package configs are not rejected.
| it.each(entries)('bin "%s" -> "%s" exists on disk', (_name, rel) => { | ||
| const abs = join(repoRoot, rel.replace(/^\.\//, '')); | ||
| expect(existsSync(abs)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Build-order dependency is currently breaking CI.
This assertion depends on prebuilt dist artifacts, and CI is already failing on this exact check. Make this suite self-contained (fixture/temp artifact) or guarantee npm run build always runs before this test job.
🧰 Tools
🪛 GitHub Actions: CI
[error] 34-36: Jest failed: package.json bin entries expects "forge-ai-init" -> "./dist/index.js" to exist on disk. expect(existsSync(abs)).toBe(true) received false.
🪛 GitHub Actions: SonarCloud Analysis
[error] 34-36: Jest test failed: package.json bin entries expected the target file to exist on disk. Failed assertion expect(existsSync(abs)).toBe(true) for bin "forge-ai-init" -> "./dist/index.js". Received false.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/bin-entries.test.ts` around lines 34 - 37, The test relies on prebuilt
dist artifacts and should be made self-contained: in tests/bin-entries.test.ts
update the suite that uses it.each(entries) (and the repoRoot/existsSync check)
to create a temporary test fixture directory in a beforeAll (using
fs.mkdtempSync or fs.promises.mkdtemp) and write dummy files for each rel from
entries into that temp dir (preserving the same relative paths), then point
repoRoot to that temp dir for the assertions; remove dependence on global build
artifacts and clean up the temp directory in afterAll. Ensure references to
entries, repoRoot, and the it.each test remain but change the path resolution to
use the new temp fixture directory.
| it.each(entries)('bin "%s" -> "%s" starts with a node shebang', (_name, rel) => { | ||
| const abs = join(repoRoot, rel.replace(/^\.\//, '')); | ||
| if (!existsSync(abs)) return; | ||
| const firstLine = readFileSync(abs, 'utf-8').split('\n', 1)[0] ?? ''; | ||
| expect(firstLine).toMatch(/^#!.*\bnode\b/); | ||
| }); |
There was a problem hiding this comment.
Shebang test can pass even when the bin target is missing.
Early-return on missing file masks the failure when this test is run in isolation. Assert file existence inside this test too, then check the shebang.
Suggested fix
it.each(entries)('bin "%s" -> "%s" starts with a node shebang', (_name, rel) => {
const abs = join(repoRoot, rel.replace(/^\.\//, ''));
- if (!existsSync(abs)) return;
+ expect(existsSync(abs)).toBe(true);
const firstLine = readFileSync(abs, 'utf-8').split('\n', 1)[0] ?? '';
expect(firstLine).toMatch(/^#!.*\bnode\b/);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it.each(entries)('bin "%s" -> "%s" starts with a node shebang', (_name, rel) => { | |
| const abs = join(repoRoot, rel.replace(/^\.\//, '')); | |
| if (!existsSync(abs)) return; | |
| const firstLine = readFileSync(abs, 'utf-8').split('\n', 1)[0] ?? ''; | |
| expect(firstLine).toMatch(/^#!.*\bnode\b/); | |
| }); | |
| it.each(entries)('bin "%s" -> "%s" starts with a node shebang', (_name, rel) => { | |
| const abs = join(repoRoot, rel.replace(/^\.\//, '')); | |
| expect(existsSync(abs)).toBe(true); | |
| const firstLine = readFileSync(abs, 'utf-8').split('\n', 1)[0] ?? ''; | |
| expect(firstLine).toMatch(/^#!.*\bnode\b/); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/bin-entries.test.ts` around lines 39 - 44, The test currently
early-returns when the computed bin path (abs) is missing, masking failures;
instead assert the file exists inside the test by replacing the early return
with an explicit assertion (e.g.,
expect(existsSync(abs)).toBeTruthy()/toBe(true)) before reading firstLine, then
proceed to read the file and assert the shebang; update the test in the
it.each(entries) block that computes abs from repoRoot/rel and uses firstLine so
missing bin targets cause a test failure rather than being skipped.
Without pretest=build, bin-entries.test.ts asserts against dist/index.js that does not exist yet in CI (jest runs before the build step), producing a false-positive failure. A pretest hook guarantees dist/ exists in every invocation path (CI, local, validate).
|
1 participant
Summary
Adds a parameterised Jest test suite that validates all
package.jsonbin entries:#!/usr/bin/env node)pkg.filesWhy This Matters
Forge-Space/core shipped v1.4.0 through v1.14.0 with
bin.forge-patternspointing at a non-existentdist/cli.jsfile — an error that went undetected for over a year because there was no regression guard. Everynpx @forgespace/core forge-patternsinvocation failed silently.See Forge-Space/core#202 for the original incident and test implementation.
Test Plan
npm run build)**/tests/**/*.test.ts)bin.forge-ai-initto non-existent path → test fails on existence assertionbin.forge-ai-init->./dist/index.jsrestored → all tests passNotes
This PR references Forge-Space/core#202 as the pattern implementation source. Rolling out to all Forge Space npm packages with
binblocks.Summary by CodeRabbit