release: v17.0.0 follow-up substrate closeout#85
Merged
Conversation
📝 WalkthroughWalkthroughThis PR modernizes git-warp's adapter layer to use git-cas v6 streaming APIs for graph operations. It extracts blob and tree reading into ChangesV17 git-cas adapter parity modernization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Release Preflight
If you tag this commit as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/upgrade-v16-to-v17.ts (1)
37-47: ⚡ Quick winReplace positional booleans in
V16ToV17UpgradeArgswith a named options object.The constructor call site passes multiple booleans positionally, which is a boolean trap and easy to misuse.
Suggested refactor
+interface V16ToV17UpgradeArgsOptions { + readonly repo: string; + readonly graphNames: readonly string[]; + readonly dryRun: boolean; + readonly json: boolean; + readonly help: boolean; +} + export class V16ToV17UpgradeArgs { - constructor( - readonly repo: string, - readonly graphNames: readonly string[], - readonly dryRun: boolean, - readonly json: boolean, - readonly help: boolean, - ) { + readonly repo: string; + readonly graphNames: readonly string[]; + readonly dryRun: boolean; + readonly json: boolean; + readonly help: boolean; + + constructor(options: V16ToV17UpgradeArgsOptions) { + this.repo = options.repo; + this.graphNames = options.graphNames; + this.dryRun = options.dryRun; + this.json = options.json; + this.help = options.help; Object.freeze(this); } } @@ - return new V16ToV17UpgradeArgs(repo, graphNames, dryRun, json, help); + return new V16ToV17UpgradeArgs({ repo, graphNames, dryRun, json, help });As per coding guidelines, "Avoid boolean trap parameters; use named option objects or separate methods instead."
Also applies to: 130-130
🤖 Prompt for 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. In `@scripts/upgrade-v16-to-v17.ts` around lines 37 - 47, The constructor for V16ToV17UpgradeArgs exposes multiple positional boolean parameters (dryRun, json, help) which is a boolean-trap; change the constructor signature to accept a single options object (e.g., opts: { dryRun?: boolean; json?: boolean; help?: boolean }) or a typed interface, update properties on the class to read from that options object, freeze the instance as before, and then update all call sites (places creating new V16ToV17UpgradeArgs) to pass a single named options object instead of positional booleans; ensure the class still exposes readonly repo and graphNames and preserve Object.freeze(this).
🤖 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 `@scripts/upgrade-v16-to-v17.ts`:
- Around line 97-111: The parser accepts next argv tokens that are flags as
values for --repo and --graph; update the parsing around the arg/argv/i handling
so that after reading const value = argv[i + 1] you also reject it if it is
undefined or startsWith('-'), throwing V16ToV17UpgradeArgumentError('--repo
requires a path') or V16ToV17UpgradeArgumentError('--graph requires a graph
name') respectively; ensure you check this for both the repo assignment (repo =
value) and graphNames.push(value) branches so flag-like tokens (e.g.
'--dry-run') are not treated as values.
In `@test/unit/domain/services/CheckpointService.test.ts`:
- Around line 1032-1075: This test adds anchor-focused coverage into an already
oversized test file; extract the it('preserves legacy raw blob content anchors
when creating a checkpoint'...) case into a new dedicated spec (e.g.,
CheckpointService.anchors.test.ts), copy/import the required helpers and mocks
(createCheckpointEnvelope, createEmptyState, makeSequentialOid, makeOid,
encodePropKeyV5, CONTENT_PROPERTY_KEY, createFrontier, updateFrontier,
mockPersistence, crypto) so it is self-contained, keep the same assertions, and
adjust mock setup and imports to match the new file; remove the test from the
original file to keep it under the 800 LOC limit and run the test suite to
ensure imports/mocks are correct.
---
Nitpick comments:
In `@scripts/upgrade-v16-to-v17.ts`:
- Around line 37-47: The constructor for V16ToV17UpgradeArgs exposes multiple
positional boolean parameters (dryRun, json, help) which is a boolean-trap;
change the constructor signature to accept a single options object (e.g., opts:
{ dryRun?: boolean; json?: boolean; help?: boolean }) or a typed interface,
update properties on the class to read from that options object, freeze the
instance as before, and then update all call sites (places creating new
V16ToV17UpgradeArgs) to pass a single named options object instead of positional
booleans; ensure the class still exposes readonly repo and graphNames and
preserve Object.freeze(this).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98c9c337-fb8a-4805-baa0-76d66379fe35
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
CHANGELOG.mddocs/method/backlog/up-next/INFRA_git-cas-vault-encryption.mddocs/method/backlog/up-next/INFRA_policy-as-port-streaming-resilience.mddocs/method/backlog/v17.0.0/INFRA_git-cas-adapter-parity.mddocs/method/backlog/v17.0.0/INFRA_substrate-upgrade-tool.mddocs/migrations/v17.0.0.mddocs/releases/v17.0.0/README.mddocs/specs/CONTENT_ATTACHMENT.mdpackage.jsonscripts/upgrade-v16-to-v17.tssrc/domain/services/state/checkpointCreate.tssrc/domain/services/state/checkpointHelpers.tssrc/infrastructure/adapters/CliJsonFormatterAdapter.tssrc/infrastructure/adapters/GitCasGraphReaderAdapter.tssrc/infrastructure/adapters/GitGraphAdapter.tstest/helpers/WarpGraphTestRepositories.tstest/integration/WarpGraph.integration.test.tstest/integration/api/helpers/setup.tstest/integration/domain/orset/trie/TrieCursor.flush.integration.test.tstest/integration/infrastructure/adapters/GitTrieStoreAdapter.integration.test.tstest/runtime/deno/deno.jsontest/runtime/deno/helpers.tstest/unit/domain/services/CheckpointService.test.tstest/unit/domain/services/GitGraphAdapter.test.tstest/unit/infrastructure/adapters/CliJsonFormatterAdapter.test.tstest/unit/infrastructure/adapters/GitGraphAdapter.coverage.test.tstest/unit/infrastructure/adapters/GitGraphAdapter.gitCasPersistence.test.tstest/unit/scripts/streaming-memory-audit-closeout.test.tstest/unit/scripts/uniform-git-cas-closeout.test.tstest/unit/scripts/v16-to-v17-upgrade.test.tstsconfig.publish.json
Release Preflight
If you tag this commit as |
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/unit/domain/services/CheckpointService.anchors.test.ts (3)
120-122: ⚡ Quick winConsider making the fallback to
'tree'explicit or failing for unmocked OIDs.The silent default to
'tree'when an OID is not found could hide incomplete test setup. If a test forgets to callsetObjectTypefor a content anchor, it will silently default to'tree'rather than failing with a clear error.🛡️ Option 1: Add comment documenting the default behavior
async readObjectType(oid: string): Promise<ContentAnchorObjectType> { + // Default to 'tree' for unmocked OIDs to match git-cas CAS graph behavior return this._objectTypes.get(oid) ?? 'tree'; }🛡️ Option 2: Throw error for unmocked OIDs
async readObjectType(oid: string): Promise<ContentAnchorObjectType> { - return this._objectTypes.get(oid) ?? 'tree'; + const objectType = this._objectTypes.get(oid); + if (objectType === undefined) { + throw new AnchorCheckpointPersistenceError(`Object type not mocked for OID: ${oid}`); + } + return objectType; }🤖 Prompt for 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. In `@test/unit/domain/services/CheckpointService.anchors.test.ts` around lines 120 - 122, The test helper currently returns a silent default 'tree' in readObjectType when an OID is missing; change this so unmocked OIDs fail loudly: in the method readObjectType use this._objectTypes to check for presence of oid and if not present throw a descriptive Error (e.g. "unmocked OID <oid> in readObjectType") instead of returning 'tree'; this will force tests to call setObjectType for anchors and avoid hidden test setup bugs.
157-163: ⚡ Quick winConsider using order-independent assertions for better test resilience.
The current assertion expects envelope tree entries in a specific order. If the implementation changes the order (e.g., sorting differently), the test will break even though the functionality is correct.
🧪 Use arrayContaining for order-independent matching
expect(persistence.envelopeTreeEntries()).toEqual([ - `100644 blob ${legacyBlobOid}\t_content_${legacyBlobOid}`, - `040000 tree ${casTreeOid}\t_content_${casTreeOid}`, - expect.stringContaining('\tappliedVV.cbor'), - expect.stringContaining('\tfrontier.cbor'), - expect.stringContaining('\tstate'), -]); + expect.arrayContaining([ + `100644 blob ${legacyBlobOid}\t_content_${legacyBlobOid}`, + `040000 tree ${casTreeOid}\t_content_${casTreeOid}`, + expect.stringContaining('\tappliedVV.cbor'), + expect.stringContaining('\tfrontier.cbor'), + expect.stringContaining('\tstate'), + ]) +);Alternatively, verify the array length and check each entry individually:
-expect(persistence.envelopeTreeEntries()).toEqual([ - `100644 blob ${legacyBlobOid}\t_content_${legacyBlobOid}`, - `040000 tree ${casTreeOid}\t_content_${casTreeOid}`, - expect.stringContaining('\tappliedVV.cbor'), - expect.stringContaining('\tfrontier.cbor'), - expect.stringContaining('\tstate'), -]); +const entries = persistence.envelopeTreeEntries(); +expect(entries).toHaveLength(5); +expect(entries).toContain(`100644 blob ${legacyBlobOid}\t_content_${legacyBlobOid}`); +expect(entries).toContain(`040000 tree ${casTreeOid}\t_content_${casTreeOid}`); +expect(entries.some(e => e.includes('\tappliedVV.cbor'))).toBe(true); +expect(entries.some(e => e.includes('\tfrontier.cbor'))).toBe(true); +expect(entries.some(e => e.includes('\tstate'))).toBe(true);🤖 Prompt for 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. In `@test/unit/domain/services/CheckpointService.anchors.test.ts` around lines 157 - 163, Change the brittle ordered assertion over persistence.envelopeTreeEntries() to an order-independent check: replace the toEqual([...]) usage with an assertion that uses expect.arrayContaining([...]) (or expect(persistence.envelopeTreeEntries()).toEqual(expect.arrayContaining([...]))), and additionally assert the array length if you need to guarantee no extra entries; keep the same matchers (string literals and expect.stringContaining('\tappliedVV.cbor'), expect.stringContaining('\tfrontier.cbor'), expect.stringContaining('\tstate')) so the test validates presence regardless of order.
50-56: ⚡ Quick winDocument why index
[1]retrieves the envelope tree.The hardcoded index assumes the envelope tree is always the second tree written, but this assumption is implicit. If the write order changes in
createCheckpointEnvelope, this test will break silently or return incorrect data.📝 Add clarifying comment
envelopeTreeEntries(): readonly string[] { + // Index 1: createCheckpointEnvelope writes state tree first (index 0), then envelope tree (index 1) const entries = this.writtenTrees[1];🤖 Prompt for 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. In `@test/unit/domain/services/CheckpointService.anchors.test.ts` around lines 50 - 56, The test currently uses a magic index (this.writtenTrees[1]) in envelopeTreeEntries to assume the envelope tree is always the second tree written; add a concise clarifying comment above the envelopeTreeEntries() method that states why index 1 corresponds to the envelope tree (referencing the write order established by createCheckpointEnvelope and the structure of writtenTrees), and consider noting that if createCheckpointEnvelope's write order changes the test must be updated (or suggest replacing the magic index with a named constant or a lookup by tree type such as an "ENVELOPE_TREE_INDEX" or scanning writtenTrees for the envelope tree).
🤖 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.
Nitpick comments:
In `@test/unit/domain/services/CheckpointService.anchors.test.ts`:
- Around line 120-122: The test helper currently returns a silent default 'tree'
in readObjectType when an OID is missing; change this so unmocked OIDs fail
loudly: in the method readObjectType use this._objectTypes to check for presence
of oid and if not present throw a descriptive Error (e.g. "unmocked OID <oid> in
readObjectType") instead of returning 'tree'; this will force tests to call
setObjectType for anchors and avoid hidden test setup bugs.
- Around line 157-163: Change the brittle ordered assertion over
persistence.envelopeTreeEntries() to an order-independent check: replace the
toEqual([...]) usage with an assertion that uses expect.arrayContaining([...])
(or
expect(persistence.envelopeTreeEntries()).toEqual(expect.arrayContaining([...]))),
and additionally assert the array length if you need to guarantee no extra
entries; keep the same matchers (string literals and
expect.stringContaining('\tappliedVV.cbor'),
expect.stringContaining('\tfrontier.cbor'), expect.stringContaining('\tstate'))
so the test validates presence regardless of order.
- Around line 50-56: The test currently uses a magic index
(this.writtenTrees[1]) in envelopeTreeEntries to assume the envelope tree is
always the second tree written; add a concise clarifying comment above the
envelopeTreeEntries() method that states why index 1 corresponds to the envelope
tree (referencing the write order established by createCheckpointEnvelope and
the structure of writtenTrees), and consider noting that if
createCheckpointEnvelope's write order changes the test must be updated (or
suggest replacing the magic index with a named constant or a lookup by tree type
such as an "ENVELOPE_TREE_INDEX" or scanning writtenTrees for the envelope
tree).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7080a01-5794-4a2a-822e-478bf993f3d8
📒 Files selected for processing (6)
CHANGELOG.mdscripts/upgrade-v16-to-v17.tstest/unit/domain/services/CheckpointService.anchors.test.tstest/unit/infrastructure/adapters/GitGraphAdapter.coverage.test.tstest/unit/infrastructure/adapters/GitGraphAdapter.listRefs.test.tstest/unit/scripts/v16-to-v17-upgrade.test.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unit/scripts/v16-to-v17-upgrade.test.ts
- scripts/upgrade-v16-to-v17.ts
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
This is the tiny v17 closure PR after PR #84 merged. It lands the local substrate/runtime commits that were still ahead of
origin/main, so v17 does not split into archaeology about which substrate shape actually shipped.Included substrate/runtime commits:
b99a9b4cfix(checkpoint): preserve legacy blob content anchorsb62d51ffchore: upgrade git stunts substrate packagesad673d39feat: add v16 to v17 upgrade utilitycb4c5802feat: route git graph reads through git-cas streamsAdditional CI closure commit:
80a3d19atest: cover git graph object type readsDeferred from this PR:
Status
v17 PR #84 is merged, but release closeout is blocked on landing these substrate/runtime commits. After this PR lands, validate on
main, then tag and publish v17.Validation
Local branch validation:
npm run lintnpm run typechecknpx vitest run test/unit/scripts/v16-to-v17-upgrade.test.ts test/unit/infrastructure/adapters/CliJsonFormatterAdapter.test.ts test/unit/domain/services/CheckpointService.test.ts test/unit/domain/services/GitGraphAdapter.test.ts test/unit/infrastructure/adapters/GitGraphAdapter.coverage.test.ts test/unit/infrastructure/adapters/GitGraphAdapter.gitCasPersistence.test.ts test/unit/scripts/uniform-git-cas-closeout.test.ts— 7 files / 191 testsnpm run buildnpm run lint:mdnpm exec vitest -- run test/unit/infrastructure/adapters/GitGraphAdapter.coverage.test.ts test/unit/infrastructure/adapters/GitGraphAdapter.listRefs.test.ts— 2 files / 98 testsnpm run test:coverage:ci— 440 files / 6783 tests, line coverage 91.87% against 91.74% thresholdnpm run test:local— 440 files / 6783 testsGitHub validation is green on PR #85:
Summary by CodeRabbit
Release Notes
Documentation
Chores
@git-stunts/alfred,@git-stunts/git-cas, and@git-stunts/plumbingdependencies.npm run upgradecommand for automated v16→v17 graph repository migration with dry-run support.