Advance v18 release-candidate evidence#106
Merged
Merged
Conversation
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 15 minutes and 2 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 (25)
📝 WalkthroughWalkthroughAdds guarded CLI finalization with reviewed JSON, production-runtime scratch replay and public-read/restored-legacy builders, a v17 wet-run harness/report, generated-contract conformance and warp-ttd smoke, transfer-op retirement, fixture/manifest updates, and many unit tests. ChangesV18 migration runtime, CLI, and evidence
Estimated code review effort Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommandCli.ts (2)
1-316: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftReduce this CLI script below the script LOC ceiling.
At 316 LOC, this file exceeds the scripts limit. Please split parsing, finalization-option shaping, and execution/reporting into separate modules.
As per coding guidelines: "Maximum file size: 500 LOC (source), 800 LOC (test), 300 LOC (scripts)".
🤖 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/v18.0.0/migrations/graph-model/GraphModelMigrationCommandCli.ts` around lines 1 - 316, This file is too large for scripts—split parsing, finalization shaping, and execution/reporting into separate modules: extract parseGraphModelMigrationCommandCliArgs (and its helper readArgValue, requireString, GraphModelMigrationCommandCliArgs/GraphModelMigrationCommandCliArgumentError) into a new cli-args module and export the parser and types; extract finalizationOptions (and requireFinalizationString) plus its GraphModelMigrationFinalizationRequest typing usage into a new finalization-builder module that exports a function to build the finalization object and the helper; leave runGraphModelMigrationCommandCli in this file but refactor it to import parseGraphModelMigrationCommandCliArgs and the finalization builder and move report-generation and file I/O (readFile/writeFile, report write) into a small execution/reporting helper module if desired; ensure all exported functions/classes (parseGraphModelMigrationCommandCliArgs, finalizationOptions builder, GraphModelMigrationCommandCliArgs, GraphModelMigrationCommandCliArgumentError, runGraphModelMigrationCommandCli) keep their names so callers (and tests) continue to work and update imports accordingly.
157-160:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the legacy-flag rejection message to match current behavior.
Now that finalization is supported via
--finalization-request, saying finalization is “not supported ... yet” is misleading. Prefer wording that explicitly rejects only legacy flags and points to the request artifact flag.🤖 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/v18.0.0/migrations/graph-model/GraphModelMigrationCommandCli.ts` around lines 157 - 160, Replace the misleading rejection message thrown in the block checking FINALIZATION_FLAGS so it explicitly rejects legacy finalization flags and points users to the new request flag; modify the GraphModelMigrationCommandCliArgumentError thrown when (arg !== undefined && FINALIZATION_FLAGS.has(arg)) to say something like "legacy finalization flags are not accepted; use --finalization-request to submit a finalization request" (referencing FINALIZATION_FLAGS, arg, and GraphModelMigrationCommandCliArgumentError in your edit).scripts/v18.0.0/migrations/graph-model/V17GoldenGraphFixtureWetRunHarness.ts (1)
1-585: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit this script file to satisfy script LOC limits.
This script is 585 LOC, which exceeds the repo’s script-size cap and makes this slice harder to maintain and review. Please break it into focused modules (e.g., harness orchestration, drift checks, scratch-boundary mapping, and target-key decoding).
As per coding guidelines: "Maximum file size: 500 LOC (source), 800 LOC (test), 300 LOC (scripts)".
🤖 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/v18.0.0/migrations/graph-model/V17GoldenGraphFixtureWetRunHarness.ts` around lines 1 - 585, The file exceeds the script LOC cap; split it into focused modules: keep harness orchestration (exporting runV17GoldenGraphFixtureWetRun, V17GoldenGraphFixtureWetRunHarnessResult, V17GoldenGraphFixtureWetRunDriftCheckResult, V17GoldenGraphFixtureWetRunHarnessError) in one file, move drift-check logic (checkV17GoldenGraphFixtureWetRunDrift) into a drift module, move scratch-boundary mapping (requireScratchWriteResult, scratchBoundariesByFactKey, factKeyForWrittenPatch, factKeyForWrittenPatch helpers like publicContentFactKey, publicPropertyFactKey) into a scratch-boundary module, and move target-key decoding/parsings (decodePropertyTargetKey, readLength, readSizedField, publicPropertyFactKey) into a target-key decoder module; update imports/exports between these modules and ensure createV17GoldenFixtureScratchReadingProvider and withFixtureCoverageFacts reference the relocated helpers, preserving public function/class names and behavior.
🧹 Nitpick comments (8)
src/domain/migrations/V17GoldenGraphFixtureGenesisReading.ts (1)
83-89: ⚡ Quick winClarify error message to reflect multi-colon support.
The implementation uses
lastIndexOf(':')to find the separator, which means keys like"owner:sub:property"are valid and will split as"owner:sub\0property". However, the error message states"must use owner:property format", which suggests exactly one colon is required. Consider rewording to something like"must use owner:property format (colons allowed in owner segment)"or"must contain at least one colon not at the boundaries"to better reflect the actual validation logic.🤖 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 `@src/domain/migrations/V17GoldenGraphFixtureGenesisReading.ts` around lines 83 - 89, Update the error message thrown in legacyPropertyKeyFor to accurately describe the validation (it requires at least one colon not at the boundaries and allows additional colons in the owner segment); change the WarpError message from 'property fixture fact key must use owner:property format' to something like 'property fixture fact key must contain at least one colon not at the boundaries (colons allowed in owner segment)' so the message matches the lastIndexOf(':') logic used in legacyPropertyKeyFor.docs/design/0240-v18-generated-contract-evidence-replan/v18-generated-contract-evidence-replan.md (1)
25-25: 💤 Low valueConsider hyphenating compound adjective.
The phrase "runtime-boundary generated fixture" could be clearer as "runtime-boundary-generated fixture" when the compound modifier precedes the noun.
🤖 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/0240-v18-generated-contract-evidence-replan/v18-generated-contract-evidence-replan.md` at line 25, Change the phrase "runtime-boundary generated fixture" to the hyphenated compound adjective "runtime-boundary-generated fixture" so the compound modifier correctly precedes the noun; update the string in the docs line containing that phrase (search for "runtime-boundary generated fixture") and replace with "runtime-boundary-generated fixture".test/unit/scripts/v18-v17-public-read-legacy-reading-builder.test.ts (2)
77-83: 💤 Low valueConsider cleanup for temporary test directories.
The
restoredFixturehelper creates temporary directories but doesn't explicitly clean them up. While test frameworks often handle cleanup, consider verifying that vitest cleans up temp directories or adding explicit cleanup usingafterEachor try-finally blocks to prevent accumulation during test runs.🤖 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/scripts/v18-v17-public-read-legacy-reading-builder.test.ts` around lines 77 - 83, restoredFixture creates temp directories via mkdtemp(join(tmpdir(), prefix)) but never cleans them up; modify the test setup to ensure created targetDirectory is removed after each test (e.g., track returned targetDirectory paths and call fs.rm/recursive cleanup in an afterEach hook) or change restoredFixture to return a cleanup callback (or use try/finally inside tests) so restoreV17GoldenGraphFixture's targetDirectory is explicitly deleted after use.
85-89: ⚡ Quick winExtract duplicated test helper to shared utility.
The
gitOkhelper is duplicated fromtest/unit/scripts/v18-v17-fixture-wet-run-harness.test.ts(lines 192-196). Consider extracting it to a shared test utilities module to follow DRY principles and ensure consistent Git command handling across migration tests.♻️ Suggested refactor
Create a shared utilities file (e.g.,
test/unit/scripts/migration-test-helpers.ts):import { runMigrationGit } from '../../../scripts/v18.0.0/migrations/graph-model/GitMigrationCommandRunner.ts'; import { expect } from 'vitest'; export async function gitOk(repositoryPath: string, args: readonly string[]): Promise<string> { const result = await runMigrationGit(repositoryPath, args, null, { deterministicIdentity: true }); expect(result.ok()).toBe(true); return result.stdout.trim(); }Then import and use in both test files:
+import { gitOk } from './migration-test-helpers.ts'; - -async function gitOk(repositoryPath: string, args: readonly string[]): Promise<string> { - const result = await runMigrationGit(repositoryPath, args, null, { deterministicIdentity: true }); - expect(result.ok()).toBe(true); - return result.stdout.trim(); -}🤖 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/scripts/v18-v17-public-read-legacy-reading-builder.test.ts` around lines 85 - 89, Extract the duplicated gitOk helper into a shared test utility module and import it in both tests: create a new module exporting async function gitOk(repositoryPath: string, args: readonly string[]) that calls runMigrationGit(..., { deterministicIdentity: true }), asserts result.ok() true, and returns result.stdout.trim(); update the two test files to remove the local gitOk implementations and import the shared gitOk; ensure you import runMigrationGit from its current module only in the new helper and update any relative import paths accordingly.src/domain/migrations/GraphModelMigrationRuntimeReplayRequest.ts (1)
47-52: ⚡ Quick winExtract duplicated validation helper.
The
requireNonEmptyStringhelper is duplicated fromGraphModelMigrationRuntimeReplayResult.ts(lines 78-83). Consider extracting it to a shared domain utility module (e.g.,src/domain/migrations/validationHelpers.ts) to prevent future divergence and improve maintainability.♻️ Suggested refactor
Create
src/domain/migrations/validationHelpers.ts:import WarpError from '../errors/WarpError.ts'; export function requireNonEmptyString(value: string, name: string): string { if (typeof value !== 'string' || value.length === 0) { throw new WarpError(`${name} must be a non-empty string`, 'E_VALIDATION'); } return value; }Then import and use in both files:
+import { requireNonEmptyString } from './validationHelpers.ts'; import GraphModelMigrationScratchRef from './GraphModelMigrationScratchRef.ts'; import WarpError from '../errors/WarpError.ts'; // ... rest of file ... -function requireNonEmptyString(value: string, name: string): string { - if (typeof value !== 'string' || value.length === 0) { - throw new WarpError(`${name} must be a non-empty string`, 'E_VALIDATION'); - } - return value; -}Apply the same change to
GraphModelMigrationRuntimeReplayResult.ts.🤖 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 `@src/domain/migrations/GraphModelMigrationRuntimeReplayRequest.ts` around lines 47 - 52, Extract the duplicated requireNonEmptyString helper into a new shared module named validationHelpers.ts, export the function there (keeping the same signature and throwing WarpError with 'E_VALIDATION'), then remove the duplicate implementations and import requireNonEmptyString into both GraphModelMigrationRuntimeReplayRequest and GraphModelMigrationRuntimeReplayResult to use the shared helper; ensure you import WarpError in the new module or re-export it as needed so the behavior and error type remain identical.scripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommand.ts (1)
1-335: ⚖️ Poor tradeoffConsider splitting this file to meet the 300 LOC guideline for scripts.
The file currently has 335 lines, exceeding the 300 LOC limit for scripts. The review-guard helpers (lines 203-284) could be extracted into a separate module, though this adds some coordination overhead.
As per coding guidelines, scripts should not exceed 300 LOC.
🤖 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/v18.0.0/migrations/graph-model/GraphModelMigrationCommand.ts` around lines 1 - 335, This file exceeds the 300 LOC guideline—extract the finalization review / guard helper functions into a new module and import them: move reviewedSafetyResult, finalizationReviewFatalErrors, finalizationReviewMismatches, stringMismatch, equivalenceSummaryKey, runtimeConformanceKey (and the small helper runtimeConformanceFromProvider if you prefer) into a separate helper module, export them, then replace their local definitions with imports and keep runFinalization, resolveReadings, and top-level flow unchanged; ensure exported functions keep the same signatures (types like GraphModelMigrationFinalizationRequest, GraphModelMigrationFinalizationSafetyResult, GraphModelMigrationNotice, GraphModelMigrationScratchWriteResult are imported where needed) and update calls in this file to use the imported symbols so the script falls under 300 LOC.docs/design/0231-v18-live-finalization-cli-confirmation/v18-live-finalization-cli-confirmation.md (1)
91-91: ⚡ Quick winAdd a trailing newline at end of file.
Markdown files should end with a newline for POSIX compliance and diff cleanliness.
🤖 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/0231-v18-live-finalization-cli-confirmation/v18-live-finalization-cli-confirmation.md` at line 91, Add a single trailing newline character at the end of the markdown file docs/design/0231-v18-live-finalization-cli-confirmation/v18-live-finalization-cli-confirmation.md so the file ends with a newline (POSIX-compliant) to avoid dirty diffs; simply ensure the last line is terminated with "\n".docs/design/0228-v18-fixture-lifecycle-and-writer-coverage/v18-fixture-lifecycle-and-writer-coverage.md (1)
54-54: ⚡ Quick winAdd a trailing newline at end of file.
Markdown files should end with a newline for POSIX compliance and diff cleanliness.
🤖 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/0228-v18-fixture-lifecycle-and-writer-coverage/v18-fixture-lifecycle-and-writer-coverage.md` at line 54, This markdown file lacks a trailing newline at EOF; open v18-fixture-lifecycle-and-writer-coverage.md (the document being edited) and add a single newline character at the end of the file so the file ends with a trailing newline for POSIX compliance and cleaner diffs.
🤖 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/BEARING.md`:
- Line 454: Update the three checklist items that read "Inventory current
Wesley/Continuum generated graph contracts" (the repeated checklist entries) to
use consistent hyphenation for the compound modifier—e.g., change to "Inventory
current Wesley/Continuum generated-graph contracts" or rephrase to "Inventory
current Wesley/Continuum graph contracts" so the modifier is clear and
consistent across the occurrences referenced in the diff.
In
`@docs/design/0237-v18-runtime-boundary-fixture-ingestion/v18-runtime-boundary-fixture-ingestion.md`:
- Line 37: The phrase "runtime-boundary generated fixture" should be hyphenated
as "runtime-boundary-generated fixture"; update the instance of
"runtime-boundary generated fixture" in the document (look for that exact phrase
in v18-runtime-boundary-fixture-ingestion.md) so the compound adjective
correctly reads "runtime-boundary-generated fixture".
In `@scripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommandReport.ts`:
- Around line 89-95: The report is using the wrong field for archive evidence:
replace the mistaken uses of finalization.previousLiveHead when rendering
archiveHead and archivePreserved with the correct archive head property
(finalization.archiveHeadName); specifically, change the archiveHead line to use
displayNullable(finalization.archiveHeadName) and make archivePreserved depend
on finalization.archiveHeadName === null ? 'no' : 'yes' within
GraphModelMigrationCommandReport where finalization and displayNullable are
used.
---
Outside diff comments:
In `@scripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommandCli.ts`:
- Around line 1-316: This file is too large for scripts—split parsing,
finalization shaping, and execution/reporting into separate modules: extract
parseGraphModelMigrationCommandCliArgs (and its helper readArgValue,
requireString,
GraphModelMigrationCommandCliArgs/GraphModelMigrationCommandCliArgumentError)
into a new cli-args module and export the parser and types; extract
finalizationOptions (and requireFinalizationString) plus its
GraphModelMigrationFinalizationRequest typing usage into a new
finalization-builder module that exports a function to build the finalization
object and the helper; leave runGraphModelMigrationCommandCli in this file but
refactor it to import parseGraphModelMigrationCommandCliArgs and the
finalization builder and move report-generation and file I/O
(readFile/writeFile, report write) into a small execution/reporting helper
module if desired; ensure all exported functions/classes
(parseGraphModelMigrationCommandCliArgs, finalizationOptions builder,
GraphModelMigrationCommandCliArgs, GraphModelMigrationCommandCliArgumentError,
runGraphModelMigrationCommandCli) keep their names so callers (and tests)
continue to work and update imports accordingly.
- Around line 157-160: Replace the misleading rejection message thrown in the
block checking FINALIZATION_FLAGS so it explicitly rejects legacy finalization
flags and points users to the new request flag; modify the
GraphModelMigrationCommandCliArgumentError thrown when (arg !== undefined &&
FINALIZATION_FLAGS.has(arg)) to say something like "legacy finalization flags
are not accepted; use --finalization-request to submit a finalization request"
(referencing FINALIZATION_FLAGS, arg, and
GraphModelMigrationCommandCliArgumentError in your edit).
In
`@scripts/v18.0.0/migrations/graph-model/V17GoldenGraphFixtureWetRunHarness.ts`:
- Around line 1-585: The file exceeds the script LOC cap; split it into focused
modules: keep harness orchestration (exporting runV17GoldenGraphFixtureWetRun,
V17GoldenGraphFixtureWetRunHarnessResult,
V17GoldenGraphFixtureWetRunDriftCheckResult,
V17GoldenGraphFixtureWetRunHarnessError) in one file, move drift-check logic
(checkV17GoldenGraphFixtureWetRunDrift) into a drift module, move
scratch-boundary mapping (requireScratchWriteResult, scratchBoundariesByFactKey,
factKeyForWrittenPatch, factKeyForWrittenPatch helpers like
publicContentFactKey, publicPropertyFactKey) into a scratch-boundary module, and
move target-key decoding/parsings (decodePropertyTargetKey, readLength,
readSizedField, publicPropertyFactKey) into a target-key decoder module; update
imports/exports between these modules and ensure
createV17GoldenFixtureScratchReadingProvider and withFixtureCoverageFacts
reference the relocated helpers, preserving public function/class names and
behavior.
---
Nitpick comments:
In
`@docs/design/0228-v18-fixture-lifecycle-and-writer-coverage/v18-fixture-lifecycle-and-writer-coverage.md`:
- Line 54: This markdown file lacks a trailing newline at EOF; open
v18-fixture-lifecycle-and-writer-coverage.md (the document being edited) and add
a single newline character at the end of the file so the file ends with a
trailing newline for POSIX compliance and cleaner diffs.
In
`@docs/design/0231-v18-live-finalization-cli-confirmation/v18-live-finalization-cli-confirmation.md`:
- Line 91: Add a single trailing newline character at the end of the markdown
file
docs/design/0231-v18-live-finalization-cli-confirmation/v18-live-finalization-cli-confirmation.md
so the file ends with a newline (POSIX-compliant) to avoid dirty diffs; simply
ensure the last line is terminated with "\n".
In
`@docs/design/0240-v18-generated-contract-evidence-replan/v18-generated-contract-evidence-replan.md`:
- Line 25: Change the phrase "runtime-boundary generated fixture" to the
hyphenated compound adjective "runtime-boundary-generated fixture" so the
compound modifier correctly precedes the noun; update the string in the docs
line containing that phrase (search for "runtime-boundary generated fixture")
and replace with "runtime-boundary-generated fixture".
In `@scripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommand.ts`:
- Around line 1-335: This file exceeds the 300 LOC guideline—extract the
finalization review / guard helper functions into a new module and import them:
move reviewedSafetyResult, finalizationReviewFatalErrors,
finalizationReviewMismatches, stringMismatch, equivalenceSummaryKey,
runtimeConformanceKey (and the small helper runtimeConformanceFromProvider if
you prefer) into a separate helper module, export them, then replace their local
definitions with imports and keep runFinalization, resolveReadings, and
top-level flow unchanged; ensure exported functions keep the same signatures
(types like GraphModelMigrationFinalizationRequest,
GraphModelMigrationFinalizationSafetyResult, GraphModelMigrationNotice,
GraphModelMigrationScratchWriteResult are imported where needed) and update
calls in this file to use the imported symbols so the script falls under 300
LOC.
In `@src/domain/migrations/GraphModelMigrationRuntimeReplayRequest.ts`:
- Around line 47-52: Extract the duplicated requireNonEmptyString helper into a
new shared module named validationHelpers.ts, export the function there (keeping
the same signature and throwing WarpError with 'E_VALIDATION'), then remove the
duplicate implementations and import requireNonEmptyString into both
GraphModelMigrationRuntimeReplayRequest and
GraphModelMigrationRuntimeReplayResult to use the shared helper; ensure you
import WarpError in the new module or re-export it as needed so the behavior and
error type remain identical.
In `@src/domain/migrations/V17GoldenGraphFixtureGenesisReading.ts`:
- Around line 83-89: Update the error message thrown in legacyPropertyKeyFor to
accurately describe the validation (it requires at least one colon not at the
boundaries and allows additional colons in the owner segment); change the
WarpError message from 'property fixture fact key must use owner:property
format' to something like 'property fixture fact key must contain at least one
colon not at the boundaries (colons allowed in owner segment)' so the message
matches the lastIndexOf(':') logic used in legacyPropertyKeyFor.
In `@test/unit/scripts/v18-v17-public-read-legacy-reading-builder.test.ts`:
- Around line 77-83: restoredFixture creates temp directories via
mkdtemp(join(tmpdir(), prefix)) but never cleans them up; modify the test setup
to ensure created targetDirectory is removed after each test (e.g., track
returned targetDirectory paths and call fs.rm/recursive cleanup in an afterEach
hook) or change restoredFixture to return a cleanup callback (or use try/finally
inside tests) so restoreV17GoldenGraphFixture's targetDirectory is explicitly
deleted after use.
- Around line 85-89: Extract the duplicated gitOk helper into a shared test
utility module and import it in both tests: create a new module exporting async
function gitOk(repositoryPath: string, args: readonly string[]) that calls
runMigrationGit(..., { deterministicIdentity: true }), asserts result.ok() true,
and returns result.stdout.trim(); update the two test files to remove the local
gitOk implementations and import the shared gitOk; ensure you import
runMigrationGit from its current module only in the new helper and update any
relative import paths accordingly.
🪄 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: ef243ab4-4da4-4bb3-aba9-47acdb0c3d0e
📒 Files selected for processing (79)
CHANGELOG.mddocs/BEARING.mddocs/design/0203-v18-content-property-closeout-audit/v18-content-property-closeout-audit.mddocs/design/0214-v18-production-runtime-scratch-replay-conformance/v18-production-runtime-scratch-replay-conformance.mddocs/design/0215-v18-runtime-scratch-replay-nouns/v18-runtime-scratch-replay-nouns.mddocs/design/0216-v18-production-runtime-scratch-replay-provider/v18-production-runtime-scratch-replay-provider.mddocs/design/0217-v18-v17-public-read-legacy-reading/v18-v17-public-read-legacy-reading.mddocs/design/0218-v18-scratch-public-read-reading/v18-scratch-public-read-reading.mddocs/design/0219-v18-v17-fixture-wet-run-harness/v18-v17-fixture-wet-run-harness.mddocs/design/0220-v18-wet-run-operator-report/v18-wet-run-operator-report.mddocs/design/0221-v18-wet-run-failure-fixtures/v18-wet-run-failure-fixtures.mddocs/design/0222-v18-wet-run-drift-checks/v18-wet-run-drift-checks.mddocs/design/0223-v18-production-replay-drift-checkup/v18-production-replay-drift-checkup.mddocs/design/0224-v18-wet-run-mismatch-classification/v18-wet-run-mismatch-classification.mddocs/design/0225-v18-fixture-property-equivalence-values/v18-fixture-property-equivalence-values.mddocs/design/0226-v18-fixture-content-runtime-oids/v18-fixture-content-runtime-oids.mddocs/design/0227-v18-fixture-edge-endpoint-coverage/v18-fixture-edge-endpoint-coverage.mddocs/design/0228-v18-fixture-lifecycle-and-writer-coverage/v18-fixture-lifecycle-and-writer-coverage.mddocs/design/0229-v18-zero-mismatch-wet-run-proof/v18-zero-mismatch-wet-run-proof.mddocs/design/0230-v18-finalization-replan-after-zero-mismatch/v18-finalization-replan-after-zero-mismatch.mddocs/design/0231-v18-live-finalization-cli-confirmation/v18-live-finalization-cli-confirmation.mddocs/design/0232-v18-finalization-request-json-adapters/v18-finalization-request-json-adapters.mddocs/design/0233-v18-finalization-report-archive-evidence/v18-finalization-report-archive-evidence.mddocs/design/0234-v18-guarded-cli-finalization/v18-guarded-cli-finalization.mddocs/design/0235-v18-finalization-drift-and-archive-tests/v18-finalization-drift-and-archive-tests.mddocs/design/0236-v18-generated-contract-inventory/v18-generated-contract-inventory.mddocs/design/0237-v18-runtime-boundary-fixture-ingestion/v18-runtime-boundary-fixture-ingestion.mddocs/design/0238-v18-graph-model-contract-conformance/v18-graph-model-contract-conformance.mddocs/design/0239-v18-warp-ttd-generated-family-smoke/v18-warp-ttd-generated-family-smoke.mddocs/design/0240-v18-generated-contract-evidence-replan/v18-generated-contract-evidence-replan.mddocs/design/0241-v18-coordinate-fact-export-raw-boundary-retirement/v18-coordinate-fact-export-raw-boundary-retirement.mddocs/design/0242-v18-content-property-retirement-ratchet/v18-content-property-retirement-ratchet.mddocs/design/0243-v18-release-candidate-evidence/v18-release-candidate-evidence.mddocs/design/0244-v18-backlog-reconciliation/v18-backlog-reconciliation.mddocs/method/backlog/README.mddocs/method/backlog/bad-code/RELEASE_TRIAGE.mddocs/method/backlog/v17.0.0/README.mddocs/method/backlog/v18.0.0/INFRA_graph-model-migration-tool.mddocs/method/backlog/v18.0.0/PROTO_content-attachment-plane-cutover.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.mddocs/method/backlog/v20.0.0/PERF_end-to-end-graph-streaming.mddocs/method/backlog/v20.0.0/README.mddocs/releases/v18.0.0-rc/README.mdfixtures/v17/graph-model-golden/README.mdfixtures/v17/graph-model-golden/manifest.jsonscripts/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/GraphModelMigrationProductionRuntimeReplayProvider.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchPublicReadBuilder.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchReadingBuilder.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchRuntimeReplayer.tsscripts/v18.0.0/migrations/graph-model/V17GoldenGraphFixtureWetRunHarness.tsscripts/v18.0.0/migrations/graph-model/V17GoldenGraphFixtureWetRunReport.tsscripts/v18.0.0/migrations/graph-model/V17RestoredPublicReadLegacyReadingBuilder.tssrc/domain/continuum/GitWarpGraphModelContractConformance.tssrc/domain/continuum/GitWarpWarpTtdGeneratedFamilySmoke.tssrc/domain/migrations/GraphModelMigrationRuntimeReplayRequest.tssrc/domain/migrations/GraphModelMigrationRuntimeReplayResult.tssrc/domain/migrations/V17GoldenGraphFixtureGenesisReading.tssrc/domain/services/CoordinateFactExport.tssrc/domain/services/transfer/transferOps.tssrc/infrastructure/adapters/GraphModelMigrationFinalizationRequestJsonAdapter.tstest/fixtures/continuum/runtime-boundary-family-generated-artifact.jsontest/unit/domain/continuum/GitWarpGraphModelContractConformance.test.tstest/unit/domain/continuum/GitWarpWarpTtdGeneratedFamilySmoke.test.tstest/unit/domain/migrations/GraphModelMigrationRuntimeReplayResult.test.tstest/unit/domain/migrations/V17GoldenGraphFixtureGenesisReading.test.tstest/unit/infrastructure/adapters/ContinuumArtifactJsonFileAdapter.test.tstest/unit/infrastructure/adapters/GraphModelMigrationFinalizationRequestJsonAdapter.test.tstest/unit/scripts/v18-content-property-closeout-audit.test.tstest/unit/scripts/v18-graph-model-migration-command-cli.test.tstest/unit/scripts/v18-migration-command.test.tstest/unit/scripts/v18-production-runtime-scratch-replay-provider.test.tstest/unit/scripts/v18-scratch-public-read-builder.test.tstest/unit/scripts/v18-v17-fixture-wet-run-harness.test.tstest/unit/scripts/v18-v17-public-read-legacy-reading-builder.test.ts
Member
Author
|
@codex Self-review findings from strict Code Lawyer pass against
No Markdown lint/style violation was found by |
Member
Author
|
@codex Review follow-up landed in
Verification run locally before push:
Pre-push IRONCLAD M9 also passed and pushed |
Release Preflight
If you tag this commit as |
Member
Author
|
@codex Self-review follow-up findings after
Local evidence: pre-review worktree was clean, |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/GraphModelMigrationScratchRuntimeReplayer.ts`:
- Around line 171-176: The decodeLegacyEdgePropNode call can throw a foreign
error; wrap the decodeLegacyEdgePropNode(property.ownerId) invocation in a
try/catch and, on any failure, throw/raise the standardized replayer error
E_RUNTIME_REPLAY_INVALID_OPERATION_TARGET (preserving/including the original
error message as context) so failures are classified consistently; keep the
existing behavior for non-legacy nodes (isLegacyEdgePropNode) and then call
patch.setEdgeProperty(edge.from, edge.to, edge.label, property.propertyKey,
value) when decode succeeds.
In
`@src/infrastructure/adapters/GraphModelMigrationFinalizationRequestJsonAdapter.ts`:
- Around line 59-64: The JSON parse error for confirmations uses the wrong
context text; update the parsing block inside parseDomainValue for
GraphModelMigrationFinalizationConfirmation so that when calling parseJson(raw)
(and any parse failure) it reports a context-specific message like
"finalizationConfirmation" or "finalization confirmation" instead of the generic
"finalization request JSON". Locate the parseDomainValue call that constructs a
GraphModelMigrationFinalizationConfirmation (which uses parseJson,
requireJsonObject, rejectUnknownKeys, CONFIRMATION_KEYS and readRequiredString
for 'finalizationConfirmation.confirmationToken') and change the error/context
string passed to parseJson or the surrounding parseDomainValue wrapper to
reflect confirmation parsing. Ensure rejectUnknownKeys and error messages
consistently reference 'finalizationConfirmation'.
🪄 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: 2466f1cf-5a7f-4561-8829-ffb12c210272
📒 Files selected for processing (21)
CHANGELOG.mddocs/BEARING.mddocs/design/0214-v18-production-runtime-scratch-replay-conformance/v18-production-runtime-scratch-replay-conformance.mddocs/design/0228-v18-fixture-lifecycle-and-writer-coverage/v18-fixture-lifecycle-and-writer-coverage.mddocs/design/0230-v18-finalization-replan-after-zero-mismatch/v18-finalization-replan-after-zero-mismatch.mdfixtures/v17/graph-model-golden/README.mdfixtures/v17/graph-model-golden/manifest.jsonscripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommand.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommandCli.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchPublicReadBuilder.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchRuntimeReplayer.tsscripts/v18.0.0/migrations/graph-model/V17GoldenGraphFixtureWetRunHarness.tssrc/domain/migrations/GraphModelMigrationRuntimeReplayResult.tssrc/infrastructure/adapters/GraphModelMigrationFinalizationRequestJsonAdapter.tstest/unit/domain/migrations/V17GoldenGraphFixtureGenesisReading.test.tstest/unit/infrastructure/adapters/GraphModelMigrationFinalizationRequestJsonAdapter.test.tstest/unit/scripts/v18-graph-model-migration-command-cli.test.tstest/unit/scripts/v18-production-runtime-scratch-replay-provider.test.tstest/unit/scripts/v18-scratch-public-read-builder.test.tstest/unit/scripts/v18-v17-fixture-wet-run-harness.test.tstest/unit/scripts/v18-v17-public-read-legacy-reading-builder.test.ts
✅ Files skipped from review due to trivial changes (6)
- fixtures/v17/graph-model-golden/README.md
- fixtures/v17/graph-model-golden/manifest.json
- docs/design/0228-v18-fixture-lifecycle-and-writer-coverage/v18-fixture-lifecycle-and-writer-coverage.md
- docs/design/0230-v18-finalization-replan-after-zero-mismatch/v18-finalization-replan-after-zero-mismatch.md
- docs/design/0214-v18-production-runtime-scratch-replay-conformance/v18-production-runtime-scratch-replay-conformance.md
- CHANGELOG.md
Member
Author
|
Resolution summary for the self-review findings posted in #106 (comment).
Verification completed locally and in pre-push:
✅ Addressed in commits |
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchRuntimeReplayer.ts (1)
94-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
runtimeRepositoryPathbefore using it as the runtime repo cwd.If a caller passes
'', this branch skips temp-dir creation and initializes the replay repo in the current working directory instead of an isolated location. That turns a verification path into a write against an unintended repo.Suggested fix
export async function replayGraphModelMigrationScratchIntoRuntime( options: GraphModelMigrationScratchRuntimeReplayOptions, ): Promise<GraphModelMigrationScratchRuntimeReplayOutput> { const sourceRepositoryPath = requireGraphModelMigrationRuntimeReplayString( options.sourceRepositoryPath, 'sourceRepositoryPath', ); const request = requireGraphModelMigrationRuntimeReplayRequest(options.request); - let runtimeRepositoryPath = options.runtimeRepositoryPath ?? null; + let runtimeRepositoryPath = options.runtimeRepositoryPath === null + || options.runtimeRepositoryPath === undefined + ? null + : requireGraphModelMigrationRuntimeReplayString( + options.runtimeRepositoryPath, + 'runtimeRepositoryPath', + ); let shouldCleanup = false; if (runtimeRepositoryPath === null) { runtimeRepositoryPath = await mkdtemp(join(tmpdir(), 'git-warp-v18-runtime-replay-')); shouldCleanup = true; }As per coding guidelines, "Validate at boundaries and constructors; constructors establish invariants and must not perform I/O".
🤖 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/v18.0.0/migrations/graph-model/GraphModelMigrationScratchRuntimeReplayer.ts` around lines 94 - 104, The code fails to validate runtimeRepositoryPath and treats an empty string as a valid path, causing repo initialization to run in CWD; update the initialization so runtimeRepositoryPath is normalized/validated (e.g., call the same validator used for sourceRepositoryPath or explicitly treat '' as null) before deciding to create a temp dir: change the runtimeRepositoryPath assignment (currently using options.runtimeRepositoryPath ?? null) to validate/normalize options.runtimeRepositoryPath (reject empty string) and only skip mkdtemp when a non-empty, validated path is provided; refer to the runtimeRepositoryPath and shouldCleanup variables and the mkdtemp(join(tmpdir(), 'git-warp-v18-runtime-replay-')) call when making the fix.
🤖 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/V17GoldenFixtureScratchReadingProvider.ts`:
- Around line 87-99: The code uses repeated magic strings for scratch operation
kinds and fact keys (e.g., 'node-record', 'edge-record', 'property',
'content-attachment', 'visibility', 'value', 'payload.oid', 'coverage',
'removed'); replace these literals with named constants (or import existing
domain constants) and update the control flow and factKey calls accordingly —
locate the switch/if block around factKey(...) calls and the thrown
V17GoldenFixtureScratchReadingProviderError in
V17GoldenFixtureScratchReadingProvider.ts and define constants (e.g.,
SCRATCH_KIND_NODE_RECORD, FACT_KEY_VISIBILITY, FACT_KEY_VALUE,
FACT_KEY_PAYLOAD_OID, etc.) then use those constants in the if conditions and in
calls to factKey, publicPropertyFactKey, publicContentFactKey, and anywhere the
other literals (coverage, removed) appear (also apply the same change for the
similar block at lines ~142-157).
---
Outside diff comments:
In
`@scripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchRuntimeReplayer.ts`:
- Around line 94-104: The code fails to validate runtimeRepositoryPath and
treats an empty string as a valid path, causing repo initialization to run in
CWD; update the initialization so runtimeRepositoryPath is normalized/validated
(e.g., call the same validator used for sourceRepositoryPath or explicitly treat
'' as null) before deciding to create a temp dir: change the
runtimeRepositoryPath assignment (currently using options.runtimeRepositoryPath
?? null) to validate/normalize options.runtimeRepositoryPath (reject empty
string) and only skip mkdtemp when a non-empty, validated path is provided;
refer to the runtimeRepositoryPath and shouldCleanup variables and the
mkdtemp(join(tmpdir(), 'git-warp-v18-runtime-replay-')) call when making the
fix.
🪄 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: fedb3144-a25d-4dd6-b40b-2c0abf8a8995
📒 Files selected for processing (14)
CHANGELOG.mdscripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommand.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommandCli.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationCommandCliArgs.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationFinalizationReview.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchRuntimeReplayErrors.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchRuntimeReplayValidation.tsscripts/v18.0.0/migrations/graph-model/GraphModelMigrationScratchRuntimeReplayer.tsscripts/v18.0.0/migrations/graph-model/V17GoldenFixtureScratchFactKeyCodec.tsscripts/v18.0.0/migrations/graph-model/V17GoldenFixtureScratchReadingProvider.tsscripts/v18.0.0/migrations/graph-model/V17GoldenGraphFixturePropertyMappings.tsscripts/v18.0.0/migrations/graph-model/V17GoldenGraphFixtureWetRunHarness.tstest/unit/scripts/v18-production-runtime-scratch-replay-provider.test.tstest/unit/scripts/v18-v17-fixture-wet-run-harness.test.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Member
Author
|
@codex Self-audit findings discovered during the final verification pass before pushing the CodeRabbit fixes.
|
Member
Author
Code Lawyer Activity SummaryResolved all currently known actionable feedback and the self-audit findings from this pass.
Verification before push:
All review threads are resolved as of this comment. |
Release Preflight
If you tag this commit as |
Member
Author
|
Follow-up cleanup landed in
Verification:
All currently verified review-discovered cleanup items are addressed in this branch update. |
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
Release posture
This PR puts v18 in release-candidate evidence posture, not public-release posture. Public release still requires final release-prep gates, package/version/tag work, operator release notes, residual raw content/property risk review, and an explicit guard against claiming end-to-end graph streaming in v18.
Verification
Pre-push IRONCLAD M9 passed:
Summary by CodeRabbit
New Features
Documentation
Tests