Add managed CacheSet lifecycle#64
Conversation
📝 WalkthroughWalkthroughThis change adds RootSet-backed managed cache sets, streaming bundle member iteration with logical-byte accounting, generation-aware root mutations, structured Git object metadata reads, public type and facade wiring, documentation, and lifecycle, concurrency, repair, and reachability tests. ChangesManaged cache contracts and bundle inventory
Cache lifecycle and publication
Git metadata inspection and supporting documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant ContentAddressableStore
participant CacheSetRegistry
participant CacheSet
participant RootSet
Client->>ContentAddressableStore: caches.open(namespace, policy)
ContentAddressableStore->>CacheSetRegistry: open(namespace, policy)
CacheSetRegistry->>RootSet: construct managed cache root
CacheSetRegistry->>CacheSet: return CacheSet
Client->>CacheSet: put/get/sweep
CacheSet->>RootSet: publish guarded generation
RootSet-->>CacheSet: generation result
CacheSet-->>Client: cache result or hit
sequenceDiagram
participant BundleService
participant GitPersistenceAdapter
participant CacheIndex
BundleService->>GitPersistenceAdapter: readObjectSize(handle)
GitPersistenceAdapter->>GitPersistenceAdapter: cat-file --batch-check
GitPersistenceAdapter-->>BundleService: object type and size
BundleService->>CacheIndex: yield validated member descriptor
CacheIndex-->>BundleService: logical-byte accounting
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Self-review and Code LawyerReviewed the GitHub-visible 31-file diff against issue #59, the application-storage boundary in #50, the public declarations, and the real-Git lifecycle proof. Contract audit
Findings resolved before PRTen findings were found and corrected: stale retry state, rejected-write staging, preflight bundle materialization, recursive sweep resolution, unchecked logical-byte metadata, malformed Unicode keys, malformed index shape, accidental registry export, duplicate repair authority, and shallow result immutability. No unresolved correctness or API-contract finding remains. Graft reports an additive minor export change with no removals or changed signatures. The full source-pinned review and residual constraints are in |
CI follow-upThe first Bun matrix run exposed a cross-runtime error-normalization defect in the previously landed asset-handle path: Commit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@index.d.ts`:
- Line 342: Update the CacheHit.logicalBytes declaration from number | null to
number so it matches CacheHitData, the constructor parameter, and the runtime
validation contract; do not alter the surrounding CacheHit API.
In `@src/domain/services/CachePolicyEnforcer.js`:
- Around line 52-56: Update createCacheState to import and use the exported
CACHE_METADATA_VERSION and CACHE_ACCOUNTING_VERSION constants for version and
accountingVersion instead of hardcoded literals, keeping state production
aligned with CacheMetadataCodec validation.
In `@test/integration/cache-set.test.js`:
- Around line 73-80: Make the test “makes a removed target collectible while
retaining the winning index” self-contained by creating its target and cache
entry before calling cache.get('current'), using a dedicated namespace to avoid
dependence on preceding tests. Preserve the existing removal assertions and
doctor health check.
In `@test/unit/domain/services/CacheCandidateHeap.test.js`:
- Around line 5-17: Update the test “retains only the bounded oldest candidates
with deterministic ties” to include at least two candidates sharing the same
sortKey, then assert their documented deterministic digest tie-break order in
the sorted result while preserving the bounded oldest-candidate coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8408ede7-9b3b-481a-858f-c1568556b6e8
📒 Files selected for processing (31)
CHANGELOG.mdREADME.mdSTATUS.mddocs/API.mddocs/design/0047-application-storage-cache-boundary/witness/cache-set.mdindex.d.tsindex.jssrc/domain/errors/Codes.jssrc/domain/services/BundleService.jssrc/domain/services/CacheCandidateHeap.jssrc/domain/services/CacheIndex.jssrc/domain/services/CacheMetadataCodec.jssrc/domain/services/CachePolicyEnforcer.jssrc/domain/services/CacheSet.jssrc/domain/services/CacheSetRegistry.jssrc/domain/services/RootSet.jssrc/domain/services/RootSetMetadataCodec.jssrc/domain/services/RootSetPersistence.jssrc/domain/value-objects/CacheHit.jssrc/domain/value-objects/CacheKey.jssrc/domain/value-objects/CachePolicy.jssrc/domain/value-objects/CacheSetRef.jssrc/domain/value-objects/CollectionNamespace.jstest/integration/cache-set.test.jstest/unit/domain/services/BundleService.test.jstest/unit/domain/services/CacheCandidateHeap.test.jstest/unit/domain/services/CacheSet.test.jstest/unit/domain/services/RootSet.test.jstest/unit/domain/value-objects/CacheCollectionValues.test.jstest/unit/facade/ContentAddressableStore.application-storage.test.jstest/unit/types/declaration-accuracy.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test-docker (deno)
- GitHub Check: test-docker (bun)
- GitHub Check: test-docker (node)
🧰 Additional context used
📓 Path-based instructions (4)
STATUS.md
📄 CodeRabbit inference engine (AGENTS.md)
Use
STATUS.mdas a compact snapshot of release and runtime truth
Files:
STATUS.md
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Use
CHANGELOG.mdto record the historical truth of merged behavior
Files:
CHANGELOG.md
README.md
📄 CodeRabbit inference engine (AGENTS.md)
Use
README.mdas the public front door, core value prop, and quick start documentation
Files:
README.md
docs/design/**
📄 CodeRabbit inference engine (AGENTS.md)
Use
docs/design/directory for durable design contracts and proof plans
Files:
docs/design/0047-application-storage-cache-boundary/witness/cache-set.md
🧠 Learnings (2)
📚 Learning: 2026-02-28T19:21:13.982Z
Learnt from: flyingrobots
Repo: git-stunts/git-cas PR: 15
File: test/unit/domain/services/CasService.envelope.test.js:326-330
Timestamp: 2026-02-28T19:21:13.982Z
Learning: In fuzz tests for cryptographic operations, use a seeded PRNG (e.g., xorshift32) for control-flow variables such as plaintext size selection and recipient index selection to ensure reproducibility. Continue to use randomBytes() for cryptographic keys and nonces to preserve realistic randomness and avoid security anti-patterns. This guideline applies to test files that perform fuzz testing of crypto logic; implement a consistent seed setup (e.g., fixed seed in test initialization) and document the rationale to enable deterministic replays across runs.
Applied to files:
test/unit/domain/services/CacheCandidateHeap.test.jstest/unit/domain/value-objects/CacheCollectionValues.test.jstest/unit/types/declaration-accuracy.test.jstest/unit/facade/ContentAddressableStore.application-storage.test.jstest/integration/cache-set.test.jstest/unit/domain/services/BundleService.test.jstest/unit/domain/services/RootSet.test.jstest/unit/domain/services/CacheSet.test.js
📚 Learning: 2026-03-30T19:53:48.000Z
Learnt from: flyingrobots
Repo: git-stunts/git-cas PR: 29
File: docs/design/TR-010-planning-index-consistency-review.md:121-131
Timestamp: 2026-03-30T19:53:48.000Z
Learning: In this repo, CHANGELOG.md entries should be user-facing and descriptive (bullet points) rather than cycle-ID headings (e.g., use a phrase like “Planning-index consistency review” instead of a “TR-010 — …” heading). When searching the changelog, don’t rely on grepping for cycle IDs (e.g., “TR-010”) because it may cause false negatives; search for the descriptive keywords/phrases from the bullet entries instead.
Applied to files:
CHANGELOG.md
🪛 ast-grep (0.44.1)
test/integration/cache-set.test.js
[warning] 4-4: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
🔇 Additional comments (32)
src/domain/errors/Codes.js (1)
14-20: LGTM!index.js (1)
15-16: LGTM!Also applies to: 66-66, 91-92, 179-181, 196-196, 223-224, 282-290, 346-351, 400-405, 949-949
README.md (1)
26-29: LGTM!Also applies to: 101-101, 119-122
CHANGELOG.md (1)
41-49: LGTM!Also applies to: 60-62
test/unit/facade/ContentAddressableStore.application-storage.test.js (1)
5-7: LGTM!Also applies to: 31-44, 57-69
test/unit/types/declaration-accuracy.test.js (1)
93-96: LGTM!STATUS.md (1)
91-95: LGTM!docs/design/0047-application-storage-cache-boundary/witness/cache-set.md (1)
1-220: LGTM!test/integration/cache-set.test.js (1)
1-71: LGTM!Also applies to: 84-149
test/unit/domain/services/CacheSet.test.js (1)
1-387: LGTM!test/unit/domain/services/RootSet.test.js (1)
4-5: LGTM!Also applies to: 69-89
test/unit/domain/value-objects/CacheCollectionValues.test.js (1)
1-46: LGTM!src/domain/services/CacheSet.js (3)
43-168: LGTM!
205-703: LGTM!
170-203: 🎯 Functional CorrectnessNo issue:
CacheIndex.entries()already iterates in canonical path order, andentries/<64-hex-digest>paths preserve ascendingkeyDigestorder.src/domain/services/CacheSetRegistry.js (1)
1-89: LGTM!src/domain/services/RootSet.js (1)
28-31: LGTM!Also applies to: 112-112, 124-128, 201-201
src/domain/services/RootSetMetadataCodec.js (1)
17-22: LGTM!Also applies to: 36-36, 157-157
src/domain/services/RootSetPersistence.js (1)
30-34: LGTM!src/domain/value-objects/CacheKey.js (1)
11-62: LGTM!src/domain/value-objects/CollectionNamespace.js (1)
12-54: LGTM!src/domain/value-objects/CachePolicy.js (1)
13-59: LGTM!src/domain/value-objects/CacheSetRef.js (1)
12-40: LGTM!src/domain/value-objects/CacheHit.js (1)
13-93: LGTM!index.d.ts (1)
306-334: LGTM!Also applies to: 707-822, 1014-1051, 1100-1100
src/domain/services/BundleService.js (2)
88-105: LGTM!Also applies to: 204-333, 356-375, 438-499, 548-555, 720-726, 838-848
677-689: 🗄️ Data Integrity & IntegrationNo action needed
readObjectSizeis already implemented by the in-repo persistence adapters, and the port contract requires it.> Likely an incorrect or invalid review comment.test/unit/domain/services/BundleService.test.js (1)
197-264: LGTM!docs/API.md (1)
1076-1114: LGTM!Also applies to: 1404-1560, 2844-2850
src/domain/services/CacheMetadataCodec.js (1)
17-164: LGTM!src/domain/services/CacheCandidateHeap.js (1)
1-71: LGTM!src/domain/services/CacheIndex.js (1)
30-362: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 85-88: Move the “Git object metadata port” entry describing
GitPersistencePort.readObjectType() and readObjectSize() from the dated 6.1.0
section into Unreleased, preserving its wording and ensuring the 6.1.0
historical record no longer claims this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3b4651b-6619-477d-89e4-533d85cdb025
📒 Files selected for processing (9)
CHANGELOG.mddocs/API.mddocs/design/0047-application-storage-cache-boundary/witness/cache-set.mdindex.d.tssrc/domain/services/CachePolicyEnforcer.jssrc/infrastructure/adapters/GitPersistenceAdapter.jstest/integration/cache-set.test.jstest/unit/domain/services/CacheCandidateHeap.test.jstest/unit/infrastructure/adapters/GitPersistenceAdapter.readTree.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test-docker (deno)
- GitHub Check: test-docker (bun)
- GitHub Check: test-docker (node)
🧰 Additional context used
📓 Path-based instructions (2)
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Use
CHANGELOG.mdto record the historical truth of merged behavior
Files:
CHANGELOG.md
docs/design/**
📄 CodeRabbit inference engine (AGENTS.md)
Use
docs/design/directory for durable design contracts and proof plans
Files:
docs/design/0047-application-storage-cache-boundary/witness/cache-set.md
🧠 Learnings (2)
📚 Learning: 2026-02-28T19:21:13.982Z
Learnt from: flyingrobots
Repo: git-stunts/git-cas PR: 15
File: test/unit/domain/services/CasService.envelope.test.js:326-330
Timestamp: 2026-02-28T19:21:13.982Z
Learning: In fuzz tests for cryptographic operations, use a seeded PRNG (e.g., xorshift32) for control-flow variables such as plaintext size selection and recipient index selection to ensure reproducibility. Continue to use randomBytes() for cryptographic keys and nonces to preserve realistic randomness and avoid security anti-patterns. This guideline applies to test files that perform fuzz testing of crypto logic; implement a consistent seed setup (e.g., fixed seed in test initialization) and document the rationale to enable deterministic replays across runs.
Applied to files:
test/unit/domain/services/CacheCandidateHeap.test.jstest/unit/infrastructure/adapters/GitPersistenceAdapter.readTree.test.jstest/integration/cache-set.test.js
📚 Learning: 2026-03-30T19:53:48.000Z
Learnt from: flyingrobots
Repo: git-stunts/git-cas PR: 29
File: docs/design/TR-010-planning-index-consistency-review.md:121-131
Timestamp: 2026-03-30T19:53:48.000Z
Learning: In this repo, CHANGELOG.md entries should be user-facing and descriptive (bullet points) rather than cycle-ID headings (e.g., use a phrase like “Planning-index consistency review” instead of a “TR-010 — …” heading). When searching the changelog, don’t rely on grepping for cycle IDs (e.g., “TR-010”) because it may cause false negatives; search for the descriptive keywords/phrases from the bullet entries instead.
Applied to files:
CHANGELOG.md
🔇 Additional comments (10)
test/integration/cache-set.test.js (1)
74-83: LGTM!test/unit/infrastructure/adapters/GitPersistenceAdapter.readTree.test.js (1)
95-110: LGTM!Also applies to: 125-132
test/unit/domain/services/CacheCandidateHeap.test.js (1)
6-17: LGTM!docs/API.md (1)
1349-1351: LGTM!Also applies to: 1399-1403, 2845-2850
docs/design/0047-application-storage-cache-boundary/witness/cache-set.md (1)
11-12: LGTM!Also applies to: 199-218
src/infrastructure/adapters/GitPersistenceAdapter.js (3)
211-238: 🎯 Functional Correctness | 💤 Low valueDoctor-comment nit: "missing" case is documented, "ambiguous" case is not called out.
Git's batch-check protocol also emits
<object> ambiguousfor an unresolvable short SHA (distinct frommissing); the current code silently folds this into the genericfields.length !== 3→GIT_ERRORpath rather than a dedicated code. Given the codebase's stated fail-closed design goals, this still fails closed correctly — just with a less specific error code thanmissing. Low practical impact if all callers pass full-length OIDs (see the related comment on this function).
21-22: LGTM!Also applies to: 190-201
211-238: 🎯 Functional CorrectnessPotential abbreviated-OID regression
If any caller can pass a short hash here,
fields[0] !== oidwill reject a Git-resolvable object because%(objectname)returns the canonical full OID. The previouscat-file -t/-spath accepted abbreviated OIDs. If upstream always expands to full-length OIDs, ignore this.index.d.ts (1)
337-359: Past comment addressed.
CacheHit.logicalBytesis now typed asnumber, matchingCacheHitData, the constructor parameter, and the runtime non-negative-safe-integer validation inCacheHit.#assertValue.src/domain/services/CachePolicyEnforcer.js (1)
4-7: Past comment addressed.
createCacheState()now sourcesversion/accountingVersionfrom the sharedCACHE_METADATA_VERSION/CACHE_ACCOUNTING_VERSIONconstants instead of hardcoded literals, keeping the producer in sync withCacheMetadataCodecvalidation.Also applies to: 56-73
1 participant
Summary
CacheSetAPI for open/get/put/replace/remove/touch/sweep lifecycle managementmaxEntries,maxBytes, approximate-LRU, and pinned-entry policy without materializing target graphs during index scansLinked Issue
Design / Proof
docs/design/0047-application-storage-cache-boundary/application-storage-cache-boundary.mddocs/design/0047-application-storage-cache-boundary/witness/cache-set.mdThe witness includes source-pinned evidence for the public surface, lifecycle ordering, streaming bounds, retention behavior, concurrency, doctor/repair, self-review findings, CI root cause, and CodeRabbit follow-up.
The real-Git integration proof covers pre-anchor prunability, post-publish reachability, parentless RootSet generations, removal and eviction collectibility, pin survival, concurrent insertion, and single-winner guarded replacement.
Validation
pnpm lintpnpm test(1,829 passed; 2 skipped)GIT_STUNTS_DOCKER=1 pnpm test:integration(170 passed)NodeNextgit diff --checkRelease Impact