fix(tests): use fs.cpSync for fixture copy to handle subdirectories by carlos-alm · Pull Request #584 · optave/ops-codegraph-tool

carlos-alm · 2026-03-24T06:51:57Z

Summary

pipeline.test.js and scoped-rebuild.test.js used readdirSync + copyFileSync to copy the fixture project into a temp dir. When .codegraph/ exists in the fixture directory, copyFileSync throws EPERM because it can't copy directories.
Replaced the shallow loop with fs.cpSync(src, dest, { recursive: true }) which handles nested directories correctly.

Test plan

npx vitest run tests/builder/pipeline.test.js tests/integration/scoped-rebuild.test.js — 9/9 pass

copyFileSync throws EPERM when readdirSync encounters .codegraph/ (a directory). Replace the shallow loop with fs.cpSync({ recursive: true }).

claude · 2026-03-24T06:52:08Z

Claude finished @carlos-alm's task —— View job

PR Review Complete

Todo List:

Phase 1: Root Cause Analysis - understand the problem being solved
Phase 2: Configuration Change Detection - check for high-risk changes
Phase 3: Test Integrity - verify test quality and assertions
Phase 4: Code Quality - assess implementation details
Phase 5: Backlog Compliance - rate against project criteria
Final Assessment - provide rating and recommendation

Root Cause Analysis

Problem Being Solved: The manual file copying approach (readdirSync + copyFileSync loop) in test setup fails with EPERM errors when the fixture directory contains subdirectories like .codegraph/. The copyFileSync API cannot handle directories, causing test failures.

Why This Approach: Using fs.cpSync(src, dest, { recursive: true }) is the correct modern Node.js API (available since Node 16.7.0) designed specifically for recursive directory copying. This replaces a fragile manual implementation with a robust standard library function.

Risk Assessment: ✅ LOW RISK - This is a pure improvement that fixes a real bug in test infrastructure without changing any production code or test logic.

Backlog Compliance

Zero-dep: ✅ — Uses built-in Node.js fs module, no new dependencies
Foundation-aligned: ✅ — Test infrastructure improvement, fully aligned with all principles
Problem-fit: 3 — Useful test infrastructure fix, but doesn't directly address the core "lost AI" problem
Breaking: No — Pure test infrastructure change, no user-facing API changes
Tier: 1 (zero-dep ✓ + foundation-aligned ✓)

Technical Assessment

Code Quality: ✅ EXCELLENT

tests/builder/pipeline.test.js:18 and tests/integration/scoped-rebuild.test.js:21 — Replaced 3-line manual loops with single fs.cpSync() calls
Eliminates EPERM failure mode when .codegraph/ directories exist in fixtures
Uses appropriate { recursive: true } option for deep copying
Maintains identical functionality while being more robust

Test Impact: ✅ POSITIVE

No test assertions modified — pure infrastructure improvement
Fixes real test failures without masking underlying issues
Both files maintain complete test coverage and validation logic

Security: ✅ NO CONCERNS — Only test code changes, uses standard Node.js API

Performance & Reliability

fs.cpSync() is more efficient than manual loops for deep directories
Handles edge cases (permissions, symlinks, special files) better than manual implementation
Atomic operation vs multiple discrete file operations

Minor Observations

Consistency: Both files now use identical fixture copying approach ✅
Node Version: Requires Node ≥16.7.0, but codegraph already requires Node ≥20 per CLAUDE.md ✅
Error Handling: fs.cpSync() provides better error messages than the manual loop ✅

Final Recommendation

Rating: ⭐⭐⭐⭐☆ (4/5)
Action: APPROVE
Reasoning: Clean bug fix that replaces fragile manual implementation with robust standard library API. Addresses real EPERM failures without any downsides. Well-scoped change that improves test reliability.

This is exactly the type of incremental improvement that strengthens the codebase — fixing real issues with minimal, focused changes.

greptile-apps · 2026-03-24T06:54:15Z

Greptile Summary

This PR fixes a test setup bug in pipeline.test.js and scoped-rebuild.test.js where a shallow readdirSync + copyFileSync loop failed with EPERM when the fixture directory contained a .codegraph/ subdirectory. Both files now use fs.cpSync with { recursive: true } to correctly handle nested directories.

Key changes:

Replaces the flat copy loop with fs.cpSync(FIXTURE_DIR, tmpDir, { recursive: true, filter }) in both test files.
Adds a filter using path.basename(src) !== '.codegraph' to exclude any .codegraph/ directory from the temp copy, preventing stale graph state from leaking into fresh test runs.
The path.basename-based filter ensures parent path components containing .codegraph in their name do not accidentally suppress the entire copy.

All previous review feedback (adding the filter, switching to path.basename precision) has been addressed in commits 6e80058 and 8abeb83. The implementation is correct and consistent across both test files.

Confidence Score: 5/5

This PR is safe to merge — it is a minimal, well-scoped test infrastructure fix with no production code changes.
The change is confined to two test helper setup blocks, uses a stable Node.js built-in API available since v16.7.0 (well within the project's Node >= 20 requirement), and all previously raised review concerns have been fully addressed. The filter logic is correct and the behavior of cpSync merging contents directly into an existing tmpDir matches the original shallow-copy intent.
No files require special attention.

Important Files Changed

Filename	Overview
tests/builder/pipeline.test.js	Replaced shallow `readdirSync` + `copyFileSync` loop with `fs.cpSync` (recursive, filtered) to correctly handle subdirectories and exclude stale `.codegraph/` state.
tests/integration/scoped-rebuild.test.js	Same `cpSync` fix applied to `copyFixture()` helper with identical `path.basename`-based filter; no new issues introduced.

Sequence Diagram

sequenceDiagram
    participant Test as Test (beforeAll / copyFixture)
    participant FS as node:fs
    participant TmpDir as tmpDir (os.tmpdir())
    participant Fixture as FIXTURE_DIR (sample-project/)

    Test->>FS: mkdtempSync("codegraph-*")
    FS-->>Test: tmpDir path

    Test->>FS: cpSync(FIXTURE_DIR, tmpDir, {recursive, filter})
    Note over FS,Fixture: filter: path.basename(src) !== '.codegraph'
    FS->>Fixture: read entries
    Fixture-->>FS: [math.js, utils.js, index.js, .codegraph/]
    FS->>TmpDir: copy math.js ✓
    FS->>TmpDir: copy utils.js ✓
    FS->>TmpDir: copy index.js ✓
    FS-xTmpDir: skip .codegraph/ (filtered)

    Test->>Test: buildGraph(tmpDir, ...)
    Note over Test,TmpDir: Builds fresh .codegraph/graph.db in tmpDir

    Test->>FS: rmSync(tmpDir, {recursive, force}) [afterAll]

_{Reviews (4): Last reviewed commit: "fix: format cpSync calls to satisfy biom..." | Re-trigger Greptile}

greptile-apps · 2026-03-24T06:54:18Z

tests/builder/pipeline.test.js

-  for (const file of fs.readdirSync(FIXTURE_DIR)) {
-    fs.copyFileSync(path.join(FIXTURE_DIR, file), path.join(tmpDir, file));
-  }
+  fs.cpSync(FIXTURE_DIR, tmpDir, { recursive: true });


Consider filtering .codegraph/ from the copy

fs.cpSync with { recursive: true } will copy any .codegraph/ directory present in the fixture (which is exactly what triggered this bug). That means tmpDir starts with whatever stale database files were in the fixture rather than a clean slate.

Tests currently survive this because all builds use incremental: false, which resets the DB — but future tests that omit that flag could pick up stale state from the fixture's .codegraph/. Adding a filter makes the intent explicit and guards against that class of issue:

Suggested change

fs.cpSync(FIXTURE_DIR, tmpDir, { recursive: true });

fs.cpSync(FIXTURE_DIR, tmpDir, { recursive: true, filter: (src) => !src.includes('.codegraph') });

Done — added the filter option to exclude .codegraph/ from the copy in 6e80058.

greptile-apps · 2026-03-24T06:54:19Z

tests/integration/scoped-rebuild.test.js

-  for (const file of fs.readdirSync(FIXTURE_DIR)) {
-    fs.copyFileSync(path.join(FIXTURE_DIR, file), path.join(dir, file));
-  }
+  fs.cpSync(FIXTURE_DIR, dir, { recursive: true });


Same .codegraph/ filter suggestion applies here

Same concern as pipeline.test.js: if the fixture directory happens to contain a .codegraph/ subdirectory it will be copied into every copyFixture() temp dir, potentially carrying stale graph data into fresh test runs. The filter option keeps temp dirs clean:

Suggested change

fs.cpSync(FIXTURE_DIR, dir, { recursive: true });

fs.cpSync(FIXTURE_DIR, dir, { recursive: true, filter: (src) => !src.includes('.codegraph') });

Done — same filter applied here in 6e80058.

carlos-alm · 2026-03-24T07:30:35Z

@greptileai

carlos-alm · 2026-03-24T07:30:45Z

@greptileai

greptile-apps · 2026-03-24T07:33:19Z

tests/builder/pipeline.test.js

-  for (const file of fs.readdirSync(FIXTURE_DIR)) {
-    fs.copyFileSync(path.join(FIXTURE_DIR, file), path.join(tmpDir, file));
-  }
+  fs.cpSync(FIXTURE_DIR, tmpDir, { recursive: true, filter: (src) => !src.includes('.codegraph') });


Filter could match parent path components

src.includes('.codegraph') checks the full source path string, not just the entry's basename. If the fixture is ever located under a parent directory whose name happens to contain .codegraph (e.g. /home/user/.codegraph-tools/...), the filter would silently skip every file in the copy, producing an empty tmpDir and silent test failures.

Using path.basename(src) makes the intent precise and path-independent:

Suggested change

fs.cpSync(FIXTURE_DIR, tmpDir, { recursive: true, filter: (src) => !src.includes('.codegraph') });

fs.cpSync(FIXTURE_DIR, tmpDir, { recursive: true, filter: (src) => path.basename(src) !== '.codegraph' });

Done — switched to path.basename(src) !== '.codegraph' in 8abeb83. This ensures the filter only matches the directory entry itself, not parent path components.

greptile-apps · 2026-03-24T07:33:20Z

tests/integration/scoped-rebuild.test.js

-  for (const file of fs.readdirSync(FIXTURE_DIR)) {
-    fs.copyFileSync(path.join(FIXTURE_DIR, file), path.join(dir, file));
-  }
+  fs.cpSync(FIXTURE_DIR, dir, { recursive: true, filter: (src) => !src.includes('.codegraph') });


Same filter precision concern

Same issue as pipeline.test.js: if any ancestor directory of FIXTURE_DIR contains .codegraph in its name, the filter returns false for every path and the temp directory is left empty. Prefer matching on path.basename for safety:

Suggested change

fs.cpSync(FIXTURE_DIR, dir, { recursive: true, filter: (src) => !src.includes('.codegraph') });

fs.cpSync(FIXTURE_DIR, dir, { recursive: true, filter: (src) => path.basename(src) !== '.codegraph' });

Done — same path.basename fix applied here in 8abeb83.

carlos-alm · 2026-03-24T07:52:39Z

@greptileai

carlos-alm · 2026-03-24T08:30:23Z

Fixed formatting in both test files — biome required the \ options object to be broken across multiple lines. This was the cause of the CI lint failure (the only 2 errors in the biome run were format errors in the PR's test files).

carlos-alm · 2026-03-24T08:30:31Z

@greptileai

fix(tests): use fs.cpSync for fixture copy to handle subdirectories

3854515

copyFileSync throws EPERM when readdirSync encounters .codegraph/ (a directory). Replace the shallow loop with fs.cpSync({ recursive: true }).

greptile-apps bot reviewed Mar 24, 2026

View reviewed changes

fix(tests): filter .codegraph/ from fixture copies (#584)

6e80058

greptile-apps bot reviewed Mar 24, 2026

View reviewed changes

fix(tests): use path.basename for .codegraph filter precision (#584)

8abeb83

carlos-alm added 2 commits March 24, 2026 02:13

merge: sync with origin/main

a34e859

fix: format cpSync calls to satisfy biome line-length (#584)

01fac2f

carlos-alm merged commit 79288ec into main Mar 24, 2026
16 checks passed

carlos-alm deleted the fix/test-fixture-copy branch March 24, 2026 09:06

github-actions bot locked and limited conversation to collaborators Mar 24, 2026

	fs.cpSync(FIXTURE_DIR, tmpDir, { recursive: true });
	fs.cpSync(FIXTURE_DIR, tmpDir, { recursive: true, filter: (src) => !src.includes('.codegraph') });

	fs.cpSync(FIXTURE_DIR, dir, { recursive: true });
	fs.cpSync(FIXTURE_DIR, dir, { recursive: true, filter: (src) => !src.includes('.codegraph') });

Conversation

carlos-alm commented Mar 24, 2026

Summary

Test plan

Uh oh!

claude bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Complete

Root Cause Analysis

Backlog Compliance

Technical Assessment

Performance & Reliability

Minor Observations

Final Recommendation

Uh oh!

greptile-apps bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

carlos-alm Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

carlos-alm Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

carlos-alm commented Mar 24, 2026

Uh oh!

carlos-alm commented Mar 24, 2026

Uh oh!

greptile-apps bot Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

carlos-alm Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

carlos-alm Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

carlos-alm commented Mar 24, 2026

Uh oh!

carlos-alm commented Mar 24, 2026

Uh oh!

carlos-alm commented Mar 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

claude bot commented Mar 24, 2026 •

edited

Loading

greptile-apps bot commented Mar 24, 2026 •

edited

Loading