fix(model): correct predictive registry cache paths - #8438
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 |
🔍 PR Contract Check SummaryOverall Status: 🟢 PASSED
ℹ️ Info
This check is mechanized. Please resolve any blockers to pass CI. |
github-actions
Bot
added
the
review-bot-auto
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review — PR #8438
- Head reviewed:
b0e206eee1ab7cc06467183040e335bc7ac06099 - Base reviewed:
origin/main=f891abee5523b40ff7863fb3bdfc218a1526c056 - Scope: exactly the two predictive-proxy
local_pathvalues inmodel/registry.yaml, plus the
bounded cache-integrity repair, regression tests, and its documentation. - The existing-cache checksum bypass identified during review is repaired: release-backed paths
insideoutput/model_cache/<model_id>/now require the registry SHA-256 before reuse, while
unpacked in-tree paths for archive-backed checkpoints retain their direct-resolution contract. - Focused registry/preflight proof:
49 passed; scoped Ruff check/format, compilation, and diff
checks passed. - Exact release-backed smoke: both predictive assets resolve offline to the intended paths and match
their pinned sizes and SHA-256 values.
No blocking implementation finding remains. Hosted checks and the current-base merge gate are still
required before merge admission; this PR makes no benchmark, planner-quality, metric, or research
claim.
b0e206e to
96fe539
Compare
github-actions
Bot
removed
the
review-bot-auto
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review — PR #8438
- Head reviewed:
96fe5395d183d32dc6600ab943e290658c087058 - Base reviewed:
origin/main=f891abee5523b40ff7863fb3bdfc218a1526c056 - The exact remote head is pointer-only: exactly the two predictive-proxy
local_pathvalues in
model/registry.yamlchanged, with no model bytes or other registry entries modified. - The two release assets and SHA-256 pins were independently verified in the companion worktree;
the pointers resolve to the intended cache paths.
Blocking finding: the current canonical resolver returns an existing local_path before checking a
GitHub-release SHA, so this pointer-only head must not merge standalone. PR #8439 contains the
bounded resolver checksum hardening and regression coverage; merge #8439 first (or admit both heads
in one current-base sequence), then refresh this review at the final exact head.
No benchmark, planner-quality, metric, or research result is claimed.
ll7
changed the title
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review
- Head reviewed:
96fe5395d183d32dc6600ab943e290658c087058 - Base reviewed:
origin/main=f891abee5523b40ff7863fb3bdfc218a1526c056 - Intended design: issue #8427 requires only the two stale predictive-model
local_pathcorrections
and preservation of all registry provenance and other entries. - Scope check: the head is one commit; its diff contains only
model/registry.yamlwith exactly two
deleted and two added scalar lines. - Validation: the downloads-disabled registry smoke passed for both exact paths;
robot-sf models verifyreportedok: true,checked: 2, andpassed: 2;tests/unit/test_registry.pypassed
32 tests; finalpr_ready_check.shpassed on this committed head with a clean-tree stamp. - Artifact/provenance check: the two ignored cache artifacts matched their pinned sizes and SHA-256
values; no model file, hash, release metadata, documentation, configuration, note, loader, or test
changed in the head. - 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 @ 96fe539
pr-metadata: reconciled @ 7562ebb0d189636bf14f02fa468361387ae1229e7a54e844c62a69bf312ab232
github-actions
Bot
added
the
review-bot-auto
|
Exact-head implementation review — PR #8438
Blocking finding: the current canonical resolver returns an existing No benchmark, planner-quality, metric, or research result is claimed. |
## Summary Harden the model resolver so an existing GitHub-release cache path is reused only after matching its registry-pinned SHA-256. This closes the cache-integrity gap exposed while reviewing the predictive registry path correction in #8427. ## Linked Issues - Relates to #8427 ## Scope and rationale - Verify existing `output/model_cache/<model_id>/` paths against the entry's `github_release.sha256` before returning them. - Route invalid cache files through the existing bounded, checksum-verified release hydration path; downloads-disabled callers fail closed. - Preserve direct resolution for unpacked in-tree checkpoint paths when release metadata names an archive bundle, such as the GA3C-CADRL TensorFlow checkpoint prefix. - Add focused tests for corrupt-cache rejection, valid reuse, and unpacked bundle compatibility. ## Why It Matters The registry already documents SHA-256-verified cache reuse, but the early `local_path` return could accept substituted bytes. The repair makes the documented cache contract effective without changing model bytes, release metadata, planner behavior, or benchmark semantics. ## Research / Evidence Notes Not applicable — this is a model-provisioning integrity repair with no empirical or research claim. ## Validation / Proof - `uv run pytest -q tests/unit/test_registry.py tests/models/test_model_preflight.py tests/models/test_preflight_models_cli.py` — 48 passed. - Corrupt release-backed cache rejection and valid offline reuse are covered by regression tests. - Existing unpacked archive-backed local-path behavior remains covered. - Scoped Ruff check/format, Python compilation, and `git diff --check` passed. - The #8427 predictive registry smoke independently resolved both release assets and matched the pinned sizes and SHA-256 values; this PR does not commit those transient cache files or pointer assertions. ## Risks / Rollback Release-backed cache files that are corrupt or substituted now trigger replacement hydration or an explicit downloads-disabled error. Unpacked in-tree checkpoint contracts are unchanged. Rollback is a single commit revert. ## Docs / Provenance The existing `model/registry.yaml` release URL, asset identity, and SHA-256 remain authoritative. The documentation now distinguishes verified model-cache paths from intentionally unpacked local bundle paths. ## Downstream Propagation Not applicable — support/runtime integrity only; no benchmark, metric, or paper-facing result. ## Follow-Up / Residual Scope The predictive-proxy pointer correction remains in #8427/#8438; merge this hardening first or as part of the same current-base integration sequence. ## Reviewer Notes Verify that checksum validation is limited to the conventional release cache path, that invalid cache bytes cannot be returned with downloads disabled, and that GA3C-CADRL's unpacked `.meta` path is not compared with its tarball checksum. <!-- pr-contract:v2 change_class: runtime linked_issues: closes: [] relates: [8427] deferred_work: status: open issues: [8427] reason: "The related predictive-proxy registry pointer correction is tracked separately in #8427." evidence: applicability: na tier: null result: na domain_approval: required: false status: not_required domains: [] note: "NA - model-provisioning integrity repair; no scientific or benchmark claim." performance: claimed: false exact_head: 1a6084e -->
cbc3e3a to
9246b81
Compare
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head implementation review — PR #8438
- Head reviewed:
9246b817a61586aa372e4e636ceb70f2d4a4e3db - Base reviewed:
origin/main=65f2d21cf70f36302868e8190d09782b24740b76 - The exact remote head changes exactly the two predictive-proxy
local_pathvalues in
model/registry.yamland adds one focused registry regression test; no model bytes or other
registry entries changed. - Focused registry proof:
35 passed, including the pointer/asset alignment assertion, after
rebasing onto the merged resolver hardening in #8439. - The two release assets and SHA-256 pins were independently verified in the companion worktree;
the pointers resolve to the intended cache paths.
The resolver checksum hardening is now in the reviewed base through merged PR #8439, so the two
pointer changes are safe to admit as this separate current-base slice. The focused test binds each
cache path to its registry model ID and release asset name.
No benchmark, planner-quality, metric, or research result is claimed.
ll7
left a comment
ll7
left a comment
There was a problem hiding this comment.
Exact-head self-review
Reviewed exact head 9246b817a61586aa372e4e636ceb70f2d4a4e3db against
origin/main=65f2d21cf70f36302868e8190d09782b24740b76.
The intended design is a narrow registry correction: the two predictive-proxy entries must point
to their verified release-backed cache locations, preserving each model ID, release asset name,
URL, and SHA-256 pin. The exact diff contains those two local_path replacements and one focused
regression test that binds the cache directory and filename to the registry metadata. The merged
#8439 resolver hardening is present in the reviewed base, so existing release-backed cache reuse is
checksum-verified before these pointers are used.
Local proof at this exact head passed all 35 tests/unit/test_registry.py tests, including the
pointer/asset alignment assertion, plus Ruff check/format, Python compilation, and git diff --check. Fresh hosted exact-head validation is terminally green: 33 successes and two expected
skips, with no stale or failing jobs. The exact-head diagnostic review was refreshed after the
rebase.
No benchmark, planner-quality, metric, release-rights, security, dependency, or research result is
claimed. No model binaries or transient cache files are committed.
Single-account internal-review waiver: no distinct repository human implementation reviewer is
available in this lane; the exact-head implementation review and independent sidecar review are
recorded. No unresolved actionable review thread or requested external reviewer remains.
base-policy: ordinary-cas @ 9246b817a61586aa372e4e636ceb70f2d4a4e3db
pr-metadata: reconciled @ 322ea3527e9609b3247f9c88789719cb3bfc2e994f5c227501f60af5bf8a987e
gate-verdict: accepted @ 9246b817a61586aa372e4e636ceb70f2d4a4e3db
Confidence: High. The PR is ready for the guarded expected-head merge and closes the bounded #8427
pointer correction.
## 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_path` corrections and all registry provenance remain untouched. ## Linked Issues - Closes #8448 - Relates to #8427 - Relates to #8438 ## Stack / Dependency - Base dependency: PR #8438, already merged into `main`. - Required prior PRs and stack follow-up issues: none. - Safe to review independently: yes — this deletes only the test addition from #8438. ## What Changed - Delete the 12-line `test_predictive_proxy_registry_paths_match_release_assets` addition from `tests/unit/test_registry.py`. - Do not change `model/registry.yaml`, loader code, documentation, configurations, model files, hashes, notes, or registry provenance. ## Why It Matters - Restores the explicitly requested scope for #8427: exactly the two registry scalar substitutions, with no committed regression test. ## 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. - The branch diff against current `origin/main` is exactly 12 deletions in `tests/unit/test_registry.py`; `model/registry.yaml` is unchanged. ## Risks / Rollback - No runtime or model-selection behavior changes. Rollback is a single commit revert if the test is intentionally reinstated. ## Docs / Provenance - No documentation or provenance metadata changed. The two corrected registry paths and their pinned release metadata remain as merged by #8438. ## Downstream Propagation Not applicable — support change with no empirical result or benchmark claim. ## Follow-Up / Residual Scope No deferred work remains. ## Reviewer Notes - Confirm that the merged #8438 result retains only the two requested `local_path` substitutions and that this follow-up removes only its additional test. <!-- pr-contract:v2 change_class: tooling linked_issues: closes: [8448] relates: [8427, 8438] deferred_work: status: none issues: [] reason: "The merged #8438 scope residue is removed; no deferred work remains." evidence: applicability: na tier: null result: na domain_approval: required: false status: not_required domains: [] note: "NA - test-scope cleanup; no experimental claim." performance: claimed: false exact_head: 8c4ce20 -->
1 participant
Summary
Point the two predictive-proxy registry entries at their verified release-backed
output/model_cache/<model_id>/<asset_name>locations. No model binaries, release metadata, orother registry pointers change.
Linked Issues
Scope and rationale
local_pathvalues forpredictive_proxy_selected_v1andpredictive_proxy_selected_v2_fullinmodel/registry.yaml.behavior.
output/model_cache/worktree-local and ignored; no checkpoint is added to Git.Validation / Proof
exact-base validation worktree.
registry model ID and GitHub-release asset name.
uv run pytest -q tests/unit/test_registry.pypasses all 35 registry tests after the rebase ontothe merged resolver hardening.
0e7e775fd09509543761d8326237c4f85b0cc540fb24f98b3c5f8f7c0009fccc; v2 full is 4,958,329bytes with SHA-256
a28aed6d6ad7e1ebf597277ade1cf908efa6da038d0a9fcfdf80c7c31d8d1be1.local_pathreplacements plus the focused regression test, andgit diff --checkpasses.Risks / Rollback
The pointers rely on the normal release-backed cache hydration path when the ignored cache is cold.
The resolver checksum hardening is now in
origin/mainthrough #8439, so these pointers use theverified release-backed cache contract; rollback is a single commit revert.
Docs / Provenance
The existing
github_releasemetadata inmodel/registry.yamlremains authoritative. Transientcache files are not part of this PR.
Downstream Propagation
Not applicable — registry-path compatibility only; no empirical result or benchmark claim.
Follow-Up / Residual Scope
#8439 merged the resolver checksum-hardening follow-up before this pointer correction was admitted.
Reviewer Notes
Verify the two exact pointer changes, their release asset-name alignment, and the merged #8439
resolver hardening before merging this PR.