Add v18 migration dry-run CLI and genesis equivalence evidence#103
Merged
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 36 minutes and 47 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughPR ChangesV18 Genesis Equivalence Proof & Dry-Run CLI
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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: 3
🧹 Nitpick comments (1)
test/unit/domain/migrations/GenesisEquivalenceFixtureCase.ts (1)
13-24: ⚡ Quick winAdd input validation to establish invariants.
The constructor uses
Object.freeze(this)and the class is described as "runtime-backed," suggesting it should follow domain patterns. However, it doesn't validate constructor inputs (e.g.,namecould be empty, readings could be malformed). Based on coding guidelines, runtime-backed types with invariants should validate in constructors.🛡️ Proposed validation
constructor( name: string, legacyReading: GenesisEquivalenceReading, migratedReading: GenesisEquivalenceReading, expectedResult: GenesisEquivalenceFixtureExpectedResult, ) { + if (typeof name !== 'string' || name.length === 0) { + throw new Error('GenesisEquivalenceFixtureCase requires non-empty name'); + } + if (!(legacyReading instanceof GenesisEquivalenceReading)) { + throw new Error('GenesisEquivalenceFixtureCase requires valid legacyReading'); + } + if (!(migratedReading instanceof GenesisEquivalenceReading)) { + throw new Error('GenesisEquivalenceFixtureCase requires valid migratedReading'); + } this.name = name; this.legacyReading = legacyReading; this.migratedReading = migratedReading; this.expectedResult = expectedResult; Object.freeze(this); }🤖 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/migrations/GenesisEquivalenceFixtureCase.ts` around lines 13 - 24, Add input validation in the GenesisEquivalenceFixtureCase constructor to enforce invariants before freezing: verify that `name` is a non-empty string, `legacyReading` and `migratedReading` are present and match the expected shape (use any existing validator/isX function for GenesisEquivalenceReading or check required properties), and that `expectedResult` is defined and valid; if any check fails, throw a clear Error/TypeError. Perform these checks at the start of the constructor in the `constructor(...)` of class `GenesisEquivalenceFixtureCase`, then assign fields and call `Object.freeze(this)` only after validation succeeds.
🤖 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/v18.0.0/migrations/graph-model/GraphModelMigrationDryRunCli.ts`:
- Around line 21-29: GraphModelMigrationDryRunCliArgs currently accepts a
positional boolean (helpRequested) which is error-prone; refactor the
constructor to take a single named options object (e.g. constructor(opts: {
requestPath?: string | null; manifestOutPath?: string | null; helpRequested?:
boolean })) and update the class fields to read from that options object while
preserving Object.freeze(this); then update all call sites that instantiate
GraphModelMigrationDryRunCliArgs to pass a descriptive object (instead of
positional args) and adjust any tests or usages that relied on the old
positional order.
In `@src/domain/migrations/GenesisEquivalenceMismatch.ts`:
- Around line 72-78: The function requireFactKind uses inline string literals
('node','edge','property','content-attachment') which violates the
no-magic-strings rule; replace these literals with the canonical named constants
(e.g., FACT_KIND_NODE, FACT_KIND_EDGE, FACT_KIND_PROPERTY,
FACT_KIND_CONTENT_ATTACHMENT or the equivalent enum members that define
GenesisEquivalenceReadingFactKind) so the checks read like kind !==
FACT_KIND_NODE && ...; update imports to pull those constants/enum members into
the file and ensure the function still returns
GenesisEquivalenceReadingFactKind.
In `@test/unit/scripts/v18-graph-model-migration-dry-run.test.ts`:
- Around line 11-68: Add unit tests in
test/unit/scripts/v18-graph-model-migration-dry-run.test.ts to exercise the
remaining CLI branches: (1) call
parseGraphModelMigrationDryRunCliArgs(['--help']) or
runGraphModelMigrationDryRunCli(['--help']) and assert it prints usage and exits
0, (2) invoke runGraphModelMigrationDryRunCli without the required --request (or
with a nonexistent request path) and assert it exits nonzero and reports the
missing/usage error, and (3) run runGraphModelMigrationDryRunCli with a valid
request but omit --manifest-out and assert the manifest is emitted to stdout
(and exitCode is 0); reference parseGraphModelMigrationDryRunCliArgs and
runGraphModelMigrationDryRunCli to locate where to add these specs.
---
Nitpick comments:
In `@test/unit/domain/migrations/GenesisEquivalenceFixtureCase.ts`:
- Around line 13-24: Add input validation in the GenesisEquivalenceFixtureCase
constructor to enforce invariants before freezing: verify that `name` is a
non-empty string, `legacyReading` and `migratedReading` are present and match
the expected shape (use any existing validator/isX function for
GenesisEquivalenceReading or check required properties), and that
`expectedResult` is defined and valid; if any check fails, throw a clear
Error/TypeError. Perform these checks at the start of the constructor in the
`constructor(...)` of class `GenesisEquivalenceFixtureCase`, then assign fields
and call `Object.freeze(this)` only after validation succeeds.
🪄 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: dda50332-f3f7-46c0-9f4d-c3b625553c66
📒 Files selected for processing (36)
CHANGELOG.mddocs/BEARING.mddocs/design/0189-v18-migration-dry-run-cli/v18-migration-dry-run-cli.mddocs/design/0190-v18-genesis-equivalence-nouns/v18-genesis-equivalence-nouns.mddocs/design/0191-v18-genesis-equivalence-fixtures/v18-genesis-equivalence-fixtures.mddocs/design/0192-v18-genesis-divergence-reporter/v18-genesis-divergence-reporter.mddocs/design/0193-v18-replan-with-migration-evidence/v18-replan-with-migration-evidence.mddocs/design/0194-v18-real-source-inventory-collector/v18-real-source-inventory-collector.mddocs/design/0195-v18-migration-operation-lowering/v18-migration-operation-lowering.mddocs/design/0196-v18-scratch-migration-writer/v18-scratch-migration-writer.mddocs/design/0197-v18-scratch-equivalence-gate/v18-scratch-equivalence-gate.mddocs/design/0198-v18-migration-finalization-safety/v18-migration-finalization-safety.mddocs/method/backlog/v18.0.0/INFRA_graph-model-migration-tool.mddocs/method/backlog/v18.0.0/README.mddocs/method/backlog/v18.0.0/TRUST_genesis-replay-equivalence.mdscripts/v18.0.0/migrations/graph-model/GraphModelMigrationDryRunCli.tsscripts/v18.0.0/migrations/graph-model/dry-run.tssrc/domain/migrations/GenesisDivergenceReport.tssrc/domain/migrations/GenesisDivergenceReporter.tssrc/domain/migrations/GenesisEquivalenceBoundary.tssrc/domain/migrations/GenesisEquivalenceComparisonBasis.tssrc/domain/migrations/GenesisEquivalenceMismatch.tssrc/domain/migrations/GenesisEquivalenceProof.tssrc/domain/migrations/GenesisEquivalenceProofFailure.tssrc/domain/migrations/GenesisEquivalenceProofResult.tssrc/domain/migrations/GenesisEquivalenceProofSuccess.tssrc/domain/migrations/GenesisEquivalenceProofSummary.tssrc/domain/migrations/GenesisEquivalenceReading.tssrc/domain/migrations/GenesisEquivalenceReadingFact.tssrc/infrastructure/adapters/GraphModelMigrationDryRunRequestJsonAdapter.tstest/unit/domain/migrations/GenesisDivergenceReporter.test.tstest/unit/domain/migrations/GenesisEquivalenceFixtureCase.tstest/unit/domain/migrations/GenesisEquivalenceFixtures.test.tstest/unit/domain/migrations/GenesisEquivalenceFixtures.tstest/unit/domain/migrations/GenesisEquivalenceProof.test.tstest/unit/scripts/v18-graph-model-migration-dry-run.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/design/0193-v18-replan-with-migration-evidence/v18-replan-with-migration-evidence.md (1)
121-123: 💤 Low valueClarify the cycle numbering discrepancy.
The text states "cycles 0199, then 0194 through 0197" which lists cycle 0199 out of numeric sequence. While the bearing_task numbers are in order (0199=46, 0194=47, etc.), the cycle numbers appear non-sequential. This could confuse readers expecting cycles to follow chronological order.
Consider adding a brief note explaining that cycle numbers reflect creation order while bearing_task numbers reflect execution sequence, or renumber the cycles to match their execution order for clarity.
🤖 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 `@docs/design/0193-v18-replan-with-migration-evidence/v18-replan-with-migration-evidence.md` around lines 121 - 123, Clarify the apparent cycle-numbering discrepancy in the sentence that currently reads "cycles 0199, then 0194 through 0197" by either renumbering to match execution order or adding a brief explanatory note; explicitly mention that cycle IDs (e.g., 0199, 0194–0197) reflect creation/order-of-recording while bearing_task numbers (e.g., 0199=46, 0194=47) reflect execution sequence, so the ordering is intentional and not a typo—update the phrase around "restored-fixture equivalence gate" accordingly to remove ambiguity.
🤖 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
`@docs/design/0193-v18-replan-with-migration-evidence/v18-replan-with-migration-evidence.md`:
- Around line 121-123: Clarify the apparent cycle-numbering discrepancy in the
sentence that currently reads "cycles 0199, then 0194 through 0197" by either
renumbering to match execution order or adding a brief explanatory note;
explicitly mention that cycle IDs (e.g., 0199, 0194–0197) reflect
creation/order-of-recording while bearing_task numbers (e.g., 0199=46, 0194=47)
reflect execution sequence, so the ordering is intentional and not a typo—update
the phrase around "restored-fixture equivalence gate" accordingly to remove
ambiguity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fc43368-da4a-4b3c-9d7e-3f8edf64143e
📒 Files selected for processing (12)
CHANGELOG.mddocs/BEARING.mddocs/design/0193-v18-replan-with-migration-evidence/v18-replan-with-migration-evidence.mddocs/design/0194-v18-real-source-inventory-collector/v18-real-source-inventory-collector.mddocs/design/0195-v18-migration-operation-lowering/v18-migration-operation-lowering.mddocs/design/0196-v18-scratch-migration-writer/v18-scratch-migration-writer.mddocs/design/0197-v18-scratch-equivalence-gate/v18-scratch-equivalence-gate.mddocs/design/0198-v18-migration-finalization-safety/v18-migration-finalization-safety.mddocs/design/0199-v18-v17-golden-graph-fixtures/v18-v17-golden-graph-fixtures.mddocs/method/backlog/v18.0.0/INFRA_graph-model-migration-tool.mddocs/method/backlog/v18.0.0/README.mddocs/method/backlog/v18.0.0/TRUST_genesis-replay-equivalence.md
✅ Files skipped from review due to trivial changes (7)
- docs/design/0195-v18-migration-operation-lowering/v18-migration-operation-lowering.md
- docs/design/0197-v18-scratch-equivalence-gate/v18-scratch-equivalence-gate.md
- docs/method/backlog/v18.0.0/README.md
- docs/design/0198-v18-migration-finalization-safety/v18-migration-finalization-safety.md
- docs/design/0196-v18-scratch-migration-writer/v18-scratch-migration-writer.md
- CHANGELOG.md
- docs/BEARING.md
Release Preflight
If you tag this commit as |
Member
Author
Resolved the corresponding review threads after pushing the update. |
Release Preflight
If you tag this commit as |
Release Preflight
If you tag this commit as |
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
ADR Checks
Verification
Coverage gate passed locally after review fixes: 498 test files, 7024 tests, 92.05% line coverage.
Summary by CodeRabbit
New Features
Documentation
Tests