Skip to content

fix(#2045): use PR head SHA in status comment instead of GITHUB_SHA#2048

Merged
ggallen merged 1 commit into
mainfrom
agent/2045-status-comment-sha
Jun 9, 2026
Merged

fix(#2045): use PR head SHA in status comment instead of GITHUB_SHA#2048
ggallen merged 1 commit into
mainfrom
agent/2045-status-comment-sha

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

In cross-repo workflow_dispatch mode, GITHUB_SHA points to the .fullsend config repo's default branch HEAD, not the PR's actual head commit. This made the status comment's "Commit:" line show an unrelated SHA.

Two changes fix this:

  1. dispatch.yml: Include pull_request.head.sha in the event
    payload that gets forwarded to agent workflows. Previously
    only head.ref and head.repo were extracted.

  2. run.go: Add prHeadSHAFromEventPath() which reads the
    workflow_dispatch event file (GITHUB_EVENT_PATH), parses
    the nested event_payload JSON string, and extracts
    pull_request.head.sha. setupStatusNotifier() now prefers
    this value over GITHUB_SHA when available, falling back
    gracefully for non-PR events or direct triggers.

Note: pre-commit could not run in sandbox (gitleaks install failed due to Go module cache permissions). The post-script runs pre-commit authoritatively on the runner.


Closes #2045

Post-script verification

  • Branch is not main/master (agent/2045-status-comment-sha)
  • Secret scan passed (gitleaks — c4f2c746654a35a9a0d904082ad2326a2ecc56af..HEAD)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Site preview

Preview: https://87d2a21d-site.fullsend-ai.workers.dev

Commit: 50a7389316299bf00f55b3141fc7e83518ff8f42

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · Started 8:33 PM UTC
Commit: d0ac11b · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [protected-path] .github/workflows/reusable-dispatch.yml:320 — This PR modifies .github/workflows/reusable-dispatch.yml, which is under the .github/ protected path. The PR links to issue statuscomment: "Commit:" line shows .fullsend repo HEAD, not PR head SHA #2045 and explains the rationale (adding pull_request.head.sha to the event payload for correct status comment display). Human approval is required for protected-path changes regardless of context.

Low

  • [data-exposure] internal/cli/run.go:1934 — The prHeadSHAFromEventPath function parses nested JSON from the GITHUB_EVENT_PATH file and extracts pull_request.head.sha. This value originates from untrusted PR event data. However, the extracted SHA flows into statuscomment.New() where shortSHA() calls isHexOnly(), which rejects any non-hex characters. The risk is limited to displaying an attacker-chosen (but valid hex) SHA string, which is the expected behavior for PR head commits. Not exploitable.

Info

  • [prior finding resolved] .github/workflows/reusable-dispatch.yml:320 — The prior medium finding (stale-reference: reusable-dispatch.yml missing sha: .head.sha) is resolved. Both workflow files now include sha: .head.sha in the jq payload construction.

  • [test adequacy] internal/cli/run_test.go:1079 — The test suite for prHeadSHAFromEventPath covers six scenarios. A minor gap is the absence of a test for malformed event_payload JSON content, though the function handles this correctly via the json.Unmarshal error check.

  • [silent-error-handling] internal/cli/run.go:1954 — Silent error handling (return empty string) is the established pattern for optional fallback helpers in this codebase. The caller falls back to GITHUB_SHA when empty string is returned.

  • [inline-struct-definition] internal/cli/run.go:1960 — Two anonymous struct definitions for targeted JSON unmarshaling. Idiomatic Go for single-use parsing, matches existing codebase patterns.

  • [inline-comment-style] internal/cli/run.go:1932 — Multi-line inline comment explaining GITHUB_SHA behavior follows established style with sentence-case opening, technical detail, and issue reference.

Previous run

Review

Findings

Medium

  • [stale-reference] .github/workflows/reusable-dispatch.yml:320 — The jq payload construction in reusable-dispatch.yml still uses head: {ref: .head.ref, repo: {full_name: .head.repo.full_name}} without the new sha: .head.sha field that was added to the scaffold dispatch.yml. Any workflow dispatched through the reusable workflow (per-repo mode) will not include pull_request.head.sha in the event payload, causing prHeadSHAFromEventPath to return empty and falling back to the potentially incorrect GITHUB_SHA — the exact bug this PR intends to fix.
    Remediation: Apply the same jq change (sha: .head.sha) to .github/workflows/reusable-dispatch.yml line 320.

Low

  • [data-exposure] internal/cli/run.go:1937 — The prHeadSHAFromEventPath function parses nested JSON from the workflow_dispatch event payload. The extracted SHA is used only for display in a status comment. The downstream shortSHA / isHexOnly validation ensures only hex characters are rendered, and shortSHA truncates to 7 chars. Not exploitable.

  • [error-handling-consistency] internal/cli/run.go:1936 — Silent error handling (return empty string on error) is used consistently, but the function performs two levels of JSON unmarshaling with identical error handling. The silent-return pattern is deliberate — the function is a best-effort extraction with a fallback to GITHUB_SHA.

  • [struct-inline-definition] internal/cli/run.go:1938 — The function uses two anonymous struct definitions for JSON unmarshaling. This is idiomatic Go for targeted single-use parsing, but nesting two levels may reduce readability if the structure grows.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 8, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:33 PM UTC · Completed 8:44 PM UTC
Commit: d0ac11b · View workflow run →

In cross-repo workflow_dispatch mode, GITHUB_SHA points to the
.fullsend config repo's default branch HEAD, not the PR's actual
head commit. This made the status comment's "Commit:" line show an
unrelated SHA.

- dispatch.yml / reusable-dispatch.yml: include pull_request.head.sha
  in the event payload forwarded to agent workflows
- run.go: add prHeadSHAFromEventPath() to extract the real SHA from
  the workflow_dispatch event file, preferring it over GITHUB_SHA

Closes #2045

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Greg Allen <gallen@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 9, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:44 AM UTC · Completed 10:54 AM UTC
Commit: ba204cb · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 9, 2026
@ggallen ggallen self-assigned this Jun 9, 2026
@ggallen ggallen added this pull request to the merge queue Jun 9, 2026
Merged via the queue into main with commit 46eb234 Jun 9, 2026
7 of 9 checks passed
@ggallen ggallen deleted the agent/2045-status-comment-sha branch June 9, 2026 11:23
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jun 9, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 11:26 AM UTC · Completed 11:38 AM UTC
Commit: ba204cb · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #2048fix(#2045): use PR head SHA in status comment instead of GITHUB_SHA

Timeline

  1. Issue #2045 filed (2026-06-08 20:18Z) — status comment shows .fullsend repo HEAD instead of PR head SHA.
  2. Triage (run 27164441237) succeeded.
  3. Code attempt 1 (run 27164639249) cancelled by concurrency group when a second /fs-code dispatch arrived 13s later — expected behavior.
  4. Code attempt 2 (run 27164650728) succeeded, opened PR #2048 with changes to dispatch.yml (scaffold), run.go, and tests. But missed reusable-dispatch.yml, leaving the fix incomplete for per-repo mode.
  5. Review 1 (run 27165169087) correctly flagged the missing reusable-dispatch.yml change as a medium [stale-reference] finding. No fix agent was dispatched.
  6. Human fixggallen manually force-pushed the fix ~14 hours later (2026-06-09 10:43Z).
  7. Review 2 (run 27200774113) confirmed the finding was resolved.
  8. Human approvalrh-hemartin approved. PR merged via merge queue (2026-06-09 11:23Z).

Assessment

Review quality: Good. The review agent caught a real, impactful omission on the first pass — the code agent fixed the scaffold dispatch.yml but missed the repo's reusable-dispatch.yml, which would have left the bug unfixed for per-repo mode installations.

Rework rate: 1 iteration. The code agent's initial output required one human-driven fix. The review agent identified the problem immediately, so the rework was targeted.

Token cost: Acceptable. The cancelled first code run wasted minimal tokens (~2s of agent time). The second review run was necessary to verify the fix.

Time to resolution: 14h delay due to human-in-the-loop fix. The review flagged the issue at 20:44Z but no fix agent was dispatched; the human applied the fix at 10:43Z the next day.

Proposals filtered (already covered by open issues)

  • Fix agent dispatch for medium findings — covered by #870
  • General multi-site update pattern for code agent — covered by #1255
  • Double code dispatch deduplication — covered by #766, #1362, and others

Proposals filed

ifireball pushed a commit to ifireball/fullsend that referenced this pull request Jun 10, 2026
Add a new bold-titled paragraph documenting that
dispatch.yml and reusable-dispatch.yml must stay in
sync, following the same pattern used for the mint
function sync guidance. This addresses a documentation
gap that caused PR fullsend-ai#2048 to miss syncing a jq payload
change across the two files.

Note: make lint could not run in sandbox due to Go
module cache permission errors. This is a
documentation-only change with no Go code modified.

Closes fullsend-ai#2067
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

statuscomment: "Commit:" line shows .fullsend repo HEAD, not PR head SHA

2 participants