Skip to content

feat(#6002): add --signoff flag to github setup command - #6004

Merged
maruiz93 merged 7 commits into
mainfrom
agent/6002-signoff-flag
Aug 13, 2026
Merged

feat(#6002): add --signoff flag to github setup command#6004
maruiz93 merged 7 commits into
mainfrom
agent/6002-signoff-flag

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

  • Add --signoff flag to fullsend github setup that appends a Signed-off-by trailer to scaffold commits, enabling compatibility with repos that enforce DCO checks
  • When --signoff is set, the user's identity (name + email) is resolved via the forge API; if unavailable (e.g. bot tokens), the command errors with a clear message
  • Without --signoff (default), behavior is unchanged — no trailer is appended, keeping autonomous agent commits exempt per project DCO policy

Context

Repos like kubearchive/kubearchive enforce DCO via a Probot app. When fullsend github setup creates scaffold commits without a Signed-off-by trailer, the DCO check fails and maintainers must manually amend and force-push to add the trailer. This was reported in kubearchive/kubearchive PRs #2001 and #2021.

Testing

  • TestRunGitHubSetupPerRepo_SignoffAddsTrailer — verifies the trailer is present in the commit message when --signoff is set
  • TestRunGitHubSetupPerRepo_WithoutSignoffOmitsTrailer — verifies no trailer when --signoff is omitted
  • TestRunGitHubSetupPerRepo_SignoffMissingIdentity — verifies error when identity cannot be resolved (bot token)
  • TestRunGitHubSetupPerRepo_SignoffDirect — verifies trailer works in --direct mode
  • TestApplyPerRepoScaffold_WithSignOff / _WithoutSignOff — unit tests for the applyPerRepoScaffold function
  • All existing applyPerRepoScaffold tests updated and passing

Closes #6002

Post-script verification

  • Branch is not main/master (agent/6002-signoff-flag)
  • Secret scan passed (gitleaks — 87fd13d4b2ee4c004303313c02526713ce7bc847..HEAD)
  • PR body secret scan passed (gitleaks — no-git)

@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner August 7, 2026 13:09
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Aug 7, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 7, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:10 PM UTC · Completed 1:27 PM UTC

Commit: 44ab7c2 · View workflow run →

@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.87879% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/cli/github.go 81.25% 1 Missing and 2 partials ⚠️
internal/cli/admin.go 83.33% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [naming-convention] internal/cli/github.go:74 — The struct field is named signoff (no camel case) while every other multi-word field in githubSetupConfig uses camelCase (skipAppSetup, dryRun, enrollAll, enrollNone, publicApps, fullsendBinary, fullsendSource, configPreset, configHash). The companion struct scaffoldOptions in admin.go also uses camelCase (signOffTrailer).
    Remediation: Rename the field from signoff to signOff and update all references.

  • [missing-flag-documentation] docs/cli/github.md:58 — The new --signoff flag added to fullsend github setup is missing from the flags table in the CLI reference documentation. The table lists all other setup flags (--direct, --runtime, --config, --config-hash) but omits --signoff. Users consulting the CLI reference will not discover this flag exists.
    Remediation: Add a row to the flags table: | --signoff | false | Add Signed-off-by trailer to scaffold commits (requires GitHub user identity; per-repo only) |

Low

  • [error-handling-pattern] internal/layers/workflows.go:69 — The refactored WithSignOff method silently ignores errors from forge.FormatSignOffTrailer. The code comment documents this as intentional for best-effort paths, and the new --signoff flag path is properly fail-closed. The risk is marginal since callers already guard for empty inputs and the only new failure mode requires adversarial input.

  • [intent-scope-mismatch] internal/cli/github.go:194 — The PR title uses feat(#6002) but issue github setup scaffold commits lack DCO sign-off, breaking repos that enforce DCO #6002 is labeled bug. COMMITS.md explicitly lists "A new CLI command or flag they can invoke" as valid for feat, and the PR adds an opt-in --signoff flag rather than changing default behavior. Both feat and fix are defensible.

Previous run

Review

Findings

Medium

  • [missing-flag-documentation] docs/cli/github.md:58 — The new --signoff flag added to fullsend github setup is missing from the flags table in the CLI reference documentation. The table lists all other setup flags (--direct, --runtime, --config, --config-hash) but omits --signoff. Users consulting the CLI reference will not discover this flag exists.
    Remediation: Add a row to the flags table: | --signoff | false | Add Signed-off-by trailer to scaffold commits (requires GitHub user identity; per-repo only) |

  • [naming-convention] internal/cli/github.go:74 — The struct field is named signoff (no camel case) while other multi-word bool fields in the same struct use camelCase (skipAppSetup, dryRun, enrollAll, enrollNone, publicApps). The options struct in admin.go also uses signOffTrailer (camelCase).
    Remediation: Rename the field from signoff to signOff in githubSetupConfig and update all references.

  • [intent-scope-mismatch] internal/cli/github.go:194 — The PR title uses feat(#6002) but issue github setup scaffold commits lack DCO sign-off, breaking repos that enforce DCO #6002 is labeled bug and titled "github setup scaffold commits lack DCO sign-off, breaking repos that enforce DCO". Per COMMITS.md, feat populates the Features section of release notes. A flag that makes an existing command work in environments where it currently fails is better classified as fix.
    Remediation: Consider changing the PR title to fix(#6002): add --signoff flag to github setup command.

  • [error-handling-pattern] internal/layers/workflows.go:69 — The refactored WithSignOff method silently ignores errors from forge.FormatSignOffTrailer. The original code used fmt.Sprintf which could not fail; now that validation exists via FormatSignOffTrailer, errors are swallowed. The code comment documents this as intentional for best-effort paths, and the new --signoff flag path is properly fail-closed.
    Remediation: Consider documenting the design rationale more prominently, or changing the signature to return an error.

Low

  • [edge-case] internal/forge/forge.go:265FormatSignOffTrailer does not trim leading/trailing whitespace from name or email. A whitespace-only name (e.g., " ") passes both the pre-check in runGitHubSetupPerRepo (non-empty) and the post-sanitization check (non-empty), producing a malformed trailer. While GitHub profiles are unlikely to have whitespace-only names, adding a strings.TrimSpace call would be more robust.
Previous run (2)

Review

Findings

Medium

  • [missing-flag-documentation] docs/cli/github.md:58 — The new --signoff flag added to fullsend github setup is missing from the flags table in the CLI reference documentation. The table lists all other setup flags (--direct, --runtime, --config, --config-hash) but omits --signoff. Multiple guide pages link to this flags table as the authoritative reference.
    Remediation: Add a row to the flags table: | --signoff | false | Add Signed-off-by trailer to scaffold commits (requires GitHub user identity; per-repo only) |

  • [naming-convention] internal/cli/github.go:71 — The struct field is named signoff (no camel case) while other multi-word bool fields in the same struct use camelCase (dryRun, configHash, enrollAll, enrollNone, skipAppSetup, publicApps). The options struct in admin.go also uses signOffTrailer (camelCase).
    Remediation: Rename the field from signoff to signOff in githubSetupConfig and update all references.

  • [intent-scope mismatch] internal/cli/github.go:194 — The PR title uses feat(#6002) but issue github setup scaffold commits lack DCO sign-off, breaking repos that enforce DCO #6002 is labeled bug and titled "github setup scaffold commits lack DCO sign-off, breaking repos that enforce DCO". Per COMMITS.md, feat populates the Features section of release notes. A flag that makes an existing command work in environments where it currently fails is better classified as fix.
    Remediation: Consider changing the PR title to fix(#6002): add --signoff flag to github setup command.

Previous run (3)

Review

Findings

Medium

  • [missing-flag-documentation] docs/cli/github.md:58 — The new --signoff flag added to fullsend github setup is missing from the flags table in the CLI reference documentation. The table lists all other setup flags (--direct, --runtime, --config, --config-hash) but omits --signoff. Multiple guide pages link to this flags table as the authoritative reference.
    Remediation: Add a row to the flags table: | --signoff | false | Add Signed-off-by trailer to scaffold commits (requires GitHub user identity; per-repo only) |

Low

  • [silent-failure] internal/layers/workflows.go:68WithSignOff silently swallows errors from FormatSignOffTrailer. If a name consists entirely of stripped characters (e.g., <>), it passes the name != "" pre-check but FormatSignOffTrailer returns an error after sanitization empties it, and the trailer is silently omitted. The behavior is explicitly documented as intentional ("callers in best-effort paths already guard for this"), and the triggering condition is unrealistic for GitHub identities. This also creates a minor behavioral inconsistency: fullsend github setup --signoff hard-fails on invalid identity, while sync-scaffold (via WithSignOff) silently degrades — though this difference is by design (explicit opt-in vs. best-effort).
Previous run (4)

Review

Findings

Medium

  • [missing-flag-documentation] docs/cli/github.md:58 — The new --signoff flag added to fullsend github setup is missing from the flags table in the CLI reference documentation. The table lists all other setup flags (--direct, --runtime, --config, --config-hash) but omits --signoff. Multiple guide pages link to this flags table as the authoritative reference.
    Remediation: Add a row to the flags table: | --signoff | false | Add Signed-off-by trailer to scaffold commits (requires GitHub user identity; per-repo only) |

Low

  • [edge-case] internal/cli/github.go:422 — The emptiness checks (id.Name == "" and id.Email == "") run on raw values from GetAuthenticatedUserIdentity, but FormatSignOffTrailer then strips \n, \r, <, and >. A display name consisting entirely of stripped characters (e.g., "<>") would pass the guard but produce an error from FormatSignOffTrailer, which is correctly caught and propagated. The redundant guard is harmless but slightly misleading. Practical risk is minimal.

  • [architectural-inconsistency] internal/cli/github.go — The PR adds a --signoff flag to github setup (per-repo install), but the existing per-org setup already applies sign-off automatically via WithSignOff() when a user identity is available (admin.go:2093). This creates an inconsistent UX: per-org setup auto-signs, per-repo setup requires an explicit flag. The code explicitly documents this design choice, and the issue authorized either approach.

  • [type-mismatch] PR title — PR title uses feat(#6002) but issue github setup scaffold commits lack DCO sign-off, breaking repos that enforce DCO #6002 is labeled bug. Per COMMITS.md, GoReleaser uses PR titles to build release notes. However, COMMITS.md also lists "a new CLI flag" as valid feat usage, so the classification is defensible.

  • [naming-convention] internal/cli/admin.go:1213 — The new scaffoldOptions struct uses the Options suffix, but the internal/cli package consistently uses Config or Opts for similar structs. The struct serves a different purpose (internal function parameter) than command-level Config or Opts patterns.

Previous run (5)

Review

Findings

Medium

  • [missing-flag-documentation] docs/cli/github.md:58 — The new --signoff flag added to fullsend github setup is missing from the flags table in the CLI reference documentation. The table lists all other setup flags (--direct, --runtime, --config, --config-hash) but omits --signoff. Multiple guide pages link to this flags table as the authoritative reference.
    Remediation: Add a row to the flags table: | --signoff | false | Add Signed-off-by trailer to scaffold commits (requires GitHub user identity; per-repo only) |

Low

  • [edge-case] internal/cli/github.go:347 — The emptiness checks (id.Name == "" and id.Email == "") run on raw values from GetAuthenticatedUserIdentity, but FormatSignOffTrailer then strips \n, \r, <, and >. A display name consisting entirely of stripped characters (e.g., <>) would pass the guard but produce a malformed trailer with an empty name field. Practical risk is minimal — GitHub profiles are unlikely to have such names.

  • [type-mismatch] PR title — The PR title uses feat(#6002) but issue github setup scaffold commits lack DCO sign-off, breaking repos that enforce DCO #6002 is labeled bug. Per COMMITS.md, GoReleaser uses PR titles to build release notes. However, COMMITS.md also lists "a new CLI flag" as valid feat usage, so the classification is defensible.
    Remediation: No action required if the team considers the new flag the primary deliverable; change to fix(#6002) if the bug fix is the primary intent.

Previous run (6)

Review

Findings

Medium

  • [missing-flag-documentation] docs/cli/github.md:58 — The new --signoff flag added to fullsend github setup is missing from the flags table in the CLI reference documentation. The table lists all other setup flags (--direct, --runtime, --config, --config-hash) but omits --signoff. Multiple guide pages link to this flags table as the authoritative reference.
    Remediation: Add a row to the flags table: | --signoff | false | Add Signed-off-by trailer to scaffold commits (requires GitHub user identity; per-repo only) |

Low

  • [edge-case] internal/cli/github.go:362 — When --dry-run and --signoff are used together, the identity resolution block is never executed because dry-run returns early. A user running fullsend github setup owner/repo --signoff --dry-run will see a clean dry-run even if their token lacks the ability to resolve identity (e.g., a GitHub App token). They will only discover the failure when they remove --dry-run.
    Remediation: Consider moving the signoff identity resolution before the dry-run early return, or adding a dry-run output line to signal the intent.

  • [type-mismatch] PR title — The PR title uses feat(#6002) but issue github setup scaffold commits lack DCO sign-off, breaking repos that enforce DCO #6002 is labeled bug. Per COMMITS.md, GoReleaser uses PR titles to build release notes. However, COMMITS.md also lists "a new CLI flag" as valid feat usage, so the classification is defensible.
    Remediation: No action required if the team considers the new flag the primary deliverable; change to fix(#6002) if the bug fix is the primary intent.

Previous run (7)

Review

Findings

Medium

  • [type-mismatch] PR title — The PR title uses feat(#6002) but issue github setup scaffold commits lack DCO sign-off, breaking repos that enforce DCO #6002 is labeled bug and describes existing functionality failing (scaffold commits rejected by DCO-enforcing repos). Per COMMITS.md, GoReleaser uses PR titles to build release notes — a feat prefix places this under "Features" instead of "Bug Fixes." However, COMMITS.md also lists "a new CLI flag" as valid feat usage, so there is legitimate ambiguity. If fixing the bug is the primary intent, fix is more accurate.
    Remediation: Change the PR title from feat(#6002) to fix(#6002), or keep feat and update the issue label if the new flag is the primary deliverable.

  • [missing-flag-documentation] docs/cli/github.md:58 — The new --signoff flag added to fullsend github setup is missing from the flags table in the CLI reference documentation. The table lists all other setup flags (--direct, --runtime, --config, --config-hash) but omits --signoff. Multiple guide pages link to this flags table as the authoritative reference.
    Remediation: Add a row to the flags table: | --signoff | false | Add Signed-off-by trailer to scaffold commits (requires GitHub user identity; per-repo only) |

Low

  • [naming-consistency] internal/cli/admin.go — The new scaffoldOptions struct uses the Options suffix, which is not used by any other struct in internal/cli. The package consistently uses Config (e.g., perRepoInstallConfig, githubSetupConfig) or Opts (e.g., statusOpts, dispatchOpts).
    Remediation: Rename to scaffoldConfig or scaffoldOpts to match existing conventions.

  • [file-organization] internal/forge/forge.goFormatSignOffTrailer is added to forge.go (which contains sentinel errors, data types, and the Client interface), while its tests live in a new signoff_test.go. The forge package organizes helpers into purpose-named files (detect.go/detect_test.go, url.go/url_test.go). Note: SignOffTrailer() is a method on UserIdentity defined in forge.go, so moving the method splits the type from its method — a trade-off.
    Remediation: Consider moving FormatSignOffTrailer into a new signoff.go file to match signoff_test.go.

Previous run (8)

Review

Findings

Medium

  • [type-mismatch] PR title — The PR title uses feat(#6002) but issue github setup scaffold commits lack DCO sign-off, breaking repos that enforce DCO #6002 is labeled bug and describes existing functionality failing (scaffold commits rejected by DCO-enforcing repos). Per COMMITS.md, GoReleaser uses PR titles to build release notes — a feat prefix places this under "Features" instead of "Bug Fixes."
    Remediation: Change the PR title from feat(#6002) to fix(#6002).

  • [missing-flag-documentation] docs/cli/github.md:58 — The new --signoff flag added to fullsend github setup is missing from the flags table in the CLI reference documentation. The table lists all other setup flags (--direct, --runtime, --config, --config-hash) but omits --signoff.
    Remediation: Add a row to the flags table: | --signoff | false | Add Signed-off-by trailer to scaffold commits (requires GitHub user identity) |

Low

  • [sanitization-gap] internal/forge/forge.go:266FormatSignOffTrailer strips angle brackets from the name field but not from the email field. If the email contained >, the trailer would be prematurely closed (e.g., Signed-off-by: Name <evil>@example.com>). In practice, email addresses come from GitHub/GitLab API responses which validate email format, so the residual risk is negligible, but the asymmetric sanitization is inconsistent.
    Remediation: Add > to the email replacer for consistency.

  • [naming-consistency] internal/forge/forge.goFormatSignOffTrailer uses an intermediate strings.NewReplacer assigned to r that is only used once, while a second replacer is constructed and called inline. Inconsistent style within the same function.
    Remediation: Either name both replacers descriptively or inline both.

Previous run (9)

Review

Findings

Medium

  • [missing-flag-documentation] docs/cli/github.md:58 — The new --signoff flag added to fullsend github setup is missing from the flags table in the CLI reference documentation. The table lists all other setup flags (--direct, --runtime, --config, --config-hash) but omits --signoff. Users consulting this reference would not know the flag exists.
    Remediation: Add a row to the flags table: | --signoff | false | Add Signed-off-by trailer to scaffold commits (requires GitHub user identity) |

Low

  • [injection/incomplete-sanitization] internal/forge/forge.go:268FormatSignOffTrailer strips \n and \r but does not strip the > character from the name field. A GitHub user who sets their display name to contain > (e.g., Foo> <evil@attacker.com) could produce a malformed trailer. This does not escalate privileges since the commit is authored by the authenticated user via the forge API, but could produce a misleading Signed-off-by line in the git log.
    Remediation: Consider stripping or escaping < and > from the name field, or validating that name does not contain angle brackets before formatting the trailer.

  • [missing-context-documentation] docs/guides/getting-started/configuring-github.md:91 — The getting-started guide links to the CLI reference for setup flags but does not mention the --signoff flag. For repos enforcing DCO, this is the primary entry point — a brief note about the flag would help discoverability.
    Remediation: Add a brief note about the --signoff flag for DCO compliance.

Previous run (10)

Review

Findings

Medium

  • [missing-flag-documentation] docs/cli/github.md:58 — The new --signoff flag added to fullsend github setup is missing from the flags table. The table lists all other setup flags (--direct, --runtime, --config, --config-hash) but omits --signoff. Readers consulting this reference would not know the flag exists.
    Remediation: Add a row to the flags table: | --signoff | false | Add Signed-off-by trailer to scaffold commits (requires git user identity) |

  • [architectural-coherence] internal/cli/github.go:362 — The github setup command uses an explicit opt-in --signoff flag with hard-failure when identity is unavailable, while github sync-scaffold (github.go:1094) automatically attempts sign-off and degrades gracefully when identity cannot be resolved. This creates inconsistent behavior across CLI commands for the same sign-off concern. The opt-in design is a valid choice (backward-compatible, explicit), but the divergence from the existing auto-sign pattern should be intentional and documented.
    Remediation: Consider aligning with the graceful-degradation pattern used in sync-scaffold, or add a code comment explaining why opt-in with hard-failure is preferred for setup.

  • [scope-authorization] PR title — The PR title uses the feat prefix (feat(#6002): add --signoff flag), but issue github setup scaffold commits lack DCO sign-off, breaking repos that enforce DCO #6002 is labeled bug and describes a functional gap ("scaffold commits lack DCO sign-off, breaking repos that enforce DCO"). Per COMMITS.md, GoReleaser uses PR titles to build release notes — a fix prefix would categorize this correctly as a bug fix rather than a new feature.
    Remediation: Consider changing the PR title prefix from feat to fix.

Low

  • [error-handling] internal/cli/github.go:367 — The error message --signoff requires git user identity (name and email) says "git user identity" but the identity is resolved via the GitHub API (GetAuthenticatedUserIdentity calls GET /user), not from local git config. A user with local git config but a GitHub App token would find this guidance misleading.
    Remediation: Consider rewording to: --signoff requires a GitHub user identity (name and email) — this is not available for GitHub App tokens.

  • [naming-convention] internal/cli/github.go:69 — The new struct field signoff uses single-word naming, while the parameter signOffTrailer in the same change uses camelCase. "Signoff" as a single compound word (like git --signoff) is defensible, but Go convention would favor signOff for a two-word concept.

  • [missing-context-documentation] docs/guides/getting-started/configuring-github.md:91 — The getting-started guide links to the CLI reference for setup flags but does not mention --signoff. For repos enforcing DCO, this is the primary entry point — a brief note about the flag would help discoverability.


Labels: PR modifies CLI install/setup code in internal/cli/ and adds a new flag to the github setup command

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment component/install CLI install and app setup go Pull requests that update go code labels Aug 7, 2026
Comment thread internal/cli/github.go Outdated
Comment thread internal/cli/admin.go Outdated
Comment thread internal/cli/github.go Outdated
Comment thread internal/cli/github.go Outdated
Comment thread internal/cli/github.go Outdated
Comment thread internal/cli/github_test.go Outdated
@maruiz93

Copy link
Copy Markdown
Contributor

/fs-fix Rebase onto main and address all review comments on this PR.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 9:33 AM UTC · Completed 9:50 AM UTC

Commit: 44ab7c2 · View workflow run →

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (human-triggered)

Addressed 8 of 9 review findings with code fixes. Rebased onto main. Disagreed with 1 (naming convention: signoff is intentionally single-word to match git --signoff). Key changes: extracted scaffoldOptions struct, added org-target validation for --signoff, added empty name/email guard and newline stripping, extracted shared FormatSignOffTrailer helper, rewrote error messages, added architectural comment, and extracted test helper.

Fixed (8):

  1. --signoff flag silently accepted for org targets (internal/cli/github.go): Added 'signoff' to the per-repo-only flag validation slice at line 112, matching the pattern used for --config and --config-hash
  2. applyPerRepoScaffold has 10 positional parameters (internal/cli/admin.go): Extracted scaffoldOptions struct for the optional behavioral modifiers (direct, signOffTrailer), reducing parameter count and improving call-site readability. Updated all 21 call sites across admin.go, admin_test.go, and github.go
  3. id.Name and id.Email not checked for empty values (internal/cli/github.go): Added guard: if id.Name == '' || id.Email == '' return error with descriptive message showing the actual values. Added TestRunGitHubSetupPerRepo_SignoffEmptyIdentityFields test
  4. id.Name and id.Email not stripped of newline characters (internal/forge/forge.go): FormatSignOffTrailer strips \n and \r from both name and email fields using strings.NewReplacer before formatting. Added TestFormatSignOffTrailer_StripsNewlines test
  5. Trailer formatting duplicated between github.go and WorkflowsLayer (internal/forge/forge.go): Extracted shared FormatSignOffTrailer function and UserIdentity.SignOffTrailer method in the forge package. Updated both github.go (id.SignOffTrailer()) and workflows.go (forge.FormatSignOffTrailer) to use the shared helper
  6. Four Signoff tests duplicate ~15 lines of setup each (internal/cli/github_test.go): Extracted newSignoffTestSetup(t) helper that returns a pre-configured fake client and base config. All 4 signoff tests now override only the fields that differ
  7. Error message says 'git user identity' but resolved via GitHub API (internal/cli/github.go): Reworded to: '--signoff requires a GitHub user identity (name and email) -- this is not available for GitHub App tokens'. Also updated the flag help text to match
  8. Inconsistency between setup hard-failure and sync-scaffold graceful degradation (internal/cli/github.go): Added code comment explaining why setup intentionally differs: the user explicitly requested DCO sign-off via --signoff flag, so silently omitting the trailer would cause the DCO check to fail with a confusing error

Disagreed (1):

  1. Naming convention: signoff (single word) vs signOffTrailer (camelCase): The struct field 'signoff' uses single-word naming to match git's --signoff flag, which is the standard CLI convention. The parameter 'signOffTrailer' refers to the formatted trailer string, not the flag. The naming accurately reflects the distinction between the boolean flag and its derived string value

Tests: passed

Decision points
  • Where to place the shared FormatSignOffTrailer helper (alternatives: New signoff package, In layers package, In forge package; rationale: Placed in forge package because UserIdentity is already defined there, making it natural to add a method on that type and a related standalone function)
  • Whether to change WorkflowsLayer.WithSignOff signature to take *UserIdentity (alternatives: Change to take *UserIdentity, Keep name/email params but use shared formatter; rationale: Kept the existing (name, email) signature to minimize API churn -- only changed the internal formatting to use forge.FormatSignOffTrailer)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

fullsend-ai-coder Bot added a commit that referenced this pull request Aug 10, 2026
- Extract scaffoldOptions struct for applyPerRepoScaffold to reduce
  positional parameter count (10 → 8 + struct)
- Add --signoff to per-repo-only flag validation so it is rejected for
  org targets like --config and --config-hash
- Guard against empty name/email in identity before formatting the
  Signed-off-by trailer
- Strip newline characters from identity fields to prevent trailer
  injection via crafted profile names
- Extract shared FormatSignOffTrailer helper in forge package to
  deduplicate formatting between github setup and WorkflowsLayer
- Reword error message: "git user identity" → "GitHub user identity"
  since identity is resolved via the GitHub API, not local git config
- Add comment explaining why setup hard-fails on missing identity
  (unlike sync-scaffold which gracefully degrades)
- Extract newSignoffTestSetup helper to reduce test duplication
- Add tests for org-target rejection and empty identity fields

Addresses review feedback on #6004
@fullsend-ai-coder
fullsend-ai-coder Bot force-pushed the agent/6002-signoff-flag branch from 44ab7c2 to 6840bed Compare August 10, 2026 09:50
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:51 AM UTC · Completed 10:06 AM UTC

Commit: 6840bed · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@maruiz93

Copy link
Copy Markdown
Contributor

/fs-fix Rebase onto main and address the open review thread on internal/forge/forge.go (angle bracket sanitization in FormatSignOffTrailer).

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 10:22 AM UTC · Completed 10:32 AM UTC

Commit: 6840bed · View workflow run →

fullsend-ai-coder Bot added a commit that referenced this pull request Aug 10, 2026
- Extract scaffoldOptions struct for applyPerRepoScaffold to reduce
  positional parameter count (10 → 8 + struct)
- Add --signoff to per-repo-only flag validation so it is rejected for
  org targets like --config and --config-hash
- Guard against empty name/email in identity before formatting the
  Signed-off-by trailer
- Strip newline characters from identity fields to prevent trailer
  injection via crafted profile names
- Extract shared FormatSignOffTrailer helper in forge package to
  deduplicate formatting between github setup and WorkflowsLayer
- Reword error message: "git user identity" → "GitHub user identity"
  since identity is resolved via the GitHub API, not local git config
- Add comment explaining why setup hard-fails on missing identity
  (unlike sync-scaffold which gracefully degrades)
- Extract newSignoffTestSetup helper to reduce test duplication
- Add tests for org-target rejection and empty identity fields

Addresses review feedback on #6004
fullsend-ai-coder Bot added a commit that referenced this pull request Aug 10, 2026
Strip < and > from the name field in FormatSignOffTrailer to prevent
malformed Signed-off-by trailers when a forge user's display name
contains angle brackets.

Addresses review feedback on #6004
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (human-triggered)

Rebased branch onto main and addressed the open review thread: FormatSignOffTrailer now strips angle brackets from the name field in addition to newline characters, preventing malformed trailers from crafted display names.

Fixed (1):

  1. angle bracket sanitization in FormatSignOffTrailer (internal/forge/forge.go): Strip < and > from the name field in FormatSignOffTrailer to prevent malformed Signed-off-by trailers when a forge user's display name contains angle brackets. Added test coverage for angle bracket stripping.

Tests: passed

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-coder
fullsend-ai-coder Bot force-pushed the agent/6002-signoff-flag branch from 6840bed to df0ded5 Compare August 10, 2026 10:32
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 10, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:33 AM UTC · Completed 10:49 AM UTC

Commit: df0ded5 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@maruiz93

Copy link
Copy Markdown
Contributor

/fs-fix Address the open review thread on internal/forge/forge.go — strip angle brackets from the email field too in FormatSignOffTrailer.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 11:14 AM UTC · Completed 11:22 AM UTC

Commit: df0ded5 · View workflow run →

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 3 (human-triggered)

Addressed the open review thread: FormatSignOffTrailer now strips angle brackets from the email field too, using a single shared sanitize replacer for both fields. Added test coverage for the new behavior.

Fixed (2):

  1. strip angle brackets from email field in FormatSignOffTrailer (internal/forge/forge.go): Unified the sanitize replacer to strip newlines and angle brackets from both name and email fields. Previously angle brackets were only stripped from name, leaving email vulnerable to malformed trailers like <user@example.com>.
  2. add test coverage for angle bracket stripping in email (internal/forge/signoff_test.go): Added TestFormatSignOffTrailer_StripsAngleBracketsFromEmail to verify angle brackets are stripped from the email field.

Tests: passed

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

fullsend-ai-coder Bot added a commit that referenced this pull request Aug 10, 2026
Use a single sanitize replacer for both name and email fields, stripping
\n, \r, <, and > from both. Previously angle brackets were only stripped
from the name field, leaving the email vulnerable to malformed trailers
(e.g. <<user@example.com>>).

Addresses review feedback on #6004
fullsend-ai-review[bot]

This comment was marked as outdated.

maruiz93 pushed a commit that referenced this pull request Aug 11, 2026
- Extract scaffoldOptions struct for applyPerRepoScaffold to reduce
  positional parameter count (10 → 8 + struct)
- Add --signoff to per-repo-only flag validation so it is rejected for
  org targets like --config and --config-hash
- Guard against empty name/email in identity before formatting the
  Signed-off-by trailer
- Strip newline characters from identity fields to prevent trailer
  injection via crafted profile names
- Extract shared FormatSignOffTrailer helper in forge package to
  deduplicate formatting between github setup and WorkflowsLayer
- Reword error message: "git user identity" → "GitHub user identity"
  since identity is resolved via the GitHub API, not local git config
- Add comment explaining why setup hard-fails on missing identity
  (unlike sync-scaffold which gracefully degrades)
- Extract newSignoffTestSetup helper to reduce test duplication
- Add tests for org-target rejection and empty identity fields

Addresses review feedback on #6004
maruiz93 pushed a commit that referenced this pull request Aug 11, 2026
Strip < and > from the name field in FormatSignOffTrailer to prevent
malformed Signed-off-by trailers when a forge user's display name
contains angle brackets.

Addresses review feedback on #6004
@maruiz93
maruiz93 force-pushed the agent/6002-signoff-flag branch from 221e4c8 to 3ad1b7f Compare August 11, 2026 17:52
maruiz93 pushed a commit that referenced this pull request Aug 11, 2026
Use a single sanitize replacer for both name and email fields, stripping
\n, \r, <, and > from both. Previously angle brackets were only stripped
from the name field, leaving the email vulnerable to malformed trailers
(e.g. <<user@example.com>>).

Addresses review feedback on #6004
maruiz93 pushed a commit that referenced this pull request Aug 11, 2026
Move identity resolution (--signoff) before the dry-run exit in
runGitHubSetupPerRepo so that --dry-run --signoff validates the
token's identity up front instead of silently skipping the check.
Add a dry-run output line showing the Signed-off-by trailer that
would be appended.

Addresses review feedback on #6004
maruiz93 pushed a commit that referenced this pull request Aug 11, 2026
FormatSignOffTrailer now returns (string, error) and rejects inputs
that become empty after stripping newlines and angle brackets. This
closes the edge case where raw values pass the pre-sanitization
emptiness check in github.go but produce an empty name or email
after sanitization (e.g. a name consisting entirely of newlines).

All callers updated: github.go propagates the error, WithSignOff
silently omits the trailer (best-effort builder path).

Addresses review feedback on #6004
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 11, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 5:53 PM UTC · Completed 6:10 PM UTC

Commit: 3ad1b7f · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

Repos that enforce DCO (Developer Certificate of Origin) reject
scaffold commits from `fullsend github setup` because they lack
a Signed-off-by trailer. This adds a `--signoff` flag that
resolves the authenticated user's identity and appends the
trailer to the scaffold commit message.

When --signoff is set:
- Resolves the user's name and email via the forge API
- Appends "Signed-off-by: Name <email>" to the commit message
- Errors if identity cannot be resolved (e.g. bot tokens)

When --signoff is omitted (default), behavior is unchanged —
no trailer is appended. This ensures autonomous agent commits
remain exempt from DCO per project policy.

Note: pre-commit could not run in sandbox (network restriction).

Closes #6002
- Extract scaffoldOptions struct for applyPerRepoScaffold to reduce
  positional parameter count (10 → 8 + struct)
- Add --signoff to per-repo-only flag validation so it is rejected for
  org targets like --config and --config-hash
- Guard against empty name/email in identity before formatting the
  Signed-off-by trailer
- Strip newline characters from identity fields to prevent trailer
  injection via crafted profile names
- Extract shared FormatSignOffTrailer helper in forge package to
  deduplicate formatting between github setup and WorkflowsLayer
- Reword error message: "git user identity" → "GitHub user identity"
  since identity is resolved via the GitHub API, not local git config
- Add comment explaining why setup hard-fails on missing identity
  (unlike sync-scaffold which gracefully degrades)
- Extract newSignoffTestSetup helper to reduce test duplication
- Add tests for org-target rejection and empty identity fields

Addresses review feedback on #6004
Strip < and > from the name field in FormatSignOffTrailer to prevent
malformed Signed-off-by trailers when a forge user's display name
contains angle brackets.

Addresses review feedback on #6004
Use a single sanitize replacer for both name and email fields, stripping
\n, \r, <, and > from both. Previously angle brackets were only stripped
from the name field, leaving the email vulnerable to malformed trailers
(e.g. <<user@example.com>>).

Addresses review feedback on #6004
Move identity resolution (--signoff) before the dry-run exit in
runGitHubSetupPerRepo so that --dry-run --signoff validates the
token's identity up front instead of silently skipping the check.
Add a dry-run output line showing the Signed-off-by trailer that
would be appended.

Addresses review feedback on #6004
FormatSignOffTrailer now returns (string, error) and rejects inputs
that become empty after stripping newlines and angle brackets. This
closes the edge case where raw values pass the pre-sanitization
emptiness check in github.go but produce an empty name or email
after sanitization (e.g. a name consisting entirely of newlines).

All callers updated: github.go propagates the error, WithSignOff
silently omits the trailer (best-effort builder path).

Addresses review feedback on #6004
@maruiz93
maruiz93 force-pushed the agent/6002-signoff-flag branch from 3ad1b7f to d4bf64b Compare August 13, 2026 07:45
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 13, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:46 AM UTC · Completed 8:03 AM UTC

Commit: d4bf64b · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 13, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 8:28 AM UTC · Ended 8:31 AM UTC

Commit: e075229 · View workflow run →

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Marta Anon <manon@redhat.com>
@maruiz93
maruiz93 force-pushed the agent/6002-signoff-flag branch from e075229 to 8f9a921 Compare August 13, 2026 08:30
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 13, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:32 AM UTC · Completed 8:46 AM UTC

Commit: 8f9a921 · View workflow run →

@maruiz93
maruiz93 enabled auto-merge August 13, 2026 08:32
@maruiz93
maruiz93 added this pull request to the merge queue Aug 13, 2026
Merged via the queue into main with commit fafb2df Aug 13, 2026
15 checks passed
@maruiz93
maruiz93 deleted the agent/6002-signoff-flag branch August 13, 2026 08:52
@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 13, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 8:55 AM UTC · Completed 9:20 AM UTC

Commit: 8f9a921 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #6004feat(#6002): add --signoff flag to github setup command

This PR was created by the code agent from issue #6002 and merged after 6 days (Aug 7–13). The code agent produced a functional initial implementation, the review agent and human reviewer (maruiz93) caught several defensive coding gaps, and 6 /fs-fix iterations refined the code. One fix iteration failed due to a known infrastructure bug. The workflow completed successfully but exhibited several known pain points.

Timeline

  1. Aug 7 — Triage and code agents processed issue github setup scaffold commits lack DCO sign-off, breaking repos that enforce DCO #6002. PR feat(#6002): add --signoff flag to github setup command #6004 created with initial implementation (329 additions, 7 files).
  2. Aug 7 — Review agent posted first review: missing CLI docs, architectural coherence concern, naming/scope observations.
  3. Aug 10 — Human review (maruiz93) with CHANGES_REQUESTED: 6 findings covering org-target validation, struct extraction, empty-field guards, injection risk, code duplication, and test helper extraction. Four /fs-fix iterations followed.
  4. Aug 11 — Two more /fs-fix iterations. Fix iteration 6 failed due to a false positive in post-fix.sh's Signed-off-by scan — the scan range PRE_AGENT_HEAD..HEAD included an upstream commit with a legitimate Signed-off-by trailer after the agent rebased. Human retried with explicit "Do NOT use git commit -s" instruction (a misdiagnosis — the agent hadn't used -s; the bug was in the scan range). The retry succeeded.
  5. Aug 13 — Human replied to three remaining review agent findings (naming convention, intent-scope mismatch, silent error handling), explaining they were intentional design decisions. Human approved; PR merged.

Review quality

The review agent and human reviewer had roughly complementary coverage. The review agent uniquely identified missing CLI reference docs and architectural coherence between setup and sync-scaffold. The human uniquely caught architectural improvements (extract scaffoldOptions struct, org-target validation, code deduplication, test helper extraction). Both identified sanitization and empty-field gaps.

Known issues observed — fresh evidence

Assessment

No novel improvement opportunities were identified. All pain points observed in this workflow are already tracked by existing open issues across fullsend-ai/fullsend and fullsend-ai/agents. The most impactful fix would be fullsend-ai/agents#318 (post-fix.sh scan range), which caused a concrete workflow failure and human misdiagnosis on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/install CLI install and app setup go Pull requests that update go code ready-for-review Triggers review agent dispatch requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

github setup scaffold commits lack DCO sign-off, breaking repos that enforce DCO

1 participant