chore(diag): log fast-skip rejection reason in CI bench gate - #1067
Conversation
The pre-publish bench gate's native incremental-rebuild metrics regressed from ~10ms to ~2000ms in the v3.10.0 publish run, indicating the JS-side fast-skip from #1064 isn't firing in CI. Reading the code didn't pinpoint which check is rejecting — the suspects (mtime precision mismatch, file enumeration drift, pending-analysis guard) all look plausible. Wrap each return-false in detectNoChanges with an env-gated info() log that names the check and prints the differing values. Set the env var on the pre-publish-benchmark job so the next workflow_dispatch surfaces the cause in stderr (visible in CI logs; bench scripts only redirect stdout to JSON files). Behavior unchanged when CODEGRAPH_FAST_SKIP_DIAG is unset — gate is opt-in to keep regular incremental builds quiet. Refs #1066 Impact: 2 functions changed, 7 affected
|
Claude finished @carlos-alm's task —— View job PR Review CompleteReviewing PR #1067: chore(diag): log fast-skip rejection reason in CI bench gate Review Tasks
Root Cause AnalysisProblem Being Solved: Native incremental no-op rebuilds regressed from 10-13ms baseline (v3.9.6) to ~2000ms (v3.10.0), precisely contradicting PR #1064's fast-skip optimization. Code inspection identified 4 potential causes but couldn't pinpoint which. Why This Approach: Targeted diagnostic logging is justified here because:
Risk Assessment: Very low. Changes are purely additive logging behind an environment gate. Code Quality ReviewImplementation Quality: ⭐⭐⭐⭐⭐ Excellent
Technical Correctness: ⭐⭐⭐⭐⭐ Perfect
Configuration Change: ✓ Appropriate
Backlog Compliance
Critical ConcernsNone. This is an exemplary diagnostic PR. Strengths:
Minor Observations:
Final Recommendation
Next Steps After Merge:
|
Greptile Summary
Confidence Score: 5/5Safe to merge — diagnostic-only change with no behavioral impact when the env var is unset. All production code paths are unaffected when CODEGRAPH_FAST_SKIP_DIAG is unset. The single P2 finding (silent catch path) only reduces diagnostic coverage for an uncommon exception scenario; it cannot cause incorrect output or regressions. src/domain/graph/builder/pipeline.ts — catch block uses verbose-only debug() instead of info() for the exception path when the diag env var is set. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[buildGraph called] --> B{CODEGRAPH_FAST_SKIP_DIAG=1?}
B -- Yes --> C[Collect failing pre-flight conditions]
C --> D{reasons.length > 0?}
D -- Yes --> E["info: [fast-skip] false: pre-flight gate skipped — reasons"]
B -- No --> F[Skip diag block]
F --> G
D -- No --> G{nativeAvailable && native && incremental && !forceFullRebuild && !scope?}
E --> H[Pre-flight condition failed — skip detectNoChanges]
G -- No --> H
G -- Yes --> I[collectFiles]
I --> J[detectNoChanges]
J --> K{result?}
K -- true --> L["info: [fast-skip] true: all checks passed (N files)"]
K -- false --> M["info: [fast-skip] false: specific reason"]
I -- throws --> N[catch block]
J -- throws --> N
N --> O["debug: native fast-skip pre-flight failed (verbose-only)"]
O --> P["No [fast-skip] log visible in CI without --verbose"]
L --> Q[Return early — graph up to date]
M --> R[Fall through to orchestrator]
H --> R
P --> R
Reviews (2): Last reviewed commit: "chore(diag): log call-site fast-skip rej..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Diagnostic logging gated by env var — used by the bench gate to surface | ||
| // why the fast-skip is not firing on CI runners (#1066). Off by default to | ||
| // avoid noise on every regular incremental build. | ||
| const diag = process.env.CODEGRAPH_FAST_SKIP_DIAG === '1'; | ||
| const log = (reason: string): void => { | ||
| if (diag) info(`[fast-skip] ${reason}`); | ||
| }; |
There was a problem hiding this comment.
forceFullRebuild hypothesis not covered
In pipeline.ts (line 1017), detectNoChanges is only reached when !ctx.forceFullRebuild. If forceFullRebuild is the cause of the regression (one of the four stated hypotheses in the PR description), no [fast-skip] line will ever appear in the logs — the entire guard is bypassed before entering this function. The CI run would be silent and inconclusive for that hypothesis, wasting the diagnostic opportunity.
Consider adding a log at the call site in pipeline.ts before the condition, for example logging when forceFullRebuild is true so all four hypotheses produce observable output.
There was a problem hiding this comment.
Fixed in c6c4d46 — added env-gated diagnostic logging at the pipeline.ts call site (lines 1013-1027) that names which guard(s) short-circuited (nativeAvailable, engineName, incremental, forceFullRebuild, scope) when the fast-skip is bypassed. All four hypotheses from #1066 now produce observable output even if detectNoChanges is never entered.
| for (const file of allFiles) { | ||
| const relPath = normalizePath(path.relative(rootDir, file)); | ||
| const record = existing.get(relPath); | ||
| if (!record) return false; | ||
| if (!record) { | ||
| log(`false: collected file missing from file_hashes: ${relPath}`); | ||
| return false; | ||
| } | ||
| const stat = fileStat(file) as FileStat | undefined; | ||
| if (!stat) return false; | ||
| if (!stat) { | ||
| log(`false: stat failed for ${relPath}`); | ||
| return false; | ||
| } | ||
| const storedMtime = record.mtime || 0; | ||
| const storedSize = record.size || 0; | ||
| if (storedSize <= 0) return false; | ||
| if (Math.floor(stat.mtimeMs) !== storedMtime || stat.size !== storedSize) return false; | ||
| if (storedSize <= 0) { | ||
| log(`false: stored size <= 0 for ${relPath} (stored=${record.size})`); | ||
| return false; | ||
| } | ||
| if (Math.floor(stat.mtimeMs) !== storedMtime || stat.size !== storedSize) { | ||
| log( | ||
| `false: mtime/size diff for ${relPath}: stat=${Math.floor(stat.mtimeMs)}/${stat.size} stored=${storedMtime}/${storedSize} (mtimeMs=${stat.mtimeMs})`, | ||
| ); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The allFiles loop exits on the very first failing file. If the root cause is an mtime drift, you'll see exactly one false: mtime/size diff line. That's fine for a single-file regression, but if multiple files exhibit drift you won't see a count or pattern. For this single-run diagnostic this is acceptable, but worth noting: if the log shows the first rejected file and the follow-up fix doesn't resolve the issue, a broader scan might be needed.
There was a problem hiding this comment.
Acknowledged — for this single-run diagnostic, logging the first mismatch is intentional and acceptable as you note. If the follow-up fix doesn't fully resolve the issue, we'll widen to a count/pattern scan in the next iteration. Leaving the early-exit behaviour as-is for this PR to keep the diagnostic minimal and easy to revert.
Codegraph Impact Analysis3 functions changed → 8 callers affected across 7 files
|
Greptile flagged that the original detect-changes diagnostic only fires once detectNoChanges is entered. The pipeline.ts call-site guards (forceFullRebuild, engineName, incremental, scope) bypass the function entirely, so a forceFullRebuild-driven regression (one of #1066's hypotheses) would produce zero [fast-skip] output and waste the CI diagnostic run. Add an env-gated log at the call site that names the failing guard(s) when any short-circuit, ensuring all four hypotheses produce observable output. Behaviour is unchanged when CODEGRAPH_FAST_SKIP_DIAG is unset. Refs #1066
1 participant
Summary
Behavior is unchanged when the env var is unset, so regular incremental builds stay quiet.
Why
The v3.10.0 publish gate (run 25361005381) measured native incremental no-op rebuilds at ~2000ms vs the 10–13ms 3.9.6 baseline — exactly the regression PR #1064's JS-side fast-skip was meant to fix. Code inspection (issue #1066) narrowed the cause to one of:
Reading the code didn't pinpoint which. One CI run with this diagnostic does.
What to look for in the next publish run
After merging and triggering a workflow_dispatch publish, search the "Run incremental benchmark" / "Run build benchmark" step logs for `[fast-skip]`. Expected outcomes:
Test plan
Refs #1066