feat(mise): support selector lockfile updates - #44615
Conversation
zeitlinger
left a comment
There was a problem hiding this comment.
Addressed all requested changes:
- Added a maintenance comment next to the Java LTS list directing future updates to follow the OpenJDK release roadmap.
- Removed the redundant
=== truecomparison. - Refactored the selector-lockfile logic into
extractSelectorLockedDependency, leavingextractToolEntryfocused on normal extraction and delegation. - Removed the invalid TOML literal-string
replaceAllescaping while preserving the existing quote style. - Added JSDoc for
ignoreUnstable.
I also merged the current main branch and resolved the conflict. The focused checks and all GitHub CI checks pass.
jamietanna
left a comment
There was a problem hiding this comment.
Some findings per Claude Opus 5:
Code review finished. Two blocking bugs, one working-tree security issue, plus minor notes.
## HIGH
**1. `lib/modules/manager/mise/extract.ts:104` — lockfile lookup uses the raw TOML key instead of the sanitised `depName`** (regression vs `main`, verified empirically)
Old code looked up `dep.depName` (options-in-name stripped via `optionInToolNameRegex`, trimmed). New code passes the raw `Object.entries` key. So for `"ubi:cli/cli[exe=gh]" = "2"` with a `[[tools."ubi:cli/cli"]]` lock entry:
- `main` → `lockedVersion: '2.63.0'`
- branch → no `lockedVersion`, dep never enters the new selector path, so Renovate rewrites `mise.toml` from `2` to a concrete version and **destroys the user's selector**
The backend-prefix fallback doesn't rescue it (only strips up to the first `:`). Quoted keys with surrounding whitespace (`" node " = "20"`) regress the same way. Fix: derive `depName` (trim + strip `[...]`) before the lookup, or move the lookup inside `extractToolEntry` after `depName` exists.
**2. `lib/modules/manager/mise/update-locked.ts:137` — returning `updated` silently disables the `mise lock` artifact refresh**
The success branch in `get-updated.ts` puts only the lock file into `updatedFileContents` and leaves `mise.toml` out of both maps. `getManagersForPackageFiles` matches against `managerPackageFiles['mise'] = {'mise.toml'}`, so `mise.lock` doesn't match and `updateArtifacts` never runs. Previously the `unsupported` return put `mise.toml` into `nonUpdatedFileContents`, which *did* trigger it.
Result: `node = "lts"` at `22.14.0` → PR writes `version = "22.15.0"` while every `[tools.node.platforms.*]` entry keeps 22.14.0's checksum/URL, and `mise install` fails checksum verification. `MiseLockTool` models `platforms` explicitly, so this is the normal lock-file shape. Either strip the stale `platforms` table or keep emitting the unchanged package file so artifacts still run.
## LOW
4. `extract.ts:298` — Java `lts` `allowedVersions` regex is `(?:\.|-|$)` but the partial-selector `precisionPattern` is `(?:\.|-|\+|$)`; a release like `25+36` gets filtered. Looks accidental.
5. `extract.ts:305` — `partialSelectorRegex` matches mise's non-version specifiers (`ref:main2`, `path:/opt/tools/1.2`), producing bogus `allowedVersions`. The `lockedVersion === version` guard rescues it in practice, so latent rather than broken — an explicit reject-list for `ref:`/`path:`/`sub-N:` would make the guard's load-bearing role explicit.
6. `extract.ts:356` — `currentRawValue` has no consumer for mise (only `deno` and `npm` read it). Dead metadata.
## Checked and fine
`generate.ts:59` newValue short-circuit (`isLockfileUpdate` still set at line 113, survives the `res.updates` filter); `index.ts:270` is-pinned and `index.ts:565` isCompatible bypasses; dep-level `allowedVersions`/`ignoreUnstable` reaching `filterVersions` with user `packageRules` still winning via the later `pre-lookup` pass; `RegExp.escape` output through RE2; `formatLockedVersion` vendor-prefix reattachment; `astTableForTool`/`getVersionKeyValue` degrading to `unsupported` rather than a wrong edit.
|
Addressed the actionable findings from the review:
Focused checks/type-check/tests pass, and all current GitHub checks are green. The review preamble mentions a working-tree security issue but does not identify a separate file, behavior, or reproduction beyond the findings above. |
|
Please remember:
|
This is in the PR body - but I admit that I sometimes forget about the commenting policy Yes — substantive assistance (AI-generated non-trivial portions of code, tests, or documentation). |
|
Yeah we added a new section to the PR template explicitly to cover PR comments, because it's hard to know if I'm talking to a human or an agent at times, and if there are issues, I don't know if the human's gonna step in or what :) |
There was a problem hiding this comment.
A few additional things that Claude Opus 5 has caught (from d2ceff9):
⏺ Code review(high · 5 findings)
⎿ lib/modules/manager/mise/update-locked.ts
● 126 [correctness] The hand-written lockfile version replacement
leaves the entry's `platforms` checksum/size/url
pointing at the previous version, and the
artifact refresh the comment relies on never
sees the updated content.
● 70 [correctness] `astTableForTool` parses with `tomlVersion:
'1.0'` while the schema path (lib/util/toml.ts)
parses with `'1.1'`, so TOML 1.1-only syntax
passes validation then throws in parseTOML.
lib/modules/manager/mise/extract.ts
● 110 [correctness] The lockfile lookup key changed from the dep's
final `depName` to the raw tool name, so tools
whose tooling config overrides `depName`
silently lose their `lockedVersion`.
● 326 [correctness] The generated `allowedVersions` regex is
anchored to the raw datasource version but only
tolerates a prefix present in the mise selector,
so github-backed tools with `v`-prefixed tags
match nothing.
● 327 [correctness] Pinning `allowedVersions` to the selector's
major means partial selectors can never receive
a major update in mise.toml again.
Review done — 5 findings reported above (3 high, 1 medium, 1 low). `pnpm vitest run lib/modules/manager/mise` passes (267 tests).
Two things outside the findings list:
**Noted but not filed:**
- `lookup/index.ts:563` disables `isCompatible` entirely for `isLockfileOnly` deps. Inert today (no mise tooling uses a versioning scheme with a compatibility group), but it's a broad escape hatch in shared code, and the mise case already compares against a concrete locked version.
- `extract.ts:305`'s hardcoded Java LTS list needs manual maintenance per the OpenJDK roadmap.
- `update-locked.ts:132`'s `!== undefined` check doesn't screen out the `null` that `get-updated.ts` can pass via `packageFileContent!`.
Finding 1 (`update-locked.ts:126`) is the one I'd fix first — the stale-checksum path produces a lockfile that fails `mise install`, and it directly contradicts the premise in the comment added by `b2180eae3c`.
Here's the answer:
|
|
Thanks - a couple of issues Opus still believes need to be fixed: Mostly interested in 313, 322, 116 and 212 |
addressed |
|
@zeitlinger mind merging (I don't have access) |
done - and also gave access |
jamietanna
left a comment
There was a problem hiding this comment.
Claude Opus 5 is still seeing some findings that might not be fixed:
⏺ Code review(review · 9 findings)
lib/modules/manager/mise/extract.ts
● 320 [correctness] getSelectorConfig only allows a `v?` prefix in `allowedVersions` when the datasource is github-releases, so numeric selectors on tools whose datasource emits prefixed raw versions (notably `node`) filter out every release.
● 298 [maintainability] The `lts` selector for java relies on a hardcoded allowedVersions list `/^(?:8|11|17|21|25)(?:\.|-|\+|$)/`, guarded only by a code comment.
lib/modules/manager/mise/update-locked.ts
● 38 [correctness] formatLockedVersion returns `newVersion` verbatim when the current locked value has no prefix, but the lockfile-only path deliberately bypasses `versioningApi.getNewValue()` — which is where node versioning strips the `v` — so a prefixed release string lands in mise.lock as-is.
● 81 [correctness] astTableForTool hard-codes `resolvedKey[2] === 0`, so with a multi-version tool the rewrite always targets the first lock entry regardless of which one corresponds to the extracted dep.
lib/workers/repository/update/branch/get-updated.ts
● 390 [correctness] removeUpdatedLockFileChanges is manager-agnostic: on any artifact error it splices out every lockfile entry written by updateLockedDependency, which can leave nothing to commit (mise) or commit a package-file change without its lock change (npm).
lib/modules/manager/mise/backends.ts
● 139 [correctness] The new unconditional `'^v?(?<version>.+)'` extractVersion for `github:` tools is unnecessary for concrete versions (semver.getNewValue already handles the bare case) and regresses pins that intentionally carry a `v`, contradicting the sibling ubi helper which preserves it.
lib/workers/repository/process/lookup/generate.ts
● 58 [correctness] The `isLockfileOnly` newValue override also requires `rangeStrategy === 'update-lockfile'`, but packageRules run after the dep object is merged in fetch.ts, so a rule can change rangeStrategy while leaving isLockfileOnly true — dropping into the else branch.
lib/modules/manager/mise/artifacts.ts
● 217 [correctness] When mise rewrites the lockfile, the refreshed content is added to `updatedArtifacts` while `updatedPackageFiles` still carries the pre-refresh content for the same path, so the commit file list holds two conflicting entries for one path.
do.sh
● 1 [hygiene] Three unrelated untracked files sit in the worktree — `do.sh` (contains `export RENOVATE_TOKEN=...` plus an Azure DevOps endpoint/repo), `a.js` (an unrelated example config), and `docs/usage/key-concepts/update-types.md` (an unrelated docs page).
So we don't have a hardcoded set of data in the mise manager. Co-authored-by: Claude Sonnet 5 <jamie.tanna+claude-code@mend.io>
Changes
latest,lts, partial versions, and vendor-prefixed partial versions as constraints resolved frommise.lock.mise.tomlselector.mise.lockversion updates, including vendor prefixes and quote styles.mise.updatePinnedDependencies: falseto preserve exact pins.Context
Discussion: #44418
AI assistance disclosure
Did you use AI tools to create any part of this pull request?
Documentation
How I've tested my work
I have verified these changes via:
Focused mise and lookup suites pass (336 tests), along with TypeScript, schema validation, formatting, linting, and documentation fence checks.