Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .github/CODEOWNERS
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# Merge-governance surface. Any change under /.github/ — including a NEW
# 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Kodiak's merge-queue config — an unreviewed edit here could weaken or
# disable branch-protection-equivalent merge gating (e.g. the automerge
# label, blocking labels, or merge method).
/.kodiak.toml @derivita/engineering
13 changes: 13 additions & 0 deletions .github/dependabot.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
version: 2
updates:
- package-ecosystem: npm

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

directory: /
schedule:
interval: weekly
cooldown:
default-days: 7
groups:
minor-and-patch:
update-types:
- minor
- patch
32 changes: 32 additions & 0 deletions .github/workflows/dependabot-automerge-label.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
name: Dependabot Auto-merge Label

# Labels Dependabot's own patch/minor npm bumps `automerge` +
# `dependabot-automerge` so Kodiak can auto-approve and merge them. See
# derivita/shared-workflows's dependabot-automerge-label.yml (the reusable
# workflow this calls) for the full security rationale, and
# derivita/infrastructure#1716 for the reference rollout.
on:
pull_request_target:
types: [opened, reopened, synchronize]
branches: [main]

permissions:
contents: read
pull-requests: write

jobs:
label:
# Skip the runner allocation entirely for human PRs — the callee no-ops
# on these anyway (it does its own author and head-commit-signature
# checks), but there's no reason to spend a job on every PR to main to
# find that out.
if: github.event.pull_request.user.login == 'dependabot[bot]'
# Pinned to the v1 tag's commit, not the mutable tag itself — this
# workflow controls what gets auto-approved, and anyone with write
# access to shared-workflows can retarget a tag with no PR in this repo.
uses: derivita/shared-workflows/.github/workflows/dependabot-automerge-label.yml@9752b04e2e27d6cbc6a06908345e85db2b576abf # v1
with:
# fetch-metadata's package-ecosystem slug for npm/pnpm, not
# dependabot.yml's "npm" value — see the callee's own comment on
# this naming split.
ecosystems: npm_and_yarn
26 changes: 26 additions & 0 deletions .kodiak.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
version = 1

[merge]
automerge_label = "automerge"
method = "squash"
delete_branch_on_merge = true
blocking_labels = ["wip", "do-not-merge"]

[approve]
# Dedicated label, not the general-purpose `automerge` merge-queue label:
# auto_approve_labels grants Kodiak's own bot approval to anything carrying
# this label regardless of author. Reusing `automerge` would let any
# contributor with label-write access get an unreviewed PR auto-approved
# just by attaching it.
#
# dependabot-automerge is only *intended* to be applied by
# dependabot-automerge-label.yml, after its own semver/ecosystem check
# confirms the PR is a provable patch/minor dependency bump — never by
# hand. GitHub doesn't restrict which repo collaborators can attach an
# *existing* label, and Kodiak's evaluation here only checks label
# presence, not who added it or the PR's author, so anyone with
# 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 automerge and dependabot-automerge already exist as labels in this repo, so any Triage/Write collaborator can attach dependabot-automerge to their own PR, collect Kodiak's bot approval, add automerge, and squash-merge to main with 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 revoke job 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 of revoke.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

auto_approve_labels = ["dependabot-automerge"]
Loading