-
Notifications
You must be signed in to change notification settings - Fork 56
fix(#2574): embed provenance headers in shim template#2578
fix(#2574): embed provenance headers in shim template #2578fullsend-ai-coder[bot] wants to merge 2 commits into
Conversation
The reconciliation bot's shim output included two provenance-comment lines (managed-by header and upstream URL) that were not present in the template at templates/shim-workflow-call.yaml. This caused the review agent to flag template-drift on reconciliation PRs whose descriptions claimed exact sync. Root cause: the Go install path (PrependManagedHeader) added provenance headers at deploy time, but the template itself did not contain them. The reconciliation script preserved these headers from the remote file, producing output that diverged from the template. Changes: - Add provenance headers to shim-workflow-call.yaml so the template is the single source of truth for the full generated output - Make PrependManagedHeader idempotent — skip prepending when the content already starts with the managed header - Strip provenance headers from preserved user headers in shim_with_header_b64() to avoid duplication when updating shims that already have them - Update tests: relax YAML document-start marker check to allow comment headers before ---, add idempotency test for PrependManagedHeader, update reconciliation test template Closes #2574
E2E tests did not run
E2E tests run automatically for org/repo members and collaborators on pull requests.
For other contributors, a maintainer must add the ok-to-test label after the latest push.
See E2E testing guide for details.
🤖 Finished Review · ✅ Success · Started 5:32 PM UTC · Completed 5:44 PM UTC
Commit: 8627f86 · View workflow run →
Codecov Report
✅ All modified and coverable lines are covered by tests.
📢 Thoughts on this report? Let us know!
Review
Findings
Medium
-
[edge-case]
internal/scaffold/scaffold.go:210— The idempotency checkstrings.HasPrefix(s, header)only detects a pre-existing header when it appears at the very start of the content. For files with a shebang (#!),PrependManagedHeaderinserts the header AFTER the first line, so a second call would not find the header at position 0 and would insert a duplicate. While current call sites only invoke this function on fresh template content (making duplication unlikely in practice), the docstring now claims the function is "idempotent" without qualification, which is incorrect for the shebang path.
Remediation: Add a second check after the shebang-prefix branch to detect the header after the first line, or clarify the docstring that idempotency only applies when the content does not start with a shebang. Add aTestPrependManagedHeaderIdempotentShebangtest. -
[architectural-coherence]
internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh:130— The stripping logic that removes provenance headers from extracted user content exists because the reconcile script now prepends headers that were not in the original template. This creates a circular pattern: the script adds headers, then must strip them when preserving user content, then re-adds them. The Go installer (PrependManagedHeaderin scaffold.go) and the shell reconcile script implement parallel header-injection logic that can drift independently.
Remediation: Consider refactoring so both paths share the same header definition or cross-validate their output in tests, ensuring the two code paths stay synchronized.
Previous run
Review
Findings
High
- [architectural-divergence]
internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml:1— Embedding provenance headers directly in the template contradicts ADR 43's explicit design decision. ADR 43 states: "The upstream scaffold source files remain header-free so that agents editing templates are not confused by self-referential headers" and "Header injection is invisible to the scaffold source tree, keeping the embedded templates clean for direct editing and testing." The template should remain header-free, with headers added only at install/deploy time by PrependManagedHeader.
Remediation: Revert the template to header-free state. Fix the reconciliation drift issue by ensuring reconcile-repos.sh applies PrependManagedHeader when comparing or deploying the shim, rather than embedding headers in the template. The idempotent PrependManagedHeader change is correct and should be kept, but applied consistently at deploy time.
Medium
- [scope-divergence]
internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh:130— The new logic strips provenance headers from user content because the template now includes them natively. This stripping logic is a workaround for the architectural violation above. If the template remained header-free per ADR 43, this complexity would be unnecessary — the reconcile script would apply headers at deploy time, and user headers would be preserved without special filtering.
Remediation: Remove the provenance-stripping logic. Instead, ensure reconcile-repos.sh calls PrependManagedHeader (or equivalent header injection) when building the final deployed content.
Labels: PR modifies scaffold templates and reconciliation scripts in the install/scaffold component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the review comment for full details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[high] architectural-divergence
Embedding provenance headers directly in the template contradicts ADR 43's explicit design decision. ADR 43 states: 'The upstream scaffold source files remain header-free so that agents editing templates are not confused by self-referential headers' and 'Header injection is invisible to the scaffold source tree, keeping the embedded templates clean for direct editing and testing.' The template should remain header-free, with headers added only at install/deploy time by PrependManagedHeader.
Suggested fix: Revert the template to header-free state. Fix the reconciliation drift issue by ensuring reconcile-repos.sh applies PrependManagedHeader when comparing or deploying the shim, rather than embedding headers in the template. The idempotent PrependManagedHeader change is correct and should be kept, but applied consistently at deploy time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[medium] scope-divergence
The new logic strips provenance headers from user content because the template now includes them natively. This stripping logic is a workaround for the architectural violation. If the template remained header-free per ADR 43, this complexity would be unnecessary.
Suggested fix: Remove the provenance-stripping logic. Instead, ensure reconcile-repos.sh calls PrependManagedHeader (or equivalent header injection) when building the final deployed content.
ifireball
commented
Jun 24, 2026
/fs-fix
🤖 Finished Fix · ✅ Success · Started 9:48 AM UTC · Completed 9:59 AM UTC
Commit: 6bd5f3a · View workflow run →
Revert embedded provenance headers from the shim template to restore the header-free state required by ADR 43. Instead, add deploy-time header injection in reconcile-repos.sh via a PROVENANCE_HEADER variable prepended by shim_content_b64() and shim_with_header_b64(). Keep the provenance-stripping logic in shim_with_header_b64 to prevent header duplication from existing deployed shims that already have provenance headers from the Go installer. Update Go and shell tests to reflect header-free template and deploy-time provenance injection. Addresses review feedback on #2578
🔧 Fix agent — iteration 1 (human-triggered)
Addressed both review findings. Reverted template to header-free state per ADR 43 and added deploy-time provenance header injection in reconcile-repos.sh. Retained provenance-stripping logic to prevent duplication from existing deployed shims.
Fixed (2):
- architectural-divergence: template should be header-free per ADR 43 (
internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml): Reverted template to header-free state. Added PROVENANCE_HEADER variable and deploy-time injection in shim_content_b64() and shim_with_header_b64() in reconcile-repos.sh. Updated Go tests to expect header-free template. Updated shell test template and assertions. - scope-divergence: stripping logic is a workaround for template having headers (
internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh): Added deploy-time provenance header injection per ADR 43. Retained provenance-stripping logic in shim_with_header_b64 because existing deployed shims already have provenance headers from the Go installer — without stripping, headers would be duplicated when the reconciliation script extracts and re-prepends user headers. Updated the comment to clarify the stripping prevents duplication rather than working around template-embedded headers.
Tests: passed
Decision points
- Retained provenance-stripping logic despite review suggesting removal (alternatives: Remove stripping logic entirely, Move stripping into extract_user_header; rationale: Existing deployed shims have provenance headers added by the Go installer's PrependManagedHeader. extract_user_header captures everything above the sentinel, including those headers. Without stripping, deploy-time injection would duplicate them. The reviewer's suggestion assumed no existing deployed shims had provenance headers.)
Updated by fullsend fix agent
Site preview
Preview: https://3a52860d-site.fullsend-ai.workers.dev
Commit: 6c9a9615d0914752b8c6455ac59c0eae83d1d0e2
🤖 Finished Review · ✅ Success · Started 10:03 AM UTC · Completed 10:15 AM UTC
Commit: 6c9a961 · View workflow run →
The reconciliation bot's shim output included two provenance-comment lines (managed-by header and upstream URL) that were not present in the template at templates/shim-workflow-call.yaml. This caused the review agent to flag template-drift on reconciliation PRs whose descriptions claimed exact sync.
Root cause: the Go install path (PrependManagedHeader) added provenance headers at deploy time, but the template itself did not contain them. The reconciliation script preserved these headers from the remote file, producing output that diverged from the template.
Changes:
is the single source of truth for the full generated output
content already starts with the managed header
shim_with_header_b64() to avoid duplication when updating shims
that already have them
comment headers before ---, add idempotency test for
PrependManagedHeader, update reconciliation test template
Closes #2574
Post-script verification
agent/2574-template-provenance-headers)21cc2c25f3fc43d52321b3c58deb1326e17d666c..HEAD)