diff --git a/docs/METRICS.md b/docs/METRICS.md index 413f0af..cf8cda9 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,21 @@ 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`. +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. --- @@ -626,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 | -| `median_time_to_first_review_hours` | float ≥ 0 | same | no merged PR had a review | +| `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): @@ -728,10 +741,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): @@ -803,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/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..6b615c5 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,26 +291,36 @@ 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 } - - # Flow Efficiency — active vs wait decomposition of merged PR lifecycle + 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. + # 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, @@ -334,7 +350,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, @@ -371,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 ) @@ -451,9 +470,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..23cccca 100644 --- a/tests/test_pr_fetch_degradation.py +++ b/tests/test_pr_fetch_degradation.py @@ -457,3 +457,189 @@ 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 CommitRef, 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", +) + +# 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 = [ + 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, + 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() + _open_prs(), + pr_fetch_degraded=("reviews",), + ).to_dict() + + assert payload["pr_merged_count"] == 6 + 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 + 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() + _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 + + +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