fix(ci): catch every spelling of the App noreply email in the commit-attribution gate (BLO-26647) - #1350
fix(ci): catch every spelling of the App noreply email in the commit-attribution gate (BLO-26647)#1350allyblockcast[bot] wants to merge 1 commit into
Conversation
…id-prefixed form (BLO-26647) findAttributionOffenses matched authorEmail against a single literal string, so the same allyblockcast[bot] App installation (id 290875700) passed the gate under a bare form, a wrong-numeric-prefix form, and (on one open PR) a pre-cutoff bare-form commit that would have retro-broken once anyone tightened the matcher. Broadens the match to a regex covering any numeric prefix on the allyblockcast[bot] local part, scoped to users.noreply.github.com. That alone would also flag the graphify-reindex bot's own git-push identity, which happens to share the bare email — so the exemption for it moves from "email doesn't match" (accidental) to an explicit author-name denylist (NON_AGENT_PROCESS_AUTHOR_NAMES), since email can't tell the two apart. Confirmed via the GitHub commits API that the still-unmatched allyblockcast@users.noreply.github.com (no [bot]) is a different, real, resolvable account (id 296676656) and stays out of scope; confirmed the one 220200645-prefixed commit doesn't resolve to any account at all and is deliberately caught rather than exempted. Grandfathers 6e7440d on open PR #1076, a genuine pre-cutoff bare-form commit that only passed policy today because of this bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 similar comment
|
@ally please review at head 03d5115 — ci: catch every spelling of the App noreply email in the commit-attribution gate. Focus on false negatives from casing/subaddressing variants. Context: the original review request on this PR was lost during the codex provider outage (BLO-27123) — codex |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 03d5115
Critical Issues (0)
Important Issues (2)
- [native-codex]
scripts/check-commit-author-attribution.mjs:177—APP_NOREPLY_EMAIL_PATTERNis case-sensitive and does not accept subaddressed local parts, so casing/subaddressing variants of the App address remain false negatives even though this change claims to cover every spelling.- Normalize the email (or use a case-insensitive matcher) and define the supported subaddress form explicitly, then add regression tests for mixed-case domain/local-part and the subaddress separator/segment observed from the write paths. Preserve the no-
[bot]account boundary.
- Normalize the email (or use a case-insensitive matcher) and define the supported subaddress form explicitly, then add regression tests for mixed-case domain/local-part and the subaddress separator/segment observed from the write paths. Preserve the no-
- [gstack/review]
scripts/check-commit-author-attribution.mjs:334— the graphify exemption trusts the exact, caller-controlled Git author name while matching a caller-controlled bare email. An agent or compromised checkout can set both tographify-reindex (allyblockcast)and evade the gate entirely.- Do not use an unverified author name as the sole exemption. Bind the exemption to trusted evidence such as the verified graphify branch/ref and/or an independently authenticated commit/PR signal, or remove the exemption and handle graphify attribution through a separate trusted path. Add a test showing an arbitrary commit using that name cannot bypass enforcement.
Suggestions (0)
Strengths
- The regex is anchored to the intended domain and retains the
[bot]boundary, avoiding an accidental match of the distinctallyblockcastuser. - Local parsing and REST audit normalization now carry author names consistently, and the new tests cover the main observed spellings plus the grandfathered SHA.
Recommended Action
- Address the Important issues before merge.
- Add regression tests for the variant and exemption-boundary cases.
Thinking Path
Linked Issues or Issue Description
Fixes: BLO-26647 (internal Paperclip tracker; no public GitHub issue — Blockcast/paperclip is managed via the Paperclip board, see AGENTS.md).
Follow-on to #(BLO-21416, the gate) and #(BLO-23894, its grandfather clause) — both already merged.
What Changed
scripts/check-commit-author-attribution.mjs:APP_NOREPLY_EMAILcomparison withAPP_NOREPLY_EMAIL_PATTERN, a regex matching an optional any-digits numeric prefix + theallyblockcast[bot]local part onusers.noreply.github.com. This catches the bare form, and the one observed wrong-prefix form (220200645+…), which the GitHub commits API confirms does not resolve to any account (not a second legitimate installation — a malformed stamp).NON_AGENT_PROCESS_AUTHOR_NAMES(currently justgraphify-reindex (allyblockcast)) and threadedauthorNamethrough local-range parsing (%anin the git-log format) and the--audit-mergedAPI normalization, so the graphify-reindex bot's git-push identity — which shares the bare email with several genuine agent commits (author namesCTO,Staff Engineer, plainallyblockcast[bot]) — is excluded by name instead of accidentally by email.allyblockcast@users.noreply.github.com(no[bot]) is a different, real, resolvable account (login: allyblockcast, id296676656) — out of scope for this gate (scoped to installation290875700), documented rather than silently matched or silently dropped.6e7440da271f5df50de35ec8bfeed5afaf70f168(open PR fix(github-webhook): close four ownership-parsing leaks found in review (BLO-20886) #1076) toGRANDFATHERED_OFFENSE_SHAS— a genuine pre-cutoff, bare-form App commit that only passedpolicytoday because of this bug.main()error strings to match.scripts/check-commit-author-attribution.test.mjs: fixed the graphify-reindex test (it previously asserted the exemption without ever passing anauthorName, which is exactly the premise this PR corrects) and added 8 new cases covering every spelling, the name-based exemption, the two identity resolutions, and the new grandfather entry..github/workflows/pr.yml/AGENTS.md§9: updated the accompanying comments, which stated the now-incorrect premise that the bare email belonged exclusively tographify-reindex.Verification
node --test scripts/check-commit-author-attribution.test.mjs— 36/36 passing (28 pre-existing + 8 new).node scripts/check-commit-author-attribution.mjs --base <merge-base> --head <pr-1076-head>against this branch: exits 0, confirming the grandfather addition keeps that PR green under the broadened matcher.git log origin/master --no-merges --since=2026-07-01, bucketed throughfindAttributionOffenseswith no allowlist): before this PR, exact-match form catches 192, three variant spellings evade it (18 bare, 2 no-[bot]-but-those-are-the-296676656-identity, 1 wrong-prefix) — 7 of the newly-caught-in-principle commits landed afterATTRIBUTION_GATE_CUTOFF(up from the 2 recorded when BLO-26647 was filed a few hours earlier — this leak is active, not historical). All 7 are already merged intomaster, so none need grandfathering: they're common ancestors of every future PR base and cannot reappear in abase..headdiff. Going forward, the broadened matcher blocks this class before merge.Risks
allyblockcast(no-[bot]) account; (c) grandfathering the one known live pre-cutoff casualty (fix(github-webhook): close four ownership-parsing leaks found in review (BLO-20886) #1076).NON_AGENT_PROCESS_AUTHOR_NAMES— documented in both the script docblock and thepr.ymlcomment as the intended remediation (not a branch exemption).6c0e9c336,d41030016, and 5 others) — merged history is intentionally not rewritten, matching BLO-23894's existing precedent.Model Used
Claude Sonnet 5 (
claude-sonnet-5[1m]), 1M context window, extended reasoning, agentic tool use (Bash, GitHub MCPget_commit, git worktree isolation) — used to investigate real commit/API evidence and implement + test the fix.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template