From 7d81af2fa1297bc184655c68d6351c7fcdce840e Mon Sep 17 00:00:00 2001 From: Chandrasekharan M Date: Fri, 7 Aug 2026 17:58:29 +0530 Subject: [PATCH 1/6] UN-3770 [FIX] Scope and gate critical-path regressions in the rig report `report combine` is the only cross-tier evaluation, but it called evaluate() without scope_groups, so every baseline-covered path whose tier sat out the build was classified as a regression. A frontend-only PR skips the unit and integration tiers, so all nine integration-tier paths were reported as regressed on each one. It also never gated on the result, returning 0 no matter how many regressions it rendered into the PR comment. The two defects masked each other: the false positives were loud but harmless, so a real regression would have been just as harmless. Scope to the groups that actually emitted junit, and fail on what survives. Split the report's gap section so a path covered by a tier that did not run is no longer listed as "not yet covered", which reads as missing tests. On a full run every group reports, so scoping is a no-op there; only builds with a skipped tier change behaviour. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UpQ4upCajYZKzTFLL23cT1 --- tests/rig/cli.py | 24 ++++++- tests/rig/reporting.py | 21 ++++++- tests/rig/tests/test_cli.py | 122 ++++++++++++++++++++++++++++++++++++ 3 files changed, 164 insertions(+), 3 deletions(-) diff --git a/tests/rig/cli.py b/tests/rig/cli.py index f93fe622f4..eba36ce8cb 100644 --- a/tests/rig/cli.py +++ b/tests/rig/cli.py @@ -371,8 +371,18 @@ def cmd_report(args: argparse.Namespace) -> int: print(f"[rig] {exc}", file=sys.stderr) baseline = None baseline_corrupt = True + # A tier the path filter skipped emits no junit, so its groups are absent + # here. Scoping to the groups that actually reported keeps their paths as + # gaps rather than regressions — nothing regressed, the tier never ran. A + # group that ran and went red still reports, so it stays in scope and a + # genuine regression is still caught. + scope_groups = [r.name for r in group_results] statuses = cp.evaluate( - registry, groups_run_green=green, baseline=baseline, marker_proven=proven + registry, + groups_run_green=green, + baseline=baseline, + scope_groups=scope_groups, + marker_proven=proven, ) write_summary( reports_dir=reports_dir, @@ -388,6 +398,16 @@ def cmd_report(args: argparse.Namespace) -> int: f"(not in tests/critical_paths.yaml)", file=sys.stderr, ) + # This is the only cross-tier evaluation, so it is also the only place a + # regression can be gated on. cmd_run deliberately doesn't: a single tier + # can't tell "another tier's path went red" from "another tier didn't run". + regressions = [s for s in statuses if s.state == "regression"] + if regressions: + ids = ", ".join(s.path.id for s in regressions) + print( + f"\n[rig] ❌ {len(regressions)} critical-path regression(s) detected: {ids}", + file=sys.stderr, + ) if args.update_baseline: red = [ r.name @@ -404,7 +424,7 @@ def cmd_report(args: argparse.Namespace) -> int: return 1 cp.merge_into_baseline(statuses, baseline_path) print(f"[rig] merged into baseline: {baseline_path}") - return 1 if unknown_marker_ids else 0 + return 1 if unknown_marker_ids or regressions else 0 def cmd_run(args: argparse.Namespace) -> int: diff --git a/tests/rig/reporting.py b/tests/rig/reporting.py index 4c8ed8eda4..b1a6527cf2 100644 --- a/tests/rig/reporting.py +++ b/tests/rig/reporting.py @@ -318,7 +318,11 @@ def _render_markdown( if critical_statuses: regressions = [s for s in critical_statuses if s.state == "regression"] - gaps = [s for s in critical_statuses if s.state == "gap"] + all_gaps = [s for s in critical_statuses if s.state == "gap"] + # A path whose covering groups exist but sat out this build is not + # "not yet covered" — reporting it as such reads as missing tests. + unexercised = [s for s in all_gaps if not s.in_scope and s.path.covered_by] + gaps = [s for s in all_gaps if s not in unexercised] covered = [s for s in critical_statuses if s.state == "covered"] lines.append("## Critical paths") @@ -339,6 +343,21 @@ def _render_markdown( f"(declared coverage: {covers})" ) lines.append("") + if unexercised: + lines.append( + "
💤 Covered, but not exercised in this " + "build" + ) + lines.append("") + for s in unexercised: + covers = ", ".join(s.path.covered_by) + lines.append( + f"- **{s.path.id}** — {s.path.description} " + f"(covered by {covers}; that tier did not run here)" + ) + lines.append("") + lines.append("
") + lines.append("") if covered: lines.append("
✅ Covered critical paths") lines.append("") diff --git a/tests/rig/tests/test_cli.py b/tests/rig/tests/test_cli.py index 17138bf6dc..b1e972c489 100644 --- a/tests/rig/tests/test_cli.py +++ b/tests/rig/tests/test_cli.py @@ -955,3 +955,125 @@ def test_resolve_configfile_ignores_commented_section(tmp_path: Path) -> None: child.mkdir() assert cli._resolve_pytest_configfile(child) is None + + +def _drive_cmd_report( + tmp_path: Path, + monkeypatch, + *, + reporting_groups: dict[str, int], +) -> tuple[int, dict[str, str]]: + """Drive ``cmd_report`` over one integration group covering ``int-path``. + + ``reporting_groups`` maps group name -> failure count for the groups that + emitted junit this build. A group absent from it emitted nothing, standing + in for a tier the CI path filter skipped. Returns the exit code and each + path's final state. + """ + from tests.rig.reporting import GroupResult + + test_dir = Path(__file__).parent + manifest_yaml = ( + "version: 1\n" + "groups:\n" + " int-g:\n" + " tier: integration\n" + f" workdir: {test_dir}\n" + " paths: [.]\n" + ) + (tmp_path / "groups.yaml").write_text(manifest_yaml) + (tmp_path / "critical_paths.yaml").write_text( + "version: 1\n" + "paths:\n" + " - id: int-path\n" + " description: covered only by the integration tier\n" + " covered_by: [int-g]\n" + ) + + import tests.rig.cli as cli_mod + import tests.rig.critical_paths as cp_mod + import tests.rig.groups as groups_mod + + monkeypatch.setattr(groups_mod, "DEFAULT_MANIFEST", tmp_path / "groups.yaml") + monkeypatch.setattr(cp_mod, "DEFAULT_REGISTRY", tmp_path / "critical_paths.yaml") + # Coverage combining needs real .coverage files and is not under test here. + monkeypatch.setattr(cli_mod, "combine_and_report", lambda reports_dir: None) + + def fake_parse_junit(name, tier, reports_dir): + if name not in reporting_groups: + return None + failed = reporting_groups[name] + return GroupResult( + name=name, + tier=tier, + exit_code=1 if failed else 0, + passed=0 if failed else 1, + failed=failed, + errors=0, + skipped=0, + duration_seconds=0.01, + ) + + monkeypatch.setattr(cli_mod, "parse_junit", fake_parse_junit) + + states: dict[str, str] = {} + real_evaluate = cp_mod.evaluate + + def spy_evaluate(*args, **kwargs): + statuses = real_evaluate(*args, **kwargs) + states.update({s.path.id: s.state for s in statuses}) + return statuses + + monkeypatch.setattr(cli_mod.cp, "evaluate", spy_evaluate) + + reports_dir = tmp_path / "reports" + reports_dir.mkdir() + # Baseline says the path was green on main, which is what makes the + # not-covered-now decision a regression-or-gap question at all. + (reports_dir / "previous-summary.json").write_text( + '{"covered_paths": ["int-path"]}' + ) + + args = cli_mod._build_parser().parse_args( + [ + "report", + "combine", + "--reports-dir", + str(reports_dir), + "--baseline", + str(reports_dir / "previous-summary.json"), + ] + ) + return cli_mod.cmd_report(args), states + + +def test_cmd_report_skipped_tier_is_a_gap_not_a_regression( + tmp_path: Path, monkeypatch +) -> None: + """A frontend-only PR skips the integration tier, so its groups emit no + junit. Those paths must degrade to ``gap`` — nothing regressed, the tier + never ran. Without ``scope_groups`` every baseline path in a skipped tier + is reported as regressed on every such PR. + """ + exit_code, states = _drive_cmd_report(tmp_path, monkeypatch, reporting_groups={}) + + assert states["int-path"] == "gap", ( + "a path whose tier did not run must not be called a regression; " + f"got {states['int-path']}" + ) + assert exit_code == 0 + + +def test_cmd_report_gates_on_a_real_regression(tmp_path: Path, monkeypatch) -> None: + """The covering group ran and went red, so the path really did regress. + ``cmd_report`` is the only cross-tier evaluation, so it must gate on this — + it previously reported regressions in the PR comment while exiting 0. + """ + exit_code, states = _drive_cmd_report( + tmp_path, monkeypatch, reporting_groups={"int-g": 1} + ) + + assert states["int-path"] == "regression" + assert exit_code == 1, ( + "a regression must fail the report job, not just print into the comment" + ) From 786f7cf553d0fbedf9256e6e0a91614df49eba7c Mon Sep 17 00:00:00 2001 From: Chandrasekharan M Date: Fri, 7 Aug 2026 18:11:02 +0530 Subject: [PATCH 2/6] test(settings): share test-only deltas between the OSS and cloud suites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `copy_cloud_deps` overwrites `backend/settings/test.py` on a cloud build, so every test-only setting defined there is silently lost on that tree. That is how `MCP_PLATFORM_SERVER_ENABLED = True` failed to reach the cloud suite: the org-scoped MCP route stayed unmounted, five tests 404'd, and a sixth passed because a 404 satisfies "this credential is refused" just as well as the 401 it meant to assert. Move the deltas into `settings/test_base.py`, which both trees import: OSS test.py = base + test_base cloud test_cloud.py = cloud + test_base `test_base` deliberately imports nothing. A `from base import *` there would re-export base's names and clobber whatever the importer derived from `cloud` — which is the same failure one level up, and order cannot fix it. Also sets `INTERNAL_SERVICE_API_KEY`, which is env-driven and unset under test, so a request to an internal API fails on its own merits rather than as "not configured". `backend/backend/settings/*` is gitignored so users can drop local overrides there, with one negation per real settings module, so `test_base.py` needs its own negation to reach the repo at all. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FnH9Fx24oPA8Vzf1GS9Rkz --- .gitignore | 1 + backend/backend/settings/test.py | 20 +++++------------ backend/backend/settings/test_base.py | 32 +++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 14 deletions(-) create mode 100644 backend/backend/settings/test_base.py diff --git a/.gitignore b/.gitignore index 427afc934d..494dec71aa 100644 --- a/.gitignore +++ b/.gitignore @@ -670,6 +670,7 @@ backend/backend/settings/* !backend/backend/settings/base.py !backend/backend/settings/dev.py !backend/backend/settings/test.py +!backend/backend/settings/test_base.py # Local Dependencies for docker testing unstract/unstract-sdk/ diff --git a/backend/backend/settings/test.py b/backend/backend/settings/test.py index 04fa607682..192fefa4c3 100644 --- a/backend/backend/settings/test.py +++ b/backend/backend/settings/test.py @@ -1,16 +1,8 @@ -from backend.settings.base import * # noqa: F401, F403 - -DEBUG = True +"""OSS test settings: the production base plus the shared test-only deltas. -# Django's default PBKDF2 hasher is deliberately slow; suites that seed several -# users per test spend most of their time there. Test fixtures need speed, not -# resistance to offline cracking. -PASSWORD_HASHERS = ["django.contrib.auth.hashers.MD5PasswordHasher"] +The deltas live in `test_base` rather than here because `copy_cloud_deps` +replaces this file on a cloud build. +""" -# The organization-scoped MCP server ships disabled (see -# MCP_PLATFORM_SERVER_ENABLED in base.py), but its tests must still exercise -# it: several drive requests through the full URL stack precisely so the auth -# middleware runs, and an unmounted route would make them 404 — turning a suite -# that checks a credential is *rejected* into one that passes because nothing -# is there. Shipping-off and untested are different things. -MCP_PLATFORM_SERVER_ENABLED = True +from backend.settings.base import * # noqa: F401, F403 +from backend.settings.test_base import * # noqa: F401, F403 diff --git a/backend/backend/settings/test_base.py b/backend/backend/settings/test_base.py new file mode 100644 index 0000000000..1524adc04e --- /dev/null +++ b/backend/backend/settings/test_base.py @@ -0,0 +1,32 @@ +"""Test-only setting deltas, shared by the OSS and cloud test settings. + +This module imports nothing on purpose. `copy_cloud_deps` replaces the OSS +`settings/test.py` on a cloud build, so a delta that lives only in that file is +silently lost there; keeping the deltas here is what stops the two trees +diverging. Star-importing a base settings module would re-export its names and +clobber whatever the importer derived from `cloud`, so this file defines only +what it overrides. +""" + +DEBUG = True + +# Django's default PBKDF2 hasher is deliberately slow; suites that seed several +# users per test spend most of their time there. Test fixtures need speed, not +# resistance to offline cracking. +PASSWORD_HASHERS = ["django.contrib.auth.hashers.MD5PasswordHasher"] + +# Prod enforces HTTPS-only cookies; tests run over plain HTTP. +SESSION_COOKIE_SECURE = False +CSRF_COOKIE_SECURE = False + +# Env-driven and unset under test, which would make any request to an internal +# API fail as "not configured" rather than on its own merits. +INTERNAL_SERVICE_API_KEY = "test-internal-service-key" + +# The organization-scoped MCP server ships disabled (see +# MCP_PLATFORM_SERVER_ENABLED in base.py), but its tests must still exercise +# it: several drive requests through the full URL stack precisely so the auth +# middleware runs, and an unmounted route would make them 404 — turning a suite +# that checks a credential is *rejected* into one that passes because nothing +# is there. Shipping-off and untested are different things. +MCP_PLATFORM_SERVER_ENABLED = True From cb7ae4a0c817945d28b05bd946d7faa93e65552a Mon Sep 17 00:00:00 2001 From: Chandrasekharan M Date: Fri, 7 Aug 2026 18:49:22 +0530 Subject: [PATCH 3/6] fix(rig): gate the report on a corrupt baseline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A corrupt baseline cache leaves `previously_covered` empty, so no path can reach `state == "regression"` and the new gate passes vacuously — a required check goes green while regression detection is off, behind an advisory banner a human has to notice. `cmd_run` already flips its exit code on the same condition; mirror it here. Also corrects the comment above the gate: `cmd_run` does gate on regressions, just only on those visible within its own tier. `cmd_report` is the only place a cross-tier regression can be gated. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FnH9Fx24oPA8Vzf1GS9Rkz --- tests/rig/cli.py | 11 +++++++---- tests/rig/tests/test_cli.py | 30 +++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/tests/rig/cli.py b/tests/rig/cli.py index eba36ce8cb..37d08c0d14 100644 --- a/tests/rig/cli.py +++ b/tests/rig/cli.py @@ -398,9 +398,9 @@ def cmd_report(args: argparse.Namespace) -> int: f"(not in tests/critical_paths.yaml)", file=sys.stderr, ) - # This is the only cross-tier evaluation, so it is also the only place a - # regression can be gated on. cmd_run deliberately doesn't: a single tier - # can't tell "another tier's path went red" from "another tier didn't run". + # cmd_run gates on the regressions it can see, but only within its own tier. + # This is the only cross-tier evaluation, so it is the only place a regression + # spanning tiers can be gated on. regressions = [s for s in statuses if s.state == "regression"] if regressions: ids = ", ".join(s.path.id for s in regressions) @@ -424,7 +424,10 @@ def cmd_report(args: argparse.Namespace) -> int: return 1 cp.merge_into_baseline(statuses, baseline_path) print(f"[rig] merged into baseline: {baseline_path}") - return 1 if unknown_marker_ids or regressions else 0 + # A corrupt baseline empties previously_covered, so no path can reach + # "regression" and the gate above silently passes. Fail on it directly + # rather than leaving a required check green behind an advisory banner. + return 1 if unknown_marker_ids or regressions or baseline_corrupt else 0 def cmd_run(args: argparse.Namespace) -> int: diff --git a/tests/rig/tests/test_cli.py b/tests/rig/tests/test_cli.py index b1e972c489..8e4824185e 100644 --- a/tests/rig/tests/test_cli.py +++ b/tests/rig/tests/test_cli.py @@ -962,13 +962,15 @@ def _drive_cmd_report( monkeypatch, *, reporting_groups: dict[str, int], + baseline_blob: str = '{"covered_paths": ["int-path"]}', ) -> tuple[int, dict[str, str]]: """Drive ``cmd_report`` over one integration group covering ``int-path``. ``reporting_groups`` maps group name -> failure count for the groups that emitted junit this build. A group absent from it emitted nothing, standing - in for a tier the CI path filter skipped. Returns the exit code and each - path's final state. + in for a tier the CI path filter skipped. ``baseline_blob`` is written as the + cached baseline verbatim, so a caller can hand over an unparseable one. + Returns the exit code and each path's final state. """ from tests.rig.reporting import GroupResult @@ -1030,9 +1032,7 @@ def spy_evaluate(*args, **kwargs): reports_dir.mkdir() # Baseline says the path was green on main, which is what makes the # not-covered-now decision a regression-or-gap question at all. - (reports_dir / "previous-summary.json").write_text( - '{"covered_paths": ["int-path"]}' - ) + (reports_dir / "previous-summary.json").write_text(baseline_blob) args = cli_mod._build_parser().parse_args( [ @@ -1077,3 +1077,23 @@ def test_cmd_report_gates_on_a_real_regression(tmp_path: Path, monkeypatch) -> N assert exit_code == 1, ( "a regression must fail the report job, not just print into the comment" ) + + +def test_cmd_report_gates_on_a_corrupt_baseline(tmp_path: Path, monkeypatch) -> None: + """An unreadable baseline leaves ``previously_covered`` empty, so no path can + ever be classified as a regression and the gate above passes vacuously. The + corruption itself has to fail the job, or a required check stays green while + regression detection is off. + """ + exit_code, states = _drive_cmd_report( + tmp_path, + monkeypatch, + reporting_groups={"int-g": 1}, + baseline_blob='{"covered_paths": ["int-pat', + ) + + assert states["int-path"] == "gap", ( + "without a baseline the regression classification is unreachable, which " + "is exactly why the corrupt flag must gate on its own" + ) + assert exit_code == 1 From eaba47763f5d015ee1d2f4e90f140dc96985d0b6 Mon Sep 17 00:00:00 2001 From: Chandrasekharan M Date: Fri, 7 Aug 2026 18:52:57 +0530 Subject: [PATCH 4/6] docs(rig): tighten the comments added in this PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cut the narrative out of the settings and rig comments — keep the WHY, drop the retelling. Test docstrings lose the references to the specific PR shape that prompted them, which would not survive the next change to the tiers. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FnH9Fx24oPA8Vzf1GS9Rkz --- backend/backend/settings/test_base.py | 25 ++++++++----------------- tests/rig/cli.py | 13 +++++-------- tests/rig/tests/test_cli.py | 21 +++++++++------------ 3 files changed, 22 insertions(+), 37 deletions(-) diff --git a/backend/backend/settings/test_base.py b/backend/backend/settings/test_base.py index 1524adc04e..b4d2835642 100644 --- a/backend/backend/settings/test_base.py +++ b/backend/backend/settings/test_base.py @@ -1,32 +1,23 @@ """Test-only setting deltas, shared by the OSS and cloud test settings. -This module imports nothing on purpose. `copy_cloud_deps` replaces the OSS -`settings/test.py` on a cloud build, so a delta that lives only in that file is -silently lost there; keeping the deltas here is what stops the two trees -diverging. Star-importing a base settings module would re-export its names and -clobber whatever the importer derived from `cloud`, so this file defines only -what it overrides. +Imports nothing on purpose: `copy_cloud_deps` replaces the OSS `settings/test.py` +on a cloud build, so deltas kept only there are lost, and star-importing a base +module here would re-export its names over whatever the importer derived. """ DEBUG = True -# Django's default PBKDF2 hasher is deliberately slow; suites that seed several -# users per test spend most of their time there. Test fixtures need speed, not -# resistance to offline cracking. +# PBKDF2 is deliberately slow, and suites seeding users per test pay it repeatedly. PASSWORD_HASHERS = ["django.contrib.auth.hashers.MD5PasswordHasher"] # Prod enforces HTTPS-only cookies; tests run over plain HTTP. SESSION_COOKIE_SECURE = False CSRF_COOKIE_SECURE = False -# Env-driven and unset under test, which would make any request to an internal -# API fail as "not configured" rather than on its own merits. +# Env-driven, so unset under test: without it internal-API requests fail as +# "not configured" rather than on their own merits. INTERNAL_SERVICE_API_KEY = "test-internal-service-key" -# The organization-scoped MCP server ships disabled (see -# MCP_PLATFORM_SERVER_ENABLED in base.py), but its tests must still exercise -# it: several drive requests through the full URL stack precisely so the auth -# middleware runs, and an unmounted route would make them 404 — turning a suite -# that checks a credential is *rejected* into one that passes because nothing -# is there. Shipping-off and untested are different things. +# Ships disabled, but its tests drive the full URL stack to exercise the auth +# middleware — an unmounted route would 404 and pass them vacuously. MCP_PLATFORM_SERVER_ENABLED = True diff --git a/tests/rig/cli.py b/tests/rig/cli.py index 37d08c0d14..797a22c25c 100644 --- a/tests/rig/cli.py +++ b/tests/rig/cli.py @@ -371,11 +371,9 @@ def cmd_report(args: argparse.Namespace) -> int: print(f"[rig] {exc}", file=sys.stderr) baseline = None baseline_corrupt = True - # A tier the path filter skipped emits no junit, so its groups are absent - # here. Scoping to the groups that actually reported keeps their paths as - # gaps rather than regressions — nothing regressed, the tier never ran. A - # group that ran and went red still reports, so it stays in scope and a - # genuine regression is still caught. + # A skipped tier emits no junit, so scoping to the groups that reported keeps + # its paths as gaps rather than regressions — nothing regressed, it never ran. + # A group that ran and went red still reports, so real regressions survive. scope_groups = [r.name for r in group_results] statuses = cp.evaluate( registry, @@ -424,9 +422,8 @@ def cmd_report(args: argparse.Namespace) -> int: return 1 cp.merge_into_baseline(statuses, baseline_path) print(f"[rig] merged into baseline: {baseline_path}") - # A corrupt baseline empties previously_covered, so no path can reach - # "regression" and the gate above silently passes. Fail on it directly - # rather than leaving a required check green behind an advisory banner. + # A corrupt baseline makes "regression" unreachable, so the gate above would + # pass vacuously; it has to fail on its own. return 1 if unknown_marker_ids or regressions or baseline_corrupt else 0 diff --git a/tests/rig/tests/test_cli.py b/tests/rig/tests/test_cli.py index 8e4824185e..fd1242315b 100644 --- a/tests/rig/tests/test_cli.py +++ b/tests/rig/tests/test_cli.py @@ -1050,10 +1050,8 @@ def spy_evaluate(*args, **kwargs): def test_cmd_report_skipped_tier_is_a_gap_not_a_regression( tmp_path: Path, monkeypatch ) -> None: - """A frontend-only PR skips the integration tier, so its groups emit no - junit. Those paths must degrade to ``gap`` — nothing regressed, the tier - never ran. Without ``scope_groups`` every baseline path in a skipped tier - is reported as regressed on every such PR. + """A path filter can skip a whole tier, whose groups then emit no junit. Its + paths must degrade to ``gap``: nothing regressed, the tier never ran. """ exit_code, states = _drive_cmd_report(tmp_path, monkeypatch, reporting_groups={}) @@ -1066,8 +1064,8 @@ def test_cmd_report_skipped_tier_is_a_gap_not_a_regression( def test_cmd_report_gates_on_a_real_regression(tmp_path: Path, monkeypatch) -> None: """The covering group ran and went red, so the path really did regress. - ``cmd_report`` is the only cross-tier evaluation, so it must gate on this — - it previously reported regressions in the PR comment while exiting 0. + Being the only cross-tier evaluation, ``cmd_report`` has to gate on it rather + than just render it into the comment. """ exit_code, states = _drive_cmd_report( tmp_path, monkeypatch, reporting_groups={"int-g": 1} @@ -1080,10 +1078,9 @@ def test_cmd_report_gates_on_a_real_regression(tmp_path: Path, monkeypatch) -> N def test_cmd_report_gates_on_a_corrupt_baseline(tmp_path: Path, monkeypatch) -> None: - """An unreadable baseline leaves ``previously_covered`` empty, so no path can - ever be classified as a regression and the gate above passes vacuously. The - corruption itself has to fail the job, or a required check stays green while - regression detection is off. + """An unreadable baseline leaves ``previously_covered`` empty, making the + regression state unreachable. Without gating on the corruption itself, the + job goes green with regression detection silently off. """ exit_code, states = _drive_cmd_report( tmp_path, @@ -1093,7 +1090,7 @@ def test_cmd_report_gates_on_a_corrupt_baseline(tmp_path: Path, monkeypatch) -> ) assert states["int-path"] == "gap", ( - "without a baseline the regression classification is unreachable, which " - "is exactly why the corrupt flag must gate on its own" + "with no usable baseline the regression state is unreachable, which is " + "why the corrupt flag must gate on its own" ) assert exit_code == 1 From 3c9c31fe12aa71ef19366f9874fc778262bc5773 Mon Sep 17 00:00:00 2001 From: Chandrasekharan M Date: Fri, 7 Aug 2026 19:03:52 +0530 Subject: [PATCH 5/6] fix(rig): keep the new gate off non-blocking and unrelated failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups, all downstream of promoting the regression warning to a required-check failure. - Exclude `optional` groups from the scope the gate uses. They are documented as non-blocking and `cmd_run` honours that, but a red optional group stayed in scope here, so its baseline-covered paths became regressions and gated the build. Latent today — no critical path names an optional group — but `integration-workflow-execution` is a placeholder waiting for exactly that. - Prune ids absent from the registry when merging the baseline. The union never forgets, so a deliberately retired path had no way out of the cache; editing `critical_paths.yaml` is now what accepts a removal, and the failure message says so. - Split the regression output: a covering group that ran green without attesting (skipped or unmarked test) needs a different fix from one that went red, and the single message named neither. - Drop `INTERNAL_SERVICE_API_KEY` from the test deltas. `CustomAuthMiddleware` treats it as a blanket `X-API-Key` bypass, which would hand the suite a skeleton key past the very paths that exist to prove credentials are refused. Nothing needed it: 856 passed, 29 skipped either way. - `reporting.py`: stop claiming a cause ("that tier did not run") that junit presence cannot distinguish from a lost artifact, and partition the gap list in one pass instead of an O(n^2) scan leaning on dataclass equality. The cookie flags stay: they are a no-op against `base.py` but cloud sets both to True. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FnH9Fx24oPA8Vzf1GS9Rkz --- backend/backend/settings/test_base.py | 4 --- tests/rig/cli.py | 37 +++++++++++++++++++++--- tests/rig/critical_paths.py | 7 ++++- tests/rig/reporting.py | 16 +++++++---- tests/rig/tests/test_cli.py | 20 +++++++++++++ tests/rig/tests/test_critical_paths.py | 40 +++++++++++++++++++++----- 6 files changed, 102 insertions(+), 22 deletions(-) diff --git a/backend/backend/settings/test_base.py b/backend/backend/settings/test_base.py index b4d2835642..76af7b3323 100644 --- a/backend/backend/settings/test_base.py +++ b/backend/backend/settings/test_base.py @@ -14,10 +14,6 @@ SESSION_COOKIE_SECURE = False CSRF_COOKIE_SECURE = False -# Env-driven, so unset under test: without it internal-API requests fail as -# "not configured" rather than on their own merits. -INTERNAL_SERVICE_API_KEY = "test-internal-service-key" - # Ships disabled, but its tests drive the full URL stack to exercise the auth # middleware — an unmounted route would 404 and pass them vacuously. MCP_PLATFORM_SERVER_ENABLED = True diff --git a/tests/rig/cli.py b/tests/rig/cli.py index 797a22c25c..caeafae07a 100644 --- a/tests/rig/cli.py +++ b/tests/rig/cli.py @@ -374,7 +374,10 @@ def cmd_report(args: argparse.Namespace) -> int: # A skipped tier emits no junit, so scoping to the groups that reported keeps # its paths as gaps rather than regressions — nothing regressed, it never ran. # A group that ran and went red still reports, so real regressions survive. - scope_groups = [r.name for r in group_results] + # `optional` groups are excluded: they are documented as non-blocking, and + # leaving them in scope would let a red one gate the build through a + # regression instead. + scope_groups = [r.name for r in group_results if not manifest.get(r.name).optional] statuses = cp.evaluate( registry, groups_run_green=green, @@ -399,11 +402,37 @@ def cmd_report(args: argparse.Namespace) -> int: # cmd_run gates on the regressions it can see, but only within its own tier. # This is the only cross-tier evaluation, so it is the only place a regression # spanning tiers can be gated on. - regressions = [s for s in statuses if s.state == "regression"] + # A covering group that ran green without attesting means its marked test + # was skipped or unmarked; a group that went red means the test failed. The + # remedies differ, so the two are reported apart rather than as one count. + unproven: list[cp.CriticalPathStatus] = [] + uncovered: list[cp.CriticalPathStatus] = [] + for s in statuses: + if s.state != "regression": + continue + target = unproven if any(g in green for g in s.path.covered_by) else uncovered + target.append(s) + regressions = unproven + uncovered + if uncovered: + ids = ", ".join(s.path.id for s in uncovered) + print( + f"\n[rig] ❌ {len(uncovered)} critical-path regression(s) — no covering " + f"group ran green: {ids}", + file=sys.stderr, + ) + if unproven: + ids = ", ".join(s.path.id for s in unproven) + print( + f"\n[rig] ❌ {len(unproven)} critical-path regression(s) — covering group " + f"ran green but no passing @pytest.mark.critical_path test attested " + f"them (skipped or unmarked?): {ids}", + file=sys.stderr, + ) if regressions: - ids = ", ".join(s.path.id for s in regressions) print( - f"\n[rig] ❌ {len(regressions)} critical-path regression(s) detected: {ids}", + "[rig] to accept a deliberate removal, drop or re-point the path in " + "tests/critical_paths.yaml in the same PR — the baseline is keyed off " + "that registry.", file=sys.stderr, ) if args.update_baseline: diff --git a/tests/rig/critical_paths.py b/tests/rig/critical_paths.py index e9cb3ccbbb..c5f2f6b792 100644 --- a/tests/rig/critical_paths.py +++ b/tests/rig/critical_paths.py @@ -226,6 +226,10 @@ def merge_into_baseline(statuses: list[CriticalPathStatus], destination: Path) - treated as empty: silently dropping previously-covered paths would erase the other tier's contribution and turn the next build into a regression festival. CI should delete the cache and retry on this exception. + + Ids absent from the registry are pruned. The union alone never forgets, so a + deliberately retired path would otherwise sit in the cache forever; pruning + makes editing ``critical_paths.yaml`` the way to accept a removal. """ existing: set[str] = set() if destination.exists(): @@ -238,7 +242,8 @@ def merge_into_baseline(statuses: list[CriticalPathStatus], destination: Path) - "Delete the cache entry and re-run." ) from exc fresh = {s.path.id for s in statuses if s.state == "covered"} - payload = {"covered_paths": sorted(existing | fresh)} + known = {s.path.id for s in statuses} + payload = {"covered_paths": sorted((existing | fresh) & known)} destination.parent.mkdir(parents=True, exist_ok=True) destination.write_text(json.dumps(payload, indent=2)) diff --git a/tests/rig/reporting.py b/tests/rig/reporting.py index b1a6527cf2..897b9f895e 100644 --- a/tests/rig/reporting.py +++ b/tests/rig/reporting.py @@ -318,11 +318,15 @@ def _render_markdown( if critical_statuses: regressions = [s for s in critical_statuses if s.state == "regression"] - all_gaps = [s for s in critical_statuses if s.state == "gap"] - # A path whose covering groups exist but sat out this build is not - # "not yet covered" — reporting it as such reads as missing tests. - unexercised = [s for s in all_gaps if not s.in_scope and s.path.covered_by] - gaps = [s for s in all_gaps if s not in unexercised] + # A path whose covering groups exist but reported nothing is not "not yet + # covered" — listing it as such reads as missing tests. + unexercised: list[CriticalPathStatus] = [] + gaps: list[CriticalPathStatus] = [] + for s in critical_statuses: + if s.state != "gap": + continue + target = unexercised if not s.in_scope and s.path.covered_by else gaps + target.append(s) covered = [s for s in critical_statuses if s.state == "covered"] lines.append("## Critical paths") @@ -353,7 +357,7 @@ def _render_markdown( covers = ", ".join(s.path.covered_by) lines.append( f"- **{s.path.id}** — {s.path.description} " - f"(covered by {covers}; that tier did not run here)" + f"(covered by {covers}; no result reported in this build)" ) lines.append("") lines.append("
") diff --git a/tests/rig/tests/test_cli.py b/tests/rig/tests/test_cli.py index fd1242315b..53495a24db 100644 --- a/tests/rig/tests/test_cli.py +++ b/tests/rig/tests/test_cli.py @@ -963,6 +963,7 @@ def _drive_cmd_report( *, reporting_groups: dict[str, int], baseline_blob: str = '{"covered_paths": ["int-path"]}', + optional: bool = False, ) -> tuple[int, dict[str, str]]: """Drive ``cmd_report`` over one integration group covering ``int-path``. @@ -982,6 +983,7 @@ def _drive_cmd_report( " tier: integration\n" f" workdir: {test_dir}\n" " paths: [.]\n" + f" optional: {str(optional).lower()}\n" ) (tmp_path / "groups.yaml").write_text(manifest_yaml) (tmp_path / "critical_paths.yaml").write_text( @@ -1077,6 +1079,24 @@ def test_cmd_report_gates_on_a_real_regression(tmp_path: Path, monkeypatch) -> N ) +def test_cmd_report_does_not_gate_on_an_optional_group( + tmp_path: Path, monkeypatch +) -> None: + """``optional`` groups are documented as non-blocking, and ``cmd_run`` honours + that. A red optional group must not reach the gate here through a regression, + or the two commands disagree about the same result. + """ + exit_code, states = _drive_cmd_report( + tmp_path, monkeypatch, reporting_groups={"int-g": 1}, optional=True + ) + + assert states["int-path"] == "gap", ( + f"an optional group's red result must not read as a regression; " + f"got {states['int-path']}" + ) + assert exit_code == 0 + + def test_cmd_report_gates_on_a_corrupt_baseline(tmp_path: Path, monkeypatch) -> None: """An unreadable baseline leaves ``previously_covered`` empty, making the regression state unreachable. Without gating on the corruption itself, the diff --git a/tests/rig/tests/test_critical_paths.py b/tests/rig/tests/test_critical_paths.py index 214c958d62..845e094662 100644 --- a/tests/rig/tests/test_critical_paths.py +++ b/tests/rig/tests/test_critical_paths.py @@ -59,17 +59,20 @@ def test_regression_when_baseline_covered_but_now_not() -> None: def test_baseline_merge_unions_with_existing(tmp_path: Path) -> None: - """Two tier runs in sequence must both contribute to the baseline.""" + """Two tier runs in sequence must both contribute to the baseline. + + Every invocation reads the same registry and differs only in which groups + ran, so each one sees the other tier's paths as uncovered — the union is + what stops the second run erasing the first. + """ baseline = tmp_path / "previous-summary.json" - registry_a = _registry(("p1", ("g1",))) - statuses_a = evaluate( - registry_a, groups_run_green={"g1"}, baseline=None - ) + registry = _registry(("p1", ("g1",)), ("p2", ("g2",))) + + statuses_a = evaluate(registry, groups_run_green={"g1"}, baseline=None) merge_into_baseline(statuses_a, baseline) - registry_b = _registry(("p2", ("g2",))) statuses_b = evaluate( - registry_b, groups_run_green={"g2"}, baseline=load_baseline(baseline) + registry, groups_run_green={"g2"}, baseline=load_baseline(baseline) ) merge_into_baseline(statuses_b, baseline) @@ -77,6 +80,29 @@ def test_baseline_merge_unions_with_existing(tmp_path: Path) -> None: assert sorted(final["covered_paths"]) == ["p1", "p2"] +def test_baseline_merge_prunes_paths_dropped_from_the_registry(tmp_path: Path) -> None: + """Retiring a path must be able to clear it from the baseline. + + The union never forgets on its own, so without pruning a deliberately + removed path stays cached forever and there is no in-repo way to accept the + removal. + """ + baseline = tmp_path / "previous-summary.json" + before = _registry(("p1", ("g1",)), ("p2", ("g2",))) + merge_into_baseline( + evaluate(before, groups_run_green={"g1", "g2"}, baseline=None), baseline + ) + assert sorted((load_baseline(baseline) or {})["covered_paths"]) == ["p1", "p2"] + + after = _registry(("p1", ("g1",))) + merge_into_baseline( + evaluate(after, groups_run_green={"g1"}, baseline=load_baseline(baseline)), + baseline, + ) + + assert (load_baseline(baseline) or {})["covered_paths"] == ["p1"] + + def test_by_id_lookup_caches() -> None: registry = _registry(("p1", ("g1",)), ("p2", ())) # Two lookups must return identical instances; tests both correctness and From 68482924b9cf30e888316bcfd50d10e30d8340d0 Mon Sep 17 00:00:00 2001 From: Chandrasekharan M Date: Fri, 7 Aug 2026 19:07:41 +0530 Subject: [PATCH 6/6] fix(rig): treat a misshapen baseline as corrupt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Parseable JSON of the wrong shape — a bare array, or `covered_paths` holding a non-list or non-string elements — passed the JSON check and then surfaced as an `AttributeError`/`TypeError` from whichever caller touched it first, bypassing the handled corrupt path that the report gate depends on. One shared shape check now backs both `load_baseline` and `merge_into_baseline`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FnH9Fx24oPA8Vzf1GS9Rkz --- tests/rig/critical_paths.py | 30 ++++++++++++++++++++------ tests/rig/tests/test_critical_paths.py | 21 ++++++++++++++++++ 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/tests/rig/critical_paths.py b/tests/rig/critical_paths.py index c5f2f6b792..3b6a72c814 100644 --- a/tests/rig/critical_paths.py +++ b/tests/rig/critical_paths.py @@ -234,9 +234,8 @@ def merge_into_baseline(statuses: list[CriticalPathStatus], destination: Path) - existing: set[str] = set() if destination.exists(): try: - parsed = json.loads(destination.read_text()) - existing = set(parsed.get("covered_paths") or []) - except (json.JSONDecodeError, OSError) as exc: + existing = set(_read_baseline(destination)["covered_paths"]) + except (OSError, ValueError) as exc: raise BaselineCorruptError( f"refusing to merge into corrupt baseline {destination}: {exc}. " "Delete the cache entry and re-run." @@ -248,18 +247,35 @@ def merge_into_baseline(statuses: list[CriticalPathStatus], destination: Path) - destination.write_text(json.dumps(payload, indent=2)) +def _read_baseline(source: Path) -> dict[str, Any]: + """Parse a baseline file and check its shape. + + Well-formed JSON of the wrong shape is as unusable as a truncated file, and + would otherwise surface as an ``AttributeError``/``TypeError`` from whichever + caller touched it first. Raises ``OSError`` or ``ValueError`` + (``JSONDecodeError`` is one) for callers to translate. + """ + parsed = json.loads(source.read_text()) + if not isinstance(parsed, dict): + raise ValueError(f"expected a JSON object, got {type(parsed).__name__}") + covered = parsed.get("covered_paths") or [] + if not isinstance(covered, list) or not all(isinstance(p, str) for p in covered): + raise ValueError("'covered_paths' must be a list of strings") + return {**parsed, "covered_paths": covered} + + def load_baseline(source: Path) -> dict[str, Any] | None: """Load the cached baseline. Returns None if the file doesn't exist (first build / fresh cache). - Raises :class:`BaselineCorruptError` if the file exists but is unreadable - or unparseable — see :func:`merge_into_baseline` for the rationale. + Raises :class:`BaselineCorruptError` if the file exists but is unreadable, + unparseable, or misshapen — see :func:`merge_into_baseline` for the rationale. """ if not source.exists(): return None try: - return json.loads(source.read_text()) - except (json.JSONDecodeError, OSError) as exc: + return _read_baseline(source) + except (OSError, ValueError) as exc: raise BaselineCorruptError( f"baseline at {source} is unreadable: {exc}. " "Delete the cache entry and re-run." diff --git a/tests/rig/tests/test_critical_paths.py b/tests/rig/tests/test_critical_paths.py index 845e094662..903e7c0d5e 100644 --- a/tests/rig/tests/test_critical_paths.py +++ b/tests/rig/tests/test_critical_paths.py @@ -143,6 +143,27 @@ def test_load_baseline_raises_on_corrupt_file(tmp_path: Path) -> None: load_baseline(baseline) +@pytest.mark.parametrize( + "blob", ["[]", '{"covered_paths": 1}', '{"covered_paths": ["ok", 2]}'] +) +def test_baseline_of_the_wrong_shape_is_corrupt(tmp_path: Path, blob: str) -> None: + """Parseable JSON of the wrong shape is as unusable as a truncated file. + + Left unchecked it surfaces as an ``AttributeError``/``TypeError`` from + whichever caller touched it first, rather than the handled corrupt path. + """ + baseline = tmp_path / "previous-summary.json" + baseline.write_text(blob) + registry = _registry(("p1", ("g1",))) + + with pytest.raises(BaselineCorruptError): + load_baseline(baseline) + with pytest.raises(BaselineCorruptError): + merge_into_baseline( + evaluate(registry, groups_run_green={"g1"}, baseline=None), baseline + ) + + def test_merge_raises_on_corrupt_existing_baseline(tmp_path: Path) -> None: """merge_into_baseline must not silently overwrite a corrupt file — that would erase the other tier's previously-covered paths.