Skip to content

fix(engine): omit review-derived metrics when the reviews read fails #284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
clickmatos merged 2 commits into from
Oct 8, 2026
Merged

fix(engine): omit review-derived metrics when the reviews read fails #284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
556d347
fix(engine): omit review metrics when the PR reviews read failed
cpenaforte-clickbus Oct 5, 2026
52ded1f
fix(engine): also omit flow and staleness when PR reviews failed
cpenaforte-clickbus Oct 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 30 additions & 17 deletions docs/METRICS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).

---

Expand Down Expand Up @@ -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
Expand All @@ -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.

---
Expand Down Expand Up @@ -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<phase, hours>` | same | same as `flow_efficiency_median` |
| `flow_efficiency_by_intent` | `Record<intent, ratio>` | same | < `min_sample` (default 10) PRs in the segment |
| `flow_efficiency_by_origin` | `Record<origin, ratio>` | same | no `commit_origin_map` provided, or < `min_sample` PRs in the segment |
| `flow_efficiency_by_intent` | `Record<intent, ratio>` | same | < `min_sample` (default 10) PRs in the segment, or `reviews` degraded |
| `flow_efficiency_by_origin` | `Record<origin, ratio>` | 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):

Expand Down Expand Up @@ -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<intent, ratio>` | same | < `min_sample` (default 10) PRs in the segment |
| `human_review_coverage_by_origin_of_pr` | `Record<origin, ratio>` | same | no `commit_origin_map` provided, or < `min_sample` PRs in the segment |
| `human_review_coverage_by_intent` | `Record<intent, ratio>` | same | < `min_sample` (default 10) PRs in the segment, or `reviews` degraded |
| `human_review_coverage_by_origin_of_pr` | `Record<origin, ratio>` | same | no `commit_origin_map` provided, or < `min_sample` PRs in the segment, or `reviews` degraded |

Definition (per merged PR, intermediate only):

Expand Down Expand Up @@ -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<intent, days>` | same | < `min_sample` (default 5) PRs in segment |
| `stale_open_pr_pct_by_origin` | `Record<origin, ratio>` | same | no `commit_origin_map`, or < `min_sample` PRs in segment |
| `stale_open_pr_pct_by_origin` | `Record<origin, ratio>` | same | no `commit_origin_map`, or < `min_sample` PRs in segment, or `reviews` degraded |

Per-PR signals (intermediate, **never persisted**):

Expand Down
19 changes: 12 additions & 7 deletions iris/analysis/trend_delta.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
64 changes: 42 additions & 22 deletions iris/metrics/aggregator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand All @@ -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)

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
)
Expand Down Expand Up @@ -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 = {}
Expand Down
22 changes: 13 additions & 9 deletions iris/reports/narrative.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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%}",
))
Expand Down Expand Up @@ -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)

Expand Down
24 changes: 19 additions & 5 deletions iris/reports/writer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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("")

Expand All @@ -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("")

Expand Down Expand Up @@ -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)
Expand Down
Loading
Loading