Gate the load-bearing dependencies behind a human merge - #2178
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Renovate configuration in renovate.json by increasing the concurrent PR limit and restructuring dependency grouping and automerge rules. Specifically, it excludes compiled addons, wire encoders, and the SQL engine from the automerged minor/patch group to ensure they are merged manually. The review feedback correctly points out that Renovate normalizes official Docker Hub images to include the library/ namespace, meaning docker.io/node is extracted as docker.io/library/node. To prevent this from bypassing the exclusions and being automerged, it is recommended to explicitly add docker.io/library/node (and its negated form) to the matchPackageNames lists.
| "!node", | ||
| "!docker.io/node", |
There was a problem hiding this comment.
Renovate's Docker manager normalizes official Docker Hub images to include the library/ namespace. When parsing a Dockerfile with FROM docker.io/node:..., Renovate extracts the dependency name as docker.io/library/node rather than docker.io/node or node.
Because docker.io/library/node is not present in either list, it will bypass the exclusions and be grouped under 'all non-major dependencies' and automerged.
To prevent this, we should explicitly add docker.io/library/node (and its negated form) to both matchPackageNames lists (lines 41 and 73).
| "!node", | |
| "!docker.io/node", | |
| "!node", | |
| "!docker.io/node", | |
| "!docker.io/library/node", |
|
One blocker remains: the |
Majors, 0.x minors (which Renovate still classifies as minor, and which convey breaking changes by convention), the compiled addons, the at-rest/wire encoders and the SQL engine no longer automerge. Each is a case where a green suite is not evidence: msgpackr v2 downgrade compatibility, the Node 26 Windows LTO leak, and 5.1.22 shipping rocksdb-js 2.4.0 when the cross-column-family read fix was in 2.5.0. They are also excluded from the non-major group, so a bump that must be read does not arrive inside a group PR of fifteen others. That carve-out is what raises prConcurrentLimit to 20: these PRs wait on a human, and at a limit of 2 a pair of them would stop the group PR — security patches included — from being opened at all. rocksdb-js is disabled outright. update-rocksdb-js.yml already opens a PR per upstream release and its cleanup only closes PRs on its own head branch, so Renovate would have produced a competing PR for the same bump. Exclusions use matchPackageNames negation rather than the deprecated excludePackageNames, and the node matchers name docker.io/node: a bare `node` is an exact match and never matched the Dockerfile base image. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
474b1cb to
361c578
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "!ordered-binary", | ||
| "!structon", | ||
| "!node", | ||
| "!docker.io/node", |
There was a problem hiding this comment.
What: Renovate's Docker datasource normalizes official Docker Hub images to include the library/ namespace. FROM docker.io/node:... in the Dockerfiles is very likely tracked internally as packageName docker.io/library/node, not docker.io/node. Neither this exclusion list nor the human-merge block list (line 75) contains docker.io/library/node.
Why it matters: This is the same bug class this PR just fixed for the bare node matcher (an exact match that never matched docker.io/node:...). If the normalized name really is docker.io/library/node, the Node base image bump — the exact dependency the PR body calls out by name ("Node 26 Windows LTO leak") as the reason to require human merge — would still silently ride in the auto-merged group. gemini-code-assist flagged this as a high-priority inline finding on this PR (comment id 3787836354) with the same rationale and a concrete suggested diff; it has no reply or resolution yet.
Suggested fix: Add docker.io/library/node (and !docker.io/library/node in the exclusion list) alongside the existing docker.io/node entries in both lists — or confirm via a Renovate debug/dry-run log what packageName is actually resolved for this Dockerfile reference before merging.
| "!docker.io/node", | |
| "!node", | |
| "!docker.io/node", | |
| "!docker.io/library/node", |
Replace timing-dependent assertions that failed in the Node 24 and 26 CI matrix with condition waits for the observed committed state. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Allow the cache propagation poll to continue through transient non-success responses on loaded CI runners. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Review follow-ups on the transaction commit-callback work, all raised as Low: - Widen lastConfirmableEntry()'s contract comment: the walk skips two kinds of write, not one — explicit opt-outs and writes with no stored entry (a delete) — and pin the second with a put-then-delete case on an `audit: false` table. - Drain staged completions in abort(). A completion staged but never aggregated by commit() previously failed silently (stageCompletion()'s no-op handler had marked it handled), and survived into a reused transaction's next commit(). - Release a still-live native handle in abort(). A write-only transaction takes no read reference, so the read-txn drain released nothing even though save() had created a handle. - Retime the LMDB keyed-write ordering test on the conditional batch instead of a 50 ms timer, so a loaded runner cannot turn it into a silent false pass. - Run the commit-callback rejection test on LMDB too, covering the no-op rejection handler in LMDBTransaction.doWrite. Refs #2178 Co-Authored-By: Claude Opus <noreply@anthropic.com>
A delete on an audited or delete-tracking table stores a null-value tombstone (Table.ts: `if (audit || trackDeletes) updateRecord(id, null, …)`), so getEntry() stays truthy and the confirmation walk stops there. Only a delete on a table with neither is entry-less — which is the shape the reloadMarker test builds. Comment-only; no behavior change. Refs #2178 Co-Authored-By: Claude Opus <noreply@anthropic.com>
Review follow-ups on the transaction commit-callback work, all raised as Low: - Widen lastConfirmableEntry()'s contract comment: the walk skips two kinds of write, not one — explicit opt-outs and writes with no stored entry (a delete) — and pin the second with a put-then-delete case on an `audit: false` table. - Drain staged completions in abort(). A completion staged but never aggregated by commit() previously failed silently (stageCompletion()'s no-op handler had marked it handled), and survived into a reused transaction's next commit(). - Release a still-live native handle in abort(). A write-only transaction takes no read reference, so the read-txn drain released nothing even though save() had created a handle. - Retime the LMDB keyed-write ordering test on the conditional batch instead of a 50 ms timer, so a loaded runner cannot turn it into a silent false pass. - Run the commit-callback rejection test on LMDB too, covering the no-op rejection handler in LMDBTransaction.doWrite. Refs #2178 Co-Authored-By: Claude Opus <noreply@anthropic.com>
A delete on an audited or delete-tracking table stores a null-value tombstone (Table.ts: `if (audit || trackDeletes) updateRecord(id, null, …)`), so getEntry() stays truthy and the confirmation walk stops there. Only a delete on a table with neither is entry-less — which is the shape the reloadMarker test builds. Comment-only; no behavior change. Refs #2178 Co-Authored-By: Claude Opus <noreply@anthropic.com>
2 participants
Majors,
0.xminors, the compiled addons, the at-rest/wire encoders and the SQL engine no longer automerge. Each is a case where a green suite is not evidence about the change: msgpackr v2 downgrade compatibility, the Node 26 Windows LTO leak, and 5.1.22 shipping rocksdb-js 2.4.0 while the cross-column-family read fix sat in 2.5.0.They are also pulled out of the non-major group, so a bump that has to be read arrives in its own PR rather than inside a group of fifteen. rocksdb-js is disabled outright —
.github/workflows/update-rocksdb-js.ymlalready opens a PR per upstream release, and Renovate would have produced a competing one for the same bump.@harperfast/extended-iterableis on the list for a sharper reason than the rest: rocksdb-js requires it at exactly1.0.3, so letting the root float admits a second copy of the module — and therefore a secondSKIPsentinel, which surfaces as a phantom record from a vector query whose candidate was deleted. The companion pin PR narrows the range; this stops automerge widening it again.This is the config half of a larger change.
allow_auto_mergeis now on for this repo and theMainruleset requires ten real checks (unit + the six integration shards + validate/runLinter/format) instead ofvalidate / validatealone, so "green" now means the suites actually ran.For the human reviewer
0.xblanket block. Renovate classifies0.45.1 → 0.46.0asminor, so the majors rule misses it, but by convention a0.xminor is breaking. This blocks automerge for every0.xdependency rather than only the load-bearing ones — the alternative is a shorter list and accepting that e.g.passport 0.7 → 0.8(auth session handling) automerges. Chosen conservatively; one line to narrow.prConcurrentLimit2 → 20. The carve-out creates PRs no bot will ever close, and at a limit of 2 a pair of them stops the group PR — security patches included — from being opened at all. 20 is headroom, not a measurement. NotebranchConcurrentLimitdefaults to it, so a busy Monday can open more parallel CI fan-outs than before.groupNameoverride, but I could not prove the semantics without a Renovate dry run and did not want to guess in the direction of "all automerge silently stops".Verification
renovate-config-validatorpasses with zero migration warnings — the previous file needed one (excludePackageNamesis deprecated and removed in Renovate v40; the negatedmatchPackageNameshere is that migration). Not observable end-to-end short of a Renovate dry run: nothing in CI exercises grouping or automerge resolution, so the rule interactions rest on documented precedence, which the review checked independently.The
docker.io/nodematcher is a fix found in review, not cosmetic: every Dockerfile saysFROM docker.io/node:..., and a barenodeinmatchPackageNamesis an exact match that never matched it.Review coverage
Authored by Claude (Opus 5). Outside lenses: codex (graded leg), gemini via agy, and the harper-domain adjudicator.
cursor-composerfailed its artifact contract on this round and did not contribute;cursor-grokwas pruned as the round's second Cursor lens. Four rounds; no correctness findings survived adjudication. The review found the two defects since fixed here — the inertnodematcher and the0.xgap — plus the competing rocksdb-js PR stream.Human-Review-Need: 3 (decisions: carve-out-scope, concurrency-ceiling, zero-x-blanket, rocksdb-js-disabled, pin-vs-automerge) @ 361c578