freshness gate: overdue review is advisory, not a fleet-wide hard block - #2833
Conversation
A registry review whose review_by date simply passed (today it went overdue by one day) flipped tools/check_model_registry_freshness.py to a non-zero exit, which a unit test asserts must be zero — so the required `summary` check failed on EVERY open PR fleet-wide, none of them related to model selection. A "review is due" signal is not a "this work is dangerous" signal and must not gate unrelated work. maint-77's scheduled path was already written to never fail (it opens a tracking issue); a unit test quietly re-introduced the hard block. - check_model_registry_freshness.py: partition_findings() splits findings into BLOCKING (structural — malformed registry, absent/blocked model, approved selection with no passing evidence) and ADVISORY (cadence — review_overdue / provisional_overdue / selection_review_overdue). The default gate now exits non-zero only on blocking findings; --strict restores fail-on-any for callers gating a model-config change. JSON output gains ok/blocking/advisory alongside the existing fresh/findings keys. - maint-77: only structural findings fail a model-config PR; the scheduled run still opens a non-blocking tracking issue for advisory staleness (new has_advisory output). - tests + MODEL_SELECTION_POLICY.md updated for the advisory/blocking split. Effect: the provisional incumbents remain the runtime baseline, the overdue review is still surfaced (as a tracking issue), and a date rollover can no longer redden the whole fleet. Verified on the live config: default exit 0 (4 advisory, 0 blocking), --strict exit 1; 26 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
GitHub Actions
Inactive
📝 WalkthroughWalkthroughThe freshness gate now separates advisory overdue reviews from blocking structural findings, adds strict mode, updates tests and policy documentation, and propagates advisory status through the maintenance workflow for scheduled tracking-issue updates. ChangesFreshness finding semantics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FreshnessGate
participant FreshnessJob
participant TrackingIssueJob
FreshnessGate->>FreshnessJob: writes rc and has_advisory outputs
FreshnessJob->>TrackingIssueJob: passes structural and advisory status
TrackingIssueJob->>TrackingIssueJob: updates evidence-review issue when advisory status is true
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Workflow source neededPR #2833 needs either a linked GitHub issue or one valid non-issue Workflow Source before PR metadata automation can manage it safely. Please do one of:
Once a valid source is present, this warning will not be reposted. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce3c3a0d35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "review_overdue", | ||
| "provisional_overdue", | ||
| "selection_review_overdue", |
There was a problem hiding this comment.
When the registry date is missing or unparseable, _review_date emits review_overdue; a missing or invalid selection date similarly emits selection_review_overdue. Classifying these kinds wholesale as advisory therefore makes malformed model configuration exit 0 under the default gate, allowing a model-config PR with no valid review deadline to pass. Reserve these advisory kinds for successfully parsed dates that have merely elapsed, or emit distinct blocking kinds for missing/invalid dates.
Useful? React with 👍 / 👎.
Automated Status SummaryHead SHA: d11d5d3
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Low Coverage Files (<50.0%)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeNo scope information available Tasks
Acceptance criteria
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.github/workflows/maint-77-model-registry-freshness.yml:
- Around line 75-81: Add the freshness tool’s --strict flag to the pull-request
gate step that produces steps.gate.outputs.rc, ensuring advisory findings fail
model-config PRs. Leave the scheduled tracking-issue invocation non-strict, and
preserve the existing structural-finding failure behavior in “Fail PRs on
structural findings.”
In `@tests/test_check_model_registry_freshness.py`:
- Around line 322-332: Extend the freshness-gate tests around gate.main to
invoke --json and assert the response contract fields ok, blocking, and
advisory. Add cases covering provisional_overdue and selection_review_overdue in
addition to review_overdue, verifying their advisory behavior by default and
blocking behavior under --strict where applicable, while preserving the existing
structural finding assertion.
In `@tools/check_model_registry_freshness.py`:
- Around line 40-55: Update _review_date to emit distinct structural finding
kinds for unparseable dates and missing review_by/as_of values, while reserving
review_overdue for valid dates that have elapsed. Keep only genuinely temporal
overdue findings in ADVISORY_FINDING_KINDS so partition_findings treats
malformed or incomplete registries as blocking, and add regression tests
covering both invalid and missing date cases.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6d0c511-b60a-4bdc-92b8-47f220e1982c
📒 Files selected for processing (4)
.github/workflows/maint-77-model-registry-freshness.ymldocs/MODEL_SELECTION_POLICY.mdtests/test_check_model_registry_freshness.pytools/check_model_registry_freshness.py
| # Only structural/dangerous findings fail a model-config PR. An overdue | ||
| # review is advisory and is surfaced by the scheduled tracking issue. | ||
| - name: Fail PRs on structural findings | ||
| if: github.event_name == 'pull_request' && steps.gate.outputs.rc == '1' | ||
| run: | | ||
| echo "::error::Model registry/slots are stale — see job summary." | ||
| echo "::error::Model registry/slots have a structural problem — see job summary." | ||
| exit 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
echo "== Workflow file outline =="
fd -a 'maint-77-model-registry-freshness.yml' .github templates || true
echo "== Search for strict usage and model-config path checks =="
rg -n --hidden --glob '.github/workflows/*.yml' --glob '.github/workflows/*.yaml' --glob 'templates/consumer-repo/.github/workflows/*.yml' --glob 'templates/consumer-repo/.github/workflows/*.yaml' -- '--strict|model configuration|model-config|registry/slots|structural findings|advisory' .
echo "== Read target workflow around relevant lines =="
file=".github/workflows/maint-77-model-registry-freshness.yml"
if [ -f "$file" ]; then
nl -ba "$file" | sed -n '1,140p'
fi
echo "== Read template counterpart if present =="
tfile="templates/consumer-repo/.github/workflows/maint-77-model-registry-freshness.yml"
if [ -f "$tfile" ]; then
nl -ba "$tfile" | sed -n '1,140p'
fiRepository: stranske/Workflows
Length of output: 2909
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/maint-77-model-registry-freshness.yml')
print(p.exists())
if p.exists():
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if '--strict' in line or 'gate' in line or 'pull_request' in line or 'model' in line:
print(f'{i}: {line}')
PYRepository: stranske/Workflows
Length of output: 2002
🏁 Script executed:
set -euo pipefail
echo "== template counterpart presence =="
fd -a 'maint-77-model-registry-freshness.yml' templates .github || true
echo "== workflow snippet with line numbers =="
sed -n '45,90p' .github/workflows/maint-77-model-registry-freshness.yml | cat -n
echo "== strict support in freshness tool =="
rg -n --hidden --glob 'tools/check_model_registry_freshness.py' -- '--strict|argparse|add_argument|rc|advisory|structural' tools/check_model_registry_freshness.py
echo "== tool file outline =="
python3 - <<'PY'
from pathlib import Path
p = Path('tools/check_model_registry_freshness.py')
print("exists:", p.exists())
if p.exists():
for i, line in enumerate(p.read_text().splitlines(), 1):
if any(k in line for k in ['--strict','argparse','add_argument','rc','advisory','structural','parser']):
print(f'{i}: {line}')
PYRepository: stranske/Workflows
Length of output: 9231
🏁 Script executed:
set -euo pipefail
rg -n --hidden --glob '.github/workflows/*' --glob 'templates/consumer-repo/.github/workflows/*' -- 'check_model_registry_freshness\.py|--strict' .Repository: stranske/Workflows
Length of output: 847
Add --strict to 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/maint-77-model-registry-freshness.yml around lines 75 -
81, Add the freshness tool’s --strict flag to the pull-request gate step that
produces steps.gate.outputs.rc, ensuring advisory findings fail model-config
PRs. Leave the scheduled tracking-issue invocation non-strict, and preserve the
existing structural-finding failure behavior in “Fail PRs on structural
findings.”
Source: Path instructions
| # 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 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add coverage for the new JSON and finding-category contract.
This test covers exit codes, but never invokes --json or asserts ok, blocking, and advisory. It also exercises only review_overdue; regressions affecting provisional_overdue or selection_review_overdue would pass unnoticed.
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
Context: json.dumps(_registry(review_by="2026-07-01"))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 330-330: use jsonify instead of json.dumps for JSON output
Context: json.dumps(structural)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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/test_check_model_registry_freshness.py` around lines 322 - 332, Extend
the freshness-gate tests around gate.main to invoke --json and assert the
response contract fields ok, blocking, and advisory. Add cases covering
provisional_overdue and selection_review_overdue in addition to review_overdue,
verifying their advisory behavior by default and blocking behavior under
--strict where applicable, while preserving the existing structural finding
assertion.
Source: Path instructions
| ADVISORY_FINDING_KINDS = frozenset( | ||
| { | ||
| "review_overdue", | ||
| "provisional_overdue", | ||
| "selection_review_overdue", | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| 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 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not classify malformed review dates as advisory.
_review_date uses review_overdue for unparseable dates and missing review_by/as_of values at Lines [87-95]. Adding that kind to ADVISORY_FINDING_KINDS makes malformed or incomplete registries return ok: true and exit successfully, contradicting the blocking contract for malformed registries.
Use distinct structural finding kinds for invalid/missing dates and reserve review_overdue for a valid date that has merely elapsed. Add regression tests for both cases.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ADVISORY_FINDING_KINDS = frozenset( | |
| { | |
| "review_overdue", | |
| "provisional_overdue", | |
| "selection_review_overdue", | |
| } | |
| ) | |
| 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 | |
| ADVISORY_FINDING_KINDS = frozenset( | |
| { | |
| "registry_invalid", | |
| "provisional_overdue", | |
| "selection_review_overdue", | |
| } | |
| ) | |
| 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 |
🤖 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 `@tools/check_model_registry_freshness.py` around lines 40 - 55, Update
_review_date to emit distinct structural finding kinds for unparseable dates and
missing review_by/as_of values, while reserving review_overdue for valid dates
that have elapsed. Keep only genuinely temporal overdue findings in
ADVISORY_FINDING_KINDS so partition_findings treats malformed or incomplete
registries as blocking, and add regression tests covering both invalid and
missing date cases.
Source: Path instructions
1 participant
The problem (fleet-wide red main)
As of 2026-07-25 the registry review went overdue by one day (
review_by=2026-07-24). That flippedtools/check_model_registry_freshness.pyto a non-zero exit, and a unit test (test_direct_script_execution_uses_shared_default_profile) asserts it must be zero — so the requiredsummarycheck failed on every open PR in the repo, none related to model selection.The incumbents are all
provisionaland working fine as the runtime baseline. Nothing is broken. A "a review is due" signal is not a "this work is dangerous" signal, and it must not gate unrelated work. The tell that this is a defect:maint-77's scheduled path is explicitly written to never fail — it opens a tracking issue instead. A unit test quietly re-introduced the hard block through the pytest suite.The fix — advisory vs. blocking findings
check_model_registry_freshness.pynow splits findings:review_overdue,provisional_overdue,selection_review_overdue. → reported and surfaced, never fails the default gate.The default gate exits non-zero only on blocking findings.
--strictrestores fail-on-any for the narrow case of gating a PR that itself edits model config. JSON output gainsok/blocking/advisoryalongside the existingfresh/findings.maint-77: only structural findings fail a model-config PR; the scheduled run still opens a non-blocking tracking issue for advisory staleness (newhas_advisoryoutput), so the overdue review is never silently dropped — just never blocks.Why this is the right shape
Verification (local, CI-pinned)
--strictexit 1.maint-77YAML valid.--strictand structural findings still fail.Unblocks #2831 and #2832 (and the rest of the open PRs). Follow-up context: the evaluation pilot that would supply real review evidence is exactly what #2831/#2832 enable.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation