Skip to content

fix: update benchmark script import paths after src/ restructure#521

Merged
carlos-alm merged 2 commits intomainfrom
fix/benchmark-import-paths
Mar 19, 2026
Merged

fix: update benchmark script import paths after src/ restructure#521
carlos-alm merged 2 commits intomainfrom
fix/benchmark-import-paths

Conversation

@carlos-alm
Copy link
Copy Markdown
Contributor

Summary

  • All 4 benchmark scripts (benchmark.js, query-benchmark.js, incremental-benchmark.js, embedding-benchmark.js) and fork-engine.js still referenced flat src/ paths (builder.js, queries.js, parser.js, native.js, resolve.js, embeddings/index.js) that no longer exist after the domain/infrastructure restructure
  • Updated all srcImport() calls to use correct subdirectory paths (domain/graph/builder.js, domain/queries.js, domain/parser.js, infrastructure/native.js, domain/graph/resolve.js, domain/search/index.js)
  • Fixed PROBE_FILE paths in benchmark.js and incremental-benchmark.js (src/queries.jssrc/domain/queries.js)
  • Fixed collectImportPairs() in incremental-benchmark.js to recursively walk src/ (only 2 files remain at root level after restructure)

Test plan

  • Smoke-tested all 6 corrected import paths resolve successfully in local mode
  • Re-run benchmark workflow via workflow_dispatch with version: dev to confirm all 4 jobs pass

Benchmark scripts still referenced flat src/ paths (builder.js,
queries.js, parser.js, native.js, resolve.js, embeddings/index.js)
that no longer exist after the domain/infrastructure restructure.
Also fix collectImportPairs() to walk src/ recursively since only
2 files remain at the root level.

Impact: 3 functions changed, 4 affected
@claude
Copy link
Copy Markdown

claude bot commented Mar 19, 2026

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


PR Review: Import Path Fixes After src/ Restructure

Review Progress

  • Read and analyze all 5 changed files to understand the import path fixes
  • Phase 1: Root cause analysis - determine if this fixes a real issue
  • Phase 2: Check for configuration changes (high scrutiny)
  • Phase 3: Verify test integrity
  • Phase 4: Code quality review
  • Phase 5: Backlog compliance assessment
  • Provide final assessment with rating and recommendation

Root Cause Analysis

Problem Being Solved: Benchmark scripts broke after the src/ directory restructure (PR #516) because they still referenced flat import paths like src/builder.js, src/queries.js, etc. that were moved to subdirectories like src/domain/graph/builder.js, src/domain/queries.js.

Why This Approach: This is a direct, surgical fix addressing the exact problem. Each srcImport() call was updated to use the correct new subdirectory path. The approach is justified because:

  • It fixes real broken functionality (benchmark scripts would fail with module resolution errors)
  • Changes are minimal and targeted - only fixing import paths, no logic changes
  • Maintains the same benchmark functionality with corrected module loading

Risk Assessment: LOW RISK - Pure maintenance fix with no behavioral changes.

Backlog Compliance

  • Zero-dep: ✓ — No new dependencies added, pure path corrections
  • Foundation-aligned: ✓ — Maintains existing benchmark infrastructure, no conflicts with core principles
  • Problem-fit: 2 — Infrastructure maintenance, doesn't directly help AI agents but keeps benchmarking working
  • Breaking: No — Same benchmark APIs, corrected module resolution
  • Tier: 1

Code Quality Assessment

Changes Made (All Correct):

scripts/benchmark.js:91-93:

  • builder.jsdomain/graph/builder.js
  • queries.jsdomain/queries.js
  • ✅ PROBE_FILE: src/queries.jssrc/domain/queries.js

scripts/incremental-benchmark.js:38-44:

  • queries.jsdomain/queries.js
  • resolve.jsdomain/graph/resolve.js
  • native.jsinfrastructure/native.js
  • ✅ PROBE_FILE: src/queries.jssrc/domain/queries.js
  • ✅ Fixed collectImportPairs() to recursively walk src/ (only 2 files remain at root level)

scripts/lib/fork-engine.js:141,146:

  • parser.jsdomain/parser.js
  • native.jsinfrastructure/native.js

scripts/query-benchmark.js:93-95:

  • builder.jsdomain/graph/builder.js
  • queries.jsdomain/queries.js

scripts/embedding-benchmark.js:32-34:

  • embeddings/index.jsdomain/search/index.js

Verification: All 6 corrected import paths exist in the new directory structure.

Critical Concerns

None. This is a straightforward maintenance fix with proper execution.

Final Recommendation

  • Rating: ⭐⭐⭐⭐☆ (4/5)
  • Action: APPROVE
  • Reasoning: Well-executed fix that directly addresses import path breakage after restructure. Changes are surgical, verified correct, and maintain all existing functionality. Only missing the 5th star because it's pure maintenance rather than feature enhancement.

Next Steps: As noted in the test plan, re-running the benchmark workflow will verify all fixes work end-to-end.


@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 19, 2026

Greptile Summary

This PR corrects six stale import paths across the four benchmark scripts and fork-engine.js following a src/ domain/infrastructure restructure, and fixes the collectImportPairs() helper to recursively walk the src/ tree rather than reading only flat root-level files.

All six updated target paths were verified to exist in the current src/ tree:

  • src/domain/graph/builder.js
  • src/domain/queries.js
  • src/domain/graph/resolve.js
  • src/infrastructure/native.js
  • src/domain/parser.js
  • src/domain/search/index.js

Key observations:

  • Both PROBE_FILE paths (benchmark.js line 98, incremental-benchmark.js line 152) correctly resolve to src/domain/queries.js, which exists.
  • The collectImportPairs() refactor in incremental-benchmark.js changes from fs.readdirSync(srcDir).filter(…) (flat, root-only) to a recursive walk(). This is necessary because .js files no longer exist at the src/ root — without this fix the import-resolution benchmark would have sampled only the 2 remaining root-level files, making timings unrepresentative.
  • The shared importRe regex (with the g flag) used inside the recursive walk() is safe: each file's while-loop runs until exec() returns null, which resets lastIndex to 0 before the next file is processed.

Confidence Score: 5/5

  • This PR is safe to merge — all corrected import paths resolve to confirmed files, and the recursive walk logic is sound.
  • All six import paths were verified against the actual src/ directory tree and exist. The PROBE_FILE updates are correct. The collectImportPairs() recursive-walk refactor is a necessary and correct fix; the stateful regex reuse across calls is safe because each file's while-loop always consumes to null. No logic regressions were found. The only incomplete item is the benchmark workflow dispatch (marked unchecked in the test plan), but this is a CI validation step, not a code correctness concern.
  • No files require special attention.

Important Files Changed

Filename Overview
scripts/benchmark.js Updated srcImport() calls to use domain/graph/builder.js and domain/queries.js; fixed PROBE_FILE to src/domain/queries.js. All paths confirmed to exist.
scripts/embedding-benchmark.js Updated both srcImport() calls from embeddings/index.js to domain/search/index.js. Path confirmed to exist at src/domain/search/index.js.
scripts/incremental-benchmark.js Updated four srcImport() calls and PROBE_FILE; refactored collectImportPairs() from a flat readdirSync to a recursive walk() — necessary because files no longer live at src/ root. Note: the shared stateful importRe regex is safe because each file's while-loop runs to null, resetting lastIndex before the next file.
scripts/lib/fork-engine.js Updated srcImport() calls from parser.jsdomain/parser.js and native.jsinfrastructure/native.js. Both target paths confirmed to exist.
scripts/query-benchmark.js Updated srcImport() calls from builder.jsdomain/graph/builder.js and queries.jsdomain/queries.js. Both paths confirmed to exist.

Sequence Diagram

sequenceDiagram
    participant BS as benchmark scripts
    participant BC as lib/bench-config.js
    participant FE as lib/fork-engine.js
    participant SRC as src/ (restructured)

    BS->>BC: resolveBenchmarkSource()
    BC-->>BS: { srcDir, version, cleanup }

    BS->>BS: srcImport(srcDir, 'domain/graph/builder.js')
    BS->>SRC: dynamic import → src/domain/graph/builder.js ✓

    BS->>BS: srcImport(srcDir, 'domain/queries.js')
    BS->>SRC: dynamic import → src/domain/queries.js ✓

    BS->>FE: forkEngines(import.meta.url, argv)
    FE->>BC: resolveBenchmarkSource()
    BC-->>FE: { srcDir, cleanup }
    FE->>FE: srcImport(srcDir, 'domain/parser.js')
    FE->>SRC: dynamic import → src/domain/parser.js ✓
    FE->>FE: srcImport(srcDir, 'infrastructure/native.js')
    FE->>SRC: dynamic import → src/infrastructure/native.js ✓
    FE-->>BS: { wasm, native }

    note over BS,SRC: incremental-benchmark additionally imports
    BS->>BS: srcImport(srcDir, 'domain/graph/resolve.js')
    BS->>SRC: dynamic import → src/domain/graph/resolve.js ✓

    note over BS,SRC: embedding-benchmark imports
    BS->>BS: srcImport(srcDir, 'domain/search/index.js')
    BS->>SRC: dynamic import → src/domain/search/index.js ✓
Loading

Last reviewed commit: "Merge branch 'main' ..."

@carlos-alm carlos-alm merged commit 202d35a into main Mar 19, 2026
15 checks passed
@carlos-alm carlos-alm deleted the fix/benchmark-import-paths branch March 19, 2026 08:12
@github-actions github-actions bot locked and limited conversation to collaborators Mar 19, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant