You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Context: landing the 13-PR MiniMax H3 stack (#56-#84). Five of the defects hit during that work were invisible in any individual PR — they only existed once two branches met, and git reported a clean auto-merge for most of them. Two classes have no test today.
Two others already do, and worked: test_bundled_workflow_nodes_match_invocation_registry caught the node-version mismatches in #63 and #64, and openapi-checks / typegen-checks cover generated-schema drift. Nothing needed there.
1. Params-slice migration chain has no structural invariant test
Each PR added a persisted-state migration step at what was, on its own branch, the next free version. main independently took the same version. On merge, paramsSlice.ts and types.tsauto-merged without a conflict, producing two consecutive steps sharing a from version:
if(state._version===5){/* flux2 fields */state._version=6;}// winsif(state._version===5){/* H3 fields */state._version=6;}// dead code
The second step never runs. Its fields are required with no default, so zParamsState.parse() throws and redux-persist wipes the entire params slice — prompt, seed, dimensions, every model selection — on upgrade.
The existing tests in paramsSlice.test.ts are all per-migration (backfills X when migrating from vN). They catch this only if a test happens to exercise a blob at exactly the colliding version, and they report it as a confusing downstream parse error rather than naming the cause. Nothing asserts the chain's shape.
Suggested test
Two assertions over the migrate function's source, or over a declarative step table if one is preferred:
no two steps share a from version
the chain terminates at the _version literal in zParamsState
Ad-hoc version used during the stack:
grep -o "state._version === [0-9]*" paramsSlice.ts | sort | uniq -d # must be empty
grep -n "_version: z.literal" types.ts # must equal the chain terminus
A real test should assert the same two properties. It is O(1) to write, cannot be defeated by a merge resolution, and fails with the actual cause.
2. Vendored MiniMax-H3 files can drift from upstream with nothing to catch it
invokeai/backend/minimax_h3/ holds five files vendored from huggingface/diffusers@abc5e9bf71 (the unreleased minimax-h3 branch). The contract is stated in invokeai/backend/minimax_h3/__init__.py: keep them byte-identical to upstream apart from the absolute-import rewrite, so that re-syncing when a tagged diffusers ships the H3 classes is a clean copy rather than a manual re-application.
Two ways it drifted, neither detected:
ruff format reached them. Any branch created before the [tool.ruff.format] exclude landed had its vendored files rewrapped — one line each in packing.py, transformer_minimax_h3.py, autoencoder_kl_minimax_h3_audio.py.
First-party code was added into a vendored module.feat(minimax-h3): dropdowns for canvas size and frame count #82 originally added MINIMAX_H3_VIDEO_FRAME_CHOICES, _validated_aspect_ratio and resolve_lowres_canvas_size directly to packing.py, and refactored the vendored resolve_canvas_size to share a helper. (Moved to presets.py before merge.)
Both are silent. Nothing fails; the cost lands on whoever does the eventual upstream re-sync and finds it is no longer a copy.
Related, and worth guarding at the same time: the exclude was initially written as a directory glob, invokeai/backend/minimax_h3/*.py. That was right when everything in the package was vendored, but the package now also holds first-party modules (presets.py, sampling.py, denoise.py, int8_convrot.py, pruned_adaln_lora.py, taehv_decoder.py) which would silently stop being formatted. It is now a list of five named files — a test should keep it that way.
Suggested test
Vendor the five upstream files as fixtures (or check in their hashes) and assert each differs from its vendored copy only by lines matching ^(from|import) . That catches both formatter drift and added first-party code, and it fails with a diff pointing at the offending lines.
A cheaper variant, if fixtures are unwelcome: assert the [tool.ruff.format] exclude lists exactly the five vendored files by name, and that no other module in the package appears in it. That catches the glob regression but not content drift.
Why file this
Neither check is about MiniMax H3 specifically. The migration-chain one applies to every persisted redux slice with a version chain — uiSlice has its own, and the same collision is available there. The vendoring one applies wherever we vendor upstream code under a byte-identical contract.
Both are cheap. The migration test in particular would have prevented a slice-wiping bug from reaching main three separate times, each of which git presented as a clean merge.
Context: landing the 13-PR MiniMax H3 stack (#56-#84). Five of the defects hit during that work were invisible in any individual PR — they only existed once two branches met, and git reported a clean auto-merge for most of them. Two classes have no test today.
Two others already do, and worked:
test_bundled_workflow_nodes_match_invocation_registrycaught the node-version mismatches in #63 and #64, andopenapi-checks/typegen-checkscover generated-schema drift. Nothing needed there.1. Params-slice migration chain has no structural invariant test
Hit three times: #59, #61, #62.
Each PR added a persisted-state migration step at what was, on its own branch, the next free version.
mainindependently took the same version. On merge,paramsSlice.tsandtypes.tsauto-merged without a conflict, producing two consecutive steps sharing afromversion:The second step never runs. Its fields are required with no default, so
zParamsState.parse()throws and redux-persist wipes the entire params slice — prompt, seed, dimensions, every model selection — on upgrade.The existing tests in
paramsSlice.test.tsare all per-migration (backfills X when migrating from vN). They catch this only if a test happens to exercise a blob at exactly the colliding version, and they report it as a confusing downstream parse error rather than naming the cause. Nothing asserts the chain's shape.Suggested test
Two assertions over the migrate function's source, or over a declarative step table if one is preferred:
fromversion_versionliteral inzParamsStateAd-hoc version used during the stack:
A real test should assert the same two properties. It is O(1) to write, cannot be defeated by a merge resolution, and fails with the actual cause.
2. Vendored MiniMax-H3 files can drift from upstream with nothing to catch it
Hit twice: #77 and #82.
invokeai/backend/minimax_h3/holds five files vendored fromhuggingface/diffusers@abc5e9bf71(the unreleasedminimax-h3branch). The contract is stated ininvokeai/backend/minimax_h3/__init__.py: keep them byte-identical to upstream apart from the absolute-import rewrite, so that re-syncing when a tagged diffusers ships the H3 classes is a clean copy rather than a manual re-application.Two ways it drifted, neither detected:
ruff formatreached them. Any branch created before the[tool.ruff.format]exclude landed had its vendored files rewrapped — one line each inpacking.py,transformer_minimax_h3.py,autoencoder_kl_minimax_h3_audio.py.MINIMAX_H3_VIDEO_FRAME_CHOICES,_validated_aspect_ratioandresolve_lowres_canvas_sizedirectly topacking.py, and refactored the vendoredresolve_canvas_sizeto share a helper. (Moved topresets.pybefore merge.)Both are silent. Nothing fails; the cost lands on whoever does the eventual upstream re-sync and finds it is no longer a copy.
Related, and worth guarding at the same time: the exclude was initially written as a directory glob,
invokeai/backend/minimax_h3/*.py. That was right when everything in the package was vendored, but the package now also holds first-party modules (presets.py,sampling.py,denoise.py,int8_convrot.py,pruned_adaln_lora.py,taehv_decoder.py) which would silently stop being formatted. It is now a list of five named files — a test should keep it that way.Suggested test
Vendor the five upstream files as fixtures (or check in their hashes) and assert each differs from its vendored copy only by lines matching
^(from|import). That catches both formatter drift and added first-party code, and it fails with a diff pointing at the offending lines.A cheaper variant, if fixtures are unwelcome: assert the
[tool.ruff.format]exclude lists exactly the five vendored files by name, and that no other module in the package appears in it. That catches the glob regression but not content drift.Why file this
Neither check is about MiniMax H3 specifically. The migration-chain one applies to every persisted redux slice with a version chain —
uiSlicehas its own, and the same collision is available there. The vendoring one applies wherever we vendor upstream code under a byte-identical contract.Both are cheap. The migration test in particular would have prevented a slice-wiping bug from reaching
mainthree separate times, each of which git presented as a clean merge.