Repository navigation
aggregator(forecast): derive epic ETA bands and explainable stall risk (#3982) - #5236
Conversation
![developing-today[bot]](https://s.lequ.pw/_assets/images/gravatars/gravatar-user-420.png){kind=avatar}
#3982) Pure derivations over the #3981 item-history cutoffs (design sections 1, 3, 4): - deriveEpicEta: direct native child issue closures as units (PRs never count, reopened children are remaining work), complete weekly buckets only including zero weeks, trailing 4-week primary / 8-week comparison windows, P50/P80 dates from median / lower-quartile pace, evidence-grade confidence, named borrowed baseline or not_enough_history / no_measurable_pace with no dates below the 3-completion / 4-week floor, effort kept explicit (unsized + open-ended named). - deriveStallRisk: versioned 0-100 rule score (blocked-edge lower-bound age +30, updatedAt activity proxy +20, queue heartbeat +30, requeues +20), reason codes with timestamps, unknown signals contribute nothing and reduce coverage, >=60 is informational at-risk only and never escalates by itself. Tests: 16 new cases in test/forecast-3982.test.ts (missing history, zero pace, reopen, sparse, borrowed baseline, PR exclusion, effort, each stall signal); 29/29 pass with the existing item-history tests on the laptop.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c80fe80387
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const paceP80 = Math.max(lowerQuartile(paceSeries), Number.EPSILON); | ||
| // Whole weeks, rounded up: a partial week is not a completion date. | ||
| result.p50 = addDays(asOf, Math.ceil(remainingUnits / paceP50) * 7); | ||
| result.p80 = addDays(asOf, Math.ceil(remainingUnits / paceP80) * 7); |
There was a problem hiding this comment.
With four complete weekly buckets such as [0, 1, 1, 1] and at least one remaining child, the local evidence floor passes and the median pace is positive, but lowerQuartile returns zero. Substituting Number.EPSILON produces roughly 4.5 quadrillion weeks per remaining issue, exceeding JavaScript's date range and causing addDays(...).toISOString() to throw RangeError. A single observed zero-completion week can therefore crash an otherwise eligible forecast. Handle an unmeasurable conservative pace explicitly rather than converting it into a finite denominator.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 40ed243 (task 8962, lane code-h): the EPSILON substitution is gone. A zero lower-quartile pace now leaves p80 explicitly null with a limitation naming the unmeasurable conservative pace, and a new etaDate guard returns null for horizons outside the representable Date range instead of throwing. Regression test: "zero lower-quartile pace: P50 present, P80 explicitly unmeasurable, no crash" (primary weeks [0,1,1,1]).
There was a problem hiding this comment.
Fixed in 40ed243 (already on this PR's head before this reply): the EPSILON substitution is gone — a zero lower-quartile pace now returns p80: null with an explicit limitation, and etaDate() guards the representable Date range so no pace can make toISOString() throw. Covered by the test "zero lower-quartile pace: P50 present, P80 explicitly unmeasurable, no crash". — task 8962, lane code-e
| latest.items | ||
| .filter( | ||
| (i) => | ||
| i.kind !== "pr" && (i.parentRef === input.subjectRef || i.epicRef === input.subjectRef), |
There was a problem hiding this comment.
When an epic contains nested sub-issues, this predicate includes grandchildren as independent forecast units because epicRef represents ancestor membership: resolveEpicRefs in projects.ts walks the entire parent chain. For an epic with child A and A's child B, both A and B enter the remaining/completed counts and throughput series, contrary to the direct-child unit specified in the module and forecasting design §1. Infer direct children from parentRef rather than treating recursive epicRef membership as a direct relationship.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 40ed243 (task 8962, lane code-h): inferred scope now requires parentRef === subject only. epicRef is ancestor membership (resolveEpicRefs walks the whole parent chain), so it no longer counts grandchildren as independent units; the EpicEtaInput doc says the same. Regression test: "inferred scope counts direct children only, not grandchildren via epicRef".
There was a problem hiding this comment.
Fixed in 40ed243: inferred scope now derives direct native children via parentRef === subject (epicRef ancestor membership is no longer used for scope), exactly as this finding requires. Covered by "inferred scope counts direct children only, not grandchildren via epicRef". — task 8962, lane code-e
| const complete = completeCutoffs(input.cutoffs); | ||
| const latest = complete[complete.length - 1]; | ||
| const asOf = input.asOf ?? latest?.cutoffAt ?? new Date(0).toISOString(); |
There was a problem hiding this comment.
When asOf precedes the newest stored cutoff, the function still selects the newest item states and computes throughput from the entire history, then anchors those future-derived results to the earlier instant. For example, a child closed after the requested date is already counted as completed in that date's forecast. This leaks future outcomes into historical forecasts and violates the rolling-origin evaluation requirement in forecasting design §5. Restrict the history used for scope, transitions, and coverage to cutoffs at or before asOf; deriveStallRisk has the same unrestricted-history behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 40ed243 (task 8962, lane code-h): both deriveEpicEta and deriveStallRisk now scope to cutoffs at or before asOf before selecting the latest cutoff, computing transitions (weeklyTransitions over the scoped set), coverage, and blocker episodes — future outcomes no longer leak into historical forecasts. Regression test: "asOf excludes later cutoffs: a closure after asOf is not yet a completion".
There was a problem hiding this comment.
Fixed in 40ed243: deriveEpicEta and deriveStallRisk scope the history to cutoffs at or before asOf before selecting the latest state or computing transitions, so future outcomes no longer leak into historical forecasts. Covered by "asOf excludes later cutoffs: a closure after asOf is not yet a completion". (The representation flaw inside that comparison is the round-2 finding, fixed separately in e10e253.) — task 8962, lane code-e
| for (const blocker of item.blockedBy) { | ||
| const first = blockerFirstObserved(input.cutoffs, input.ref, blocker); | ||
| if (first && first.lowerBoundAgeDays >= 7) { |
There was a problem hiding this comment.
When an item retains a blockedBy relation for at least seven days but the referenced blocker is now closed in the latest cutoff, this loop still assigns 30 points and reports an unresolved blocker. History capture preserves raw relations, so their presence does not establish that the blocker remains open: graph.ts explicitly treats closed/merged in-pool blockers as resolved, and graph.test.ts covers this persistent-edge scenario. Combined with a firing heartbeat signal, this incorrectly reaches the at-risk threshold. Check the blocker's latest state before assigning blocked-edge points, while preserving the existing unresolved treatment for out-of-pool blockers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 40ed243 (task 8962, lane code-h): the blocked-edge loop now checks the blocker at the latest cutoff — a blocker held closed/merged there is resolved and contributes nothing (matching graph.ts blockerUnresolved), while an out-of-pool blocker still counts as unresolved. Regression test: "a blocker closed at the latest cutoff is resolved: no blocked-edge points".
There was a problem hiding this comment.
Fixed in 40ed243: the blocked-edge loop now skips blockers the latest cutoff holds as closed/merged (resolved, matching graph.ts), while absent-from-pool blockers stay unresolved. Covered by "a blocker closed at the latest cutoff is resolved: no blocked-edge points". — task 8962, lane code-e
- P1: zero lower-quartile pace no longer becomes an EPSILON denominator (crashed toISOString via an out-of-range date); P80 is explicitly null with a limitation, and etaDate guards unrepresentable horizons. - P2: inferred epic scope counts direct native children only (parentRef), not grandchildren via ancestor epicRef. - P2: deriveEpicEta and deriveStallRisk restrict scope/transitions/coverage to cutoffs at or before asOf (rolling-origin honesty). - P2: blocked-edge risk skips blockers the latest cutoff holds closed/merged (resolved, matching graph.ts); out-of-pool blockers stay unresolved. - Regression tests for all four findings; targeted suite 20/20 on the laptop.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40ed243f24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // cutoffs are future outcomes and must not leak into scope, | ||
| // transitions, or coverage. | ||
| const scopedCutoffs = input.asOf | ||
| ? input.cutoffs.filter((c) => c.cutoffAt <= input.asOf!) |
There was a problem hiding this comment.
When cutoff and asOf timestamps use different valid ISO representations, this string comparison selects the wrong history. Fresh evidence after the earlier filtering fix is that isItemHistoryCutoff accepts optional fractional seconds and timezone offsets through isIsoDate: cutoff 2026-11-02T00:00:00Z is excluded for the identical instant 2026-11-02T00:00:00.000Z, while cutoff 2026-11-02T00:00:00-05:00 is included for asOf: "2026-11-02T01:00:00Z" despite occurring four hours later. This can discard the latest observation or leak future outcomes into historical forecasts. Compare parsed instants in both this filter and the equivalent deriveStallRisk filter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e10e253: asOf scoping in both derivations and the completeCutoffs sort now compare timestamps as instants via Date.parse (atOrBefore/instantOrder helpers, string-order fallback only for unparseable values). Three new regression tests cover the equal-instant fractional-second case and the -05:00 offset case for both deriveEpicEta and deriveStallRisk. bun test forecast-3982: 23 pass, 0 fail. — task 8962, lane code-e
…gs (#3982) Codex round-2 finding on PR #5236 (forecast.ts:155): the asOf scoping filter and cutoff sort compared ISO timestamps as strings, so equivalent instants in different representations (fractional seconds, timezone offsets) were scoped/sorted wrong. Add atOrBefore/instantOrder helpers (Date.parse, string fallback for unparseable values), use them in both deriveEpicEta and deriveStallRisk scoping and in completeCutoffs sorting. Three regression tests cover the equal-instant and offset cases for both derivations. bun test forecast-3982: 23 pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e10e253be6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| resolvedBlockers++; | ||
| continue; | ||
| } | ||
| const first = blockerFirstObserved(scopedCutoffs, input.ref, blocker); |
There was a problem hiding this comment.
When asOf is omitted and an incomplete cutoff follows the latest complete cutoff, the result uses the complete cutoff's timestamp but this call still receives the later history. blockerFirstObserved measures age through the last supplied cutoff regardless of its coverage. For example, a newly observed edge at a complete September 7 cutoff followed by an unavailable September 14 cutoff receives 30 points in a result dated September 7. Unlike the earlier explicit-asOf issue, the remaining evidence is the unfiltered default path at lines 427–432. Bound the helper's history to the effective asOf so later unavailable observations cannot prematurely trigger blocked-edge risk.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the commit pushed on top of this PR (task 8962, lane code-a): deriveStallRisk now bounds the history handed to blockerFirstObserved to the effective asOf (boundedCutoffs = scopedCutoffs.filter(atOrBefore(cutoffAt, asOf))), so when asOf is omitted, later unavailable cutoffs no longer age an edge inside a result dated at the latest complete cutoff. Regression test "default asOf bounds blocker age to the latest complete cutoff": edge first observed at the complete Sep 7 cutoff with an unavailable Sep 14 cutoff also listing it now contributes 0 (was 30). bun test test/forecast-3982.test.ts on the laptop: 25 pass, 0 fail.
| .filter((i) => i.kind !== "pr" && i.parentRef === input.subjectRef) | ||
| .map((i) => i.ref); | ||
| const childSet = new Set(childRefs); | ||
| const children = latest.items.filter((i) => childSet.has(i.ref) && i.kind !== "pr"); |
There was a problem hiding this comment.
When explicit childRefs contains a child absent from the latest complete cutoff, this intersection silently drops that child rather than recording its state as unknown. For example, with requested children [A, B] and only closed A present in the latest curated pool, the function reports zero remaining units and returns p50 = p80 = asOf through the nothing-left branch, although B's completion was never observed. A complete source cutoff establishes coverage of the fetched pool, not the presence or completion of every explicitly requested child. Report missing requested refs as a scope limitation and avoid declaring the subject complete while their states are unknown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the commit pushed on top of this PR (task 8962, lane code-a): explicitly requested childRefs absent from the latest complete cutoff are now computed as missingRefs, reported as a scope limitation naming the refs ("state unknown"), and block the nothing-left branch — when every observed child is complete but requested children are unobserved, the result is not_enough_history with null p50/p80 instead of declaring the subject complete. Regression test "explicit childRefs absent from the latest cutoff are unknown, not complete" covers the [A closed, B unobserved] case plus the observed-only control (still p50 = p80 = asOf). bun test test/forecast-3982.test.ts on the laptop: 25 pass, 0 fail.
…quested children block completion (#3982)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee57a979ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const paceP50 = median(paceSeries); | ||
| if (paceP50 <= 0) { | ||
| result.status = "no_measurable_pace"; | ||
| limitations.push("observed weekly pace is zero in the primary window; no date is returned"); | ||
| return result; |
There was a problem hiding this comment.
When four complete primary weeks contain [0, 0, 0, 4] closures and work remains, the local evidence floor passes, but the median weekly bucket is zero, so this branch returns no_measurable_pace and omits both dates despite positive measured throughput. This prevents forecasts for batch-delivery scopes with sufficient evidence. The forecasting design §1 specifies resampling completion buckets to estimate remaining-work duration; accumulating those sampled buckets produces finite completion-time percentiles even when most individual weeks have zero closures. Estimate that duration distribution rather than treating a zero single-week median as no measured pace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Dispositioned as a tracked follow-up rather than fixed in this PR (task 8962, lane code-a). The finding is correct against the design: §1 of doc/2026-11-01T21-07-00Z_design_pm-stack_forecasting.md (line 36) specifies a resampling estimator over complete weekly buckets, and the median/lower-quartile pace in this PR is a simplification that returns no_measurable_pace for bursty series like [0, 0, 0, 4]. Replacing the estimator is a model change — it alters the pinned "zero lower-quartile pace → P80 unmeasurable" behavior and raises a FORECAST_MODEL_VERSION question — so it is filed as its own issue with acceptance criteria (deterministic seeded resampling, bursty-case forecast, truly-zero pace still no_measurable_pace, pinned-test updates, version decision): issue #5274. This PR keeps the remaining findings fixed (rounds 1–3) and merges with the estimator limitation tracked, not silent.
Implements #3982 (task 8962): pure forecast derivations over the #3981 item-history cutoffs, per the forecasting design §1, §3, §4.
What
pm-stack/aggregator/src/forecast.ts—deriveEpicEtaandderiveStallRisk, both pure and deterministic, model versionsforecast/v1andstall-risk/v1.not_enough_historywith no dates; zero pace in a full primary window isno_measurable_pace. Effort stays explicit — remaining work is reported per effort label, unsized counted as unsized,l/xlflagged open-ended; no invented precision.updatedAtactivity proxy ≥7d (+20, labelled a proxy), queue heartbeat missed ×2 / lease near expiry (+30), ≥2 requeues in 14d (+20). Unsupplied signals stay unknown, contribute nothing, and reduce evidence coverage. Score ≥60 is informational "at risk" only;escalatesToNeedsYouis always false.pm-stack/aggregator/test/forecast-3982.test.ts— 16 cases: missing history, incomplete-week exclusion, steady pace, zero pace, sparse sample, borrowed baseline, reopen, PR exclusion, effort explicitness, and each stall signal including the 60-point at-risk boundary and the 100 cap.Tests
bun test test/forecast-3982.test.ts test/item-history.test.ts— 29 pass, 0 fail.bun build src/forecast.tsbundles clean.bunx tsc --noEmitcannot run in the fresh worktree (no node_modules installed —Cannot find type definition file for 'bun', pre-existing environment gap, no forecast-specific errors); CI typecheck will cover it.Refs #3982 — issue stays open until this merges.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.