UN-3638 [MISC] Scope and gate critical-path regressions in the rig report - #2232
Merged
Conversation
`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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UpQ4upCajYZKzTFLL23cT1
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe report command now scopes critical-path evaluation to reported groups, distinguishes skipped paths from regressions, prunes stale baseline paths, and validates corrupt baselines. Shared backend test settings now reside in a tracked module. ChangesCritical-path reporting
Shared test settings
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant cmd_report
participant CriticalPathEvaluator
participant CriticalPathReporter
cmd_report->>CriticalPathEvaluator: evaluate reported non-optional groups
CriticalPathEvaluator-->>cmd_report: return gaps and regressions
cmd_report->>CriticalPathReporter: render path states
CriticalPathReporter-->>cmd_report: return Markdown report
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chandrasekharan-zipstack
changed the title
chandrasekharan-zipstack
marked this pull request as ready for review
August 7, 2026 12:32
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/rig/tests/test_cli.py (1)
1050-1079: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the report output contract.
These tests verify path state and exit status only. They do not verify that
summary.mdmoves skipped-tier paths out ofCritical paths not yet covered. They also do not verify that the regression diagnostic includes the path ID.Read
summary.mdin the skipped-tier test. Capturestderrin the regression test. Assert the skipped-tier section andint-pathdiagnostic output.Proposed test additions
-def test_cmd_report_skipped_tier_is_a_gap_not_a_regression( - tmp_path: Path, monkeypatch +def test_cmd_report_skipped_tier_is_a_gap_not_a_regression( + tmp_path: Path, monkeypatch, capsys ) -> None: @@ assert exit_code == 0 + summary = (tmp_path / "reports" / "summary.md").read_text() + assert "### ⚠️ Critical paths not yet covered" not in summary + assert "💤 Covered, but not exercised in this build" in summary + assert "**int-path**" in summary @@ -def test_cmd_report_gates_on_a_real_regression(tmp_path: Path, monkeypatch) -> None: +def test_cmd_report_gates_on_a_real_regression(tmp_path: Path, monkeypatch, capsys) -> None: @@ assert exit_code == 1, ( "a regression must fail the report job, not just print into the comment" ) + assert "critical-path regression(s) detected: int-path" in capsys.readouterr().err🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/rig/tests/test_cli.py` around lines 1050 - 1079, Extend test_cmd_report_skipped_tier_is_a_gap_not_a_regression to read summary.md and assert int-path is excluded from “Critical paths not yet covered” and appears in the skipped-tier section. Update test_cmd_report_gates_on_a_real_regression to capture stderr and assert the regression diagnostic includes the path ID “int-path”, while preserving the existing state and exit-code assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/rig/tests/test_cli.py`:
- Around line 1050-1079: Extend
test_cmd_report_skipped_tier_is_a_gap_not_a_regression to read summary.md and
assert int-path is excluded from “Critical paths not yet covered” and appears in
the skipped-tier section. Update test_cmd_report_gates_on_a_real_regression to
capture stderr and assert the regression diagnostic includes the path ID
“int-path”, while preserving the existing state and exit-code assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f3c8e517-33be-4dae-a0f4-3f902a459dd5
📒 Files selected for processing (3)
tests/rig/cli.pytests/rig/reporting.pytests/rig/tests/test_cli.py
Contributor
|
| Filename | Overview |
|---|---|
| tests/rig/cli.py | Scopes combined-report evaluation to reporting non-optional groups and gates the command on regressions, unknown markers, and corrupt baselines. |
| tests/rig/critical_paths.py | Centralizes baseline shape validation and removes identifiers no longer present in the critical-path registry during baseline updates. |
| tests/rig/reporting.py | Separates declared-but-unexercised paths from paths that have no established coverage. |
| backend/backend/settings/test_base.py | Defines shared test-only settings overrides for OSS and downstream cloud test configurations. |
| tests/rig/tests/test_cli.py | Adds end-to-end report-command coverage for skipped tiers, genuine regressions, optional groups, and corrupt baselines. |
| tests/rig/tests/test_critical_paths.py | Covers registry pruning and malformed baseline shapes while preserving cross-tier baseline unions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Test groups execute] --> B[Collect JUnit group results]
B --> C[Exclude skipped and optional groups from report scope]
C --> D[Evaluate critical paths against baseline]
D --> E{Path state}
E -->|Covered| F[Covered section]
E -->|Out of scope with declared group| G[Not exercised section]
E -->|Uncovered and in scope| H{Previously covered?}
H -->|No| I[Coverage gap section]
H -->|Yes| J[Regression section]
J --> K[Fail report job]
D --> L{Baseline corrupt?}
L -->|Yes| K
Reviews (7): Last reviewed commit: "Merge branch 'main' into fix/rig-report-..." | Re-trigger Greptile
`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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FnH9Fx24oPA8Vzf1GS9Rkz
Deepak-Kesavan
left a comment
Deepak-Kesavan
left a comment
Contributor
There was a problem hiding this comment.
🤖 Automated review
Automated review by Unstract PR review kit (Claude Code). Each finding below was reproduced against the code rather than inferred, so treat it as something to resolve before merge. If one is wrong, disagree on the thread and close it — that is the expected way to clear a finding. Anything tagged [unverified] was not reproduced and is flagged for your judgement instead.
2 inline comment(s).
review-pr-bot:review
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FnH9Fx24oPA8Vzf1GS9Rkz
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FnH9Fx24oPA8Vzf1GS9Rkz
pk-zipstack
reviewed
Aug 7, 2026
pk-zipstack
left a comment
pk-zipstack
left a comment
Contributor
There was a problem hiding this comment.
Careful pass over both commits. The scoping fix reads correct — cmd_run already did exactly this and cmd_report was the odd one out — and the two new tests genuinely drive the changed branch rather than passing vacuously.
My concerns are all downstream of the gate. Promoting a printed warning to a required-check failure changes the blast radius of several behaviours that were previously only noisy, and two of them are documented contracts that now quietly don't hold. Plus the settings commit's fix isn't reachable from the cloud tree yet, which interacts badly with the gate.
7 comments, roughly in severity order. Nothing here disputes the diagnosis in the description — it's about what else becomes blocking once this is in.
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FnH9Fx24oPA8Vzf1GS9Rkz
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/rig/critical_paths.py`:
- Around line 245-246: Update load_baseline() and merge_into_baseline() to use
one shared validator that requires the decoded baseline to be a mapping with
covered_paths as a list of strings, raising BaselineCorruptError for invalid
JSON shapes or element types. Ensure cmd_report() continues returning its
handled failure status, and add coverage for a JSON array and an invalid
covered_paths type.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aa526184-9266-40cf-b400-07c27b7e4796
📒 Files selected for processing (6)
backend/backend/settings/test_base.pytests/rig/cli.pytests/rig/critical_paths.pytests/rig/reporting.pytests/rig/tests/test_cli.pytests/rig/tests/test_critical_paths.py
💤 Files with no reviewable changes (1)
- backend/backend/settings/test_base.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/rig/reporting.py
- tests/rig/cli.py
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FnH9Fx24oPA8Vzf1GS9Rkz
pk-zipstack
approved these changes
Aug 7, 2026
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
scope_groupstoevaluate()incmd_report, derived from the groups that actually emitted junit.Why
Spotted on #2230, a frontend-only PR. Its report comment listed nine critical paths under ❌ Regressions (must be zero) —
adapter-register-llm,workflow-author,api-deployment-provision,api-deployment-auth,mcp-server-auth,mcp-platform-auth,prompt-studio-author,connector-register-test,usage-aggregate-read— while every check on the PR was green.Two independent defects, which masked each other.
1. False positives. All nine paths are covered solely by
integration-backend. Thechangesfilter marksfrontend/**as notrelevant, so thetestjob (unit + integration) skips and emits no junit.cmd_reportcalledevaluate()withoutscope_groups, which the docstring documents as "no scoping is applied (back-compat)", makingin_scopeunconditionally true. Every one of those paths was in main's baseline and not covered in this build, so:cmd_runalready passesscope_groupsand degrades out-of-tier paths togap.cmd_reportnever got the same treatment, even though the workflow comment names it "the sole regression authority".2. No gate.
cmd_report's only exit path wasreturn 1 if unknown_marker_ids else 0— it rendered "must be zero" into the comment and exited 0 regardless. Thereportjob's fail step only inspectsneeds.*.result, and a path-filtered skip counts as a pass, so nothing failed.Fixing either alone is wrong: gate-only turns every frontend PR red, scope-only leaves genuine regressions unenforced.
The practical cost today is a comment that cries wolf under a heading reading "must be zero", which trains reviewers to scroll past the section that is supposed to stop a merge.
How
scope_groups = [r.name for r in group_results]— a skipped tier contributes no groups, so its paths fall togap. A group that ran and went red still reports, stays in scope, and is still classified as a regression.regressionsfrom the evaluated statuses, print the ids to stderr, and fold them into the return code.reporting.pysplits out-of-scope gaps that have declared covering groups into a collapsed "💤 Covered, but not exercised in this build" section. Paths with no declared groups (e.g.workflow-execution-fan-out) stay under "not yet covered", where they belong.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
It changes when a required check fails, so it is worth reviewing on that basis rather than as a no-op.
scope_groupscovers all of them andin_scopematches the previous unconditionalTrue. Only builds with a skipped tier change classification.mainalways runs both tiers.report, where it previously only printed. If main's baseline is currently out of step with reality, the first build after this merges will surface it — that is the intended effect, but it is the reason to land this deliberately rather than fold it into an unrelated PR._coverage_attesting_groupsexcludesempty), yet it does report, so it stays in scope. Its baseline-covered paths will now be regressions and will fail the job. That is the correct reading of a group that silently stopped collecting, andcmd_runalready fails empty non-optional groups, but it is a new failure mode for the report job.previous-summary.jsonemptiespreviously_covered, which makes theregressionstate unreachable and would have let the new gate pass vacuously.cmd_reportnow flips its exit code onbaseline_corrupt, matchingcmd_run. A missing baseline still exits 0, so first runs and post-eviction runs are unaffected — only a file that exists and is unreadable fails.@pytest.mark.critical_pathtests attest, so an env-dependentskipTestleaves its group green and in scope while attesting nothing — which reads as a regression. Raised in review; kept as a failure (a silently-skipped critical-path test should shout), but the stderr line now distinguishes "covering group ran green but nothing attested" from "no covering group ran green", since the fixes differ.optionalgroups are excluded from the gate's scope. They are documented as non-blocking andcmd_runhonours that; leaving them in scope let a red optional group gate the build through a regression instead. Latent today (no critical path names one), fixed before it isn't.critical_paths.yamlis what accepts a removal, and the failure message says so.--update-baselineremains main-only and green-only, so no PR can poison it.Database Migrations
Env Config
Related Issues or PRs
Notes on Testing
tests/rig/tests/test_cli.pydrivecmd_reportend to end:test_cmd_report_skipped_tier_is_a_gap_not_a_regression(covering group emits no junit →gap, exit 0) andtest_cmd_report_gates_on_a_real_regression(covering group reports red →regression, exit 1).cli.pyand pass with it, so they exercise the changed branch rather than passing vacuously.tests/rig/**flipsrelevantto true, so the integration tier runs and those paths are attested normally. The live confirmation is UN-3770 [MISC] Remove noisy tooltips from the resource list table #2230's next run once this lands — its nine paths should move out of the regression section.Checklist
I have read and understood the Contribution Guidelines.