fix(#1138): scope sign-off scan to agent changes

[shairevivo]

Summary

• scope validation-loop sign-off checks to commits introduced or rewritten after PRE_AGENT_HEAD
• preserve existing human DCO trailers across ordinary rebases and --rebase-merges
• reject sign-offs added by agents, including amend-only and merge-commit cases

Testing

• uv run --with jsonschema bash scripts/validate-code-output-test.sh
• bundled validate-code-output-test.sh suite
• make check-bundle
• pre-commit run --all-files
• make lint

The full make script-test suite was also attempted. It reached the unrelated Jira harness tests, which require JIRA_TOKEN and JIRA_BASE_URL fixtures that are unavailable locally.

Closes #1138

[github-actions]

Functional tests did not run

Functional tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

[qodo-code-review]

PR Summary by Qodo

Scope sign-off validation to agent-authored changes

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Restricts sign-off validation to commits introduced or rewritten during fix runs.
• Preserves matching human DCO trailers across standard and merge-preserving rebases.
• Rejects agent-created sign-offs on new, amended, and merge commits.

Diagram

graph TD
  A["Fix validation"] --> B["Changed commits"] --> C{"Has sign-off?"}
  C -->|No| D["Continue checks"]
  C -->|Yes| E["Signed identity"] --> G{"Identity matches?"}
  F["Pre-agent identities"] --> G
  G -->|Yes| D
  G -->|No| H["Reject commit"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use git cherry equivalence

• ➕ Uses Git's built-in patch-equivalence comparison
• ➕ Avoids maintaining a custom commit identity function
• ➖ Omits merge commits
• ➖ Does not verify that sign-off trailers are unchanged
• ➖ Cannot detect all amend-only attribution changes

2. Allow pre-agent commit SHAs

• ➕ Simple and inexpensive comparison
• ➕ Precisely identifies untouched commits
• ➖ Commit SHAs change during ordinary rebases
• ➖ Cannot preserve legitimate trailers through rebase-merges
• ➖ Would retain the false positives addressed by this PR

Recommendation: Keep the patch-and-trailer identity approach. It survives rebases, distinguishes merge commits, and verifies exact sign-off preservation; SHA allowlists fail after history rewriting, while git cherry lacks merge and trailer coverage.

Files changed (3) +222 / -8

Bug fix (2) +114 / -8

validate-code-output.shBundle agent-scoped sign-off validation +57/-4

Bundle agent-scoped sign-off validation

• Updates the generated validator to select post-agent commits using PRE_AGENT_HEAD and compare signed commits against pre-agent identities. Identities combine commit kind, stable patch identity, and normalized sign-off trailers so equivalent rebased human commits remain allowed.

scripts/validate-code-output.sh

validate-code-output.src.shScope sign-off checks using pre-agent commit identities +57/-4

Scope sign-off checks using pre-agent commit identities

• Implements PRE_AGENT_HEAD-aware commit selection and signed-commit identity matching in the validator source. It preserves equivalent human sign-offs across rebases while rejecting new, amended, or merge-commit sign-offs introduced by agents.

scripts/validate-code-output.src.sh

Tests (1) +108 / -0

validate-code-output-test.shCover human and agent sign-off attribution scenarios +108/-0

Cover human and agent sign-off attribution scenarios

• Adds a repository-based test harness for signed human commits, standard rebases, merge-preserving rebases, agent sign-offs, amended commits, and signed agent merges. The cases verify preserved human DCO trailers pass while agent-added trailers fail validation.

scripts/validate-code-output-test.sh

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (1)

1. Whitespace changes evade attribution ✓ Resolved 🐞 Bug ≡ Correctness

Description

signed_commit_identity uses whitespace-insensitive stable patch IDs, so an agent can amend a
signed human commit with whitespace-only changes—including semantically significant indentation—and
retain the same accepted identity. The rewritten signed commit is then incorrectly treated as an
unchanged pre-agent patch.

Code

scripts/validate-code-output.src.sh[R161-162]

+      patch_id="$(git diff "${commit_sha}^1" "${commit_sha}" \
+      | git patch-id --stable | awk '{ print $1 }')"

Relevance

●●● Strong

Recent validator reviews accepted closely matching attribution hardening for rebase and
Signed-off-by correctness.

PR-#1068
PR-#573

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Both pre-agent and post-agent identities derive their patch component through `git patch-id
--stable`, and the validator allows a candidate solely when that normalized identity appears in the
baseline set. Stable patch IDs ignore whitespace differences, so the equality check cannot
distinguish a replay from a whitespace-modified signed commit.

scripts/validate-code-output.src.sh[151-170]
scripts/validate-code-output.src.sh[173-193]
scripts/validate-code-output.sh[901-920]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The sign-off attribution identity uses `git patch-id --stable`, which normalizes whitespace. A rewritten signed commit containing agent-authored whitespace or indentation changes can therefore match the pre-agent identity and bypass rejection.

## Issue Context
The validator must preserve genuinely replayed human commits while rejecting signed commits whose patch was modified by the agent. Add regression coverage using a signed pre-agent commit amended with whitespace-significant changes.

## Fix Focus Areas
- scripts/validate-code-output.src.sh[157-170]
- scripts/validate-code-output.sh[907-920]
- scripts/validate-code-output-test.sh[366-469]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Trailer mutations evade attribution ✓ Resolved 🐞 Bug ≡ Correctness

Description

Hashing Signed-off-by lines after sort -u discards their order and multiplicity, so an agent can
append a duplicate existing sign-off or reorder trailers without changing the accepted identity.
This permits a post-agent commit whose exact trailer list differs from the pre-agent commit,
contrary to the attribution rule.

Code

scripts/validate-code-output.src.sh[R168-169]

+    signoff_id="$(git show -s --format='%b' "${commit_sha}" \
+      | grep '^Signed-off-by:' | sort -u | git hash-object --stdin)"

Relevance

●●● Strong

Recent validator reviews accepted narrowly scoped correctness fixes preventing attribution and
trailer-validation bypasses.

PR-#1068
PR-#573

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The identity hashes the output only after sort -u, which explicitly removes duplicate trailers and
canonicalizes their order. The subsequent exact identity lookup therefore accepts commits whose raw
sign-off trailer sequences differ from the pre-agent versions.

scripts/validate-code-output.src.sh[168-170]
scripts/validate-code-output.src.sh[173-193]
scripts/validate-code-output.sh[918-943]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The sign-off identity sorts and deduplicates trailer lines before hashing. This makes commits with reordered or duplicated `Signed-off-by` trailers indistinguishable and allows agent-introduced trailer mutations to pass as pre-existing human sign-offs.

## Issue Context
Hash the exact extracted trailer sequence, including order and duplicates. Add tests where the agent appends an identical existing trailer and reorders multiple existing sign-offs.

## Fix Focus Areas
- scripts/validate-code-output.src.sh[168-170]
- scripts/validate-code-output.sh[918-920]
- scripts/validate-code-output-test.sh[366-469]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Protected scripts require human approval 📜 Skill insight § Compliance

Description

This PR modifies protected scripts/ files, including the sign-off validation guard and its tests.
Although issue #1138 provides justification, these governance changes require human approval and
must not be auto-approved.

Code

scripts/validate-code-output.src.sh[R150-152]

+if [ -n "${PRE_AGENT_HEAD:-}" ]; then
+  signed_commit_identity() {
+    local commit_sha="$1" parent_count patch_id signoff_id kind="commit"

Relevance

●●● Strong

Protected-path governance findings are accepted for scripts changes, including recent enforcement
precedents.

PR-#569

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1538392 designates every file under scripts/ as protected and requires a finding
whenever such files are modified. The cited files show changes to the source validator, generated
validator, and validation tests; the linked issue justifies the changes but does not remove the
human-approval requirement.

scripts/validate-code-output.src.sh[150-152]
scripts/validate-code-output.sh[900-902]
scripts/validate-code-output-test.sh[366-370]
Skill: pr-review

---

Context sources

✅ Compliance rules (platform): 57 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

Review mode: ⚖️ Balanced

Tip of the day

💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[rh-hemartin]

/ok-to-test

[waynesun09]

HIGH: post-fix.src.sh (the authoritative push-time gate) is not updated — still hard-rejects legitimately-signed human commits replayed by a rebase

scripts/post-fix.src.sh line 197 (not part of this PR's diff)

This PR's summary claims to "preserve existing human DCO trailers across ordinary rebases and --rebase-merges" and adds tests proving that for the in-loop check in scripts/validate-code-output.src.sh. But scripts/post-fix.src.sh — the authoritative script that actually pushes, run after the validation loop — is untouched by this PR (confirmed via the PR's changed-files list: only validate-code-output.sh/.src.sh and validate-code-output-test.sh are modified).

At the PR head commit, scripts/post-fix.src.sh still has: when PRE_AGENT_HEAD is no longer an ancestor of HEAD (rebase detected, ~lines 189-206), DIFF_BASE falls back to merge-base(HEAD, origin/TARGET_BRANCH), and SCAN_RANGE="${DIFF_BASE}..HEAD" (~line 251) is scanned at ~line 268 with a plain git log --format='%b' "${SCAN_RANGE}" | grep -q '^Signed-off-by:' — no patch-id/signoff-identity allowlist, no distinction between agent-added and pre-existing human trailers.

Because a rebase replays pre-existing signed human commits into the merge-base..HEAD range, a fix run on a rebased, DCO-signed PR branch will now PASS the new in-loop validate-code-output.sh check, then hard-fail at push time via post_fail_to_pr signed-off-by "..." with the exact same false-positive message #1138 was filed to eliminate — except now the failure is terminal (blocks the push entirely) rather than a retryable in-loop iteration, and is harder to diagnose since the loop appeared to pass.

Issue #1138's own validation criteria are scoped to validate-code-output.sh's in-loop behavior (and assume post-fix.sh "already correctly scopes" to PRE_AGENT_HEAD), so this PR does satisfy #1138's literal text — but it does not satisfy the PR's own broader stated claim of preserving human DCO trailers across rebases end-to-end, and leaves a real, user-facing regression: a case that now looks like it passed can still be rejected at the final, non-retryable gate.

Suggestion: Port the same signed_commit_identity/patch-id+trailer allowlist logic into post-fix.src.sh's Signed-off-by check (ideally factored into a shared lib function both scripts call), or explicitly narrow this PR's summary to state that rebase-preservation is only guaranteed for the in-loop check, with a tracking issue filed for post-fix.src.sh before treating the rebase case as fully solved.

[shairevivo]

Closing as superseded by #1185. That merged change solves the same false-positive end to end: the validation loop soft-passes sign-off trailers, while the post-scripts strip trailers only from agent-authored commits and preserve human DCO trailers across rebases. It also addresses the outstanding post-fix review concern; resolving this conflict by retaining the older identity-allowlist logic would reintroduce obsolete hard-rejection behavior.

[fullsend-ai-retro]

🤖 Retro · Started 11:42 AM UTC

Commit: f00df8c · View workflow run →