Skip to content

feat(pr-pipeline): judge only lane-authored pull requests in pr-verify - #7710

Merged
kyle-sexton merged 5 commits into
mainfrom
feat/pr-verify-lane-authors-only
Oct 17, 2026
Merged

kyle-sexton merged 5 commits into
mainfrom
feat/pr-verify-lane-authors-only

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 17, 2026 •

Copy link
Copy Markdown
Member

Refs: #7709

Summary

pr-verify now judges only PRs the lanes App authored, which are the only PRs pr-merge reads a verdict for. Other PRs skip before the model step with skipped: author not the lanes App. The owner decided this (Q623); it settles #7709 item 9. The first live verdict, a FAIL on owner PR 7698 for no linked issue, had no reader and still cost a model run.

Fix

  • New setting merge.verify-authors: lane (default) or any. Any other value skips with reason verify-authors-invalid.
  • resolve-pr.mjs matches lane authorship the way pr-merge's _lane_authored does: account type Bot, and the login compared trimmed, lowercased and without [bot] against the configured agent identity. An empty identity skips. Dependency managers keep their own skip, which runs first.
  • pr-verify.yml reads both values with yq from main's docs/conventions/pr-pipeline.yaml and passes them as env.
  • Schema, README "Verifier verdict" section and convention CHANGELOG (1.23.0; main took 1.22.0) updated. The README now also says the trusted-spec rule trusts an account, not a person. Agent sessions that use an owner's gh login post as that owner. Until they have their own GitHub identity, report-only stays on (source-control: settle the verifier verdict follow-ups before report-only flips #7709 item 10).

Verification

  • node --test in .github/scripts/pr-verify: 29 pass. These tests failed before the change: a human PR skips, an unknown value skips, and main's summary line. The lane PR and any cases passed before the change too, because every PR went through then. The "another App" case fails when the check is cut down to account type alone (mutation run on a scratch copy).
  • After merging main: check-convention-yaml.sh --check and actionlint on pr-verify.yml are clean; check-affected.sh --tests: 6 passed, 3 skipped.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 5 commits October 17, 2026 01:19
merge.verify-authors (lane, any; absent is lane) decides whose pull
requests pr-verify.yml judges. Under lane, resolve-pr.mjs skips any PR
not opened by a Bot account whose login is merge.agent-identity, read
from main's pr-pipeline.yaml and compared as babysit_merge_policy
_lane_authored does, before the model runs: reason author-not-lane and
the step summary line "skipped: author not the lanes App". any judges
every author's; any other value skips (verify-authors-invalid).

Refs: #7709

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs: #7709

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs: #7709

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main took the pr-pipeline convention's 1.22.0 entry, so this branch's
merge.verify-authors entry moves to 1.23.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every earlier case was told apart by account type alone, so a check
reduced to type Bot passed them all. Another App's Bot account is now
expected to skip with author-not-lane, and the no-identity case passes
the empty string the workflow sends.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 17, 2026 05:36
@claude

claude Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 46s —— View job


Security review complete

  • Gather context (PR diff, changed files)
  • Review resolve-pr.mjs authorship-matching logic
  • Review pr-verify.yml workflow changes
  • Review convention/schema/docs changes for security-relevant claims
  • Check instruction-surface deletions against protected classes
  • Post findings

No security issues found in this diff.

What was checked:

  • laneAuthored() in resolve-pr.mjs mirrors pr-merge's _lane_authored (account type === "Bot" plus a trimmed/lowercased/[bot]-stripped login match). GitHub assigns type: "Bot" and the [bot] login suffix itself for App-backed accounts, so this can't be spoofed by a regular user or a same-named human account — matches the existing trust model, not a new surface.
  • An empty laneIdentity (unset merge.agent-identity) makes laneAuthored always false, so a missing/misconfigured identity fails closed (skips everyone) rather than open (judging/admitting everyone). Safe default.
  • VERIFY_AUTHORS/LANE_IDENTITY are read via yq from the main-branch checkout (.base/docs/conventions/pr-pipeline.yaml), not from the PR's own files, and are passed to Node through env, never interpolated into a shell command — no injection surface and the PR branch can't override the policy that judges it.
  • The new skip (author-not-lane) only narrows which PRs get a model verdict; it doesn't change pr-merge's eligibility logic. Non-lane-authored PRs were already ineligible for unattended merge there (_lane_authored gates eligibility in babysit_merge_policy.py), so this doesn't open a bypass — it just stops spending a verifier run on PRs whose verdict was never read.
  • Tests (resolve-pr.test.mjs) cover the relevant spoofing attempts directly: a human account holding the lane's literal login, a different Bot App account, case/spelling variants, and an empty identity — all correctly skip.
  • No changes to workflow permissions:, triggers, or the model's tool allowlist in pr-verify.yml; the added lines are two read-only yq lookups from the trusted base checkout.

claude-lane-summary: lane=claude-security-review sha=145d510694086cec75227234d5f69d57bcdb29a8 run=39006887302

@claude

claude Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 4m 43s —— View job


Review complete

  • Gather context (read diff, changed files)
  • Invoke /review:code-review and follow its procedure
  • Post inline findings (none)
  • Post summary comment

Summary

Reviewed all 7 changed files against REVIEW.md/CLAUDE.md criteria: correctness, maintainability, test coverage. No findings meet the high-signal bar for an inline comment.

Notable things checked and confirmed correct (not restating the PR body's own claims, but independently traced):

  • The premise holds. I traced whether pr-merge ever reads a pr-verify verdict for a non-lane-authored PR (e.g. one using the merge: agent-allowed opt-in label), which would make the new default verify-authors: lane wrongly starve such a PR of a verdict forever. It doesn't: require-verifier-verdict is gated on authority == PR_MERGE, and that authority's eligibility path is lane_eligibility (strict _lane_authored, babysit_merge_policy.py#L1459), never the opt-in-label eligibility() path used by local-babysit authority. So "lane-authored PRs are the only ones pr-merge reads a verdict for" is accurate, and the new skip is safe.
  • loginKey/laneAuthored in resolve-pr.mjs#L68-70 match babysit_merge_policy._login_key/_lane_authored semantics (trim, casefold, strip trailing [bot], require type/__typename == Bot).
  • Skip order (dependency-manager → verify-authors validity → lane-authored → already-judged) matches the code comment and the PR body's stated ordering.
  • Schema/CHANGELOG/README additions are internally consistent (version stays 1, no duplicate CHANGELOG entries, verify-authors correctly nested under merge in the schema).
  • Test coverage for the new behavior (invalid value, human PR, another App spoofing the login, case-insensitive login match, empty identity, any mode, and main's step-summary/log output) looks complete and exercises the real edge cases, including the type-confusion case ("a User account holding the App's login is not the App").

Not independently verified: this sandbox's Bash tool denies node invocations, so I could not run node --test .github/scripts/pr-verify/resolve-pr.test.mjs myself. The PR body's "29 pass" / mutation-testing claim is author-claimed and unverified by me; I verified correctness by reading the test and implementation code instead.

claude-lane-summary: lane=claude-review sha=145d510694086cec75227234d5f69d57bcdb29a8 run=39006887308

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-17T05:38:22.984255Z 145d510 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

Copy link
Copy Markdown
Contributor

claude-security-review has reviewed this pull request through 145d510; a later push is reviewed from there.

@github-actions

Copy link
Copy Markdown
Contributor

claude-review has reviewed this pull request through 145d510; a later push is reviewed from there.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 17, 2026
Merged via the queue into main with commit 9d753f5 Oct 17, 2026
63 of 74 checks passed
@kyle-sexton
kyle-sexton deleted the feat/pr-verify-lane-authors-only branch October 17, 2026 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant