Repository navigation
ci: let Renovate's action bumps pass (match node/bun by dep name; tests read workflow versions) - #696
Conversation
…rsions in tests The github-actions uses-with rule matched package names, but setup-node's node dependency is named node and packaged as actions/node-versions, so the rule never fired. Match matchDepNames instead; the npm/bun rule keeps matchPackageNames because there the names are equal. Reviewer tests now derive skipped-action tags and runtime pins from the workflow, and the trusted runtime matrix gains Node 24.22.0 for the pin cli#613 carries. The allowlist guard remains: a workflow SHA that is not reviewed still fails.
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Renovate dependency matching .github/renovate.json, .changelog/unreleased/fixed-renovate-action-bumps.md |
The uses-with rule matches bun and node by dependency name. The changelog describes the rule and related reviewer checks. |
Workflow-derived reviewer expectations packages/cli/test/reviewer/ci-job.test.ts, packages/cli/test/reviewer/resolve-runtime.test.ts |
Reviewer tests parse workflow steps, derive reviewed action tags and runtime pins, and compare planned jobs and resolved runtime images against workflow values. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Possibly related PRs
- tpsdev-ai/cli#695: Adds an
actions/cacheversion toSKIPPED_ACTIONS, which the reviewer tests now use to validate workflow action references.
Suggested reviewers: tps-sherlock, tps-kern
Merge Risk: 
Minimal · up to 25a4a
-
No merge-blocking issue is established. Confirm the hosted Renovate result after merge as planned.
Pre-merge checks: 
5
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (2 skipped: 2 unsupported.)
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
With the uses-with rule matching dep names, Renovate no longer proposes the Node bump, so the Node 24.22.0 entry and its image are removed and the resolve-runtime and reviewer-launch cases go back to main's expectations.
|
Claim sweep — head b9348f3 (not a review request)
|
|
Claim sweep — head 25a4ab5 (not a review request) Scope: every sentence this round adds or changes (rule
Not changed, and why: the earlier lane/build/lint counts are marked as measured on b9348f3 with the later commit named and its non-re-run status stated. |
|
@coderabbitai review |
✅ Action performed
|
tps-sherlock
left a comment
There was a problem hiding this comment.
APPROVE — reviewed at head 25a4ab5. Repo visibility checked: tpsdev-ai/cli is PUBLIC.
Scope: 4 files (.github/renovate.json, packages/cli/test/reviewer/ci-job.test.ts, packages/cli/test/reviewer/resolve-runtime.test.ts, one changelog fragment). git diff origin/main -- docker/reviewer/runtime-matrix.json .github/workflows/ is empty, so the trusted table and the workflows are untouched, as described.
What I ran (isolated HOME/TPS_TEST_ROOT via scripts/test-suite.mjs cli)
- Head, unmodified:
ci-job.test.ts+resolve-runtime.test.ts→ 122 pass / 0 fail. - Mutation 1 —
node-version: '24.21.0'→'24.22.0'intest.yml: the exact-image test fails onexpect(expected.length).toBe(1)while the ci-job tests still pass. That is the intended split: pins may track the workflow, the matrix still decides. - Mutation 2 — an unreviewed 40-hex checkout SHA in
test.yml: 5 failures, includingevery ref this repository's workflows use for a skipped action is in the reviewed allowlist. - Mutation 3 — node 24.22.0 in the build job only (a non-test member of the needs closure): same resolve-runtime failure, so the derived expectation covers the whole planned closure, not just the test job.
Why deriving from the workflow does not let a bad ref or an untrusted runtime through
-
ci-job.test.ts:48-53— derivation is also the validator:const [name, sha] = uses.split("@"); ... const tag = action.refs[sha]; if (!tag) throw new Error(\${name}@${sha} is not in the reviewed allowlist`);`. A non-allowlisted SHA fails the test; it is not blessed by being read out of the workflow. -
ci-job.test.ts:212— the repo-wide allowlist guard is unchanged and still covers every workflow file. -
ci-job.test.ts:134still pins the planned job set ([["build", []], ["test", ["build"]]]), so thep.jobs.map((j) => j.id)argument at:193cannot silently shrink what the pins are compared against. -
resolve-runtime.test.ts:17reads the realdocker/reviewer/runtime-matrix.json, and:92-94require exactly one matching image andexpect(r.ok).toBe(true): an untrusted pin yields no image and a red test, not a pass.
Two non-blocking notes
[packages/cli/test/reviewer/ci-job.test.ts:191]— the tag half of this assertion is near-tautological: both sides read the sameSKIPPED_ACTIONStable, soworkflowSkippedcannot independently confirm the planner's tag. The load-bearing parts are the allowlist-membership throw at:51-52and the guard at:212. Fine as written; just do not read the tag equality as evidence about naming.[packages/cli/test/reviewer/ci-job.test.ts:43-55]—workflowSkippedcovers only the test job's steps. Other jobs rely on the regex at:212, which matches only an unquoteduses: name@<40hex>; a quoteduses:in another job would evade it. Pre-existing, not introduced here, andplanJobstill refuses such a ref at plan time — a follow-up, not this PR.
I did not independently re-derive Renovate's depName/packageName split for actions/setup-node (that is the other reviewer's focus); the rule is fail-safe in the direction that matters — if matchDepNames still does not fire, behaviour is today's. I did not assess any CI lane.
Build note: bun run build at this head fails on TS2307 Cannot find module '@tpsdev-ai/agent' and TS7006 in src/utils/*; the same TS2307 class fails identically on origin/main 80c5fb7, so it is a workspace build-order issue, not this diff.
tps-kern
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — reviewed at head 25a4ab5e (worktree HEAD verified == headRefOid). Repo visibility checked before writing this: repos/tpsdev-ai/cli → visibility: public, so this review is posted in the open.
The Renovate matcher (.github/renovate.json:38-50) — correct matcher, correctly scoped
- The rule is exactly
matchManagers: ["github-actions"]+matchDepTypes: ["uses-with"]+matchDepNames: ["bun","node"],enabled: false. Renovate's manager docs defineuses-withas "a language/runtime version passed as an input to a versioned action (e.g. node-version for actions/setup-node)" — so onlywith:runtime pins are suppressed; action SHA/tag pins (depTypeaction) still flow to the group PR. That is what unblocks cli#613, not a blanket freeze. - The depName/packageName split is confirmed by live Renovate output on this repo, not just docs: cli#613's update table lists the dependency as
[node]linked toactions/node-versions, typeuses-with— depNamenode, packageNameactions/node-versions.matchPackageNames: ["node"]could never match it;matchDepNamesdoes. The npm/bun sibling rule, where depName == packageName, is correctly left onmatchPackageNames. - Bound I could not close: I could not read Renovate's per-action extraction mapping for
oven-sh/setup-bun'sbun-version(upstream source fetch truncated before that section; no bunuses-withupdate has ever been proposed here, so there is no live evidence either way). If a future bun uses-with dep were extracted under a depName this rule misses, the trusted matrix still fails closed: an out-of-matrix pin refuses with a named missing image (pinned synthetically atresolve-runtime.test.ts:98-107and in the build-args suite), and the derived A2 test reds on any pin the table cannot satisfy (verified by mutation below).
Derived expectations (ci-job.test.ts, resolve-runtime.test.ts) — the guards survive derivation
- The helpers fail closed on the workflow itself:
workflowSkipped(ci-job.test.ts:40-55) throws if auses:SHA is not in the reviewed allowlist, or the action is not a skipped one at all;workflowPins(:58-76) throws on a setup action with no version input; the A2 test (resolve-runtime.test.ts:84-99) requires the workflow pins to select exactly one image from the trusted table before asserting the resolver's choice. - Allowlist tag correctness is not tautological: entry format is pinned at ci-job.test.ts:222-231, checkout's v4.4.0 forward entry semantically at :234-249 (SHA → tag at the plan level), and the synthetic refusal tests (:240-253, unchanged) pin tag/branch/short-SHA/unreviewed-SHA refusals from fixtures decoupled from live pins.
- The trust boundary is unchanged:
SKIPPED_ACTIONSdecides which SHAs may be skipped (refusal at scripts/reviewer/ci-job.mjs:402-405), the runtime matrix decides which runtimes may run (out-of-matrix refusal naming the missing image). Deriving the pin values from the workflow removes the churn coupling, not a guard.docker/reviewer/runtime-matrix.jsonis untouched by this diff (verified:git diff origin/main..HEADis exactly the 4 files; node images remain 22.22.1 / 24.21.0 only).
Verification runs (repo launcher, HOME-isolated, TMPDIR=/tmp; no build needed — these tests import scripts/reviewer/*.mjs directly):
test/reviewer/on head: 219 pass / 0 fail (5 files). Root suiterenovate-tpsdev-deps+changelog-fragments: 81 pass / 0 fail.lint:ci: clean.- Mutation A — non-allowlisted 40-hex checkout SHA in test.yml (13 sites): 6 unique reds, including the allowlist guard ("every ref this repository's workflows use for a skipped action is in the reviewed allowlist") and the plan tests. A non-allowlisted SHA cannot pass.
- Mutation B —
node-version: '24.21.0'→'24.22.0'in test.yml (exactly what the broken rule let through): exactly 1 red, the A2 test — 0 images match the pin. The pin-fidelity tests pass because the planner does mirror the workflow; the matrix layer refuses the untrusted runtime. That separation is the design working. - Forward-compat — cli#613's five workflow files at its head (b39d6e6) with both node bumps reverted (what the fixed rule will produce): 219 pass / 0 fail, zero test edits. The bumped SHAs land on the allowlisted forward entries (checkout v4.4.0
11d5960a…, setup-bun v2.2.00c5077e5…). The PR's stated goal, reproduced. - cli#613's test.yml delta audited: zero changed lines outside
uses:/node-version— nothing the plan-acceptance tests pin is touched. The three non-reviewer tests that read workflows (workflow-permissions,real-flair-lane,real-flair) couple only to structure/paths, not pins — no hidden hardcoded-version tripwire outsidetest/reviewer/.
Non-blocking notes
ci-job.test.ts:46-48—workflowSkippedthrows on auses:step whose action is not inSKIPPED_ACTIONSat all (e.g. a future localuses: ./…step in the test job). Loud is right; noting it so the throw is recognized as a test-side guard, not a planner defect.- The floating
node-version: '24'(release.yml:224) is now manual forever, like every other runtime pin — consistent with "pins are manual by design"; nobody should wait for Renovate to advance a deliberate floating major. - If Renovate ever renames the
uses-withdepType, this rule silently stops matching; the first symptom would be a node row in a future group PR. The matrix backstop still fails closed, so this is watch-not-fix.
Changelog fragment matches the fragments convention and its content is accurate.
What changed
Renovate's setup-action runtime rule matched the wrong name.
.github/renovate.jsonhad a github-actionsuses-withrule withmatchPackageNames: ["bun", "node"]. Renovate matchesmatchPackageNamesagainstpackageName; foractions/setup-nodethe dependency name isnodebut its package isactions/node-versions, so the rule never fired and the group PR carriednode | uses-with | minor | 24.21.0 -> 24.22.0. The rule now usesmatchDepNamesfor that manager and dep type (Renovate's docs:matchDepNamesbehaves asmatchPackageNamesbut matchesdepName). The npm/bun rule beside it keepsmatchPackageNames: for that manager the package and dependency names are equal, so it matches.Reviewer tests restated versions written in the workflow. With the group PR's workflow changes:
ci-job.test.ts: skipped-action tags and runtime pins are read from the workflow and the reviewed allowlist instead of listingv4.2.2/v2.0.2/24.21.0. It still requires every skipped step to be an allowlisted SHA (an unallowlisted SHA fails), and every pin to equal the workflow'swith:value.ci-job.test.ts: every plannedrun:step of the test job must be on the Node version the test job'ssetup-nodepins.resolve-runtime.test.ts: the expected image is derived from the workflow's node/bun pins and the matrix instead of naming the 24.21.0 image; it still requires exactly one matching image.The trusted runtime matrix is unchanged: with the rule fixed, Renovate no longer proposes the Node bump, so no new runtime image is added.
Verification of the Renovate rule
The rule is evaluated by the hosted Renovate app, an external service, so there is no in-repo test that runs Renovate's own extractor. Adding Renovate as a cli dependency to test one matcher would put a very large dependency tree under the audit gate, and a local copy may not match the hosted version. The check is observable instead: after this PR merges, Renovate regenerates cli#613, which must contain no
node-versionchange; if it does, this rule change is reverted.The dependency must be matched by
depNamebecause Renovate's docs state thatmatchDepNamesbehaves asmatchPackageNamesexcept that it matchesdepNameinstead ofpackageName. cli#613's own table listsnodeas auses-withdependency linked toactions/node-versions.local proof
Checked out cli#613's five workflow files into the branch worktree without committing them, then reverted every
node-versionchange to main's values (keeping only the action SHA bumps) — the state the fixed rule produces (measured onb9348f3b, main80c5fb7f):ci-job.test.tsandresolve-runtime.test.ts, the two changed test files, plus the unchangedreviewer-launch.test.ts: 185 pass, 0 fail with those workflows in the tree.25a4ab5e:actions/checkout@aaaaaaaa...intest.yml: 5 of 57 fail inci-job.test.ts, including the allowlist guard (every ref this repository's workflows use for a skipped action is in the reviewed allowlist); restored: 185 pass, 0 fail.Verification (measured on b9348f3, main 80c5fb7)
bun run test(cli lane): 3848 pass, 25 skip, 0 fail, 3873 tests. The launcher exited 1 on its caller-HOME diagnostic because concurrent processes wrote the caller's data directory during the run; the runs above use the CI step's HOME isolation and exited 0.bun run build(cli typecheck): exit 0.bun run lint:ci: exit 0.node scripts/changelog-fragments.mjs check: exit 0.git diff --check: exit 0.25a4ab5echanges only adescriptionstring in.github/renovate.json; the lane, build and lint results above were measured onb9348f3band were not re-run, while the three reviewer test files were re-run on25a4ab5e(185 pass, 0 fail).No issue: unblocks the GitHub-Actions group PR at the source. A bump whose action SHAs are already in the reviewer allowlist, as cli#613's are, no longer needs a human commit; a bump to a new SHA still needs its allowlist entry first, as cli#695 did for cli#670.
Summary by CodeRabbit