Skip to content

refactor(scaffold): base URL generation and content hashing#2261

Merged
ggallen merged 1 commit into
fullsend-ai:mainfrom
ggallen:worktree-adr-0045-phase2-pr3
Jun 15, 2026
Merged

refactor(scaffold): base URL generation and content hashing#2261
ggallen merged 1 commit into
fullsend-ai:mainfrom
ggallen:worktree-adr-0045-phase2-pr3

Conversation

@ggallen

@ggallen ggallen commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary

  • Add ScaffoldBaseURL, ScaffoldContentHash, ScaffoldBaseURLWithHash, and ScaffoldHarnessNames to the scaffold package
  • Generates integrity-verified raw.githubusercontent.com base URLs for embedded harness templates, with #sha256=... fragments
  • Foundation for PR 4 (install generates thin wrapper harnesses with base: references)

ADR-0045 Phase 2, PR 3. See docs/plans/adr-0045-forge-portable-harness-phase2.md.

Test plan

  • ScaffoldBaseURL validates harness name (^[a-z][a-z0-9_-]*$) and commit SHA (40-char hex)
  • ScaffoldContentHash returns correct SHA-256 of embedded content; verified against manual computation
  • ScaffoldBaseURLWithHash produces valid URL with integrity hash fragment
  • ScaffoldHarnessNames returns all 6 harnesses sorted alphabetically
  • All 20 new tests pass; 100% coverage on reachable code paths
  • No regressions in existing scaffold tests
  • go vet and make lint clean

🤖 Generated with Claude Code

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 12, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:30 PM UTC · Completed 7:40 PM UTC
Commit: 9bcba6d · View workflow run →

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

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

Files with missing lines Patch % Lines
internal/scaffold/baseurl.go 88.23% 2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [test-organization] internal/scaffold/baseurl_test.go:159TestHarnessNames hard-codes the expected harness list ["code", "fix", "prioritize", "retro", "review", "triage"]. This is intentional snapshot-style testing — when harnesses are added or removed, the test will break to force deliberate acknowledgment of the change.

Info

  • [naming-convention] internal/scaffold/baseurl.go:18 — Regex variable naming uses validHarnessName/validCommitSHA while the harness package uses validAgentName/validRoleName. The naming is semantically correct for what each validates — different variable names for different validation domains is correct practice.

  • [edge-case] internal/scaffold/baseurl.go:20validCommitSHA only accepts 40-character SHA-1 hashes. GitHub uses SHA-1 today so this is correct, but Git SHA-256 repositories use 64-character hashes. Worth noting as a design constraint.

  • [error-message-pattern] internal/scaffold/baseurl.go:28 — Error message includes regex string representation via validHarnessName.String(). A human-readable description of valid characters may be more user-friendly.

  • [naming-evolution] internal/scaffold/baseurl.go — Function names changed from Scaffold* to Harness* prefix, addressing the naming stutter concern from the prior review. The ADR plan document has been updated consistently to reflect the new names throughout.

  • [architectural-fit] internal/scaffold/baseurl.go — The new functions operate directly on the package's embedded content FS and follow the same URL construction pattern as existing infrastructure. Clean architectural fit for Phase 2.

  • [authorization-via-plan] docs/plans/adr-0045-forge-portable-harness-phase2.md — This PR has no linked issue but implements PR 3 from the phased ADR-0045 Phase 2 plan. The plan document serves as the authorization.

Previous run

Review

Findings

Medium

  • [stale-identifier-reference] docs/plans/adr-0045-forge-portable-harness-phase2.md:174 — Plan document references old Scaffold* function names (ScaffoldBaseURL, ScaffoldContentHash, ScaffoldBaseURLWithHash, ScaffoldHarnessNames) in ~15 locations (lines 174, 180, 186, 190, 204-209, 233, 366, 386, 388, 408) that were renamed to Harness* prefix (HarnessBaseURL, HarnessContentHash, HarnessBaseURLWithHash, HarnessNames) in this PR.
    Remediation: Update all function name references from Scaffold* to Harness* prefix in the plan document.

Low

  • [naming-convention] internal/scaffold/baseurl.go:18 — Regex variable naming uses validHarnessName/validCommitSHA while the harness package uses validAgentName/validRoleName. The naming is semantically correct for what each validates, but the prefix convention differs slightly across packages.

  • [test-organization] internal/scaffold/baseurl_test.go:159TestHarnessNames hard-codes the expected harness list ["code", "fix", "prioritize", "retro", "review", "triage"]. This is intentional snapshot-style testing — when harnesses are added or removed, the test will break to force deliberate acknowledgment of the change.

Info

  • [edge-case] internal/scaffold/baseurl.go:20validCommitSHA only accepts 40-character SHA-1 hashes. GitHub uses SHA-1 today so this is correct, but Git SHA-256 repositories use 64-character hashes. Worth noting as a design constraint.

  • [error-message-pattern] internal/scaffold/baseurl.go:28 — Error message includes regex string representation via validHarnessName.String(). A human-readable description of valid characters may be more user-friendly.

  • [architectural-fit] internal/scaffold/baseurl.go — The new functions operate directly on the package's embedded content FS and follow the same URL construction pattern as existing infrastructure. Clean architectural fit for Phase 2.

  • [naming-evolution] internal/scaffold/baseurl.go — Function names changed from Scaffold* to Harness* prefix since the prior review, addressing the naming stutter concern. The new naming is more precise since these functions specifically handle harness templates.

Previous run

Review

Findings

Low

  • [plan-document-mismatch] The PR body references docs/plans/adr-0045-forge-portable-harness-phase2.md, but this file does not exist. The actual plan document is at docs/plans/adr-0045-forge-portable-harness-schema.md.
    Remediation: Update the PR description to reference the correct plan document.

  • [phase-sequencing-unclear] The PR claims to be "ADR-0045 Phase 2, PR 3" but the implementation plan does not define a numbered PR sequence for Phase 2. The code clearly aligns with Phase 2's stated goal of generating base URL references with integrity hashes, but the numbering is confusing.
    Remediation: Clarify in the PR description which Phase 2 objective this PR addresses.

  • [scope-clarity] The PR body references "Foundation for PR 4" which could be confused with Phase 1's PR 4 (base composition). In context, this refers to Phase 2's next PR for generating thin wrapper harnesses.
    Remediation: Clarify the PR numbering to avoid confusion with Phase 1.

Info

  • [edge-case] internal/scaffold/baseurl.go:20validCommitSHA only accepts 40-character SHA-1 hashes. GitHub uses SHA-1 today so this is correct, but Git SHA-256 repositories use 64-character hashes. Worth noting as a design constraint.

  • [test-brittleness] internal/scaffold/baseurl_test.go:161TestHarnessNames hard-codes the expected harness list. Adding or removing a harness YAML file will break this test. This is intentional snapshot-style testing and provides good coverage.

  • [architectural-fit] internal/scaffold/baseurl.go — The new functions operate directly on the package's embedded content FS and follow the same URL construction pattern as the existing upstreamBase constant. This is the correct package for these functions.

  • [phase-prerequisite-check] Phase 2 assumes Phase 1 is complete. The repository shows Phase 1 infrastructure (compose.go, forge.go) is in place.

Previous run (2)

Review

Findings

Low

  • [logic error] internal/scaffold/baseurl.go:42ScaffoldContentHash does not validate harnessName against harnessNamePattern before constructing a file path. While embed.FS rejects invalid paths and the function returns a clear error for unknown harnesses, this creates an inconsistent API surface compared to ScaffoldBaseURL which validates the same parameter. The existing FullsendRepoFile in scaffold.go follows the same pattern, so this is consistent with the package but worth noting.
    Remediation: Add harnessNamePattern validation at the top of ScaffoldContentHash for consistency.

  • [logic error] internal/scaffold/baseurl.go:42ScaffoldContentHash hardcodes .yaml extension while ScaffoldHarnessNames accepts both .yaml and .yml. Currently all 6 harness files use .yaml exclusively, so this is theoretical. If a .yml harness were added, ScaffoldHarnessNames would list it but ScaffoldContentHash would fail.
    Remediation: Either restrict ScaffoldHarnessNames to .yaml only, or have ScaffoldContentHash try both extensions.

  • [naming-conventions] internal/scaffold/baseurl.go — All 4 exported function names use a redundant Scaffold prefix (ScaffoldBaseURL, ScaffoldContentHash, ScaffoldBaseURLWithHash, ScaffoldHarnessNames). Existing functions in this package use unqualified names (FullsendRepoFile, FileMode, ManagedHeader, CustomizedDirs). In Go, callers already write scaffold.ScaffoldBaseURL which stutters.
    Remediation: Consider renaming to HarnessBaseURL, HarnessContentHash, HarnessBaseURLWithHash, HarnessNames.

  • [api-shape] internal/scaffold/baseurl.go:54ScaffoldHarnessNames returns ([]string, error) but the error from fs.ReadDir on embed.FS for a known embedded directory is likely unreachable in practice. Returning the error is idiomatic Go and defensively correct.

  • [naming-conventions] internal/scaffold/baseurl.go:19 — Pattern variable names (harnessNamePattern, commitSHAPattern) do not follow the valid prefix convention used in harness/harness.go (validAgentName, validRoleName).

Info

  • [path traversal] internal/scaffold/baseurl.go:36ScaffoldContentHash accepts harnessName without regex validation for file path construction. Go's embed.FS rejects path traversal sequences, making this unexploitable. Same pattern as existing FullsendRepoFile.

  • [plan-document-mismatch] The PR body references docs/plans/adr-0045-forge-portable-harness-phase2.md, but the actual plan document is at docs/plans/adr-0045-forge-portable-harness-schema.md.

Previous run

Review

Findings

Medium

  • [stale-identifier-reference] docs/plans/adr-0045-forge-portable-harness-phase2.md:174 — Plan document references old Scaffold* function names (ScaffoldBaseURL, ScaffoldContentHash, ScaffoldBaseURLWithHash, ScaffoldHarnessNames) in ~15 locations (lines 174, 180, 186, 190, 204-209, 233, 366, 386, 388, 408) that were renamed to Harness* prefix (HarnessBaseURL, HarnessContentHash, HarnessBaseURLWithHash, HarnessNames) in this PR.
    Remediation: Update all function name references from Scaffold* to Harness* prefix in the plan document.

Low

  • [naming-convention] internal/scaffold/baseurl.go:18 — Regex variable naming uses validHarnessName/validCommitSHA while the harness package uses validAgentName/validRoleName. The naming is semantically correct for what each validates, but the prefix convention differs slightly across packages.

  • [test-organization] internal/scaffold/baseurl_test.go:159TestHarnessNames hard-codes the expected harness list ["code", "fix", "prioritize", "retro", "review", "triage"]. This is intentional snapshot-style testing — when harnesses are added or removed, the test will break to force deliberate acknowledgment of the change.

Info

  • [edge-case] internal/scaffold/baseurl.go:20validCommitSHA only accepts 40-character SHA-1 hashes. GitHub uses SHA-1 today so this is correct, but Git SHA-256 repositories use 64-character hashes. Worth noting as a design constraint.

  • [error-message-pattern] internal/scaffold/baseurl.go:28 — Error message includes regex string representation via validHarnessName.String(). A human-readable description of valid characters may be more user-friendly.

  • [architectural-fit] internal/scaffold/baseurl.go — The new functions operate directly on the package's embedded content FS and follow the same URL construction pattern as existing infrastructure. Clean architectural fit for Phase 2.

  • [naming-evolution] internal/scaffold/baseurl.go — Function names changed from Scaffold* to Harness* prefix since the prior review, addressing the naming stutter concern. The new naming is more precise since these functions specifically handle harness templates.

Previous run (2)

Review

Findings

Low

  • [plan-document-mismatch] The PR body references docs/plans/adr-0045-forge-portable-harness-phase2.md, but this file does not exist. The actual plan document is at docs/plans/adr-0045-forge-portable-harness-schema.md.
    Remediation: Update the PR description to reference the correct plan document.

  • [phase-sequencing-unclear] The PR claims to be "ADR-0045 Phase 2, PR 3" but the implementation plan does not define a numbered PR sequence for Phase 2. The code clearly aligns with Phase 2's stated goal of generating base URL references with integrity hashes, but the numbering is confusing.
    Remediation: Clarify in the PR description which Phase 2 objective this PR addresses.

  • [scope-clarity] The PR body references "Foundation for PR 4" which could be confused with Phase 1's PR 4 (base composition). In context, this refers to Phase 2's next PR for generating thin wrapper harnesses.
    Remediation: Clarify the PR numbering to avoid confusion with Phase 1.

Info

  • [edge-case] internal/scaffold/baseurl.go:20validCommitSHA only accepts 40-character SHA-1 hashes. GitHub uses SHA-1 today so this is correct, but Git SHA-256 repositories use 64-character hashes. Worth noting as a design constraint.

  • [test-brittleness] internal/scaffold/baseurl_test.go:161TestHarnessNames hard-codes the expected harness list. Adding or removing a harness YAML file will break this test. This is intentional snapshot-style testing and provides good coverage.

  • [architectural-fit] internal/scaffold/baseurl.go — The new functions operate directly on the package's embedded content FS and follow the same URL construction pattern as the existing upstreamBase constant. This is the correct package for these functions.

  • [phase-prerequisite-check] Phase 2 assumes Phase 1 is complete. The repository shows Phase 1 infrastructure (compose.go, forge.go) is in place.

Previous run (3)

Review

Findings

Low

  • [plan-document-mismatch] The PR body references docs/plans/adr-0045-forge-portable-harness-phase2.md, but this file does not exist. The actual plan document is at docs/plans/adr-0045-forge-portable-harness-schema.md.
    Remediation: Update the PR description to reference the correct plan document.

  • [phase-sequencing-unclear] The PR claims to be "ADR-0045 Phase 2, PR 3" but the implementation plan does not define a numbered PR sequence for Phase 2. The code clearly aligns with Phase 2's stated goal of generating base URL references with integrity hashes, but the numbering is confusing.
    Remediation: Clarify in the PR description which Phase 2 objective this PR addresses.

  • [scope-clarity] The PR body references "Foundation for PR 4" which could be confused with Phase 1's PR 4 (base composition). In context, this refers to Phase 2's next PR for generating thin wrapper harnesses.
    Remediation: Clarify the PR numbering to avoid confusion with Phase 1.

Info

  • [edge-case] internal/scaffold/baseurl.go:20validCommitSHA only accepts 40-character SHA-1 hashes. GitHub uses SHA-1 today so this is correct, but Git SHA-256 repositories use 64-character hashes. Worth noting as a design constraint.

  • [test-brittleness] internal/scaffold/baseurl_test.go:161TestHarnessNames hard-codes the expected harness list. Adding or removing a harness YAML file will break this test. This is intentional snapshot-style testing and provides good coverage.

  • [architectural-fit] internal/scaffold/baseurl.go — The new functions operate directly on the package's embedded content FS and follow the same URL construction pattern as the existing upstreamBase constant. This is the correct package for these functions.

  • [phase-prerequisite-check] Phase 2 assumes Phase 1 is complete. The repository shows Phase 1 infrastructure (compose.go, forge.go) is in place.

Previous run (4)

Review

Findings

Low

  • [logic error] internal/scaffold/baseurl.go:42ScaffoldContentHash does not validate harnessName against harnessNamePattern before constructing a file path. While embed.FS rejects invalid paths and the function returns a clear error for unknown harnesses, this creates an inconsistent API surface compared to ScaffoldBaseURL which validates the same parameter. The existing FullsendRepoFile in scaffold.go follows the same pattern, so this is consistent with the package but worth noting.
    Remediation: Add harnessNamePattern validation at the top of ScaffoldContentHash for consistency.

  • [logic error] internal/scaffold/baseurl.go:42ScaffoldContentHash hardcodes .yaml extension while ScaffoldHarnessNames accepts both .yaml and .yml. Currently all 6 harness files use .yaml exclusively, so this is theoretical. If a .yml harness were added, ScaffoldHarnessNames would list it but ScaffoldContentHash would fail.
    Remediation: Either restrict ScaffoldHarnessNames to .yaml only, or have ScaffoldContentHash try both extensions.

  • [naming-conventions] internal/scaffold/baseurl.go — All 4 exported function names use a redundant Scaffold prefix (ScaffoldBaseURL, ScaffoldContentHash, ScaffoldBaseURLWithHash, ScaffoldHarnessNames). Existing functions in this package use unqualified names (FullsendRepoFile, FileMode, ManagedHeader, CustomizedDirs). In Go, callers already write scaffold.ScaffoldBaseURL which stutters.
    Remediation: Consider renaming to HarnessBaseURL, HarnessContentHash, HarnessBaseURLWithHash, HarnessNames.

  • [api-shape] internal/scaffold/baseurl.go:54ScaffoldHarnessNames returns ([]string, error) but the error from fs.ReadDir on embed.FS for a known embedded directory is likely unreachable in practice. Returning the error is idiomatic Go and defensively correct.

  • [naming-conventions] internal/scaffold/baseurl.go:19 — Pattern variable names (harnessNamePattern, commitSHAPattern) do not follow the valid prefix convention used in harness/harness.go (validAgentName, validRoleName).

Info

  • [path traversal] internal/scaffold/baseurl.go:36ScaffoldContentHash accepts harnessName without regex validation for file path construction. Go's embed.FS rejects path traversal sequences, making this unexploitable. Same pattern as existing FullsendRepoFile.

  • [plan-document-mismatch] The PR body references docs/plans/adr-0045-forge-portable-harness-phase2.md, but the actual plan document is at docs/plans/adr-0045-forge-portable-harness-schema.md.

Comment thread internal/scaffold/baseurl.go
Comment thread internal/scaffold/baseurl.go
Comment thread internal/scaffold/baseurl.go
Comment thread internal/scaffold/baseurl.go Outdated
Comment thread internal/scaffold/baseurl.go Outdated
@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jun 12, 2026
@ggallen ggallen force-pushed the worktree-adr-0045-phase2-pr3 branch from 9bcba6d to f75a820 Compare June 12, 2026 19:46
@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown

Site preview

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

Commit: f21436b8ce2d1b30b7405bf4e8c6db56b1e1be1e

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 12, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:50 PM UTC · Completed 8:02 PM UTC
Commit: f75a820 · View workflow run →

Comment thread internal/scaffold/baseurl.go
Comment thread internal/scaffold/baseurl_test.go
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jun 12, 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 thing on the commit type — this adds internal library functions with no user-facing behavior change, so per COMMITS.md it should be refactor(scaffold) rather than feat(scaffold). GoReleaser uses feat commits to populate the Features section of release notes, and this isn't something an end user would recognize as new capability. Worth updating the PR title and commit message before merge.

@ggallen ggallen changed the title feat(scaffold): base URL generation and content hashing refactor(scaffold): base URL generation and content hashing Jun 15, 2026
@ggallen ggallen force-pushed the worktree-adr-0045-phase2-pr3 branch from f75a820 to 6d3c538 Compare June 15, 2026 12:52
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 15, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 12:56 PM UTC · Ended 12:59 PM UTC
Commit: 4e21a60 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 15, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:03 PM UTC · Completed 1:15 PM UTC
Commit: 69d3dbd · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed ready-for-merge All reviewers approved — ready to merge labels Jun 15, 2026
…arness templates

Add infrastructure for generating integrity-verified base URLs pointing
to upstream scaffold harness templates. This is the foundation for PR 4
(install generates thin wrapper harnesses with base: references).

ADR-0045 Phase 2, PR 3.

Signed-off-by: Greg Allen <gallen@redhat.com>
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 15, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:07 PM UTC · Completed 2:19 PM UTC
Commit: f21436b · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Jun 15, 2026
@ggallen ggallen added this pull request to the merge queue Jun 15, 2026
Merged via the queue into fullsend-ai:main with commit 0c3d956 Jun 15, 2026
14 checks passed
@ggallen ggallen deleted the worktree-adr-0045-phase2-pr3 branch June 15, 2026 14:25
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jun 15, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 2:30 PM UTC · Completed 2:38 PM UTC
Commit: f21436b · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #2261refactor(scaffold): base URL generation and content hashing

Overall assessment: This workflow went well. The review bot caught real code issues (input validation gaps, naming conventions, .yml/.yaml inconsistency), the human reviewer caught a commit prefix mistake (featrefactor), and a later review run caught stale identifier references in the plan document. The PR merged cleanly after all feedback was addressed.

Timeline

  1. Jun 12 19:26 — ggallen opens PR with 3 files (2 new Go files + plan doc update)
  2. Jun 12 19:40 — Review bot approves with 5 low/info findings on code quality
  3. Jun 12 19:46 — ggallen force-pushes fixes addressing review feedback
  4. Jun 12 20:02 — Second review bot run approves the updated code
  5. Jun 12 20:53 — ralphbean (human) approves but flags commit prefix should be refactor not feat
  6. Jun 15 12:52 — ggallen renames PR title and force-pushes with updated commit message
  7. Jun 15 13:15 — Third review bot run finds medium-severity stale Scaffold* references in plan doc → applies requires-manual-review
  8. Jun 15 14:03 — ggallen force-pushes fix for stale doc references
  9. Jun 15 14:19 — Fourth review bot run clears findings → ready-for-merge
  10. Jun 15 14:25 — Merged via merge queue

Improvement opportunities (all already tracked)

All candidate proposals are covered by existing open issues:

  • Review non-determinism (first run approved, third run found medium-severity stale doc references on similar code): covered by #1389, #947, #1350
  • Commit prefix validation (bot didn't catch feat vs refactor — human did): covered by #2105
  • Redundant review runs on rapid force pushes (4 review runs, 1 cancelled): covered by #1418, #1422, #1357
  • Cross-referencing doc identifiers against code renames: covered by #2172, #1061

No new proposals filed — existing issues adequately cover all identified improvements.

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

Labels

ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants