Skip to content

fix(engine): detect maxIterations/percentComplete changes in progressChanged (#9323) - #9408

Closed
Yurii214 wants to merge 1 commit into
JSONbored:mainfrom
Yurii214:fix/9323-progresschanged-budget-axes
Closed

fix(engine): detect maxIterations/percentComplete changes in progressChanged (#9323)#9408
Yurii214 wants to merge 1 commit into
JSONbored:mainfrom
Yurii214:fix/9323-progresschanged-budget-axes

Conversation

@Yurii214

Copy link
Copy Markdown

Summary

progressChanged (packages/loopover-engine/src/loop-progress.ts) decides when a new ProgressSnapshot differs from the previous one enough to push to the customer's streaming surface — the on-change-not-polling design from #4800. Its doc comment says it compares the displayed axes, but it only checked phase, status, iteration, and the activity tail; it never compared maxIterations or the derived percentComplete.

So when an operator raises the iteration budget mid-run (maxIterations changes) while iteration / phase / status / recentActivity all stay the same, percentComplete (= iteration / maxIterations, #6773) silently changes value but progressChanged returned false — the surface never pushed the update, and the customer's progress bar went stale on a real, displayed number.

This adds both maxIterations and percentComplete to the existing OR-chain, so any change to a shown number streams on change. It is a pure diff function: buildProgressSnapshot is unchanged and no other behavior is touched. progressChanged's only real consumer is the streaming surface, which benefits directly.

Closes #9323

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (Closes #9323).

Validation

  • git diff --check — clean
  • npm run actionlint — pass
  • npm run typecheck — pass (whole repo)
  • npm run test:coverage — measured on the changed file: 100% of changed lines AND branches (23/23 branches), no uncovered lines. Also ran npm run test:changed (every test whose import graph the diff touches): 237 files / 3427 tests pass.
  • npm run test:workers — not run; the diff touches no worker-runtime code.
  • npm run build:mcp — pass (also built @loopover/engine, @loopover/miner)
  • npm run test:mcp-pack — not run; no MCP packaging surface change.
  • npm run ui:openapi:check — n/a; no API / OpenAPI schema change.
  • npm run ui:lint / ui:typecheck / ui:build — n/a; no UI (apps/loopover-ui) change.
  • npm audit --audit-level=moderate — see note below.
  • New behavior has tests for new branches: three cases added — budget-only change pushes, percentComplete-only change pushes, and all-six-axes-identical does not push (over-trigger guard).

If any required check was skipped, explain why:

  • The change is confined to one pure function in packages/loopover-engine/src/loop-progress.ts, with no API/OpenAPI, UI (apps/loopover-ui), Cloudflare-worker, wrangler binding, migration, or MCP-surface change — so test:workers, test:mcp-pack, ui:*, and ui:openapi:check cover subsystems this diff does not touch. Verified locally: git diff --check, actionlint, whole-repo typecheck, patch coverage 100% on the changed file (23/23 branches), test:changed (237 files / 3427 tests green), @loopover/engine build + engine-parity (drift-check and contract test, both pass).
  • npm audit flags only pre-existing advisories already present in the dependency tree; this PR changes no dependency (no package.json / package-lock.json diff), so it introduces none.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, PATs, private keys, raw trust scores, private rankings, or private maintainer evidence — pure logic + unit tests, none present.
  • Public GitHub text stays sanitized and low-noise — no public-surface text change.
  • Auth / cookie / CORS / GitHub App / Cloudflare / session negative-path tests — n/a; no such change.
  • API / OpenAPI / MCP behavior — n/a; no contract change.
  • UI live-data / empty-error-loading states — n/a; no UI change.
  • UI Evidence — n/a; no visible UI, frontend, or docs change (pure @loopover/engine logic).
  • Public docs / changelog — n/a; no changelog edit.

UI Evidence

Not applicable — this is a pure @loopover/engine diff-logic change with no visible UI, frontend, or docs surface.

Notes

  • percentComplete is derived from iteration / maxIterations, so in normal data it moves only when one of those does. It is compared as its own displayed axis defensively (and per the issue's requirement); the third regression test isolates that axis by constructing the ProgressSnapshot directly, since buildProgressSnapshot cannot produce a percentComplete change with iteration and maxIterations held equal.

…Changed (JSONbored#9323)

progressChanged compared only phase, status, iteration, and the activity tail,
so raising the iteration budget mid-run (maxIterations, and the derived
percentComplete = iteration / maxIterations) moved a displayed number while the
customer surface never pushed — the progress bar showed a stale percent. Add
both budget axes to the change diff so any shown-number change streams on change.

Closes JSONbored#9323
@Yurii214
Yurii214 requested a review from JSONbored as a code owner July 27, 2026 16:26
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent did not find any vulnerabilities or security issues in this PR.

@loopover-orb

loopover-orb Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - reject/close recommended

Review updated: 2026-07-27 16:51:00 UTC

2 files · 1 AI reviewer · 1 blocker · CI failing · dirty

🛑 Suggested Action - Reject/Close

Review summary
This is a straightforward, correctly-scoped fix: `progressChanged` now also compares `maxIterations` and `percentComplete`, closing the gap where a mid-run budget change silently failed to push an update to the streaming surface. `buildProgressSnapshot` is untouched, the fix lives at the right layer (the diff comparator itself, not a symptom), and the new tests exercise the real path — including isolating `percentComplete` alone by constructing a `ProgressSnapshot` directly rather than fabricating an impossible state. The PR is tightly scoped to the linked issue (#9323) with no scope creep.

Nits — 3 non-blocking

Why this is blocked

  • No linked issue detected: The PR cites an issue number, but it could not be verified as a currently open issue. — If this PR is intended to solve an issue, link it explicitly in the PR body.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. No linked issue detected: The PR cites an issue number, but it could not be verified as a currently open issue. — If this PR is intended to solve an issue, link it explicitly in the PR body.

CI checks failing

  • codecov/patch — 77.77% of diff hit (target 99.00%)

Decision drivers

  • ❌ Code review — 1 blocker (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9323
Related work ⚠️ Same linked issue: #9400 Another open PR references the same linked issue.
Change scope ❌ 8/20 High review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 16 registered-repo PR(s), 10 merged, 1 issue(s).
Contributor context ✅ Confirmed Gittensor contributor Yurii214; Gittensor profile; 16 PR(s), 1 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The diff adds prev.maxIterations !== next.maxIterations and prev.percentComplete !== next.percentComplete to progressChanged's OR-chain exactly as required, without touching activityChanged or buildProgressSnapshot's computation. It also adds a true-path test (maxIterations-only change causing progressChanged to return true) and a false-path regression test (all six axes identical returning false)

Review context
Contributor next steps
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Decision record
  • action: close · clause: policy_close:heuristic
  • config: 6d557b1ba940cbbecc95c39f530ca31941582cb91c69ea37724ba303cd651cc8 · pack: oss-anti-slop · ci: failed
  • record: 642fffb388fd647600e3dba23cba5293500a4f84140c6ccb6c6d2376fe53a21f (schema v5, head 0248e37)

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.48%. Comparing base (7e98041) to head (0248e37).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
packages/loopover-engine/src/loop-progress.ts 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9408      +/-   ##
==========================================
- Coverage   89.49%   89.48%   -0.01%     
==========================================
  Files         838      838              
  Lines      109676   109684       +8     
  Branches    26116    26116              
==========================================
+ Hits        98154    98156       +2     
- Misses      10259    10261       +2     
- Partials     1263     1267       +4     
Flag Coverage Δ
backend 95.22% <ø> (-0.01%) ⬇️
engine 65.72% <77.77%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/loopover-engine/src/loop-progress.ts 76.53% <77.77%> (-0.14%) ⬇️

... and 1 file with indirect coverage changes

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 27, 2026
@loopover-orb loopover-orb Bot removed the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 27, 2026
@loopover-orb

loopover-orb Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (CI is failing (codecov/patch); conflicts with the base branch — resolve and open a fresh PR; No linked issue detected). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

@loopover-orb loopover-orb Bot closed this Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

engine: progressChanged ignores maxIterations/percentComplete changes in the loop-progress snapshot diff

1 participant