fix(e2e-gate): skip Drift Bot when unreported, raise MAX_WAIT to 3600s #5332
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ | |
|
|
||
| * an exact name (``"Syntax & Lint"``) for a single job; | ||
| * an fnmatch pattern plus ``min_count`` for a matrix job, where the pattern | ||
| expands to one check per leg (``"pip install (*)"``) -> four legs). ``min_count`` | ||
| is what stops a *shrinking* matrix from quietly passing: drop macOS from the | ||
| matrix and the pattern still matches, but the count no longer does. | ||
|
|
||
|
|
@@ -39,7 +39,7 @@ | |
| from dataclasses import dataclass, field | ||
|
|
||
| POLL_INTERVAL = 30 | ||
| DEFAULT_MAX_WAIT = 3000 # 50 min default; workflow sets 3600 (60 min) | ||
|
|
||
| # GitHub treats skipped/neutral as non-blocking; match that. | ||
| PASSING = {"success", "skipped", "neutral"} | ||
|
|
@@ -59,11 +59,19 @@ class Spec: | |
| before the spec can pass. For a single job that is 1. For a matrix it is the | ||
| number of legs, so removing a leg fails the gate instead of silently | ||
| reducing coverage. | ||
|
|
||
| ``skip_if_unreported`` marks a spec whose reporter may legitimately never | ||
| post on certain PR types (e.g. a GitHub App that only evaluates Python or | ||
| product-record changes). When True and zero check-runs/statuses have been | ||
|
There was a problem hiding this comment. The blueprint states: "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec, because a workflow that stays silent would hold the gate pending until timeout and block every merge." The code now implements skip_if_unreported=True for Drift Bot, explicitly allowing a check that does not report on all PR types (frontend-only npm bumps) to pass the gate without reporting. This contradicts the blueprint's foundational design contract. There was a problem hiding this comment. The blueprint's Key Contracts state: "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec." The code now implements skip_if_unreported=True for Drift Bot, explicitly allowing a check that does not report on all PR types to pass the gate without reporting, which contradicts this foundational contract. There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec". The code now adds a |
||
| posted for this spec, evaluate() returns "passed" (skipped) rather than | ||
| "pending". A failing status still fails the gate -- the guard is preserved | ||
| whenever the reporter does post. | ||
| """ | ||
|
|
||
| label: str | ||
| pattern: str | ||
| min_count: int = 1 | ||
| skip_if_unreported: bool = False | ||
|
There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec." The code adds a skip_if_unreported field that explicitly permits checks not reporting on all PR types to be added to the spec and pass the gate when unreported, violating this foundational contract. There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec" because unreported checks would hold the gate pending until timeout. The code adds a skip_if_unreported field that explicitly permits checks not reporting on all PR types to pass the gate when unreported, violating this foundational contract.
Comment on lines
59
to
+74
There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec". The code adds a There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec". The code adds a There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec". The code adds a skip_if_unreported field to Spec that explicitly permits checks not reporting on all PR types to pass the gate when unreported, violating this foundational contract. There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec". The code adds a skip_if_unreported field to Spec that explicitly permits checks not reporting on all PR types to pass when unreported, violating this foundational contract. There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec." The code adds a skip_if_unreported field to Spec that explicitly permits checks not reporting on all PR types to pass when unreported, violating this foundational contract. There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec." The code adds a skip_if_unreported field to Spec that explicitly permits checks not reporting on all PR types to pass when unreported, violating this foundational contract. There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec." The code adds a skip_if_unreported field to Spec that explicitly permits checks not reporting on all PR types to pass when unreported, violating this foundational contract. There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec." The code adds a skip_if_unreported field to Spec that explicitly permits checks not reporting on all PR types to pass when unreported, directly violating this foundational contract. There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec." The code adds a skip_if_unreported field to Spec that explicitly permits checks not reporting on all PR types to pass when unreported, directly violating this foundational contract. There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec." The code adds a skip_if_unreported field to Spec that explicitly allows checks not reporting on all PR types to pass when unreported, contradicting this foundational contract. There was a problem hiding this comment. The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec." The code adds a skip_if_unreported field to Spec that explicitly allows checks not reporting on all PR types to pass when unreported, directly violating this foundational contract. |
||
|
|
||
| def matches(self, name: str) -> bool: | ||
| return fnmatch.fnmatchcase(name, self.pattern) | ||
|
|
@@ -111,7 +119,14 @@ def matches(self, name: str) -> bool: | |
| # Unlike everything above, drift-bot is a COMMIT STATUS from the | ||
| # 8090-software-factory GitHub App, not an Actions check run. It never | ||
| # appears in /check-runs, which is why list_commit_statuses exists. | ||
| # | ||
| # skip_if_unreported=True: the 8090 App only posts drift-bot on PRs that | ||
| # touch Python or product-record files. Frontend-only npm bumps (dependabot | ||
|
There was a problem hiding this comment. The blueprint's Key Contracts state: "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec, because a workflow that stays silent would hold the gate pending until timeout and block every merge." The code now implements skip_if_unreported=True for Drift Bot, explicitly allowing a check that does not report on all PR types to pass the gate without reporting, which contradicts this foundational contract. |
||
| # i18next / postcss / react-router / etc.) never receive a status -- making | ||
| # this spec a guaranteed 30-min hang on every such PR. When the App does | ||
| # post (on any PR touching Python or product files), the gate still enforces | ||
| # it. Only the "never posted" case is treated as skipped. | ||
|
There was a problem hiding this comment. Drift Bot is configured with There was a problem hiding this comment. Drift Bot is configured with There was a problem hiding this comment. Drift Bot is configured with skip_if_unreported=True despite only reporting on Python/product-record changes, not all PR types. This directly violates the blueprint's Key Contract that only checks reporting on every pull request may appear in RequiredCheckSpec. There was a problem hiding this comment. Drift Bot is configured with skip_if_unreported=True despite only reporting on Python/product-record changes, not all PR types. This directly violates the blueprint's Key Contract that only checks reporting on every pull request may appear in RequiredCheckSpec. There was a problem hiding this comment. Drift Bot is configured with skip_if_unreported=True despite only reporting on Python/product-record changes, not all PR types. This directly violates the blueprint's Key Contract that only checks reporting on every pull request may appear in RequiredCheckSpec. There was a problem hiding this comment. Drift Bot is configured with skip_if_unreported=True despite only reporting on Python/product-record changes, not all PR types. This directly violates the blueprint's Key Contract that only checks reporting on every pull request may appear in RequiredCheckSpec. There was a problem hiding this comment. Drift Bot is configured with skip_if_unreported=True despite only reporting on Python/product-record changes, not all PR types. This directly violates the blueprint's Key Contract that only checks reporting on every pull request may appear in RequiredCheckSpec. There was a problem hiding this comment. Drift Bot is configured with skip_if_unreported=True despite only reporting on Python/product-record changes, not all PR types. This violates the blueprint's Key Contract that only checks reporting on every pull request may appear in RequiredCheckSpec. There was a problem hiding this comment. Drift Bot is configured with skip_if_unreported=True despite only reporting on Python/product-record changes, not all PR types. This violates the blueprint's Key Contract that only checks reporting on every pull request may appear in RequiredCheckSpec. |
||
| Spec("Drift Bot", "drift-bot", skip_if_unreported=True), | ||
|
There was a problem hiding this comment. Drift Bot is configured with There was a problem hiding this comment. Drift Bot is configured with skip_if_unreported=True despite not reporting on all PR types (only on Python/product-record changes). This directly violates the blueprint's Key Contract that only checks reporting on every pull request may appear in RequiredCheckSpec. There was a problem hiding this comment. Drift Bot is configured with skip_if_unreported=True despite not reporting on all PR types (only on Python/product-record changes). This directly violates the blueprint's Key Contract that only checks reporting on every pull request may appear in RequiredCheckSpec. There was a problem hiding this comment. Drift Bot is configured with skip_if_unreported=True despite not reporting on all PR types (only on Python/product-record changes). This directly violates the blueprint's Key Contract that only checks reporting on every pull request may appear in RequiredCheckSpec. There was a problem hiding this comment. Drift Bot is configured with skip_if_unreported=True despite only reporting on Python/product-record changes, not on all PR types. This violates the blueprint's Key Contract that only checks reporting on every pull request may appear in RequiredCheckSpec. There was a problem hiding this comment. Drift Bot is configured with skip_if_unreported=True despite only reporting on Python/product-record changes, not all PR types. This violates the blueprint's Key Contract that only checks reporting on every pull request may appear in RequiredCheckSpec. |
||
| ] | ||
|
|
||
|
|
||
|
|
@@ -166,6 +181,14 @@ def evaluate(specs, runs): | |
| for spec in specs: | ||
| matched = {n: r for n, r in best.items() if spec.matches(n)} | ||
|
|
||
| # When skip_if_unreported is set and no status has been posted at all, | ||
|
There was a problem hiding this comment. MergeGateEvaluator now implements conditional skip logic that treats unreported checks as "passed (skipped)". The blueprint documents that MergeGateEvaluator has responsibility to "resolve each entry of RequiredCheckSpec against the check runs" with no conditional skipping logic, but the code adds undocumented responsibility for handling skip_if_unreported. There was a problem hiding this comment. MergeGateEvaluator now implements conditional skip logic (skip_if_unreported) that treats unreported checks as passed. The blueprint documents that MergeGateEvaluator "resolves each entry of RequiredCheckSpec against the check runs" with no conditional skipping responsibility, adding an undocumented capability. There was a problem hiding this comment. MergeGateEvaluator now implements conditional skip logic (skip_if_unreported) that treats unreported checks as "passed (skipped)". The blueprint documents that MergeGateEvaluator's only responsibility is to "resolve each entry of RequiredCheckSpec against the check runs" with no conditional skipping, adding an undocumented capability. There was a problem hiding this comment. MergeGateEvaluator now implements conditional skip logic (skip_if_unreported) that treats unreported checks as "passed (skipped)". The blueprint documents that MergeGateEvaluator's only responsibility is to "resolve each entry of RequiredCheckSpec against the check runs" with no conditional skipping, adding an undocumented capability. There was a problem hiding this comment. MergeGateEvaluator now implements conditional skip logic (skip_if_unreported) that treats unreported checks as "passed (skipped)". The blueprint specifies MergeGateEvaluator's responsibility is to "resolve each entry of RequiredCheckSpec against the check runs" with no conditional skipping for unreported checks, adding an undocumented capability that bypasses the Key Contract. |
||
| # treat the spec as passed (skipped). A reporter that posts but fails | ||
| # is still caught below -- this only short-circuits the "never posted" | ||
| # hang that occurs when a GitHub App does not evaluate this PR type. | ||
| if spec.skip_if_unreported and not matched: | ||
| results.append(SpecResult(spec, "passed", "no status reported, treated as skipped")) | ||
|
Comment on lines
181
to
+189
There was a problem hiding this comment. The blueprint documents that MergeGateEvaluator "resolves each entry of RequiredCheckSpec against the check runs" and includes no responsibility for conditionally skipping checks. The code now implements conditional skip logic that treats unreported checks as passed, which adds an undocumented responsibility to the evaluator.
Comment on lines
181
to
+189
There was a problem hiding this comment. The blueprint documents that MergeGateEvaluator "resolves each entry of RequiredCheckSpec against the check runs" with no responsibility for conditionally skipping checks. The code now implements conditional skip logic (skip_if_unreported) that treats unreported checks as passed, adding an undocumented responsibility to the evaluator.
Comment on lines
+184
to
+189
There was a problem hiding this comment. MergeGateEvaluator now implements conditional skip logic (skip_if_unreported) that treats unreported checks as "passed (skipped)". The blueprint documents that MergeGateEvaluator's only responsibility is to "resolve each entry of RequiredCheckSpec against the check runs" with no conditional skipping, adding an undocumented capability.
Comment on lines
+184
to
+189
There was a problem hiding this comment. MergeGateEvaluator now implements conditional skip logic (skip_if_unreported) that treats unreported checks as passed. The blueprint documents that MergeGateEvaluator's responsibility is to "resolve each entry of RequiredCheckSpec against the check runs" with no conditional skipping, adding an undocumented capability.
Comment on lines
+184
to
+189
There was a problem hiding this comment. MergeGateEvaluator now implements conditional skip logic (skip_if_unreported) that treats unreported checks as "passed (skipped)". The blueprint documents that MergeGateEvaluator's only responsibility is to "resolve each entry of RequiredCheckSpec against the check runs" with no conditional skipping, adding an undocumented capability. |
||
| continue | ||
|
|
||
| failing = [ | ||
| run | ||
| for run in matched.values() | ||
|
|
@@ -312,7 +335,8 @@ def main(): | |
| if args.list: | ||
| for spec in REQUIRED_SPECS: | ||
| legs = f" x{spec.min_count}" if spec.min_count > 1 else "" | ||
| suffix = " [skip_if_unreported]" if spec.skip_if_unreported else "" | ||
| print(f"{spec.label}{legs}: {spec.pattern}{suffix}") | ||
| return 0 | ||
|
|
||
| token = os.environ.get("GITHUB_TOKEN", "") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -175,9 +175,13 @@ def test_a_failed_drift_bot_fails_the_gate(): | |
| assert result.state == "failed" | ||
|
|
||
|
|
||
| def test_a_missing_drift_bot_skips_not_blocks(): | ||
|
There was a problem hiding this comment. The test named test_a_missing_drift_bot_does_not_pass_the_gate is renamed to test_a_missing_drift_bot_skips_not_blocks with its expected behavior changed from "pending" (blocked) to "passed" (skipped), formalizing a departure from the blueprint's Key Contract that missing checks block the gate. |
||
| """When skip_if_unreported=True and no status has been posted, the spec | ||
| passes as 'skipped' so frontend-only PRs are not held waiting forever. | ||
| A real failure (status posted + failing) still blocks -- see the test above.""" | ||
| gate = _gate() | ||
| spec = next(s for s in gate.REQUIRED_SPECS if s.pattern == "drift-bot") | ||
| assert spec.skip_if_unreported, "Drift Bot spec must carry skip_if_unreported=True" | ||
| (result,) = gate.evaluate([spec], []) | ||
| assert result.state == "passed" | ||
| assert "skipped" in result.detail | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The blueprint's Key Contract states "Only a check whose workflow reports on every pull request may appear in RequiredCheckSpec". The code now adds a skip_if_unreported field that explicitly permits checks not reporting on all PR types to pass the gate, violating this foundational contract.