feat(kodiak): auto-approve+merge Dependabot patch/minor bumps - #6
feat(kodiak): auto-approve+merge Dependabot patch/minor bumps#6rivernate wants to merge 1 commit into
Conversation
Add .kodiak.toml, the dependabot-automerge-label caller workflow, .github/dependabot.yml (npm), and CODEOWNERS protection for /.github/ and /.kodiak.toml, per derivita/infrastructure#1719.
wbhumphrey
left a comment
There was a problem hiding this comment.
Config is well-built and the composition with the shared reusable workflow checks out — correct fetch-metadata slug, correct trigger types for the callee's label/revoke split, SHA pin matches the current v1 tag, real Kodiak bot login, allowed merge method.
Two blocking findings, both about enabling state rather than the diff: the CODEOWNERS rules are invalid as written and provide no gate, and main has neither branch protection nor rulesets — which makes the accepted-risk reasoning in .kodiak.toml false today. Details inline.
| # workflow file, which can run on its own PR with GITHUB_TOKEN write | ||
| # permissions — requires a human code-owner review to merge. Pairs with | ||
| # branch protection "Require review from Code Owners". | ||
| /.github/ @derivita/engineering |
There was a problem hiding this comment.
GitHub reports both owner lines in this file as invalid. gh api repos/derivita/protobuf-javascript/codeowners/errors?ref=nathan/kodiak-dependabot-automerge returns Unknown owner on lines 5 and 10: @derivita/engineering exists (13 members), but no org team has access granted to this repo, and CODEOWNERS requires the team be publicly visible with write access to resolve.
Effect: these two rules are inert. Even once "Require review from Code Owners" is enabled, /.github/ and /.kodiak.toml carry no owner requirement — so a new workflow file (which can run on its own PR with a write-scoped GITHUB_TOKEN) or an edit that weakens blocking_labels merges with no code-owner review, which is precisely what this file exists to prevent. It fails silently: GitHub surfaces the error in the CODEOWNERS editor but doesn't block anything.
The fix is a repo-settings change rather than a diff change — grant @derivita/engineering write access to this repo, then confirm the codeowners/errors endpoint comes back empty. Flagging it as blocking because the description raises this only as "is this the right team?", where the actual state is "the entry does nothing."
There was a problem hiding this comment.
Confirmed, current state matches: gh api repos/derivita/protobuf-javascript/codeowners/errors?ref=nathan/kodiak-dependabot-automerge still returns Unknown owner on lines 5 and 10, and gh api repos/derivita/protobuf-javascript/teams returns an empty list — no team currently has any access to this repo, so there's no better team to redirect these entries to.
This is a repo-settings/access grant (give @derivita/engineering write access to the repo), not a file change, so I'm leaving it for a human with the appropriate admin access rather than attempting it myself. Not resolving this thread.
| # Triage/Write access could still self-apply it to their own PR for an | ||
| # unreviewed auto-approval. Accepted risk, matching derivita/infrastructure | ||
| # #1716: this repo's collaborators are trusted, and required status checks | ||
| # still gate every PR regardless of label. |
There was a problem hiding this comment.
This mitigation does not hold on this repo today. main has no branch protection and no rulesets — gh api repos/derivita/protobuf-javascript/branches/main/protection returns 404 Branch not protected, and .../rulesets returns []. There are no required status checks: build.yml and codeql.yml do run on Dependabot PRs, but nothing gates the merge on them.
Two consequences of merging as-is:
- The self-applied-label risk documented just above loses its stated backstop. Both
automergeanddependabot-automergealready exist as labels in this repo, so any Triage/Write collaborator can attachdependabot-automergeto their own PR, collect Kodiak's bot approval, addautomerge, and squash-merge tomainwith no human review and no passing CI. The reusable workflow's semver and head-commit-identity checks never enter the picture, because it isn't a Dependabot PR. - The callee's own "repo settings each caller depends on" list names Dismiss stale pull request approvals when new commits are pushed as what makes its
revokejob a backstop rather than the sole line of defence. Without it, Kodiak's approval survives a collaborator's push to a labeled Dependabot branch independently ofrevoke.
The description flags branch protection as unverified — this is the verification, and none of it is in place. Worth landing a ruleset on main (required build + CodeQL checks, code-owner review, dismiss-stale-approvals-on-push) before this merges, then either correcting or dropping the claim on these two lines.
Two things that did check out, for what it's worth: method = "squash" is allowed on this repo and nothing requires linear history, and derivita-kodiak[bot] (the callee's default kodiak-bot-login) is the real Kodiak login.
There was a problem hiding this comment.
Confirmed, current state matches: gh api repos/derivita/protobuf-javascript/branches/main/protection still returns 404 Branch not protected, and gh api repos/derivita/protobuf-javascript/rulesets still returns []. No required status checks, no code-owner review requirement, no dismiss-stale-approvals-on-push.
This needs a ruleset/branch-protection change on main (required build + CodeQL checks, code-owner review, dismiss-stale-approvals-on-push) before the accepted-risk claim on these two lines is accurate. That's a branch-protection change, so I'm not attempting it — leaving this for a human to land the ruleset and then either confirm or amend the comment. Not resolving this thread.
| @@ -0,0 +1,13 @@ | |||
| version: 2 | |||
| updates: | |||
| - package-ecosystem: npm | |||
There was a problem hiding this comment.
Non-blocking, and a scope call rather than a defect. With only npm configured, none of this repo's pinned GitHub Actions ever get update PRs — including the 9752b04 pin this PR introduces, plus actions/checkout, actions/setup-node, the CodeQL actions, and scorecard-action. This repo runs OpenSSF scorecard, which grades pinned-dependency freshness, so stale pins are a visible cost.
A github_actions entry composes safely with this design and needs no CODEOWNERS carve-out: the caller passes ecosystems: npm_and_yarn, so action bumps would get neither label and route to human review by construction. Whether that belongs in this PR or a follow-up is your call.
The two-lockfile question the description raises is already settled, incidentally — #4 and #5 each updated package-lock.json and yarn.lock together, so the single npm entry handles both.
There was a problem hiding this comment.
Agreed this is sound and non-blocking. Since you've framed it as a scope call ("your call" whether it belongs in this PR or a follow-up) rather than a defect in what's here, I'm leaving that decision to a repo owner rather than making it unilaterally. Not resolving this thread so it stays visible for that call.
There was a problem hiding this comment.
Re-reviewed at the same commit — no code change since my last pass.
@rivernate independently re-ran both checks and confirms the state is unchanged: the code-owner entries still fail GitHub's CODEOWNERS validation, and main still has neither branch protection nor rulesets. One new data point from that pass strengthens the first finding rather than softening it — no org team holds any access to this repo at all, so there is no alternative team the entries could be redirected to. Both are access/settings changes outside this diff, and both remain blocking until they land.
The actions-ecosystem note stays non-blocking and is a scope call for a repo owner, as before. No further findings.
mhjenkins
left a comment
There was a problem hiding this comment.
This PR adds GitHub merge-governance config (CODEOWNERS, dependabot.yml, a Dependabot auto-merge labeling workflow, and .kodiak.toml) to auto-approve/merge Dependabot patch/minor npm bumps. Three prior human reviewers already found the safety claims don't hold today: the CODEOWNERS team has no repo access (rules are inert) and main has no branch protection/rulesets at all (the "required status checks" backstop claim is false as written) — both unresolved and blocking. Two independent AI reviewers (architecture + security) additionally converged on a structural gap not previously raised: the actual auto-merge gating logic lives entirely in an external repo (derivita/shared-workflows) that this repo's new CODEOWNERS can never cover, and its safety claims (grouped-dependency handling, head-commit-signature checks against a synchronize-triggered re-label path) are unverifiable from here — worth a maintainer's confirmation before relying on this for unattended merges.
|
Closing without merging. This repo is being excluded from the Kodiak Dependabot auto-merge rollout (derivita/infrastructure#1719) rather than pushed through — CI is broken on a pre-existing build bug unrelated to this diff ( |
Onboards this repo to the Kodiak Dependabot auto-merge rollout (derivita/infrastructure#1719), so Dependabot's own patch/minor bumps auto-approve and auto-merge via Kodiak, while major-version bumps still require full human review. Reference: derivita/infrastructure#1716 (first rollout), derivita/derivita-ui#247 (closer analog — also started from zero prerequisites).
Files added
.github/dependabot.yml— did not previously exist. Added a singlenpmecosystem entry (weekly schedule, 7-day cooldown,minor-and-patchupdate-types group), matching the org convention (see derivita/launchpad's dependabot.yml). This repo's only dependency manifest ispackage.json/package-lock.json/yarn.lockat the root — no go.mod, Dockerfile, requirements.txt, or Chart.yaml. TheMODULE.bazel/WORKSPACEBazel deps are out of scope; Dependabot has no ecosystem for bzlmod/WORKSPACE..kodiak.toml— did not previously exist.[merge]withmethod = "squash"and[approve] auto_approve_labels = ["dependabot-automerge"], matching #1716's reference implementation verbatim..github/workflows/dependabot-automerge-label.yml— thin caller intoderivita/shared-workflows's reusabledependabot-automerge-label.yml@v1(pinned to its current commit,9752b04, rather than the mutable tag — matches derivita-ui#247's pattern, since anyone with write access to shared-workflows could otherwise retargetv1without a PR in this repo).ecosystems: npm_and_yarn(fetch-metadata's slug for npm/yarn — notdependabot.yml'snpmvalue). Gated todependabot[bot]-authored PRs only, trigger scoped tobranches: [main](the repo's default branch)..github/CODEOWNERS— did not previously exist. Added/.github/and/.kodiak.tomlentries requiring review from@derivita/engineering.Ecosystem chosen
npm_and_yarnonly. This repo has bothpackage-lock.jsonandyarn.lockat the root but fetch-metadata'snpm_and_yarnslug covers both. No other ecosystem is in use.CODEOWNERS team — flagged as a guess
gh api repos/derivita/protobuf-javascript/teamsreturned an empty list — no org team currently has push/admin access to this repo, so there's no clearly-fitting domain team to use. Falling back to@derivita/engineeringper the rollout's guidance. A reviewer should confirm this is the right owner (or add a more specific team if one should have access to this repo).What's NOT done — needs a human
main's branch protection has:build.yml,codeql.yml)allow_squash_merge,allow_merge_commit,allow_rebase_mergealltrue), and I can't tell from the API alone whethermainrequires linear history. Defaulted tomethod = "squash"(matches every repo onboarded so far) — please verify this is compatible withmain's actual merge-method restrictions, if any.automerge/dependabot-automergelabels, stays for human review)Links: derivita/infrastructure#1719, derivita/derivita-ui#247