Skip to content

feat(scaffold): pin workflow refs to release commit SHA - #2615

Merged
waynesun09 merged 1 commit into
mainfrom
feat-1933-pin-scaffold-sha
Jun 24, 2026
Merged

feat(scaffold): pin workflow refs to release commit SHA#2615
waynesun09 merged 1 commit into
mainfrom
feat-1933-pin-scaffold-sha

Conversation

@waynesun09

Copy link
Copy Markdown
Member

Summary

  • Pin scaffolded uses: and fullsend_ai_ref to the release commit SHA instead of the mutable @v0 tag
  • Dev builds (commitSHA="dev") fall back to @v0
  • Covers both per-repo shim and per-org thin-caller workflows

Closes #1933

Test plan

  • go build ./cmd/fullsend/ passes
  • go test ./internal/scaffold/... passes (new pinned-SHA tests + fallback test)
  • go test ./internal/layers/... passes
  • go test ./internal/cli/... passes
  • make lint passes

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Pin scaffolded workflow refs to release commit SHA (dev builds fall back to @v0)
✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

Description

• Pin scaffolded workflow uses: and fullsend_ai_ref to the CLI build commit SHA.
• Fall back to @v0/v0 when commitSHA="dev" or unset.
• Add rendering/scaffold tests to verify pinning, comments, and fallback behavior.
Diagram

graph TD
  A["CLI github/admin commands"] --> B["resolveUpstreamRef()"] --> C["WorkflowsLayer / CollectPerRepoInstallFiles"] --> D["scaffold.RenderTemplate"] --> E["Workflow & shim templates"] --> F["Generated repo workflows"]
  D --> G["DefaultUpstreamRef (@v0)"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Pin to immutable release tag (e.g. v0.19.0) instead of commit SHA
  • ➕ More readable in workflow files
  • ➕ Still immutable if tags are protected and never moved
  • ➖ Relies on tag immutability policy; SHA is stronger by construction
  • ➖ Less precise for supply-chain auditing than a commit SHA
2. Fetch latest release commit SHA at scaffold time
  • ➕ Always pins to newest available release without rebuilding the CLI
  • ➕ Can auto-heal users stuck on older versions
  • ➖ Requires network access and GitHub API handling during setup
  • ➖ Adds failure modes and potentially non-reproducible scaffolds
3. Embed a release manifest (version → SHA) in the binary
  • ➕ No network dependency while still pinning to known SHAs
  • ➕ Allows stable behavior even for dev builds
  • ➖ Manifest maintenance overhead
  • ➖ Needs updates for every release and increases coupling to release process

Recommendation: The PR’s approach (pin to the build-time commit SHA, with an explicit dev fallback) is the best default for reproducibility and supply-chain integrity while keeping setup offline. The added UpstreamTag comment preserves human traceability without weakening the actual pin.

Files changed (17) +125 / -29

Enhancement (6) +65 / -14
admin.goPlumb upstream ref/tag into per-repo install and workflow layer +4/-2

Plumb upstream ref/tag into per-repo install and workflow layer

• Computes an upstream ref/tag via 'resolveUpstreamRef()' and passes it into per-repo scaffold collection and workflow layer creation. This enables SHA-pinned workflow references when scaffolding from admin commands.

internal/cli/admin.go

github.goUse upstream ref/tag for GitHub setup and scaffold sync +4/-2

Use upstream ref/tag for GitHub setup and scaffold sync

• Threads 'resolveUpstreamRef()' into 'CollectPerRepoInstallFiles' and 'WorkflowsLayer' for setup and sync paths. Ensures both per-repo shim and per-org thin-caller scaffolding get consistent pinning behavior.

internal/cli/github.go

root.goAdd upstream ref resolution from build metadata +11/-0

Add upstream ref resolution from build metadata

• Introduces 'resolveUpstreamRef()' to return (commitSHA, version tag) for release builds, and empty values for dev builds. Empty values intentionally trigger renderer fallback to 'config.DefaultUpstreamRef'.

internal/cli/root.go

workflows.goAllow WorkflowsLayer installs to pin upstream workflow refs +10/-1

Allow WorkflowsLayer installs to pin upstream workflow refs

• Adds 'upstreamRef'/'upstreamTag' to 'WorkflowsLayer' plus a 'WithUpstreamRef' builder. Uses these values when creating scaffold render options during workflow installation.

internal/layers/workflows.go

installfiles.goPass upstream ref/tag through per-repo scaffold collection +3/-3

Pass upstream ref/tag through per-repo scaffold collection

• Extends 'CollectPerRepoInstallFiles' to accept upstream ref/tag and forwards them into render options. Updates managed-path collection to call the new render options signature with empty ref/tag.

internal/scaffold/installfiles.go

render.goSupport SHA pinning and traceability comments in template rendering +33/-6

Support SHA pinning and traceability comments in template rendering

• Adds upstream ref/tag fields to 'RenderOptions' and renders a new '__FULLSEND_AI_REF__' placeholder. Updates reusable-workflow 'uses:' generation to pin to the resolved ref and optionally append a '# vX.Y.Z' comment for traceability, while falling back to 'config.DefaultUpstreamRef' when unset.

internal/scaffold/render.go

Tests (4) +53 / -8
workflows_test.goUpdate layer tests for new RenderOptions signature +2/-2

Update layer tests for new RenderOptions signature

• Updates test rendering calls to pass explicit upstream ref/tag arguments (empty strings) to the new 'RenderOptionsForInstall' signature. Keeps expected content comparisons unchanged.

internal/layers/workflows_test.go

installfiles_test.goUpdate scaffold installfile tests for upstream ref/tag params +5/-5

Update scaffold installfile tests for upstream ref/tag params

• Adjusts tests for the new 'RenderOptionsForInstall' and 'CollectPerRepoInstallFiles' signatures. Preserves existing behavior by passing empty ref/tag values.

internal/scaffold/installfiles_test.go

render_test.goAdd tests for pinned SHA rendering and fallback behavior +45/-0

Add tests for pinned SHA rendering and fallback behavior

• Extends placeholder-scan coverage and adds explicit tests verifying that thin callers and per-repo shim output pin 'uses:' and 'fullsend_ai_ref' to a provided SHA with a version comment. Adds a fallback test confirming default '@v0'/'v0' behavior when no upstream ref is provided.

internal/scaffold/render_test.go

workflow_call_alignment_test.goUpdate workflow alignment test for new render options signature +1/-1

Update workflow alignment test for new render options signature

• Updates the rendering helper to pass empty upstream ref/tag arguments to 'RenderOptionsForInstall' so existing alignment assertions continue to operate on default rendering output.

internal/scaffold/workflow_call_alignment_test.go

Other (7) +7 / -7
code.ymlReplace hardcoded fullsend_ai_ref with placeholder +1/-1

Replace hardcoded fullsend_ai_ref with placeholder

• Switches 'fullsend_ai_ref: v0' to '__FULLSEND_AI_REF__' so the renderer can pin to a commit SHA (or default).

internal/scaffold/fullsend-repo/.github/workflows/code.yml

fix.ymlReplace hardcoded fullsend_ai_ref with placeholder +1/-1

Replace hardcoded fullsend_ai_ref with placeholder

• Switches 'fullsend_ai_ref: v0' to '__FULLSEND_AI_REF__' to enable SHA pinning during scaffold rendering.

internal/scaffold/fullsend-repo/.github/workflows/fix.yml

prioritize.ymlReplace hardcoded fullsend_ai_ref with placeholder +1/-1

Replace hardcoded fullsend_ai_ref with placeholder

• Replaces the fixed 'v0' reference with '__FULLSEND_AI_REF__' for install-time substitution.

internal/scaffold/fullsend-repo/.github/workflows/prioritize.yml

retro.ymlReplace hardcoded fullsend_ai_ref with placeholder +1/-1

Replace hardcoded fullsend_ai_ref with placeholder

• Replaces 'fullsend_ai_ref: v0' with a render-time placeholder to support SHA pinning plus optional version comment.

internal/scaffold/fullsend-repo/.github/workflows/retro.yml

review.ymlReplace hardcoded fullsend_ai_ref with placeholder +1/-1

Replace hardcoded fullsend_ai_ref with placeholder

• Moves 'fullsend_ai_ref' to '__FULLSEND_AI_REF__' so the scaffold renderer can inject the resolved upstream ref.

internal/scaffold/fullsend-repo/.github/workflows/review.yml

triage.ymlReplace hardcoded fullsend_ai_ref with placeholder +1/-1

Replace hardcoded fullsend_ai_ref with placeholder

• Updates triage workflow template to use '__FULLSEND_AI_REF__' instead of a fixed 'v0' value.

internal/scaffold/fullsend-repo/.github/workflows/triage.yml

shim-per-repo.yamlMake per-repo shim fullsend_ai_ref renderable +1/-1

Make per-repo shim fullsend_ai_ref renderable

• Replaces the comment-tied 'fullsend_ai_ref: v0' with '__FULLSEND_AI_REF__' so per-repo shim scaffolding can pin to a release SHA consistently with 'uses:'.

internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown

Site preview

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

Commit: 39a55a5bacedbad1b442442060e6a83a640532e7

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 58 rules

Grey Divider


Informational

1. Dev installs fall back v0 📎 Requirement gap ≡ Correctness
Description
resolveUpstreamRef() returns empty strings for dev builds, which makes scaffolded workflow refs
fall back to config.DefaultUpstreamRef (v0) instead of the current git branch associated with
the dev build method. This violates the requirement that dev installs (e.g., go run) pin workflows
to the version/branch associated with the installed binary/build.
Code

internal/cli/root.go[R22-31]

+// resolveUpstreamRef returns the SHA and version tag for pinning scaffold
+// workflow refs. Release builds (commitSHA is a real SHA) return the SHA
+// and the corresponding version tag. Dev builds return empty strings,
+// causing the render layer to fall back to config.DefaultUpstreamRef.
+func resolveUpstreamRef() (ref, tag string) {
+	if commitSHA != "" && commitSHA != "dev" {
+		return commitSHA, "v" + version
+	}
+	return "", ""
+}
Relevance

⭐ Low

Repo previously chose dev builds fall back to stable refs; PR1278 deferred installer pinning; PR790
avoided pinning dev versions.

PR-#1278
PR-#790

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The checklist requires dev build installs to reference the current git branch. The new
resolveUpstreamRef() returns empty strings for dev builds, and the render layer then falls back to
config.DefaultUpstreamRef, which is defined as v0 and is asserted by tests.

Installer pins installed GitHub workflows to the version/branch associated with the installed binary/build
internal/cli/root.go[22-31]
internal/scaffold/render.go[78-83]
internal/config/config.go[12-17]
internal/scaffold/render_test.go[180-189]

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

## Issue description
`resolveUpstreamRef()` currently returns `( "", "" )` for dev builds, which causes scaffold rendering to fall back to `config.DefaultUpstreamRef` (`v0`). Compliance requires dev installation modes (e.g., `go run cmd/fullsend/` and local builds) to pin installed workflows to the current git branch (even if not pushed), not a generic mutable tag.

## Issue Context
- `resolveUpstreamRef()` explicitly treats `commitSHA == "dev"` as a dev build and returns empty values.
- The render layer interprets an empty upstream ref as “use DefaultUpstreamRef”, and the default is `v0`.
- Tests currently assert that fallback behavior includes `@v0` and `fullsend_ai_ref: v0`.

## Fix Focus Areas
- internal/cli/root.go[22-31]
- internal/scaffold/render.go[78-83]
- internal/config/config.go[12-17]
- internal/scaffold/render_test.go[180-189]

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


Grey Divider

Qodo Logo

When the CLI scaffolds workflow files during `fullsend github setup`,
pin the `uses:` directive and `fullsend_ai_ref` parameter to the
binary's build-time commit SHA instead of the mutable `@v0` tag.
Dev builds fall back to `@v0`.

Closes #1933

Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 24, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:15 PM UTC · Completed 4:24 PM UTC
Commit: 39a55a5 · View workflow run →

@fullsend-ai-review

Copy link
Copy Markdown

Looks good to me


Labels: PR modifies scaffold workflow rendering and CLI install paths to pin upstream refs at install time.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge component/install CLI install and app setup labels Jun 24, 2026
@waynesun09 waynesun09 added the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Jun 24, 2026

@ralphbean ralphbean left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One non-blocking note inline.

UpstreamRef string // commit SHA to pin workflow refs to; empty = use DefaultUpstreamRef
UpstreamTag string // version tag for traceability comment (e.g. "v0.19.0")
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[moderate] This constructor is getting wide — four positional args, and most callers pass "", "" for the new ones. Since it returns a RenderOptions struct, callers could just build the struct directly:

scaffold.RenderOptions{Vendored: false}

Zero-value defaults handle the common case, and future additions (like a --ref override) wouldn't require touching every call site.

Non-blocking — the current shape works, just worth considering if we're going to keep adding fields.

@waynesun09
waynesun09 added this pull request to the merge queue Jun 24, 2026
Merged via the queue into main with commit fcc8f32 Jun 24, 2026
27 of 28 checks passed
@waynesun09
waynesun09 deleted the feat-1933-pin-scaffold-sha branch June 24, 2026 17:53
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jun 24, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 5:57 PM UTC · Completed 6:03 PM UTC
Commit: 39a55a5 · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #2615 — feat(scaffold): pin workflow refs to release commit SHA

Timeline: Human-authored PR by waynesun09 implementing issue #1933. The review agent (fullsend-ai-review) approved in ~9 minutes with "Looks good to me" and zero findings. Human reviewer ralphbean also approved but left one moderate non-blocking inline comment noting that the RenderOptionsForInstall constructor was growing wide (4 positional args, most callers passing "", "" for the new ones) and suggesting callers build the RenderOptions struct directly instead.

Review quality gap: The review agent missed a valid Go API design concern that the human caught. This is a common pattern in Go — when a function accumulates positional parameters where most callers use zero values, it's a signal to switch to struct-based options. The agent should be able to detect this pattern, especially when the diff shows new parameters being added to an existing function and most call sites pass zero/empty values.

What went well: Fast turnaround (PR opened to merged in ~2 hours). Good test coverage — all modified lines covered. The qodo bot provided useful context summaries. The review agent's approval was directionally correct (no bugs or correctness issues).

1 proposal filed about teaching the review agent to detect growing positional parameter lists in Go functions. No duplicates found among existing open issues.

Proposals filed

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 ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Installation should pin workflows to version being installed

2 participants