fix(bench): repair benchmark workflow broken by TypeScript migration#612
fix(bench): repair benchmark workflow broken by TypeScript migration#612carlos-alm merged 4 commits intomainfrom
Conversation
The TS migration renamed src/cli.js → src/cli.ts and all src/ files from .js to .ts, but the benchmark workflow and srcImport helper were not updated: - embedding-benchmark "Build graph" step referenced `node src/cli.js` which no longer exists — now uses strip-types + ts-resolve-loader - srcImport() constructed file:// URLs with .js extensions pointing at files that only exist as .ts — now checks for .ts on disk first - HuggingFace cache key referenced stale `src/embeddings/**` path, updated to `src/domain/search/**` - Removed `2>/dev/null` from benchmark runner commands so failures produce visible error output in CI logs
|
Claude finished @carlos-alm's task —— View job PR Review Complete ✅Reviewing this TypeScript migration fix for the benchmark workflow. Todo List:
Root Cause AnalysisProblem Being Solved: Post-TypeScript migration, the v3.4.0 benchmark workflow failed because:
Why This Approach: ✅ Justified. The fixes directly address the root causes without introducing workarounds or masking symptoms. Risk Assessment: ✅ Low Risk. These are targeted fixes that restore broken functionality without introducing new behaviors. Backlog Compliance
Critical ConcernsNone. This is a clean, targeted fix for post-migration breakage. Technical Assessment✅ Excellent execution: Smart path resolution (
Workflow fixes (
Quality highlights:
Minor optimization note: Final Recommendation
|
Greptile SummaryThis PR repairs the benchmark CI workflow that was broken by the TypeScript migration in PR #580, fixing four distinct issues across the workflow YAML and the shared benchmark config helper.\n\nChanges made:\n\n- Confidence Score: 5/5Safe to merge — all four fixes are correct, targeted, and consistent with existing workflow patterns. Every changed line addresses a concrete, confirmed breakage from the failed v3.4.0 benchmark run. The No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[workflow_dispatch] --> B{source = npm?}
B -- yes --> C[Wait for package on npm registry]
B -- no --> D[Use local checkout]
C --> E[Cache HuggingFace models hashFiles src/domain/search/**]
D --> E
E --> F[Build graph node --strip-types ts-resolve-loader src/cli.ts build .]
F --> G[Run embedding benchmark stderr now visible in CI]
G --> H[Upload result artifact]
subgraph srcImport resolution
P[srcImport srcDir foo.js] --> Q{foo.ts exists on disk?}
Q -- yes --> R[return file URL for foo.ts]
Q -- no --> S[return file URL for foo.js]
end
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
scripts/lib/bench-config.ts
Outdated
| export function srcImport(srcDir, file) { | ||
| return pathToFileURL(path.join(srcDir, file)).href; | ||
| const full = path.join(srcDir, file); | ||
| if (file.endsWith('.js')) { | ||
| const tsVariant = full.replace(/\.js$/, '.ts'); | ||
| if (fs.existsSync(tsVariant)) return pathToFileURL(tsVariant).href; | ||
| } | ||
| return pathToFileURL(full).href; | ||
| } |
There was a problem hiding this comment.
srcImport is in a .ts file but its parameters are untyped. The JSDoc annotations provide documentation but don't give the compiler any enforcement. Consider adding explicit types:
| export function srcImport(srcDir, file) { | |
| return pathToFileURL(path.join(srcDir, file)).href; | |
| const full = path.join(srcDir, file); | |
| if (file.endsWith('.js')) { | |
| const tsVariant = full.replace(/\.js$/, '.ts'); | |
| if (fs.existsSync(tsVariant)) return pathToFileURL(tsVariant).href; | |
| } | |
| return pathToFileURL(full).href; | |
| } | |
| export function srcImport(srcDir: string, file: string): string { | |
| const full = path.join(srcDir, file); | |
| if (file.endsWith('.js')) { | |
| const tsVariant = full.replace(/\.js$/, '.ts'); | |
| if (fs.existsSync(tsVariant)) return pathToFileURL(tsVariant).href; | |
| } | |
| return pathToFileURL(full).href; | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — added explicit TypeScript parameter types (srcDir: string, file: string) and return type (: string) to srcImport.
Impact: 1 functions changed, 26 affected
|
CI failure analysis: The test failure in |
1 similar comment
|
CI failure analysis: The test failure in |
…ation Impact: 1 functions changed, 2 affected
|
Merged |
1 participant
Summary
The v3.4.0 benchmark workflow (run #23576682993) failed across all 4 jobs after the TypeScript migration (PR #580). Three issues:
node src/cli.js build .— the embedding benchmark's graph-build step hardcoded the old.jspath. Fixed to use--strip-types+ts-resolve-loaderwithsrc/cli.tssrcImport()resolved.jspaths that only exist as.ts— the helper now checks for.tson disk first, falling back to.jsfor backwards compatibility2>/dev/nullon benchmark commands — silenced all errors, making the failures invisible. Removed so CI logs show actual errorssrc/embeddings/**(pre-migration path), updated tosrc/domain/search/**Test plan
workflow_dispatchwithversion: devto verify all 4 jobs passversion: 3.4.0