Skip to content

fix(ci): catch every spelling of the App noreply email in the commit-attribution gate (BLO-26647) - #1350

Open
allyblockcast[bot] wants to merge 1 commit into
masterfrom
blo-26647
Open

fix(ci): catch every spelling of the App noreply email in the commit-attribution gate (BLO-26647)#1350
allyblockcast[bot] wants to merge 1 commit into
masterfrom
blo-26647

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 12, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • The commit-attribution gate (BLO-21416, policy CI job) exists to catch commits that the shared allyblockcast[bot] GitHub App identity (id 290875700) stamped over an agent's real authorship, since every agent pod authenticates as that one App installation.
  • The gate matched authorEmail against a single literal string (290875700+allyblockcast[bot]@users.noreply.github.com), but the App stamps its noreply address under at least three other spellings measured on origin/master (bare form, a wrong-numeric-prefix form, and a no-[bot] form) — 15+ commits since 2026-07-01 evaded the gate on spelling alone, 7 of them landing after the gate's own cutoff.
  • This is a live, ongoing leak, not a one-time historical gap: an open PR (fix(github-webhook): close four ownership-parsing leaks found in review (BLO-20886) #1076) currently only passes policy because of this bug, and would retro-break the moment anyone tightened the matcher without also grandfathering it.
  • This pull request broadens the email match to a regex covering every numeric-prefix spelling of the allyblockcast[bot] local part, while keeping the one legitimate exemption (the graphify-reindex maintenance bot, which shares the bare email) — but keys that exemption on author name instead of email, since email can't tell the two apart. It also grandfathers the one specific open-PR commit this breaks.
  • The benefit is the gate now measures identity (any spelling of the same App installation) instead of one spelling of it, closing the exact bypass BLO-21416/BLO-23894 were built to prevent.

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:
    • Replaced the single-literal APP_NOREPLY_EMAIL comparison with APP_NOREPLY_EMAIL_PATTERN, a regex matching an optional any-digits numeric prefix + the allyblockcast[bot] local part on users.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).
    • Added NON_AGENT_PROCESS_AUTHOR_NAMES (currently just graphify-reindex (allyblockcast)) and threaded authorName through local-range parsing (%an in the git-log format) and the --audit-merged API normalization, so the graphify-reindex bot's git-push identity — which shares the bare email with several genuine agent commits (author names CTO, Staff Engineer, plain allyblockcast[bot]) — is excluded by name instead of accidentally by email.
    • Confirmed via the GitHub commits API that the still-unmatched allyblockcast@users.noreply.github.com (no [bot]) is a different, real, resolvable account (login: allyblockcast, id 296676656) — out of scope for this gate (scoped to installation 290875700), documented rather than silently matched or silently dropped.
    • Added 6e7440da271f5df50de35ec8bfeed5afaf70f168 (open PR fix(github-webhook): close four ownership-parsing leaks found in review (BLO-20886) #1076) to GRANDFATHERED_OFFENSE_SHAS — a genuine pre-cutoff, bare-form App commit that only passed policy today because of this bug.
    • Rewrote the module docblock and the two 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 an authorName, 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 to graphify-reindex.

Verification

  • node --test scripts/check-commit-author-attribution.test.mjs — 36/36 passing (28 pre-existing + 8 new).
  • Fetched PR fix(github-webhook): close four ownership-parsing leaks found in review (BLO-20886) #1076's head and ran 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.
  • Manual re-scan (git log origin/master --no-merges --since=2026-07-01, bucketed through findAttributionOffenses with 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 after ATTRIBUTION_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 into master, so none need grandfathering: they're common ancestors of every future PR base and cannot reappear in a base..head diff. Going forward, the broadened matcher blocks this class before merge.
  • Did not re-run the full 168-open-PR sweep BLO-23894 originally did under the new predicate — only the two live instances flagged on the BLO-26647 issue thread were checked: fix(github-webhook): close four ownership-parsing leaks found in review (BLO-20886) #1076 (grandfathered here) and fix(approvals): decide both approval-link doors through one evaluator (BLO-24699) #1293 (explicitly deferred by the issue reporter, who is tracking it separately and asked that it not be grandfathered reflexively).

Risks

  • Low-to-medium. This tightens a CI gate — the main risk is a false positive blocking an unrelated legitimate PR. Mitigated by: (a) the name-based graphify-reindex exemption, tested directly; (b) the explicit, resolved carve-out for the real 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).
  • If another automated process starts sharing the bare App email under a new author name, it will now be flagged until that name is added to NON_AGENT_PROCESS_AUTHOR_NAMES — documented in both the script docblock and the pr.yml comment as the intended remediation (not a branch exemption).
  • Does not retroactively fix the 7 already-merged post-cutoff leaks (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 MCP get_commit, git worktree isolation) — used to investigate real commit/API evidence and implement + test the fix.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots — N/A, no UI change
  • I have updated relevant documentation to reflect my changes
  • I have considered and documented any risks above
  • All Paperclip CI gates are green — pending CI run on this PR
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review
  • I will address all Greptile and reviewer comments before requesting merge

…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>
@allyblockcast

allyblockcast Bot commented Aug 12, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-26647
🔗 Paperclip issue: BLO-23894
🔗 Paperclip issue: BLO-21416

1 similar comment
@allyblockcast

allyblockcast Bot commented Aug 12, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-26647
🔗 Paperclip issue: BLO-23894
🔗 Paperclip issue: BLO-21416

@allyblockcast

allyblockcast Bot commented Aug 14, 2026

Copy link
Copy Markdown
Author

@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 success sat at 0/min from ~14:50Z to 17:54Z and Ally is pinned to openai/gpt-5.6-terra on that pool. Recovery does not revisit the stranded set, so this is a forward-only re-request. Codex recovered 17:56Z (~55 req/min, near-zero errors) and the path is verified working (#1329, #1341 reviewed at head in ~3 min).

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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:177APP_NOREPLY_EMAIL_PATTERN is 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.
  • [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 to graphify-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 distinct allyblockcast user.
  • 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

  1. Address the Important issues before merge.
  2. Add regression tests for the variant and exemption-boundary cases.

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.

0 participants