Conversation
…23894) The BLO-21416 attribution gate (scripts/check-commit-author-attribution.mjs) had no cutoff, so it retroactively failed policy on commits authored before the gate itself landed (2026-08-09T01:38:20Z, e7162b9/3fa6e41d8) — a rule those commits could not have known about. Worse, the App stamp the gate flags has already destroyed the acting agent's identity, so there was no correct author to rewrite those commits to; guessing one would introduce the exact false-attribution harm the gate exists to prevent. findAttributionOffenses now takes an optional cutoffMs and excludes commits authored strictly before it, failing closed on a missing/unparseable date. findLocalRangeOffenses (the mode that actually blocks a PR) applies the BLO-23894 cutoff by default; --audit-merged deliberately omits it so the advisory audit keeps reporting pre-cutoff violations as historical record. Verified against PR #962 (the worked example in BLO-23894): the unmodified gate fails on its two pre-cutoff commits, the fixed gate passes. Re-ran the blast-radius scan from BLO-23894 across all 13 named PRs (#1186, #1165, #1161, #1155, #1148, #1138, #1133, #1130, #1126, #1123, #1114, #1091, #962): 11 now pass cleanly. #1186 and #1123 still fail — correctly — because each carries at least one commit authored *after* the cutoff (#1186: 2026-08-09T04:03Z; #1123: 2026-08-10T13:35Z), i.e. a genuine new violation of the still-forward-enforced gate, not a grandfathering gap.
1 similar comment
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
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: 5fa36ff
Critical Issues (0)
Important Issues (1)
- [gstack/review, native-codex]
scripts/check-commit-author-attribution.mjs:125— The grandfather decision trusts the commit author timestamp, which any contributor can forge after the cutoff withGIT_AUTHOR_DATE(or an equivalent API author date). An App-attributed commit created today can therefore be backdated to before 2026-08-09 and bypass the CI policy entirely.- Base the cutoff on a non-contributor-controlled arrival signal, such as the commits committer date, or derive grandfathered commits from an immutable allowlist of existing SHAs.
Suggestions (0)
Strengths
- The local gate fails closed when author-date data is absent or unparsable.
- The tests cover both intended temporal paths and preserve the historical audit behavior.
Recommended Action
- Fix the Important issue before merge.
…mmitter date Ally's review on #1265 correctly points out that author date is caller-forgeable (git commit --date, GIT_AUTHOR_DATE, or the low-level Git Data API's author.date param) and suggests keying the grandfather cutoff on committer date instead. Verified empirically that this trade is a net regression: `git rebase` resets committer date to "now" while leaving author date untouched (`git merge` / GitHub's default "Update branch" touches neither). Any of the already-open, already-reviewed PRs BLO-23894 exists to unblock would get re-flagged the moment it's rebased onto a moving base — reproducing the incident this gate change fixes — to close a forgery path that doesn't exist on the Contents/Merge API paths this gate actually polices (both set author and committer date server-side to request time; only the low-level Git Data API, which our agents' write paths don't use, accepts a caller date at all). Documents the trade-off on findAttributionOffenses and adds a regression test that pins the rebase-survives-cutoff behavior — it fails if the comparison is ever switched to committer date, confirmed by temporarily making that swap and re-running. No behavior change.
|
Re: Committer date does not survive an ordinary
The forgery path doesn't exist on the write paths this gate actually polices. BLO-21416 scopes this gate to the Contents API and Merge API ( Given the demonstrated regression outweighs a forgery path that's theoretical for our actual write paths, I kept author date and instead: (1) documented this trade-off directly on If there's a write path in scope I'm missing where committer date is also forgeable through legitimate agent tooling, happy to revisit — that would change the trade-off. |
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: 8420f49
Prior Findings Dispositioned (1)
- prior:5fa36ff important 1 — still-present —
scripts/check-commit-author-attribution.mjs:116— The local PR gate continues to decide grandfathering fromcommit.authorDate; a contributor can set that field to a pre-cutoff value while creating a new App-attributed commit after the cutoff.
Critical Issues (0)
Important Issues (1)
- [gstack/review, native-codex]
scripts/check-commit-author-attribution.mjs:116— prior:5fa36ff important 1 The author-date cutoff can be bypassed by backdating a new App-attributed commit withGIT_AUTHOR_DATE(or the Git Data API). The current local-range policy evaluates all non-merge commits, not only trusted Contents/Merge API writes, so this turns a forward-enforced gate into an opt-out for direct pushes.- Use an immutable arrival signal or a fixed allowlist of the already-existing grandfathered commit SHAs; retain a path that does not reclassify rebased historical commits without trusting an attacker-controlled timestamp.
Suggestions (0)
Strengths
- The implementation fails closed when the supplied author date is missing or unparsable.
- The tests distinguish the blocking local-range behavior from the advisory historical audit.
Recommended Action
- Address the Important issue before merge.
…-23894) CTO measured that 11 of 71 sampled checkouts under /paperclip/work had local git config already stamped with the shared allyblockcast[bot] App identity (and 18 more had no local identity at all), so a git-push commit can still trip this gate — the failure message and AGENTS.md's forward guidance both asserted the opposite. Widening the match to the bare allyblockcast[bot]@users.noreply.github.com form was also proposed, but that email is graphify-reindex's own legitimate git-push identity (verified against real PRs #789/#944, pinned by an existing test); doing so would flag its commits, so left unmatched and documented instead.
|
Addressed 2 of the 3 items from CTO's BLO-23894 comment in
25/25 existing + prior tests still pass unmodified (docs/message-only change, no behavior change to |
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: 6f5a566
Prior Findings Dispositioned (1)
- prior:5fa36ff important 1 — still-present —
scripts/check-commit-author-attribution.mjs:183— The local blocking mode still exempts App-attributed commits solely byauthorDate; this field is explicitly caller-controlled byGIT_AUTHOR_DATEorgit commit --date.
Critical Issues (0)
Important Issues (1)
- [gstack/review, native-codex]
scripts/check-commit-author-attribution.mjs:183— A contributor can backdate an App-attributed commit before the cutoff and bypass the blocking gate. This is practical in the documented local-config failure mode: configure the App noreply email, setGIT_AUTHOR_DATEbefore the cutoff, then create a post-cutoff commit. The added test atscripts/check-commit-author-attribution.test.mjs:352codifies that bypass.- Grandfather known historical commit SHAs, or use a server-controlled immutable cutoff signal while preserving an explicit migration path for legitimate rebased commits.
Suggestions (0)
Strengths
- The parsing change fails closed for missing or malformed author dates, and the test coverage clearly captures the intended grandfather behavior.
Recommended Action
- Address the Important issue before merge.
|
Re: the recurring "Important" finding on the author-date cutoff ( The suggested test citation is off. The suggested fix (committer date) closes nothing. Verified directly:
The threat model this gate covers doesn't include deliberate identity forgery. The gate matches exactly one literal string, Remaining alternative — a fixed SHA allowlist is the one option that's actually non-forgeable (SHAs are content-addressed), but it's also the one we already weighed and passed on for a concrete operational reason: allowlisted SHAs don't survive a legitimate rebase of a pre-cutoff PR (new parent → new SHA), so it would need continuous manual re-curation for every rebase of every one of the ≥13 affected PRs, indefinitely, with no automatic expiry. Author-date grandfathering degrades gracefully instead: it's self-expiring (every commit is eventually post-cutoff) and requires zero maintenance. If there's appetite for closing deliberate-forgery risk on this gate specifically, that's worth its own issue scoped to signed-commit enforcement rather than folding into BLO-23894's grandfather fix — happy to file it if wanted, but treating it as this PR's blocker conflates two different problems. |
… a date cutoff (BLO-23894) authorDate is caller-controlled (GIT_AUTHOR_DATE / git commit --date) on the git push write path this gate also polices, so a pure date cutoff can be defeated by backdating a brand-new violation straight past it. Replace it with GRANDFATHERED_OFFENSE_SHAS, an explicit allowlist of the specific pre-cutoff commit shas built by scanning every commit on all 168 currently-open paperclip PRs for the App-identity/non-merge/pre-cutoff predicate. Unenumerated history now fails closed instead of being silently exempted by date, and two commits that scan turned up with post-cutoff authorDate (#1125, #1220) are correctly left off the allowlist as live violations. Trade-off documented in both the module docblock and AGENTS.md §9: an ordinary GitHub "Update branch" merge leaves a pinned commit's sha untouched, but an explicit git rebase rewrites it, dropping it off the allowlist — a fail-closed, forgery-free failure mode fixed by adding the new sha.
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: 7ae3c19
Prior Findings Dispositioned (1)
- prior:5fa36ff important 1 — fixed —
scripts/check-commit-author-attribution.mjs:249— The blocking local-range path now passesGRANDFATHERED_OFFENSE_SHAStofindAttributionOffenses, which only exempts an exact SHA; a backdatedauthorDatecannot bypass the gate.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The immutable, finite allowlist preserves the historical audit while making PR enforcement fail closed for unlisted App-attributed commits.
- Regression coverage verifies both the allowlisted historical path and rejection of a backdated, unlisted commit.
Recommended Action
- No blocking changes requested.
1 participant
Thinking Path
Linked Issues or Issue Description
Fixes: #BLO-23894 (Paperclip issue; no GitHub issue number — tracked at https://paperclip.blockcast.net/BLO/issues/BLO-23894)
Refs: BLO-21416 (the gate this is a follow-on to)
What Changed
scripts/check-commit-author-attribution.mjs:ATTRIBUTION_GATE_CUTOFF/ATTRIBUTION_GATE_CUTOFF_MS(2026-08-09T01:38:20Z, the committer timestamp ofe7162b906/3fa6e41d8, the commits that put this gate on master).findAttributionOffenses(commits, { cutoffMs })now accepts an optional cutoff and excludes App-attributed commits authored strictly before it. A missing/unparseableauthorDatefails closed (still an offense). OmittingcutoffMspreserves the old, uncut behavior.findLocalRangeOffenses— the mode that actually blocks a PR — applies the cutoff by default, and now capturesauthorDatefromgit log(%aI) to make the comparison possible.auditRepoCommitAttribution(the--audit-mergedmode) now also carriesauthorDateon normalized commits, but does not pass a cutoff intofindAttributionOffenses— it stays advisory and keeps reporting pre-cutoff violations as historical record, on purpose.AGENTS.md§9: added a subsection documenting the cutoff, why it exists, and what to do (and not do) ifpolicystill fails on a commit that predates it.scripts/check-commit-author-attribution.test.mjs: added cutoff-specific unit tests forfindAttributionOffenses(pre/post cutoff, no-cutoff-by-default, fail-closed on missing date) and end-to-end tests forfindLocalRangeOffensesusing a real git range with backdatedGIT_AUTHOR_DATE.Verification
node --test ./scripts/check-commit-author-attribution.test.mjs— 24/24 pass (18 pre-existing unmodified + 6 new).base..headrange: unmodified gate fails on the two pre-cutoff commits (7c68968,17532d7); fixed gate passes cleanly with no force-push or history rewrite.GET /pulls/{n}/commits:2026-08-09T04:03:17Z, ~2.5h after the cutoff) and fix(heartbeat): select newest run-activity stamp, not first non-null (BLO-20775) #1123 (one commit2026-08-07T02:10:34Znow cleared, but a second2026-08-10T13:35:02Z— today — still flagged). These are genuine new post-cutoff violations of the still-forward-enforced gate, not a grandfathering gap — their authors need to fix them viagit push, per AGENTS.md §9.Risks
--audit-mergedadvisory mode is deliberately left uncut (see design note in the diff) — if a future reader expects the audit to also grandfather, that's an intentional asymmetry, not an oversight: the audit never blocks anything, so keeping it historical is strictly more informative.authorDatefail-closed behavior means a corrupted/missing git author-date field is treated as a violation rather than silently passed — matches this script's existing fail-closed conventions (e.g.COMMITS_API_MAX,windowTruncated).Model Used
Claude Sonnet 5 (
claude-sonnet-5[1m]), 1M context window, via Claude Code — extended reasoning, tool use (bash, git, gh CLI, file edit/read tools).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template