fix(#1184): strip Signed-off-by trailers instead of rejecting - #1185
Conversation
Change post-code.sh, post-fix.sh, and precommit-gate.lib.sh to strip Signed-off-by trailers from agent commits rather than discarding the entire run. Uses git commit --amend for single-commit and git filter-branch for multi-commit cases, failing only if the trailer persists after the rewrite attempt. Closes #1184 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🤖 Review · Commit: |
|
/fs-fix-stop |
|
Fix agent disabled for this PR. Remove the |
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
…e rewrite The strip added in f359597 rewrote every commit in SCAN_RANGE. On the post-fix rebase path DIFF_BASE widens to merge-base, so that range covers the whole PR branch, including commits a human authored and signed off. Reproduced end to end: a human opens a PR with a DCO-signed commit, /fs-fix runs, the branch is rebased, and the strip removes the human's trailer and force-pushes it. CONTRIBUTING.md requires that sign-off and the DCO app waives bot authors only, so this destroyed a legal attestation and turned the DCO check red on the contributor's own commit. Move the logic into shared helpers in precommit-gate.lib.sh, which all four call sites already source, so the copies cannot drift again: - Scope to agent-authored commits, matched on AUTHOR email. A rebase re-stamps the committer onto commits the human wrote, so committer would classify a bot-rebased human commit as the bot's and strip it. Author is also what the DCO app checks when it waives bots. - Rewrite only the contiguous run of agent commits at the tip. filter-branch re-creates every commit it is handed even through a cat filter, and commit-tree cannot reproduce gpgsig: a signed human commit inside the range lost its signature and changed SHA. An agent trailer below a human commit fails closed rather than rewriting past it. - Refuse to rewrite when the agent identity is unknown. Both forges export GIT_BOT_EMAIL to the runner, but an empty value made every commit look like the agent's, which is the one outcome the scoping exists to prevent. - Detect on %B, not %b. A trailer on line 2 with no blank line is folded into the subject, leaving %b empty, so the old detection shipped it unstripped. The strip sed skips line 1 so a message whose subject is the trailer keeps a subject. - git commit --amend --only, so a staged-but-uncommitted file cannot ride into the commit past the secret scan that already ran. Carry GIT_COMMITTER_* across the amend; --amend re-stamps the committer while filter-branch preserves it, and the two paths must not disagree. - Refresh the index and pre-check for unstaged changes and a dirty index, both of which filter-branch refuses. These previously surfaced as "Signed-off-by rejected" and discarded the run; they now fail under a new signoff-rewrite-failed category with a diagnostic naming the cause. - Guard the amend in precommit_run_gate. It was a bare command under set -euo pipefail, so a failure aborted the script and the rescue rescan that sets the category was unreachable. Surface the gate's category at both call sites instead of the literal signed-off-by. - Pass the bot address to the msg-filter through the environment; it contains "[bot]" and "+", and splicing it into a nested shell command was a quoting hazard. Record the strip where a human will see it: the PR body on creation, a comment on the existing-PR path (which exits before the body is assembled), and the post-fix summary comment via SIGNOFF_STRIPPED_COUNT, with a direct comment when that summary is skipped. Assisted-by: Claude (fix), Claude (review), Codex gpt-6-astra (review), Grok 4.6 (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
validate-code-output.src.sh is the validation_loop.script for both the code and fix harnesses, and it still hard-failed on a trailer with exit 1. That is the half of the #1184 cost the strip did not address: the issue's own evidence has the trailer surfacing on iteration 2, so each occurrence burned a retry, a full agent turn, for something the post-script now repairs. It also exits before the pre-commit gate, so that iteration's feedback carried only the sign-off complaint and the agent lost the lint output. Soft-pass instead, in the same bucket as secret-scan: not agent-fixable at this stage, handled terminally downstream. A rewrite here is pointless anyway; TARGET_REPO_DIR is an extracted copy and the PRECOMMIT_GATE_AUTOFIX contract forbids git writes in this mode. Detect on %B for the same folded-subject reason as the post-scripts, and drop signed-off-by from the agent-fixable case arm, which was unreachable and contradicted the header. Assisted-by: Claude (fix), Claude (review), Codex gpt-6-astra (review), Grok 4.6 (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
The cases added in f359597 asserted nothing about the rewrite. Both test files defined a local detect_signed_off_by() that grep'd a supplied string and echoed "stripped" or "blocked" based on a boolean the test itself passed in: no repository, no filter-branch, no amend. signoff-multi-commit-stripped passed a single string; signoff-rewrite-failure-blocked tested the helper's own parameter. Every one would have passed unchanged against the old rejecting code with the labels renamed. Replace them with scripts/signoff-strip-test.sh, which drives the real helpers against real git repositories and asserts: the trailer is gone and the rest of the message is intact; author and committer identity and dates survive on both the amend and the filter-branch path; commit count, order and subjects are unchanged and the range base keeps its SHA and tree; a human commit in range keeps its sign-off and SHA, including after the agent rebases it (committer re-stamped to the bot, author still human); a GPG-signed human commit below the agent tip keeps its gpgsig and SHA; an agent trailer below a human commit fails closed with nothing rewritten; a trailer folded into the subject is detected and stripped; staged content is not swept into the amended commit; unstaged changes, a dirty index and a missing agent identity each fail closed with a diagnostic naming the cause; a second pass is a no-op. Cover the strip note in process-fix-result: it renders with the right plural from SIGNOFF_STRIPPED_COUNT, is absent when unset, zero or not a number, and survives body truncation. The PR-body checkbox in post-code is not covered on purpose; post-code-test.sh's build_pr_body is a local reimplementation and a case there would test the copy. Remove the orphaned section headers and trailing blank lines the stub deletion left behind. Assisted-by: Claude (fix), Claude (review), Codex gpt-6-astra (review), Grok 4.6 (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
docs/code.md and docs/fix.md described the post-script pipeline but not what it does with a trailer, which changed from "discard the whole run" to "strip and continue". Readers hitting the new status note or the new Signed-off-by strip failed category had nothing to look it up in. Show the before/after of a real rewrite, including a human commit on the same branch keeping its trailer and its SHA, and state the rule that decides it (author, not committer). Record where the note appears on each path, and note that the validation loop soft-passes so a trailer no longer costs a retry iteration. The SKILL.md prohibition is unchanged. Assisted-by: Claude (fix), Claude (review), Codex gpt-6-astra (review), Grok 4.6 (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
One idea per comment; the narrative moves to the commit messages. The section headers f359597 wrote restated the DCO rationale at length at each call site, and the soft-pass and note comments repeated it. Comment-only; the generated bundles change to match. Assisted-by: Claude (fix), Claude (review), Codex gpt-6-astra (review), Grok 4.6 (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
cbd8c10 to
9dd355b
Compare
|
🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 3:27 AM UTC · Completed 4:09 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
waynesun09
left a comment
There was a problem hiding this comment.
Approved after takeover: rewrite scoped to the contiguous agent-authored tip, real-repository tests incl. GPG-signed and below-human cases, validator soft-pass, two squad rounds, CI green on 9dd355b.
|
🤖 Finished Retro · ✅ Success · Started 1:11 PM UTC · Completed 1:22 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.07 |
Retro: PR #1185 — Strip Signed-off-by trailers instead of rejectingTimeline
Key FindingsReview timeout wasted completed work. The review agent finished all its analytical work — the challenger returned adjudicated findings at ~17 minutes — but had insufficient time remaining to write the output file. The 20-minute budget killed a review that was substantively complete. This is additional evidence for #1177 (per-iteration 20-minute budget kills large-PR reviews and retries from scratch). This PR's 17 changed files and +1403 lines represent the kind of large, complex change that #1177 predicts will fail. Sonnet model alias resolution failed for review sub-agents. Five sub-agents ( Code agent quality was high. The code agent produced a thorough implementation with real-repository tests (54 assertions across 7 test groups), proper conventional commits, and an exceptionally detailed PR body documenting rationale, test evidence, and known limitations. The human took over for additional refinements and squad review. No agent-vs-human review delta possible. Because the review timed out before writing output, no autonomy-readiness comparison between agent and human review is available for this PR. Proposals filed
|
PR #1185 changed the post-script from rejecting agent commits with Signed-off-by trailers to stripping the trailer before pushing. Update the constraint text in both agent definitions to match the actual behavior: strips this trailer from agent commits before pushing instead of rejects commits that include this trailer. Note: pre-commit hooks were not run. pre-commit could not complete (infrastructure failure), and the direct-execution fallback was skipped. hack/lint-agent-docs and hack/lint-agent-docs-test.sh both pass. Addresses #1094 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Closes #1184
post-code.sh§3b,post-fix.sh§1b and the precommit-gate re-check treated aSigned-off-by:trailer on an agent commit as fatal: the run was discarded, nothing pushed. The trailer was the only thing wrong with the commits, and one such run cost $14.29 (fullsend-ai/fullsend#7031). This PR strips the trailer and continues, failing only if the rewrite itself cannot run or a trailer survives it.What changed
Scoped to agent-authored commits. On the post-fix rebase path
SCAN_RANGEwidens to merge-base and can cover a human's commits on the PR branch. A human's sign-off is a DCO attestation the DCO app requires (it waives bot authors only), so those commits pass through untouched — matched on author, because a rebase re-stamps the committer onto commits the human wrote.The rewrite preserves everything but the trailer. Author and committer identity and dates, commit count and order, the range base, and the rest of the message. Multi-commit ranges go through
git filter-branch --msg-filter; a single commit throughgit commit --amend --only, with the original committer carried across so the two paths agree.Detection on
%B. A trailer on line 2 with no blank line is folded into the subject and invisible to%b; the old check would have shipped it.Fails closed, under an honest category. Anything that stops the rewrite from running — unstaged changes, a dirty index (filter-branch refuses both), a missing agent identity — reports as Signed-off-by strip failed (
signoff-rewrite-failed). Signed-off-by rejected (signed-off-by) now means only that a trailer survived a rewrite that reported success.The validation loop no longer burns an iteration on it.
validate-code-output.shis thevalidation_loop.scriptfor both harnesses and still hard-failed on a trailer; that was the half of #1184's cost the strip alone did not address (the issue's evidence has the trailer surfacing on iteration 2). It soft-passes now, in the same bucket as secret-scan — a rewrite there is impossible anyway,TARGET_REPO_DIRis an extracted copy. This is a fourth site beyond the three the issue names; included because the issue's purpose is "stop discarding runs for the trailer", and this site discards iterations for it.Recorded where a human will see it. PR body on creation; a comment on the existing-PR path (which exits before the body is built); the post-fix summary comment.
The logic lives in three helpers in
scripts/lib/precommit-gate.lib.sh, which all four sites already source, so the copies cannot drift.The
SKILL.mdprohibition is unchanged — agents still must not sign off. The guard is the safety net, not the enforcement.Testing
All under podman (
debian:bookworm-slim+ git, jq, python3, python3-jsonschema) from the committed HEAD. macOS cannot run these suites — both die on a pre-existing BSD-sed incompatibility, identical onmain.The original PR's
run_signoff_testcases were string-matching stubs —detect_signed_off_bywas reimplemented in the test file and never touched git, sosignoff-multi-commit-strippedpassed a single string. They are replaced byscripts/signoff-strip-test.sh, which drives the real helpers against real repositories and asserts each of the following:gpgsigheader and SHA unchanged; bot trailers above it removed; count unchanged%b, detected, stripped, subject preservedGIT_BOT_EMAIL; human sign-off untouchedEnd-to-end shape of the rebase case, from a fixture through the real helpers (
123+fullsend-ai-coder[bot]@…is the agent):Before this scoping, the same fixture stripped the human's trailer and the push fell back to
--force-with-lease.Known limits
--author=<a human>and appends that human's sign-off is out of scope and the trailer ships; before this PR any trailer killed the run, which incidentally blocked that. Authorship forgery itself is pre-existing and the SKILL.md prohibition plus human merge review remain the controls. Provenance scoping ("commits this run created") is not available on the rebase path, which is exactly where the widening happens.post-code-test.sh'sbuild_pr_bodyis a local reimplementation, and a case there would test the copy rather than the script.signed-off-by:is intentionally not detected (pre-existing, case-sensitive contract). Under the new semantics that means it ships and fails gitlint rather than being rejected.filter-branchleaves a trailing blank line that--amend -F(--cleanup=whitespace) does not; harmless for gitlint.refs/original/is left behind byfilter-branch -f. Nothing in either script traverses all refs and both pushes are single-refspec, so it stays local to the ephemeral runner.