chore(model): restore registry-only scope - #8452
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 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 |
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review
- Head reviewed:
f0d58304e24c3fc85b5d0deb88754f45e63690c2 - Base reviewed:
origin/main=5f476625eba9c4ae2fa5770781852b78c865fd8b - Intended cleanup: remove the 12-line predictive registry test that was added beyond #8427's requested registry-only scope in merged #8438.
- Scope check: the head is one commit; its diff contains only
tests/unit/test_registry.pywith exactly 12 deletions and no additions. - Registry preservation:
model/registry.yamlis unchanged relative to currentorigin/main, which retains the exact two requested cache-path substitutions and all provenance fields. - Validation:
tests/unit/test_registry.pypassed 34 tests; Ruff andgit diff --checkpassed; the final readiness functional lanes passed and recorded a clean committed-head stamp. - Readiness caveat: the wrapper reported a shared-host orphaned pytest worker from another registered worktree (issue #5594); no unrelated worker was terminated.
- Findings disposition: no blocking implementation finding remains; no benchmark, planner-quality, metric, or research claim is made.
- Single-account waiver: this is the authenticated maintainer's exact-head implementation review; no independent review identity is available for this repository.
gate-verdict: accepted @ f0d5830
pr-metadata: reconciled @ 78592945bf5198bad0a5a69561a5ab1301d4f8f6673df931cc0a92b5b9d906d0
🔍 PR Contract Check SummaryOverall Status: 🟢 PASSED
ℹ️ Info
This check is mechanized. Please resolve any blockers to pass CI. |
## Summary This successor closes the remaining boundedness gap in the CI `ci_uv_sync_diag` probe. It removes the unbounded `uv cache size` traversal and makes both advisory `du` probes deadline-aware and diagnostically explicit. ## Linked Issues - Relates to #8249 (successor follow-up to merged PR #8365; the issue is reopened for this work). ## Stack / Dependency - Base dependency: none. - Required prior PRs and stack follow-up issues: none. - Safe to review independently: yes; the branch is based on current `main`. ## What Changed - Removed the redundant `uv cache size` cache-root traversal. - Require a positive `ROBOT_SF_DIAG_DU_TIMEOUT_SECONDS` value, defaulting invalid values (including zero) to 10 seconds. - Detect GNU `timeout(1)` explicitly and use `--kill-after=2s` after the configured deadline. - Emit `ok`, `timed-out`, or `error` sizing status markers and exit codes for cache and virtualenv probes. - Added deterministic tests for the unbounded-`uv` regression, zero timeout, hard timeout timing, non-timeout errors, no-GNU fallback behavior, and the script contract. ## Why It Matters The diagnostic remains advisory and exits zero, while its expensive cache and virtual-environment walks no longer hide failures or consume the full readiness test budget. Hosts without GNU `timeout(1)` retain the prior direct-`du` fallback and ordinary output keys. ## Research / Evidence Notes Not applicable — this is a CI/tooling reliability change with no research, benchmark, metric, or paper-facing claim. ## Validation / Proof - `tests/dev/test_ci_uv_sync_diag.py`: 12 passed. - `tests/test_ci_script_contract.py`: 153 passed. - `bash -n scripts/dev/ci_uv_sync_diag.sh`: passed. - Ruff check and format checks for both changed Python test files: passed. - `BASE_REF=origin/main PR_READY_MODE=final ... scripts/dev/pr_ready_check.sh`: passed on committed head `8e7015c578279c1d4e1dd31279b0143bf9161eec` against base `5f476625eba9c4ae2fa5770781852b78c865fd8b`. ## Risks / Rollback The no-GNU fallback remains intentionally unbounded for compatibility with stock macOS-style hosts. Revert commit `8e7015c57` to restore the preceding diagnostic implementation if needed. ## Docs / Provenance No durable benchmark or model artifacts were produced. Readiness receipts remain worktree-local ignored output; the source contract and reproducible test fixtures are tracked in this PR. ## Downstream Propagation Not applicable — support/tooling change; no benchmark, metric, model, claim-map, registry, or durable evidence update is required. ## Follow-Up / Residual Scope No deferred work remains for this implementation slice. Issue #8249 is reopened for this PR; this is the immediate successor requested for the remaining `uv sync` diagnostic gaps. ## Reviewer Notes - Verify that `uv cache size` is absent from the executable path and that the later bounded `du` remains the sole cache-size traversal. - Verify that timeout expirations are not attributed to host contention without evidence and that non-timeout tool failures remain visible. <!-- Keep this marker and its keys unchanged. Update values to match the human narrative and changed files. --> <!-- pr-contract:v2 change_class: tooling linked_issues: closes: [] relates: [8249] deferred_work: status: none issues: [] reason: "" evidence: applicability: na tier: null result: na domain_approval: required: false status: not_required domains: [] note: "NA - support/tooling change; no experimental claim." performance: claimed: false -->
…b.com/ll7/robot_sf_ll7 into autopilot/sync-pr8452-20260905-cycle45
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review
Reviewed PR #8452 at exact head 998ce72bc4e7754a84025b40e9f42e86da9ee153 against fresh
origin/main a2bf33373e5775eacbeed5049c15b987ad5af9df after synchronizing the branch from the
current base.
This is a registry-scope cleanup for #8448. The branch deletes exactly the 12-line predictive
proxy path test accidentally carried by #8438; it leaves the two requested model/registry.yaml
path corrections, loader behavior, model files, release metadata, hashes, and provenance intact.
The change makes no research, benchmark, model-performance, or paper-facing claim.
Focused registry proof passed (34 passed), with the supplemental model-preflight/campaign checks
also passing (44 passed), plus Ruff 0.16.5, format, diff, and merge-tree checks clean in the
fresh-base worktree. Independent review found no actionable P0–P2 defect. A non-blocking P3
coverage follow-up remains: the deleted test was the only direct assertion for concrete predictive
entry path consistency, and restoring it here would violate the explicit registry-only cleanup
scope.
Hosted checks for this synchronized head are still pending; no hosted success is inferred yet.
Accept provisionally for the guarded merge path, subject to terminal current-head checks and the
repository’s exact-head receipt.
Decision: accept this tooling/scope-cleanup slice provisionally; no source repair is required.
gate-verdict: accepted @ 998ce72
pr-metadata: reconciled @ 829c3f7f5376da53804ee172bd19bd6cc7f0950e26f418cda76fdb1a55ff6007
## Summary Repair the scheduled main-CI incident reconciler's evidence acquisition so cancellation-heavy workflow history cannot hide the decisive runs required by the two-green closure rule. ## Linked Issues - Refs #8414 (historical cancellation-saturated reconciliation window; the incident is already closed and this PR does not change its state). ## Stack / Dependency - Base dependency: none. - Required prior PRs and stack follow-up issues: none. - Safe to review independently: yes; this is a self-contained maintenance-tooling change. ## What Changed - Replaced the default raw `gh run list` evidence cutoff with bounded GitHub REST pagination. - Resolve the configured workflow display name, read full 100-run pages, and stop only after two decisive completed green/red runs are visible. - Keep cancellations, pending runs, malformed payloads, and page-budget exhaustion fail-closed. - Add focused tests for cancellation-saturated pages, page-budget exhaustion, and page-budget routing; document the new `--max-run-pages` option and legacy `--run-limit` behavior. ## Why It Matters The prior ten-raw-run window could contain only cancelled runs under the repository's latest-main-wins concurrency policy, producing a pending report despite decisive greens just outside the window. The new default searches a bounded evidence window without treating cancellations as failures or making a current-head-green claim. ## Research / Evidence Notes Not applicable — no research claim. This changes only incident-reconciliation tooling and its operational evidence-window contract. ## Validation / Proof - `python -m pytest -q tests/dev/test_reconcile_main_ci_incidents.py` — 19 passed. - Ruff check and format checks passed for the changed Python files. - `python scripts/dev/check_docs_evidence_integrity.py --full` — 2052 Markdown files passed. - `bash scripts/dev/check_context_notes.sh` — passed. - Read-only live Actions REST probe from the linked worktree returned 100 current `CI` runs and identified the current decisive successes/failures. - Full final PR readiness is run against the committed exact head before publication. ## Risks / Rollback The default may make additional read-only Actions requests when cancellations fill a page; the ten-page ceiling bounds that work. If workflow inventory or run pagination cannot be verified, the reconciler returns an error and performs no issue mutation. Rollback is a single revert of this tooling commit. ## Docs / Provenance Updated `docs/dev_guide_reference.md` with the paginated evidence-window contract. No benchmark, model, checkpoint, raw log, or generated evidence artifact is changed. ## Downstream Propagation Not applicable — support change. ## Follow-Up / Residual Scope None; current-head CI failures remain separate incidents keyed to their own deciding runs. ## Reviewer Notes - Verify that only completed `success`/`failure` runs count as decisive evidence. - Verify that the page budget fails closed before any issue comment or close mutation. - This PR intentionally does not close or reopen #8414 and does not assert that any current head is green. <!-- pr-contract:v2 change_class: tooling linked_issues: closes: [] relates: [8414] deferred_work: status: none issues: [] reason: "" evidence: applicability: na tier: null result: na domain_approval: required: false status: not_required domains: [] note: "NA - support/tooling change; no experimental claim." performance: claimed: false -->
…e-cleanup-20260904' into autopilot/sync-pr8452-20260905-cycle47
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review
- PR: #8452
- Exact head reviewed:
8c4ce20038f30b19da95bc3cc6895a3a4f7582aa - Current
origin/mainbase:e7b6f9bfde9f80ca65d62bb1d85c681946e2a3dd - Scope: the current-base diff is exactly 12 deletions in
tests/unit/test_registry.py; the
registry corrections and all production/model/provenance files remain unchanged. - Focused proof:
tests/unit/test_registry.py— 34 passed; scoped Ruff and format checks passed;
git diff --checkpassed. - Independent review: no P0/P1/P2 findings. One non-blocking P3 coverage gap remains: the deleted
test was the only direct assertion tying concrete predictivelocal_pathvalues to release
asset names/cache directories. Restoring it would violate the explicitly requested registry-only
cleanup boundary, so it should remain a separate follow-up if that contract is desired. - Gate disposition: accepted for normal admission after terminal hosted checks and a fresh
single-account merge receipt. This is a support/model-registry scope cleanup with no research or
benchmark claim.
gate-verdict: accepted @ 8c4ce20
pr-metadata: reconciled @ dd706e1508f794576abdc8dce97d31b896bbf456f0fd4434dce94c4b94efd620
|
review-claim: codex-goal-autopilot-20260905 @ 8c4ce20 until 2026-09-05T00:15:10Z |
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review
Reviewed PR #8452 at exact head 8c4ce20038f30b19da95bc3cc6895a3a4f7582aa against current origin/main e7b6f9bfde9f80ca65d62bb1d85c681946e2a3dd in a dedicated linked review worktree.
Findings and disposition
No actionable P0-P3 implementation finding was verified. The current-head diff is exactly 12 deletions from tests/unit/test_registry.py, removing only the predictive-proxy test that was outside the requested registry-only scope; model registry data, loader code, release metadata, hashes, and provenance are unchanged.
Focused proof at this exact head passed:
tests/unit/test_registry.py: 34 passed;- scoped Ruff check and format check: passed;
git diff --check origin/main...HEAD: passed;git merge-tree --write-tree origin/main HEAD: conflict-free.
The PR is not yet merge-admissible because six required hosted checks remain pending. No research, benchmark, release, or paper-facing claim is made or admitted. The current ordinary-CAS proof and final metadata digest must be reread after CI settles before any merge-ready label or merge operation.
base-policy: ordinary-cas @ 8c4ce20038f30b19da95bc3cc6895a3a4f7582aa
pr-metadata: reconciled @ dd706e1508f794576abdc8dce97d31b896bbf456f0fd4434dce94c4b94efd620
gate-verdict: blocked @ 8c4ce20038f30b19da95bc3cc6895a3a4f7582aa
merge-ready: no @ 8c4ce20038f30b19da95bc3cc6895a3a4f7582aa
|
review-claim: released @ 8c4ce20 Exact-head review published as COMMENTED review 5118391420. Disposition: no actionable implementation finding; hold merge-ready and await the six exact-head hosted checks before rebuilding the final merge receipt. |
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review
- PR: #8452
- Exact head reviewed:
8c4ce20038f30b19da95bc3cc6895a3a4f7582aa - Current
origin/mainbase:e7b6f9bfde9f80ca65d62bb1d85c681946e2a3dd - Scope: the current-base diff is exactly 12 deletions in
tests/unit/test_registry.py; the
registry corrections and all production/model/provenance files remain unchanged. - Focused proof:
tests/unit/test_registry.py— 34 passed; scoped Ruff and format checks passed;
git diff --checkpassed. - Independent review: no P0/P1/P2 findings. One non-blocking P3 coverage gap remains: the deleted
test was the only direct assertion tying concrete predictivelocal_pathvalues to release
asset names/cache directories. Restoring it would violate the explicitly requested registry-only
cleanup boundary, so it should remain a separate follow-up if that contract is desired. - Gate disposition: accepted for normal admission after terminal hosted checks and a fresh
single-account merge receipt. This is a support/model-registry scope cleanup with no research or
benchmark claim.
gate-verdict: accepted @ 8c4ce20
base-policy: ordinary-cas @ 8c4ce20
pr-metadata: reconciled @ dd706e1508f794576abdc8dce97d31b896bbf456f0fd4434dce94c4b94efd620
|
review-claim: codex-goal-autopilot-20260905-final @ 8c4ce20 until 2026-09-05T00:25:49Z |
ll7
left a comment
There was a problem hiding this comment.
Exact-head self-review
Reviewed PR #8452 at exact head 8c4ce20038f30b19da95bc3cc6895a3a4f7582aa against current origin/main e7b6f9bfde9f80ca65d62bb1d85c681946e2a3dd in a dedicated linked review worktree.
No actionable P0-P3 implementation finding was verified. The final diff is exactly 12 deletions in tests/unit/test_registry.py, removing only the out-of-scope predictive-proxy test while preserving the registry-only model-path corrections and all model/release provenance.
Proof at this exact head:
tests/unit/test_registry.py: 34 passed;- scoped Ruff check and format check: passed;
git diff --check origin/main...HEAD: passed;git merge-tree --write-tree origin/main HEAD: conflict-free;- hosted checks: 32 successful and 1 intentional skip, with no failures or pending required checks.
This is a support/tooling scope correction with no research, benchmark, release, or paper-facing claim. The ordinary-CAS compare-and-swap remains the final merge guard; no source repair or branch push is needed.
base-policy: ordinary-cas @ 8c4ce20038f30b19da95bc3cc6895a3a4f7582aa
pr-metadata: reconciled @ dd706e1508f794576abdc8dce97d31b896bbf456f0fd4434dce94c4b94efd620
gate-verdict: accepted @ 8c4ce20038f30b19da95bc3cc6895a3a4f7582aa
merge-ready: yes @ 8c4ce20038f30b19da95bc3cc6895a3a4f7582aa
Summary
Remove the focused predictive-proxy path test that was accidentally included in merged #8438, restoring #8427's requested registry-only change boundary. The two
local_pathcorrections and all registry provenance remain untouched.Linked Issues
Stack / Dependency
main.What Changed
test_predictive_proxy_registry_paths_match_release_assetsaddition fromtests/unit/test_registry.py.model/registry.yaml, loader code, documentation, configurations, model files, hashes, notes, or registry provenance.Why It Matters
Research / Evidence Notes
Not applicable — no research claim.
Validation / Proof
uv run python -m pytest tests/unit/test_registry.py -q— 34 passed.uv run ruff check tests/unit/test_registry.py— passed.git diff --check— passed.origin/mainis exactly 12 deletions intests/unit/test_registry.py;model/registry.yamlis unchanged.Risks / Rollback
Docs / Provenance
Downstream Propagation
Not applicable — support change with no empirical result or benchmark claim.
Follow-Up / Residual Scope
No deferred work remains.
Reviewer Notes
local_pathsubstitutions and that this follow-up removes only its additional test.