fix(#5470): prefer direct push for write-access users, detect gitlint, suppress self-dispatch by fullsend-ai-coder[bot] · Pull Request #5473 · fullsend-ai/fullsend

fullsend-ai-coder · 2026-07-22T16:14:41Z

Summary

Non-owner collaborators with write/admin access now push scaffold PRs directly to upstream instead of forking, avoiding /ok-to-test trust gates that block CI on cross-fork PRs
Reads .gitlint title-match-regex from target repos before creating the scaffold PR and warns when the commit message won't match
Drops the version slug from the scaffold commit message (was fullsend-dev in dev builds)
Adds scaffold branch filter to both shim workflow templates to suppress self-dispatch

Related Issue

Closes #5470

Changes

Direct push for write-access users (sub-problem #2: fork trust gates)

After the owner check, commitScaffoldViaPR now calls GetCollaboratorPermission via the GitHubExtensions interface to check if the non-owner user has write or admin access
If they do, pushes directly to upstream — no fork, no trust gate issue
Falls through to the existing fork logic on error or read permission

Gitlint detection (sub-problem #1: commit message linting)

CommitScaffoldFiles now reads .gitlint from the target repo via GetFileContent
If a title-match-regex section exists and the scaffold commit message doesn't match, warns the user that commit-lint CI may fail
Gracefully handles missing .gitlint, missing section, and invalid regex (all no-ops)

Self-dispatch suppression (sub-problem #3)

Both shim templates now check !startsWith(github.event.pull_request.head.ref, 'fullsend/scaffold') before dispatching

Cleanup

Removed scaffoldPRBody() disclaimer function and its test — replaced with a simple defaultScaffoldPRBody constant containing the original PR body
Dropped version slug from commit message in admin.go (was chore: initialize fullsend-{version}, now matches repos.go: chore: initialize fullsend per-repo installation)

Testing

go build ./internal/cli/... ./internal/layers/... passes
go test ./internal/layers/... passes (all tests including new write-access and gitlint tests)
go test ./internal/scaffold/... passes (including TestShimScaffoldBranchFilter)
Pre-commit hooks pass

Closes #5470

fullsend-ai-review · 2026-07-22T16:16:30Z

🤖 Review · ❌ Terminated · Started 4:16 PM UTC · Ended 4:47 PM UTC
Commit: dca6731 · View workflow run →

github-actions · 2026-07-22T16:17:09Z

Site preview

Preview: https://d0d14fcd-site.fullsend-ai.workers.dev

Commit: 378d1eea804822678a04c6e3636cde9d6374cfed

fullsend-ai-review · 2026-07-22T16:47:57Z

🤖 Finished Review · ❌ Failure · Started 4:16 PM UTC · Completed 4:47 PM UTC
Commit: dca6731 · View workflow run →

codecov · 2026-07-22T20:58:13Z

Codecov Report

❌ Patch coverage is 97.05882% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/layers/commit.go	96.82%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

ggallen · 2026-07-22T21:00:17Z

/fs-review

fullsend-ai-review · 2026-07-22T21:01:48Z

🤖 Finished Review · ✅ Success · Started 9:01 PM UTC · Completed 9:19 PM UTC
Commit: 7ada4e0 · View workflow run →

fullsend-ai-review · 2026-07-22T21:19:41Z

Review

Findings

Low

[semantic-mismatch] internal/layers/commit.go:438 — adaptCommitMsg hardcodes alternatives using conventional-commit prefixes (chore:, ci:, build:, bare description), but CommitScaffoldFiles is also called for upgrade and config-repo scaffold operations with different commit messages. The function gracefully falls back to returning the original message with a warning — no silent corruption occurs — but the alternatives list is scaffold-install-centric.
[redundant-alternative] internal/layers/commit.go:455 — When the input commitMsg already uses the chore: prefix (the common case for scaffold commits), adaptCommitMsg includes chore: + desc in the alternatives list, which reconstructs the identical title that already failed the regex match. The alternative is skipped by the if alt == title { continue } guard so it is harmless, but it adds no value for this common case.
[attacker-controlled-regex] internal/layers/commit.go — gitlintTitleRegex compiles a regex pattern from the target repo's .gitlint file without length/complexity bounds. Go's regexp package uses a linear-time RE2 engine which mitigates classic ReDoS. The remaining risk is limited to memory consumption from compiling very large patterns, but this is bounded by Go's regexp.Compile memory limits and the file is read from a repo the attacker already controls.
[self-DoS via branch naming] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml:40 — The dispatch job's if: condition suppresses dispatch when pull_request_target or pull_request_review events arrive with head.ref == 'fullsend/scaffold-install'. An attacker opening a cross-fork PR with a branch named exactly fullsend/scaffold-install would suppress the dispatch job for that PR. This is self-DoS only — the attacker suppresses agent dispatch on their own PR. The same condition applies to shim-workflow-call.yaml.
[scope-alignment] internal/cli/admin.go:43 — The PR extracts defaultScaffoldPRBody as a shared constant and removes the version slug from commit messages. While this cleanup is related to gitlint compatibility, it is not explicitly called out in issue fix(install): scaffold PRs fail target repo CI checks #5470. However, it is a reasonable consequence of the gitlint adaptation work.

Previous run

Review

Findings

Low

[semantic-mismatch] internal/layers/commit.go — adaptCommitMsg hardcodes alternatives using conventional-commit prefixes (chore:, ci:, build:, bare description), but CommitScaffoldFiles is also called for upgrade and config-repo scaffold operations with different commit messages. The function gracefully falls back to returning the original message with a warning — no silent corruption occurs — but the alternatives list is scaffold-install-centric.
[redundant-alternative] internal/layers/commit.go — When the input commitMsg already uses the chore: prefix (the common case for scaffold commits), adaptCommitMsg includes chore: + desc in the alternatives list, which reconstructs the identical title that already failed the regex match. The alternative is skipped by the if alt == title { continue } guard so it is harmless, but it adds no value for this common case.
[attacker-controlled-regex] internal/layers/commit.go — gitlintTitleRegex compiles a regex pattern from the target repo's .gitlint file without length/complexity bounds. Go's regexp package uses a linear-time RE2 engine which mitigates classic ReDoS. The remaining risk is limited to memory consumption from compiling very large patterns, but this is bounded by Go's regexp.Compile memory limits and the file is read from a repo the attacker already controls.
[self-DoS via branch naming] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml:40 — The dispatch job's if: condition suppresses dispatch when pull_request_target or pull_request_review events arrive with head.ref == 'fullsend/scaffold-install'. An attacker opening a cross-fork PR with a branch named exactly fullsend/scaffold-install would suppress the dispatch job for that PR. This is self-DoS only — the attacker suppresses agent dispatch on their own PR. The same condition applies to shim-workflow-call.yaml.
[naming-conventions] internal/cli/admin.go:43 — defaultScaffoldPRBody is defined in internal/cli/admin.go but used cross-file in internal/cli/repos.go. Both files are in package cli, so the constant is correctly placed where its consumers reside — unlike defaultScaffoldBranch in internal/layers/commit.go, which is internal to the layers package.
[scope-alignment] internal/cli/admin.go — The PR extracts defaultScaffoldPRBody as a shared constant and removes the version slug from commit messages. While this cleanup is related to gitlint compatibility, it is not explicitly called out in issue fix(install): scaffold PRs fail target repo CI checks #5470. However, it is a reasonable consequence of the gitlint adaptation work.

Previous run (2)

Review

Findings

Low

[semantic-mismatch] internal/layers/commit.go — adaptCommitMsg hardcodes alternatives using conventional-commit prefixes (chore:, ci:, build:, bare description), but CommitScaffoldFiles is also called for upgrade and config-repo scaffold operations with different commit messages. The function gracefully falls back to returning the original message with a warning — no silent corruption occurs — but the alternatives list is scaffold-install-centric.
[redundant-alternative] internal/layers/commit.go — When the input commitMsg already uses the chore: prefix (the common case for scaffold commits), adaptCommitMsg includes chore: + desc in the alternatives list, which reconstructs the identical title that already failed the regex match. The alternative is skipped by the if alt == title { continue } guard so it is harmless, but it adds no value for this common case.
[attacker-controlled-regex] internal/layers/commit.go — gitlintTitleRegex compiles a regex pattern from the target repo's .gitlint file without length/complexity bounds. Go's regexp package uses a linear-time RE2 engine which mitigates classic ReDoS. The remaining risk is limited to memory consumption from compiling very large patterns, but this is bounded by Go's regexp.Compile memory limits and the file is read from a repo the attacker already controls.
[self-DoS via branch naming] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml:40 — The dispatch job's if: condition suppresses dispatch when pull_request_target or pull_request_review events arrive with head.ref == 'fullsend/scaffold-install'. An attacker opening a cross-fork PR with a branch named exactly fullsend/scaffold-install would suppress the dispatch job for that PR. This is self-DoS only — the attacker suppresses agent dispatch on their own PR. The same condition applies to shim-workflow-call.yaml.
[naming-conventions] internal/cli/admin.go:43 — defaultScaffoldPRBody is defined in internal/cli/admin.go but used cross-file in internal/cli/repos.go. The existing pattern is that scaffold-related constants live in internal/layers/commit.go (see defaultScaffoldBranch). This constant should be collocated with other scaffold constants.
[intent-mismatch] internal/layers/commit.go — The adaptCommitMsg doc comment says it "tries common conventional-commit alternatives (ci:, build:, bare description)" but the implementation also tries chore: as the first alternative, which is omitted from the parenthetical list.

Previous run (3)

Review

Findings

Low

[semantic-mismatch] internal/layers/commit.go — adaptCommitMsg hardcodes alternatives using conventional-commit prefixes (chore:, ci:, build:, bare description), but CommitScaffoldFiles is also called for upgrade and config-repo scaffold operations with different commit messages. The function gracefully falls back to returning the original message with a warning — no silent corruption occurs — but the alternatives list is scaffold-install-centric.
[redundant-alternative] internal/layers/commit.go — When the input commitMsg already uses the chore: prefix (the common case for scaffold commits), adaptCommitMsg includes chore: + desc in the alternatives list, which reconstructs the identical title that already failed the regex match. The alternative is harmless but adds no value for this common case.
[self-DoS via branch naming] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml:40 — The dispatch job's if: condition suppresses dispatch when the PR head ref equals fullsend/scaffold-install. An attacker opening a cross-fork PR with a branch named exactly fullsend/scaffold-install would suppress the dispatch job for that PR. This is self-DoS only — the attacker suppresses agent dispatch on their own PR. The same condition applies to shim-workflow-call.yaml.
[error-handling] internal/layers/commit.go — hasWriteAccess silently returns false on GetCollaboratorPermission API errors, which could mask legitimate failures (e.g., network errors vs. expected "not found"). The doc comment explicitly states this as intentional design ("returns false on any error ... so the caller falls through to the fork path"), but a debug-level log would aid troubleshooting.

Previous run (4)

Review

Findings

Low

[semantic-mismatch] internal/layers/commit.go — adaptCommitMsg hardcodes alternatives using conventional-commit prefixes (chore:, ci:, build:, bare description), but CommitScaffoldFiles is also called for upgrade and config-repo scaffold operations with different commit messages. The function gracefully falls back to returning the original message with a warning — no silent corruption occurs — but the alternatives list is scaffold-install-centric.
[edge-case] internal/layers/commit.go — The gitlintTitleRegex parser handles regex= and regex = key prefixes but does not handle variations with multiple spaces or tabs between the key and =. The .gitlint INI format in practice nearly always uses one of the two recognized forms.
[self-DoS via branch naming] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml:40 — The dispatch job's if: condition suppresses dispatch when the PR head ref equals fullsend/scaffold-install. An attacker opening a cross-fork PR with a branch named exactly fullsend/scaffold-install would suppress the dispatch job for that PR. This is self-DoS only — the attacker suppresses agent dispatch on their own PR. The same condition applies to shim-workflow-call.yaml.
[scope-creep] internal/layers/commit.go — adaptCommitMsg goes beyond passive detection — it actively rewrites the commit message by trying alternative prefixes. The issue describes the symptom without prescribing a specific remedy, and active adaptation directly addresses the user pain. The fallback is safe (warns and returns original message).

Previous run (5)

Review

Findings

Low

[semantic-mismatch] internal/layers/commit.go — adaptCommitMsg hardcodes alternatives using conventional-commit prefixes (chore:, ci:, build:, bare description), but CommitScaffoldFiles is also called for upgrade and config-repo scaffold operations with different commit messages. The function gracefully falls back to returning the original message with a warning — no silent corruption occurs — but the alternatives may not match for non-scaffold operations.
[authorization bypass via branch naming] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml:40 — The startsWith(github.event.pull_request.head.ref, 'fullsend/scaffold') check can be triggered by an attacker naming their cross-fork branch accordingly, suppressing the dispatch job for their PR. This is self-DoS only — the attacker suppresses dispatch on their own PR, which stalls (but cannot bypass) merge gating if dispatch is a required check.
[scope creep] internal/layers/commit.go — adaptCommitMsg goes beyond warning — it actively rewrites the commit message by trying alternative prefixes. This is a natural extension of detection ("detect and fix if possible, warn if not") and the fallback is safe, but it exceeds the literal scope of issue fix(install): scaffold PRs fail target repo CI checks #5470's "detect and warn" framing.
[scope creep] internal/cli/admin.go:42 — Extracting the duplicated PR body string into defaultScaffoldPRBody is a minor refactor bundled with the bug fix. Reduces duplication across admin.go and repos.go but is not directly required by any of the three sub-problems in issue fix(install): scaffold PRs fail target repo CI checks #5470.
[constant placement] internal/cli/admin.go:42 — defaultScaffoldPRBody is declared in the cli package while the semantically related defaultScaffoldBranch lives in internal/layers/commit.go. Both constants describe scaffold defaults but live in different packages.
[error-handling idiom] internal/layers/commit.go:74 — if hasWrite := hasWriteAccess(...); hasWrite { declares an intermediate variable only to immediately test it. Simplify to if hasWriteAccess(ctx, client, owner, repo, user) {.

Previous run (6)

Review

Findings

Low

[semantic-mismatch] internal/layers/commit.go — adaptCommitMsg hardcodes alternatives using the string "initialize fullsend per-repo installation", but CommitScaffoldFiles is also called for upgrade and config-repo scaffold operations with different commit messages. When those messages fail the gitlint regex, the hardcoded alternatives are semantically wrong. The function gracefully falls back to returning the original message with a warning — no silent corruption occurs — but the alternatives will never match for non-scaffold operations. Consider extracting the description from the original commit message or parameterizing it.
[incomplete-filter] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml — The scaffold-branch filter uses github.event.pull_request.head.ref, which is null for issue_comment events. Comments on scaffold PRs will still trigger dispatch. The primary noise vector (PR open/synchronize from push-access delivery) is correctly filtered, and the existing bot-comment filter mitigates most remaining cases, but human comments on the scaffold PR would still dispatch.
[warn-only-mechanism] internal/layers/commit.go — adaptCommitMsg now tries common conventional-commit alternatives (ci:, build:, bare description) before falling back to a warning, which improves the prior behavior. When no alternative matches an arbitrary regex, the function returns the original message unchanged with a warning — commit-lint CI may still fail. This is an acknowledged design limitation.

Previous run (7)

Review

Findings

Low

[test-coverage-gap] internal/layers/commit_test.go — No test covers the precedence when a user has both write access AND an existing fork. The hasWriteAccess check (line 72) runs before FindExistingFork (line 82), so write access takes precedence — the fork is not used. A test would document this intended behavior and guard against accidental reordering.
[warn-only-mechanism] internal/layers/commit.go:436 — adaptCommitMsg detects .gitlint title-match-regex violations but only warns — it returns the original message unchanged. CI will still fail if the commit message does not match. This is an acknowledged design limitation (the function cannot fabricate a valid message for an arbitrary regex).

Previous run

Review

Findings

Medium

[logic-error] internal/layers/commit.go:497 — hasWriteAccess checks role == "write" || role == "admin" but omits "maintain". GitHub collaborator permission API returns "maintain" as a distinct role_name, and maintainers have push access to branches. Users with the maintain role will fall through to the fork path, partially defeating the purpose of this fix.
Remediation: Add role == "maintain": return role == "write" || role == "maintain" || role == "admin"

Low

[naming-convention] internal/cli/scaffold_pr.go — New file contains only a single 2-line constant (defaultScaffoldPRBody). While the DRY extraction is appropriate (the string was duplicated across three call sites), a dedicated file for a single constant is minimal. Consider colocating it in one of the existing files where it is used (e.g., admin.go).

Labels: PR modifies scaffold installation logic and dispatch shim templates

Previous run (8)

Review

Findings

Low

[test-coverage-gap] internal/layers/commit_test.go — No test covers the precedence when a user has both write access AND an existing fork. The hasWriteAccess check (line 72) runs before FindExistingFork (line 82), so write access takes precedence — the fork is not used. A test would document this intended behavior and guard against accidental reordering.
[warn-only-mechanism] internal/layers/commit.go:436 — adaptCommitMsg detects .gitlint title-match-regex violations but only warns — it returns the original message unchanged. CI will still fail if the commit message doesn't match. This is an acknowledged design limitation (the function cannot fabricate a valid message for an arbitrary regex).

Previous run (9)

Review

Findings

Medium

[logic-error] internal/layers/commit.go:497 — hasWriteAccess checks role == "write" || role == "admin" but omits "maintain". GitHub's collaborator permission API returns "maintain" as a distinct role_name, and maintainers have push access to branches. The codebase's own IsWriteAuthorized (in internal/forge/normevent/event.go) includes RoleMaintain alongside RoleAdmin and RoleWrite. Users with the maintain role will fall through to the fork path, partially defeating the purpose of this fix.
Remediation: Add role == "maintain": return role == "write" || role == "maintain" || role == "admin"

Low

[naming-convention] internal/cli/scaffold_pr.go — New file contains only a single 2-line constant (defaultScaffoldPRBody). While the DRY extraction is appropriate (the string was duplicated across three call sites), a dedicated file for a single constant is minimal. Consider colocating it in one of the existing files where it's used (e.g., admin.go).

Labels: PR modifies scaffold installation logic and dispatch shim templates

ggallen · 2026-07-23T02:09:28Z

/fs-review

fullsend-ai-review · 2026-07-23T02:11:11Z

🤖 Finished Review · ✅ Success · Started 2:11 AM UTC · Completed 2:29 AM UTC
Commit: 7ada4e0 · View workflow run →

fullsend-ai-review · 2026-07-23T18:03:26Z

🤖 Finished Review · ✅ Success · Started 6:03 PM UTC · Completed 6:20 PM UTC
Commit: 3eb6a6e · View workflow run →

ggallen · 2026-07-23T18:27:06Z

/fs-review

fullsend-ai-review · 2026-07-23T18:28:54Z

🤖 Finished Review · ✅ Success · Started 6:28 PM UTC · Completed 6:47 PM UTC
Commit: 3eb6a6e · View workflow run →

ggallen · 2026-07-23T18:48:48Z

/fs-review

fullsend-ai-review · 2026-07-23T18:50:28Z

🤖 Finished Review · ✅ Success · Started 6:50 PM UTC · Completed 7:05 PM UTC
Commit: 3eb6a6e · View workflow run →

ggallen · 2026-07-23T19:25:23Z

/fs-review

fullsend-ai-review · 2026-07-23T19:26:49Z

🤖 Finished Review · ✅ Success · Started 7:26 PM UTC · Completed 7:41 PM UTC
Commit: 3eb6a6e · View workflow run →

…d PRs Three CI failure modes when `fullsend admin install` creates scaffold PRs: 1. Commit message may not match target repo's .gitlint title-match-regex. Added adaptCommitMsg that reads .gitlint, tries conventional-commit alternatives (chore/ci/build/bare), and falls back with a warning. 2. Fork-based PRs are untrusted by /ok-to-test gates. Added hasWriteAccess check so collaborators with write/maintain/admin push directly to upstream, avoiding the fork trust issue entirely. 3. Shim workflow self-dispatches on the scaffold PR branch. Tightened the dispatch condition to skip only PR events on the exact scaffold branch name (fullsend/scaffold-install). Closes #5470 Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>

fullsend-ai-retro · 2026-07-23T20:46:51Z

🤖 Finished Retro · ✅ Success · Started 8:46 PM UTC · Completed 9:00 PM UTC
Commit: 378d1ee · View workflow run →

fullsend-ai-retro · 2026-07-23T21:00:56Z

Retro: PR #5473 — scaffold PR CI fixes

Timeline: Issue #5470 filed Jul 22 15:47 → PR #5473 opened Jul 22 16:14 → merged Jul 23 20:44 (~29 hours).

Code agent performed well — produced a comprehensive fix for all 3 sub-problems (fork trust gates, gitlint detection, self-dispatch) in 15 minutes (run 29935955756).

Review agent found a genuine medium-severity bug (missing maintain role in hasWriteAccess) on run 29957488819 — this would have caused a production issue. Several low-severity findings were also actionable.

Key issues identified:

Breaking change feedback dropped. rh-hemartin commented at Jul 23 08:14 that this should be marked as a breaking change. The PR title was never updated with !, no BREAKING CHANGE: trailer was added, and 7 subsequent review runs never surfaced this. COMMITS.md says "Flag a missing ! as an important-severity finding" and AGENTS.md instructs agents to follow COMMITS.md.
Human-driven fix cycle. Ggallen manually fixed all 6 actionable review findings with 14 force pushes. The fix agent was never triggered because the review agent submitted COMMENTED (not CHANGES_REQUESTED) for the medium-severity finding — evidence for #2940.
10 review runs (1 failed, 9 succeeded), 8 manually triggered via /fs-review. Runs 8 and 9 both reviewed commit 378d1ee — evidence for #4681. The "self-DoS via branch naming" finding was surfaced twice — evidence for #5007. Runs 4-10 posted progressively lower-value findings — evidence for #3025.
Human feedback not incorporated. The review agent ran 7 times after rh-hemartin's breaking-change comment and never referenced it — evidence for #4063 and #2371.

What went well: Code agent quality was high. Review agent caught a real bug. The multi-sub-problem decomposition from triage was effective.

Proposals filed

Review agent should check PR title for missing breaking-change marker per COMMITS.md (in fullsend-ai/agents)

fullsend-ai-coder Bot requested a review from a team as a code owner July 22, 2026 16:14

fullsend-ai-coder Bot added the ready-for-review Agent PR ready for human review label Jul 22, 2026

fullsend-ai-coder Bot assigned ggallen Jul 22, 2026

github-actions Bot deployed to site-preview July 22, 2026 16:17 View deployment

ggallen force-pushed the agent/5470-scaffold-pr-ci-fixes branch from dca6731 to 725e4b3 Compare July 22, 2026 20:49

github-actions Bot deployed to site-preview July 22, 2026 20:51 View deployment

ggallen changed the title ~~fix(#5470): suppress self-dispatch and set CI expectations on scaffold PRs~~ fix(#5470): prefer direct push for write-access users, detect gitlint, suppress self-dispatch Jul 22, 2026