fix(#562): align merge_success counter denominator with segment-level numerator#595
Merged
Merged
Conversation
… numerator
The merge_success event emission was pairing a segment-round numerator
(waveIndex = 0..N-1 of the segment-expanded merge rounds) with a task-
level denominator (taskLevelWaveCount, the pre-expansion wave count).
In a clean polyrepo run with 3 task-level waves expanded into 6
segment-level rounds, the operator-visible alert overflowed:
Wave 1 merged successfully (1/3). Tests pass.
Wave 2 merged successfully (2/3). Tests pass.
Wave 3 merged successfully (3/3). Tests pass.
Wave 4 merged successfully (4/3). Tests pass. <- denominator stale
Wave 5 merged successfully (5/3). Tests pass.
Wave 6 merged successfully (6/3). Tests pass.
The supervisor formatter renders 'Wave N merged successfully (X/Y)'
where N and X both derive from event.waveIndex + 1, but Y comes from
event.totalWaves. Both N/X were already segment-level, so the
denominator needs to be segment-level too — that's batchState.totalWaves
(set to rawWaves.length, the segment-expanded round count) and NOT
batchState.taskLevelWaveCount (the pre-expansion count). One-field
source-of-truth swap at the engine.ts emission site, fully described
inline as a comment so the segment/task-wave distinction is preserved
for future readers.
Functional behavior was unaffected — every wave merged successfully —
this is purely a cosmetic display fix. Reporter's preferred fix (per
issue body) and the same approach taken: keep both numerator and
denominator in segment-round units.
Adds a regression test (5.8a) that emits 6 successive merge_success
events with waveIndex 0..5 against a denominator of 6 and asserts:
1. Each rendered counter reads (waveIdx+1)/6.
2. None of the rendered counters contain the buggy '/3' denominator
pattern, which was the visual signature in the original report.
Validation:
npm run typecheck pass
npm run lint no new warnings (286/671 identical to main)
npm run format:check pass
supervisor.test.ts 114/114 pass (new 5.8a included)
Closes #562
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #562.
The bug
In a polyrepo batch where task-level waves expand into segment-level merge rounds (3 task waves → 6 segment rounds in the reporter's case), the operator-visible merge alerts overflowed their denominator:
Purely cosmetic — every wave merged successfully. Functionality unchanged.
Root cause
The supervisor's
formatEventNotificationrendersWave N merged successfully (X/Y)where:event.waveIndex + 1(set by the engine to the segment-round index 0..5)event.totalWaveswhich the engine was populating withtaskLevelWaveCount(the pre-expansion task-wave count = 3)Once segments expanded, N/X overflowed Y. The fix is to source the denominator from
batchState.totalWaves(assignedrawWaves.lengthatengine.ts:2767— the segment-expanded round count) instead.The fix
One-field source-of-truth swap at the
merge_successevent emission site inengine.ts:Plus an inline comment explaining the segment/task-wave distinction so a future reader doesn't reintroduce the bug.
Test
Adds
5.8atosupervisor.test.ts: emits 6 successivemerge_successevents withwaveIndex0..5 againsttotalWaves: 6and asserts:(waveIdx+1)/6./3pattern from the report.Validation
npm run typechecknpm run lintmain(zero new)npm run format:checksupervisor.test.ts(114 tests)Note on the design choice
The reporter offered two options: (a) flip the denominator to segment-level (what this PR does), or (b) reword the alert to disambiguate units. I went with (a) because:
Wave Nis also segment-level (useswaveIndex + 1), so changing the denominator gives the whole message a consistent unit system without touching any other emission sites.