fix: HttpLedgerBackend caches server's canonical model_hash#8
Merged
vigneshnarayanaswamy merged 2 commits intomainfrom Apr 17, 2026
Merged
fix: HttpLedgerBackend caches server's canonical model_hash#8vigneshnarayanaswamy merged 2 commits intomainfrom
vigneshnarayanaswamy merged 2 commits intomainfrom
Conversation
Codex review on PR #7 (#7) flagged that save_model was caching the client-side model_hash from the incoming ModelRef, but the server computes its own hash from a fresh created_at. The client hash and server hash therefore diverge, and the hash-to-name cache would map a stale client hash to the name. Any downstream hash-based flow (get_model, list_snapshots, set_tag) would either fail to resolve in a fresh backend instance or silently resolve to a ModelRef carrying the server's hash, making returned model_hash / snapshot_hash values inconsistent with what the caller just "saved." Fix: - RecordOutput now carries `model_hash` so /record returns the server's canonical identity - HttpLedgerBackend.save_model reads the server's hash from the response, reassigns it onto the incoming ModelRef in place (so retained references see the authoritative identity), and caches only the server hash Tests: - Schema tests updated to supply model_hash in RecordOutput fixtures + assertion that the field is required - TestSaveModelCanonicalHash suite covers ref mutation, cache contents, and the fresh-backend tag round-trip scenario Codex described 697 pass, 4 skip; ruff + mypy clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes HttpLedgerBackend’s hash→name cache by ensuring it stores and propagates the server-canonical model_hash returned from /record, avoiding stale client-side hashes derived from local created_at.
Changes:
- Add required
model_hashtoRecordOutputand populate it in therecordtool. - Update
HttpLedgerBackend.save_model()to adopt the server-returnedmodel_hashand cache only that value. - Extend tests to assert
model_hashis required in schema and to cover canonical-hash reconciliation and cache behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/model_ledger/tools/schemas.py |
Adds required RecordOutput.model_hash and documents canonical server identity semantics. |
src/model_ledger/tools/record.py |
Populates model_hash in RecordOutput for both registration and non-registration paths. |
src/model_ledger/backends/http.py |
Updates save_model() to read server model_hash, mutate the passed ModelRef, and cache server hash. |
tests/test_tools/test_schemas.py |
Updates fixtures and asserts model_hash is required in JSON Schema. |
tests/test_backends/test_http_backend.py |
Adds coverage for canonical-hash mutation, cache contents, and fresh-backend tagging round-trip. |
CHANGELOG.md |
Documents the new model_hash field and the canonical-hash caching fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses all 3 review comments on PR #8 from copilot-pull-request-reviewer: 1. save_model now calls resp.raise_for_status() before mutating cache state, so a 4xx/5xx response no longer leaves the hash-to-name cache pointing at a model that was never persisted. 2. save_model now raises ValueError when a 2xx response lacks a valid model_hash string, instead of silently falling back to the client-computed hash. That fallback would reintroduce the exact stale-hash bug this PR was fixing, which could trigger during a client/server version mismatch where the server predates the required RecordOutput.model_hash field. 3. test_tag_flow_survives_fresh_backend_instance now asserts model is not None before dereferencing, for a clearer failure message. Added defensive-path coverage: - test_http_error_raises_and_does_not_cache — 500 response raises HTTPStatusError and leaves the cache empty - test_success_without_model_hash_raises — 200 response missing model_hash raises ValueError and leaves the cache empty Both use httpx.MockTransport to exercise the failure paths without needing a live server. 699 pass, 4 skip; ruff + mypy clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #7 addressing a P1 Codex review comment on the hash-to-name cache.
HttpLedgerBackend.save_modelwas caching the client-sidemodel_hashfrom the incomingModelRef, butModelRef.model_hashis derived fromcreated_at, which is set locally when the client callsLedger.register()— and the server computes its own hash from its owncreated_atwhen processing/record. The two values diverge, so the cache ended up mapping a stale hash to the model name.Downstream hash-based flows (
get_model,list_snapshots,set_tag) would then either fail to resolve in a fresh backend instance or silently resolve to aModelRefcarrying the server's hash — inconsistent with what the caller just "saved."Fix
RecordOutputgains a requiredmodel_hashfield so the server communicates its canonical identity back to the caller.record_fnpopulatesmodel_hashfrommodel.model_hashin both theregisteredand non-registration branches.HttpLedgerBackend.save_modelreads the server's hash from the response, reassigns it onto the incomingModelRef(so callers who retain the reference see the authoritative identity), and caches only the server hash.Tests
TestRecordOutputfixtures to supplymodel_hash+ assert the field is required in JSON Schema.TestSaveModelCanonicalHashsuite covers:save_modelmutates the incoming ref to the server's hash (client hash differs intentionally, using a stalecreated_at)HttpLedgerBackendinstance (empty cache) can still tag by name and list the tag back — the exact round-trip scenario Codex describedTest plan
.venv/bin/python -m pytest tests/— 697 pass, 4 skip (+4 new tests, +1 required-field assertion).venv/bin/python -m ruff check src/ tests/— clean.venv/bin/python -m mypy src/— clean🤖 Generated with Claude Code