diff --git a/docs/METRICS.md b/docs/METRICS.md index 99b6412..6131bd8 100644 --- a/docs/METRICS.md +++ b/docs/METRICS.md @@ -236,15 +236,17 @@ its PR (if any) and aggregated by origin and AI tool. | Field | Unit | Source | Nullable when | |---|---|---|---| -| `acceptance_by_origin` | `Record` | `analysis/acceptance_rate.py` | no PR data, or < 5 commits per origin, or `enrichment` degraded | -| `acceptance_by_tool` | `Record` | same | < 5 commits per tool, or `enrichment` degraded | +| `acceptance_by_origin` | `Record` | `analysis/acceptance_rate.py` | no PR data, or < 5 commits per origin, or `basic` or `enrichment` failed for merged PRs | +| `acceptance_by_tool` | `Record` | same | < 5 commits per tool, or `basic` or `enrichment` failed for merged PRs | `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`. The last two are omitted when `reviews` is in -`pr_enrichment_degraded` (§15). Both maps are omitted entirely when `enrichment` -is in `pr_enrichment_degraded`: commits are matched to PRs through the commit +zero `CHANGES_REQUESTED` / PRs), `median_review_rounds`. The last two are omitted when `reviews` failed +for merged PRs (§15). Both maps are omitted entirely when `enrichment` failed +for merged PRs: commits are matched to PRs through the commit refs that pass reads, so without them every group would read 0 commits in PRs. +The same goes when the merged list itself failed (`basic`): with no merged PRs +to match, every group would read 0 too. --- @@ -256,23 +258,28 @@ dataclass is always `None`; the field only exists in the emitted JSON. | Field | Unit | Source | Nullable when | |---|---|---|---| -| `origin_funnel` | `Record` | `analysis/origin_funnel.py` (via `reports/writer.py`) | no `commit_origin_distribution`, or `pr_enrichment_degraded` present and `acceptance_by_origin` absent | +| `origin_funnel` | `Record` | `analysis/origin_funnel.py` (via `reports/writer.py`) | no `commit_origin_distribution`, or `pr_enrichment_degraded` present and `acceptance_by_origin` absent (`basic` or `enrichment` failed for merged PRs, or `fetch`) | The whole funnel is omitted when `pr_enrichment_degraded` (§15) is present and `acceptance_by_origin` is absent: its `In PR` stage reads acceptance, and every -later conversion chains from that stage. That covers a failed `enrichment`, -which omits acceptance (§9), and a read that lost the PRs — `basic` failed for -every state, or `fetch` — which leaves no PRs to match commits to. Only when -PRs are absent by design (no `gh`, no GitHub remote; nothing degraded) does -`In PR` fall back to counting every commit as in a PR. +later conversion chains from that stage. That covers a failed `basic` or +`enrichment` for merged PRs, which omits acceptance (§9), and `fetch`, which +leaves no PRs to match commits to. A failure confined to closed or open PRs +does not remove acceptance or the funnel: they stay whenever a clean run +would have them — acceptance still needs an origin with at least 5 commits +(§9), and the funnel a `commit_origin_distribution`. +Only when PRs are absent by design (no `gh`, no GitHub remote; nothing +degraded) does `In PR` fall back to counting every commit as in a PR. Stages: `Committed` → `In PR` → `Stabilized` → `Lines Surviving` (the last stage only when durability data is available). Each stage carries `count` and `conversion_from_previous`. `overall_conversion` is the product of conversions. -**Limitation:** `PR → Merge` is not measured — Iris only fetches merged -PRs. +**Limitation:** `PR → Merge` is not measured. Iris reads merged, closed and +open PRs, but `In PR` comes from acceptance (§9), which counts only commits in +merged PRs: a commit in a PR still open or closed without merging reads as not +in a PR, and no stage follows the PRs that never merge. --- @@ -344,7 +351,9 @@ weeks. `ActivityWeek`: `week_start`/`week_end` (ISO date), `commits`, `lines_changed`, `intent` (distribution), `origin` (distribution), `stabilization_ratio` (or `None` for weeks < 3 commits), `churn_events`, -`prs_merged`, `pr_median_ttm_hours`. +`prs_merged`, `pr_median_ttm_hours` (both `None` with no PR data, or when the +merged list failed — `basic` for merged PRs, or `fetch` — rather than 0 PRs +merged; §15). `ActivityPattern`: `pattern` (one of `burst_then_fix`, `quiet_period`, `ai_ramp`, `intent_shift`), `week` (`MM/DD`), `description`. @@ -363,7 +372,7 @@ All fields require GitHub PR data. | Field | Unit | Source | Nullable when | |---|---|---|---| -| `pr_merged_count` | int ≥ 0 | `analysis/pr_lifecycle.py` | no PR data | +| `pr_merged_count` | int ≥ 0 | `analysis/pr_lifecycle.py` | no PR data, or `basic` failed for merged PRs | | `pr_median_time_to_merge_hours` | float ≥ 0 | same | same | | `pr_mean_time_to_merge_hours` | float ≥ 0 | same | same | | `pr_p90_time_to_merge_hours` | float ≥ 0 | same | same | @@ -371,8 +380,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, or `reviews` degraded | -| `pr_single_pass_rate` | float `0.0–1.0` | same | same, or `reviews` degraded | +| `pr_review_rounds_median` | float ≥ 0 | same | same, or `reviews` failed for merged PRs | +| `pr_single_pass_rate` | float `0.0–1.0` | same | same, or `reviews` failed for merged PRs | | `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 @@ -390,16 +399,23 @@ without persisting individual PR durations. `pr_enrichment_degraded` — the steps of the GitHub PR read that failed this run and fell back to partial data: `basic` (`gh pr list` for one state failed, so that state's PRs are missing), `enrichment` (the GraphQL pass for commit -refs and merge-commit parents failed, after one retry), `reviews` (the reviews -pass failed), `fetch` (the whole read raised). Sorted, without repeats. Unlike -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 `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: +refs and merge-commit parents failed; a pass that stops on any page counts as +failed, even when earlier pages came back), `reviews` (the reviews pass +failed), `fetch` (the whole read raised). The enrichment and reviews passes +get one retry when gh reports a transient error (HTTP 5xx, a timeout — +GitHub's GraphQL "may be the result of a timeout" error included — or a +dropped connection); a 4xx or other GraphQL error is not retried. Sorted, +without repeats. Unlike 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. With `enrichment` failed for merged PRs, Flow Efficiency (§25) is +computed only over the merged PRs whose commit refs came back, so it is +skewed rather than missing (unless none came back); `commits_in_prs` goes +missing with acceptance (§9); and `merge_strategy` is `unknown` (§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`, @@ -414,8 +430,30 @@ fields are omitted then too, and `acceptance_by_origin` and `acceptance_by_tool` (§9) are omitted whole, since they match commits to PRs through those same refs. 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. +depends only on the creation time — unless the open list itself failed (see +below). It is the first field to read when two runs on the same commit +disagree. + +The read fetches merged, closed and open PRs separately, and a step can fail +for one state only, so each omission above follows the state that feeds the +field. The review fields, Flow Efficiency, acceptance (§9) and merge strategy +(§28) read merged PRs: only a failure for merged PRs omits them. Staleness +reads open PRs: only a failure for open PRs omits it. Closed PRs feed only +Flow Load (§24), and only through their list, so a failed enrichment or +reviews pass there omits nothing. `pr_enrichment_degraded` still lists the +union over the three states. + +When a state's list failed (`basic`), its PRs are missing while the other +states' are read, so whatever it feeds would read as a real zero; those +fields are omitted instead, as on a run with no PR data. A failed merged list +omits the PR lifecycle fields above, the review fields, Flow Efficiency (§25), +Human Review Coverage (§26), acceptance (§9) and merge strategy (§28), and +leaves the activity timeline's `prs_merged` and `pr_median_ttm_hours` `None` +(§14). A failed open list omits every Open PR Aging field (§27), count and +ages included. Flow Load (§24) counts PRs of every state in flight, so a +failed list in any state omits it. `fetch` counts as every list failed. When +the per-state breakdown is unavailable, a step in the union applies to every +state. --- @@ -587,7 +625,7 @@ proxy. | Field | Unit | Source | Nullable when | |---|---|---|---| -| `flow_load` | `FlowLoadWeek[]` | `analysis/flow_load.py` | < 2 weeks of signal | +| `flow_load` | `FlowLoadWeek[]` | `analysis/flow_load.py` | < 2 weeks of signal, or `basic` failed for any PR state, or `fetch` (§15) | `FlowLoadWeek`: @@ -602,6 +640,15 @@ proxy. falling back to author name) with at least one non-merge commit in the bucket +**Failed PR list.** When `gh pr list` failed for any PR state (`basic` for +merged, closed or open PRs, or `fetch`; §15), the whole `flow_load` series +is omitted, `author_concurrency` included. Every state is in flight at some +point, so a missing list would undercount `wip_total` and `wip_by_intent` in +every bucket it overlaps. `author_concurrency` comes from commits and would +still be right, but keeping it alone would mean emitting buckets with the +WIP fields missing, a shape the platform does not read; the series goes +whole instead. + Overlap rule for a bucket `[t_start, t_end)`: ``` @@ -653,11 +700,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, or `reviews` degraded | -| `median_time_to_first_review_hours` | float ≥ 0 | same | no merged PR had a review, or `reviews` degraded | +| `flow_efficiency_median` | float `0.0–1.0` | `analysis/flow_efficiency.py` | no merged PR survives the filters, or `basic` or `reviews` failed for merged PRs | +| `median_time_to_first_review_hours` | float ≥ 0 | same | no merged PR had a review, or `basic` or `reviews` failed for merged PRs | | `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, or `reviews` degraded | -| `flow_efficiency_by_origin` | `Record` | same | no `commit_origin_map` provided, or < `min_sample` PRs in the segment, or `reviews` degraded | +| `flow_efficiency_by_intent` | `Record` | same | < `min_sample` (default 10) PRs in the segment, or `basic` or `reviews` failed for merged PRs | +| `flow_efficiency_by_origin` | `Record` | same | no `commit_origin_map` provided, or < `min_sample` PRs in the segment, or `basic` or `reviews` failed for merged PRs | Phase model (5 timestamps → 4 phases per merged PR): @@ -755,10 +802,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, or `reviews` degraded | +| `human_review_coverage_pct` | float `0.0–1.0` | `analysis/human_review_coverage.py` | no merged PR in the window, or `basic` or `reviews` failed for merged PRs | | `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, 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 | +| `human_review_coverage_by_intent` | `Record` | same | < `min_sample` (default 10) PRs in the segment, or `basic` or `reviews` failed for merged PRs | +| `human_review_coverage_by_origin_of_pr` | `Record` | same | no `commit_origin_map` provided, or < `min_sample` PRs in the segment, or `basic` or `reviews` failed for merged PRs | Definition (per merged PR, intermediate only): @@ -827,14 +874,14 @@ Aging = "do trabalho que ainda não saiu, está represado?". | Field | Unit | Source | Nullable when | |---|---|---|---| -| `open_pr_count` | int | `analysis/open_pr_aging.py` | no eligible open PR (all drafts/bots) | +| `open_pr_count` | int | `analysis/open_pr_aging.py` | no eligible open PR (all drafts/bots), or `basic` failed for open PRs | | `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, or `reviews` or `enrichment` degraded | -| `very_stale_open_pr_pct` | float `0.0–1.0` | same | same, or `reviews` or `enrichment` degraded | -| `abandonment_risk_pct` | float `0.0–1.0` | same | same, or `reviews` or `enrichment` 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, or `reviews` or `enrichment` degraded | +| `stale_open_pr_pct` | float `0.0–1.0` | same | same, or `reviews` or `enrichment` failed for open PRs | +| `very_stale_open_pr_pct` | float `0.0–1.0` | same | same, or `reviews` or `enrichment` failed for open PRs | +| `abandonment_risk_pct` | float `0.0–1.0` | same | same, or `reviews` or `enrichment` failed for open PRs | +| `median_open_pr_age_by_intent` | `Record` | same | < `min_sample` (default 5) PRs in segment, or `basic` failed for open PRs | +| `stale_open_pr_pct_by_origin` | `Record` | same | no `commit_origin_map`, or < `min_sample` PRs in segment, or `basic`, `reviews` or `enrichment` failed for open PRs | Per-PR signals (intermediate, **never persisted**): @@ -905,9 +952,9 @@ under-report AI adoption in squash repos. | Field | Unit | Source | Nullable when | |---|---|---|---| -| `merge_strategy` | `merge\|squash\|rebase\|mixed\|unknown` | `analysis/merge_strategy_detector.py` | no PR data (`prs` empty/None) | -| `merge_strategy_dominant_share` | float `0.0–1.0` | same | strategy is `unknown` | -| `commit_metrics_reliable` | bool | same | no PR data | +| `merge_strategy` | `merge\|squash\|rebase\|mixed\|unknown` | `analysis/merge_strategy_detector.py` | no PR data (`prs` empty/None), or `basic` failed for merged PRs | +| `merge_strategy_dominant_share` | float `0.0–1.0` | same | strategy is `unknown` or absent | +| `commit_metrics_reliable` | bool | same | no PR data, or `basic` failed for merged PRs | Per-PR classification (over **merged** PRs only), in confidence order: @@ -950,12 +997,14 @@ Platform: indexed columns `merge_strategy` + `commit_metrics_reliable` on carries `merge_strategy_dominant_share` for the repo-detail badge. **Degraded enrichment.** When the PR read's `enrichment` step failed -(`pr_enrichment_degraded` contains `enrichment`, from any PR state, §15), the repo is `unknown` +for merged PRs (`pr_enrichment_degraded` then contains `enrichment`, §15), the repo is `unknown` without classifying: `merge_strategy_dominant_share` is absent and `commit_metrics_reliable` is `true`. Without parent counts and commit refs, a merge PR falls through to `unknown` while a squash-stamped one still classifies, so the fallback could only ever answer `squash` — a mixed repo would read as 100% squash, with its per-commit metrics flagged unreliable. +When the merged list itself failed (`basic` for merged PRs, §15), there is +nothing to classify and the three fields are absent, as with no PR data. --- diff --git a/iris/analysis/origin_funnel.py b/iris/analysis/origin_funnel.py index b3dd246..c02f05c 100644 --- a/iris/analysis/origin_funnel.py +++ b/iris/analysis/origin_funnel.py @@ -8,8 +8,10 @@ Each stage has a conversion rate. Comparing funnels across origins reveals where AI-assisted code drops off relative to human code. -Limitation: PR→Merge stage is not measured (Iris only fetches merged PRs). -Future work could add closed/rejected PR data to fill this gap. +Limitation: PR→Merge stage is not measured. Iris reads merged, closed and open +PRs, but the "In PR" stage reads acceptance, which counts only commits in +merged PRs, and no stage follows PRs that never merge. The closed PRs already +read could fill this gap. """ # AGGREGATOR_OPT_OUT: consumes a finalized ReportMetrics (post-aggregation composition). @@ -60,10 +62,13 @@ def calculate_origin_funnel(metrics: ReportMetrics) -> FunnelResult | None: if the PR read was degraded and acceptance is absent: the "In PR" stage reads acceptance, and every later conversion chains from it. """ - # A failed enrichment omits acceptance, and a failed list or read leaves - # no PRs to match commits to; either way "In PR" would fall back to - # "every commit is in a PR". That default is kept only for PRs absent by - # design (no gh, no GitHub remote), where nothing is marked degraded. + # `pr_enrichment_degraded` is the union over PR states, so it alone cannot + # say whether the PRs acceptance reads were lost; acceptance itself can. + # The aggregator omits it when the merged PRs lost their enrichment, and a + # failed list or read leaves no PRs to match commits to: either way "In + # PR" would fall back to "every commit is in a PR". That default is kept + # only for PRs absent by design (no gh, no GitHub remote), where nothing + # is marked degraded. if metrics.pr_enrichment_degraded and not metrics.acceptance_by_origin: return None diff --git a/iris/cli.py b/iris/cli.py index 9014aad..601c73a 100644 --- a/iris/cli.py +++ b/iris/cli.py @@ -529,6 +529,7 @@ def _tick(label: str) -> None: prs=prs or None, external_data=external_data, pr_fetch_degraded=pr_fetch.degraded, + pr_fetch_degraded_by_state=pr_fetch.degraded_by_state, ) print(s["cli_classified"].format(count=len(commits))) _tick("aggregate analysis done") @@ -559,6 +560,7 @@ def _tick(label: str) -> None: recent_metrics = aggregate( recent_commits, churn_days=args.churn_days, prs=recent_prs, pr_fetch_degraded=pr_fetch.degraded, + pr_fetch_degraded_by_state=pr_fetch.degraded_by_state, ) trend = compute_trend_delta( baseline=metrics, @@ -692,10 +694,12 @@ def _tick(label: str) -> None: pre_metrics = aggregate( pre_commits, churn_days=args.churn_days, prs=pre_prs, pr_fetch_degraded=pr_fetch.degraded, + pr_fetch_degraded_by_state=pr_fetch.degraded_by_state, ) post_metrics = aggregate( post_commits, churn_days=args.churn_days, prs=post_prs, pr_fetch_degraded=pr_fetch.degraded, + pr_fetch_degraded_by_state=pr_fetch.degraded_by_state, ) # Compute day spans from commit date ranges diff --git a/iris/i18n.py b/iris/i18n.py index 511c01a..4fba747 100644 --- a/iris/i18n.py +++ b/iris/i18n.py @@ -35,10 +35,11 @@ ), "pr_enrichment_degraded_caveat": ( "PR data is incomplete for this run — failed steps: {steps}. Merge " - "strategy is reported as unknown when enrichment failed, and flow, " - "review and in-PR metrics may be missing or skewed. A transient " - "GitHub error usually clears on the next run; if every run fails, " - "check that gh is authenticated (gh auth status)." + "strategy is reported as unknown when enrichment failed for merged " + "PRs, and PR lifecycle (counts, time to merge, cycle time), flow, " + "review, in-PR and open-PR metrics may be missing or skewed. A " + "transient GitHub error usually clears on the next run; if every run " + "fails, check that gh is authenticated (gh auth status)." ), "system_disclaimer": ( "Engineering outcomes emerge from system dynamics, domain complexity, " @@ -928,10 +929,12 @@ "pr_enrichment_degraded_caveat": ( "Os dados de PR estão incompletos nesta execução — passos com falha: " "{steps}. A estratégia de merge sai como desconhecida quando o " - "enriquecimento falha, e métricas de flow, de review e de commits em " - "PR podem faltar ou sair distorcidas. Um erro transitório do GitHub " - "costuma sumir na próxima execução; se toda execução falhar, " - "verifique se o gh está autenticado (gh auth status)." + "enriquecimento dos PRs mergeados falha, e métricas de ciclo de vida " + "de PR (contagens, tempo até o merge, cycle time), de flow, de " + "review, de commits em PR e de PRs abertos podem faltar ou sair " + "distorcidas. Um erro transitório do GitHub costuma sumir na próxima " + "execução; se toda execução falhar, verifique se o gh está " + "autenticado (gh auth status)." ), "system_disclaimer": ( "Resultados de engenharia emergem de dinâmicas sistêmicas, complexidade do domínio " diff --git a/iris/ingestion/github_reader.py b/iris/ingestion/github_reader.py index 2c9fb45..fe5d712 100644 --- a/iris/ingestion/github_reader.py +++ b/iris/ingestion/github_reader.py @@ -124,7 +124,8 @@ def _fetch_prs(nwo: str, limit: int, gh_state: str) -> tuple[list[dict], set[str 3. Reviews via `gh pr list --json number,reviews` in one shot, also capped at `_BATCH_SIZE`. - Both secondary passes are best-effort — if either fails the PRs come + Both secondary passes are best-effort, with one retry on a transient + failure (`_run_gh_with_one_retry`) — if either still fails the PRs come back with the respective field empty, but the rest of the metadata is still usable. @@ -158,7 +159,9 @@ def _fetch_prs(nwo: str, limit: int, gh_state: str) -> tuple[list[dict], set[str if merge_commit: pr["mergeCommit"] = merge_commit - reviews_prs = _gh_pr_list(nwo, "number,reviews", min(limit, _BATCH_SIZE), gh_state) + reviews_prs = _gh_pr_list( + nwo, "number,reviews", min(limit, _BATCH_SIZE), gh_state, retry=True, + ) if reviews_prs is None: degraded.add(DEGRADED_REVIEWS) elif reviews_prs: @@ -204,14 +207,50 @@ def _fetch_prs(nwo: str, limit: int, gh_state: str) -> tuple[list[dict], set[str } -# One retry for the enrichment query, after this pause. GitHub answers that -# query with intermittent 504s (see `_BATCH_SIZE`); a single retry makes a -# degraded run rarer without hiding the ones that still fail. -_ENRICHMENT_RETRY_DELAY_S = 2.0 +# One retry for the enrichment query and the reviews pass, after this pause. +# GitHub answers both with intermittent 504s (see `_BATCH_SIZE`); a single +# retry makes a degraded run rarer without hiding the ones that still fail. +_RETRY_DELAY_S = 2.0 + +# gh writes why it failed to stderr. A 5xx, a timeout (GitHub's GraphQL "may +# be the result of a timeout" error included) or a dropped connection can pass +# on a second try; anything else — a 4xx or other GraphQL error — fails the +# same way again, so retrying it would only add the pause to every run. Whole +# words only, so "thereof" is not an EOF. +_TRANSIENT_GH_ERROR = re.compile( + r"\bHTTP 5\d\d\b|\btime(?:d)? ?out\b|\bdeadline exceeded\b" + r"|\bconnection (?:reset|refused)\b|\bEOF\b", + re.IGNORECASE, +) + + +def _is_transient_gh_error(stderr: str, nwo: str) -> bool: + """Whether gh's stderr reads as an error a second try could clear. + + The repo's ``owner/name``, name and owner are blanked out first: gh echoes + them in its errors, and a repo named ``timeout-svc`` would otherwise make + any error about it look like a timeout. + """ + owner, _, name = nwo.partition("/") + for part in (nwo, name, owner): + if part: + # Only where it stands alone, so a repo named "out" leaves + # "timeout" whole. + stderr = re.sub( + rf"(? subprocess.CompletedProcess | None: + """Run a gh command for ``nwo``; if it fails transiently, wait once and retry. -def _run_gh_with_one_retry(args: list[str]) -> subprocess.CompletedProcess | None: - """Run a gh command; on failure wait once and retry. None if both fail.""" + None when the command fails twice, or fails once with an error that a + retry would not fix. + """ for attempt in (1, 2): try: return subprocess.run( @@ -220,9 +259,10 @@ def _run_gh_with_one_retry(args: list[str]) -> subprocess.CompletedProcess | Non ) except FileNotFoundError: return None - except subprocess.CalledProcessError: - if attempt == 1: - time.sleep(_ENRICHMENT_RETRY_DELAY_S) + except subprocess.CalledProcessError as error: + if attempt == 2 or not _is_transient_gh_error(error.stderr or "", nwo): + return None + time.sleep(_RETRY_DELAY_S) return None @@ -249,9 +289,12 @@ def _fetch_pr_enrichment_graphql( of. Returns ``(by_pr, complete)``. ``complete`` is False when the pass stopped - on an error — a page that failed even after one retry, unparseable - output, a response without data — so ``by_pr`` holds only what earlier - pages collected. Reaching ``max_prs`` or the last page is a normal end. + on an error — a page whose gh call failed (at once on an error a retry + would not fix, after the one retry on a transient one), unparseable + output, a response without data, more pages with no cursor to reach + them — so ``by_pr`` holds only what earlier pages collected, and the + pass counts as failed even when those pages came back. Reaching + ``max_prs`` or the last page is a normal end. """ try: owner, name = nwo.split("/", 1) @@ -280,7 +323,7 @@ def _fetch_pr_enrichment_graphql( if end_cursor: args.extend(["-f", f"cursor={end_cursor}"]) - result = _run_gh_with_one_retry(args) + result = _run_gh_with_one_retry(args, nwo) if result is None: return by_pr, False @@ -342,26 +385,33 @@ def _fetch_pr_enrichment_graphql( return by_pr, True -def _gh_pr_list(nwo: str, fields: str, limit: int, gh_state: str) -> list[dict] | None: - """Run gh pr list and return parsed JSON, or None on failure.""" - try: - result = subprocess.run( - [ - "gh", "pr", "list", - "--repo", nwo, - "--state", gh_state, - "--json", fields, - "--limit", str(limit), - ], - capture_output=True, - text=True, - check=True, - env=git_env(), - ) - except (subprocess.CalledProcessError, FileNotFoundError): - return None +def _gh_pr_list( + nwo: str, fields: str, limit: int, gh_state: str, *, retry: bool = False, +) -> list[dict] | None: + """Run gh pr list and return parsed JSON, or None on failure. - if not result.stdout.strip(): + With ``retry``, a transient failure gets one more try + (`_run_gh_with_one_retry`). + """ + args = [ + "gh", "pr", "list", + "--repo", nwo, + "--state", gh_state, + "--json", fields, + "--limit", str(limit), + ] + if retry: + result = _run_gh_with_one_retry(args, nwo) + else: + try: + result = subprocess.run( + args, capture_output=True, text=True, check=True, + env=git_env(), + ) + except (subprocess.CalledProcessError, FileNotFoundError): + result = None + + if result is None or not result.stdout.strip(): return None try: @@ -441,6 +491,11 @@ def _read_pull_requests_uncached(repo_path: str, days: int) -> PullRequestFetch: return PullRequestFetch( prs=_parse_pull_requests(merged_raw + closed_raw + open_raw, since), degraded=tuple(sorted(merged_degraded | closed_degraded | open_degraded)), + degraded_by_state={ + "merged": tuple(sorted(merged_degraded)), + "closed": tuple(sorted(closed_degraded)), + "open": tuple(sorted(open_degraded)), + }, ) diff --git a/iris/ingestion/window_cache.py b/iris/ingestion/window_cache.py index 57617dc..8f2a8a8 100644 --- a/iris/ingestion/window_cache.py +++ b/iris/ingestion/window_cache.py @@ -72,6 +72,7 @@ def pull_requests( return PullRequestFetch( prs=[pr for pr in fetch.prs if keep(pr, since)], degraded=fetch.degraded, + degraded_by_state=fetch.degraded_by_state, ) fresh = load() diff --git a/iris/metrics/aggregator.py b/iris/metrics/aggregator.py index 7ade07c..8582342 100644 --- a/iris/metrics/aggregator.py +++ b/iris/metrics/aggregator.py @@ -42,7 +42,13 @@ 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, DEGRADED_REVIEWS, PullRequest +from iris.models.pull_request import ( + DEGRADED_BASIC, + DEGRADED_ENRICHMENT, + DEGRADED_FETCH, + DEGRADED_REVIEWS, + PullRequest, +) logger = logging.getLogger(__name__) @@ -69,13 +75,15 @@ def aggregate( prs: list[PullRequest] | None = None, external_data: ExternalDORAData | None = None, pr_fetch_degraded: tuple[str, ...] = (), + pr_fetch_degraded_by_state: dict[str, tuple[str, ...]] | None = None, ) -> ReportMetrics: """Run all analyses on commits and return the combined ReportMetrics. Args: commits: Commits from git_reader (sorted by date ascending). churn_days: Churn/stabilization window in days. - prs: Optional list of merged PRs from github_reader. + prs: Optional PRs from github_reader, in all three states (merged, + closed and open); each analysis keeps the states it reads. external_data: Optional pre-fetched DORA events (deployments + incidents) from a connected provider — currently Datadog. When provided, populates the ``dora_*`` fields on ReportMetrics; @@ -87,18 +95,52 @@ def aggregate( 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. + pr_fetch_degraded_by_state: The same steps per PR state + (`PullRequestFetch.degraded_by_state`). Each omission then + follows the state that feeds the field: merged PRs for review, + flow, acceptance and merge strategy, open PRs for staleness; + closed PRs feed only Flow Load, which every state feeds. A + state whose list (``basic``) failed is missing for everything + it feeds: merged for the PR lifecycle, review, flow, coverage, + acceptance, merge strategy and the timeline's ``prs_merged``; + open for every open-PR field; any state for Flow Load. Without + it (None or empty) every state takes ``pr_fetch_degraded``, + where ``fetch`` (the whole read raised) means every list failed. 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 - # Acceptance and open-PR staleness read the commit pushes (commit_refs) - # that come from the enrichment pass; without them acceptance would read - # 0 commits in PRs and staleness would be inflated. - enrichment_degraded = DEGRADED_ENRICHMENT in pr_fetch_degraded + if pr_fetch_degraded_by_state: + merged_degraded = pr_fetch_degraded_by_state.get("merged", ()) + open_degraded = pr_fetch_degraded_by_state.get("open", ()) + every_state_degraded = list(pr_fetch_degraded_by_state.values()) + else: + merged_degraded = open_degraded = pr_fetch_degraded + every_state_degraded = [pr_fetch_degraded] + # A state whose list failed (`basic`) has no PRs here while the others + # do; with `fetch` (the whole read raised, so only the union carries it) + # no state has any. Whatever a missing state feeds would read as a real + # zero — 0% acceptance, 0 PRs merged a week, a WIP short of its PRs — so + # it is left out, as with no PRs at all. + list_failed = {DEGRADED_BASIC, DEGRADED_FETCH} + merged_missing = not list_failed.isdisjoint(merged_degraded) + open_missing = not list_failed.isdisjoint(open_degraded) + any_list_missing = any( + not list_failed.isdisjoint(steps) for steps in every_state_degraded + ) + # With the reviews pass failed every merged PR has no reviews: + # review-derived fields would read as fabricated facts, so they are left out. + reviews_degraded = DEGRADED_REVIEWS in merged_degraded + # Acceptance and merge strategy read the commit pushes (commit_refs) that + # come from the enrichment pass; without them acceptance would read + # 0 commits in PRs. + enrichment_degraded = DEGRADED_ENRICHMENT in merged_degraded + # Open-PR staleness is measured from the last review or commit push; + # without either it would read older than it is. + staleness_degraded = ( + DEGRADED_REVIEWS in open_degraded or DEGRADED_ENRICHMENT in open_degraded + ) churn_result = calculate_churn(commits, churn_days) stab_result = calculate_stabilization(commits, churn_days) @@ -271,9 +313,12 @@ def aggregate( for c in churn_detail.couplings ] - # Activity timeline + # Activity timeline. Without the merged list, `prs_merged` per week is + # unknown (None), not 0. timeline_kwargs: dict = {} - timeline_result = calculate_activity_timeline(commits, churn_days, prs=prs) + timeline_result = calculate_activity_timeline( + commits, churn_days, prs=None if merged_missing else prs, + ) if timeline_result: timeline_kwargs["activity_timeline"] = [ { @@ -302,7 +347,7 @@ def aggregate( # Acceptance rate (requires PR data with commit hashes) acceptance_kwargs: dict = {} - if prs and not enrichment_degraded: + if prs and not merged_missing and not enrichment_degraded: acceptance_result = calculate_acceptance_rate(commits, prs) if acceptance_result: if acceptance_result.by_origin: @@ -318,7 +363,7 @@ def aggregate( # 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 and not reviews_degraded: + if prs and not merged_missing and not reviews_degraded: flow_efficiency_result = analyze_flow_efficiency( prs, commit_origin_map=origin_map, @@ -348,7 +393,7 @@ def aggregate( # Human Review Coverage — fraction of merged PRs a human actually reviewed human_review_coverage_kwargs: dict = {} - if prs and not reviews_degraded: + if prs and not merged_missing and not reviews_degraded: coverage_result = analyze_human_review_coverage( prs, commit_origin_map=origin_map, @@ -371,7 +416,7 @@ def aggregate( # Open PR Aging — snapshot of stuck inventory (non-draft, non-bot) open_pr_aging_kwargs: dict = {} - if prs: + if prs and not open_missing: aging_result = analyze_open_pr_aging( prs, now=now_utc(), @@ -389,9 +434,7 @@ def aggregate( open_pr_aging_kwargs["median_open_pr_age_by_intent"] = ( aging_result.median_open_pr_age_by_intent ) - # Staleness is measured from the last review or commit; without - # the reviews or the commit pushes it would read older than it is. - if not reviews_degraded and not enrichment_degraded: + if not staleness_degraded: open_pr_aging_kwargs["stale_open_pr_pct"] = ( aging_result.stale_open_pr_pct ) @@ -401,11 +444,7 @@ def aggregate( open_pr_aging_kwargs["abandonment_risk_pct"] = ( aging_result.abandonment_risk_pct ) - if ( - not reviews_degraded - and not enrichment_degraded - and aging_result.stale_open_pr_pct_by_origin - ): + if not staleness_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 ) @@ -415,7 +454,7 @@ def aggregate( # PR ground truth (merge_commit_parent_count) and commit_refs with the # local main history. Strictly per-repository — no author axis. merge_strategy_kwargs: dict = {} - if prs: + if prs and not merged_missing: merge_strategy_result = detect_merge_strategy( prs, commits, @@ -436,9 +475,11 @@ def aggregate( merge_strategy_result.distribution, ) - # Flow Load — WIP per ISO week (PRs in flight + author concurrency) + # Flow Load — WIP per ISO week (PRs in flight + author concurrency). + # Every PR state is in flight at some point, so a missing list in any of + # them would undercount the WIP. flow_load_kwargs: dict = {} - flow_load_result = analyze_flow_load(prs or [], commits) + flow_load_result = None if any_list_missing else analyze_flow_load(prs or [], commits) if flow_load_result: flow_load_kwargs["flow_load"] = [ { @@ -454,7 +495,7 @@ def aggregate( # PR lifecycle (optional — only over merged PRs) pr_kwargs: dict = {} - if prs: + if prs and not merged_missing: pr_result = analyze_pr_lifecycle(prs) if pr_result is not None: pr_kwargs = { diff --git a/iris/models/pull_request.py b/iris/models/pull_request.py index 98b7a95..baa763f 100644 --- a/iris/models/pull_request.py +++ b/iris/models/pull_request.py @@ -93,7 +93,13 @@ class PullRequestFetch: sorted and without repeats. It is empty when every step succeeded, and also when PRs are absent *by design* (no ``gh`` on the machine, no GitHub remote): Iris works without PRs, and that is not a degradation. + + ``degraded_by_state`` maps each gh state read (``merged``, ``closed``, + ``open``) to the steps that failed for it; ``degraded`` is their union. + Empty when the read never got to the states — absent by design, or the + whole read raised (``fetch``). """ prs: list[PullRequest] degraded: tuple[str, ...] = () + degraded_by_state: dict[str, tuple[str, ...]] = field(default_factory=dict) diff --git a/iris/org_runner.py b/iris/org_runner.py index 5ae2434..a3d6b5c 100644 --- a/iris/org_runner.py +++ b/iris/org_runner.py @@ -90,6 +90,7 @@ def analyze_single_repo( churn_days=churn_days, prs=prs or None, pr_fetch_degraded=pr_fetch.degraded, + pr_fetch_degraded_by_state=pr_fetch.degraded_by_state, ) # Step 4a: Repository kind (keeps docs/board repos out of org comparisons) @@ -109,6 +110,7 @@ def analyze_single_repo( recent_metrics = aggregate( recent_commits, churn_days=churn_days, prs=recent_prs, pr_fetch_degraded=pr_fetch.degraded, + pr_fetch_degraded_by_state=pr_fetch.degraded_by_state, ) trend = compute_trend_delta( baseline=metrics, @@ -147,10 +149,12 @@ def analyze_single_repo( pre_metrics = aggregate( pre_commits, churn_days=churn_days, prs=pre_prs, pr_fetch_degraded=pr_fetch.degraded, + pr_fetch_degraded_by_state=pr_fetch.degraded_by_state, ) post_metrics = aggregate( post_commits, churn_days=churn_days, prs=post_prs, pr_fetch_degraded=pr_fetch.degraded, + pr_fetch_degraded_by_state=pr_fetch.degraded_by_state, ) pre_days = max(1, (pre_commits[-1].date - pre_commits[0].date).days) diff --git a/tests/test_pr_enrichment_caveat.py b/tests/test_pr_enrichment_caveat.py index d35ffc2..64bc28f 100644 --- a/tests/test_pr_enrichment_caveat.py +++ b/tests/test_pr_enrichment_caveat.py @@ -68,3 +68,20 @@ def test_caveat_is_translated(tmp_path): def test_clean_run_has_no_caveat(tmp_path): assert _CAVEAT_EN not in _report_text(tmp_path, None) + + +def test_caveat_lists_open_pr_metrics_among_what_may_be_missing(tmp_path): + # A failed open list, or a failed enrichment or reviews pass for open + # PRs, omits open-PR fields too, so the caveat names them. + assert "flow, review, in-PR and open-PR metrics" in _report_text(tmp_path, ["basic"]) + assert "de commits em PR e de PRs abertos" in _report_text( + tmp_path, ["basic"], lang="pt-br") + + +def test_caveat_lists_pr_lifecycle_metrics_among_what_may_be_missing(tmp_path): + # A failed merged list omits the PR lifecycle fields, and a partial read + # skews them, so the caveat names them too. + assert "PR lifecycle (counts, time to merge, cycle time)" in _report_text( + tmp_path, ["basic"]) + assert "ciclo de vida de PR (contagens, tempo até o merge, cycle time)" in ( + _report_text(tmp_path, ["basic"], lang="pt-br")) diff --git a/tests/test_pr_fetch_degradation.py b/tests/test_pr_fetch_degradation.py index 2649fc3..4d6b8ee 100644 --- a/tests/test_pr_fetch_degradation.py +++ b/tests/test_pr_fetch_degradation.py @@ -71,6 +71,8 @@ def broken_read(_repo_path, _days): import subprocess from types import SimpleNamespace +import pytest + def _ok(payload) -> SimpleNamespace: return SimpleNamespace(returncode=0, stdout=json.dumps(payload), stderr="") @@ -99,10 +101,12 @@ def _fake_gh(monkeypatch, *, full=None, basic=None, graphql=(), reviews=None): """Route each gh call to a scripted answer. `full`, `basic` and `reviews` are a payload, or an exception to raise. + `reviews` can also be a tuple of answers for successive reviews calls. `graphql` is the sequence of answers for successive GraphQL calls. Returns the list of pauses the retry slept, so a test can assert on them. """ graphql_answers = list(graphql) + reviews_answers = list(reviews) if isinstance(reviews, tuple) else None sleeps: list[float] = [] def answer(spec, cmd): @@ -117,6 +121,8 @@ def fake_run(cmd, **kwargs): return answer(graphql_answers.pop(0), cmd) fields = cmd[cmd.index("--json") + 1] if fields == "number,reviews": + if reviews_answers is not None: + return answer(reviews_answers.pop(0), cmd) return answer(reviews, cmd) if "commits" in fields: return answer(full, cmd) @@ -131,6 +137,14 @@ def _failure() -> subprocess.CalledProcessError: return subprocess.CalledProcessError(1, ["gh"], stderr="HTTP 504") +# Errors that fail the same way on a second try: retrying them only waits. +_DETERMINISTIC_STDERR = ( + "HTTP 401: Bad credentials (https://api.github.com/graphql)", + "gh: Not Found (HTTP 404)", + "GraphQL: Could not resolve to a Repository with the name 'acme/widgets'. (repository)", +) + + def test_enrichment_completes_on_a_single_page(monkeypatch): sleeps = _fake_gh(monkeypatch, graphql=[_graphql_page([1, 2])]) @@ -148,7 +162,7 @@ def test_enrichment_retries_once_and_recovers(monkeypatch): assert sorted(by_pr) == [1] assert complete is True - assert sleeps == [github_reader._ENRICHMENT_RETRY_DELAY_S] + assert sleeps == [github_reader._RETRY_DELAY_S] def test_enrichment_that_fails_twice_is_incomplete(monkeypatch): @@ -158,7 +172,135 @@ def test_enrichment_that_fails_twice_is_incomplete(monkeypatch): assert by_pr == {} assert complete is False - assert sleeps == [github_reader._ENRICHMENT_RETRY_DELAY_S] + assert sleeps == [github_reader._RETRY_DELAY_S] + + +@pytest.mark.parametrize("stderr", _DETERMINISTIC_STDERR) +def test_enrichment_does_not_retry_a_deterministic_error(monkeypatch, stderr): + # One scripted answer: a second call would find the script empty and raise. + sleeps = _fake_gh(monkeypatch, graphql=[ + subprocess.CalledProcessError(1, ["gh"], stderr=stderr), + ]) + + by_pr, complete = github_reader._fetch_pr_enrichment_graphql("acme/widgets", "merged", 500) + + assert by_pr == {} + assert complete is False + assert sleeps == [] + + +# A repo whose name looks like a transient error must not make a +# deterministic error look transient, and must not hide a real one. +_TIMEOUT_REPO = "acme/timeout-svc" + + +@pytest.mark.parametrize("stderr", [ + "GraphQL: Could not resolve to a Repository with the name 'acme/timeout-svc'. (repository)", + "HTTP 404: Not Found (https://api.github.com/repos/acme/timeout-svc/pulls)", + "no pull requests match your search in timeout-svc", + "GraphQL: Field 'oid' is not defined on any part thereof (query)", +]) +def test_enrichment_does_not_read_the_repo_name_as_a_transient_error(monkeypatch, stderr): + sleeps = _fake_gh(monkeypatch, graphql=[ + subprocess.CalledProcessError(1, ["gh"], stderr=stderr), + ]) + + _by_pr, complete = github_reader._fetch_pr_enrichment_graphql(_TIMEOUT_REPO, "merged", 500) + + assert complete is False + assert sleeps == [] + + +@pytest.mark.parametrize("nwo, stderr", [ + (_TIMEOUT_REPO, "HTTP 504: Gateway Timeout (https://api.github.com/graphql)"), + (_TIMEOUT_REPO, "Post \"https://api.github.com/graphql\": unexpected EOF"), + ("acme/out", "dial tcp: i/o timeout"), + ("acme/out", "read: connection reset by peer"), + ("acme/widgets", "GraphQL: Something went wrong while executing your query. " + "This may be the result of a timeout, or it could be a GitHub bug."), +]) +def test_enrichment_still_retries_a_transient_error(monkeypatch, nwo, stderr): + sleeps = _fake_gh(monkeypatch, graphql=[ + subprocess.CalledProcessError(1, ["gh"], stderr=stderr), _graphql_page([1]), + ]) + + by_pr, complete = github_reader._fetch_pr_enrichment_graphql(nwo, "merged", 500) + + assert sorted(by_pr) == [1] + assert complete is True + assert sleeps == [github_reader._RETRY_DELAY_S] + + +def test_reviews_pass_does_not_read_the_repo_name_as_a_transient_error(monkeypatch): + sleeps = _fake_gh( + monkeypatch, + basic=[{"number": 1}], + graphql=[_graphql_page([1])], + reviews=(subprocess.CalledProcessError( + 1, ["gh"], stderr="GraphQL: Could not resolve to a Repository with " + "the name 'acme/timeout-svc'. (repository)"),), + ) + + _prs, degraded = github_reader._fetch_prs(_TIMEOUT_REPO, 1000, "merged") + + assert degraded == {"reviews"} + assert sleeps == [] + + +def test_reviews_pass_retries_a_transient_failure_and_recovers(monkeypatch): + review = {"author": {"login": "reviewer"}, "state": "APPROVED", + "submittedAt": "2026-09-01T00:00:00Z"} + sleeps = _fake_gh( + monkeypatch, + basic=[{"number": 1}], + graphql=[_graphql_page([1])], + reviews=(_failure(), [{"number": 1, "reviews": [review]}]), + ) + + prs, degraded = github_reader._fetch_prs("acme/widgets", 1000, "merged") + + assert prs[0]["reviews"] == [review] + assert degraded == set() + assert sleeps == [github_reader._RETRY_DELAY_S] + + +def test_reviews_pass_that_fails_twice_is_reported(monkeypatch): + sleeps = _fake_gh( + monkeypatch, + basic=[{"number": 1}], + graphql=[_graphql_page([1])], + reviews=(_failure(), _failure()), + ) + + _prs, degraded = github_reader._fetch_prs("acme/widgets", 1000, "merged") + + assert degraded == {"reviews"} + assert sleeps == [github_reader._RETRY_DELAY_S] + + +@pytest.mark.parametrize("stderr", _DETERMINISTIC_STDERR) +def test_reviews_pass_does_not_retry_a_deterministic_error(monkeypatch, stderr): + sleeps = _fake_gh( + monkeypatch, + basic=[{"number": 1}], + graphql=[_graphql_page([1])], + reviews=(subprocess.CalledProcessError(1, ["gh"], stderr=stderr),), + ) + + _prs, degraded = github_reader._fetch_prs("acme/widgets", 1000, "merged") + + assert degraded == {"reviews"} + assert sleeps == [] + + +def test_basic_list_is_not_retried(monkeypatch): + sleeps = _fake_gh(monkeypatch, basic=_failure()) + + prs, degraded = github_reader._fetch_prs("acme/widgets", 1000, "merged") + + assert prs == [] + assert degraded == {"basic"} + assert sleeps == [] def test_enrichment_failing_on_a_later_page_keeps_what_it_has_but_is_incomplete(monkeypatch): @@ -259,6 +401,13 @@ def test_degraded_steps_from_every_state_are_merged_and_sorted(monkeypatch): fetch = github_reader.read_pull_requests("/any/repo", days=30) assert fetch.degraded == ("enrichment", "reviews") + # Each state keeps its own steps, so a metric can be gated on the state + # that feeds it. + assert fetch.degraded_by_state == { + "merged": ("enrichment", "reviews"), + "closed": (), + "open": ("enrichment",), + } # --- the signal in the metrics --------------------------------------------- @@ -958,3 +1107,248 @@ def test_cli_metrics_json_keeps_the_funnel_on_a_clean_run(tmp_path, monkeypatch) tmp_path, monkeypatch, PullRequestFetch(prs=[], degraded=())) assert "origin_funnel" in payload + + +# --- the gate follows the PR state that feeds each metric ------------------- +# +# A step can fail for one PR state only. Review, flow, acceptance and merge +# strategy read merged PRs; staleness reads open PRs; closed PRs feed only the +# basic list. A failure in one state leaves the metrics of the others alone, +# while `pr_enrichment_degraded` still lists the union. + + +def _per_state(**steps) -> dict[str, tuple[str, ...]]: + return {"merged": (), "closed": (), "open": (), **steps} + + +def _payload(degraded, by_state=None) -> dict: + return aggregate( + _stamped_commits(), churn_days=14, prs=_reviewed_prs() + _open_prs(), + pr_fetch_degraded=degraded, pr_fetch_degraded_by_state=by_state, + ).to_dict() + + +def test_reviews_degraded_only_for_open_prs_omits_only_staleness(): + clean = _payload(()) + payload = _payload(("reviews",), _per_state(open=("reviews",))) + + assert payload.pop("pr_enrichment_degraded") == ["reviews"] + assert payload["pr_single_pass_rate"] == clean["pr_single_pass_rate"] + assert payload["flow_efficiency_median"] == clean["flow_efficiency_median"] + for key in _STALENESS_KEYS: + assert key not in payload, key + clean.pop(key, None) + assert payload == clean + + +def test_enrichment_degraded_only_for_closed_prs_omits_nothing(): + clean = _payload(()) + payload = _payload(("enrichment",), _per_state(closed=("enrichment",))) + + assert payload.pop("pr_enrichment_degraded") == ["enrichment"] + assert payload["merge_strategy"] != "unknown" + assert payload["flow_efficiency_median"] == clean["flow_efficiency_median"] + assert payload["acceptance_by_origin"] == clean["acceptance_by_origin"] + assert payload == clean + + +def test_enrichment_degraded_only_for_merged_prs_keeps_staleness(): + payload = _payload(("enrichment",), _per_state(merged=("enrichment",))) + + assert payload["merge_strategy"] == "unknown" + assert "acceptance_by_origin" not in payload + for key in _STALENESS_KEYS: + if key != "stale_open_pr_pct_by_origin": + assert key in payload, key + + +def test_origin_funnel_follows_the_merged_enrichment(): + def funnel(by_state): + return calculate_origin_funnel(aggregate( + _commits_with_ai(), churn_days=14, prs=_reviewed_prs(), + pr_fetch_degraded=("enrichment",), pr_fetch_degraded_by_state=by_state, + )) + + assert funnel(_per_state(open=("enrichment",))) is not None + assert funnel(_per_state(merged=("enrichment",))) is None + + +@pytest.mark.parametrize("entry", ["cli", "org_runner"]) +def test_every_aggregate_call_gets_the_per_state_degradation(tmp_path, monkeypatch, entry): + from iris import cli, org_runner + + repo = tmp_path / "repo" + repo.mkdir() + _build_repo(repo) + fetch = PullRequestFetch( + prs=_reviewed_prs(), degraded=("reviews",), + degraded_by_state=_per_state(open=("reviews",)), + ) + module = cli if entry == "cli" else org_runner + monkeypatch.setattr(module, "read_pull_requests_with_fallback", lambda *a, **k: fetch) + _adoption_after_first_commit(monkeypatch) + calls: list[dict] = [] + real_aggregate = module.aggregate + + def spy(*args, **kwargs): + calls.append(kwargs) + return real_aggregate(*args, **kwargs) + + monkeypatch.setattr(module, "aggregate", spy) + + if entry == "cli": + cli._run_single_repo(argparse.Namespace( + repo_path=str(repo), days=30, churn_days=14, lang="en", recent_days=30, + verbose=False, trend=True, out=str(tmp_path / "out"), no_push=True, + )) + else: + org_runner.analyze_single_repo( + str(repo), days=30, churn_days=14, out_dir=str(tmp_path / "out"), + trend_enabled=True, + ) + + # Main run, trend window, pre- and post-adoption. + assert len(calls) == 4 + for kwargs in calls: + assert kwargs["pr_fetch_degraded"] == ("reviews",) + assert kwargs["pr_fetch_degraded_by_state"] == fetch.degraded_by_state + payload = _metrics_json(tmp_path / "out") + assert payload["pr_enrichment_degraded"] == ["reviews"] + assert "pr_single_pass_rate" in payload + + +# --- a failed list omits what its state feeds ------------------------------- +# +# When `gh pr list` fails for a state, that state's PRs are missing while the +# others are read: the PR list is not empty, so every metric the missing state +# feeds would read as a real zero (0% acceptance, 0 PRs merged a week, a WIP +# short of the PRs in flight). Those are left out instead. + +# Fed by merged PRs: lifecycle, review, flow, coverage, acceptance, strategy. +_MERGED_FED_KEYS = ( + "pr_merged_count", + "pr_median_time_to_merge_hours", + "pr_mean_time_to_merge_hours", + "pr_p90_time_to_merge_hours", + "pr_pct_merged_within_24h", + "pr_cycle_time_buckets", + "pr_median_size_files", + "pr_median_size_lines", + *_REVIEW_KEYS, + *_FLOW_KEYS, + "acceptance_by_origin", + "acceptance_by_tool", + "merge_strategy", + "merge_strategy_dominant_share", + "commit_metrics_reliable", +) +_OPEN_FED_KEYS = (*_AGE_KEYS, "median_open_pr_age_by_intent", *_STALENESS_KEYS) + + +def _closed_prs() -> list[PullRequest]: + return [ + PullRequest(number=200 + n, title=f"closed {n}", author="dev", + created_at=_NOW, additions=1, deletions=0, changed_files=1, + closed_at=_NOW + timedelta(hours=n), state="closed", + commit_refs=[CommitRef(hash=f"c{n}", committed_at=_NOW)]) + for n in range(1, 4) + ] + + +def _two_week_commits() -> list[Commit]: + # The activity timeline needs at least two ISO weeks of commits. + later = Commit(hash="c9", author="dev", date=_NOW + timedelta(days=8), + message="change 9 (#9)") + return _stamped_commits() + [later] + + +def _list_payload(prs, degraded, by_state) -> dict: + return aggregate( + _two_week_commits(), churn_days=14, prs=prs, + pr_fetch_degraded=degraded, pr_fetch_degraded_by_state=by_state, + ).to_dict() + + +def test_missing_merged_list_omits_what_merged_prs_feed(): + payload = _list_payload( + _closed_prs() + _open_prs(), ("basic",), _per_state(merged=("basic",))) + + for key in _MERGED_FED_KEYS: + assert key not in payload, key + assert "flow_load" not in payload + assert payload["activity_timeline"] + for week in payload["activity_timeline"]: + assert week["prs_merged"] is None + assert week["pr_median_ttm_hours"] is None + # The open list was read: its fields stay. + for key in _AGE_KEYS: + assert key in payload, key + + +def test_missing_open_list_omits_every_open_pr_field(): + clean = _list_payload(_reviewed_prs() + _closed_prs(), (), None) + payload = _list_payload( + _reviewed_prs() + _closed_prs(), ("basic",), _per_state(open=("basic",))) + + for key in _OPEN_FED_KEYS: + assert key not in payload, key + assert "flow_load" not in payload + # The merged list was read: its fields stay as on a clean run. + assert payload["pr_merged_count"] == 6 + assert payload["acceptance_by_origin"] == clean["acceptance_by_origin"] + assert payload["activity_timeline"] == clean["activity_timeline"] + + +def test_missing_closed_list_omits_only_flow_load(): + prs = _reviewed_prs() + _closed_prs() + _open_prs() + clean = _list_payload(prs, (), None) + payload = _list_payload(prs, ("basic",), _per_state(closed=("basic",))) + + assert clean["flow_load"] + assert payload.pop("pr_enrichment_degraded") == ["basic"] + clean.pop("flow_load") + assert payload == clean + + +@pytest.mark.parametrize("by_state", [None, {}]) +def test_missing_list_without_the_per_state_dict_omits_every_state(by_state): + payload = _list_payload( + _reviewed_prs() + _closed_prs() + _open_prs(), ("basic",), by_state) + + for key in (*_MERGED_FED_KEYS, *_OPEN_FED_KEYS, "flow_load"): + assert key not in payload, key + for week in payload["activity_timeline"]: + assert week["prs_merged"] is None + + +@pytest.mark.parametrize("degraded, by_state", [ + (("fetch",), {}), + (("basic",), _per_state(merged=("basic",), closed=("basic",), open=("basic",))), +]) +def test_read_that_lost_every_list_leaves_no_pr_zeros(degraded, by_state): + payload = _list_payload([], degraded, by_state) + + assert "flow_load" not in payload + for week in payload["activity_timeline"]: + assert week["prs_merged"] is None + + +@pytest.mark.parametrize("prs", [None, []]) +def test_prs_absent_by_design_keep_flow_load(prs): + # No gh or no GitHub remote: nothing failed, so nothing is left out. + payload = _list_payload(prs, (), None) + + assert "flow_load" in payload + + +# --- without the per-state dict, the union applies --------------------------- + + +@pytest.mark.parametrize("by_state", [None, {}]) +def test_reviews_without_the_per_state_dict_omit_merged_and_open_metrics(by_state): + payload = _payload(("reviews",), by_state) + + 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 diff --git a/tests/test_window_cache.py b/tests/test_window_cache.py index 1c9cef7..83d192f 100644 --- a/tests/test_window_cache.py +++ b/tests/test_window_cache.py @@ -111,7 +111,10 @@ def load(): def test_a_narrower_window_keeps_the_wide_fetch_degradation(): def load(): - return PullRequestFetch(prs=[_pr(80), _pr(3)], degraded=("enrichment",)) + return PullRequestFetch( + prs=[_pr(80), _pr(3)], degraded=("enrichment",), + degraded_by_state={"merged": ("enrichment",), "closed": (), "open": ()}, + ) window_cache.enable() window_cache.pull_requests("r", 90, load, _keep) @@ -120,6 +123,7 @@ def load(): narrow = window_cache.pull_requests("r", 7, load, _keep) assert len(narrow.prs) == 1 assert narrow.degraded == ("enrichment",) + assert narrow.degraded_by_state == {"merged": ("enrichment",), "closed": (), "open": ()} # --- the overlap predicate (single source of truth for the filter) ---------