feat: add tag endpoints end-to-end (REST + HTTP backend + MCP)#7
Merged
vigneshnarayanaswamy merged 1 commit intomainfrom Apr 17, 2026
Merged
feat: add tag endpoints end-to-end (REST + HTTP backend + MCP)#7vigneshnarayanaswamy merged 1 commit intomainfrom
vigneshnarayanaswamy merged 1 commit intomainfrom
Conversation
The Ledger SDK's tag() method works on in-memory, SQLite, JSON-file,
and Snowflake backends, but was silently no-op on the HTTP backend
(set_tag was pass, get_tag returned None, list_tags returned []).
The REST API had no /tag endpoints at all.
This adds the missing tag surface across the stack:
- TagInput, TagOutput, TagListOutput Pydantic schemas
- tools/tag.py with tag() and list_tags() tool functions
- POST /tag and GET /tags/{model_name} REST endpoints
- Real set_tag/get_tag/list_tags on HttpLedgerBackend
- tag and list_tags MCP tools in both direct and HTTP pass-through
modes (tool count 6 -> 8)
- Schema resource exposes the new Tag models
Bugfix along the way: HttpLedgerBackend.get_model(model_hash) was
broken because list_models() reconstructs ModelRefs without created_at
(ModelSummary omits it), producing mismatched model_hash values. Fixed
with a lazy hash->name cache populated on successful name lookups in
save_model and get_model_by_name. Documented in CLAUDE.md Known Gaps.
25 new tests, 693 total pass, ruff and mypy clean.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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: 892c8f77e4
ℹ️ 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".
Comment on lines
+55
to
+59
| self._hash_to_name[model.model_hash] = model.name | ||
|
|
||
| def get_model(self, model_hash: str) -> ModelRef | None: | ||
| # HTTP backend searches by name via query, not by hash directly. | ||
| # This is a fallback — investigate is the primary read path. | ||
| models = self.list_models() | ||
| for m in models: | ||
| name = self._hash_to_name.get(model_hash) | ||
| if name is not None: |
There was a problem hiding this comment.
save_model() records model.model_hash before any round-trip to fetch the canonical server identity, but ModelRef hashes include created_at (core/ledger_models.py), so the caller-side hash from Ledger.register() will usually differ from the hash persisted by the REST server. With the new cache path in get_model(), that stale hash is then treated as valid and resolved by name, which can make follow-up tag flows return model_hash/snapshot_hash values that do not exist remotely and fail to round-trip in a new HttpLedgerBackend instance.
Useful? React with 👍 / 👎.
vigneshnarayanaswamy
pushed a commit
that referenced
this pull request
Apr 17, 2026
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>
3 tasks
vigneshnarayanaswamy
added a commit
that referenced
this pull request
Apr 17, 2026
* fix: HttpLedgerBackend caches server's canonical model_hash 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> * fix: address Copilot review — fail loudly on /record errors 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> --------- Co-authored-by: Vignesh Narayanaswamy <Vigneshn@squareup.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
1 participant
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
Completes the tag I/O surface across the stack. Previously
Ledger.tag()worked against in-memory / SQLite / JSON-file / Snowflake backends, but was a silent no-op againstHttpLedgerBackendand had no matching REST endpoints. This PR closes that gap end-to-end.What's new
Schemas (
tools/schemas.py)TagInput,TagOutput,TagListOutputTool functions (
tools/tag.py)tag(input, ledger)— create or move a tag to the latest snapshotlist_tags(model_name, ledger)— return all tags for a modelREST API (
rest/app.py)POST /tag— body{model_name, tag_name}→TagOutputGET /tags/{model_name}→TagListOutput404viaModelNotFoundErrorwhen the model is unknown.HTTP backend (
backends/http.py)set_tag/get_tag/list_tagsimplementations (replace the previous stubs).LedgerBackend.set_tag(tag: Tag)protocol (which takesmodel_hash) to the name-keyed REST API.MCP server (
mcp/server.py)tagandlist_tagstools in both the direct SDK-backed server and the HTTP pass-through server.Tag*models.Docs
CLAUDE.mdgains generic Extension Points and Known Gaps sections.CHANGELOG.mdupdated under Unreleased.Bonus bugfix
HttpLedgerBackend.get_model(model_hash)was silently returningNonefor valid models. Root cause:list_models()reconstructsModelReffrom/queryresponses, butModelSummaryomitscreated_at, so_compute_model_hash()produces a different hash every call. This broke any downstream code that went hash → name (list_snapshots,latest_snapshot, the newset_tag).Fix: lazy
hash → namecache populated on successfulsave_modelandget_model_by_namecalls.get_model(hash)consults the cache first, falls back to the oldlist_models()iteration. Documented inCLAUDE.mdKnown Gaps.Test plan
.venv/bin/python -m pytest tests/— 693 pass, 4 skip (+25 new tests).venv/bin/python -m ruff check src/ tests/— clean.venv/bin/python -m mypy src/— cleanLedger.tag()backed byHttpLedgerBackendwrites through the REST API and round-trips vialist_tags()(seeTestLedgerTagOverHttpintests/test_backends/test_http_backend.py)Non-goals
DELETE /tag—Ledger.tag()overwrites on re-set, so explicit deletion isn't needed yet.list_models()+ per-modelGET /tags/{name}if you need one.🤖 Generated with Claude Code