fix: split Renovate node artifact updates#1052
Conversation
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: CONDITIONAL
This PR fixes a real grouping problem — @types/node must not share a group with Docker/custom-manager Node surfaces, since skipArtifactsUpdate only suppresses artifact regeneration when every upgrade in the group carries the flag. Isolating it with groupName: null + matchPackageNames: ['@types/node'] is the correct mechanism for that specific dependency.
However, the diff does more than "split out" @types/node: it replaces the previous broad rule (matchManagers: ['bun', 'npm']) entirely, narrowing skipArtifactsUpdate to a single package. The original rule's own comment stated the failing install-tool bun artifact path affects both the bun manager (direct bun.lock updates) and the npm manager (any package.json dep update that touches bun.lock). After this change, every other npm dependency update (eslint, typescript, vitest, semantic-release, all @actions/*, …) and every bun-manager lockfile update will once again hit the failing renovate/artifacts path that the original rule existed to suppress.
Please confirm whether the broad artifact-skip is now provided elsewhere (e.g. the extended github>fro-bot/.github preset). If it is not, this reintroduces the original synthetic-status failure for the majority of update branches.
Blocking issues
- Artifact-skip coverage narrowed from all npm/bun updates to only
@types/node(.github/renovate.json5:146-149). The previous rule suppressedskipArtifactsUpdatefor the entirebunandnpmmanagers; the replacement scopes it to a single package. With a committedbun.lock(bun manager active) and ~30 other npm dependencies, non-@types/nodeupdates will again trigger the failing containerbaseinstall-tool bunpath and failrenovate/artifactson their branches. Either restore a broadskipArtifactsUpdaterule formatchManagers: ['bun', 'npm']in addition to the@types/nodeisolation rule, or document where the broad skip now comes from (extended preset). The retained comment still asserts the failure "affects both managers," which is inconsistent with the single-package scope.
Non-blocking concerns
- The comment block is the rule's only documentation; if the broad skip is intentionally delegated to the preset, add a one-line pointer so a future reader doesn't re-add the rule and reintroduce the grouping conflict.
Missing tests
- None expected for a Renovate config;
renovate-config-validator(already run per the PR description) is the only realistic gate. Note that the validator confirms schema validity only — it does not detect the behavioral regression above, so the validator passing is not sufficient evidence the artifact path stays suppressed.
Risk assessment (MED)
- Likelihood of regression: MED-HIGH. If the broad skip is not provided elsewhere, the original
renovate/artifactsfailure returns for all non-@types/nodeupdate branches — the exact symptom the prior rule prevented. - Security exposure: LOW. Config-only;
postUpgradeTasksbehavior is unchanged. - Blast radius: MED. Scoped to Renovate update branches in this repo; no runtime/action code paths affected, but it touches every future dependency PR.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 28290478410 |
| Cache | hit |
| Session | ses_0f6bf678effewfnyxx0N87xqcT |
94a4ae1 to
16c36c4
Compare
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
Re-review after 16c36c4. The author resolved the blocking issue from my prior review.
The new revision keeps the broad artifact-skip rule (matchManagers: ['bun', 'npm'] + skipArtifactsUpdate: true, lines 135-142) intact and adds a separate rule (lines 143-151) that isolates @types/node with groupName: null. Because packageRules apply cumulatively to a matching dependency, @types/node (npm manager) now matches both rules: it inherits skipArtifactsUpdate: true from the broad rule and gets un-grouped by the isolation rule. That is exactly the intended behavior — the failing containerbase install-tool bun artifact path stays suppressed for all npm/bun updates, while @types/node no longer risks being grouped with Docker/custom-manager Node surfaces (where a mixed group would defeat the per-upgrade skipArtifactsUpdate semantics).
Verified:
- Broad
skipArtifactsUpdatecoverage retained for the fullbun+npmmanagers (my prior blocking concern). - Docker
noderule (lines 158-164) is a separate manager/datasource and won't be grouped with the npm@types/nodeupdate. - Comment inconsistency from the prior revision is fixed: the broad rule's comment no longer over-claims, and the new rule's comment accurately states the grouping rationale.
renovate-config-validator(renovate@43.242.0) passes: "Config validated successfully against 1 file(s)".
Blocking issues
None
Non-blocking concerns
None
Missing tests
None expected for a Renovate config. renovate-config-validator is the only realistic gate and passes; the structural reasoning above (cumulative rule application + manager separation) covers the behavioral correctness the validator cannot check.
Risk assessment (LOW)
- Likelihood of regression: LOW. Broad artifact-skip is preserved, so the original
renovate/artifactsfailure stays suppressed; the added rule only narrows grouping for a single dependency. - Security exposure: LOW. Config-only;
postUpgradeTasksunchanged. - Blast radius: LOW. Scoped to Renovate update branches; no runtime/action code paths touched.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 28290616856 |
| Cache | hit |
| Session | ses_0f6bf678effewfnyxx0N87xqcT |
Summary
@types/nodeRenovate updates out of mixed Node update groupsskipArtifactsUpdatescoped to the npm@types/nodeupdate so Renovate does not run the failing built-in Bun lockfile artifact pathpostUpgradeTasksto regeneratebun.lockVerification
bunx --package renovate@43.242.0 renovate-config-validator --no-global .github/renovate.json5git diff --check