Move the partial-apply gci test into the CI integration suite

[MatiasFernandez]

Moves one test out of the on-demand gci project (which never runs in CI) into the integration suite, which runs across the release matrix in both CI passes.

It was parked in gci/ on the premise that it had to commit. It does not: it drives applyInstVar with migrate=false, deleteHistory=false, and GsInstVarRefactoring>>applyDeselected:options:migrate:deleteHistory: only calls commitStructuralThenMigrate: when migrate or deleteHistory is requested — so the engine never commits on this path. Its three System commitTransaction calls were incidental scaffolding:

• the post-fixture commit was annotated "clean session — the engine may now abort", even though the test's whole point is that the engine never aborts;
• the mid-test UserGlobals removeKey: needs no commit — a session sees its own uncommitted removals;
• the finally block existed only to undo the other two.

Changes

• Moved stops at the first failure and reports the partial apply without aborting into refactoringInstVar.integration.test.ts (7 → 8 tests): reuses that file's existing helpers, gated with requireServerPluginFeature(pluginFeatures.refactoring, …), no commits, no try/finally, no hardcoded stoneVersion literal, ASCII-only Smalltalk for the 3.6.2 floor. Its own XIvItFailBase/XIvItFailSub fixture, kept apart from the shared one since the test deletes its subclass.
• Reduced gciInstVar.e2e.test.ts to its 2 remaining tests, whose doc comment now names commitStructuralThenMigrate: as the reason they stay: the engine commits on their paths, so their assertions exist because of that commit and no test rewrite removes it.
• Corrected the integration file's doc comment, which listed the mid-apply-failure case among what cannot live there.

Verification

Run against GemStone 3.6.2 (the matrix floor) with the server plugin installed.

• Target file: 8 passed, 0 failed. Both instVar files under --project gci: 3 passed, 0 failed.
• Full gate: npm run lint, npm run format:check and npm run compile clean; npm test → 4951 client / 322 server / 92 mcp-server passing, only the 10 pre-existing client skips.
• Confirmed transient: on a freshly reset stone the run leaves zero committed residue when probed from a new session, and it passes twice consecutively on a dirty stone.

🤖 Generated with Claude Code

[ericwinger]

🚫 failed.length === 1 doesn't say which change failed.

The scenario this test claims to pin down is "the base versioned, then the sub failed, and nothing after it ran". But consider a regression where applyChange: raises on the base after makeNewVersionOf: has staged the version — exactly the case the engine's own comment in applyDeselected: calls out, since applied is bumped inside applyClassChange: rather than after applyChange: returns. That gives applied == 1, failed.length == 1, partiallyApplied == true, committed == false and hasIvar(FAIL_BASE, 'tally') == true — every assertion here passes while describing the opposite scenario.

The engine already reports the discriminator: failures add: (Array with: change id with: change className with: e messageText), surfaced as label. One line closes it:

    expect(result.failed.length).toBe(1);
    expect(result.failed[0].label).toBe(FAIL_SUB);

I'd deliberately not assert on failed[0].error — the 'Class not found' text is engine/version surface and would be fragile across the 3.6.2→3.7.5 span. The label is the class name the change carries, so it's stable.

[ericwinger]

🚫 The comment promises a dropped check the test never makes.

shadowIt is compiled here so that "a bogus dropped report is detectable", but nothing in the test reads result.dropped — so the method currently does no work and the comment overstates what is covered. This is also point 4 of .claude/rules/refactoring-tests.md ("what could not be recompiled is reported, not dropped in silence").

Since the sub's change never runs, dropped must be empty — and asserting that is a genuinely independent way to state "nothing after the failure ran", which right now rests solely on applied === 1:

    expect(result.dropped).toEqual([]);

Either add that, or drop shadowIt and its comment. I'd add the assertion — it's the stronger of the two.

[ericwinger]

💭 The abort-detection diagnostic got a little weaker in the move — worth a decision either way.

In the gci original the fixture was committed, so "the engine aborted" showed up crisply as hasIvar(FAIL_BASE, 'tally') === false: the class still existed, just without the new ivar. Now that the fixture is uncommitted, an abort would take XIvItFailBase with it, so this line throws an undefined-symbol error out of exec instead, and the failure reads as "the fixture was never created" rather than "the engine aborted".

The test still fails either way, so this is diagnostics, not coverage. If you want the original's signal back, a probe that can't throw makes the cause explicit:

    expect(
      exec(`(System myUserProfile symbolList objectNamed: #${FAIL_BASE}) notNil printString`).trim(),
    ).toBe('true'); // the engine did not abort the fixture out from under us
    expect(hasIvar(FAIL_BASE, 'tally')).toBe(true);

[ericwinger]

💭 Optional: no copy-forward assertion on the partially-applied version.

FAIL_BASE is defined with no methods, so when the apply re-versions it, the "new method dictionary starts empty" hazard that .claude/rules/refactoring-tests.md §1 is built around is never exercised on the failure path. The happy-path tests above do cover copy-forward, but they cover it on a clean apply.

If it's cheap, one method on the base plus one survival assertion would extend this test to the partial case:

    q.compileMethod(session(), FAIL_BASE, false, 'accessing', 'keepMe\n\t^ x');
    // ...after the apply:
    expect(includesSelector(FAIL_BASE, 'keepMe')).toBe(true);

Entirely reasonable to call this out of scope for a move-only PR.

[ericwinger]

💭 Slightly imprecise, and the imprecision is load-bearing for this PR's own argument.

applyDeselected:options:migrate:deleteHistory: doesn't call commitStructuralThenMigrate: whenever migrate or delete-history is requested — it's guarded by the structural apply having succeeded first:

failures isEmpty ifTrue: [
    (aBool or: [dBool]) ifTrue: [
        self commitStructuralThenMigrate: aBool deleteHistory: dBool on: failures]].

That's precisely why the moved test stays safe even in the hypothetical where someone later passes migrate: true to it. Suggest: "…calls commitStructuralThenMigrate: once the structural apply has succeeded, whenever migrate or deleteHistory is requested, because…".