Repository navigation
ci(release): baseline five squash-stamped identity trailers blocking v0.10.0 (#17717) - #17718
Conversation
…v0.10.0 (#17717) #17717 promotes main -> release and fails "No commit trailers" on commits already landed. Per-PR scans passed -- including #17696's own -- because the gate compares BASE..HEAD and the trailer does not exist until GitHub's squash-merge creates it. Only a promotion re-scans the landed range, so the check runs on an object no PR ever showed it. DERIVED AND VERIFIED, NOT TAKEN FROM A LIST. The run log reports ten "Offending commit:" lines and nine unique SHAs. The tenth is the workflow's own printf template -- `Offending commit: %s\n' "${bad[@]}"` -- echoed into the log. Nine real commits, and a count taken from the line count would have been the measurement's artefact rather than the population. The nine are TWO classes, established by reading each commit's trailers rather than assuming one cause: five Co-authored-by: t <t@example.invalid> four Co-authored-by: Claude <noreply@anthropic.com> ONLY THE FIVE ARE BASELINED. They are the same cause as the existing `test <test@test>` entries, a second time: a local user.name/user.email override shadowed the correct global identity, branch commits carried it, and the squash stamped it onto the result. `t <t@example.invalid>` is not a person and claims no attribution. The source is CLOSED -- no worktree config and no global config resolves user.name to `t`, checked rather than assumed. That check is what makes the class bounded at five, and the header says so, because the previous instance's header claimed the override was "since corrected" and a different one appeared anyway. The claim was true and insufficient; the check is the thing. THE FOUR ARE HELD, AND THE HEADER SAYS WHY. `Co-authored-by: Claude` is a real attribution claim -- exactly what no-commit-trailers.yml (#13654) exists to reject -- not a misconfigured identity. The reasoning that justifies the five does not reach it, so folding it in would exempt the thing the rule is for on a ruling about something else. A reader of this file must not conclude that a `Claude` co-author is baselinable. Its source is also not closed, which is why it is a separate question rather than a longer list: the agent harness instructs sessions to append that trailer while this project's rules forbid it and this gate enforces them. Until that is resolved at the source, new instances keep landing and a baseline is treating the symptom. Filed separately. THE TOP RULE IS NARROWED, NOT SUSPENDED. It said entries may only be removed. It now names the one class an addition is permitted for -- a trailer no branch commit carried, created by the squash, found only at promotion, on a history that cannot be rewritten -- and says explicitly that a trailer which WAS on a branch commit does not qualify, because that one was fixable and the fix is to fix it. Verified the gate's own parse: of the nine, exactly the five appear in `sed 's/#.*//' | tr -d '[:blank:]'`, and the four named in the header comment do not. The workflow is untouched. Refs #17717 Refs #13654
|
Warning Review limit reachedNext included review available in 45 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mrveiss
left a comment
mrveiss
left a comment
There was a problem hiding this comment.
Review — the five are right, confirmed by a second route. One paragraph is wrong, and the check this PR ran by hand is the guard the file does not have.
The five entries are correct. I measured the population independently of the run log, from the commit objects rather than from the workflow's output:
git log origin/release..origin/main --format='%(trailers:key=Co-authored-by,valueonly)'
99 commits, 33 with a Co-authored-by
35 github-actions[bot] (two spellings) -> allowed, bot pattern
4 Claude <noreply@anthropic.com>
5 t <t@example.invalid>
The five SHAs added here are exactly those five, and the four named in the header comment are exactly the other four. Two routes to the same nine — the log parse whose %s template you correctly discounted, and the trailer field itself. No commit trailers is SUCCESS on this PR, so the gate agrees.
Strike the THE SOURCE IS CLOSED paragraph
no worktree config and no global config resolves \user.name` to `t` -- checked, not assumed`
The measurement is real and it is about the wrong object. t@example.invalid is this repository's own test-fixture identity — nine files carry it, every one a test suite:
.claude/hooks/block-dangerous-commands_test.sh scripts/lib/branch-guards_test.sh
autobot_shared/paths_test.py scripts/lib/session-handoffs_test.sh
pipeline-scripts/check_baseline_no_growth_test.py scripts/lint_conventions_test.py
repo_tests/hooks_path_override_15961_test.py scripts/research/safe_clone_test.py
repo_tests/stylelint_changed_files_skip_deletions_17571_test.py
Three of them write it into a repository's config rather than passing it per-invocation:
scripts/lib/branch-guards_test.sh:117 git -C "$TESTREPO" config user.email t@example.invalid
scripts/lib/session-handoffs_test.sh:88 git -C "$TMP_REPO" config user.email t@example.invalid
repo_tests/hooks_path_override_15961_test.py:479 _git("config", "user.email", "t@example.invalid", cwd=main)
Two things follow, and together they undo the paragraph:
- Worktrees share one config through the common dir, so a single such write with a mis-resolved repo path sets the identity for every worktree, which is the shape of five commits across five branches in one afternoon.
- A config inspection afterwards cannot see a write that was later corrected — which is precisely the failure the paragraph immediately below it describes, one level up. The previous instance's "since corrected" was true and insufficient; "no config resolves
ttoday" is true and insufficient in the same way.
This is #15353 (two shell test suites wrote git commits/config onto the real repo under an ambient GIT_DIR — no guard catches the class), open, with #16124 as the detection gap. So the class is not bounded at five and new ones will arrive. The sentence you already wrote — "What makes this bounded is the check, not the claim" — is the correct one; delete the claim it is contradicting and point at #15353 instead. Posted with the SHAs on #16268.
Holding the four Claude commits out, and saying in the header that a reader must not conclude they are baselinable, is right and is the part of this PR I would not change.
The verification in the body is the guard this file lacks
You verified by hand that each of the five carries the trailer, with the reason stated: "a SHA in a baseline that does not carry the trailer is an exemption for nothing, and it looks authoritative." Nothing in the tree checks that. stranded_recorder_entries_test.py:164-168 deliberately excludes this file — "neither entry is a path, so path-existence is the wrong staleness test" — which is correct about path-existence and leaves the file with no guard at all: its own header declares it "a RATCHET, not an allowlist" whose entries "may only be REMOVED", and that rule is a comment.
A SHA-specific check sidesteps the stated reason for the exclusion entirely: for each entry, (a) the SHA is reachable from release/main, and (b) its message carries a trailer the gate bans. (b) catches the exemption-for-nothing; (a) catches an entry stranded by a rewrite. Worth its own issue alongside the blind-spot one you already scoped under #13654 — the gate's BASE..HEAD scan cannot see the object the squash creates.
State
Green where it counts (No commit trailers, code-quality, smoke-test); 12 contexts still reporting, none red. Not approving — every session here authors as the repository owner, so GitHub refuses the approval and this is a comment review by necessity, not by reservation. On the header paragraph corrected in this PR rather than a follow-up, it is good to land.
…e class-1 cause (#17717) Two corrections to the first commit on this branch, both because the original was wrong about a fact it asserted. ALL NINE ARE NOW BASELINED, by explicit owner ruling. The first commit carried five and held four; the ruling reached both this session and the coordinator's and now agrees. The class-2 paragraph is unchanged in substance -- it still says a `Co-authored-by: Claude` trailer is the thing the gate exists to reject, is exempted only because `main` cannot be rewritten, and is NOT a precedent -- and it now also says its source is open, which makes it worse than class 1 rather than merely different. CLASS 1 WAS NOT A MISCONFIGURED USER IDENTITY. The first commit said the source was closed because no worktree config and no global config resolves `user.name` to `t`. That measurement was correct and about the wrong thing. `t <t@example.invalid>` is this repository's TEST-FIXTURE identity -- ten files carry it, eight run `git commit`, two of those under the pre-push hook. The reason it reaches the real repository is that an inherited `GIT_DIR` OUTRANKS `-C` and `cwd=`; `hooks_path_override_15961_test.py:450` records learning that the hard way. So no config override was ever there to find, and checking config is how you fail to find it. Three of the fixture files write the identity with `git config` rather than a per-invocation `-c`. Worktrees share one config through the common dir, so a single unscrubbed write sets it for every worktree at once -- which is the shape of five commits across five branches in one afternoon. WHAT I VERIFIED RATHER THAN RELAYED: every one of those sites currently scrubs. All eight committing tests pass `scrubbed_git_env()`, and all three config writers unset the ambient variables -- checked per file, not inferred from the helper existing. So no current test leaks, and the header says that instead of claiming the source is closed. The open window is a NEW fixture that commits or writes config, run before its scrub is added. That is what happened here: 969c195 is the commit that introduced one such test. Source is #15353, open; detection gap #16124; release-facing home #16268, which already carries 18 from an earlier promotion. So this baseline is the third containment of one root cause -- 22, then 18, now 9 -- and the header says so, because whoever picks up #15353 should be able to find the cost of not doing it in one place. Gate parse verified against the file as committed: 31 entries, all nine present. Refs #17717 Refs #13654 Refs #15353
1 participant
Thinking Path
#17717 promotes
main→releasefor v0.10.0 and failsNo commit trailerson commits already landed. Per-PR scans passed — including #17696's own — because the gate comparesBASE..HEADand the trailer does not exist until GitHub's squash-merge creates it. Only a promotion re-scans the landed range, so the check runs against an object no PR ever showed it. That is the same "correct for all N inputs, cannot say which" seam as the rest of today; here the input the check never saw is the merge commit.I derived the SHAs from the run log rather than taking a list, and the count needed resolving. The log has ten
Offending commit:lines and nine unique SHAs. The tenth is the workflow's own printf template echoed into the log:So nine real commits. A count taken from the line count would have been an artefact of the measurement, not the population — and both the coordinator and I had independently seen "9 against 10" without resolving it.
Then I checked each one's trailer, and they are two classes, not one:
What Changed
Only the five are baselined.
They are the existing
test <test@test>cause a second time: a localuser.name/user.emailoverride shadowed the correct global identity, branch commits carried it, and the squash stamped it onto the result.t <t@example.invalid>is not a person and claims no attribution.The source is closed — no worktree config and no global config resolves
user.nametot. Checked, not assumed, and the header says so for a specific reason: the previous instance's header stated the override was "since corrected", and it was, and a different one appeared anyway. The claim was true and insufficient. What bounds this class at five is the check, not the sentence.The four are held, and the header says why.
Co-authored-by: Claudeis a real attribution claim — precisely whatno-commit-trailers.yml(#13654) exists to reject — not a misconfigured identity. The reasoning that justifies the five does not reach it, so including it would exempt the thing the rule is for, under a ruling about something else. The header states explicitly that a reader must not conclude aClaudeco-author is baselinable.And its source is not closed, which is why it is a separate question rather than a longer list: the agent harness instructs sessions to append that trailer, while this project's rules forbid it and this gate enforces them. Until that conflict is resolved at the source, new instances keep landing and a baseline entry treats the symptom. Filed separately.
The top rule is narrowed, not suspended. It said entries may only be removed. It now names the one class an addition is permitted for — a trailer no branch commit carried, created by the squash, discovered only at promotion, on a history that cannot be rewritten — and says that a trailer which was on a branch commit does not qualify, because that one was fixable and the fix is to fix it.
Verification
The gate's own parse, run against the file as committed:
Exactly the five intended are exempt; the four named in the header comment are not — checked because their SHAs appear in prose in that file, and a
#-stripping parser that treated them as entries would silently grant the exemption this PR deliberately withholds.Each of the five verified with
git log -1 --format=%B <sha>to carryt@example.invalidbefore being added. A SHA in a baseline that does not carry the trailer is an exemption for nothing, and it looks authoritative.The workflow is untouched. The broader fix — exempting promotion PRs from the co-author arm — was considered and not taken here.
Not claimed
This does not fix the gate's blind spot.
No commit trailerswill keep passing per-PR and failing at promotion for any future squash-stamped trailer, because it scansBASE..HEADby design. That is worth its own issue against #13654 and is not something a baseline entry addresses.Refs #17717
Refs #13654
Model Used
Claude Opus 5 (1M context)