Scope Dependabot to our beyond-template packages - #224
Conversation
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughAdded dependency-scope validation against a recorded merged template commit. The lint workflow now fetches full history, Dependabot manages monthly updates, and the README documents baseline and dependency-scope maintenance. ChangesDependency scope validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟡 Moderate · up to Adding js-yaml without updating package-lock.json will cause the lint workflow's dependency installation to fail, so the change is not ready to merge until the lockfile is regenerated and committed. Sequence Diagram(s)sequenceDiagram
participant Lint as npm lint
participant Scope as check-dependency-scope.cjs
participant Git as Git history
participant Config as Dependabot config
Lint->>Scope: Run dependency-scope validation
Scope->>Git: Read merged template commit manifest
Scope->>Config: Read npm allow and ignore lists
Scope->>Scope: Compare manifests and validate scope
Scope-->>Lint: Report violations and exit status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Why this config has an
|
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed all commit messages and made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc).
.github/dependabot.yml line 15 at r1 (raw file):
directory: / schedule: interval: weekly
Weekly can be pretty annoying. I'd opt for monthly.
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
❓ Might we want a script that compares our package-lock.json to the template's, to prevent drift?
@imnasnainaec made 3 comments.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on alex-rawlings-yyc).
.github/dependabot.yml line 12 at r1 (raw file):
version: 2 updates: - package-ecosystem: npm
⛏️ from Devin that I agree with:
Only npm is covered; GitHub Actions workflows get no updates
The repo has six workflows under .github/workflows/ plus composite actions in .github/actions/, and no github-actions ecosystem entry is configured. Action pins therefore receive neither version nor security updates. This is consistent with the PR's npm-only scoping, but the template rationale (lockfile conflicts on template merges) does not apply to action pins, so a separate github-actions entry may be worth adding later.
.github/dependabot.yml line 19 at r1 (raw file):
# checkout, which exists on developer machines and in CI but not inside # Dependabot's container. Without these entries Dependabot aborts the whole # job during file fetching and never reaches the allow list below.
❓ from Devin:
Ignore entries may not actually prevent the abort on file: dependencies
The comment claims Dependabot "aborts the whole job during file fetching" for the three file:../paranext-core/... dependencies and that these ignore entries prevent it. ignore rules are applied when Dependabot filters the parsed dependency list, which happens after the file fetcher/parser stage; if the failure genuinely occurs during file fetching or lockfile parsing, ignore would not suppress it. Since these three packages are also absent from allow, the entries are otherwise redundant. Worth confirming against an actual Dependabot run log rather than relying on the stated rationale.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
Might the comparing the lockfiles be too noisy? Though, if we're only updating it on Template merge, it may not actually be
@alex-rawlings-yyc made 4 comments.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on alex-rawlings-yyc and imnasnainaec).
.github/dependabot.yml line 12 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ from Devin that I agree with:
Only npm is covered; GitHub Actions workflows get no updates
The repo has six workflows under
.github/workflows/plus composite actions in.github/actions/, and nogithub-actionsecosystem entry is configured. Action pins therefore receive neither version nor security updates. This is consistent with the PR's npm-only scoping, but the template rationale (lockfile conflicts on template merges) does not apply to action pins, so a separategithub-actionsentry may be worth adding later.
Do we want to add an issue to address this later or do it right now?
.github/dependabot.yml line 15 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Weekly can be pretty annoying. I'd opt for monthly.
Agreed
.github/dependabot.yml line 19 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
❓ from Devin:
Ignore entries may not actually prevent the abort on
file:dependencies
The comment claims Dependabot "aborts the whole job during file fetching" for the threefile:../paranext-core/...dependencies and that theseignoreentries prevent it.ignorerules are applied when Dependabot filters the parsed dependency list, which happens after the file fetcher/parser stage; if the failure genuinely occurs during file fetching or lockfile parsing,ignorewould not suppress it. Since these three packages are also absent fromallow, the entries are otherwise redundant. Worth confirming against an actual Dependabot run log rather than relying on the stated rationale.
I'm looking into this now; I'll come back with some sort of answer
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
Claude's thoughts:
Worth doing, and I went and measured it — the drift is real, but it's the manifest that's checkable, not the lockfile.
package.json vs template/main:
shared deps: 57 | version-range mismatches: 2
@tailwindcss/postcss: tmpl ^4.0.0 vs ours ^4.3.0
tailwindcss: tmpl ^4.0.0 vs ours ^4.3.0
ours-only: 14 ← exactly the allow list in this PR, no more, no less
template-only: 0
package-lock.json vs template/main:
ours 1266 packages, template 1012
only in ours: 390 only in template: 136
shared package, different version: 278 (mostly @babel/*, all ours-newer)
~670 differences today, with no Dependabot PR ever merged — a lockfile comparison would need ~670 baselined exceptions before it could report anything new. It isn't the template running ahead of us either; template/main is only 3 commits past our last update-from-template, and none of them touch dependencies.
The cause is known: 5f44a9d (the React 19 / Tailwind 4 baseline) is a commit shared with the template, so at that point our lockfile was the template's and described none of our own dependencies. 791ffd6 the next day regenerated it (+343 packages, −89, 219 version changes) — 219 of the 278 in one commit. The rest came from running core:reinstall before committing on a handful of feature PRs. Our policy is that the lockfile moves only on template merges and, once this lands, Dependabot PRs.
One wrinkle worth naming: there's a third legitimate case. Our lockfile snapshots the full dependency manifests of the three file:-linked core libs, and two of those feature-PR lockfile diffs carried real changes to them (#131 platform-bible-react; #180 platform-bible-react + platform-bible-utils). Since CI installs the extension with npm ci, if core's manifest changes never land we'd be testing against a tree that doesn't match the core we cloned. So core-lib syncs have to be allowed through.
Which suggests two checks, neither of them a lockfile diff:
- Lockfile changed,
package.jsondidn't, and no../paranext-core/lib/*entry changed — that's incidental drift with nothing legitimate in it. Checked against history it catches the five accidental ones and passes #131 and #180. Exempt Dependabot and update-from-template PRs. - Manifest comparison — shared deps must have identical ranges; every ours-only dep must be in
allow; everyallowentry must be in ourpackage.jsonand absent from the template's. That mechanizes the "keep the allow list in sync" instruction in the header comment, which is the part most likely to rot. Fires today on the two Tailwind entries — deliberate, from the React 19 / Tailwind 4 work, but recorded nowhere.
Happy to write both. Probably a separate PR from this one — want me to open an issue?
(Resetting the existing 278 back onto the template's pins is possible but I'd skip it: it downgrades 278 transitives for cosmetic gain, and check 1 stops the bleeding either way.)
@alex-rawlings-yyc made 3 comments.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on alex-rawlings-yyc and imnasnainaec).
.github/dependabot.yml line 12 at r1 (raw file):
Previously, alex-rawlings-yyc (Alex Rawlings) wrote…
Do we want to add an issue to address this later or do it right now?
Claude's opinion:
Agreed this is a gap, though I'd argue the template rationale doesn't apply here at all — I checked, and none of our action pins match the template's. It floats major tags (actions/checkout@v4, github/codeql-action@v3, ncipollo/release-action@v1); we SHA-pinned every one of them back in #82. All four template-owned workflows are already locally modified too (lint.yml +19/-5, publish.yml +68/-10, codeql.yml +5/-3, bump-versions.yml +4/-4), and update-from-template lands as a hand-curated squash — #204 was one file — so there's no mechanical merge for a pin bump to conflict with.
The more pressing part is that SHA-pinning opts us out of updates and we never added the thing that puts them back. The pins are frozen where #82 left them:
actions/checkout pinned v6.0.2 (2026-01-09) -> latest v7.0.1 (2026-07-20) major behind
actions/setup-node pinned v6.4.0 (2026-04-20) -> latest v7.0.0 (2026-07-14) major behind
github/codeql-action pinned (2026-05-22) -> latest bundle (2026-08-12) ~3 months
CodeQL three months stale is the one I'd least like to leave sitting. Since our pins already carry # v6.0.2-style comments, Dependabot maintains the SHA and the comment together — no extra setup:
- package-ecosystem: github-actions
directory: /
schedule:
interval: monthly
groups:
actions:
patterns: ['*']Happy to add that here rather than defer it — it's six lines and the npm half of this config is already doing the harder job. Shout if you'd rather it went in its own PR.
.github/dependabot.yml line 19 at r1 (raw file):
Previously, alex-rawlings-yyc (Alex Rawlings) wrote…
I'm looking into this now; I'll come back with some sort of answer
Good check to insist on — I'd only verified that the config works, not that ignore was the reason it works. Turns out it is, and the finding's premise doesn't hold for npm.
The claim is that ignore is applied when filtering the parsed dependency list, downstream of the file fetcher. npm's fetcher actually consults the ignore list itself, in two places in npm_and_yarn/lib/dependabot/npm_and_yarn/file_fetcher.rb. In path_dependencies, before it ever tries to fetch:
if dependency_ignored?(name)
Dependabot.logger.info(
"Ignored local path dependency '#{cleaned_name}' for package '#{name}' as it matches the ignore list."
)
next
end
begin
file = fetch_file_from_host(filename, fetch_submodules: true)and again in build_unfetchable_deps, which is the path that actually bites us:
filtered_deps = unfetchable_deps.reject do |name, _path|
if dependency_ignored?(name)
Dependabot.logger.info("Ignored unfetchable path dependency '#{name}' as it matches the ignore list.")
trueOur three don't trip either hard raise PathDependenciesNotReachable guard — those fire only on /-absolute paths or traversal past the repo root, and ../paranext-core/... is neither. So without ignore they fall through to fetch_file_from_host → DependencyFileNotFound → unfetchable_deps → PathDependencyBuilder, which is where the abort originates.
That also answers the "otherwise redundant" half: the fetcher calls dependency_ignored? and never looks at allow, so leaving the three out of allow cannot substitute for listing them in ignore. The two keys aren't interchangeable here.
The comment was misleading about the mechanism ("never reaches the allow list" implied allow was the relevant knob), so I've rewritten it to name ignore specifically and say why allow can't cover it.
Worth noting the related bug reports where ignore genuinely doesn't suppress this are other ecosystems — dependabot/dependabot-core#14425 is Bundler, and it's closed with a fix.
020c0e5 to
e7a4469
Compare
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
Oops, I meant just comparing package.json with the template's. We don't worry about lockfile drift; rather we update package-lock.json after syncing from the template (and we could do the same atop a dependabot update for the non-template packages).
@imnasnainaec reviewed all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc).
.github/dependabot.yml line 12 at r1 (raw file):
Previously, alex-rawlings-yyc (Alex Rawlings) wrote…
Claude's opinion:
Agreed this is a gap, though I'd argue the template rationale doesn't apply here at all — I checked, and none of our action pins match the template's. It floats major tags (
actions/checkout@v4,github/codeql-action@v3,ncipollo/release-action@v1); we SHA-pinned every one of them back in #82. All four template-owned workflows are already locally modified too (lint.yml+19/-5,publish.yml+68/-10,codeql.yml+5/-3,bump-versions.yml+4/-4), and update-from-template lands as a hand-curated squash — #204 was one file — so there's no mechanical merge for a pin bump to conflict with.The more pressing part is that SHA-pinning opts us out of updates and we never added the thing that puts them back. The pins are frozen where #82 left them:
actions/checkout pinned v6.0.2 (2026-01-09) -> latest v7.0.1 (2026-07-20) major behind actions/setup-node pinned v6.4.0 (2026-04-20) -> latest v7.0.0 (2026-07-14) major behind github/codeql-action pinned (2026-05-22) -> latest bundle (2026-08-12) ~3 monthsCodeQL three months stale is the one I'd least like to leave sitting. Since our pins already carry
# v6.0.2-style comments, Dependabot maintains the SHA and the comment together — no extra setup:- package-ecosystem: github-actions directory: / schedule: interval: monthly groups: actions: patterns: ['*']Happy to add that here rather than defer it — it's six lines and the npm half of this config is already doing the harder job. Shout if you'd rather it went in its own PR.
I'm inclined toward here over its own pr.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
Added a script that does the comparison and added it to npm run lint
@alex-rawlings-yyc made 2 comments.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
.github/dependabot.yml line 12 at r1 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
I'm inclined toward here over its own pr.
Added
Template-owned dependencies move only when we merge template changes, so the allow list covers just the packages this extension adds. Security updates stay inert until the repo's dependency graph is enabled.
npm's file fetcher consults `ignore` and never `allow`, so the comment claiming the job "never reaches the allow list" named the wrong key.
SHA-pinned actions receive no updates without an ecosystem entry, and nothing enforced the allow list's "absent from the template" rule.
Comparing against the template's moving head turned lint red on every PR whenever the template bumped a shared range; reading the baseline out of this repo's own history also lets CI drop the template checkout. Failures name a stale baseline as a possible cause, since it inverts their advice.
d991f0b to
a0bd260
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A squashed template update leaves the pinned commit unreachable, which would fail every build; the copy also drops lint's git dependency and CI-only skip. Add npm run template:baseline and document when the refresh is needed.
merged-template-package.json from the template commit that merge brought in, rather than acting on the lines above. Run npm run template:baseline while template/main still points at that commit.
Refreshing it runs a script rather than a shell redirect, which truncated the file before git could fail. The check also compares peerDependencies and names the file it could not parse.
2 participants
Template-owned dependencies move only when we merge template changes, so the allow list covers just the packages this extension adds. Security updates stay inert until the repo's dependency graph is enabled.
This change is
Summary by CodeRabbit