-
Notifications
You must be signed in to change notification settings - Fork 1
freshness gate: overdue review is advisory, not a fleet-wide hard block #2833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -319,7 +319,16 @@ def test_main_exit_codes(tmp_path: Path): | |
| "2026-07-10", | ||
| ] | ||
| assert gate.main(common) == 0 | ||
| # An overdue review is advisory: it must NOT fail the default gate (so it | ||
| # cannot block unrelated fleet-wide work)... | ||
| registry_path.write_text(json.dumps(_registry(review_by="2026-07-01")), encoding="utf-8") | ||
| assert gate.main(common) == 0 | ||
| # ...but --strict still fails on it, for callers gating a model-config change. | ||
| assert gate.main([*common, "--strict"]) == 1 | ||
| # A structural finding (selection references an absent model) always blocks. | ||
| structural = _registry() | ||
| structural["selections"][0]["model_id"] = "missing" | ||
| registry_path.write_text(json.dumps(structural), encoding="utf-8") | ||
| assert gate.main(common) == 1 | ||
|
Comment on lines
+322
to
332
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win Add coverage for the new JSON and finding-category contract. This test covers exit codes, but never invokes As per path instructions, changed Python behavior must have accompanying test coverage. 🧰 Tools🪛 ast-grep (0.44.1)[info] 323-323: use jsonify instead of json.dumps for JSON output (use-jsonify) [info] 330-330: use jsonify instead of json.dumps for JSON output (use-jsonify) 🤖 Prompt for AI AgentsSource: Path instructions |
||
| assert gate.main([*common[:-1], "not-a-date"]) == 2 | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -30,6 +30,30 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DEFAULT_MAX_AGE_DAYS = 30 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| VALID_SELECTION_STATUSES = {"provisional", "approved"} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Time-cadence findings mean "a review is due", NOT "this work is dangerous". | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # They are advisory: reported and surfaced as a tracking issue, but they never | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # fail the gate (and so never block unrelated fleet-wide work). Only structural | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # findings — a malformed registry, an absent/blocked model, an APPROVED | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # selection with no passing evidence — indicate work could be wrong, and those | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # still block. Use --strict to fail on any finding (e.g. gating a PR that itself | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # edits model config). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ADVISORY_FINDING_KINDS = frozenset( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "review_overdue", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "provisional_overdue", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "selection_review_overdue", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+42
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the registry date is missing or unparseable, Useful? React with 👍 / 👎. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def partition_findings( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| findings: list[dict[str, str]], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> tuple[list[dict[str, str]], list[dict[str, str]]]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Split findings into (blocking, advisory).""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| advisory = [f for f in findings if f.get("kind") in ADVISORY_FINDING_KINDS] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| blocking = [f for f in findings if f.get("kind") not in ADVISORY_FINDING_KINDS] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return blocking, advisory | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+40
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Do not classify malformed review dates as advisory.
Use distinct structural finding kinds for invalid/missing dates and reserve As per path instructions, changed Python behavior must prioritize correctness and test coverage. Proposed fix- findings.append(_finding("review_overdue", f"unparseable registry date: {exc}"))
+ findings.append(_finding("registry_invalid", f"unparseable registry date: {exc}"))
...
- "review_overdue",
+ "registry_invalid",📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Path instructions |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _normalize_provider(provider: str) -> str: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| normalized = (provider or "").strip().lower() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -403,6 +427,11 @@ def main(argv: list[str] | None = None) -> int: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parser.add_argument("--max-age-days", type=int, default=DEFAULT_MAX_AGE_DAYS) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parser.add_argument("--today", type=str, default=None) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parser.add_argument("--json", action="store_true") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parser.add_argument( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "--strict", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| action="store_true", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| help="Fail (exit 1) on ANY finding, including advisory cadence findings.", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| args = parser.parse_args(argv) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -421,15 +450,34 @@ def main(argv: list[str] | None = None) -> int: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| max_age_days=args.max_age_days, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| policy=policy, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| blocking, advisory = partition_findings(findings) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if args.json: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print(json.dumps({"fresh": not findings, "findings": findings}, indent=2)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| json.dumps( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "fresh": not findings, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "ok": not blocking, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "blocking": blocking, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "advisory": advisory, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "findings": findings, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| indent=2, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif findings: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print(f"Model registry freshness: {len(findings)} finding(s):") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for finding in findings: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print(f" [{finding['kind']}] {finding['detail']}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print(f"Model registry freshness: {len(blocking)} blocking, {len(advisory)} advisory:") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for finding in blocking: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print(f" [BLOCK] [{finding['kind']}] {finding['detail']}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for finding in advisory: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print(f" [advisory] [{finding['kind']}] {finding['detail']}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print("Model registry is fresh: decisions, evidence, and slots are consistent.") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 1 if findings else 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if args.strict: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 1 if findings else 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Default: only structural/dangerous findings fail the gate. A merely-overdue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # review is advisory and must never block unrelated work. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 1 if blocking else 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if __name__ == "__main__": # pragma: no cover | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: stranske/Workflows
Length of output: 2909
🏁 Script executed:
Repository: stranske/Workflows
Length of output: 2002
🏁 Script executed:
Repository: stranske/Workflows
Length of output: 9231
🏁 Script executed:
Repository: stranske/Workflows
Length of output: 847
Add
--strictto the PR gate step. The freshness tool already supports strict mode, but this PR path still runs it without that flag, so advisory-only findings won’t fail model-config PRs. Keep the scheduled tracking-issue path non-strict.🤖 Prompt for AI Agents
Source: Path instructions