Scope Dependabot to our beyond-template packages - #224
Scope Dependabot to our beyond-template packages#224alex-rawlings-yyc wants to merge 19 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ 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.
npm run template:baseline now rewrites MERGED_TEMPLATE_COMMIT itself, so skipping a manual step can no longer leave the check naming a template state it did not compare against. Every failure path reports alike and writes nothing.
An unreadable baseline, manifest, or Dependabot config reached Node's default handler, so the conflict markers a template merge leaves in the baseline arrived with no route back. A config carrying no updates key hits the missing-ecosystem guard instead of throwing a TypeError.
Reads-before-writes leaves the pair in step only when a read fails; a throw between the two writes still desynchronizes them.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A pattern outliving the package it names went unreported. Allow-list wildcards are now rejected rather than misread as literal names, and a baseline refresh that fails mid-way says which write landed.
Renames the group to match its contents, and syncs the template manifest copy into the other two ignore files.
The check verified that each group pattern matched something on the allow list, but not the reverse, so an addition could drift out of the group as js-yaml had.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 6 comments.
Reviewable status: 6 of 13 files reviewed, 6 unresolved discussions (waiting on imnasnainaec).
.prettierignore line 3 at r6 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
These 3
*ignorefiles are now out of sync.
Fixed — the entry and its comment are in all three, and they diff clean below #endregion.
One thing to flag: I reworded the comment from "whatever reformatting our config would impose" to "whatever our tooling would impose on it". The original is Prettier-specific and would be untrue in the two files that don't rewrite anything, and byte-identity across the three seemed the stronger convention. Happy to put the sharper wording back and accept the divergence if you'd rather.
Also worth knowing the entry is defensive rather than load-bearing — running Prettier over a copy of merged-template-package.json returns it byte-identical, so the template's manifest is already Prettier-clean. It guards a future one that isn't.
.github/dependabot.yml line 60 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Why leave out
js-yamlhere?
No good reason — it was the only devDependency we add that wasn't in there, so an oversight rather than a decision.
Renamed the group to dev-tooling and added it, so membership is now "everything we add on top of the template that isn't runtime". The only ungrouped allow entries left are the three that ship to users:
allow entries not in any group: @reduxjs/toolkit, fast-xml-parser, react-redux
Your first comment made me look at whether anything would have caught this, and nothing would have — the check verified that each pattern matches something on the allow list, but not the reverse, so the next dev dependency could have drifted out exactly as js-yaml did. That direction is enforced now too:
✗ js-yaml: a dev dependency on the allow list that no group collects, so its updates arrive as a pull request of their own — add it to a group, or record here why it stays out
Runtime dependencies and the file: entries are exempt by construction rather than by exception, so the three above stay quiet. There's deliberately no exception list yet — if a dev dependency ever genuinely wants its own pull request, that's a change to the check, and the message says so rather than pretending an escape hatch exists.
An entry naming no package crashed the check on `undefined`; an ignore entry whose package has left package.json now says so.
A second entry scopes another directory's package.json, so merging its lists would misreport the root manifest. The baseline and allow-list comments now name `npm run template:baseline` and the `file:` carve-out they left out.
Also record why peerDependencies stay in the comparison, and the inert allow entry a peer dependency beyond the template would be asked for.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
`process.exit` does not wait for an asynchronous one, and stderr is asynchronous on a Windows terminal and on a POSIX pipe. Also match a recorded commit id at either object-format width so a refresh can read back what it wrote, fit the scope hint to every way that read fails, and drop a hardcoded count from the config's comment.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 4 comments.
Reviewable status: 9 of 13 files reviewed, 4 unresolved discussions (waiting on imnasnainaec).
.github/dependabot.yml line 18 at r8 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ drop hardcoded "three"
Removed
scripts/check-dependency-scope.cjs line 326 at r8 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ Devin-caught:
scripts/check-dependency-scope.cjs:R326
Failure hint is off-target for the "more than one npm ecosystem entry" case
readOrFail(readNpmScope, UNSCOPED_DEPENDABOT_CONFIG_HINT)attaches a hint about there being no npm entry to declare the lists, butreadNpmScopealso throws when it finds two or more npm entries (scripts/check-dependency-scope.cjs:143-146). In that case the printed hint contradicts the error message it follows. Cosmetic only — the thrown message itself carries the actionable guidance.
Agreed, and it's broader than the two-entry case: readNpmScope has three throw sites — unreadable or non-YAML config, no npm entry, more than one — and readOrFail pairs a single hint with all of them. Reworded it to say where the lists live rather than to prescribe adding an entry, which fits every one:
✗ 2 npm ecosystem entries in .github/dependabot.yml, and this check reads one — name the directory each covers here, or teach this check to pick out the one scoping the package.json beside it
ℹ The allow, ignore, and group lists this check holds package.json to live in that file, declared by the one npm ecosystem entry scoping the directory package.json sits in.
Ran both failure modes against a temporarily-edited config to check the pairing reads straight.
scripts/refresh-template-baseline.cjs line 29 at r8 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ Could use
{40,}or{40,64}to cover the Devin-observed SHA-256 repo edgecase.
Fixed
scripts/report-failure.cjs line 5 at r8 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ Devin suggestion:
scripts/report-failure.cjs:R1-5
Diagnostic output can be truncated by immediate process exit
failwrites to stderr and callsprocess.exit(1)in the same tick; the same pattern appears atscripts/check-dependency-scope.cjs:321-331. When stdout/stderr is a pipe (npm scripts under CI), Node's writes are asynchronous andprocess.exitdoes not wait for them, so a long violation list plus the stale-baseline hint can in principle be cut short. Messages here are far below the pipe buffer so this is unlikely to bite in practice; settingprocess.exitCodeand returning would remove the risk entirely, and the trailingprocess.exit(0)on the success path is unnecessary either way.
Fixed — fail now writes the whole report with one fs.writeSync(2, …) before exiting, and the violation list in check-dependency-scope.cjs goes through the same writer instead of a console.error loop.
I couldn't reproduce the truncation: on the pinned Node (22.22.0), 50,000 console.error lines followed immediately by process.exit(1) came through a pipe complete and in order. But that's Node making POSIX stdio pipes blocking as an implementation detail — the documented contract is the opposite, and a Windows terminal is async outright, so the old code was only correct by accident.
Two notes on the suggestion itself. process.exitCode plus a return isn't a drop-in: all six call sites rely on fail never returning, so readOrFail would hand back undefined for scope, and refresh-template-baseline.cjs would reach commit.slice(0, 7) on undefined — the friendly message swapped for the TypeError this module exists to prevent. And the trailing process.exit(0) isn't in this PR; the only one under scripts/ is in delete-temp-files.cjs, which the branch doesn't touch.
I left the stdout preamble on console.log. fs.writeSync on fd 1 throws EPIPE when a consumer closes early, and trading a stack trace for two informational lines seemed the wrong way round.
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 3 files and all commit messages, made 1 comment, and resolved 4 discussions.
Reviewable status: 12 of 13 files reviewed, 1 unresolved discussion (waiting on alex-rawlings-yyc).
scripts/check-dependency-scope.cjs line 150 at r9 (raw file):
const dependencyNames = (entries) => (entries ?? []).map((entry) => entry['dependency-name']); const groups = Object.entries(npmEntry.groups ?? {}).flatMap(([group, definition]) =>
⛏️ Devin suggestion:
scripts/check-dependency-scope.cjs:R150-152
Group parsing ignores exclude-patterns, so a group can look like it collects a package it does not
readNpmScope reads only definition.patterns (scripts/check-dependency-scope.cjs:150-152). Dependabot groups also accept exclude-patterns, and dependency-type. If someone later adds exclude-patterns to dev-tooling, findUngroupedDevDependencies would still consider an excluded package grouped and stay silent, defeating the very guarantee that function exists for. Nothing today declares an exclusion, so this is a latent blind spot rather than a live miss; a throw on any group key this check cannot read (mirroring the findUnsupportedEntries treatment of allow/ignore) would keep the two in step.
`findUngroupedDevDependencies` reads a group as the packages its patterns collect. Dependabot also narrows a group by `exclude-patterns` and by `dependency-type`, and either one leaves a package the group drops still reading as collected, so the check that exists to catch an ungrouped dev dependency would pass over it in silence. Nothing declares either key today, which is what makes it a blind spot rather than a miss: the guarantee would lapse quietly the first time one arrived. Report any key beyond those the check reads, in the shape `findUnsupportedEntries` already uses for an allow or ignore entry it cannot read. `applies-to` and `update-types` stay unreported: both are declared today and narrow which updates a group collects rather than which packages, and packages are the axis the group checks reason about. An unread key holds back the two checks that open a group and no others, since the rest never read one. Both read the allow list as literal names too, so an unreadable allow entry still holds them as before.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 1 comment.
Reviewable status: 12 of 13 files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
scripts/check-dependency-scope.cjs line 150 at r9 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
⛏️ Devin suggestion:
scripts/check-dependency-scope.cjs:R150-152
Group parsing ignores
exclude-patterns, so a group can look like it collects a package it does not
readNpmScopereads onlydefinition.patterns(scripts/check-dependency-scope.cjs:150-152). Dependabot groups also acceptexclude-patterns, anddependency-type. If someone later addsexclude-patternstodev-tooling,findUngroupedDevDependencieswould still consider an excluded package grouped and stay silent, defeating the very guarantee that function exists for. Nothing today declares an exclusion, so this is a latent blind spot rather than a live miss; a throw on any group key this check cannot read (mirroring thefindUnsupportedEntriestreatment of allow/ignore) would keep the two in step.
Real, and fixed in readNpmScope / findUnsupportedGroupKeys.
I narrowed it slightly from "throw on any group key this check cannot read". applies-to and update-types are declared today and are deliberately unread: they narrow which updates a group collects, not which packages, and the config comment beside them already records that majors arrive ungrouped on purpose. Package membership is the only axis findUngroupedDevDependencies reasons about, so those two are safe to pass over and everything else is not — which catches exclude-patterns and dependency-type alike, and any key Dependabot adds later.
The report follows findUnsupportedEntries rather than throwing: same violation-string shape, and it holds back only the two checks that open a group. The rest never read one, so an unread group key shouldn't suppress an allow-list or template violation that is perfectly readable. In the other direction the dependency does hold: both group checks read the allow list as literal names too, so an unreadable allow entry still holds them as before.
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