From 7249bdddacddf3243d16845d92ce618b787ad56c Mon Sep 17 00:00:00 2001 From: 0xdhx Date: Thu, 10 Sep 2026 23:42:55 -0500 Subject: [PATCH] fix(#4294): reserve a full progress bar for 100% and give the render half one owner (#4473) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#4294): reserve a full progress bar for 100% and give the render half one owner Six call sites each carried `Math.round((percent / 100) * width)` inline, and every copy rounded to a full bar before the percent reached 100: from 95 up at width 10, from 98 up at width 20. A project at 19/20 plans drew the same bar as a shipped one beside a number that said otherwise, and an out-of-range percent threw `RangeError` from the unguarded `'░'.repeat`. ADR-3180 Decision 7 gave the completion-RATIO derivation one owner (`clampPercentFromFraction`). This gives the RENDER half the same: `progressBarFilledCells` / `renderProgressBar` in phase-lifecycle.cts, with the `progress` table and bar renderers, the stats renderer, the gsd2 import writer, and #4231's `formatProgressMachineSegment` (which now serves both STATE.md writers) all drawing through it. Contract: below 100 the fill is held one cell short of the width, so only the saturating percents move (95-99 at width 10, 98-99 at width 20) and every other value in 0-100 renders as before — pinned by an exhaustive comparison against the legacy formula at both widths. Null / non-finite renders an empty bar; out-of-range is clamped, never thrown. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_012hbFn24VWaxJBw8DUWjAmU * chore(#4294): set changeset fragment pr to 4473 * docs(#4294): correct the pre-fix inline call-site count to five The kernel's doc comment said SIX call sites carried their own `Math.round((percent / 100) * width)`. The base tree has five: three in `commands.cts` plus one each in `gsd2-import.cts` and `formatProgressMachineSegment`, the latter two using `/ 10` with the width already substituted (`pct` and `clamped` respectively). The six is #4294's count of consumers -- it counts `cmdStateUpdateProgress` and `syncCore` separately, but #4231 had already routed both through `formatProgressMachineSegment` (as it does `applyPostSyncPreservation`), so by this branch's base they share one copy. The comment now states the tree's count and records where the six comes from, so neither number reads as an error later. Comment-only; no behaviour change, and no change to compiled output. --------- Co-authored-by: Claude Fable 5.1 Co-authored-by: Tom Boucher --- .changeset/brave-elks-glide.md | 5 + src/commands.cts | 14 +-- src/gsd2-import.cts | 5 +- src/phase-lifecycle.cts | 58 +++++++++++ src/state-transition.cts | 13 +-- tests/4213-progress-helpers.property.test.cjs | 8 ++ tests/phase-lifecycle.test.cjs | 98 ++++++++++++++++++- 7 files changed, 180 insertions(+), 21 deletions(-) create mode 100644 .changeset/brave-elks-glide.md diff --git a/.changeset/brave-elks-glide.md b/.changeset/brave-elks-glide.md new file mode 100644 index 000000000..31003f20f --- /dev/null +++ b/.changeset/brave-elks-glide.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4473 +--- +**A progress bar is full only at 100%** — every bar-drawing surface (`progress` in table and bar format, `stats`, `state update-progress`, the STATE.md progress line written by `state sync`, and the gsd2 import writer) now draws through one render kernel, `renderProgressBar`, beside the completion-ratio kernel in `phase-lifecycle`. The six inline copies of `Math.round((percent / 100) * width)` each rounded to a full bar before the percent reached 100: at the 10-cell width every percent from 95 up drew `[██████████]`, at the 20-cell width every percent from 98 up, so a project at 19/20 plans was visually indistinguishable from a shipped one beside a number that said otherwise. Below 100 the fill is now held one cell short; only those percents move (95-99 at width 10, 98-99 at width 20), every other value in 0-100 renders exactly as before. A null or non-finite percent still renders an empty bar, and an out-of-range percent is clamped instead of throwing `RangeError` from `'░'.repeat` as the inline form did at 120%. diff --git a/src/commands.cts b/src/commands.cts index 58cb8d837..c84dd8cd5 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -58,7 +58,7 @@ import modelProfiles = require('./model-profiles.cjs'); const { MODEL_PROFILES, VALID_PHASE_TYPES } = modelProfiles; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; import { realClock } from './clock.cjs'; -import { clampPercent } from './phase-lifecycle.cjs'; +import { clampPercent, renderProgressBar } from './phase-lifecycle.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import planScanMod = require('./plan-scan.cjs'); const { scanPhasePlans } = planScanMod; @@ -2772,9 +2772,7 @@ function cmdProgressRender(cwd: string, format: string | undefined, raw: boolean if (format === 'table') { // Render markdown table - const barWidth = 10; - const filled = percent === null ? 0 : Math.round((percent / 100) * barWidth); - const bar = '█'.repeat(filled) + '░'.repeat(barWidth - filled); + const bar = renderProgressBar(percent, 10); const percentSuffix = percent === null ? '' : ` (${percent}%)`; let out = `# ${milestone?.version ?? ''} ${milestone?.name ?? ''}\n\n`; out += `**Progress:** [${bar}] ${totalSummaries}/${totalPlans} plans${percentSuffix}\n\n`; @@ -2785,9 +2783,7 @@ function cmdProgressRender(cwd: string, format: string | undefined, raw: boolean } output({ rendered: out }, raw, out); } else if (format === 'bar') { - const barWidth = 20; - const filled = percent === null ? 0 : Math.round((percent / 100) * barWidth); - const bar = '█'.repeat(filled) + '░'.repeat(barWidth - filled); + const bar = renderProgressBar(percent, 20); const percentSuffix = percent === null ? '' : ` (${percent}%)`; const text = `[${bar}] ${totalSummaries}/${totalPlans} plans${percentSuffix}`; output({ bar: text, percent, completed: totalSummaries, total: totalPlans }, raw, text); @@ -3256,9 +3252,7 @@ function cmdStats(cwd: string, format: string | undefined, raw: boolean): void { }; if (format === 'table') { - const barWidth = 10; - const filled = percent === null ? 0 : Math.round((percent / 100) * barWidth); - const bar = '█'.repeat(filled) + '░'.repeat(barWidth - filled); + const bar = renderProgressBar(percent, 10); let out = `# ${milestone?.version ?? ''} ${milestone?.name ?? ''} — Statistics\n\n`; const percentSuffix = percent === null ? '' : ` (${percent}%)`; out += `**Progress:** [${bar}] ${completedPhases}/${phases.length} phases${percentSuffix}\n`; diff --git a/src/gsd2-import.cts b/src/gsd2-import.cts index 4334deab9..d5037dd7f 100644 --- a/src/gsd2-import.cts +++ b/src/gsd2-import.cts @@ -24,7 +24,7 @@ import path from 'node:path'; import { platformWriteSync } from './shell-command-projection.cjs'; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; import { realClock } from './clock.cjs'; -import { clampPercent } from './phase-lifecycle.cjs'; +import { clampPercent, renderProgressBar } from './phase-lifecycle.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- core-utils.cjs is an export= CommonJS module import coreUtilsMod = require('./core-utils.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports @@ -382,8 +382,7 @@ function buildStateMd(phaseMap: PhaseMapEntry[]): string { const currentSlug = currentEntry ? slugify(currentEntry.slice.title) : 'complete'; const status = currentEntry ? 'Ready to plan' : 'All phases complete'; - const filled = Math.round(pct / 10); - const bar = `[${'█'.repeat(filled)}${'░'.repeat(10 - filled)}]`; + const bar = `[${renderProgressBar(pct, 10)}]`; const today = realClock.localToday(); return [ diff --git a/src/phase-lifecycle.cts b/src/phase-lifecycle.cts index 67f3a361e..62111d5cd 100644 --- a/src/phase-lifecycle.cts +++ b/src/phase-lifecycle.cts @@ -151,3 +151,61 @@ export function clampPercent(completed: number, total: number): number { if (!total || total <= 0) return 0; return clampPercentFromFraction(completed / total); } + +/** + * How many cells of a `width`-cell progress bar are filled for `percent`. + * + * #4294: the RENDER half's kernel — the counterpart of `clampPercentFromFraction` + * one layer down. That function owns `fraction -> integer percent`; this one + * owns `integer percent -> filled cells`, and is the single place that rounding + * is expressed. Five inline copies of the rounding carried it before this + * change — three in `commands.cts`, and one each in `gsd2-import.cts` and + * `formatProgressMachineSegment` here; the latter two used `/ 10` with the + * width already substituted (`pct` in `gsd2-import.cts`, `clamped` in the + * formatter). (#4294 counts SIX call sites because it counts + * `cmdStateUpdateProgress` and `syncCore` separately; #4231 had already routed + * both through `formatProgressMachineSegment` — as it does + * `applyPostSyncPreservation` — so by this branch's base they share one copy.) + * Every copy saturated: at width 10 that rounds to a full bar from 95 up, at + * width 20 from 98 up, so a project at 19/20 plans drew the same bar as a + * shipped one beside a number that said otherwise. + * + * Contract: + * - A FULL bar is reserved for an actual 100. Below 100 the fill is held one + * cell short of the width. This is the only departure from the old formula: + * at width 10 exactly 95-99 move (10 -> 9), at width 20 exactly 98-99 + * (20 -> 19); every other percent in 0-100 rounds as before. + * - `null` / `undefined` / non-finite renders an empty bar, matching the + * `percent === null ? 0 : ...` guard the `progress` renderers already carried. + * - Out-of-range input is clamped to 0-100 before rounding, so the count is + * always within `[0, width]` and a `'░'.repeat(width - filled)` can never + * be handed a negative count (the old inline form threw `RangeError` at + * 120%). A non-positive width yields 0. + * + * Callers wanting the glyph run call `renderProgressBar`; this is exported so + * the rounding rule can be pinned directly against the legacy curve. + */ +export function progressBarFilledCells(percent: number | null | undefined, width: number): number { + const cells = Number.isFinite(width) && width > 0 ? Math.floor(width) : 0; + if (cells === 0) return 0; + if (typeof percent !== 'number' || !Number.isFinite(percent)) return 0; + const clamped = Math.max(0, Math.min(100, percent)); + if (clamped >= 100) return cells; + // Scale by the WIDTH, not by 100 — this is cells-from-percent, not the + // completion-ratio derivation lint-completion-ratio-drift.cjs guards. + const rounded = Math.round((clamped / 100) * cells); + return Math.min(rounded, cells - 1); +} + +/** + * Render the glyph run of a `width`-cell progress bar for `percent` — + * `'█'` for each filled cell, `'░'` for the rest, always exactly `width` + * glyphs (an empty string for a non-positive width). Brackets, the printed + * percent and any suffix stay with the caller; the fill rule is + * `progressBarFilledCells` (#4294). + */ +export function renderProgressBar(percent: number | null | undefined, width: number): string { + const filled = progressBarFilledCells(percent, width); + const cells = Number.isFinite(width) && width > 0 ? Math.floor(width) : 0; + return '█'.repeat(filled) + '░'.repeat(cells - filled); +} diff --git a/src/state-transition.cts b/src/state-transition.cts index 9a1875c25..c08f3dbc6 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -20,7 +20,7 @@ import { stateReplaceField, stateExtractField, stateReplaceFieldIfTemplate, stat import { KNOWN_TEMPLATE_DEFAULTS, toFiniteNumber, computeProgressPercent } from './state-document.cjs'; import { tokenizeHeadings } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; -import { deriveProgressFromRoadmap, clampPercent, clampPercentFromFraction } from './phase-lifecycle.cjs'; +import { deriveProgressFromRoadmap, clampPercent, clampPercentFromFraction, renderProgressBar } from './phase-lifecycle.cjs'; import { escapeRegex } from './pattern.cjs'; // #4129: the completion-ratio kernel for the resync-arm ratchet's percent // (planning-scope's SCOPE — state-document's own dependency, no cycle here: @@ -38,12 +38,13 @@ export function formatProgressMachineSegment(percent: number): string { // ADR-3180 Decision 7: rounding and the 100 ceiling belong to the // completion-ratio kernel. The floor is added here because this helper is // also fed persisted frontmatter values (hand-editable, unlike the - // count-shaped entries into that kernel), and `'░'.repeat` throws on a - // negative count. Bar and printed percent use the clamped value so the two - // halves of the segment can never disagree. + // count-shaped entries into that kernel). Bar and printed percent use the + // clamped value so the two halves of the segment can never disagree. + // #4294: the CELL count is the render kernel's — `renderProgressBar` holds a + // sub-100 percent one cell short of full, so `[██████████]` beside `95%` + // cannot recur here as a seventh inline copy of the rounding. const clamped = Math.max(0, clampPercentFromFraction(percent / 100)); - const filled = Math.round(clamped / 10); - return `[${'█'.repeat(filled)}${'░'.repeat(10 - filled)}] ${clamped}%`; + return `[${renderProgressBar(clamped, 10)}] ${clamped}%`; } // Consumers (a future STATE.md writer that bypasses all three reintroduces the diff --git a/tests/4213-progress-helpers.property.test.cjs b/tests/4213-progress-helpers.property.test.cjs index c3b7a3583..7fa10b75e 100644 --- a/tests/4213-progress-helpers.property.test.cjs +++ b/tests/4213-progress-helpers.property.test.cjs @@ -56,6 +56,14 @@ describe('formatProgressMachineSegment properties (#4213 review finding)', () => assert.equal(formatProgressMachineSegment(105), '[██████████] 100%'); assert.equal(formatProgressMachineSegment(-30), '[░░░░░░░░░░] 0%'); }); + + // #4294: the bar is drawn by the render kernel, so a sub-100 percent can no + // longer round up to a full bar beside a number that says otherwise. + test('#4294 boundary literals: 95 and 99 hold one cell short of full; 94 is unchanged', () => { + assert.equal(formatProgressMachineSegment(94), '[█████████░] 94%'); + assert.equal(formatProgressMachineSegment(95), '[█████████░] 95%'); + assert.equal(formatProgressMachineSegment(99), '[█████████░] 99%'); + }); }); describe('stateReplaceProgressPercent properties (#4213 review finding)', () => { diff --git a/tests/phase-lifecycle.test.cjs b/tests/phase-lifecycle.test.cjs index b947023af..8a0a2b513 100644 --- a/tests/phase-lifecycle.test.cjs +++ b/tests/phase-lifecycle.test.cjs @@ -4,7 +4,7 @@ * Behavioral tests for phase-lifecycle.cjs * * Module: gsd-core/bin/lib/phase-lifecycle.cjs - * Exports: deriveProgressFromRoadmap, clampPercent + * Exports: deriveProgressFromRoadmap, clampPercent, progressBarFilledCells, renderProgressBar * * ADR-2143 (epic #2143) migrated deriveProgressFromRoadmap from position-based * regexes to the markdown-table schema registry (collectSection + parseMarkdownTable @@ -21,7 +21,13 @@ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); -const { deriveProgressFromRoadmap, clampPercent } = require('../gsd-core/bin/lib/phase-lifecycle.cjs'); +const { + deriveProgressFromRoadmap, + clampPercent, + progressBarFilledCells, + renderProgressBar, +} = require('../gsd-core/bin/lib/phase-lifecycle.cjs'); +const fc = require('./helpers/fast-check-setup.cjs'); describe('deriveProgressFromRoadmap', () => { test('parses the 4-column flat Progress table (behaviour preserved)', () => { @@ -182,3 +188,91 @@ describe('clampPercent', () => { assert.equal(clampPercent(3, -1), 0); }); }); + +describe('renderProgressBar / progressBarFilledCells (#4294 — the render half has one owner)', () => { + // The formula every call site used to carry inline. Kept here as the + // reference curve the kernel is pinned against, NOT as an implementation. + const legacyFilled = (percent, width) => Math.round((percent / 100) * width); + const WIDTHS_IN_USE = [10, 20]; + + const count = (bar, glyph) => bar.split('').filter((ch) => ch === glyph).length; + + test('a full bar is reserved for 100: the last sub-100 percents are held one cell short', () => { + // width 10 — 94 already rounded to 9; 95-99 used to round to 10. + assert.equal(renderProgressBar(94, 10), '█████████░'); + assert.equal(renderProgressBar(95, 10), '█████████░'); + assert.equal(renderProgressBar(99, 10), '█████████░'); + assert.equal(renderProgressBar(100, 10), '██████████'); + // width 20 — 97 already rounded to 19; 98-99 used to round to 20. + assert.equal(renderProgressBar(97, 20), '███████████████████░'); + assert.equal(renderProgressBar(98, 20), '███████████████████░'); + assert.equal(renderProgressBar(99, 20), '███████████████████░'); + assert.equal(renderProgressBar(100, 20), '████████████████████'); + // The bottom of the range is untouched. + assert.equal(renderProgressBar(0, 10), '░░░░░░░░░░'); + assert.equal(renderProgressBar(5, 10), '█░░░░░░░░░'); + assert.equal(renderProgressBar(50, 10), '█████░░░░░'); + }); + + test('exhaustive 0-100 curve at both widths: identical to the legacy formula except exactly the saturating percents', () => { + const expectedMoved = { 10: [95, 96, 97, 98, 99], 20: [98, 99] }; + for (const width of WIDTHS_IN_USE) { + const moved = []; + for (let percent = 0; percent <= 100; percent++) { + const legacy = legacyFilled(percent, width); + const actual = progressBarFilledCells(percent, width); + if (actual !== legacy) { + moved.push(percent); + // The only permitted departure: legacy saturated below 100, kernel holds one short. + assert.equal(legacy, width, `width ${width}, ${percent}%: moved but legacy was not saturated`); + assert.equal(actual, width - 1, `width ${width}, ${percent}%: moved to ${actual}, expected ${width - 1}`); + } + // The rendered glyph run agrees with the count and is always width glyphs long. + const bar = renderProgressBar(percent, width); + assert.equal(bar.length, width); + assert.equal(count(bar, '█'), actual); + assert.equal(count(bar, '░'), width - actual); + } + assert.deepEqual(moved, expectedMoved[width], `width ${width}: the set of moved percents`); + } + }); + + test('withheld / non-finite percent renders an empty bar (the `percent === null ? 0` guard, centralized)', () => { + for (const value of [null, undefined, NaN, Infinity, -Infinity, '50']) { + assert.equal(renderProgressBar(value, 10), '░░░░░░░░░░', `value ${String(value)}`); + assert.equal(progressBarFilledCells(value, 10), 0, `value ${String(value)}`); + } + }); + + test('out-of-range percent is clamped, never thrown: 120% is a full bar, -30% an empty one', () => { + // The old inline form threw `RangeError: Invalid count value: -2` at 120%. + assert.equal(renderProgressBar(120, 10), '██████████'); + assert.equal(renderProgressBar(-30, 10), '░░░░░░░░░░'); + assert.equal(renderProgressBar(100.4, 10), '██████████'); + assert.equal(renderProgressBar(99.9, 10), '█████████░'); + }); + + test('property: for any finite percent and width in use, exactly width glyphs, and full only at >= 100', () => { + fc.assert( + fc.property( + fc.double({ noDefaultInfinity: true, noNaN: true }), + fc.constantFrom(...WIDTHS_IN_USE), + (percent, width) => { + const bar = renderProgressBar(percent, width); + assert.equal(bar.length, width); + assert.match(bar, /^█*░*$/); + const full = count(bar, '█') === width; + assert.equal(full, percent >= 100, `${percent}% at width ${width}: full=${full}`); + }, + ), + ); + }); + + test('degenerate widths: non-positive yields nothing, width 1 fills only at 100', () => { + assert.equal(renderProgressBar(50, 0), ''); + assert.equal(renderProgressBar(50, -3), ''); + assert.equal(progressBarFilledCells(50, 0), 0); + assert.equal(renderProgressBar(99, 1), '░'); + assert.equal(renderProgressBar(100, 1), '█'); + }); +});