Advance v18 migration scratch proof and command wrapper#104
Merged
Conversation
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 13 minutes and 22 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 (40)
📝 WalkthroughWalkthroughAdds full v18 graph-model migration pipeline: fixtures and manifest, source inventory, operation lowering, scratch writer/reader, equivalence gate, finalization safety/finalizer, orchestration/CLI/report formatter, comprehensive design docs, and tests validating end-to-end flows and failure-closed behavior. ChangesV18 graph-model migration end-to-end
Sequence Diagram(s)sequenceDiagram
participant Operator
participant CLI
participant Command
participant Git
participant Domain
Operator->>CLI: repo, request, manifest, scratch-ref
CLI->>Command: run with providers
Command->>Domain: plan + lower
Command->>Git: write scratch commits
Command->>Domain: build readings + gate
Command->>Domain: safety (with runtime conformance)
Command-->>Git: optional archive+live CAS finalization
Command-->>CLI: formatted deterministic report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
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: 11
🧹 Nitpick comments (1)
docs/design/0200-v18-migration-finalization-implementation/v18-migration-finalization-implementation.md (1)
4-4: ⚡ Quick winStandardize status field value across design documents.
The design documents in this PR use inconsistent status values: some use
status: Complete(docs 0194, 0195, 0199) while others usestatus: Completed(docs 0196, 0197, 0198, 0200). This inconsistency should be resolved by standardizing on a single value across all design documents in the repository.🤖 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/0200-v18-migration-finalization-implementation/v18-migration-finalization-implementation.md` at line 4, Several design docs use inconsistent status values; change the YAML/status field in this file (the "status: Completed" entry in v18-migration-finalization-implementation.md) to the project's canonical value ("status: Complete") and ensure the same replacement is applied across the related design docs (0194, 0195, 0196, 0197, 0198, 0199, 0200) so all documents use the identical "status" string; update only the status field value and run a quick repo-wide search for "status: Completed" to replace any other occurrences.
🤖 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/GitMigrationCommandRunner.ts`:
- Around line 66-70: When options.deterministicIdentity is true the child spawn
call replaces the entire environment with MIGRATION_GIT_IDENTITY causing
PATH/HOME to be lost; update the spawn options in the branch that returns
spawn('git', args, { cwd, env: MIGRATION_GIT_IDENTITY }) so the child env merges
the parent process.env with MIGRATION_GIT_IDENTITY (e.g., env: { ...process.env,
...MIGRATION_GIT_IDENTITY }) while keeping cwd the same; ensure you modify the
code where options.deterministicIdentity, spawn, and MIGRATION_GIT_IDENTITY are
referenced.
In `@scripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommand.ts`:
- Around line 96-99: The call to writeGraphModelMigrationScratchHistory uses
options.scratchRefName without validating it; add a boundary check in
GraphModelMigrationCommand (e.g., in the command's constructor or run method
before calling writeGraphModelMigrationScratchHistory) to ensure
options.scratchRefName is a non-empty, non-whitespace string and throw/return a
clear Error when invalid; this enforces the invariant at the command boundary
and prevents passing an empty scratchRefName into
writeGraphModelMigrationScratchHistory.
- Around line 143-147: The provider outputs from options.readingProviders are
used without validation; call the existing validation function (requireReading)
on both await results from options.readingProviders. Specifically, await
options.readingProviders.legacyReading() and
options.readingProviders.scratchReading(scratchWriteResult), pass each result
into requireReading(...) to enforce the reading contract, and only then return
Object.freeze({ legacyReading: validatedLegacy, scratchReading: validatedScratch
}); ensuring any bad provider output fails with a clear contract error instead
of later obscure failures.
In `@scripts/v18.0.0/migrations/graph-model/GraphModelMigrationFinalizer.ts`:
- Around line 134-138: The requireFinalizationString function currently only
rejects null; update it to also reject empty or all-whitespace strings by
checking (value === null || value.trim() === '') and throwing
GraphModelMigrationFinalizerError with the same message; ensure callers that
pass names like heads/ref rely on this invariant so invalid empty finalization
strings are caught here instead of during git execution.
In
`@scripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchReadingBuilder.ts`:
- Around line 180-184: The parseHexByte function currently uses
Number.parseInt(hex, 16) which allows partial parses (e.g., "0g" → 0); update
parseHexByte to first validate the input string is exactly two hex digits (e.g.,
with a /^[0-9a-fA-F]{2}$/ test) and only then parse it, otherwise throw
GraphModelMigrationScratchReadingBuilderError with the invalid input; ensure the
error message includes the original hex string for easier debugging and keep the
rest of the function behavior unchanged.
In
`@scripts/v18.0.0/migrations/graph-model/GraphModelMigrationSourceInventoryCollector.ts`:
- Around line 34-38: The function collectGraphModelMigrationSourceInventory
validates graphId but not repositoryPath; add a precondition check using
requireNonEmptyString(options.repositoryPath, 'repositoryPath') at the top of
the function before any Git/child-process calls (e.g., before calling
listWriterRefs) and use the resulting repositoryPath variable for subsequent
calls; apply the same defensive validation wherever this function later makes
other Git calls (the later calls around the listWriterRefs usage) so
repositoryPath is validated at the boundary.
In `@scripts/v18.0.0/migrations/graph-model/V17GoldenGraphFixtureRestore.ts`:
- Around line 33-35: Validate the input paths before performing any file-system
or Git operations: at the start of the routine that calls readManifest (the code
surrounding the readManifest(options.manifestPath) call in
V17GoldenGraphFixtureRestore), check that options.manifestPath and
options.targetDirectory are non-empty/defined and throw or return a clear error
if they are missing; only then call readManifest and compute repositoryPath =
resolve(options.targetDirectory) and bundlePath =
resolve(dirname(options.manifestPath), manifest.bundlePath) so the invariants
are established before I/O happens.
In `@src/domain/migrations/GraphModelMigrationArchiveRef.ts`:
- Around line 30-32: validateRefName currently assumes refName is a string and
directly accesses refName.length, and the constructor/validator that calls it
(requireFields / fields.refName) doesn’t ensure fields.refName exists or is a
string; add type guards so validateRefName first checks typeof refName ===
"string" and non-empty (e.g., typeof refName !== "string" || refName.length ===
0 returns the fatal GraphModelMigrationNotice), and update the caller
(requireFields / any code reading fields.refName) to verify fields.refName is
present and a string before passing it in; ensure all early-return validation
paths use GraphModelMigrationNotice rather than allowing native TypeError to be
thrown.
In `@src/domain/migrations/GraphModelMigrationScratchRef.ts`:
- Around line 30-32: The guard in validateRefName doesn't check for undefined,
so calling validateRefName with undefined causes refName.length to throw and
breaks constructor invariants; update
GraphModelMigrationScratchRef.validateRefName to treat undefined like null
(e.g., if (refName == null || refName.length === 0)) and return the same
GraphModelMigrationNotice.fatal path, ensuring no I/O or side effects and
preserving the invariant expected by the constructor.
In `@src/domain/migrations/V17GoldenGraphFixtureGenesisReading.ts`:
- Around line 53-90: The code currently branches on fact.kind string values in
projectionFor, compatibilityProjectionFor, and nonVisibleLifecycleProjectionFor;
replace those tag-string checks with runtime type dispatch using instanceof
checks against the concrete classes that implement
V17GoldenGraphFixtureVisibleFact (e.g., V17GoldenNodeFact, V17GoldenEdgeFact,
V17GoldenPropertyFact, V17GoldenContentFact, V17GoldenRemovalFact,
V17GoldenMultiWriterFact). Update the three functions to import and test fact
with instanceof (for example: if (fact instanceof V17GoldenNodeFact) { return
projection(...); } ), remove string comparisons, and keep the final WarpError
throw for unsupported types; ensure imports for the concrete classes are
added/updated so the runtime instanceof checks work.
In `@test/unit/scripts/v18-content-property-closeout-audit.test.ts`:
- Around line 74-75: The test collects OS-specific file paths with
files.push(relative('', path)) which causes Windows backslashes to fail
POSIX-style expectations; normalize the collected path before pushing by
converting it to POSIX separators (e.g., replace platform path.sep or
backslashes with '/') so the files array contains consistent POSIX-style paths
for assertions.
---
Nitpick comments:
In
`@docs/design/0200-v18-migration-finalization-implementation/v18-migration-finalization-implementation.md`:
- Line 4: Several design docs use inconsistent status values; change the
YAML/status field in this file (the "status: Completed" entry in
v18-migration-finalization-implementation.md) to the project's canonical value
("status: Complete") and ensure the same replacement is applied across the
related design docs (0194, 0195, 0196, 0197, 0198, 0199, 0200) so all documents
use the identical "status" string; update only the status field value and run a
quick repo-wide search for "status: Completed" to replace any other occurrences.
🪄 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: 52d243b2-b279-44ec-bd68-c7f59cdda67d
📒 Files selected for processing (72)
CHANGELOG.mddocs/BEARING.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/design/0200-v18-migration-finalization-implementation/v18-migration-finalization-implementation.mddocs/design/0201-v18-migration-command-wiring/v18-migration-command-wiring.mddocs/design/0202-v18-post-migration-runtime-conformance/v18-post-migration-runtime-conformance.mddocs/design/0203-v18-content-property-closeout-audit/v18-content-property-closeout-audit.mddocs/design/0204-v18-legacy-fixture-reading-construction/v18-legacy-fixture-reading-construction.mddocs/design/0205-v18-scratch-operation-reading-construction/v18-scratch-operation-reading-construction.mddocs/design/0206-v18-command-reading-providers/v18-command-reading-providers.mddocs/design/0207-v18-scratch-runtime-conformance-provider/v18-scratch-runtime-conformance-provider.mddocs/design/0208-v18-command-provider-finalization/v18-command-provider-finalization.mddocs/design/0209-v18-provider-divergence-coverage/v18-provider-divergence-coverage.mddocs/design/0210-v18-migration-command-report/v18-migration-command-report.mddocs/design/0211-v18-migration-command-cli-wrapper/v18-migration-command-cli-wrapper.mddocs/design/0212-v18-public-release-blockers/v18-public-release-blockers.mddocs/design/0213-v18-replan-after-command-cli/v18-replan-after-command-cli.mddocs/method/backlog/v18.0.0/INFRA_graph-model-migration-tool.mddocs/method/backlog/v18.0.0/README.mddocs/method/backlog/v18.0.0/RELEASE_v18-public-release-blockers.mddocs/method/backlog/v18.0.0/TRUST_genesis-replay-equivalence.mdfixtures/v17/graph-model-golden/README.mdfixtures/v17/graph-model-golden/manifest.jsonfixtures/v17/graph-model-golden/v17-golden-graph.bundlescripts/v18.0.0/migrations/graph-model/GitMigrationCommandRunner.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommand.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommandCli.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommandReport.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationFinalizer.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchReadingBuilder.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchRuntimeConformanceProvider.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchWriter.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationSourceInventoryCollector.tsscripts/v18.0.0/migrations/graph-model/V17GoldenGraphFixtureRestore.tsscripts/v18.0.0/migrations/graph-model/migrate.tssrc/domain/migrations/GenesisEquivalenceGate.tssrc/domain/migrations/GenesisEquivalenceGateResult.tssrc/domain/migrations/GraphModelMigrationArchiveRef.tssrc/domain/migrations/GraphModelMigrationFinalizationConfirmation.tssrc/domain/migrations/GraphModelMigrationFinalizationRequest.tssrc/domain/migrations/GraphModelMigrationFinalizationResult.tssrc/domain/migrations/GraphModelMigrationFinalizationSafety.tssrc/domain/migrations/GraphModelMigrationFinalizationSafetyResult.tssrc/domain/migrations/GraphModelMigrationLoweredOperation.tssrc/domain/migrations/GraphModelMigrationLoweredPatchPlan.tssrc/domain/migrations/GraphModelMigrationOperationLowerer.tssrc/domain/migrations/GraphModelMigrationOperationLoweringResult.tssrc/domain/migrations/GraphModelMigrationRuntimeConformanceResult.tssrc/domain/migrations/GraphModelMigrationScratchRef.tssrc/domain/migrations/GraphModelMigrationScratchWriteResult.tssrc/domain/migrations/GraphModelMigrationScratchWrittenPatch.tssrc/domain/migrations/V17GoldenGraphFixtureGenesisReading.tssrc/domain/migrations/V17GoldenGraphFixtureManifest.tssrc/infrastructure/adapters/V17GoldenGraphFixtureManifestJsonAdapter.tstest/unit/domain/migrations/GenesisEquivalenceGate.test.tstest/unit/domain/migrations/GraphModelMigrationFinalizationSafety.test.tstest/unit/domain/migrations/GraphModelMigrationOperationLowering.test.tstest/unit/domain/migrations/V17GoldenGraphFixtureGenesisReading.test.tstest/unit/scripts/v18-content-property-closeout-audit.test.tstest/unit/scripts/v18-graph-model-migration-command-cli.test.tstest/unit/scripts/v18-graph-model-source-inventory-collector.test.tstest/unit/scripts/v18-migration-command.test.tstest/unit/scripts/v18-migration-finalizer.test.tstest/unit/scripts/v18-scratch-migration-writer.test.tstest/unit/scripts/v18-scratch-reading-builder.test.tstest/unit/scripts/v18-scratch-runtime-conformance-provider.test.tstest/unit/scripts/v18-v17-golden-graph-fixtures.test.ts
Release Preflight
If you tag this commit as |
Member
Author
|
Resolved the outstanding CodeRabbit review feedback in fba2827.
|
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
Verification
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests