Skip to content

Two merge-time invariants have no test: params-slice migration chain integrity, and vendored MiniMax-H3 drift from upstream #104

Description

@lstein

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

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. main independently took the same version. On merge, paramsSlice.ts and types.ts auto-merged without a conflict, producing two consecutive steps sharing a from version:

if (state._version === 5) { /* flux2 fields */  state._version = 6; }   // wins
if (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

Hit twice: #77 and #82.

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions