diff --git a/docs/METRICS.md b/docs/METRICS.md index e181bdc..6131bd8 100644 --- a/docs/METRICS.md +++ b/docs/METRICS.md @@ -236,8 +236,8 @@ 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 @@ -245,6 +245,8 @@ zero `CHANGES_REQUESTED` / PRs), `median_review_rounds`. The last two are omitte 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,13 +258,16 @@ 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` for -merged PRs, 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. +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. @@ -271,8 +276,10 @@ 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,18 +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), `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, a dropped -connection); a 4xx or 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 — 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`, @@ -416,16 +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 -counts (Flow Load, §24), so a failure there omits nothing. -`pr_enrichment_degraded` still lists the union over the three states. +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. --- @@ -597,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`: @@ -612,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)`: ``` @@ -663,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): @@ -765,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): @@ -837,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**): @@ -915,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: @@ -966,6 +1003,8 @@ without classifying: `merge_strategy_dominant_share` is absent and 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 d231d4a..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). diff --git a/iris/i18n.py b/iris/i18n.py index 71cbfa6..4fba747 100644 --- a/iris/i18n.py +++ b/iris/i18n.py @@ -36,7 +36,8 @@ "pr_enrichment_degraded_caveat": ( "PR data is incomplete for this run — failed steps: {steps}. Merge " "strategy is reported as unknown when enrichment failed for merged " - "PRs, and flow, review and in-PR metrics may be missing or skewed. A " + "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)." ), @@ -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 dos PRs mergeados 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 ab85714..fe5d712 100644 --- a/iris/ingestion/github_reader.py +++ b/iris/ingestion/github_reader.py @@ -212,18 +212,41 @@ def _fetch_prs(nwo: str, limit: int, gh_state: str) -> tuple[list[dict], set[str # 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 or a slow or dropped connection can -# pass on a second try; anything else — a 4xx, a GraphQL error — fails the same -# way again, so retrying it would only add the pause to every run. +# 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"HTTP 5\d\d|timeout|timed out|deadline exceeded|connection reset" - r"|connection refused|EOF", + r"\bHTTP 5\d\d\b|\btime(?:d)? ?out\b|\bdeadline exceeded\b" + r"|\bconnection (?:reset|refused)\b|\bEOF\b", re.IGNORECASE, ) -def _run_gh_with_one_retry(args: list[str]) -> subprocess.CompletedProcess | None: - """Run a gh command; if it fails transiently, wait once and retry. +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. None when the command fails twice, or fails once with an error that a retry would not fix. @@ -237,7 +260,7 @@ def _run_gh_with_one_retry(args: list[str]) -> subprocess.CompletedProcess | Non except FileNotFoundError: return None except subprocess.CalledProcessError as error: - if attempt == 2 or not _TRANSIENT_GH_ERROR.search(error.stderr or ""): + if attempt == 2 or not _is_transient_gh_error(error.stderr or "", nwo): return None time.sleep(_RETRY_DELAY_S) return None @@ -266,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) @@ -297,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 @@ -375,7 +401,7 @@ def _gh_pr_list( "--limit", str(limit), ] if retry: - result = _run_gh_with_one_retry(args) + result = _run_gh_with_one_retry(args, nwo) else: try: result = subprocess.run( diff --git a/iris/metrics/aggregator.py b/iris/metrics/aggregator.py index caa1357..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__) @@ -76,7 +82,8 @@ def aggregate( 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; @@ -92,8 +99,13 @@ def aggregate( (`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 counts, so they omit nothing. Without it - (None or empty) every state takes ``pr_fetch_degraded``. + 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 @@ -102,8 +114,21 @@ def aggregate( 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 @@ -288,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"] = [ { @@ -319,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: @@ -335,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, @@ -365,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, @@ -388,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(), @@ -426,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, @@ -447,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"] = [ { @@ -465,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/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 b0204ed..4d6b8ee 100644 --- a/tests/test_pr_fetch_degradation.py +++ b/tests/test_pr_fetch_degradation.py @@ -189,6 +189,64 @@ def test_enrichment_does_not_retry_a_deterministic_error(monkeypatch, stderr): 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"} @@ -1157,3 +1215,140 @@ def spy(*args, **kwargs): 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