feat(harness): add base composition for harness inheritance (ADR-0045 PR 4/7)

[ggallen]

Summary

• Add base field to harness YAML schema for harness-to-harness inheritance
• Implement LoadWithBase() for recursive base chain merging with cycle detection
• URL bases use ADR-0038's SSRF-hardened fetch infrastructure with mandatory integrity hashes
• Return base dependencies for lock file integration (wired in PR 5)

Test plan

• make go-test — all tests pass including 24 new compose tests
• make go-vet — no issues
• make lint — passes
• PR 5 will wire LoadWithBase into CLI for end-to-end verification

🤖 Generated with Claude Code

[github-actions]

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.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:09 PM UTC · Completed 3:22 PM UTC
Commit: d690457 · View workflow run →

[github-actions]

Site preview

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

Commit: f3ccb4323b69af49667b8130acf895fda37ff01c

[codecov]

Codecov Report

❌ Patch coverage is 79.47761% with 55 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/harness/compose.go	81.27%	28 Missing and 19 partials ⚠️
internal/harness/harness.go	52.94%	4 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Low

• [edge-case] internal/harness/compose.go:397 — mergeBaseIntoChild uses zero-value checks (== 0, == "") for scalar override decisions. A child harness cannot explicitly set timeout_minutes: 0 or sandbox_timeout_seconds: 0 to override a base's non-zero value. This is standard practice in this codebase (forge.go uses the same pattern), but the semantic should be documented.
• [API-shape-consistency] internal/harness/harness.go:297 — The new Validate() check for unconsumed base field causes Load()/LoadWithOpts() to reject any harness YAML containing a base field. Intentional fail-fast guard for the staged rollout (PR 4/7); no existing YAML files use base yet.
• [dead-code] internal/harness/harness.go:674 — The base entry in ValidateResourceTypes' declFields is unreachable in normal flow. Defense-in-depth that serves as a safety net if ValidateResourceTypes is ever called standalone.
• [missing-consumer] internal/harness/harness.go:760 — MatchingAllowedPrefixInList is exported but only called via the unexported matchingAllowedPrefix wrapper. Presumably exported for use by later PRs in the 7-PR series.
• [privilege-escalation] internal/harness/compose.go:62 — allowSelfAllowlist permits self-authorization when OrgAllowlist is empty. Now unexported (improved from prior review where it was exported), reducing risk to same-package callers only.
• [data-exposure] internal/harness/compose.go:458 — mergeBaseIntoChild inherits Security config from a base harness when child's is nil. A URL-fetched base (integrity-pinned and org-allowlisted) could set security.fail_mode to open, weakening the child's security posture. Documented in code comment.
• [fail-open] internal/harness/compose.go:208 — When WorkspaceRoot is empty, the path traversal containment check falls back to childDir. In practice this is more restrictive (smaller boundary), not less.
• [comment-accuracy] internal/harness/compose.go:516 — mergeForgeConfigInto comment could more precisely describe the ordering difference vs mergeForgeConfig in forge.go.
• [function-naming] internal/harness/compose.go:348 — matchingAllowedPrefix is a trivial one-line wrapper delegating to MatchingAllowedPrefixInList. Could be inlined at the single call site.
• [error-message-consistency] internal/harness/harness.go:297 — Error message names specific function LoadWithBase. Intentional developer guidance, but differs from the codebase convention of describing issues rather than solutions.
• [missing-user-documentation] docs/guides/user/customizing-agents.md — Does not document the new base field. Expected to be deferred until the feature is wired end-to-end in a later PR (PR 5+).

Info

• [missing-authorization] — No linked GitHub issue. Authorization established through accepted ADR-0045 with documented 7-PR implementation plan.
• [test-coverage] internal/harness/compose_test.go — Prior findings addressed: chained URL bases test (TestLoadWithBase_ChainedURLBases) added, and mergeForgeConfigInto slice mutation fixed with proper pre-allocation.
• [sub-agent-failure] — The intent-coherence sub-agent did not return findings: PR branch not checked out locally.
• [missing-developer-documentation] docs/guides/dev/cli-internals.md:266 — Sandbox Lifecycle section does not document LoadWithBase flow. Premature until PR 5 wires it into the CLI.

Previous run

Review

Findings

Medium

• [slice-mutation-pattern] internal/harness/compose.go:522 — mergeForgeConfigInto uses child.Skills = append(base.Skills, child.Skills...) which may mutate the base ForgeConfig's Skills backing array if it has remaining capacity. The top-level mergeBaseIntoChild was fixed (pre-allocating new slices at line 399), but mergeForgeConfigInto was not.
  Remediation: Pre-allocate like the top-level merge: merged := make([]string, 0, len(base.Skills)+len(child.Skills)); merged = append(merged, base.Skills...); merged = append(merged, child.Skills...); child.Skills = merged

Low

• [edge-case] internal/harness/compose.go:389 — mergeBaseIntoChild uses zero-value checks (== 0, == "") for scalar override decisions. A child harness cannot explicitly set timeout_minutes: 0 or sandbox_timeout_seconds: 0 to override a base's non-zero value. This is standard practice in this codebase (forge.go uses the same pattern), but the semantic should be documented.
• [API-shape-consistency] internal/harness/harness.go:297 — The new Validate() check for unconsumed base field causes Load()/LoadWithOpts() to reject any harness YAML containing a base field. Intentional fail-fast guard for the staged rollout (PR 4/7); no existing YAML files use base yet.
• [dead-code] internal/harness/harness.go:674 — The base entry in ValidateResourceTypes' declFields is unreachable in normal flow. Defense-in-depth that serves as a safety net if ValidateResourceTypes is ever called standalone.
• [missing-test-coverage] internal/harness/compose_test.go — No test covers chained URL bases (URL base whose own base is also a URL) or chains mixing URL and local bases.
• [privilege-escalation] internal/harness/compose.go:104 — AllowSelfAllowlist permits a child harness to self-authorize its own URL base fetches when OrgAllowlist is empty. Documented as "testing only" but is a public exported field with no build-tag gating.
• [data-exposure] internal/harness/compose.go:449 — mergeBaseIntoChild inherits Security config from a base harness when child's is nil. A URL-fetched base (integrity-pinned and org-allowlisted) could set security.fail_mode to open, weakening the child's security posture.
• [fail-open] internal/harness/compose.go:208 — When WorkspaceRoot is empty, the path traversal containment check falls back to childDir. The containment boundary depends on where the child file happens to be rather than the actual repository root. In practice this is more restrictive.
• [comment-accuracy] internal/harness/compose.go:349 — The merge rules comment lists slices as "concatenated" without noting that AllowedRemoteResources is explicitly excluded from merging (documented at line 416).
• [documentation-completeness] internal/harness/compose.go:506 — mergeForgeConfigInto comment says "Uses the same rules as mergeForgeConfig in forge.go" but the implementation differs in ordering: forge.go appends forge skills to harness skills, while this function prepends base skills.
• [error-message-consistency] internal/harness/compose.go:99 — Error message references config.yaml explicitly. Other errors in the codebase avoid referencing specific file names.

Info

• [missing-authorization] — No linked GitHub issue. Authorization established through accepted ADR-0045 with documented 7-PR implementation plan.

Previous run

Review

Findings

Medium

• [slice-mutation-pattern] internal/harness/compose.go:522 — mergeForgeConfigInto uses child.Skills = append(base.Skills, child.Skills...) which may mutate the base ForgeConfig's Skills backing array if it has remaining capacity. The top-level mergeBaseIntoChild was fixed (pre-allocating new slices at line 399), but mergeForgeConfigInto was not.
  Remediation: Pre-allocate like the top-level merge: merged := make([]string, 0, len(base.Skills)+len(child.Skills)); merged = append(merged, base.Skills...); merged = append(merged, child.Skills...); child.Skills = merged

Low

• [edge-case] internal/harness/compose.go:389 — mergeBaseIntoChild uses zero-value checks (== 0, == "") for scalar override decisions. A child harness cannot explicitly set timeout_minutes: 0 or sandbox_timeout_seconds: 0 to override a base's non-zero value. This is standard practice in this codebase (forge.go uses the same pattern), but the semantic should be documented.
• [API-shape-consistency] internal/harness/harness.go:297 — The new Validate() check for unconsumed base field causes Load()/LoadWithOpts() to reject any harness YAML containing a base field. Intentional fail-fast guard for the staged rollout (PR 4/7); no existing YAML files use base yet.
• [dead-code] internal/harness/harness.go:674 — The base entry in ValidateResourceTypes' declFields is unreachable in normal flow. Defense-in-depth that serves as a safety net if ValidateResourceTypes is ever called standalone.
• [missing-test-coverage] internal/harness/compose_test.go — No test covers chained URL bases (URL base whose own base is also a URL) or chains mixing URL and local bases.
• [privilege-escalation] internal/harness/compose.go:104 — AllowSelfAllowlist permits a child harness to self-authorize its own URL base fetches when OrgAllowlist is empty. Documented as "testing only" but is a public exported field with no build-tag gating.
• [data-exposure] internal/harness/compose.go:449 — mergeBaseIntoChild inherits Security config from a base harness when child's is nil. A URL-fetched base (integrity-pinned and org-allowlisted) could set security.fail_mode to open, weakening the child's security posture.
• [fail-open] internal/harness/compose.go:208 — When WorkspaceRoot is empty, the path traversal containment check falls back to childDir. The containment boundary depends on where the child file happens to be rather than the actual repository root. In practice this is more restrictive.
• [comment-accuracy] internal/harness/compose.go:349 — The merge rules comment lists slices as "concatenated" without noting that AllowedRemoteResources is explicitly excluded from merging (documented at line 416).
• [documentation-completeness] internal/harness/compose.go:506 — mergeForgeConfigInto comment says "Uses the same rules as mergeForgeConfig in forge.go" but the implementation differs in ordering: forge.go appends forge skills to harness skills, while this function prepends base skills.
• [error-message-consistency] internal/harness/compose.go:99 — Error message references config.yaml explicitly. Other errors in the codebase avoid referencing specific file names.

Info

• [missing-authorization] — No linked GitHub issue. Authorization established through accepted ADR-0045 with documented 7-PR implementation plan.

Previous run (2)

Review

Findings

High

• [API-shape-consistency] internal/harness/harness.go:297 — The new Validate() check for unconsumed base field breaks the existing API contract. Validate() is called by Load() and LoadWithOpts(). If a harness YAML contains a base field and is loaded via these paths (rather than LoadWithBase), Validate() now fails with "harness was not loaded with LoadWithBase." This is intentional per ADR-0045 (fail-loud to prevent silent misconfiguration), but the breaking change should be documented and callers should be audited.
  Remediation: Document the breaking change in the PR description. Audit all internal Load()/LoadWithOpts() call sites to confirm none could encounter a base field before PR 5 wires LoadWithBase into the CLI.

Medium

• [edge-case] internal/harness/compose.go:389 — mergeBaseIntoChild uses zero-value checks for scalar override (if child.TimeoutMinutes == 0). A child cannot explicitly set timeout_minutes: 0 to override a base's non-zero value — the base value always wins. Same applies to sandbox_timeout_seconds. This semantic is undocumented and could surprise users.
  Remediation: Document in the ADR or harness schema that zero-valued scalars cannot override a base's non-zero value. If explicit zeroing is needed in the future, consider pointer types for these fields.

• [slice-mutation-pattern] internal/harness/compose.go:397 — Slice concatenation for Skills, Plugins, Providers, and APIServers uses append(base.Skills, child.Skills...) which may mutate base.Skills if the backing array has remaining capacity. While the current recursive flow discards the base after merging, this is a known Go footgun that creates risk for future refactors.
  Remediation: Pre-allocate a new slice: merged := make([]string, 0, len(base.Skills)+len(child.Skills)); merged = append(merged, base.Skills...); merged = append(merged, child.Skills...); child.Skills = merged. Apply the same pattern to Plugins, Providers, and APIServers.

• [test-gap] internal/harness/compose_test.go — No test exercises the directory containment validation (compose.go:206-214) that prevents base paths from escaping the workspace root via ../ traversal. This is a security control with zero direct test coverage.
  Remediation: Add TestLoadWithBase_LocalBase_PathTraversal that attempts base: ../../etc/passwd and verifies the "escapes workspace root" error is returned.

Low

• [privilege-escalation] internal/harness/compose.go:104 — AllowSelfAllowlist permits a child harness to self-authorize its own URL base fetches when OrgAllowlist is empty. Documented as "testing only" but is a public field with no build-tag gating.
• [data-exposure] internal/harness/compose.go:442 — mergeBaseIntoChild inherits Security config from a base harness when the child's is nil. A URL-fetched base (even integrity-pinned) could set security.fail_mode to open, weakening the child's security posture. This is a design trade-off — child authors must explicitly set their own security block to prevent inheritance of a weaker posture.
• [dead-code] internal/harness/harness.go:674 — The base entry in ValidateResourceTypes' declFields is unreachable in normal flow. Validate() rejects base != "" at line 297 before reaching ValidateResourceTypes. LoadWithBase clears base before calling Validate. The check is harmless defense-in-depth but misleading without a comment.
• [missing-integrity-verification] internal/harness/compose.go:268 — On cache hit in fetchBaseURL, the code relies on fetch.CacheGet's content-addressable storage (hash-keyed directory) for integrity. No independent hash verification in fetchBaseURL itself. Low risk given the content-addressable scheme, but noted for defense-in-depth awareness.
• [stale-doc] docs/guides/dev/cli-internals.md:266 — Documents harness loading pipeline as LoadWithOpts only. LoadWithBase is a new alternative entry point. The doc is accurate for non-base harnesses but incomplete. Since PR 5 will wire LoadWithBase into the CLI, updating now may be premature — but the gap should be tracked.
• [missing-test-coverage] internal/harness/compose_test.go — No test covers chained URL bases (a URL base whose own base is also a URL). Only local-to-local chaining (TestLoadWithBase_ChainedBases) and single URL bases are tested.

Previous run (3)

Review

Findings

High

• [API-shape-consistency] internal/harness/harness.go:297 — The new Validate() check for unconsumed base field breaks the existing API contract. Validate() is called by Load() and LoadWithOpts(). If a harness YAML contains a base field and is loaded via these paths (rather than LoadWithBase), Validate() now fails with "harness was not loaded with LoadWithBase." This is intentional per ADR-0045 (fail-loud to prevent silent misconfiguration), but the breaking change should be documented and callers should be audited.
  Remediation: Document the breaking change in the PR description. Audit all internal Load()/LoadWithOpts() call sites to confirm none could encounter a base field before PR 5 wires LoadWithBase into the CLI.

Medium

• [edge-case] internal/harness/compose.go:389 — mergeBaseIntoChild uses zero-value checks for scalar override (if child.TimeoutMinutes == 0). A child cannot explicitly set timeout_minutes: 0 to override a base's non-zero value — the base value always wins. Same applies to sandbox_timeout_seconds. This semantic is undocumented and could surprise users.
  Remediation: Document in the ADR or harness schema that zero-valued scalars cannot override a base's non-zero value. If explicit zeroing is needed in the future, consider pointer types for these fields.

• [slice-mutation-pattern] internal/harness/compose.go:397 — Slice concatenation for Skills, Plugins, Providers, and APIServers uses append(base.Skills, child.Skills...) which may mutate base.Skills if the backing array has remaining capacity. While the current recursive flow discards the base after merging, this is a known Go footgun that creates risk for future refactors.
  Remediation: Pre-allocate a new slice: merged := make([]string, 0, len(base.Skills)+len(child.Skills)); merged = append(merged, base.Skills...); merged = append(merged, child.Skills...); child.Skills = merged. Apply the same pattern to Plugins, Providers, and APIServers.

• [test-gap] internal/harness/compose_test.go — No test exercises the directory containment validation (compose.go:206-214) that prevents base paths from escaping the workspace root via ../ traversal. This is a security control with zero direct test coverage.
  Remediation: Add TestLoadWithBase_LocalBase_PathTraversal that attempts base: ../../etc/passwd and verifies the "escapes workspace root" error is returned.

Low

• [privilege-escalation] internal/harness/compose.go:104 — AllowSelfAllowlist permits a child harness to self-authorize its own URL base fetches when OrgAllowlist is empty. Documented as "testing only" but is a public field with no build-tag gating.
• [data-exposure] internal/harness/compose.go:442 — mergeBaseIntoChild inherits Security config from a base harness when the child's is nil. A URL-fetched base (even integrity-pinned) could set security.fail_mode to open, weakening the child's security posture. This is a design trade-off — child authors must explicitly set their own security block to prevent inheritance of a weaker posture.
• [dead-code] internal/harness/harness.go:674 — The base entry in ValidateResourceTypes' declFields is unreachable in normal flow. Validate() rejects base != "" at line 297 before reaching ValidateResourceTypes. LoadWithBase clears base before calling Validate. The check is harmless defense-in-depth but misleading without a comment.
• [missing-integrity-verification] internal/harness/compose.go:268 — On cache hit in fetchBaseURL, the code relies on fetch.CacheGet's content-addressable storage (hash-keyed directory) for integrity. No independent hash verification in fetchBaseURL itself. Low risk given the content-addressable scheme, but noted for defense-in-depth awareness.
• [stale-doc] docs/guides/dev/cli-internals.md:266 — Documents harness loading pipeline as LoadWithOpts only. LoadWithBase is a new alternative entry point. The doc is accurate for non-base harnesses but incomplete. Since PR 5 will wire LoadWithBase into the CLI, updating now may be premature — but the gap should be tracked.
• [missing-test-coverage] internal/harness/compose_test.go — No test covers chained URL bases (a URL base whose own base is also a URL). Only local-to-local chaining (TestLoadWithBase_ChainedBases) and single URL bases are tested.

Previous run (4)

Review

Findings

Medium

• [fail-open] internal/harness/compose.go:207 — Directory containment check for local base paths is skipped when WorkspaceRoot is empty. The guard if opts.WorkspaceRoot != "" { means that if no caller provides WorkspaceRoot, any relative path traversal (e.g., base: ../../../../etc/passwd) is silently allowed. This is a fail-open pattern: the containment check should be mandatory, not opt-in.
  Remediation: When WorkspaceRoot is empty and the base is a local path, return an error rather than silently skipping the containment check. Alternatively, default WorkspaceRoot to the child harness's directory if unset. This should be addressed before PR 5 wires LoadWithBase into the CLI.

• [stale-doc] docs/guides/dev/cli-internals.md:266 — Documents harness loading as LoadWithOpts: unmarshal → validateForge → ResolveForge → Validate. This PR introduces LoadWithBase as a new alternative entry point for the harness loading pipeline when base composition is used. The existing documentation is not incorrect but is now incomplete.
  Remediation: Update the "Load harness" box in the Sandbox Lifecycle diagram to include the LoadWithBase pipeline: LoadWithBase → loadBaseChain (if base) → mergeBaseIntoChild → ResolveForge → Validate.

Low

• [missing-authorization] — PR has no linked issue. The work is authorized by accepted ADR-0045 with a documented 7-PR implementation plan, but a tracking issue linking the PR sequence would establish a clear authorization trail.
• [privilege-escalation] internal/harness/compose.go:104 — AllowSelfAllowlist permits a child harness to self-authorize its own URL base fetches when OrgAllowlist is empty. While documented as "testing only," it is a public field with no build-tag gating. The default is safe (false), but consider gating behind a build tag or using a package-internal test helper.
• [data-exposure] internal/harness/compose.go:438 — mergeBaseIntoChild inherits Security config from a base harness when the child's is nil. A URL-fetched base (even integrity-pinned) could set security.fail_mode to open, weakening the child's security posture. Consider documenting this as a known trust delegation.
• [scope-alignment] internal/harness/compose.go:37 — ComposeOpts omits the HarnessAllowlist field from the implementation plan; reads child.AllowedRemoteResources directly instead. Functionally equivalent but the plan should be updated to reflect this design choice.
• [scope-alignment] internal/harness/harness.go:293 — Validate() rejects unconsumed base field — a beneficial defensive check not specified in the plan. Update ADR-0045 to document this validation rule.
• [architectural-coherence] internal/harness/compose.go:411 — AllowedRemoteResources is explicitly NOT merged from base to child (correct for security), but ADR-0045's merge rules table doesn't document this non-merge behavior.
• [logic-error] internal/harness/harness.go:403 — ResolveRelativeTo does not resolve the Base field. Latent since LoadWithBase clears Base before returning, but violates the function's stated contract.
• [architectural-conflict] internal/harness/compose.go:26 — Local Dependency type mirrors resolve.Dependency to avoid circular imports. Standard Go pattern but creates type duplication.
• [helper-function-organization] internal/harness/compose.go:345 — matchingAllowedPrefix is a trivial one-line wrapper delegating to MatchingAllowedPrefixInList. Can be inlined at the single call site.
• [stale-doc] docs/plans/adr-0045-forge-portable-harness-schema.md:97 — Plan describes loadRaw as unexported; actual implementation exports it as LoadRaw.
• [missing-doc] docs/guides/user/customizing-agents.md — Does not document the new base field. Expected to be deferred until the feature is wired end-to-end in a later PR.

Info

• [authorization] internal/harness/compose.go:98 — Prior finding [fail-open] HIGH (self-authorization when OrgAllowlist is empty) has been remediated. URL bases are now rejected when OrgAllowlist is empty and AllowSelfAllowlist is false.
• [path-traversal] internal/harness/compose.go:207 — Prior finding [path-traversal] MEDIUM (directory containment) has been partially remediated. A containment check now exists but is conditional on WorkspaceRoot being non-empty.
• [privilege-escalation] internal/harness/compose.go:409 — Prior finding [privilege-escalation] MEDIUM (AllowedRemoteResources merged from base) has been fully remediated.

Previous run (5)

Review

Findings

High

• [fail-open] internal/harness/compose.go:98 — When OrgAllowlist is empty, the child harness's own AllowedRemoteResources is used as the allowlist for base URL fetching (lines 98–101). Since the child harness is attacker-controlled YAML, this is a self-authorization pattern: an attacker adds arbitrary URL prefixes to allowed_remote_resources and sets base to a URL matching that prefix, bypassing the intended org-level restriction. The code comment says "supports standalone testing without config.yaml" but this creates a production bypass when the org allowlist is simply not configured.
  Remediation: When OrgAllowlist is empty, reject URL bases entirely rather than falling back to the child's self-declared list. If standalone testing is needed, use an explicit opt-in flag like AllowSelfAuthorization rather than silently degrading.

Medium

• [path-traversal] internal/harness/compose.go:186 — Local base path resolution in loadBaseChain does not enforce directory containment. A relative base like base: ../../../../etc/some-harness.yaml resolves via filepath.Join(childDir, basePath) and is loaded without checking that the result stays within the workspace. Absolute paths are also loaded directly. The existing ResolveRelativeTo method has a containment check for other fields but is not applied to the base field.
  Remediation: After resolving the absolute path, verify it falls within opts.WorkspaceRoot or the child's directory tree. Also reject filepath.IsAbs(baseRef) unless it starts with the workspace root.

• [privilege-escalation] internal/harness/compose.go:390 — mergeHarness concatenates base.AllowedRemoteResources into child.AllowedRemoteResources. A URL-fetched base harness (whose content is integrity-pinned but whose author is a third party) can inject arbitrary URL prefixes into the merged harness's allowlist. Since ValidateAllowedRemoteResources is not called within LoadWithBase (deferred to the integration layer), there is no guarantee the injected entries are ever validated against the org allowlist.
  Remediation: Either do not merge AllowedRemoteResources from base harnesses, or call ValidateAllowedRemoteResources(opts.OrgAllowlist) within LoadWithBase after merging.

Low

• [logic-error] internal/harness/harness.go:252 — Neither Load() nor LoadWithOpts() process the base field — they silently ignore it. This is expected for PR 4/7 of a staged rollout, but a harness loaded via Load() with base: ../other.yaml will pass validation silently with no inheritance applied. Consider having Validate() reject a non-empty Base field until the integration PR wires LoadWithBase into the CLI.
• [logic-error] internal/harness/harness.go:403 — ResolveRelativeTo does not resolve the Base field. Currently LoadWithBase clears Base before returning so this is latent, but it violates the function's contract of resolving all relative paths.
• [architectural-conflict] internal/harness/compose.go:26 — Local Dependency type mirrors resolve.Dependency to avoid circular imports. This is a common Go pattern but creates type duplication requiring conversion at call sites.
• [edge-case] internal/harness/compose.go:170 — In the URL-base branch, the cycle check happens after the fetch. If a base chain references the same URL multiple times, content is fetched and parsed before cycle detection, wasting a network round-trip.
• [test-inadequate] internal/harness/compose_test.go:811 — TestLoadWithBase_DepthExceeded generates filenames using string(rune('0'+i)), which works for current MaxBaseDepth=5 but would produce malformed filenames if raised above 7. Use fmt.Sprintf("h%d.yaml", i) instead.
• [function-naming] internal/harness/compose.go:338 — mergeHarness mutates child but name doesn't indicate direction; forge.go uses mergeForgeConfigInto for similar semantics. Consider mergeBaseIntoChild.
• [function-naming] internal/harness/compose.go:322 — matchingAllowedPrefix creates a temporary Harness struct solely to call MatchingAllowedPrefix; consider extracting the matching logic into a standalone utility.
• [scope-alignment] internal/harness/compose.go:37 — ComposeOpts omits the HarnessAllowlist field from the implementation plan; reads child.AllowedRemoteResources directly instead.
• [stale-doc] docs/guides/dev/cli-internals.md:266 — Documents harness loading pipeline as LoadWithOpts only. Will need updating when PR 5 wires LoadWithBase into the CLI.
• [missing-doc] docs/guides/user/customizing-agents.md — Does not document the new base field. Will need updating when the feature is wired end-to-end.

Info

• [incomplete-integration] internal/harness/compose.go:73 — LoadWithBase is not yet wired into the CLI layer. Expected for PR 4/7 of a staged rollout.
• [scope-completeness] internal/harness/compose.go:228 — The implementation plan specified exporting a FetchAndVerify function; compose.go directly calls internal fetch package functions instead.
• [code-organization] internal/harness/compose.go:477 — mergeForgeConfigInto has similar logic to mergeForgeConfig in forge.go, though they operate on different target types (ForgeConfig vs Harness).

Previous run (6)

Review

Findings

High

• [fail-open] internal/harness/compose.go:98 — When OrgAllowlist is empty and the child harness has AllowedRemoteResources, the child's own list is used as the allowlist for base URL fetching. Since the child harness is untrusted YAML authored by a repository contributor, this is a self-authorization pattern: an attacker can add arbitrary URL prefixes to allowed_remote_resources in their harness file and then set base to that URL, bypassing the org-level allowlist entirely. The comment says this "supports standalone testing without config.yaml," but in production this is a fail-open vulnerability — the absence of an org allowlist should deny remote base fetches, not permit them.
  Remediation: Remove the fallback. When OrgAllowlist is empty and a URL base is used, return an error. If standalone testing needs this, gate it behind an explicit opt-in flag (e.g., ComposeOpts.AllowSelfAllowlist bool) that production callers never set.

Medium

• [path-traversal] internal/harness/compose.go:182 — Local base path resolution in loadBaseChain does not enforce directory containment. A relative base like base: ../../../../etc/some-harness.yaml or an absolute path will be loaded without restriction. By contrast, ResolveRelativeTo (harness.go:403) explicitly rejects paths that resolve outside the fullsend directory. The inconsistency leaves base resolution without the same directory-traversal protection.
  Remediation: Add a containment check similar to ResolveRelativeTo. After resolving absBasePath, verify it stays within the workspace root or the child's directory tree.

• [logic-error] internal/harness/harness.go:668 — Neither Load() nor LoadWithOpts() process the base field — they silently ignore it. A harness loaded via Load() with base: ../other.yaml will pass validation, but base composition is never applied. This is a silent misconfiguration bug: users who add base: to their harness YAML via the existing loading paths will get no error and no inheritance.
  Remediation: In Validate(), if h.Base != "", return an error like "base field is set but harness was not loaded with LoadWithBase; use LoadWithBase to enable base composition". This also covers the run.go and lock.go callers that still use LoadWithOpts until PR 5 wires them.

• [architectural-conflict] internal/harness/compose.go:26 — The implementation defines a local Dependency type that "mirrors resolve.Dependency but lives here to avoid circular imports." The implementation plan specifies that LoadWithBase should return []resolve.Dependency, not a local mirror type. This creates type duplication and will require conversion at call sites.
  Remediation: Evaluate whether the circular import concern is real (harness → resolve → harness). If it is, document the cycle. If it can be resolved (e.g., shared types package), align with the plan.

Low

• [scope-alignment] internal/harness/compose.go:37 — ComposeOpts omits the HarnessAllowlist field from the implementation plan; the implementation reads child.AllowedRemoteResources directly instead. Functionally equivalent but architecturally different.
• [defense-in-depth] internal/harness/harness.go:668 — The base field in ValidateResourceTypes is dead code in all current paths: LoadWithBase clears it before Validate(); Load/LoadWithOpts never process it.
• [allowlist-widening] internal/harness/compose.go:385 — mergeHarness concatenates the base's AllowedRemoteResources into the child's list. A base harness can inject additional URL prefixes into the merged allowlist. Document this design decision.
• [logic-error] internal/harness/compose.go:376 — Skills, plugins, and providers are concatenated without deduplication. Multi-level chains can produce duplicate entries. Document whether this is intentional or add dedup.
• [test-inadequate] internal/harness/compose_test.go:801 — TestLoadWithBase_DepthExceeded generates filenames using string(rune('0'+i)), which breaks for values ≥ 10. Use fmt.Sprintf("h%d.yaml", i).
• [edge-case] internal/harness/harness.go:403 — ResolveRelativeTo does not handle the Base field. Latent inconsistency if called before Base is consumed.
• [stale-doc] docs/guides/dev/cli-internals.md:266 — Documents harness loading pipeline as LoadWithOpts only. Will need updating when PR 5 wires LoadWithBase into the CLI.
• [missing-doc] docs/guides/user/customizing-agents.md — Does not document the new base field. Will need updating when the feature is wired end-to-end.
• [function-naming] internal/harness/compose.go:333 — mergeHarness mutates child but name doesn't indicate direction; forge.go uses mergeForgeConfigInto for similar semantics.
• [function-naming] internal/harness/compose.go:317 — matchingAllowedPrefix creates a temporary Harness struct solely to call MatchingAllowedPrefix; consider extracting the matching logic.

Info

• [scope-completeness] The implementation plan specified exporting a FetchAndVerify function in internal/resolve/resolve.go; compose.go directly uses the fetch package instead. Consider updating the plan to reflect this decision.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:39 PM UTC · Completed 3:51 PM UTC
Commit: 2affba7 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:03 PM UTC · Completed 4:15 PM UTC
Commit: eaadd7a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:37 PM UTC · Completed 4:51 PM UTC
Commit: 3694b84 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:37 PM UTC · Completed 5:49 PM UTC
Commit: c3f11d0 · View workflow run →

[fullsend-ai-review]

🤖 Review · Started 6:31 PM UTC
Commit: cc00b23 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:34 PM UTC · Completed 6:46 PM UTC
Commit: f3ccb43 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 11:48 AM UTC · Completed 11:59 AM UTC
Commit: f3ccb43 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2180 — feat(harness): add base composition for harness inheritance

Overall assessment: Workflow went well. The review agent delivered high-quality findings on a security-sensitive 1684-line change, catching a self-authorization bypass (fail-open when OrgAllowlist empty), path traversal in local base resolution, and privilege escalation via AllowedRemoteResources injection from URL-fetched bases. All high/medium findings were remediated by the author across 3 review iterations before approval.

What went well

• Review agent correctly identified 3 high/medium security findings that warranted remediation on a complex harness inheritance feature
• The author responded to each finding with clear "Fixed" or "Acknowledged" replies, and the review agent approved once remediations were verified
• A human reviewer also approved, confirming the review agent's judgment

Known waste patterns observed (already tracked)

• 22 no-op workflow runs from pull_request_review events that matched no dispatch stage — tracked by #1125
• 3 review runs dispatched from a single trigger at 18:31 UTC — tracked by #1452 and #981
• 2 concurrent retro dispatches on PR merge — tracked by #2190
• Findings re-raised after author response in iteration 2 — tracked by #1013

No new proposals — all improvement opportunities identified are already covered by existing open issues.