feat: content-addressed model cache + PR area auto-labeling - #227
feat: content-addressed model cache + PR area auto-labeling#227albin-george-kurian wants to merge 7 commits into
Conversation
…hts are stored by SHA-256 and hard-linked to their readable path, so identical content is never duplicated and a known digest skips the download. Adds path-based PR labels (cli/core/adapters/docs/tests/ci)." -m "Closes OpenAgentHQ#59" -m "Closes OpenAgentHQ#137
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a content-addressed blob store to FilesystemCache: downloaded weights land at cache/blobs/<sha256[:2]>/<sha256> and are exposed at cache/models/<name>/<tag>.gguf via a hard link (copy fallback where links are unavailable), so byte-identical weights cost disk once and are reference-counted — evict() reclaims a blob only when its last referencing entry is gone and clean() prunes unreferenced blobs. Introduces ContentStorePort as an optional companion to CachePort (has_blob, blob_path, store_blob, link_into, record_artifact) so manifest-only caches stay valid, and makes ModelManager.install() skip the download entirely when the catalog publishes a variant's sha256 and those bytes are already stored. Adds a pull_request_target PR labeler that tags PRs by touched area (cli/core/adapters/docs/tests/ci) additively, ensuring the area labels exist first without overwriting manually curated ones.
Worth a look
- Privileged pull_request_target workflow uses mutable action tags —
.github/workflows/labeler.yml:42· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- link_into may create hardlink to shared blob, mutable via readable path —
src/modeldock/adapters/cache/filesystem.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- link_into must not consume the source blob —
src/modeldock/adapters/cache/filesystem.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- store_blob return contract changed from name-keyed to content-keyed —
src/modeldock/adapters/cache/filesystem.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- TOCTOU between _prune_orphan_blobs and store_blob deletes freshly stored weights —
src/modeldock/adapters/cache/filesystem.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 467 functions depend on the 380 functions this change touches.
Health — this change adds coupling hotspots:
- new:
ModelManager— 34 callers, 25 callees - new:
_manager()— 21 callers, 3 callees - new:
_manager()— 10 callers, 3 callees - new:
_manager()— 5 callers, 3 callees
Verification — 467 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 467 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 3 more finding(s) on lines outside this diff (see the check run).
| ) | ||
|
|
||
|
|
||
| def _manager( |
There was a problem hiding this comment.
_manager()
high coupling complexity (Ca·Ce = 15).
Grounded coupling-delta finding (deterministic), not an LLM guess.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a content-addressed blob store to FilesystemCache: downloaded weights land at cache/blobs/<sha256[:2]>/<sha256> and are surfaced at cache/models/<name>/<tag>.gguf via a hard link (copy fallback where links are unavailable), so byte-identical weights are stored once and reference-counted through new manifest blob/path fields; the blob operations live behind a separate optional ContentStorePort so manifest-only caches stay valid. Makes ModelManager.install() skip the download entirely and just link when the catalog publishes a variant's sha256 and those bytes are already stored, and reworks evict() to unlink and reclaim a blob only once its last referencing entry is gone while clean() prunes unreferenced blobs. Adds a pull_request_target labeler workflow that applies area: labels from changed paths (additive, sync-labels off) and ensures the label set exists idempotently.
Worth a look
- Concurrent manifest records can overwrite each other —
src/modeldock/adapters/cache/filesystem.py:91· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Reused orphan blob can be pruned before it is recorded —
src/modeldock/adapters/cache/filesystem.py:225· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Manifest path can unlink files outside cache —
src/modeldock/adapters/cache/filesystem.py:302· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- _gc_blob accepts malformed digests that can unlink files outside the blob store —
src/modeldock/adapters/cache/filesystem.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- store_blob then clean race: fresh blob only protected by mtime grace window —
src/modeldock/adapters/cache/filesystem.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 475 functions depend on the 388 functions this change touches.
Health — this change adds coupling hotspots:
- new:
ModelManager— 34 callers, 25 callees - new:
_manager()— 21 callers, 3 callees - new:
_manager()— 10 callers, 3 callees - new:
_manager()— 5 callers, 3 callees
Verification — 475 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 475 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 3 more finding(s) on lines outside this diff (see the check run).
| ) | ||
|
|
||
|
|
||
| def _manager( |
There was a problem hiding this comment.
_manager()
high coupling complexity (Ca·Ce = 15).
Grounded coupling-delta finding (deterministic), not an LLM guess.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a content-addressed blob store to FilesystemCache: downloaded weights are stored once at cache/blobs/<sha256[:2]>/<sha256> and exposed to callers as a hard link at cache/models/<name>/<tag>.gguf, falling back to a copy where hard links are unavailable. Makes ModelManager.install() skip the download entirely when the catalog publishes a variant's sha256 and those bytes are already stored, and reworks evict()/clean() to reference-count blobs so weights are reclaimed only once the last referencing entry is gone and orphaned blobs are pruned. Introduces ContentStorePort as an optional companion to CachePort (manifest-only caches stay valid), and adds a pull_request_target labeler workflow that additively tags PRs by touched area from .github/labeler.yml.
Worth a look
- store_blob can discard the download when a reused blob is deleted after the existence check —
src/modeldock/adapters/cache/filesystem.py:223· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- clean() prunes orphan blobs from the surviving-entries snapshot but does not respect grace period for blobs recorded via record but not touched —
src/modeldock/adapters/cache/filesystem.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Manifest read-modify-write is not atomic across evict/clean/record —
src/modeldock/adapters/cache/filesystem.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- clean() can delete a blob for an in-flight install past grace window —
src/modeldock/adapters/cache/filesystem.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- clean() return value now includes pruned blob digests mixed with entry keys —
src/modeldock/adapters/cache/filesystem.py:132· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 487 functions depend on the 400 functions this change touches.
Health — this change adds coupling hotspots:
- new:
ModelManager— 34 callers, 25 callees - new:
_manager()— 21 callers, 3 callees - new:
_manager()— 10 callers, 3 callees - new:
_manager()— 5 callers, 3 callees
Verification — 487 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 487 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 3 more finding(s) on lines outside this diff (see the check run).
| ) | ||
|
|
||
|
|
||
| def _manager( |
There was a problem hiding this comment.
_manager()
high coupling complexity (Ca·Ce = 15).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| lock = _lock(tmp_path) | ||
| with pytest.raises(RuntimeError), lock: | ||
| raise RuntimeError("boom") | ||
| assert not lock.held |
There was a problem hiding this comment.
Fix: Restructure the test so CodeQL can see that execution continues after the exception is caught. The combined form (with pytest.raises(RuntimeError), lock:) confuses the analyzer — it does not model pytest.raises as an exception-catching context manager, so it treats everything after the raise as dead code.
Separate the two context managers:
def test_releases_on_exception(tmp_path: Path) -> None:
lock = _lock(tmp_path)
with pytest.raises(RuntimeError):
with lock:
raise RuntimeError('boom')
assert not lock.held # now visibly reachable
with lock:
assert lock.held
Behaviour is identical: CacheLock.exit still receives the exception (verifying the lock is released on error), and pytest.raises catches it. The assert not lock.held line is now clearly reachable to both humans and CodeQL.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 5 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a content-addressed blob store to FilesystemCache: downloaded weights land at cache/blobs/<sha256[:2]>/<sha256> and are exposed at cache/models/<name>/<tag>.gguf via a hard link (falling back to a copy where links aren't available), so byte-identical weights cost disk once and are reference-counted across manifest entries. Exposes this behind a new optional ContentStorePort separate from CachePort, and makes ModelManager.install() skip the download entirely when the catalog publishes a variant's sha256 whose bytes are already stored, while evict() reclaims a blob only after its last referencing entry is gone and clean() prunes unreferenced blobs. Also adds a pull_request_target PR labeler workflow that tags PRs by touched area (cli, core, adapters, docs, tests, ci), creating the labels idempotently and only ever adding them.
Worth a look
- transaction deadlocks when wrapping cache mutations —
src/modeldock/adapters/cache/filesystem.py:54· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Concurrent HTTP installs write the same destination outside the cache transaction —
src/modeldock/core/manager.py:393· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Privileged pull_request_target workflow uses unpinned actions —
.github/workflows/labeler.yml:43· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Concurrent hard-link creation can fall through to copying onto the same file —
src/modeldock/adapters/cache/filesystem.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Freshly stored blobs can be pruned immediately if source mtime is old —
src/modeldock/adapters/cache/filesystem.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 534 functions depend on the 450 functions this change touches.
Health — this change adds coupling hotspots:
- new:
ModelManager— 34 callers, 25 callees - new:
FilesystemCache— 48 callers, 3 callees - new:
_manager()— 21 callers, 3 callees - new:
_manager()— 10 callers, 3 callees - new:
_manager()— 5 callers, 4 callees
Verification — 534 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 534 function(s) in the blast radius were not formally verified this run
· 2 grounded finding(s) anchored inline below; 3 more finding(s) on lines outside this diff (see the check run).
|
@himanshu231204, Can you guide me what to do in here to unblock it? |
PR Review — Content-Addressed Cache + PR Auto-LabelingHi @albin-george-kurian, great contribution! The design is solid and the test coverage is impressive. Here's a detailed walkthrough to help you land this cleanly. What's done really wellArchitecture: Reference counting: The interlock between Locking: Security:
Issues / suggestions1.
|
Resolving the GitHub Advanced Security (CodeQL) findingsHi @albin-george-kurian — here's what CodeQL is flagging and exactly how to fix each one so those alerts go green. 1. "Statement has no effect" —
|
| File | Change |
|---|---|
src/modeldock/ports/cache.py |
Replace ... with pass in all 6 ContentStorePort stub methods |
tests/unit/test_cache_lock.py |
Nest with lock: inside with pytest.raises(RuntimeError): instead of using the combined form |
tests/unit/test_labeler_config.py |
Check for any lambda x: f(x) patterns; replace with f directly (may already be resolved) |
These are all non-functional changes — the behaviour of the code and tests stays exactly the same. Once pushed, the CodeQL alerts should close automatically on re-scan.
|
Thank you, @himanshu231204, for answering my query. I will make the changes accordingly. |
|
@himanshu231204 , is this rdy? |
3 participants
Summary
Implements two issues: the filesystem cache now stores model weights content-addressed, so identical weights are never duplicated on disk, and pull requests are automatically labeled by the areas they touch.
Closes #59
Closes #137
Changes
Content-addressed cache (#59)
src/modeldock/adapters/cache/filesystem.py— weights now live in a blob store atcache/blobs/<sha256[:2]>/<sha256>and are exposed at their readablecache/models/<name>/<tag>.ggufpath via a hard link (copy fallback where hard links are unsupported).store_blob()discards a duplicate download and returns the existing blob; a digest that does not match the catalog's is rejected before anything enters the store. Blobs are reference-counted through the manifest, soevict()never deletes weights another ref still uses, andclean()reclaims orphaned blobs (force=Truewipes them).src/modeldock/ports/cache.py— newContentStorePortprotocol (has_blob,blob_path,store_blob,link_into,record_artifact), deliberately separate fromCachePortso manifest-only caches remain valid implementations and callers feature-detect the blob API.src/modeldock/core/manager.py—_install_via_http()skips the download entirely when the catalog publishes a digest whose bytes are already stored (instant, offline-capable re-install); otherwise it downloads, stores, links and records. Caches that are not a content store keep the previous behaviour unchanged.src/modeldock/ports/__init__.py— exportsContentStorePort.PR auto-labeling (#137)
.github/labeler.yml— path globs mapping changed files toarea: cli/core/adapters/docs/tests/ci..github/workflows/labeler.yml—actions/labeler@v5onpull_request_target(so PRs from forks get a token that can write labels) withcontents: read,pull-requests: write,issues: write. Checks out no PR code, which is what keeps that trigger safe.sync-labels: false, so manually applied labels are never stripped. A bootstrap step creates the area labels on first use and leaves existing ones exactly as maintainers configured them.Docs
Architecture.md§8 — documents the real layout (blobs/+ hard link), the newblob/pathmanifest fields, the skip-the-download path, and reference-counted reclamation; module tree and layer table updated.docs/user-guide/cache.md— user-facing explanation of the blob store and what it means for disk usage.CHANGELOG.md—Unreleasedentries under Added/Changed.Testing
pytest— 697 passed, 4 skipped (skips are the Ollama integration tests; no local Ollama available).tests/unit/test_content_store.py(29 tests): identical bytes → one blob; digest mismatch rejected with nothing entering the store; hard-link fallback to copy; a shared blob survives one eviction and is reclaimed on the last; orphan pruning; and at manager level — two models with the same published digest produce 1 download, 1 blob, 2 usable artifact paths.tests/unit/test_labeler_config.py(40 tests): required areas present, every glob still matches a real path in the tree, per-area routing table, areas do not overlap, and every configured label is bootstrapped by the workflow.tests/unit/test_port_contract.py—ContentStorePortadded to the shared adapter contract suite.tests/unit/test_workflow_permissions.py—TestLabelerWorkflow: exact permission block, job inherits top level, trigger ispull_request_target, and noactions/checkout.ruff check src tests,ruff format --check src tests,mypy src(strict, 80 files),bandit -r src -c pyproject.toml— all clean.pull_request_targetruns the workflow from the base branch, so this PR does not label itself. After merge, verify on a test PR touchingsrc/modeldock/cli/app.py+docs/index.md→ expectarea: cli+area: docs.test_labeler_config.pyis the pre-merge proof that the mapping is correct.Checklist
feat/auto-label-cache-dedup)mainAGENT.mdcoding standards (type hints, Pydantic v2, no genericException, no business logic in CLI)domain/andports/stay pure (no I/O, no framework imports)ruff,mypy --strict,bandit,pytestArchitecture.md§8,docs/user-guide/cache.md,CHANGELOG.md)pyproject.tomlandsrc/modeldock/__init__.pyversions match (no version change in this PR)