From 556d3470bdd01b405c1290280f01d1e28ae633ac Mon Sep 17 00:00:00 2001 From: Carlos Penaforte Date: Mon, 5 Oct 2026 15:28:22 -0300 Subject: [PATCH 1/2] fix(engine): omit review metrics when the PR reviews read failed With the reviews pass failed, every PR carries no reviews, so review coverage can only read 0% and the single-pass rate 100%, and the narrative stated both as fact. Like merge strategy under degraded enrichment, the review-derived fields are now omitted instead: the top-level review metrics, time to first review, and the review keys of the acceptance groups. The report and the trend comparison handle their absence, and METRICS.md says when they are omitted. Refs #236 Assisted-by: Claude Code Co-Authored-By: Claude Sonnet 5.5 --- docs/METRICS.md | 27 +++--- iris/analysis/trend_delta.py | 19 +++-- iris/metrics/aggregator.py | 38 ++++++--- iris/reports/narrative.py | 22 +++-- iris/reports/writer.py | 24 ++++-- tests/test_pr_fetch_degradation.py | 129 +++++++++++++++++++++++++++++ 6 files changed, 218 insertions(+), 41 deletions(-) diff --git a/docs/METRICS.md b/docs/METRICS.md index 413f0af..708da0e 100644 --- a/docs/METRICS.md +++ b/docs/METRICS.md @@ -241,7 +241,8 @@ its PR (if any) and aggregated by origin and AI tool. `AcceptanceMetrics`: `total_commits`, `commits_in_prs`, `pr_rate` (commits in PRs / total), `single_pass_rate` (PRs merged with -zero `CHANGES_REQUESTED` / PRs), `median_review_rounds`. +zero `CHANGES_REQUESTED` / PRs), `median_review_rounds`. The last two are omitted when `reviews` is in +`pr_enrichment_degraded` (§15). --- @@ -360,8 +361,8 @@ All fields require GitHub PR data. | `pr_cycle_time_buckets` | `{same_day, one_day, two_to_three_days, four_to_seven_days, seven_plus_days}` ints | same | same | | `pr_median_size_files` | int ≥ 0 | same | same | | `pr_median_size_lines` | int ≥ 0 | same | same | -| `pr_review_rounds_median` | float ≥ 0 | same | same | -| `pr_single_pass_rate` | float `0.0–1.0` | same | same | +| `pr_review_rounds_median` | float ≥ 0 | same | same, or `reviews` degraded | +| `pr_single_pass_rate` | float `0.0–1.0` | same | same, or `reviews` degraded | | `pr_enrichment_degraded` | list of `basic\|enrichment\|reviews\|fetch` | `ingestion/github_reader.py` | every read step succeeded, or PRs are absent by design | `pr_review_rounds_median` — median count of `CHANGES_REQUESTED` reviews @@ -385,9 +386,15 @@ the fields above it does not require PR data: a read that lost every PR still reports why. Absent when every step succeeded, and when PRs are absent by design — no `gh`, no GitHub remote — because Iris works without PRs. When present, this run's PR-derived fields may be missing or skewed — flow and -`commits_in_prs` go missing, and with `reviews` failed review coverage reads -0% and the single-pass rate 100% — and `merge_strategy` is `unknown` if -`enrichment` failed (§28). It is the first field to read when two runs on the +`commits_in_prs` go missing — and `merge_strategy` is `unknown` if +`enrichment` failed (§28). With `reviews` failed every PR carries no reviews, +so the fields derived from them are omitted rather than reported as the 0% +coverage and 100% single-pass an empty review list would give: +`pr_review_rounds_median`, `pr_single_pass_rate`, +`median_time_to_first_review_hours`, `human_review_coverage_pct`, +`human_approval_coverage_pct`, `human_review_coverage_by_intent`, +`human_review_coverage_by_origin_of_pr`, and the `single_pass_rate` and +`median_review_rounds` keys of `acceptance_by_origin` / `acceptance_by_tool`. It is the first field to read when two runs on the same commit disagree. --- @@ -627,7 +634,7 @@ say nothing about whether work is flowing or queued in a different shape. | Field | Unit | Source | Nullable when | |---|---|---|---| | `flow_efficiency_median` | float `0.0–1.0` | `analysis/flow_efficiency.py` | no merged PR survives the filters | -| `median_time_to_first_review_hours` | float ≥ 0 | same | no merged PR had a review | +| `median_time_to_first_review_hours` | float ≥ 0 | same | no merged PR had a review, or `reviews` degraded | | `time_in_phase_median_hours` | `Record` | same | same as `flow_efficiency_median` | | `flow_efficiency_by_intent` | `Record` | same | < `min_sample` (default 10) PRs in the segment | | `flow_efficiency_by_origin` | `Record` | same | no `commit_origin_map` provided, or < `min_sample` PRs in the segment | @@ -728,10 +735,10 @@ event at all. | Field | Unit | Source | Nullable when | |---|---|---|---| -| `human_review_coverage_pct` | float `0.0–1.0` | `analysis/human_review_coverage.py` | no merged PR in the window | +| `human_review_coverage_pct` | float `0.0–1.0` | `analysis/human_review_coverage.py` | no merged PR in the window, or `reviews` degraded | | `human_approval_coverage_pct` | float `0.0–1.0` | same | same | -| `human_review_coverage_by_intent` | `Record` | same | < `min_sample` (default 10) PRs in the segment | -| `human_review_coverage_by_origin_of_pr` | `Record` | same | no `commit_origin_map` provided, or < `min_sample` PRs in the segment | +| `human_review_coverage_by_intent` | `Record` | same | < `min_sample` (default 10) PRs in the segment, or `reviews` degraded | +| `human_review_coverage_by_origin_of_pr` | `Record` | same | no `commit_origin_map` provided, or < `min_sample` PRs in the segment, or `reviews` degraded | Definition (per merged PR, intermediate only): diff --git a/iris/analysis/trend_delta.py b/iris/analysis/trend_delta.py index 12b7807..f5664b8 100644 --- a/iris/analysis/trend_delta.py +++ b/iris/analysis/trend_delta.py @@ -210,13 +210,18 @@ def compute_trend_delta( "h", )) - deltas.append(_make_delta( - "pr_single_pass", - s["trend_label_pr_single_pass"], - baseline.pr_single_pass_rate * 100, - recent.pr_single_pass_rate * 100, - "pp", - )) + # Absent when the reviews pass of the PR read failed in either window. + if ( + baseline.pr_single_pass_rate is not None + and recent.pr_single_pass_rate is not None + ): + deltas.append(_make_delta( + "pr_single_pass", + s["trend_label_pr_single_pass"], + baseline.pr_single_pass_rate * 100, + recent.pr_single_pass_rate * 100, + "pp", + )) return TrendResult( baseline_days=baseline_days, diff --git a/iris/metrics/aggregator.py b/iris/metrics/aggregator.py index 79cae9d..9c3e85a 100644 --- a/iris/metrics/aggregator.py +++ b/iris/metrics/aggregator.py @@ -42,7 +42,7 @@ from iris.models.commit import Commit from iris.models.external import ExternalDORAData from iris.models.metrics import ReportMetrics -from iris.models.pull_request import DEGRADED_ENRICHMENT, PullRequest +from iris.models.pull_request import DEGRADED_ENRICHMENT, DEGRADED_REVIEWS, PullRequest logger = logging.getLogger(__name__) @@ -68,12 +68,18 @@ def aggregate( pr_fetch_degraded: Steps of the PR read that failed this run (`PullRequestFetch.degraded`). Reported as ``pr_enrichment_degraded``; when it includes ``enrichment`` the - merge strategy is not classified. + merge strategy is not classified; when it includes ``reviews`` + every review-derived field is omitted rather than reported as + the 0% / 100% an empty review list would give. Returns: ReportMetrics with all fields populated. PR fields are None when prs is None or empty. """ + # With the reviews pass failed every PR has no reviews: review-derived + # fields would read as fabricated facts, so they are left out. + reviews_degraded = DEGRADED_REVIEWS in pr_fetch_degraded + churn_result = calculate_churn(commits, churn_days) stab_result = calculate_stabilization(commits, churn_days) @@ -285,22 +291,30 @@ def aggregate( "total_commits": g.total_commits, "commits_in_prs": g.commits_in_prs, "pr_rate": g.pr_rate, - "single_pass_rate": g.single_pass_rate, - "median_review_rounds": g.median_review_rounds, } for g in acceptance_result.by_origin } + if not reviews_degraded: + for g in acceptance_result.by_origin: + acceptance_kwargs["acceptance_by_origin"][g.group].update({ + "single_pass_rate": g.single_pass_rate, + "median_review_rounds": g.median_review_rounds, + }) if acceptance_result.by_tool: acceptance_kwargs["acceptance_by_tool"] = { g.group: { "total_commits": g.total_commits, "commits_in_prs": g.commits_in_prs, "pr_rate": g.pr_rate, - "single_pass_rate": g.single_pass_rate, - "median_review_rounds": g.median_review_rounds, } for g in acceptance_result.by_tool } + if not reviews_degraded: + for g in acceptance_result.by_tool: + acceptance_kwargs["acceptance_by_tool"][g.group].update({ + "single_pass_rate": g.single_pass_rate, + "median_review_rounds": g.median_review_rounds, + }) # Flow Efficiency — active vs wait decomposition of merged PR lifecycle flow_efficiency_kwargs: dict = {} @@ -319,7 +333,10 @@ def aggregate( flow_efficiency_kwargs["flow_pr_count"] = ( flow_efficiency_result.pr_count ) - if flow_efficiency_result.median_time_to_first_review_hours is not None: + if ( + not reviews_degraded + and flow_efficiency_result.median_time_to_first_review_hours is not None + ): flow_efficiency_kwargs["median_time_to_first_review_hours"] = ( flow_efficiency_result.median_time_to_first_review_hours ) @@ -334,7 +351,7 @@ def aggregate( # Human Review Coverage — fraction of merged PRs a human actually reviewed human_review_coverage_kwargs: dict = {} - if prs: + if prs and not reviews_degraded: coverage_result = analyze_human_review_coverage( prs, commit_origin_map=origin_map, @@ -451,9 +468,10 @@ def aggregate( }, "pr_median_size_files": pr_result.pr_median_size_files, "pr_median_size_lines": pr_result.pr_median_size_lines, - "pr_review_rounds_median": pr_result.pr_review_rounds_median, - "pr_single_pass_rate": pr_result.pr_single_pass_rate, } + if not reviews_degraded: + pr_kwargs["pr_review_rounds_median"] = pr_result.pr_review_rounds_median + pr_kwargs["pr_single_pass_rate"] = pr_result.pr_single_pass_rate # Fix targeting — which origin's code attracts the most bug fixes fix_target_kwargs: dict = {} diff --git a/iris/reports/narrative.py b/iris/reports/narrative.py index 8140c6b..e3ae139 100644 --- a/iris/reports/narrative.py +++ b/iris/reports/narrative.py @@ -477,7 +477,10 @@ def generate_pr_findings(metrics: ReportMetrics, lang: str = "en") -> str: hours=metrics.pr_median_time_to_merge_hours, )) - if metrics.pr_single_pass_rate >= PR_SINGLE_PASS_THRESHOLD: + # Absent when the reviews pass of the PR read failed. + if metrics.pr_single_pass_rate is None: + pass + elif metrics.pr_single_pass_rate >= PR_SINGLE_PASS_THRESHOLD: findings.append(s["finding_pr_single_pass_high"].format( rate=f"{metrics.pr_single_pass_rate:.0%}", )) @@ -508,14 +511,15 @@ def generate_pr_explanations(metrics: ReportMetrics, lang: str = "en") -> str: ), )) - sections.append(_explain( - s["explain_pr_single_pass_title"], - f"{metrics.pr_single_pass_rate:.0%}", - s["explain_pr_single_pass_body"].format( - rate=f"{metrics.pr_single_pass_rate:.0%}", - rounds=metrics.pr_review_rounds_median, - ), - )) + if metrics.pr_single_pass_rate is not None: + sections.append(_explain( + s["explain_pr_single_pass_title"], + f"{metrics.pr_single_pass_rate:.0%}", + s["explain_pr_single_pass_body"].format( + rate=f"{metrics.pr_single_pass_rate:.0%}", + rounds=metrics.pr_review_rounds_median, + ), + )) return "\n\n".join(sections) diff --git a/iris/reports/writer.py b/iris/reports/writer.py index a93b839..27c4a67 100644 --- a/iris/reports/writer.py +++ b/iris/reports/writer.py @@ -136,6 +136,14 @@ def _render_delta_table(deltas, s: dict) -> list[str]: return lines +def _acceptance_cell(entry: dict, key: str, spec: str) -> str: + """One review-derived cell of an acceptance table; a dash when the key is + missing (the reviews pass failed, so the value was omitted).""" + if key not in entry: + return "—" + return format(entry[key], spec) + + def write_report_md( ctx: AnalysisContext, metrics: ReportMetrics, @@ -806,7 +814,7 @@ def write_report_md( continue label = s.get(origin_labels.get(origin, ""), origin) lines.append( - f"| {label} | {entry['total_commits']} | {entry['commits_in_prs']} | {entry['pr_rate']:.0%} | {entry['single_pass_rate']:.0%} | {entry['median_review_rounds']:.1f} |" + f"| {label} | {entry['total_commits']} | {entry['commits_in_prs']} | {entry['pr_rate']:.0%} | {_acceptance_cell(entry, 'single_pass_rate', '.0%')} | {_acceptance_cell(entry, 'median_review_rounds', '.1f')} |" ) lines.append("") @@ -817,7 +825,7 @@ def write_report_md( ]) for tool, entry in sorted(metrics.acceptance_by_tool.items()): lines.append( - f"| {tool} | {entry['total_commits']} | {entry['commits_in_prs']} | {entry['pr_rate']:.0%} | {entry['single_pass_rate']:.0%} | {entry['median_review_rounds']:.1f} |" + f"| {tool} | {entry['total_commits']} | {entry['commits_in_prs']} | {entry['pr_rate']:.0%} | {_acceptance_cell(entry, 'single_pass_rate', '.0%')} | {_acceptance_cell(entry, 'median_review_rounds', '.1f')} |" ) lines.append("") @@ -896,10 +904,16 @@ def write_report_md( pr_rows.extend([ f"| {s['metric_pr_median_size_files']} | {metrics.pr_median_size_files} |", f"| {s['metric_pr_median_size_lines']} | {metrics.pr_median_size_lines} |", - f"| {s['metric_pr_review_rounds_median']} | {metrics.pr_review_rounds_median} |", - f"| {s['metric_pr_single_pass_rate']} | {metrics.pr_single_pass_rate:.0%} |", - f"", ]) + if metrics.pr_review_rounds_median is not None: + pr_rows.append( + f"| {s['metric_pr_review_rounds_median']} | {metrics.pr_review_rounds_median} |" + ) + if metrics.pr_single_pass_rate is not None: + pr_rows.append( + f"| {s['metric_pr_single_pass_rate']} | {metrics.pr_single_pass_rate:.0%} |" + ) + pr_rows.append("") lines.extend(pr_rows) # Merge Strategy section (conditional — only when PR data classified it) diff --git a/tests/test_pr_fetch_degradation.py b/tests/test_pr_fetch_degradation.py index fd7c83e..1e32dc2 100644 --- a/tests/test_pr_fetch_degradation.py +++ b/tests/test_pr_fetch_degradation.py @@ -457,3 +457,132 @@ def test_single_repo_cli_says_the_pr_read_failed_instead_of_skipped( out = capsys.readouterr().out assert "failed (basic)" in out assert "skipped (no GitHub remote or gh CLI)" not in out + + +# --- a failed reviews pass omits the review-derived fields ------------------ +# +# With the reviews pass failed every PR carries no reviews, so review coverage +# could only read 0% and the single-pass rate 100%. Those would be numbers the +# run fabricated, so they are left out instead. + +from dataclasses import replace + +from iris.models.context import AnalysisContext +from iris.models.metrics import ReportMetrics +from iris.models.pull_request import PRReview +from iris.reports.writer import write_report_md + +_REVIEW_KEYS = ( + "pr_review_rounds_median", + "pr_single_pass_rate", + "median_time_to_first_review_hours", + "human_review_coverage_pct", + "human_approval_coverage_pct", + "human_review_coverage_by_intent", + "human_review_coverage_by_origin_of_pr", +) + + +def _reviewed_prs() -> list[PullRequest]: + reviews = [ + PRReview(author="reviewer", state="CHANGES_REQUESTED", + submitted_at=_NOW + timedelta(minutes=30)), + PRReview(author="reviewer", state="APPROVED", + submitted_at=_NOW + timedelta(minutes=45)), + ] + return [replace(pr, reviews=reviews) for pr in _merged_prs()] + + +def test_degraded_reviews_omit_review_metrics(): + payload = aggregate( + _stamped_commits(), churn_days=14, prs=_reviewed_prs(), + pr_fetch_degraded=("reviews",), + ).to_dict() + + assert payload["pr_merged_count"] == 6 + for key in _REVIEW_KEYS: + assert key not in payload, key + for group in payload.get("acceptance_by_origin", {}).values(): + assert "single_pass_rate" not in group + assert "median_review_rounds" not in group + for group in payload.get("acceptance_by_tool", {}).values(): + assert "single_pass_rate" not in group + assert "median_review_rounds" not in group + + +def test_clean_run_keeps_review_metrics(): + payload = aggregate( + _stamped_commits(), churn_days=14, prs=_reviewed_prs(), + ).to_dict() + + assert payload["pr_single_pass_rate"] == 0.0 + assert payload["pr_review_rounds_median"] == 1.0 + for group in payload.get("acceptance_by_origin", {}).values(): + assert "single_pass_rate" in group + assert "median_review_rounds" in group + + +def test_report_renders_without_review_metrics(tmp_path): + ctx = AnalysisContext( + repo_path=str(tmp_path), repo_name="widgets", days=90, churn_days=14, + out_dir=str(tmp_path), lang="en", + ) + metrics = ReportMetrics( + commits_total=100, commits_revert=10, revert_rate=0.1, churn_events=5, + churn_lines_affected=500, files_touched=50, files_stabilized=45, + stabilization_ratio=0.90, + pr_merged_count=6, pr_median_time_to_merge_hours=3.5, + pr_median_size_files=1, pr_median_size_lines=1, + pr_review_rounds_median=None, pr_single_pass_rate=None, + acceptance_by_origin={ + "HUMAN": {"total_commits": 10, "commits_in_prs": 8, "pr_rate": 0.8}, + }, + acceptance_by_tool={ + "tool-a": {"total_commits": 4, "commits_in_prs": 3, "pr_rate": 0.75}, + }, + ) + + path = write_report_md(ctx, metrics, out_dir=str(tmp_path)) + text = Path(path).read_text(encoding="utf-8") + + assert "| 80% | — | — |" in text + assert "| 75% | — | — |" in text + assert "Single-pass rate" not in text + + +def test_narrative_skips_single_pass_without_review_metrics(): + from iris.reports.narrative import generate_pr_explanations, generate_pr_findings + + metrics = ReportMetrics( + commits_total=100, commits_revert=10, revert_rate=0.1, churn_events=5, + churn_lines_affected=500, files_touched=50, files_stabilized=45, + stabilization_ratio=0.90, + pr_merged_count=6, pr_median_time_to_merge_hours=3.5, + ) + + findings = generate_pr_findings(metrics) + explanations = generate_pr_explanations(metrics) + + assert "6" in findings + assert "single-pass" not in findings.lower() + assert "single-pass" not in explanations.lower() + + +from iris.analysis.trend_delta import compute_trend_delta + + +def test_trend_skips_single_pass_when_missing(): + def metrics(single_pass): + return ReportMetrics( + commits_total=100, commits_revert=10, revert_rate=0.1, + churn_events=5, churn_lines_affected=500, files_touched=50, + files_stabilized=45, stabilization_ratio=0.90, + pr_merged_count=6, pr_median_time_to_merge_hours=3.5, + pr_single_pass_rate=single_pass, + ) + + trend = compute_trend_delta(metrics(0.8), metrics(None), 90, 30) + + names = [d.metric for d in trend.deltas] + assert "pr_time_to_merge" in names + assert "pr_single_pass" not in names From 52ded1fee7f90e560a0fb051863ab5105b548cc0 Mon Sep 17 00:00:00 2001 From: Carlos Penaforte Date: Mon, 5 Oct 2026 15:54:34 -0300 Subject: [PATCH 2/2] fix(engine): also omit flow and staleness when PR reviews failed Flow Efficiency anchors its phases on the first review, and open PR staleness is measured from the last review or commit. With the reviews read failed every PR carries no reviews, so flow put the whole open-to-merge window into "awaiting first review" (the narrative then stated "wait time dominates" as fact) and staleness read older than it is. Absent beats fabricated, as for the other review-derived fields. Omitted when `reviews` degraded: the whole Flow Efficiency block (flow_efficiency_median, time_in_phase_median_hours, flow_pr_count, the by-intent and by-origin splits, median_time_to_first_review_hours) and stale_open_pr_pct, very_stale_open_pr_pct, abandonment_risk_pct and stale_open_pr_pct_by_origin. Kept: open_pr_count and the open PR ages, which depend only on the creation time. The narrative readers already tolerate absence. Clean runs are unchanged. The shared test fixture now carries commit refs, so the clean twin really produces the review, flow and staleness fields it asserts. Refs #236 Assisted-by: Claude Code Co-Authored-By: Claude Sonnet 5.5 --- docs/METRICS.md | 22 ++++++---- iris/metrics/aggregator.py | 34 ++++++++------- tests/test_pr_fetch_degradation.py | 67 +++++++++++++++++++++++++++--- 3 files changed, 94 insertions(+), 29 deletions(-) diff --git a/docs/METRICS.md b/docs/METRICS.md index 708da0e..cf8cda9 100644 --- a/docs/METRICS.md +++ b/docs/METRICS.md @@ -394,7 +394,13 @@ coverage and 100% single-pass an empty review list would give: `median_time_to_first_review_hours`, `human_review_coverage_pct`, `human_approval_coverage_pct`, `human_review_coverage_by_intent`, `human_review_coverage_by_origin_of_pr`, and the `single_pass_rate` and -`median_review_rounds` keys of `acceptance_by_origin` / `acceptance_by_tool`. It is the first field to read when two runs on the +`median_review_rounds` keys of `acceptance_by_origin` / `acceptance_by_tool`. +The review-anchored flow and staleness fields go too: the whole Flow +Efficiency block (§25) would put every PR's open-to-merge window into +"awaiting first review", and staleness is measured from the last review or +commit. Open PR count and ages (`open_pr_count`, `median_open_pr_age_days`, +`p90_open_pr_age_days`, `median_open_pr_age_by_intent`) are kept, since age +depends only on the creation time. It is the first field to read when two runs on the same commit disagree. --- @@ -633,11 +639,11 @@ say nothing about whether work is flowing or queued in a different shape. | Field | Unit | Source | Nullable when | |---|---|---|---| -| `flow_efficiency_median` | float `0.0–1.0` | `analysis/flow_efficiency.py` | no merged PR survives the filters | +| `flow_efficiency_median` | float `0.0–1.0` | `analysis/flow_efficiency.py` | no merged PR survives the filters, or `reviews` degraded | | `median_time_to_first_review_hours` | float ≥ 0 | same | no merged PR had a review, or `reviews` degraded | | `time_in_phase_median_hours` | `Record` | same | same as `flow_efficiency_median` | -| `flow_efficiency_by_intent` | `Record` | same | < `min_sample` (default 10) PRs in the segment | -| `flow_efficiency_by_origin` | `Record` | same | no `commit_origin_map` provided, or < `min_sample` PRs in the segment | +| `flow_efficiency_by_intent` | `Record` | same | < `min_sample` (default 10) PRs in the segment, or `reviews` degraded | +| `flow_efficiency_by_origin` | `Record` | same | no `commit_origin_map` provided, or < `min_sample` PRs in the segment, or `reviews` degraded | Phase model (5 timestamps → 4 phases per merged PR): @@ -810,11 +816,11 @@ Aging = "do trabalho que ainda não saiu, está represado?". | `open_pr_count` | int | `analysis/open_pr_aging.py` | no eligible open PR (all drafts/bots) | | `median_open_pr_age_days` | float ≥ 0 | same | same | | `p90_open_pr_age_days` | float ≥ 0 | same | same | -| `stale_open_pr_pct` | float `0.0–1.0` | same | same | -| `very_stale_open_pr_pct` | float `0.0–1.0` | same | same | -| `abandonment_risk_pct` | float `0.0–1.0` | same | same | +| `stale_open_pr_pct` | float `0.0–1.0` | same | same, or `reviews` degraded | +| `very_stale_open_pr_pct` | float `0.0–1.0` | same | same, or `reviews` degraded | +| `abandonment_risk_pct` | float `0.0–1.0` | same | same, or `reviews` degraded | | `median_open_pr_age_by_intent` | `Record` | same | < `min_sample` (default 5) PRs in segment | -| `stale_open_pr_pct_by_origin` | `Record` | same | no `commit_origin_map`, or < `min_sample` PRs in segment | +| `stale_open_pr_pct_by_origin` | `Record` | same | no `commit_origin_map`, or < `min_sample` PRs in segment, or `reviews` degraded | Per-PR signals (intermediate, **never persisted**): diff --git a/iris/metrics/aggregator.py b/iris/metrics/aggregator.py index 9c3e85a..6b615c5 100644 --- a/iris/metrics/aggregator.py +++ b/iris/metrics/aggregator.py @@ -316,9 +316,11 @@ def aggregate( "median_review_rounds": g.median_review_rounds, }) - # Flow Efficiency — active vs wait decomposition of merged PR lifecycle + # Flow Efficiency — active vs wait decomposition of merged PR lifecycle. + # Its phases anchor on the first review: with the reviews read failed, + # every PR would count as never reviewed, so the block is left out. flow_efficiency_kwargs: dict = {} - if prs: + if prs and not reviews_degraded: flow_efficiency_result = analyze_flow_efficiency( prs, commit_origin_map=origin_map, @@ -333,10 +335,7 @@ def aggregate( flow_efficiency_kwargs["flow_pr_count"] = ( flow_efficiency_result.pr_count ) - if ( - not reviews_degraded - and flow_efficiency_result.median_time_to_first_review_hours is not None - ): + if flow_efficiency_result.median_time_to_first_review_hours is not None: flow_efficiency_kwargs["median_time_to_first_review_hours"] = ( flow_efficiency_result.median_time_to_first_review_hours ) @@ -388,20 +387,23 @@ def aggregate( open_pr_aging_kwargs["p90_open_pr_age_days"] = ( aging_result.p90_open_pr_age_days ) - open_pr_aging_kwargs["stale_open_pr_pct"] = ( - aging_result.stale_open_pr_pct - ) - open_pr_aging_kwargs["very_stale_open_pr_pct"] = ( - aging_result.very_stale_open_pr_pct - ) - open_pr_aging_kwargs["abandonment_risk_pct"] = ( - aging_result.abandonment_risk_pct - ) if aging_result.median_open_pr_age_by_intent: open_pr_aging_kwargs["median_open_pr_age_by_intent"] = ( aging_result.median_open_pr_age_by_intent ) - if aging_result.stale_open_pr_pct_by_origin: + # Staleness is measured from the last review or commit; without + # the reviews it would read older than it is. + if not reviews_degraded: + open_pr_aging_kwargs["stale_open_pr_pct"] = ( + aging_result.stale_open_pr_pct + ) + open_pr_aging_kwargs["very_stale_open_pr_pct"] = ( + aging_result.very_stale_open_pr_pct + ) + open_pr_aging_kwargs["abandonment_risk_pct"] = ( + aging_result.abandonment_risk_pct + ) + if not reviews_degraded and aging_result.stale_open_pr_pct_by_origin: open_pr_aging_kwargs["stale_open_pr_pct_by_origin"] = ( aging_result.stale_open_pr_pct_by_origin ) diff --git a/tests/test_pr_fetch_degradation.py b/tests/test_pr_fetch_degradation.py index 1e32dc2..23cccca 100644 --- a/tests/test_pr_fetch_degradation.py +++ b/tests/test_pr_fetch_degradation.py @@ -469,7 +469,7 @@ def test_single_repo_cli_says_the_pr_read_failed_instead_of_skipped( from iris.models.context import AnalysisContext from iris.models.metrics import ReportMetrics -from iris.models.pull_request import PRReview +from iris.models.pull_request import CommitRef, PRReview from iris.reports.writer import write_report_md _REVIEW_KEYS = ( @@ -482,6 +482,28 @@ def test_single_repo_cli_says_the_pr_read_failed_instead_of_skipped( "human_review_coverage_by_origin_of_pr", ) +# Flow phases anchor on the first review, and staleness on the last activity: +# with no reviews read, both would be computed from a lie. +_FLOW_KEYS = ( + "flow_efficiency_median", + "time_in_phase_median_hours", + "flow_pr_count", + "flow_efficiency_by_intent", + "flow_efficiency_by_origin", +) +_STALENESS_KEYS = ( + "stale_open_pr_pct", + "very_stale_open_pr_pct", + "abandonment_risk_pct", + "stale_open_pr_pct_by_origin", +) +# Open PR age depends only on the creation time, so it survives. +_AGE_KEYS = ( + "open_pr_count", + "median_open_pr_age_days", + "p90_open_pr_age_days", +) + def _reviewed_prs() -> list[PullRequest]: reviews = [ @@ -490,18 +512,41 @@ def _reviewed_prs() -> list[PullRequest]: PRReview(author="reviewer", state="APPROVED", submitted_at=_NOW + timedelta(minutes=45)), ] - return [replace(pr, reviews=reviews) for pr in _merged_prs()] + return [ + replace( + pr, reviews=reviews, + commit_refs=[CommitRef(hash=f"c{pr.number}", + committed_at=_NOW - timedelta(hours=1))], + ) + for pr in _merged_prs() + ] + + +def _open_prs() -> list[PullRequest]: + """Five open PRs, old enough to be stale, with a commit each.""" + created = _NOW - timedelta(days=40) + return [ + PullRequest(number=100 + n, title=f"open {n}", author="dev", + created_at=created, additions=1, deletions=0, + changed_files=1, state="open", + commit_refs=[CommitRef(hash=f"c{n}", committed_at=created)]) + for n in range(1, 6) + ] def test_degraded_reviews_omit_review_metrics(): payload = aggregate( - _stamped_commits(), churn_days=14, prs=_reviewed_prs(), + _stamped_commits(), churn_days=14, prs=_reviewed_prs() + _open_prs(), pr_fetch_degraded=("reviews",), ).to_dict() assert payload["pr_merged_count"] == 6 - for key in _REVIEW_KEYS: + assert payload["open_pr_count"] == 5 + for key in _REVIEW_KEYS + _FLOW_KEYS + _STALENESS_KEYS: assert key not in payload, key + for key in _AGE_KEYS: + assert key in payload, key + assert "median_open_pr_age_by_intent" in payload for group in payload.get("acceptance_by_origin", {}).values(): assert "single_pass_rate" not in group assert "median_review_rounds" not in group @@ -512,11 +557,23 @@ def test_degraded_reviews_omit_review_metrics(): def test_clean_run_keeps_review_metrics(): payload = aggregate( - _stamped_commits(), churn_days=14, prs=_reviewed_prs(), + _stamped_commits(), churn_days=14, prs=_reviewed_prs() + _open_prs(), ).to_dict() assert payload["pr_single_pass_rate"] == 0.0 assert payload["pr_review_rounds_median"] == 1.0 + # The by-intent / by-origin splits need 10 merged PRs and acceptance by + # tool needs an AI-attributed commit, which this fixture does not have; + # their omission is covered by the degraded twin only trivially. + segmented = { + "human_review_coverage_by_intent", + "human_review_coverage_by_origin_of_pr", + "flow_efficiency_by_intent", + "flow_efficiency_by_origin", + } + for key in _REVIEW_KEYS + _FLOW_KEYS + _STALENESS_KEYS + _AGE_KEYS: + if key not in segmented: + assert key in payload, key for group in payload.get("acceptance_by_origin", {}).values(): assert "single_pass_rate" in group assert "median_review_rounds" in group