feat(layers): add HarnessWrappersLayer for install-time base composition by ggallen · Pull Request #2300 · fullsend-ai/fullsend

ggallen · 2026-06-15T16:02:25Z

Summary

Add HarnessWrappersLayer to the install layer stack that generates thin harness wrapper files in the .fullsend config repo
Each wrapper references the upstream scaffold harness via base: URL (with #sha256= integrity hash) and sets role:/slug: locally
Add commitSHA build-time variable to root.go and .goreleaser.yml so wrappers reference the exact release commit
Add default allowed_remote_resources prefix in NewOrgConfig() so generated base URLs pass the allowlist check
Handle role-to-harness mapping (coder → code.yaml + fix.yaml, fullsend → skip)
Skip customized (non-managed) harness files, overwrite managed ones
Dev builds (commitSHA="dev") skip wrapper generation gracefully

ADR-0045 Phase 2 PR 4 — depends on PR 3 (base URL generation, merged).

Test plan

🤖 Generated with Claude Code

github-actions · 2026-06-15T16:04:06Z

Site preview

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

Commit: ca32f3640efc3a2412773d5739ec145df55ceaf7

fullsend-ai-review · 2026-06-15T16:06:13Z

🤖 Finished Review · ✅ Success · Started 4:06 PM UTC · Completed 4:21 PM UTC
Commit: 94735b6 · View workflow run →

codecov · 2026-06-15T16:06:31Z

Codecov Report

❌ Patch coverage is 91.54930% with 12 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/layers/harnesswrappers.go	96.18%	3 Missing and 2 partials ⚠️
internal/cli/admin.go	25.00%	3 Missing ⚠️
internal/cli/github.go	0.00%	2 Missing ⚠️
internal/cli/root.go	0.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

fullsend-ai-review · 2026-06-15T16:20:58Z

Review

Findings

Low

[edge-case] internal/layers/harnesswrappers.go:95 — When duplicate agents with the same role are present, both the code and fix wrappers use slugForRole[ac.Role] / slugForRole["coder"], which is last-write-wins from the slugForRole map built at lines 72-74. The seen map prevents duplicate file entries, but the slug used depends on agent ordering. Currently unreachable via DefaultAgentRoles() (which has unique roles) and Validate() rejects duplicate roles, but latent if custom role lists with duplicate coder entries are used in future.
Remediation: Use slugForRole["coder"] consistently for both the code and fix wrappers, or use ac.Slug for both by capturing it before the if name == "fix" override.
[ui-message-style] internal/layers/harnesswrappers.go:68 — UI message uses Skipped harness wrappers (past tense) while other layers use present tense for skipped operations (e.g., skipping repo secrets in secrets.go). The codebase has mixed patterns (configrepo.go uses past tense), so this is a minor inconsistency.

Info

[content-sandboxing] internal/layers/harnesswrappers.go:97 — YAML construction via string concatenation is safe: role values constrained to ValidRoles enum, slug values from GitHub app slugs, and baseURL validated by scaffold.HarnessBaseURLWithHash. No YAML metacharacter injection or path traversal possible.
[authorization] internal/config/config.go:117 — Default AllowedRemoteResources allowlist is hardcoded to first-party URL prefix (https://raw.githubusercontent.com/fullsend-ai/fullsend/), HTTPS-only, validated at consumption time by ValidateAllowedRemoteResources. Integrity enforced by mandatory #sha256= hash in each base URL. No fail-open risk.
[secrets-handling] .goreleaser.yml:9 — New commitSHA ldflags injection is safe. GoReleaser's {{.FullCommit}} is a built-in template variable (not user-controlled), and the commitSHA variable is validated by scaffold.validCommitSHA regex before any URL construction.
[design-note] internal/layers/harnesswrappers.go:155 — Uninstall is a no-op; harness wrapper files committed during Install are implicitly removed when the config repo is deleted by ConfigRepoLayer. The runUninstall stack omits this layer, consistent with a no-op Uninstall. Matches VendorBinaryLayer's approach.
[dual-write-coherence] internal/cli/admin.go — During Phase 2, role/slug is written to both config.yaml and harness wrapper files. The new ADR-0045 Consequences entry explicitly documents the source of truth and states reconciliation is unnecessary because both are written atomically during fullsend install.
[missing-authorization] — This PR implements ADR-0045 Phase 2 PR 4 but has no linked issue. The work is authorized by the accepted ADR-0045 and the implementation plan at docs/plans/adr-0045-forge-portable-harness-phase2.md.

Previous run

Review

Findings

Low

[edge-case] internal/layers/harnesswrappers.go:95 — When duplicate agents with the same role are present, the code wrapper uses the slug from the first coder agent (via ac.Slug), while the fix wrapper uses the slug from the last coder agent (via slugForRole["coder"], which is last-write-wins). Currently unreachable via DefaultAgentRoles() (and Validate() rejects duplicate roles) but latent if custom role lists with duplicate coder entries are used in future.
Remediation: Use slugForRole["coder"] consistently for both the code and fix wrappers, or use ac.Slug for both by capturing it before the if name == "fix" override.

Info

[content-sandboxing] internal/layers/harnesswrappers.go:97 — YAML construction via string concatenation is safe: role values constrained to ValidRoles enum, slug values from GitHub app slugs, and baseURL validated by scaffold.HarnessBaseURLWithHash. No YAML metacharacter injection or path traversal possible.
[authorization] internal/config/config.go:117 — Default AllowedRemoteResources allowlist is hardcoded to first-party URL prefix (https://raw.githubusercontent.com/fullsend-ai/fullsend/), HTTPS-only, validated at consumption time by ValidateAllowedRemoteResources. Integrity enforced by mandatory #sha256= hash in each base URL. Appropriate for the GitHub-only phase.
[secrets-handling] .goreleaser.yml:9 — New commitSHA ldflags injection is safe. GoReleaser's {{.FullCommit}} is a built-in template variable (not user-controlled), and the commitSHA variable is validated by scaffold.validCommitSHA regex before any URL construction.
[config-dual-write-coherence] internal/cli/admin.go — During Phase 2, role/slug is written to both config.yaml and harness wrapper files. The new ADR-0045 Consequences entry explicitly documents the source of truth and states reconciliation is unnecessary because both are written atomically during fullsend install.
[design-note] internal/layers/harnesswrappers.go:155 — Uninstall is a no-op; harness wrapper files committed during Install are implicitly removed when the config repo is deleted by ConfigRepoLayer. Consistent with VendorBinaryLayer's approach.
[missing-authorization] — This PR implements ADR-0045 Phase 2 PR 4 but has no linked issue. The work is authorized by the accepted ADR-0045 and the implementation plan at docs/plans/adr-0045-forge-portable-harness-phase2.md.

Previous run (2)

Review

Findings

Medium

[off-by-one] docs/guides/dev/cli-internals.md:250 — The stack now has 8 layers (ConfigRepo, Workflows, HarnessWrappers, VendorBinary, Secrets, Inference, Dispatch, Enrollment), but the install/uninstall range comments still say process 1→7 (forward) and process 7→1 (reverse). These should be updated to 1→8 and 8→1.
Remediation: Change process 1→7 (forward) to process 1→8 (forward) and process 7→1 (reverse) to process 8→1 (reverse).

Low

[edge-case] internal/layers/harnesswrappers.go:95 — When duplicate agents with the same role (e.g., two coder entries with different slugs) are present, the code wrapper uses the slug from the first coder agent (via ac.Slug), while the fix wrapper uses the slug from the last coder agent (via slugForRole["coder"], which is last-write-wins). Currently unreachable via DefaultAgentRoles() but latent if PerRepoDefaultRoles is used in future.
[scope-creep] internal/config/config.go:117 — The AllowedRemoteResources default added to NewOrgConfig() is specified in the implementation plan and the PR adds Consequences entries to ADR-0045, but the ADR's Decision section does not explicitly call out the default allowlist. A minor documentation gap — the plan and new ADR Consequences text provide adequate coverage.
[config-dual-write-coherence] internal/cli/admin.go — During Phase 2, role/slug is written to both config.yaml (via NewOrgConfig()) and harness wrapper files. ADR-0045's migration path documents the progression (Phase 2: dual-write, Phase 3: migrate consumers, Phase 4: remove agents block), and the new ADR-0045 Consequences entry explicitly documents the source of truth. Minor gap: the plan itself does not state reconciliation behavior on re-run.
[stale-layer-stack-reference] docs/ADRs/0012-admin-install-fullsend-repo-files-v1.md:40 — ADR-0012 defines the tracked file set as "defined by the scaffold implementation and its test suite" and lists specific files but does not mention harness wrapper files. Pre-existing extensibility clause covers this implicitly.
[stale-layer-stack-reference] docs/problems/platform-nativeness.md:46 — The document describes the layer stack without mentioning HarnessWrappersLayer, VendorBinaryLayer, or InferenceLayer. Pre-existing doc gap, not introduced by this PR.
[stale-reference] docs/plans/vertex-inference-provisioning.md:148 — The numbered layer ordering at lines 148-152 still lists only 6 layers and omits both harness-wrappers and vendor-binary. While the inline text at line 233 was updated, this numbered list was not.
[stale-doc] docs/plans/adr-0045-forge-portable-harness-phase2.md:255 — The phase 2 plan's stack description does not include InferenceLayer, which was added after the plan was written. The delta instruction (after WorkflowsLayer, before SecretsLayer) remains correct.
[edge-case] internal/layers/harnesswrappers.go:62 — The Makefile go-build target does not set commitSHA via ldflags, so local make go-build binaries will always skip harness wrapper generation. Appears intentional (dev builds skip gracefully) but worth noting for manual testing.
[config-field-documentation] internal/config/config.go:118 — AllowedRemoteResources added with a hardcoded GitHub URL prefix but no inline comment explaining the purpose. An inline comment like // Default allowlist for base: composition in harness wrappers (ADR-0045 Phase 2) would help.

Info

[missing-authorization] — This PR implements ADR-0045 Phase 2 PR 4 (685 additions across 14 files) but has no linked issue. The work is authorized by the accepted ADR-0045 and the implementation plan at docs/plans/adr-0045-forge-portable-harness-phase2.md.
[design-note] internal/layers/harnesswrappers.go:155 — Uninstall is a no-op; harness wrapper files committed during Install are never individually cleaned up. They are implicitly removed when the config repo is deleted by ConfigRepoLayer. Consistent with VendorBinaryLayer's approach. A rationale comment would help future maintainers.
[content-sandboxing] internal/layers/harnesswrappers.go:97 — YAML construction via string concatenation is safe: role values constrained to ValidRoles enum, slug values from GitHub app slugs, and baseURL validated by scaffold.HarnessBaseURLWithHash. No YAML metacharacter injection possible.
[authorization] internal/config/config.go:117 — Default AllowedRemoteResources allowlist is hardcoded to first-party URL prefix, HTTPS-only, validated at consumption time by ValidateAllowedRemoteResources. Appropriate for the GitHub-only phase.
[naming-consistency] internal/layers/harnesswrappers.go — HarnessWrappersLayer uses plural form. Existing layers are mixed: WorkflowsLayer and SecretsLayer are plural, while InferenceLayer and EnrollmentLayer are singular. Either convention is acceptable.
[stale-layer-stack-reference] docs/superpowers/specs/2026-04-06-fullsend-admin-spa-design.md:170 — The admin SPA design spec lists only 4 layers in Appendix B, omitting HarnessWrappersLayer and several others. Pre-existing incompleteness, not introduced by this PR.

Previous run (3)

Review

Findings

Medium

[scope-creep] internal/config/config.go:117 — The AllowedRemoteResources default added to NewOrgConfig() is specified in the implementation plan (line 274) but not in ADR-0045's Decision or Consequences sections. Adding a default URL allowlist entry during install is a security-relevant behavioral change that warrants explicit ADR coverage. See also: [permission-manifest-change] finding at this location.
Remediation: Confirm the AllowedRemoteResources default is intentional and document it in ADR-0045 or a follow-up ADR.
[config-dual-write-coherence] internal/cli/admin.go — During Phase 2, role/slug is written to both config.yaml (via NewOrgConfig()) and harness wrapper files. The plan (line 265) acknowledges the dual-write but does not designate a source of truth or describe reconciliation if the two diverge after manual edits.
Remediation: Document which is the source of truth (config.yaml agents block vs harness wrapper files) and whether reconciliation is planned for a future phase.

Low

[stale-reference] docs/plans/vertex-inference-provisioning.md:148 — The layer ordering listed at lines 148-152 omits both harness-wrappers and vendor-binary layers. The plan was written before those layers existed.
[stale-doc] docs/plans/adr-0045-forge-portable-harness-phase2.md:255 — The phase 2 plan's stack description does not include InferenceLayer, which was added after the plan was written. The delta instruction (after WorkflowsLayer, before SecretsLayer) remains correct.
[documentation-consistency] docs/ADRs/0006-ordered-layer-model.md:28 — The ADR uses dispatch-token while cli-internals.md uses Dispatch and the code uses NewOIDCDispatchLayer. Pre-existing inconsistency not introduced by this PR.
[edge-case] internal/layers/harnesswrappers.go:62 — The Makefile go-build target does not set commitSHA via ldflags, so local make go-build binaries will always skip harness wrapper generation. Appears intentional but worth noting for manual testing.
[naming-consistency] internal/layers/harnesswrappers.go — HarnessWrappersLayer uses plural form. Existing layers are mixed: WorkflowsLayer and SecretsLayer are plural, while InferenceLayer and EnrollmentLayer are singular. Not inconsistent but worth noting.

Info

[design-note] internal/layers/harnesswrappers.go:155 — Uninstall is a no-op; harness wrapper files committed during Install are never cleaned up. This is a reasonable design choice (committed repo files are removed when the config repo is deleted by ConfigRepoLayer).
[content-sandboxing] internal/layers/harnesswrappers.go:97 — YAML construction via string concatenation is safe: role values constrained to ValidRoles (^[a-z]+$), slug values from GitHub app slugs ([a-z0-9-]), and baseURL validated by scaffold.HarnessBaseURLWithHash (harness name ^[a-z][a-z0-9_-]*$, commit SHA ^[0-9a-f]{40}$).

Previous run (4)

Review

Findings

Low

[stale-reference] docs/plans/vertex-inference-provisioning.md:231 — The vertex inference provisioning plan references a layer stack order (config-repo → workflows → secrets → inference → dispatch-token → enrollment) that omits both the pre-existing vendor-binary layer and the new harness-wrappers layer added by this PR. Plan documents are non-normative, but the staleness could confuse future readers.
[permission-manifest-change] internal/config/config.go:117 — Default AllowedRemoteResources prefix https://raw.githubusercontent.com/fullsend-ai/fullsend/ hardcoded into NewOrgConfig(), granting every new org permission to fetch from the entire fullsend scaffold repository tree. Practical risk is limited by the #sha256= integrity hash in generated base URLs, which ensures fetched content matches expected hashes. Scope is appropriate for the current GitHub-only phase.
[stale-doc] docs/plans/adr-0045-forge-portable-harness-phase2.md:255 — The phase 2 plan describes adding HarnessWrappersLayer "after WorkflowsLayer and before SecretsLayer" which is correct as a delta instruction. However, the surrounding stack description does not include InferenceLayer, which was added after the plan was written.

Info

[design-note] internal/layers/harnesswrappers.go:155 — Uninstall is a no-op; harness wrapper files committed during Install are never cleaned up. Since these are committed files in the config repo, this is a reasonable design choice, but a brief rationale comment would help future maintainers.
[content-sandboxing] internal/layers/harnesswrappers.go:97 — YAML construction via string concatenation is safe: role values are constrained to ValidRoles (^[a-z]+$), slug values originate from GitHub app slugs ([a-z0-9-]), and baseURL is validated by scaffold.HarnessBaseURLWithHash (harness name ^[a-z][a-z0-9_-]*$, commit SHA ^[0-9a-f]{40}$).

Previous run (5)

Review

Findings

Low

[logic-error] internal/layers/harnesswrappers.go:169 — The Analyze method iterates over agents and calls harnessesForRole for each, but unlike Install (which uses a seen map to deduplicate), Analyze has no deduplication. If agents contain both a coder role and a fix role, harnessesForRole("coder") returns ["code", "fix"] and harnessesForRole("fix") returns ["fix"], causing harness/fix.yaml to be checked twice. Currently unreachable via DefaultAgentRoles() but latent if PerRepoDefaultRoles is passed.
[missing-authorization] — This PR implements ADR-0045 Phase 2 PR 4 but has no linked issue. The work is authorized by the accepted ADR and implementation plan, but explicit issue-level tracking benefits non-trivial multi-PR rollouts.
[permission-manifest-change] internal/config/config.go:117 — Default AllowedRemoteResources prefix hardcoded into NewOrgConfig(), granting every new org permission to fetch from the fullsend scaffold repository. Scope is appropriate for the current GitHub-only phase. See also: a TODO comment referencing future multi-forge work would help.
[error-message-style] internal/cli/root.go:10 — UI messages use colon separator pattern (Harness wrappers: skipped, Harness wrappers: nothing to generate) not used in other layers. Other layers use plain descriptions like Scaffold up to date.
[stale-reference] docs/plans/vertex-inference-provisioning.md:233 — The vertex inference provisioning plan document references an outdated layer stack order that does not include the new harness-wrappers layer.

Info

[design-note] internal/layers/harnesswrappers.go:155 — Uninstall is a no-op; harness wrapper files committed during Install are never cleaned up. A rationale comment would be helpful.
[content-sandboxing] internal/layers/harnesswrappers.go:97 — YAML construction via string concatenation is safe: role values constrained to ValidRoles, slug from GitHub app, baseURL validated by scaffold.HarnessBaseURLWithHash.

Previous run (6)

Review

Findings

High

[missing-documentation] docs/ADRs/0006-ordered-layer-model.md:28 — The canonical layer stack ordering lists config-repo → workflows → secrets → inference → dispatch-token → enrollment but does not include the new harness-wrappers layer added by this PR. ADR-0006 is the authoritative source for layer ordering and must be updated when the stack changes.
Remediation: Update ADR-0006 line 28 to reflect the new layer stack: config-repo → workflows → harness-wrappers → vendor-binary → secrets → inference → dispatch-token → enrollment.

Medium

[error-handling-gap] internal/layers/harnesswrappers.go:191 — loadExistingHarnesses silently swallows all errors from GetFileContent, including transient network errors. If a network error occurs when reading a user-customized harness file, the file will be absent from the existing map, causing Install to overwrite the user's customization. By contrast, the Analyze method correctly distinguishes IsNotFound from other errors and propagates non-404 errors.
Remediation: In loadExistingHarnesses, distinguish forge.IsNotFound errors (file genuinely missing, OK to skip) from other errors (network issues, permission errors). For non-404 errors, return an error so Install can fail fast rather than risk overwriting customized files.
[missing-authorization] — This PR implements ADR-0045 Phase 2 PR 4 (598 changed lines) but has no linked issue. While the work is authorized by the accepted ADR and the implementation plan at docs/plans/adr-0045-forge-portable-harness-phase2.md, non-trivial implementation work benefits from explicit issue-level tracking.
Remediation: Create a tracking issue for ADR-0045 Phase 2 implementation and link it to this PR.
[missing-documentation] docs/architecture.md:41 — The architecture document's installation model description references the layer stack as config-repo → workflows → secrets → inference → dispatch → enrollment but omits the new harness-wrappers layer. Note: this doc was already missing vendor-binary before this PR.
Remediation: Update docs/architecture.md line 41 to include the complete layer stack ordering.

Low

[logic-error] internal/layers/harnesswrappers.go:86 — When an agent list contains both a coder role and a separate fix role, harnessesForRole("coder") returns ["code", "fix"] and harnessesForRole("fix") returns ["fix"], producing duplicate harness/fix.yaml entries. Currently unreachable via DefaultAgentRoles() but latent if PerRepoDefaultRoles() is used in future.
[permission-manifest-change] internal/config/config.go:117 — Default AllowedRemoteResources prefix hardcoded into NewOrgConfig, implicitly granting every new org permission to fetch from the fullsend scaffold repository. See also: [architectural-coherence] finding at this location.
[architectural-coherence] internal/config/config.go:117 — Hardcoded GitHub-specific URL prefix. The implementation plan acknowledges this is acceptable for the current GitHub-only phase, but a TODO comment referencing future multi-forge work would be helpful.
[test-inadequate] internal/layers/harnesswrappers_test.go:33 — testAgents() omits retro and prioritize roles, and does not cover the fix + coder coexistence edge case.
[test-inadequate] internal/config/config.go:118 — The addition of AllowedRemoteResources to NewOrgConfig is not covered by any test.
[layer-stack-ordering] internal/cli/admin.go:1848 — Verified: HarnessWrappersLayer is correctly inserted after WorkflowsLayer and before SecretsLayer, satisfying the plan's ordering constraint.
[version-drift] docs/guides/dev/cli-internals.md:254 — Layer stack enumeration in CLI internals guide is stale (missing harness-wrappers).

Info

[design-note] internal/layers/harnesswrappers.go:139 — Uninstall is a no-op; harness wrapper files committed during Install are never cleaned up. If intentional, a comment explaining the rationale would be helpful.
[naming-consistency] internal/cli/root.go:10 — commitSHA uses camelCase while version is lowercase. Both follow valid Go conventions.
[content-sandboxing] internal/layers/harnesswrappers.go:97 — YAML construction via string concatenation is safe: role values constrained to ValidRoles, slug from GitHub app, baseURL validated by scaffold.HarnessBaseURLWithHash.

Previous run (7)

Review

Findings

Low

[stale-reference] docs/plans/vertex-inference-provisioning.md:231 — The vertex inference provisioning plan references a layer stack order (config-repo → workflows → secrets → inference → dispatch-token → enrollment) that omits both the pre-existing vendor-binary layer and the new harness-wrappers layer added by this PR. Plan documents are non-normative, but the staleness could confuse future readers.
[permission-manifest-change] internal/config/config.go:117 — Default AllowedRemoteResources prefix https://raw.githubusercontent.com/fullsend-ai/fullsend/ hardcoded into NewOrgConfig(), granting every new org permission to fetch from the entire fullsend scaffold repository tree. Practical risk is limited by the #sha256= integrity hash in generated base URLs, which ensures fetched content matches expected hashes. Scope is appropriate for the current GitHub-only phase.
[stale-doc] docs/plans/adr-0045-forge-portable-harness-phase2.md:255 — The phase 2 plan describes adding HarnessWrappersLayer "after WorkflowsLayer and before SecretsLayer" which is correct as a delta instruction. However, the surrounding stack description does not include InferenceLayer, which was added after the plan was written.

Info

[design-note] internal/layers/harnesswrappers.go:155 — Uninstall is a no-op; harness wrapper files committed during Install are never cleaned up. Since these are committed files in the config repo, this is a reasonable design choice, but a brief rationale comment would help future maintainers.
[content-sandboxing] internal/layers/harnesswrappers.go:97 — YAML construction via string concatenation is safe: role values are constrained to ValidRoles (^[a-z]+$), slug values originate from GitHub app slugs ([a-z0-9-]), and baseURL is validated by scaffold.HarnessBaseURLWithHash (harness name ^[a-z][a-z0-9_-]*$, commit SHA ^[0-9a-f]{40}$).

Previous run (8)

Review

Findings

Low

[logic-error] internal/layers/harnesswrappers.go:169 — The Analyze method iterates over agents and calls harnessesForRole for each, but unlike Install (which uses a seen map to deduplicate), Analyze has no deduplication. If agents contain both a coder role and a fix role, harnessesForRole("coder") returns ["code", "fix"] and harnessesForRole("fix") returns ["fix"], causing harness/fix.yaml to be checked twice. Currently unreachable via DefaultAgentRoles() but latent if PerRepoDefaultRoles is passed.
[missing-authorization] — This PR implements ADR-0045 Phase 2 PR 4 but has no linked issue. The work is authorized by the accepted ADR and implementation plan, but explicit issue-level tracking benefits non-trivial multi-PR rollouts.
[permission-manifest-change] internal/config/config.go:117 — Default AllowedRemoteResources prefix hardcoded into NewOrgConfig(), granting every new org permission to fetch from the fullsend scaffold repository. Scope is appropriate for the current GitHub-only phase. See also: a TODO comment referencing future multi-forge work would help.
[error-message-style] internal/layers/harnesswrappers.go:69 — UI messages use colon separator pattern (Harness wrappers: skipped, Harness wrappers: nothing to generate) not used in other layers. Other layers use plain descriptions like Scaffold up to date.
[stale-reference] docs/plans/vertex-inference-provisioning.md:233 — The vertex inference provisioning plan document references an outdated layer stack order that does not include the new harness-wrappers layer.

Info

[design-note] internal/layers/harnesswrappers.go:155 — Uninstall is a no-op; harness wrapper files committed during Install are never cleaned up. A rationale comment would be helpful.
[content-sandboxing] internal/layers/harnesswrappers.go:97 — YAML construction via string concatenation is safe: role values constrained to ValidRoles, slug from GitHub app, baseURL validated by scaffold.HarnessBaseURLWithHash.

Previous run (9)

Review

Findings

High

[missing-documentation] docs/ADRs/0006-ordered-layer-model.md:28 — The canonical layer stack ordering lists config-repo → workflows → secrets → inference → dispatch-token → enrollment but does not include the new harness-wrappers layer added by this PR. ADR-0006 is the authoritative source for layer ordering and must be updated when the stack changes.
Remediation: Update ADR-0006 line 28 to reflect the new layer stack: config-repo → workflows → harness-wrappers → vendor-binary → secrets → inference → dispatch-token → enrollment.

Medium

[error-handling-gap] internal/layers/harnesswrappers.go:191 — loadExistingHarnesses silently swallows all errors from GetFileContent, including transient network errors. If a network error occurs when reading a user-customized harness file, the file will be absent from the existing map, causing Install to overwrite the user's customization. By contrast, the Analyze method correctly distinguishes IsNotFound from other errors and propagates non-404 errors.
Remediation: In loadExistingHarnesses, distinguish forge.IsNotFound errors (file genuinely missing, OK to skip) from other errors (network issues, permission errors). For non-404 errors, return an error so Install can fail fast rather than risk overwriting customized files.
[missing-authorization] — This PR implements ADR-0045 Phase 2 PR 4 (598 changed lines) but has no linked issue. While the work is authorized by the accepted ADR and the implementation plan at docs/plans/adr-0045-forge-portable-harness-phase2.md, non-trivial implementation work benefits from explicit issue-level tracking.
Remediation: Create a tracking issue for ADR-0045 Phase 2 implementation and link it to this PR.
[missing-documentation] docs/architecture.md:41 — The architecture document's installation model description references the layer stack as config-repo → workflows → secrets → inference → dispatch → enrollment but omits the new harness-wrappers layer. Note: this doc was already missing vendor-binary before this PR.
Remediation: Update docs/architecture.md line 41 to include the complete layer stack ordering.

Low

[logic-error] internal/layers/harnesswrappers.go:86 — When an agent list contains both a coder role and a separate fix role, harnessesForRole("coder") returns ["code", "fix"] and harnessesForRole("fix") returns ["fix"], producing duplicate harness/fix.yaml entries. Currently unreachable via DefaultAgentRoles() but latent if PerRepoDefaultRoles() is used in future.
[permission-manifest-change] internal/config/config.go:117 — Default AllowedRemoteResources prefix hardcoded into NewOrgConfig, implicitly granting every new org permission to fetch from the fullsend scaffold repository. See also: [architectural-coherence] finding at this location.
[architectural-coherence] internal/config/config.go:117 — Hardcoded GitHub-specific URL prefix. The implementation plan acknowledges this is acceptable for the current GitHub-only phase, but a TODO comment referencing future multi-forge work would be helpful.
[test-inadequate] internal/layers/harnesswrappers_test.go:33 — testAgents() omits retro and prioritize roles, and does not cover the fix + coder coexistence edge case.
[test-inadequate] internal/config/config.go:118 — The addition of AllowedRemoteResources to NewOrgConfig is not covered by any test.
[layer-stack-ordering] internal/cli/admin.go:1848 — Verified: HarnessWrappersLayer is correctly inserted after WorkflowsLayer and before SecretsLayer, satisfying the plan's ordering constraint.
[version-drift] docs/guides/dev/cli-internals.md:254 — Layer stack enumeration in CLI internals guide is stale (missing harness-wrappers).

Info

[design-note] internal/layers/harnesswrappers.go:139 — Uninstall is a no-op; harness wrapper files committed during Install are never cleaned up. If intentional, a comment explaining the rationale would be helpful.
[naming-consistency] internal/cli/root.go:10 — commitSHA uses camelCase while version is lowercase. Both follow valid Go conventions.
[content-sandboxing] internal/layers/harnesswrappers.go:97 — YAML construction via string concatenation is safe: role values constrained to ValidRoles, slug from GitHub app, baseURL validated by scaffold.HarnessBaseURLWithHash.

fullsend-ai-review

See the review comment for full details.

ralphbean

I think the managed header and the overwrite logic need a rethink before this can merge. See inline.

fullsend-ai-review · 2026-06-15T20:27:56Z

🤖 Review · ⚠️ Cancelled · Started 8:27 PM UTC · Ended 8:35 PM UTC
Commit: 4e21a60 · View workflow run →

fullsend-ai-review · 2026-06-15T20:38:27Z

🤖 Finished Review · ✅ Success · Started 8:38 PM UTC · Completed 8:50 PM UTC
Commit: 1dbe12e · View workflow run →

fullsend-ai-review · 2026-06-15T20:58:28Z

🤖 Finished Review · ✅ Success · Started 8:58 PM UTC · Completed 9:11 PM UTC
Commit: 5625e13 · View workflow run →

ralphbean

LGTM — 1dbe12e fixed the header issue.

One thing I'm mulling over: the wrapper is four lines (header, base URL, role, slug). If we gave users that file directly and let them add overrides below base:, we'd have one file per agent instead of two. No customized/harness/ indirection to learn.

The cost is Install can't freely update the base: URL on upgrades if the user edits the file. But could something like fullsend lock --update handle that?

ggallen · 2026-06-15T21:40:57Z

@ralphbean Thanks for the approval and the thoughtful design note.

The single-file approach has real appeal — simpler mental model, one file per agent, no customized/harness/ indirection to learn. The tradeoff is that base: URLs contain a commit SHA and integrity hash that change every release. With fully managed wrappers, Install updates those URLs automatically on upgrade. If the file is user-editable, that upgrade responsibility shifts to a manual fullsend lock --update step, which creates a support surface ("why aren't my agents using the latest harness?").

That said, this is exactly the kind of thing Phase 4 ("collapse dual-write") should evaluate — whether we can merge the managed/customized split once the composition model is proven. I'd rather ship the safer default now and simplify later with real usage data.

fullsend-ai-review · 2026-06-15T21:51:46Z

🤖 Finished Review · ✅ Success · Started 9:51 PM UTC · Completed 10:07 PM UTC
Commit: 782b71c · View workflow run →

fullsend-ai-review · 2026-06-15T22:40:30Z

🤖 Finished Review · ✅ Success · Started 10:40 PM UTC · Completed 10:55 PM UTC
Commit: 3f46cfe · View workflow run →

fullsend-ai-review · 2026-06-15T23:01:15Z

🤖 Finished Review · ✅ Success · Started 11:01 PM UTC · Completed 11:13 PM UTC
Commit: 32ec577 · View workflow run →

…ion (ADR-0045 Phase 2 PR 4) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>

fullsend-ai-review · 2026-06-15T23:51:32Z

🤖 Finished Review · ✅ Success · Started 11:51 PM UTC · Completed 12:03 AM UTC
Commit: ca32f36 · View workflow run →

fullsend-ai-retro · 2026-06-16T02:08:46Z

🤖 Finished Retro · ✅ Success · Started 2:08 AM UTC · Completed 2:17 AM UTC
Commit: ca32f36 · View workflow run →

fullsend-ai-retro · 2026-06-16T02:17:52Z

Retro: PR #2300 — HarnessWrappersLayer

Timeline: Human-authored PR by ggallen adding HarnessWrappersLayer (693 additions, 14 files). The review agent ran 7 times over ~8 hours. The first review caught 4 legitimate bugs (error-handling gap, duplicate harness paths, missing test coverage, deduplication gap in Analyze), all fixed by the author. Human reviewer ralphbean caught a semantic contradiction the review agent missed: the generated managed-file header said "add overrides below base:" while the architectural intent (per ADR-0045) was for these files to be fully managed, with customization in a separate customized/harness/ directory. PR merged after both human and agent approval.

What went well:

Review agent's first pass was high-quality — the medium-severity error-handling finding and the logic-error finding were both real bugs that improved the code.
The agent correctly shifted from REQUEST_CHANGES to APPROVED after fixes were applied.
The Analyze deduplication bug found on the second review (after new code was pushed) was a genuine catch the human didn't flag.

Known issues observed (already tracked):

The permission-manifest-change finding on config.go:117 was raised 3 times across separate reviews despite the author responding "by design" each time. This is tracked by Review agent should deduplicate findings across iterations on the same PR #1013 (deduplicate findings across iterations), Review agent should recognize human-resolved protected-path findings and stop re-flagging #1583 (recognize human-resolved findings), and Review agent should not re-request changes for unchanged findings #1500 (don't re-request changes for unchanged findings).
7 review dispatches for an iteratively-developed human PR is expensive. This is tracked by Reduce unnecessary re-review runs on large PRs #1390 (reduce unnecessary re-review runs) and Skip review dispatch when HEAD SHA was already reviewed and approved #963 (skip when SHA already reviewed).

One new proposal below about the review agent missing contradictions between generated code comments and stated design intent.

Proposals filed

Review agent should detect contradictions between generated code comments and PR/ADR design intent (in fullsend-ai/fullsend)

github-actions Bot deployed to site-preview June 15, 2026 16:04 View deployment

fullsend-ai-review Bot suggested changes Jun 15, 2026

View reviewed changes

ralphbean requested changes Jun 15, 2026

View reviewed changes

Comment thread internal/layers/harnesswrappers.go

ggallen force-pushed the worktree-review-adr-0045 branch from 94735b6 to de440f8 Compare June 15, 2026 20:24

github-actions Bot deployed to site-preview June 15, 2026 20:26 View deployment

ggallen force-pushed the worktree-review-adr-0045 branch from de440f8 to 1dbe12e Compare June 15, 2026 20:34

github-actions Bot deployed to site-preview June 15, 2026 20:36 View deployment

fullsend-ai-review Bot approved these changes Jun 15, 2026

View reviewed changes

Comment thread internal/layers/harnesswrappers.go

Comment thread internal/config/config.go

Comment thread internal/layers/harnesswrappers.go Outdated

Comment thread internal/layers/harnesswrappers.go

Comment thread internal/layers/harnesswrappers.go

fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jun 15, 2026

ggallen force-pushed the worktree-review-adr-0045 branch from 1dbe12e to 5625e13 Compare June 15, 2026 20:54

github-actions Bot deployed to site-preview June 15, 2026 20:56 View deployment

ralphbean approved these changes Jun 15, 2026

View reviewed changes

fullsend-ai-review Bot approved these changes Jun 15, 2026

View reviewed changes

Comment thread internal/config/config.go

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 15, 2026

ggallen added this pull request to the merge queue Jun 15, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 15, 2026

ggallen force-pushed the worktree-review-adr-0045 branch 2 times, most recently from 803f4a8 to 782b71c Compare June 15, 2026 21:47

github-actions Bot deployed to site-preview June 15, 2026 21:49 View deployment

fullsend-ai-review Bot removed the ready-for-merge All reviewers approved — ready to merge label Jun 15, 2026

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

ggallen force-pushed the worktree-review-adr-0045 branch from 782b71c to 3f46cfe Compare June 15, 2026 22:35

github-actions Bot deployed to site-preview June 15, 2026 22:37 View deployment

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 15, 2026

ggallen force-pushed the worktree-review-adr-0045 branch from 3f46cfe to 32ec577 Compare June 15, 2026 22:57

github-actions Bot deployed to site-preview June 15, 2026 22:59 View deployment

fullsend-ai-review Bot approved these changes Jun 15, 2026

View reviewed changes

Comment thread internal/layers/harnesswrappers.go

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

feat(layers): add HarnessWrappersLayer for install-time base composit…

ca32f36

…ion (ADR-0045 Phase 2 PR 4) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>

ggallen force-pushed the worktree-review-adr-0045 branch from 32ec577 to ca32f36 Compare June 15, 2026 23:48

github-actions Bot deployed to site-preview June 15, 2026 23:49 View deployment

fullsend-ai-review Bot approved these changes Jun 16, 2026

View reviewed changes

Comment thread internal/layers/harnesswrappers.go

Comment thread internal/layers/harnesswrappers.go

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 16, 2026

ggallen added this pull request to the merge queue Jun 16, 2026

Merged via the queue into fullsend-ai:main with commit 93a5ae5 Jun 16, 2026
18 checks passed

ggallen deleted the worktree-review-adr-0045 branch June 16, 2026 02:03

fullsend-ai-retro Bot mentioned this pull request Jun 16, 2026

Review agent should detect contradictions between generated code comments and PR/ADR design intent #2308

Open

qodo-code-review Bot mentioned this pull request Jun 28, 2026

feat(config): add triage-agent to default allowed_remote_resources #2733

Merged

2 tasks

Uh oh!

Conversation

ggallen commented Jun 15, 2026

Summary

Test plan

Uh oh!

github-actions Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Site preview

Uh oh!

fullsend-ai-review Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fullsend-ai-review Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

Low

Info

Review

Findings

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Low

Info

Review

Findings

Low

Info

Review

Findings

High

Medium

Low

Info

Review

Findings

Low

Info

Review

Findings

Low

Info

Review

Findings

High

Medium

Low

Info

Uh oh!

fullsend-ai-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ralphbean left a comment

github-actions Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 15, 2026 •

edited

Loading

codecov Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-retro Bot commented Jun 16, 2026 •

edited

Loading