From 4418f639992989da8d08a26361dc3c086ce977e8 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Mon, 27 Jul 2026 14:54:03 +0000 Subject: [PATCH] fix(engine): push progress on maxIterations and percentComplete changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit progressChanged only diffed phase, status, iteration, and the activity tail, but ProgressSnapshot also displays maxIterations and the derived percentComplete. When an operator raised the iteration budget mid-run those two moved while the other axes held, so no push was emitted and the customer-facing progress bar went stale on a real, displayed change. Compare both fields before the existing axis chain and cover the true and false path of each. loop-progress.ts is graded by two Codecov flags — the root vitest suite (backend) and the package's own node:test suite under c8 (engine, #9064) — so both get a matching case, otherwise the flags disagree on the new lines and the merged patch report undercounts them. --- packages/loopover-engine/src/loop-progress.ts | 2 + .../test/loop-progress.test.ts | 40 +++++++++++++++++++ test/unit/loop-progress.test.ts | 29 ++++++++++++++ 3 files changed, 71 insertions(+) create mode 100644 packages/loopover-engine/test/loop-progress.test.ts diff --git a/packages/loopover-engine/src/loop-progress.ts b/packages/loopover-engine/src/loop-progress.ts index 73e9fada3d..6d0c5a224f 100644 --- a/packages/loopover-engine/src/loop-progress.ts +++ b/packages/loopover-engine/src/loop-progress.ts @@ -81,6 +81,8 @@ function activityChanged(prev: readonly LoopProgressActivity[], next: readonly L * always pushes. Compares the displayed axes: phase, status, iteration, and the activity tail's contents. */ export function progressChanged(prev: ProgressSnapshot | null, next: ProgressSnapshot): boolean { if (prev === null) return true; + if (prev.maxIterations !== next.maxIterations) return true; + if (prev.percentComplete !== next.percentComplete) return true; return ( prev.phase !== next.phase || prev.status !== next.status || diff --git a/packages/loopover-engine/test/loop-progress.test.ts b/packages/loopover-engine/test/loop-progress.test.ts new file mode 100644 index 0000000000..2cfa0eeb48 --- /dev/null +++ b/packages/loopover-engine/test/loop-progress.test.ts @@ -0,0 +1,40 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { buildProgressSnapshot, progressChanged, type LoopProgressState } from "../dist/index.js"; + +function running(overrides: Partial = {}): LoopProgressState { + return { iteration: 2, maxIterations: 5, phase: "coding", status: "running", ...overrides }; +} + +test("barrel: the public entrypoint re-exports the progress snapshot helpers (#4800)", () => { + assert.equal(typeof buildProgressSnapshot, "function"); + assert.equal(typeof progressChanged, "function"); +}); + +// #9323: maxIterations and percentComplete are displayed axes too, so progressChanged must push when either +// moves — the root vitest suite covers this, but engine source is also graded by this node:test suite under +// c8 (#9064). Without the same coverage here the two Codecov flags disagree on the new lines' hit state. +test("progressChanged: pushes when the iteration budget (maxIterations) is raised mid-run", () => { + const prev = buildProgressSnapshot(running({ iteration: 2, maxIterations: 5, recentActivity: [{ step: "a" }] })); + const next = buildProgressSnapshot(running({ iteration: 2, maxIterations: 10, recentActivity: [{ step: "a" }] })); + // Same iteration/phase/status/activity, but the budget (and therefore the displayed percent) moved. + assert.equal(next.maxIterations, 10); + assert.equal(prev.percentComplete, 40); + assert.equal(next.percentComplete, 20); + assert.equal(progressChanged(prev, next), true); +}); + +test("progressChanged: pushes when only the derived percentComplete differs", () => { + const prev = buildProgressSnapshot(running({ recentActivity: [{ step: "a" }] })); + // maxIterations held equal so the maxIterations guard falls through to the percentComplete one. + assert.equal(progressChanged(prev, { ...prev, percentComplete: (prev.percentComplete ?? 0) + 10 }), true); +}); + +test("progressChanged: does not push when every displayed axis (maxIterations/percentComplete included) holds", () => { + const prev = buildProgressSnapshot(running({ recentActivity: [{ step: "a" }] })); + const next = buildProgressSnapshot(running({ recentActivity: [{ step: "a" }] })); + assert.equal(next.maxIterations, prev.maxIterations); + assert.equal(next.percentComplete, prev.percentComplete); + assert.equal(progressChanged(prev, next), false); +}); diff --git a/test/unit/loop-progress.test.ts b/test/unit/loop-progress.test.ts index b4e67244c9..fc4c022384 100644 --- a/test/unit/loop-progress.test.ts +++ b/test/unit/loop-progress.test.ts @@ -67,6 +67,35 @@ describe("progressChanged — push on change, not on a fixed interval (#4800)", expect(progressChanged(base, buildProgressSnapshot(running({ recentActivity: [{ step: "a" }] })))).toBe(false); }); + // #9323: maxIterations and percentComplete are displayed axes too. Raising the iteration budget mid-run + // leaves phase/status/iteration/activity untouched but moves both of these, so the diff must push. + it("pushes when only maxIterations (and the derived percentComplete) changes", () => { + const prev = buildProgressSnapshot(running({ iteration: 2, maxIterations: 5, recentActivity: [{ step: "a" }] })); + const next = buildProgressSnapshot(running({ iteration: 2, maxIterations: 10, recentActivity: [{ step: "a" }] })); + // Same iteration/phase/status/activity, but the budget (and therefore the displayed percent) moved. + expect(next.maxIterations).toBe(10); + expect(prev.percentComplete).toBe(40); + expect(next.percentComplete).toBe(20); + expect(progressChanged(prev, next)).toBe(true); + }); + + // A percentComplete-only delta (every other displayed axis, maxIterations included, held identical) still + // pushes — progressChanged diffs the displayed field, so this isolates the percentComplete comparison. + it("pushes when only percentComplete differs", () => { + const prev = buildProgressSnapshot(running({ recentActivity: [{ step: "a" }] })); + expect(progressChanged(prev, { ...prev, percentComplete: (prev.percentComplete ?? 0) + 10 })).toBe(true); + }); + + // REGRESSION: a guard against over-triggering — with all six displayed axes (the two new ones included) + // identical between snapshots, progressChanged must still return false. + it("does not push when all six displayed axes are identical", () => { + const prev = buildProgressSnapshot(running({ recentActivity: [{ step: "a" }] })); + const next = buildProgressSnapshot(running({ recentActivity: [{ step: "a" }] })); + expect(next.maxIterations).toBe(prev.maxIterations); + expect(next.percentComplete).toBe(prev.percentComplete); + expect(progressChanged(prev, next)).toBe(false); + }); + // #6171: the tail is capped, so past the cap every new event evicts the oldest and the LENGTH stops moving. // A length-only check went permanently blind here — exactly on the long runs that stream the most. describe("activity tail at its cap (#6171)", () => {