Uh oh!
There was an error while loading. Please reload this page.
fix(dashmate): migrate Drive and DAPI images onto the prerelease line - #4235
Conversation
The drive and rs-dapi image tags are derived from the package version, so the tag changes when a major crosses from a stable release into a prerelease line. Config files store resolved values, and no migration re-pinned those two images above 4.0.0, so an operator upgrading a 4.0.x install to a 4.1.0-rc dashmate kept running dashpay/drive:4 and dashpay/rs-dapi:4 instead of the 4-rc images. Add a 4.1.0-rc.3 migration that re-pins both images from the base config. Keyed at the next release rather than the released 4.1.0-rc.2 because the runner skips fromVersion === toVersion. Only stock Dash tags for this major (4, 4-rc, 4-beta, 4-dev, ...) are moved; an image the operator chose themselves is left alone, matching the guarded gateway migration. Test would have caught this in CI: 'should move a stock 4.x platform image onto the current base image' fails before the migration (dashpay/drive:4 is kept) and passes after. The companion guard test fails against an unguarded re-pin, so it pins the guard rather than snapshotting behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
⛔ Blockers found — Sonnet deferred (commit 7acc32e) |
Removes the two tests added alongside the 4.1.0-rc.3 migration: the red-green reproduction of the stale stock image tag, and the pin on the guard that keeps a custom image untouched. The migration itself is unchanged and now has no direct coverage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lease Replaces the per-migration test with one parametrized over the migration table itself: for every release an operator can be sitting on, a config stamped at that version carrying that era's derived image tags must migrate onto the images a fresh install produces today. Future migrations are covered without touching the test. The pre-existing tests could not catch a missing re-pin above 4.0.0. Both start below that key - the v0.25.0 fixture at 0.25.0 and the recent-version test at 4.0.0-rc.2 - so both cross the unconditional '4.0.0' re-pin and are rescued by it. Only a cohort at or above 4.0.0 is exposed, and nothing exercised one. Releases older than 1.3.0-dev.3 are excluded: their migrations rewrite the config shape of their own era and throw on a config built from current defaults. That range stays covered by the v0.25.0 fixture test. Test would have caught this in CI: fails without the 4.1.0-rc.3 migration with 'drive image not refreshed for base upgrading from 4.0.0', passes with it. Verified the two pre-existing tests both still pass in that same state, which is what let the bug ship. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The migration targets both version-derived image paths, uses base tags that are consistent across all default presets, and safely skips missing nested Docker sections. However, its broad stock-tag patterns overwrite operator-selected same-repository tags despite the stated preservation guarantee, and the generalized test does not model the intended rc.2-to-rc.3 repair boundary. The image guard must be tightened before merge.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/dashmate/configs/getConfigFileMigrationsFactory.js`:
- [BLOCKING] packages/dashmate/configs/getConfigFileMigrationsFactory.js:1597-1598: Stock-image patterns also match custom alphabetic tags
The optional `-[a-z]+` suffix accepts any alphabetic identifier rather than only identifiers generated by Dash releases. Consequently, locally retagged or otherwise operator-selected images such as `dashpay/drive:4-patched`, `dashpay/drive:4-custom`, or `dashpay/rs-dapi:4-latest` are silently replaced with the base image. This contradicts the migration's explicit guarantee that custom, vendor-patched, and floating tags survive. Repository tag history shows the managed prerelease identifiers are `alpha`, `beta`, `dev`, `hotfix`, `pr`, and `rc`; matching those explicitly follows the tight allowlist used by the earlier 3.x image migration.
In `packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js`:
- [SUGGESTION] packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js:118-136: Cohort sweep does not exercise the rc.3 migration boundary
This sweep iterates migration keys rather than actual operator releases, so it omits released 4.1 cohorts such as beta.1, beta.2, and rc.1. More importantly, the rc.2 case starts with an already-correct `:4-rc` image and short-circuits because the current package and `fromVersion` are both `4.1.0-rc.2`; the rc.3 case is a downgrade and invokes no migration. The 4.0.0 case passes only because the runner has no `toVersion` upper bound and therefore executes the future rc.3 migration while targeting rc.2. Add a focused `4.1.0-rc.2` config carrying inherited `:4` images migrated to `4.1.0-rc.3`, plus preservation cases for same-repository custom tags such as `:4-custom` or `:4-patched`.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The guard admitted any lowercase suffix, so it matched operator-chosen tags in the dashpay namespace as well as the ones releases publish. dashpay/drive:4-patched, :4-local and :4-mybuild were all silently re-pinned onto the stock image. That is reachable: dashmate supports running a locally built image under the operator's own tag, and docs/config/drive-abci.md documents the field as operator-settable with a dashpay/drive example. Listing the six prerelease identifiers releases actually publish fixes it; an identifier a later release invents is now skipped rather than guessed at, which leaves the operator untouched instead of overwriting them. Add the negative half of the guard, which had no coverage at all - deleting the guard entirely left the suite green. Every operator image in it except the private-registry one sits in the dashpay namespace so the suffix is what must reject them; a foreign registry passes on the namespace alone and never exercises the suffix, which is why the earlier attempt at this test agreed with the bug. Pinned at 4.0.0, the only starting point where this migration acts alone. Flatten the path-keyed map to direct property access, matching the sibling migrations. The map wrote through base.get(), so grepping for platform.drive.abci.docker.image missed this migration entirely, and a typo would have silently disabled it on both sides instead of throwing. Collect stale releases and assert once instead of failing fast, so one release cannot hide the others, and sort the table walk - the migration keys are not in semver order. Correct the comment claiming operator images are always preserved: below 4.0.0 the unconditional re-pin in the '4.0.0' migration overwrites them first. Test would have caught this in CI: 'should move only the stock version-derived tags and leave operator images alone' fails against the previous regex with 'operator drive image dashpay/drive:4-patched was overwritten', and against no guard at all; passes with the narrowed one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rs-dapi image was derived from the drive image by string replacement,
which supplied both the seed and the expectation. A case added later
without the substring would have seeded the rs-dapi slot with a drive
image and passed while testing nothing. Name both images per case.
Also record why the major in the guard is fixed at 4: it matches the tag
being migrated away from, not the one being migrated to, so a later major
needs its own migration rather than an edit here. That is the opposite of
what copying this block would suggest.
No behaviour change. Guard test still fails against the loose regex
('operator drive image dashpay/drive:4-patched was overwritten') and
against no guard at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>The comment had grown to 26 lines over a 10-line migration, against 6-8 on its neighbours, because each review point was answered with its own paragraph. Keep the four things a reader needs - why the re-pin exists, why the key is one release ahead, why the identifiers are listed rather than matched loosely, and why the major stays 4 - and drop the rest. The paragraph about configs older than 4.0.0 crossing the unconditional re-pin only existed to walk back a claim that operator images are always preserved; the claim is now narrowed instead, so the caveat is unneeded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The 4.1.0-rc.3 migration correctly re-pins stock Drive and rs-dapi v4 images across all configured networks, safely handles partial shapes, remains idempotent, and now preserves operator-selected images through an explicit release-tag allowlist. No functional blocker remains, but the tests still do not directly exercise an rc.1/rc.2-stamped config retaining inherited stable :4 images as it crosses the rc.3 migration boundary.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— general (completed)
Source: reviewersgpt-5.6-sol+claude-sonnet-5; final verifiergpt-5.6-sol(fallback after unparseable Sonnet verifier).
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js`:
- [SUGGESTION] packages/dashmate/test/unit/config/configFile/migrateConfigFileFactory.spec.js:69-80: Cohort sweep does not exercise the rc.3 migration boundary
No test directly verifies the affected persisted state: a config stamped `4.1.0-rc.1` or `4.1.0-rc.2` while retaining inherited `dashpay/drive:4` and `dashpay/rs-dapi:4` images, upgraded through `4.1.0-rc.3`. The focused case starts at `4.0.0-rc.2` with already-current `:4-rc` images and also crosses the existing unconditional `4.0.0` re-pin, so it passes independently of the new migration. In the table sweep, the rc.2 case returns immediately because it equals the current package version, while the rc.3 case is a downgrade to rc.2 and selects no migration because the runner uses strict `semver.gt`; the 4.0.0 and guard cases reach rc.3 only because the runner has no `toVersion` upper bound. Consequently, moving the repair into the existing rc.2 migration would leave the suite green even though a real rc.2-stamped config would skip it. Add a focused migration from `4.1.0-rc.2` to `4.1.0-rc.3`, seed both images with `:4`, and assert that both become the base rc images.
The table walk upgraded towards the version in package.json and seeded each release with its own derived tag. Both are wrong, and together they left the newest migration untested. package.json only equals the newest migration key in the release that ships it; the rest of the time the package trails it, so a config stamped at or above the package version either short circuits on fromVersion === toVersion or selects nothing. Both 4.1.x rows ran zero migrations. Upgrade towards the newest migration key instead. Seeding a release's own tag makes the interesting state unreachable. A config stamped at a prerelease usually carries the stable tag it inherited, because operators arrive by upgrading rather than installing fresh - that is the whole bug. Seed every tag a release of the same major published at or before each one. The newest release is excluded: its own migration is what re-pins, so a config only reaches that stamp by running it. Test would have caught this in CI: moving the re-pin from the '4.1.0-rc.3' key to '4.1.0-rc.2' left the suite green before, and now fails with '4.1.0-rc.2 carrying :4 -> base: drive dashpay/drive:4, expected dashpay/drive:4-rc' - a real rc.2-stamped config would have skipped the migration entirely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The published prerelease identifiers and the pattern built from them now live in src/config/stockImages.js, so the migration reads as what it does rather than how it recognises a published tag, and the identifier list has one home. C2, which replaces stored derived values with resolved-on-read defaults, needs the same distinction between a published tag and one the operator set. The guard test keeps its own copy of the identifier list and asserts it matches the shared one, so adding an identifier there without deciding it should move operators fails rather than silently widening what the migration overwrites - the same widening that let the earlier loose pattern overwrite operator images. Verified: adding 'local' to the shared list fails with 'the published prerelease identifiers changed; confirm the new one should move operators'. 158 passing, lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Issue being fixed or feature implemented
dashmate derives the Drive and rs-dapi image tags from the package version in
getBaseConfigFactory.js— the major on a stable release, the major plus the prerelease identifier on a prerelease:A fresh install on 4.1.0-rc.x therefore gets
dashpay/drive:4-rc/dashpay/rs-dapi:4-rccorrectly. But config files store resolved values, so existing operators only move when a migration re-pins them — and no migration re-pinned those two images above'4.0.0':'4.0.0'migration re-pins both from base config, moving operators onto the stable:4images;'4.1.0-rc.2'migration re-pins only the gateway Envoy image.Result: an operator upgrading a 4.0.x dashmate to a 4.1.0-rc dashmate keeps running the 4.0 stable Drive/DAPI images instead of the 4.1 rc images — a silent version mismatch between the node's dashmate and the platform binaries it runs.
What was done?
A
'4.1.0-rc.3'migration ingetConfigFileMigrationsFactory.jsre-pinningplatform.drive.abci.docker.imageandplatform.dapi.rsDapi.docker.imagefrom the base config.Migration key —
4.1.0-rc.3.migrateConfigFileFactory.jsshort-circuits whenfromVersion === toVersion, then applies every key wheresemver.gt(key, fromVersion)insemver.compareorder. Keying at the already-released4.1.0-rc.2would never fire for an operator already on rc.2. Keying one release ahead sorts it after'4.1.0-rc.2'and covers every affected cohort: all 4.0.x, plus 4.1.0-rc.1/rc.2. This follows the convention documented in the'4.0.0-rc.3'and'4.1.0-rc.2'migration comments.Guard — only the tags releases actually publish.
The identifiers are listed rather than matched loosely. A looser
4(-[a-z]+)?also matches operator-chosen tags in the same namespace —dashpay/drive:4-patched,:4-localand:4-mybuildwould all have been silently re-pinned onto the stock image. That is reachable:isServiceBuildRequired.jslets operators run a locally built image under their own tag, anddocs/config/drive-abci.mddocuments this field as operator-settable with adashpay/drive:*example. All six identifiers are verified against shipped git tags (dev158,pr84,rc40,alpha11,beta10,hotfix5) and against whatsemver.prerelease(v)[0]can yield —2.1.0-pr.2716.1→pr,3.0.1-hotfix.1→hotfix.An identifier a later release invents is skipped rather than guessed at, which leaves the operator untouched instead of overwriting them; the table-walk test forces a migration at the release that invents it.
The major stays at
4permanently — it matches the tag being migrated away from, not the one being migrated to, so a later major needs its own migration rather than an edit here.How Has This Been Tested?
Two complementary tests in
migrateConfigFileFactory.spec.js.1.
should refresh version-derived images from every config version that has a migration— parametrized over the migration table itself rather than written per migration. For every config version an operator can be sitting on, a config stamped at that version carrying that era's derived tags must migrate onto the images a fresh install produces today. Future migrations are covered without touching the test. Fails without the migration:2.
should move only the stock version-derived tags and leave operator images alone— pins both halves of the guard. Pinned atconfigFormatVersion 4.0.0, the only starting point where this migration acts alone: below it the unconditional re-pin in'4.0.0'overwrites every image regardless.Mutation results — each of these shipped green before this test existed, and now fails:
4(-[a-z]+)?$anchor dropped4and4-rcWhy the pre-existing tests never caught this. The
'4.0.0'migration re-pins unconditionally, so every cohort below that key is rescued by it for free. The v0.25.0 fixture test starts at0.25.0and the recent-version test at4.0.0-rc.2, so both cross it. Only a cohort at or above4.0.0was exposed, and nothing exercised one.Releases older than
1.3.0-dev.3are excluded from the walk — the migration at that key readscore.log.file.categories, which current configs no longer have, so it and everything older throw against a config built from current defaults. That range stays covered by the v0.25.0 historical fixture test.Commands run:
yarn workspace dashmate run test:unit— 158 passingyarn workspace dashmate exec eslint configs/getConfigFileMigrationsFactory.js test/unit/config/configFile/migrateConfigFileFactory.spec.js— cleanNot tested: an end-to-end upgrade of a real 4.0.x node, which would need a live masternode on the 4.0 line.
Notes for the release
Merge timing matters. The key must stay strictly above every released version at merge time.
migrateConfigFileFactory.jsfilters with strictsemver.gt, so an operator whose config is stamped exactly4.1.0-rc.3would be excluded permanently, not just once. If this does not land in the 4.1.0-rc.3 release, bump the key to whatever release it lands in.Installing an RC dashmate moves every configured network, mainnet included, onto the RC images. One
config.jsonholds every network, and any command migrates all of them. An operator running mainnet ondashpay/drive:4who installs an RC to help test testnet will find their mainnet config re-pinned todashpay/drive:4-rc, pulled on the nextdashmate --config=mainnet update. This matches what a fresh install of the same dashmate already writes to the mainnet config, so it is existing design rather than something this migration invents — but it now reaches the 4.0.x upgrade cohort too. Worth calling out in the RC release notes.Downgrading does not restore the images. Going back to a 4.0.x dashmate leaves the config on
4-rc, because the older dashmate has no migration keyed above it. Also worth a release-note line.At the rc → stable boundary this recurs by design. Nothing re-pins once a config reaches
4.1.0-rc.3, so cutting4.1.0stable needs its own'4.1.0'migration. The table-walk test goes red in that release's CI precisely to force it — the fix is to add the migration, not to delete or weaken the test. Raising the floor constant to dodge it trips theexpect(releases).to.include(newest)assertion, and a no-op migration at the new key still leaves the walk red.Breaking Changes
None. The migration moves operators from stock Dash image tags onto the tag the base config already derives for the installed dashmate version — what a fresh install of that version gets. Operator-chosen images are untouched, and re-running is idempotent.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code