fix(onboard): explain a gateway database with a modified migration - #9405
fix(onboard): explain a gateway database with a modified migration#9405udsy19 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 9 remain after this review. 📝 WalkthroughWalkthroughGateway startup now classifies modified applied migrations as incompatible database state. Onboarding errors and troubleshooting documentation describe both missing and modified migrations. Regression tests cover classification and recovery guidance. ChangesMigration incompatibility diagnostics
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR improves gateway-start diagnostics for modified migrations and documents the recovery path; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 1 semantic terminology decisionTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
jyaunches
left a comment
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Why this blocks
src/lib/onboard/docker-driver-gateway-failure.test.ts:181-209 adds a second recovery test with its own temporary directory, log write, reporter call, output collection, assertions, and cleanup. That recovery contract already exists at src/lib/onboard/docker-driver-gateway-failure.test.ts:147-179, and the reporter routes both sqlx texts through the same database_migration_incompatible branch. The signature-specific behavior is already pinned by the positive and false-positive classifier cases at src/lib/onboard/gateway-start-failure.test.ts:76-91. Keeping another full recovery scenario makes a one-alternation behavior change account for about 30 avoidable test lines and creates two places to maintain the same output contract.
Refactor direction
Delete the new reporter test. Change the existing recovery fixture at src/lib/onboard/docker-driver-gateway-failure.test.ts:155-157 to use the has been modified signature, then add the new diagnostic-text assertion to that test. The remaining reporter cases at lines 255, 286, 310, 340, and later continue to exercise the original missing-migration signature. Keep classifyGatewayStartFailure as the single owner of the two signature variants and printIncompatibleGatewayDatabaseRecovery as the single owner of their shared recovery.
Expected result
Both sqlx signatures, the false-positive guard, the database path, the archive command, and the recovery wording remain covered. The PR removes roughly 29 net lines and reduces the current net growth from 63 lines to about 34 without adding a helper or another abstraction.
sqlx reports a gateway database written by a newer OpenShell with one of two texts. Gateway start explains "is missing in the resolved migrations" but passed "migration N was previously applied but has been modified" through verbatim, with no database path, cause, or remedy. Classify both texts as an incompatible gateway database, print the same named-database recovery, and name both texts in the troubleshooting page. Refs: NVIDIA#9293 Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
4d1dca1 to
ab03ab4
Compare
|
Thanks for the review. Applied as directed. Deleted the second reporter test. The existing recovery test now writes the Net growth is now 34 lines (43 added, 9 removed), down from 63. I also rebased onto current |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 participants
Summary
A gateway database written by a newer OpenShell fails gateway start with one of two sqlx texts. Gateway start explains the
is missing in the resolved migrationstext and prints a named-database recovery, but passes the siblinghas been modifiedtext through verbatim, so that failure still names no database, no cause, and no remedy. Gateway start now classifies both texts as an incompatible gateway database and prints the same recovery, and the troubleshooting page names both texts.Related Issue
Refs #9293
This change does not close #9293 on its own, and it intentionally uses no closing keyword. The literal text in that report,
migration 6 was previously applied but is missing in the resolved migrations, is already classified onmain: #8995 addedclassifyGatewayStartFailureand #8992 refined it. Neither is contained inv0.0.103, the release the reporter read the database with; both first ship inv0.0.109. The reported repro therefore no longer produces a raw migration error on a current release, and #9293 can be closed as fixed-in-release independently of this PR.What remains is the sibling signature of the same defect. sqlx
0.8.6— the version pinned by OpenShell's workspaceCargo.toml— reports a database written by a newer OpenShell through twoMigrateErrorvariants:VersionMissingmigration {0} was previously applied but is missing in the resolved migrationsVersionMismatchmigration {0} was previously applied but has been modifiedmainclassifies only the first. The second occurs on the same downgrade when the newer OpenShell rewrote an applied migration instead of appending one, and it still reaches the user verbatim. This PR closes that path.Changes
src/lib/validation.ts:classifyGatewayStartFailurematches both sqlx signatures and returnsdatabase_migration_incompatiblefor each. The alternation is anchored to the sharedmigration N was previously appliedprefix, sohas been modifiedalone does not classify. TheGatewayStartFailuredoc names both signatures.src/lib/onboard/docker-driver-gateway-failure.ts: the incompatible-database explanation now readsThe database records a migration that this OpenShell version does not include, or defines with different contents.so it is accurate for both signatures. No other line of the recovery changes.docs/reference/troubleshooting.mdx: the section covering this failure names both error texts, states what each one means, and is retitled fromReports a Missing MigrationtoReports an Incompatible Migration. No page links to the previous heading.src/lib/onboard/gateway-start-failure.test.ts: a classifier test for the modified-migration text, and a negative guard asserting thatthe file containing migration 6 has been modified on diskstaysunknown.src/lib/onboard/docker-driver-gateway-failure.test.ts: a reporter test asserting the modified-migration failure names the database file, the cause, and the archive-and-reonboard recovery.Type of Change
Quality Gates
DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable — hooks are not installed in this worktree;git fetch origin main && npm run validate:prpassed with zero problems.npx vitest run --project cli src/lib/onboard/gateway-start-failure.test.ts src/lib/onboard/docker-driver-gateway-failure.test.ts→ 39 passed, 3 new. Reverting only the two source files toorigin/mainand keeping the tests fails both new positive tests, so they pin the new behavior.npm run typecheck:cli→ clean.npx oxlinton the four changed source files → no findings.npm run test:changedwas not run to completion on this macOS host; its affected-test selection includes subprocess-spawning lanes that need the GNU utilities described undermacOS Test DependenciesinCONTRIBUTING.md.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not run. This change is a focused onboarding diagnostic and adds no runtime or test-harness surface; the targeted CLI tests,typecheck:cli, oxlint,npm run docs, andnpm run validate:prcover the changed contracts.npm run docsbuilds without warnings (doc changes only) —npm run docs→Found 0 errors and 2 warnings. Both warnings reproduce on the unmodified page set and are unrelated to this change.Follow-Up Observation
Not changed here, and offered only as a question for maintainers:
classifyGatewayStartFailurehas one non-test caller,reportDockerDriverGatewayStartFailure. A gateway start that terminates outside that reporter therefore never produces this diagnosis, even when the log carries either migration signature. If that is a gap worth closing rather than intended scoping, I am happy to open a separate issue.Signed-off-by: Udaya Tejas udayatejas2004@gmail.com
Summary by CodeRabbit
Bug Fixes
.incompatiblestate directory.Documentation