feat: M4 Compass — lifecycle management #4
Conversation
Add readManifest(), deleteAsset(), and findOrphanedChunks() to CasService with facade pass-throughs. Includes 42 new unit tests and MANIFEST_NOT_FOUND / GIT_ERROR codes.
Break large describe callbacks into smaller blocks to satisfy max-lines-per-function (50) and max-nested-callbacks (3) rules. Remove unused variable in deleteAsset tests.
📝 WalkthroughWalkthroughThis PR introduces three new manifest-based methods to CasService: Changes
Sequence DiagramsequenceDiagram
actor Caller
participant Store as ContentAddressableStore
participant Service as CasService
participant Git as Git<br/>Persistence
participant Codec as JsonCodec
Caller->>Store: readManifest({ treeOid })
Store->>Service: readManifest({ treeOid })
Service->>Git: readTree(treeOid)
Git-->>Service: tree entries
alt manifest found
Service->>Git: readBlob(manifestBlobOid)
Git-->>Service: blob data
Service->>Codec: decode(blob)
Codec-->>Service: JSON manifest
Service->>Service: validate(manifest)
Service-->>Store: Manifest instance
Store-->>Caller: Manifest instance
else manifest not found
Service-->>Store: CasError(MANIFEST_NOT_FOUND)
Store-->>Caller: error
else Git error
Service-->>Store: CasError(GIT_ERROR)
Store-->>Caller: error
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ROADMAP.md`:
- Around line 131-134: The table rows (e.g., the lines starting with "v1.1.0",
"v1.2.0", "v1.3.0", "v1.4.0") have an extra cell for the checkmark, causing a
5th column while the header is "Version | Milestone | Codename | Theme"; fix
each row by removing the extra pipe and placing the checkmark inside the
existing Theme cell (so Theme contains "Foundation hardening ✅", "File retrieval
round trip + CLI ✅", etc.), ensuring each row has exactly four cells to match
the header.
🧹 Nitpick comments (6)
src/domain/services/CasService.js (1)
320-333:findOrphanedChunksprocesses trees sequentially — consider naming clarity.The method name says "orphaned" but it actually returns referenced chunks and a total count. The caller must diff against the full set of blobs in the ODB to determine truly orphaned chunks. The ROADMAP contract matches this behavior, but the name may mislead consumers into thinking it returns the orphaned set itself.
This is a minor naming concern — the JSDoc and return type are accurate, so consumers reading docs won't be confused.
test/unit/domain/services/CasService.findOrphanedChunks.test.js (2)
436-451: Double invocation of the SUT in error assertions — consider consolidating.The pattern of
rejects.toThrow(CasError)followed by a separatetry/catchblock callsfindOrphanedChunkstwice against the mocks. This works here because the mocks aren't usingOncevariants, but it's fragile and wasteful. You can combine both checks in a single invocation:Suggested consolidation
- await expect( - service.findOrphanedChunks({ treeOids: ['tree-missing'] }), - ).rejects.toThrow(CasError); - - try { - await service.findOrphanedChunks({ treeOids: ['tree-missing'] }); - } catch (err) { - expect(err.code).toBe('MANIFEST_NOT_FOUND'); - expect(err.message).toContain('No manifest entry'); - expect(err.meta.treeOid).toBe('tree-missing'); - } + const err = await service.findOrphanedChunks({ treeOids: ['tree-missing'] }) + .catch((e) => e); + + expect(err).toBeInstanceOf(CasError); + expect(err.code).toBe('MANIFEST_NOT_FOUND'); + expect(err.message).toContain('No manifest entry'); + expect(err.meta.treeOid).toBe('tree-missing');This same pattern applies to the other error tests in this file (lines 465–497, 511–527, 529–548).
1-6: Test helpers:digestOfimported here vs locally definedsha256DigestindeleteAssettests.This file imports
digestOffrom../../../helpers/crypto.js, whileCasService.deleteAsset.test.jsdefines its own localsha256Digestfunction that does the same thing. Consider using the shared helper consistently across all three test files.test/unit/domain/services/CasService.deleteAsset.test.js (2)
8-13: Localsha256Digestduplicates the shareddigestOfhelper.This file defines its own
sha256Digestfunction (identical todigestOffromtest/helpers/crypto.js). ThefindOrphanedChunkstest file already imports the shared helper. Consider using the same import here for consistency.Suggested change
-import { createHash } from 'node:crypto'; +import { digestOf } from '../../../helpers/crypto.js';Then replace
sha256Digest(...)calls withdigestOf(...).
228-255: Same double-invocation pattern in error assertions as noted in thefindOrphanedChunkstests.The
rejects.toThrow+try/catchpattern calls the SUT twice. Additionally, thetry/catchblocks here lackexpect.unreachable()(unlike thereadManifesttests), so if the method unexpectedly resolves on the second call, the property assertions silently don't run. The firstrejects.toThrowassertion provides a safety net, but a single-invocation pattern would be cleaner and more robust.test/unit/domain/services/CasService.readManifest.test.js (1)
9-11: LocaldigestOfduplicates the shared helper.Same function exists in
test/helpers/crypto.jsand is already imported infindOrphanedChunkstests. Consider using the shared import here too.
| | v1.1.0 | M1 | Bedrock | Foundation hardening | ✅ | | ||
| | v1.2.0 | M2 | Boomerang| File retrieval round trip + CLI | ✅ | | ||
| | v1.3.0 | M3 | Launchpad| CI/CD pipeline | ✅ | | ||
| | v1.4.0 | M4 | Compass | Lifecycle management | ✅ | |
There was a problem hiding this comment.
Table column count mismatch — the ✅ creates a 5th cell outside the header.
The header defines 4 columns (Version | Milestone | Codename | Theme) but lines 131–134 each have 5 cells. Strict Markdown renderers will drop the extra cell or display it incorrectly. Move the checkmark into the existing Theme column instead.
Proposed fix
-| v1.1.0 | M1 | Bedrock | Foundation hardening | ✅ |
-| v1.2.0 | M2 | Boomerang| File retrieval round trip + CLI | ✅ |
-| v1.3.0 | M3 | Launchpad| CI/CD pipeline | ✅ |
-| v1.4.0 | M4 | Compass | Lifecycle management | ✅ |
+| v1.1.0 | M1 | Bedrock | Foundation hardening ✅ |
+| v1.2.0 | M2 | Boomerang| File retrieval round trip + CLI ✅ |
+| v1.3.0 | M3 | Launchpad| CI/CD pipeline ✅ |
+| v1.4.0 | M4 | Compass | Lifecycle management ✅ |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | v1.1.0 | M1 | Bedrock | Foundation hardening | ✅ | | |
| | v1.2.0 | M2 | Boomerang| File retrieval round trip + CLI | ✅ | | |
| | v1.3.0 | M3 | Launchpad| CI/CD pipeline | ✅ | | |
| | v1.4.0 | M4 | Compass | Lifecycle management | ✅ | | |
| | v1.1.0 | M1 | Bedrock | Foundation hardening ✅ | | |
| | v1.2.0 | M2 | Boomerang| File retrieval round trip + CLI ✅ | | |
| | v1.3.0 | M3 | Launchpad| CI/CD pipeline ✅ | | |
| | v1.4.0 | M4 | Compass | Lifecycle management ✅ | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 131-131: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 132-132: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 133-133: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
[warning] 134-134: Table column count
Expected: 4; Actual: 5; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 Prompt for AI Agents
In `@ROADMAP.md` around lines 131 - 134, The table rows (e.g., the lines starting
with "v1.1.0", "v1.2.0", "v1.3.0", "v1.4.0") have an extra cell for the
checkmark, causing a 5th column while the header is "Version | Milestone |
Codename | Theme"; fix each row by removing the extra pipe and placing the
checkmark inside the existing Theme cell (so Theme contains "Foundation
hardening ✅", "File retrieval round trip + CLI ✅", etc.), ensuring each row has
exactly four cells to match the header.
1 participant
Summary
readManifest({ treeOid })— reads a Git tree, locates and decodes the manifest, returns a validated Manifest value objectdeleteAsset({ treeOid })— returns logical deletion metadata ({ slug, chunksOrphaned }) without destructive Git opsfindOrphanedChunks({ treeOids })— aggregates referenced chunk blob OIDs across assets, returns{ referenced, total }ContentAddressableStorefor all three methodsMANIFEST_NOT_FOUND,GIT_ERRORTest plan
Summary by CodeRabbit
New Features
Documentation