Fix conductor-migration CI regressions from PR #840 - #852
Merged
Conversation
PR #840 added NormalizeListDataType to methods.ts, which normalizes PAIR|EMPTY_LIST to LIST when inferring attachModuleMethod's argument/ return types. This made the CSG bundle's `as unknown as [DataType.PAIR]` workaround casts (added in #826, back when the inferred type still needed restating) incorrect, breaking tsc for the csg bundle - which in turn breaks `yarn install` itself, since the bundle is built as part of the workspace link step. It also caused @typescript-eslint/utils to resolve to three different versions across the workspace (8.58.2 via @vitest/eslint-plugin, 8.62.1 via the typescript-eslint meta-package's own pinned dependency, and 8.63.0 via lint-plugin's direct dependency), which made lint-plugin's rule tests fail to typecheck due to structurally incompatible RuleModule types pulled from different physical copies. The same fragmentation was also responsible for two failing modules-buildtools doc-generation tests. Pinning @typescript-eslint/utils via `resolutions` converges the whole family on a single version.
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR #840 introduced
NormalizeListDataTypeinlib/modules-lib/src/conductor/methods.ts, which normalizesPAIR|EMPTY_LISTtoLISTwhen inferringattachModuleMethod's argument/return types. This broke two things onconductor-migration:tscfailure, which actually breaksyarn installitself (the bundle is built as part of the workspace link step). The CSG bundle'sgroup/ungroupregistrations usedas unknown as [DataType.PAIR]workaround casts (added in Migrate CSG Module to Conductor #826) to compensate for the old inference behavior. With the new normalization, those casts are now actively wrong. Fix: drop the now-obsolete casts.@typescript-eslint/utilsversion fragmentation. Migrate thecurveandplotlymodules #840'syarn.lockupdate caused this package to resolve to three different versions across the workspace (8.58.2 via@vitest/eslint-plugin, 8.62.1 via thetypescript-eslintmeta-package's own pinned dependency, and 8.63.0 via lint-plugin's direct dependency). This made@sourceacademy/lint-plugin's rule tests fail to typecheck (structurally incompatibleRuleModule/RuleContexttypes pulled from different physical copies of the package), and was also responsible for two failingmodules-buildtoolsdoc-generation tests (repeat/rune bundle docs). Fix: pin@typescript-eslint/utilsviaresolutionsto converge the whole family on a single version (8.65.0), matching the existing pattern already used forvitest/vite.Verification
Manually ran the full check matrix locally (no CI runs on PRs into
conductor-migrationcurrently — see separate note), since this branch previously failed CI on the downstreamconductor-migration → masterPR (#680, run 30258399499):yarn installsucceeds (previously failed — CSG bundle build blocked it)csgbundle:tsc+ tests (25/25) passCsgtab: build +tsc+ tests passlint-plugin:tsc+ tests (54/54) pass (previously failed to typecheck)modules-buildtools: full test suite (148/148) passes, including the two previously-failing doc-generation testsyarn constraintscleanyarn lint:all— no new errors/warnings vs.conductor-migrationHEAD before this fixyarn buildtools manifestsucceeds🤖 Generated with Claude Code