feat: add opaque application asset handles#62
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)docs/design/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds opaque asset handles, streaming asset ingestion, generation-scoped retention witnesses, allowlisted compare-and-swap publications, multi-parent Git commits, SHA-256-compatible ref updates, public declarations, documentation, and tests. ChangesApplication storage
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant ContentAddressableStore
participant AssetService
participant RetentionService
participant PublicationService
Application->>ContentAddressableStore: assets.put(source, metadata)
ContentAddressableStore->>AssetService: put(options)
AssetService-->>Application: StagedAsset with AssetHandle
Application->>ContentAddressableStore: retention.retain(handle, root)
ContentAddressableStore->>RetentionService: retain(options)
RetentionService-->>Application: RetentionWitness
Application->>ContentAddressableStore: publications.commit(root, commit, ref)
ContentAddressableStore->>PublicationService: commit(options)
PublicationService-->>Application: publication result with RetentionWitness
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Self-Review and Code Lawyer VerdictVerdict: ready for external review. No unresolved findings. Reviewed the complete Corrections made before submission:
The real-Git fixture proves prune candidacy, RootSet anchoring and replacement, exact witness edges, ordered publication, stale-head conflict behavior, mirror portability, and missing-graph failure. [cite: Residual constraints are explicit in the committed witness: manifest metadata remains materialized by the existing model, explicit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/domain/services/AssetService.js`:
- Around line 103-111: Update `#assertChunkGraph` to validate all unique
chunk.blob values concurrently instead of awaiting `#assertObjectType` inside the
loop. Continue deduplicating blobs with the existing seen Set, collect one
validation promise per unique blob, and await their completion before returning.
In `@src/domain/services/PublicationService.js`:
- Around line 147-212: Update `#authorizeRef` and `#normalizePrefixes` to reject
Git-managed namespaces, including refs/heads/, refs/tags/, refs/notes/, and
refs/remotes/, in addition to refs/cas/. Ensure both configured prefixes and
candidate refs are blocked consistently while preserving allowlisting for valid
application-owned namespaces.
In `@src/domain/value-objects/RetentionWitness.js`:
- Around line 94-107: Extract the duplicated canonical UTC timestamp validation
from RetentionWitness.#assertTimestamp and StagedAsset into a shared
assertCanonicalTimestamp validator. Update both value objects to call the helper
while preserving each class’s existing invalid-error factory, message, and
observedAt payload; remove the duplicated private validation methods.
In `@test/unit/domain/services/RetentionService.test.js`:
- Around line 54-132: Add a negative-path test in the RetentionService tests
that calls retain() with a root.ref outside the refs/cas/rootsets/* namespace,
and assert it rejects before any root-set ref is mutated. Use the existing
makeServices setup and verify the relevant root-set state remains unchanged
after rejection, covering the `#assertRoot` validation path.
In `@test/unit/domain/value-objects/ApplicationStorageResults.test.js`:
- Around line 12-43: Add a separate “StagedAsset validation” test block covering
the constructor’s slug, filename, size, and observedAt validation branches with
invalid overrides, and assert each construction throws an error whose code is
HANDLE_INVALID. Reuse the existing valid fixture values and parameterize the
cases to keep the tests concise.
- Around line 88-107: Refactor the invalid-evidence test setup around the `root`
and `options` objects so `override.root` cannot overwrite the fully constructed
`root`; preserve other override fields while removing the confusing
`options.root = root` double-write. Keep each test’s intended root overrides and
invalid evidence cases unchanged.
🪄 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: 8ddb3f62-013e-4538-89ab-ce7f3e3f4a46
📒 Files selected for processing (28)
BEARING.mdCHANGELOG.mdGUIDE.mdREADME.mddocs/API.mddocs/design/0047-application-storage-cache-boundary/application-storage-cache-boundary.mddocs/design/0047-application-storage-cache-boundary/witness/asset-handles.mdindex.d.tsindex.jssrc/domain/errors/Codes.jssrc/domain/services/AssetService.jssrc/domain/services/PublicationService.jssrc/domain/services/RetentionService.jssrc/domain/value-objects/AssetHandle.jssrc/domain/value-objects/RetentionWitness.jssrc/domain/value-objects/StagedAsset.jssrc/infrastructure/adapters/GitRefAdapter.jssrc/ports/GitRefPort.jstest/helpers/MemoryRefAdapter.jstest/integration/application-storage.test.jstest/unit/domain/services/AssetService.test.jstest/unit/domain/services/PublicationService.test.jstest/unit/domain/services/RetentionService.test.jstest/unit/domain/value-objects/ApplicationStorageResults.test.jstest/unit/domain/value-objects/AssetHandle.test.jstest/unit/facade/ContentAddressableStore.application-storage.test.jstest/unit/infrastructure/adapters/GitRefAdapter.test.jstest/unit/types/declaration-accuracy.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test-docker (node)
- GitHub Check: test-docker (bun)
- GitHub Check: test-docker (deno)
🧰 Additional context used
📓 Path-based instructions (5)
BEARING.md
📄 CodeRabbit inference engine (AGENTS.md)
Use
BEARING.mdto document current execution gravity and active tensions
Files:
BEARING.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/asset-handles.mddocs/design/0047-application-storage-cache-boundary/application-storage-cache-boundary.md
GUIDE.md
📄 CodeRabbit inference engine (AGENTS.md)
Use
GUIDE.mdfor orientation and productive-fast path documentation
Files:
GUIDE.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/value-objects/AssetHandle.test.jstest/unit/types/declaration-accuracy.test.jstest/unit/facade/ContentAddressableStore.application-storage.test.jstest/unit/domain/services/AssetService.test.jstest/unit/infrastructure/adapters/GitRefAdapter.test.jstest/unit/domain/value-objects/ApplicationStorageResults.test.jstest/unit/domain/services/RetentionService.test.jstest/integration/application-storage.test.jstest/unit/domain/services/PublicationService.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/helpers/MemoryRefAdapter.js
[warning] 5-5: Avoid SHA1 security protocol
Context: createHash('sha1')
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm (SHA-1).
(avoid-crypto-sha1)
[warning] 5-5: Do not use weak hash functions (MD5/SHA1)
Context: createHash('sha1')
Note: [CWE-328] Use of Weak Hash.
(insecure-hash)
test/integration/application-storage.test.js
[warning] 8-8: 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 (36)
src/domain/services/RetentionService.js (2)
1-36: LGTM!Also applies to: 64-86, 102-119
37-62: 🗄️ Data Integrity & IntegrationRemove this comment.
root.refis rejected byRootSetRefinRootSet/RootSetPersistencebefore any write occurs, andpolicyis validated byRootSetMetadataCodec.normalizeEntries()beforerootSet.put().> Likely an incorrect or invalid review comment.test/unit/domain/services/RetentionService.test.js (1)
1-53: LGTM!Also applies to: 133-133
test/integration/application-storage.test.js (1)
1-201: LGTM!CHANGELOG.md (2)
18-22: LGTM!Also applies to: 28-38
23-27: 🗄️ Data Integrity & IntegrationNo change needed The publication result includes a
witnessfield created fromRetentionWitness, so this changelog entry matches the current behavior.GUIDE.md (2)
45-122: LGTM!Also applies to: 129-166
123-128: 🎯 Functional Correctness
AssetHandle.parse()is valid hereAssetHandle.from()delegates string inputs toAssetHandle.parse(), so this example matches the implementation.> Likely an incorrect or invalid review comment.README.md (1)
26-28: LGTM!Also applies to: 63-82, 94-94, 113-115
docs/design/0047-application-storage-cache-boundary/application-storage-cache-boundary.md (1)
181-181: LGTM!Also applies to: 797-799
docs/design/0047-application-storage-cache-boundary/witness/asset-handles.md (1)
1-136: LGTM!src/domain/services/PublicationService.js (2)
1-297: LGTM!
80-99: 🎯 Functional CorrectnessParent validation is already enforced.
git rev-list --parents -n 1 <oid>can return an empty string for tree/blob OIDs, butGitRefAdapter.resolveParents()throws when no commit OID is parsed, so#normalizeParents()does not accept non-commit parents.> Likely an incorrect or invalid review comment.src/infrastructure/adapters/GitRefAdapter.js (2)
91-104: LGTM! The multi-parent-pconstruction and hash-width-agnostic zero-OID sentinel are both correct and match the accompanying tests.Also applies to: 114-122
14-15: 📐 Maintainability & Code QualityNo external
GIT_NULL_OIDconsumersGIT_NULL_OIDno longer appears anywhere in the repo, andupdateRef()still preservesexpectedOldOid: nullby translating it to the zero OID inline.> Likely an incorrect or invalid review comment.src/ports/GitRefPort.js (1)
34-45: LGTM!test/helpers/MemoryRefAdapter.js (1)
5-15: LGTM! Multi-parent OID hashing and storage correctly mirror the real adapter's ordered-parent support. The SHA-1 static-analysis warning is a false positive here — this is a test-only in-memory OID generator, not a security-sensitive hash.Also applies to: 53-61
test/unit/domain/services/PublicationService.test.js (1)
1-172: LGTM! Thorough coverage of witness shape, namespace specificity, ordered parents, and CAS/validation failure modes; assertions correctly match thePublicationServiceimplementation.test/unit/infrastructure/adapters/GitRefAdapter.test.js (1)
95-107: LGTM!BEARING.md (1)
65-69: LGTM! Accurately reflects the shipped application-storage capabilities and fits the file's existing "Current State" documentation pattern. Based on coding guidelines,BEARING.md: "UseBEARING.mdto document current execution gravity and active tensions."Source: Coding guidelines
src/domain/errors/Codes.js (2)
33-40: LGTM!
10-14: 🎯 Functional CorrectnessNo action: these handle error codes are already consumed downstream AssetService.js and PublicationService.js reference
HANDLE_TARGET_TYPE_MISMATCH, and AssetService.js also usesHANDLE_CODEC_MISMATCHandHANDLE_TARGET_MISSING.> Likely an incorrect or invalid review comment.src/domain/value-objects/AssetHandle.js (2)
1-27: LGTM!Also applies to: 58-163
29-47: 🎯 Functional CorrectnessDrop this concern:
Oid.fromonly accepts 40- or 64-character hex strings, sohashAlgorithmcannot be misclassified from a valid OID length.> Likely an incorrect or invalid review comment.src/domain/value-objects/RetentionWitness.js (1)
1-93: LGTM!Also applies to: 109-112
test/unit/domain/value-objects/AssetHandle.test.js (1)
1-55: LGTM!test/unit/facade/ContentAddressableStore.application-storage.test.js (1)
1-36: LGTM!test/unit/types/declaration-accuracy.test.js (1)
76-90: 🎯 Functional CorrectnessNo change needed. The asserted class declarations, readonly capability properties, and optional array fields are all present in
index.d.ts.src/domain/value-objects/StagedAsset.js (1)
1-74: LGTM!index.d.ts (2)
44-152: LGTM!Also applies to: 360-381, 407-407, 642-716
267-276: 🗄️ Data Integrity & IntegrationNo issue here.
GitRefAdapter.createCommit()prefersparentOidsand falls back toparentOid, so the overload is unambiguous.docs/API.md (1)
18-26: LGTM!Also applies to: 79-80, 911-1116, 2546-2564
index.js (1)
13-15: LGTM!Also applies to: 77-79, 111-151, 153-168, 170-182, 238-258, 279-302
src/domain/services/AssetService.js (2)
1-52: LGTM!Also applies to: 63-102, 113-143, 163-202
144-161: 🎯 Functional CorrectnessNo issue here
ErrorCodes.GIT_OBJECT_NOT_FOUNDis defined insrc/domain/errors/Codes.js, so this comparison does not collapse toundefined.> Likely an incorrect or invalid review comment.test/unit/domain/services/AssetService.test.js (1)
1-129: 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
`@docs/design/0047-application-storage-cache-boundary/witness/asset-handles.md`:
- Line 7: Replace the abbreviated Git object ID 3d6eaa4 in the witness artifact
and its cited revisions with the full repository object ID, preserving the
existing revision references and durable evidence structure.
🪄 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: b0168905-5940-4a54-b73a-8061f9592b06
📒 Files selected for processing (13)
CHANGELOG.mddocs/API.mddocs/design/0047-application-storage-cache-boundary/witness/asset-handles.mdsrc/domain/helpers/isCanonicalUtcTimestamp.jssrc/domain/services/AssetService.jssrc/domain/services/PublicationService.jssrc/domain/services/RetentionService.jssrc/domain/value-objects/RetentionWitness.jssrc/domain/value-objects/StagedAsset.jstest/unit/domain/services/AssetService.test.jstest/unit/domain/services/PublicationService.test.jstest/unit/domain/services/RetentionService.test.jstest/unit/domain/value-objects/ApplicationStorageResults.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- 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/asset-handles.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/AssetService.test.jstest/unit/domain/services/RetentionService.test.jstest/unit/domain/value-objects/ApplicationStorageResults.test.jstest/unit/domain/services/PublicationService.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/unit/domain/services/AssetService.test.js
[warning] 110-110: Avoid using the initial state variable in setState
Context: setTimeout(resolve, 5)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🔇 Additional comments (13)
CHANGELOG.md (1)
18-38: LGTM!docs/design/0047-application-storage-cache-boundary/witness/asset-handles.md (1)
70-77: LGTM!Also applies to: 119-145, 165-165
src/domain/helpers/isCanonicalUtcTimestamp.js (1)
1-11: LGTM!src/domain/value-objects/StagedAsset.js (1)
3-3: LGTM!Also applies to: 57-58
docs/API.md (1)
79-80: LGTM!Also applies to: 1064-1080, 2549-2558
src/domain/services/AssetService.js (1)
103-121: LGTM!test/unit/domain/services/AssetService.test.js (1)
1-1: LGTM!Also applies to: 16-25, 93-123
src/domain/value-objects/RetentionWitness.js (1)
3-3: LGTM!Also applies to: 95-103
test/unit/domain/value-objects/ApplicationStorageResults.test.js (1)
12-21: LGTM!Also applies to: 49-58, 105-112, 121-121
src/domain/services/RetentionService.js (1)
4-4: LGTM!Also applies to: 87-100
test/unit/domain/services/RetentionService.test.js (1)
1-1: LGTM!Also applies to: 34-40, 93-109
src/domain/services/PublicationService.js (1)
11-22: LGTM!Also applies to: 159-167, 200-223, 238-242
test/unit/domain/services/PublicationService.test.js (1)
10-22: LGTM!Also applies to: 132-184
1 participant
Linked Issue
Design / Proof
docs/design/0047-application-storage-cache-boundary/application-storage-cache-boundary.mddocs/design/0047-application-storage-cache-boundary/witness/asset-handles.mdtest/integration/application-storage.test.jsWhat Changed
AssetHandle,StagedAsset, and generation-scopedRetentionWitnessvalues.cas.assets.put/adopt/open,cas.retention.retain, and allowlisted compare-and-swapcas.publications.commit.Self-Review / Code Lawyer
not-establishedunder deduplication.Validation
pnpm lint pnpm test pnpm test:integration:node docker compose run --build --rm test-bun bunx vitest run test/integration/application-storage.test.js --no-file-parallelism docker compose run --build --rm test-deno deno run -A npm:vitest run test/integration/application-storage.test.js --no-file-parallelismRelease Impact
v6.2.0milestone; this PR completes slice Add opaque asset handles and retention witnesses #54.