feat(run): add runtime agent resolution from config (ADR 0058 Phase 3)

[ggallen]

Summary

Implements Phase 3 of ADR 0058 — runtime agent resolution from config.

• Adds MergedAgents() helper that builds a merged agent set from scaffold defaults and config-registered agents, with config entries overriding scaffold entries by name (case-insensitive)
• Adds FetchAgentHarness() to fetch URL-sourced agent harnesses using the existing ADR 0038 fetch/cache/integrity infrastructure
• Updates fullsend run to resolve agents from the config agents list before falling back to disk-based harness lookup: URL sources are fetched and cached, local path sources are resolved relative to the fullsend directory
• Updates HarnessWrappersLayer to skip wrapper generation for config-driven agents (they are resolved at runtime, not via wrappers)
• 25+ new tests covering merge logic, URL agent resolution, local path resolution, scaffold fallback, config agent skip in wrapper layer

This builds on PR 1 (Phase 1) which added the config schema. Phase 2 (CLI) can proceed independently.

Test plan

• go build ./... succeeds
• go vet ./... passes
• make lint passes (all pre-commit hooks)
• All internal/config tests pass (including new MergedAgents tests)
• All internal/cli tests pass (including new config agent resolution tests)
• All internal/layers tests pass (including new config agent skip tests)
• All internal/harness tests pass
• New tests cover: MergedAgents override/append/empty/case-insensitive/sorted/nil-builder, FetchAgentHarness via URL, config agent local path, config agent URL, scaffold fallback, unknown agent error, wrapper skip for config agents

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Resolve fullsend run agents from config at runtime (ADR 0058 Phase 3)

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Resolve agent harnesses from config agents list before disk/scaffold fallback.
• Fetch and cache URL-based harnesses via existing ADR-0038 fetch/integrity pipeline.
• Skip harness-wrapper generation for config-driven agents; add extensive merge/resolve tests.

Diagram

graph TD
A["fullsend run"] --> B["Load config.yaml"] --> C{"Agent in config?"}
C -->|"No"| D["Resolve disk harness"] --> H["harness.LoadWithBase"]
C -->|"Yes"| E{"Source URL?"}
E -->|"URL"| F["Fetch+cache harness"] --> H
E -->|"Path"| G["Resolve local path"] --> H

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Generate wrappers for config agents (keep wrapper-centric install model)

• ➕ Keeps a single resolution path (always via .fullsend/harness/*.yaml wrappers)
• ➕ Avoids adding runtime decision logic in fullsend run
• ➖ Undermines the goal of runtime agent registration (config becomes a compile-time/install-time concern)
• ➖ Harder to support URL sources cleanly without duplicating fetch/cache behavior in wrapper generation
• ➖ More repo churn: wrapper files would need to be committed for every config agent

2. Move agent-source resolution into `internal/harness` as a reusable resolver

• ➕ Centralizes resolution logic (CLI and any future callers share one implementation)
• ➕ Makes allowlist + integrity enforcement harder to bypass accidentally
• ➖ Increases coupling between harness and config/scaffold concepts unless carefully abstracted
• ➖ May require additional interfaces/types to avoid circular dependencies

Recommendation: The PR’s approach (resolve config agents at runtime in run, reuse existing ADR-0038 fetch/cache, and explicitly skip wrapper generation for config-driven agents) is a good fit for ADR 0058 Phase 3. It minimizes duplication by leveraging the existing fetch infrastructure and keeps wrapper generation focused on scaffold defaults, while still preserving disk/scaffold fallback behavior when config does not define the requested agent.

Files changed (17) +1714 / -169

Enhancement (8) +475 / -52

admin.goPass config agent names into harness wrapper layer +1/-1

Pass config agent names into harness wrapper layer

• Extends 'NewHarnessWrappersLayer' construction to include derived agent names from config. Enables wrapper generation to skip config-driven agents.

internal/cli/admin.go

orgconfig.goAdd helper to derive agent names from config entries +9/-0

Add helper to derive agent names from config entries

• Introduces 'configAgentNames()' to extract 'DerivedName()' values from config 'AgentEntry' items. Used by admin install flow to inform wrapper generation skipping.

internal/cli/orgconfig.go

run.goResolve agent harness source from config before disk fallback +77/-24

Resolve agent harness source from config before disk fallback

• Refactors 'runAgent' to build 'ComposeOpts' first, then resolve harness path via 'resolveAgentSource()' using org config agents. Adds URL-fetch handling (with dependency reporting) and local-path resolution relative to the fullsend directory, preserving scaffold/disk fallback when no config agent matches.

internal/cli/run.go

agents.goImplement merged agent registry across scaffold and config agents +79/-0

Implement merged agent registry across scaffold and config agents

• Adds 'MergedAgent', 'MergedAgents()' (config-over-scaffold merge by case-insensitive name, sorted output, optional scaffold omission when builder/SHA missing), and 'LookupMergedAgent()' for case-insensitive lookup. This supports runtime resolution decisions in the CLI.

internal/config/agents.go

config.goAdd 'AgentEntry' schema, validation, and per-repo agent support +159/-12

Add 'AgentEntry' schema, validation, and per-repo agent support

• Introduces 'AgentEntry' with YAML shorthand/object forms, 'DerivedName()' naming, and strict rejection of legacy role/name/slug agent blocks. Adds agent lists and allowlists to org and per-repo configs, centralizes default allowlist via 'DefaultAllowedRemoteResources()', and validates agent entries for naming, uniqueness, URL integrity hash, allowlist coverage, https-only schemes, and safe local paths.

internal/config/config.go

compose.goAdd 'FetchAgentHarness' to reuse ADR-0038 fetch/cache for agent URLs +16/-2

Add 'FetchAgentHarness' to reuse ADR-0038 fetch/cache for agent URLs

• Clarifies 'ComposeOpts.OrgAllowlist' applies to both base URLs and agent source URLs. Adds 'FetchAgentHarness()' which fetches a URL harness with integrity verification and caching, returning a local cache path and dependency for reporting.

internal/harness/compose.go

harnesswrappers.goSkip wrapper generation/analysis for config-driven agents +30/-13

Skip wrapper generation/analysis for config-driven agents

• Extends 'HarnessWrappersLayer' to accept a set of config-registered agent names (case-insensitive) and skips wrapper generation and analyze expectations for those agents. This prevents generating redundant wrappers for agents resolved at runtime.

internal/layers/harnesswrappers.go

urlutil.goIntroduce shared URL utility package (validation, integrity hash, allowlist match) +104/-0

Introduce shared URL utility package (validation, integrity hash, allowlist match)

• Adds 'urlutil' as a leaf dependency to avoid circular imports. Provides 'IsURL', 'ParseIntegrityHash', 'NormalizeURLPath', and 'MatchingAllowedPrefixInList' for consistent URL handling across config and harness.

internal/urlutil/urlutil.go

Refactor (2) +10 / -87

harness.goUse shared URL normalization/allowlist utilities +6/-49

Use shared URL normalization/allowlist utilities

• Replaces local URL normalization logic with 'urlutil.NormalizeURLPath' and delegates allowlist matching to 'urlutil.MatchingAllowedPrefixInList'. Reduces duplication and aligns behavior with config validation.

internal/harness/harness.go

url.goDelegate URL detection and integrity-hash parsing to 'urlutil' +4/-38

Delegate URL detection and integrity-hash parsing to 'urlutil'

• Replaces in-package implementations of 'IsURL' and 'ParseIntegrityHash' with shared 'urlutil' equivalents. Ensures consistent URL validation rules across harness and config layers.

internal/harness/url.go

Tests (6) +1228 / -29

admin_test.goUpdate layer stack test for new wrapper-layer constructor signature +1/-1

Update layer stack test for new wrapper-layer constructor signature

• Adjusts 'TestCheckInstallScopes_SyncWithLayers' to pass the new 'configAgentNames' argument. Maintains test coverage for layer stack scope requirements.

internal/cli/admin_test.go

run_test.goAdd run-path tests for config agent local/URL resolution and fallback +204/-0

Add run-path tests for config agent local/URL resolution and fallback

• Adds tests covering config-driven local path agents, URL agents (with integrity hash), config override of scaffold agent names, scaffold/disk fallback when config doesn’t match, unknown agent behavior, and direct 'resolveAgentSource' cases including missing local paths.

internal/cli/run_test.go

agents_test.goTest merged agent behavior (override/append/sort/case-insensitivity) +162/-0

Test merged agent behavior (override/append/sort/case-insensitivity)

• Introduces comprehensive tests for 'MergedAgents' including scaffold-only, config-only, override semantics, append semantics, sorting, nil-builder/config-only mode, empty SHA behavior, and builder error propagation. Also tests 'LookupMergedAgent' for found/missing and case-insensitive matching.

internal/config/agents_test.go

config_test.goExpand config tests for agent parsing, validation, and round-trips +602/-4

Expand config tests for agent parsing, validation, and round-trips

• Updates legacy-agent test to assert rejection, and adds extensive coverage for 'AgentEntry' YAML forms, derived naming, marshal/unmarshal round-trips, agent entry validation edge cases, org/per-repo parsing/marshalling with agents, and default allowlist/default agent entry helpers.

internal/config/config_test.go

harnesswrappers_test.goUpdate wrapper layer tests and add coverage for config-agent skipping +67/-24

Update wrapper layer tests and add coverage for config-agent skipping

• Updates all constructor call sites for the new signature and adds tests verifying config-driven agents are excluded from both install wrapper creation and analyze output. Preserves existing wrapper generation/behavior coverage for non-config agents.

internal/layers/harnesswrappers_test.go

urlutil_test.goAdd unit tests for shared URL utilities +192/-0

Add unit tests for shared URL utilities

• Covers URL validation rules (https-only, no userinfo), integrity hash parsing/normalization, allowlist prefix matching with normalization/double-encoding rejection, and URL path normalization behavior.

internal/urlutil/urlutil_test.go

Documentation (1) +1 / -1

agent-registration.mdAlign doc plan with 'DerivedName()' helper naming +1/-1

Align doc plan with 'DerivedName()' helper naming

• Updates the agent registration plan to refer to 'AgentEntry.DerivedName()' instead of the old helper name. Keeps documentation consistent with the implemented config schema API.

docs/plans/agent-registration.md

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:32 AM UTC · Completed 3:47 AM UTC
Commit: afb34de · View workflow run →

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 51 rules

✅ Skills: writing-user-docs, writing-adrs

1. Config path escapes root 🐞 Bug ⛨ Security

Description

resolveAgentSource builds local harness paths with filepath.Join(fullsendDir, agent.Source),
which ignores fullsendDir when agent.Source is absolute/volume-qualified and can load harnesses
from outside the fullsend directory. Additionally, runAgent uses
tryLoadOrgConfig/ParseOrgConfig (no Validate() call), so the new agent-source validation is
not enforced at runtime (e.g., .. traversal can slip through).

Code

internal/cli/run.go[R2393-2396]

+	localPath := filepath.Join(fullsendDir, agent.Source)
+	if _, err := os.Stat(localPath); err != nil {
+		return "", nil, fmt.Errorf("config agent %s: local path %s: %w", agent.Name, agent.Source, err)
+	}

Relevance

⭐⭐⭐ High

Team frequently accepts path/validation hardening (symlink containment, input path validation) in
similar CLI security fixes.

PR-#1177
PR-#1780
PR-#215

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

runAgent loads config.yaml via tryLoadOrgConfig and immediately uses the parsed object for
agent resolution without calling Validate(). ParseOrgConfig only unmarshals YAML, so
validateAgentEntries is not applied at runtime. Then, for local config agents,
resolveAgentSource uses filepath.Join(fullsendDir, agent.Source), which will ignore
fullsendDir for absolute/volume paths, allowing harness resolution outside the intended directory
root.

internal/cli/run.go[185-216]
internal/cli/run.go[2355-2398]
internal/cli/orgconfig.go[20-37]
internal/config/config.go[239-246]
internal/config/config.go[306-350]

Agent prompt

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

## Issue description
Runtime agent resolution trusts `config.yaml` agent entries without enforcing the config validation rules, and it joins `fullsendDir` with `agent.Source` in a way that permits absolute (and potentially escaping) paths.

## Issue Context
- `runAgent` loads org config best-effort and then uses `orgCfg.Agents` for runtime harness resolution.
- Local-path config agents are supposed to resolve *relative to* the fullsend directory, but `filepath.Join(fullsendDir, agent.Source)` will return `agent.Source` unchanged when it is absolute (or drive-qualified on Windows).
- `ParseOrgConfig` currently only unmarshals YAML; it does not call `Validate()`, so `validateAgentEntries` is not enforced for `fullsend run`.

## Fix Focus Areas
- Enforce config validation before using `orgCfg.Agents` at runtime (either call `orgCfg.Validate()` after parsing in `tryLoadOrgConfig`/`requireOrgConfig`, or at least call `validateAgentEntries(orgCfg.Agents, orgCfg.AllowedRemoteResources)` before `resolveAgentSource`).
- In `resolveAgentSource`, require local-path sources to be relative and confined under `fullsendDir`:
 - Reject `filepath.IsAbs(agent.Source)` and Windows drive/volume paths (`filepath.VolumeName(agent.Source) != ""`).
 - Compute `resolved := filepath.Clean(filepath.Join(fullsendDir, agent.Source))` and verify containment with `filepath.Rel` (must not start with `..`).
 - Optionally reject directories (`stat.IsDir()`).

### Code references
- internal/cli/run.go[185-233]
- internal/cli/run.go[2355-2398]
- internal/cli/orgconfig.go[20-37]
- internal/config/config.go[239-246]
- internal/config/config.go[306-350]

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

---

[fullsend-ai-review]

Looks good to me.

All prior medium findings have been addressed:

• [scope-creep] Retry logic changes removed from PR scope
• [test-integrity] Retry delay test removal no longer in diff
• [path-traversal-symlink] containedLocalPath now calls filepath.EvalSymlinks and re-checks containment

Remaining low findings

• [edge-case] internal/cli/run.go — ValidateAgentEntries is called unconditionally when orgCfg has any agents, meaning a single malformed agent entry blocks ALL agent runs, including those that would resolve via disk-based fallback. Defensible fail-fast behavior but could surprise operators.

• [fail-open] internal/cli/run.go — When orgCfg is nil (config.yaml missing/unparseable), resolveAgentSource falls back to disk-based resolveHarnessPath without evaluating config-registered agents. Documented as intentional backward compatibility per ADR 0058.

• [test-integrity] internal/cli/run_test.go — TestResolveAgentSource_ConfigLocalPathAbsoluteRejected and TestResolveAgentSource_ConfigLocalPathTraversalRejected pass because ValidateAgentEntries catches errors first, not because containedLocalPath's defense-in-depth guards are exercised. The separate TestContainedLocalPath_* tests do cover the function directly.

• [test-coverage-gap] internal/cli/run_test.go — Tests cover happy paths for config agent resolution but don't test error cases like config validation failure in resolveAgentSource, URL fetch failure, or MergedAgents builder error.

• [incomplete-implementation] internal/layers/harnesswrappers.go — HarnessWrappersLayer skips wrapper generation for config agents but does not verify the config agent source exists before skipping. Validation happens elsewhere (ValidateAgentEntries for structure, resolveAgentSource for existence at runtime).

• [missing-authorization] No linked issue. The work is traceable to ADR 0058 and docs/plans/agent-registration.md, but non-trivial feature changes should have issue-level authorization.

• [naming-conventions] internal/config/config.go — ValidateAgentEntries is exported while other validation helpers remain unexported. Intentional for cross-package use in cli/run.go.

Previous run

Review

Findings

Medium

• [scope-creep] internal/forge/github/github.go:102 — GitHub client retry logic changes (maxRetries 5→3, jitter removal) are unrelated to agent registration (ADR 0058 Phase 3). These changes alter retry behavior and remove jitter from backoff, which is a separate concern from config-driven agent resolution.
  Remediation: Move GitHub client retry changes to a separate PR.

• [test-integrity] internal/forge/github/github_test.go — The diff removes all test coverage for retryDelay including TestRetryDelay_RespectsRetryAfterHeader, which validated that the Retry-After header is honored exactly. The Retry-After header parsing logic remains in retryDelay but is now untested. Jitter tests were correctly removed (jitter was removed from the implementation), but the HTTP contract test should be retained.
  Remediation: Keep or re-add a test verifying retryDelay returns the Retry-After header value when present, and a basic test that exponential backoff produces expected values (1s, 2s, 4s for attempts 0–2).

• [path-traversal-symlink] internal/cli/run.go:289 — containedLocalPath uses filepath.Clean and filepath.Rel to verify that a config-supplied local agent path stays within the fullsend directory, but it does not resolve symlinks (no filepath.EvalSymlinks call). If a symlink is placed within the fullsend directory pointing outside it, the containment check passes but the subsequent file read follows the symlink. Risk is mitigated by the requirement that an attacker would need write access to the fullsend directory.
  Remediation: After filepath.Clean, call filepath.EvalSymlinks on the resolved path, then re-check that the result is still within baseDir.

Low

• [missing-authorization] This PR has no linked issue. The work is traceable to ADR 0058 and the implementation plan at docs/plans/agent-registration.md, but non-trivial feature changes should have issue-level authorization.

• [undocumented-breaking-change] internal/scaffold/fullsend-repo/harness/triage.yaml — Harness scaffold templates migrated from env.runner/env.sandbox to top-level runner_env, with new .env files (triage.env, prioritize.env) replacing env.sandbox entries. This is an internal scaffold migration (not user-facing config), but the change should be noted.

• [documentation-correctness] docs/guides/getting-started/configuring-github.md:81 — Link text says 'Default Agents' but the target page docs/agents/README.md title was changed to 'Agents' in this PR.

• [documentation-correctness] docs/guides/getting-started/org-mode.md:198 — Same stale 'Default Agents' link text.

• [retry-behavior-change] internal/forge/github/github.go:103 — Retry behavior changed: maxRetries reduced from 5 to 3, jitter removed. Retries now use deterministic exponential backoff (1s, 2s, 4s). See also: [scope-creep] finding.

• [fail-open] internal/cli/run.go:231 — When orgCfg is nil or has no agents, resolveAgentSource falls back to disk-based resolveHarnessPath without validation. If config parsing silently fails via tryLoadOrgConfig, a config-registered agent silently falls back to a potentially different disk-based harness. Documented as intentional best-effort behavior.

• [test-integrity] internal/cli/run_test.go — TestResolveAgentSource_ConfigLocalPathAbsoluteRejected and TestResolveAgentSource_ConfigLocalPathTraversalRejected pass because ValidateAgentEntries catches errors first, not because containedLocalPath's defense-in-depth guards are exercised. The separate TestContainedLocalPath_* tests do cover the function directly.

• [test-coverage-gap] internal/cli/run_test.go — Tests cover happy paths for config agent resolution but don't test error cases like config validation failure in resolveAgentSource or URL fetch failure.

• [naming-conventions] internal/config/config.go:310 — ValidateAgentEntries is exported while other validation helpers (validateStatusNotifications, validateCreateIssues) remain unexported. The export is intentional for cross-package use in cli/run.go.

• [edge-case] internal/cli/run.go — In resolveAgentSource, ValidateAgentEntries is called unconditionally when orgCfg has agents, meaning any invalid agent entry blocks ALL agent runs — even those that would fall back to disk.

• [incomplete-implementation] internal/layers/harnesswrappers.go — HarnessWrappersLayer accepts configAgentNames to skip wrapper generation, but there's no verification that the config agent actually exists before skipping. Validation happens elsewhere (ValidateAgentEntries for structure, resolveAgentSource for existence at runtime).

Previous run (2)

Review

Findings

Low

• [documentation-correctness] docs/runtimes.md:47 — The diff changes the comment from AgentName() to DerivedName(), but the runtime code that determines the sandbox agent filename uses input.AgentName() (from BootstrapInput interface in internal/runtime/bootstrap.go), not config.AgentEntry.DerivedName(). Both resolve to the same string at runtime, so the practical impact is zero, but the method reference names a different type.
  Remediation: Remove the method reference entirely and say "filename derived from the agent name" to avoid citing a specific method on a specific type.

• [edge-case] internal/cli/run.go — In resolveAgentSource, ValidateAgentEntries is called unconditionally when orgCfg has agents, meaning any invalid agent entry in the config blocks ALL agent runs — even those that would fall back to disk because the requested agent is not in the config list.

• [test-integrity] internal/cli/run_test.go — TestResolveAgentSource_ConfigLocalPathAbsoluteRejected and TestResolveAgentSource_ConfigLocalPathTraversalRejected pass because ValidateAgentEntries catches errors first, not because resolveAgentSource's defense-in-depth guards (filepath.IsAbs and filepath.Rel checks) are exercised. Consider adding tests that bypass validation to verify the runtime guards directly.

• [error-message-consistency] internal/cli/run.go — Error messages for absolute path and path traversal rejection in resolveAgentSource use different wording from ValidateAgentEntries in config.go. These are defense-in-depth guards unlikely to be reached in practice.

• [error-message-format] internal/harness/compose.go — FetchAgentHarness error wraps with generic "fetching agent harness" without the URL, unlike the established pattern in fetchBaseURL which includes the URL in error messages.

• [naming-conventions] internal/config/config.go — ValidateAgentEntries export breaks convention where other validation helpers (validateStatusNotifications, validateCreateIssues) remain unexported. The export is intentional for cross-package use in cli/run.go.

Previous run (3)

Review

Findings

Medium

• [stale-doc] docs/agents/README.md — Lines 24-27 state "Support for adding your own custom agents to the fullsend pipeline is coming soon" but this PR delivers the feature (ADR 0058 Phase 3 runtime agent resolution from config). Lines 4-5 also describe scaffold YAML files as the sole source of agents, which is now incomplete.
  Remediation: Update the "Custom Agents" section to document the agents: config field and update the introduction to reflect the merged agent model (scaffold defaults + config overlay).

Low

• [missing-authorization] This PR has no linked issue. The work is traceable to ADR 0058 and docs/plans/agent-registration.md, but non-trivial changes should have issue-level authorization.

• [edge-case] internal/cli/run.go — In resolveAgentSource, ValidateAgentEntries is called unconditionally when orgCfg has agents, meaning any invalid agent entry blocks ALL agent runs — even those that would fall back to disk.

• [test-integrity] internal/cli/run_test.go — TestResolveAgentSource_ConfigLocalPathAbsoluteRejected and TestResolveAgentSource_ConfigLocalPathTraversalRejected pass because ValidateAgentEntries catches errors first, not because resolveAgentSource's defense-in-depth guards are exercised. Consider adding tests that bypass validation to verify the runtime guards directly.

• [naming-coherence] docs/runtimes.md — DerivedName() returns the explicit Name if set, otherwise derives from Source filename. The method name implies derivation always occurs. Introduced in Phase 1 (PR feat(config): add agent registration schema (ADR 0058 Phase 1) #2768), not in scope for this PR.

• [error-message-consistency] internal/cli/run.go — Error messages for absolute path and path traversal rejection use different wording from ValidateAgentEntries in config.go. These are defense-in-depth guards unlikely to be reached in practice.

• [error-message-format] internal/harness/compose.go — FetchAgentHarness error wraps with generic "fetching agent harness" without the URL, unlike the established pattern in fetchBaseURL.

• [architectural-coherence] internal/cli/run.go — resolveAgentSource builds the full scaffold set unconditionally. Phase 5 dependency — consider logging when scaffold fallback is used.

• [architectural-coherence] internal/config/agents.go — Case-insensitive collision preserves config-specified casing. If downstream code assumes lowercase harness filenames, this could cause issues.

• [naming-conventions] internal/config/config.go — ValidateAgentEntries export breaks convention where other validation helpers (validateStatusNotifications, validateCreateIssues) remain unexported.

• [api-shape-pattern] internal/layers/harnesswrappers.go — configAgentNames added as constructor parameter. Other layers use builder pattern (WithOIDCMode, WithScaffoldPending) for optional config.

• [missing-doc] internal/harness/compose.go — FetchAgentHarness doc comment references ADR 0038 but not ADR 0058.

• [missing-doc] internal/cli/run.go — resolveAgentSource doc comment lacks ADR 0058 reference.

Previous run (4)

Review

Findings

Low

• [edge-case] internal/cli/run.go — In resolveAgentSource, ValidateAgentEntries is called unconditionally when orgCfg has agents, meaning any invalid agent entry blocks ALL agent runs — even those that would fall back to disk. Defensible fail-fast behavior but could surprise operators who add a misconfigured entry and find unrelated agents stop working.

• [Missing Validation at Runtime] internal/cli/orgconfig.go — tryLoadOrgConfig does not call cfg.Validate() after ParseOrgConfig. Mitigated: resolveAgentSource independently calls ValidateAgentEntries before agent resolution, closing the security-critical path.

• [Fail-Open on Nil OrgConfig] internal/cli/run.go — When orgCfg is nil or has no agents, resolveAgentSource falls back to disk-based lookup, silently skipping config-driven validation. Intentional backward compatibility per ADR 0058 design ("no forced migration").

• [Security Config Inheritance] internal/harness/compose.go:492 — Pre-existing: child harnesses with no Security block inherit the base's config, potentially inheriting a weaker security posture. Not changed by this PR but agent registration increases base composition surface area.

• [doc-comment-completeness] internal/harness/compose.go — FetchAgentHarness doc comment references ADR 0038 but should also reference ADR 0058 since this function is introduced as part of agent registration.

• [doc-comment-completeness] internal/cli/orgconfig.go — tryLoadOrgConfig and requireOrgConfig doc comments could reference ADRs 0038/0045 for context on the remote resource validation they support.

Previous run (5)

Review

Findings

Medium

• [Path Traversal] internal/cli/run.go — In resolveAgentSource, when agent.Source is a local path, the code performs filepath.Join(fullsendDir, agent.Source) without runtime validation that the source path does not contain path traversal segments (..). The validateAgentEntries function checks for .. but is only invoked via OrgConfig.Validate() or PerRepoConfig.Validate() — neither of which is called by tryLoadOrgConfig (the runtime config loader). An attacker who can modify the org config YAML could insert a path-traversal source to load an arbitrary file as a harness.
  Remediation: Add a runtime containment check in resolveAgentSource for local-path agents. After filepath.Join, verify the resolved path is within fullsendDir using filepath.Rel.

Low

• [Missing Validation at Runtime] internal/cli/orgconfig.go — tryLoadOrgConfig calls config.ParseOrgConfig but does not call cfg.Validate(). The new Agents field is consumed at runtime in resolveAgentSource, which relies on structural invariants enforced by validateAgentEntries. This means all security checks — path traversal rejection, HTTPS enforcement, integrity hash requirement, allowlist membership — are bypassed at runtime. See also: [Path Traversal] finding above (same root cause).
  Remediation: Either call cfg.Validate() within tryLoadOrgConfig or add targeted validation of cfg.Agents entries in resolveAgentSource before using them.

• [Missing Absolute Path Check] internal/config/config.go — validateAgentEntries validates local-path agent sources by checking for backslashes and .. segments but does not reject absolute paths. While filepath.Join("/base", "/etc/passwd") does NOT escape in Go, an absolute path source would resolve to an unintended location relative to fullsendDir.
  Remediation: Add filepath.IsAbs(entry.Source) check to the local-path branch, consistent with validateBaseRelPath.

• [error handling] internal/cli/run.go — In resolveAgentSource, the error from scaffold.HarnessNames() is silently discarded: scaffoldNames, _ := scaffold.HarnessNames(). A corrupt binary would silently produce a merged agent set with only config agents.

• [stale-reference-renamed-method] docs/runtimes.md:47 — Documentation references AgentName() which was renamed to DerivedName() in the implementation plan.

• [stale-package-reference] docs/plans/universal-harness-access.md — References harness.IsURL() at lines 1078, 1104. While harness.IsURL() still exists as a thin wrapper delegating to urlutil.IsURL(), the canonical implementation moved to internal/urlutil. Also docs/plans/universal-harness-access-phase2.md:187.

• [stale-function-signature] docs/plans/adr-0045-forge-portable-harness-phase2.md:228 — Shows NewHarnessWrappersLayer with 5 parameters; the function now takes a 6th parameter configAgentNames []string.

• [function-naming-consistency] internal/cli/run.go — resolveAgentSource uses the 'resolve' prefix which the codebase also uses for resolveHarnessPath, so this is consistent. However, given that the function selects between config-driven and disk-based resolution, a name like lookupAgentHarness might be slightly clearer.

• [doc-comment-completeness] internal/harness/compose.go — FetchAgentHarness doc comment references ADR 0038 but should also reference ADR 0058 since this function is introduced as part of agent registration.

---

Labels: PR modifies internal/cli (run command), internal/config (agent schema), internal/harness (agent fetch), and internal/layers (wrapper generation) for config-driven agent resolution feature

Previous run (6)

Review

Findings

Low

• [documentation-correctness] docs/runtimes.md:47 — The diff changes the comment from AgentName() to DerivedName(), but the runtime code that determines the sandbox agent filename uses input.AgentName() (from BootstrapInput interface in internal/runtime/bootstrap.go), not config.AgentEntry.DerivedName(). Both resolve to the same string at runtime, so the practical impact is zero, but the method reference names a different type.
  Remediation: Remove the method reference entirely and say "filename derived from the agent name" to avoid citing a specific method on a specific type.

• [edge-case] internal/cli/run.go — In resolveAgentSource, ValidateAgentEntries is called unconditionally when orgCfg has agents, meaning any invalid agent entry in the config blocks ALL agent runs — even those that would fall back to disk because the requested agent is not in the config list.

• [test-integrity] internal/cli/run_test.go — TestResolveAgentSource_ConfigLocalPathAbsoluteRejected and TestResolveAgentSource_ConfigLocalPathTraversalRejected pass because ValidateAgentEntries catches errors first, not because resolveAgentSource's defense-in-depth guards (filepath.IsAbs and filepath.Rel checks) are exercised. Consider adding tests that bypass validation to verify the runtime guards directly.

• [error-message-consistency] internal/cli/run.go — Error messages for absolute path and path traversal rejection in resolveAgentSource use different wording from ValidateAgentEntries in config.go. These are defense-in-depth guards unlikely to be reached in practice.

• [error-message-format] internal/harness/compose.go — FetchAgentHarness error wraps with generic "fetching agent harness" without the URL, unlike the established pattern in fetchBaseURL which includes the URL in error messages.

• [naming-conventions] internal/config/config.go — ValidateAgentEntries export breaks convention where other validation helpers (validateStatusNotifications, validateCreateIssues) remain unexported. The export is intentional for cross-package use in cli/run.go.

Previous run (7)

Review

Findings

Medium

• [stale-doc] docs/agents/README.md — Lines 24-27 state "Support for adding your own custom agents to the fullsend pipeline is coming soon" but this PR delivers the feature (ADR 0058 Phase 3 runtime agent resolution from config). Lines 4-5 also describe scaffold YAML files as the sole source of agents, which is now incomplete.
  Remediation: Update the "Custom Agents" section to document the agents: config field and update the introduction to reflect the merged agent model (scaffold defaults + config overlay).

Low

• [missing-authorization] This PR has no linked issue. The work is traceable to ADR 0058 and docs/plans/agent-registration.md, but non-trivial changes should have issue-level authorization.

• [edge-case] internal/cli/run.go — In resolveAgentSource, ValidateAgentEntries is called unconditionally when orgCfg has agents, meaning any invalid agent entry blocks ALL agent runs — even those that would fall back to disk.

• [test-integrity] internal/cli/run_test.go — TestResolveAgentSource_ConfigLocalPathAbsoluteRejected and TestResolveAgentSource_ConfigLocalPathTraversalRejected pass because ValidateAgentEntries catches errors first, not because resolveAgentSource's defense-in-depth guards are exercised. Consider adding tests that bypass validation to verify the runtime guards directly.

• [naming-coherence] docs/runtimes.md — DerivedName() returns the explicit Name if set, otherwise derives from Source filename. The method name implies derivation always occurs. Introduced in Phase 1 (PR feat(config): add agent registration schema (ADR 0058 Phase 1) #2768), not in scope for this PR.

• [error-message-consistency] internal/cli/run.go — Error messages for absolute path and path traversal rejection use different wording from ValidateAgentEntries in config.go. These are defense-in-depth guards unlikely to be reached in practice.

• [error-message-format] internal/harness/compose.go — FetchAgentHarness error wraps with generic "fetching agent harness" without the URL, unlike the established pattern in fetchBaseURL.

• [architectural-coherence] internal/cli/run.go — resolveAgentSource builds the full scaffold set unconditionally. Phase 5 dependency — consider logging when scaffold fallback is used.

• [architectural-coherence] internal/config/agents.go — Case-insensitive collision preserves config-specified casing. If downstream code assumes lowercase harness filenames, this could cause issues.

• [naming-conventions] internal/config/config.go — ValidateAgentEntries export breaks convention where other validation helpers (validateStatusNotifications, validateCreateIssues) remain unexported.

• [api-shape-pattern] internal/layers/harnesswrappers.go — configAgentNames added as constructor parameter. Other layers use builder pattern (WithOIDCMode, WithScaffoldPending) for optional config.

• [missing-doc] internal/harness/compose.go — FetchAgentHarness doc comment references ADR 0038 but not ADR 0058.

• [missing-doc] internal/cli/run.go — resolveAgentSource doc comment lacks ADR 0058 reference.

Previous run (8)

Review

Findings

Low

• [edge-case] internal/cli/run.go — In resolveAgentSource, ValidateAgentEntries is called unconditionally when orgCfg has agents, meaning any invalid agent entry blocks ALL agent runs — even those that would fall back to disk. Defensible fail-fast behavior but could surprise operators who add a misconfigured entry and find unrelated agents stop working.

• [Missing Validation at Runtime] internal/cli/orgconfig.go — tryLoadOrgConfig does not call cfg.Validate() after ParseOrgConfig. Mitigated: resolveAgentSource independently calls ValidateAgentEntries before agent resolution, closing the security-critical path.

• [Fail-Open on Nil OrgConfig] internal/cli/run.go — When orgCfg is nil or has no agents, resolveAgentSource falls back to disk-based lookup, silently skipping config-driven validation. Intentional backward compatibility per ADR 0058 design ("no forced migration").

• [Security Config Inheritance] internal/harness/compose.go:492 — Pre-existing: child harnesses with no Security block inherit the base's config, potentially inheriting a weaker security posture. Not changed by this PR but agent registration increases base composition surface area.

• [doc-comment-completeness] internal/harness/compose.go — FetchAgentHarness doc comment references ADR 0038 but should also reference ADR 0058 since this function is introduced as part of agent registration.

• [doc-comment-completeness] internal/cli/orgconfig.go — tryLoadOrgConfig and requireOrgConfig doc comments could reference ADRs 0038/0045 for context on the remote resource validation they support.

Previous run

Review

Findings

Medium

• [Path Traversal] internal/cli/run.go — In resolveAgentSource, when agent.Source is a local path, the code performs filepath.Join(fullsendDir, agent.Source) without runtime validation that the source path does not contain path traversal segments (..). The validateAgentEntries function checks for .. but is only invoked via OrgConfig.Validate() or PerRepoConfig.Validate() — neither of which is called by tryLoadOrgConfig (the runtime config loader). An attacker who can modify the org config YAML could insert a path-traversal source to load an arbitrary file as a harness.
  Remediation: Add a runtime containment check in resolveAgentSource for local-path agents. After filepath.Join, verify the resolved path is within fullsendDir using filepath.Rel.

Low

• [Missing Validation at Runtime] internal/cli/orgconfig.go — tryLoadOrgConfig calls config.ParseOrgConfig but does not call cfg.Validate(). The new Agents field is consumed at runtime in resolveAgentSource, which relies on structural invariants enforced by validateAgentEntries. This means all security checks — path traversal rejection, HTTPS enforcement, integrity hash requirement, allowlist membership — are bypassed at runtime. See also: [Path Traversal] finding above (same root cause).
  Remediation: Either call cfg.Validate() within tryLoadOrgConfig or add targeted validation of cfg.Agents entries in resolveAgentSource before using them.

• [Missing Absolute Path Check] internal/config/config.go — validateAgentEntries validates local-path agent sources by checking for backslashes and .. segments but does not reject absolute paths. While filepath.Join("/base", "/etc/passwd") does NOT escape in Go, an absolute path source would resolve to an unintended location relative to fullsendDir.
  Remediation: Add filepath.IsAbs(entry.Source) check to the local-path branch, consistent with validateBaseRelPath.

• [error handling] internal/cli/run.go — In resolveAgentSource, the error from scaffold.HarnessNames() is silently discarded: scaffoldNames, _ := scaffold.HarnessNames(). A corrupt binary would silently produce a merged agent set with only config agents.

• [stale-reference-renamed-method] docs/runtimes.md:47 — Documentation references AgentName() which was renamed to DerivedName() in the implementation plan.

• [stale-package-reference] docs/plans/universal-harness-access.md — References harness.IsURL() at lines 1078, 1104. While harness.IsURL() still exists as a thin wrapper delegating to urlutil.IsURL(), the canonical implementation moved to internal/urlutil. Also docs/plans/universal-harness-access-phase2.md:187.

• [stale-function-signature] docs/plans/adr-0045-forge-portable-harness-phase2.md:228 — Shows NewHarnessWrappersLayer with 5 parameters; the function now takes a 6th parameter configAgentNames []string.

• [function-naming-consistency] internal/cli/run.go — resolveAgentSource uses the 'resolve' prefix which the codebase also uses for resolveHarnessPath, so this is consistent. However, given that the function selects between config-driven and disk-based resolution, a name like lookupAgentHarness might be slightly clearer.

• [doc-comment-completeness] internal/harness/compose.go — FetchAgentHarness doc comment references ADR 0038 but should also reference ADR 0058 since this function is introduced as part of agent registration.

---

Labels: PR modifies internal/cli (run command), internal/config (agent schema), internal/harness (agent fetch), and internal/layers (wrapper generation) for config-driven agent resolution feature

Previous run (9)

Review

Findings

Low

• [edge-case] internal/cli/run.go — In resolveAgentSource, ValidateAgentEntries is called unconditionally when orgCfg has agents, meaning any invalid agent entry blocks ALL agent runs — even those that would fall back to disk. Defensible fail-fast behavior but could surprise operators who add a misconfigured entry and find unrelated agents stop working.

• [Missing Validation at Runtime] internal/cli/orgconfig.go — tryLoadOrgConfig does not call cfg.Validate() after ParseOrgConfig. Mitigated: resolveAgentSource independently calls ValidateAgentEntries before agent resolution, closing the security-critical path.

• [Fail-Open on Nil OrgConfig] internal/cli/run.go — When orgCfg is nil or has no agents, resolveAgentSource falls back to disk-based lookup, silently skipping config-driven validation. Intentional backward compatibility per ADR 0058 design ("no forced migration").

• [Security Config Inheritance] internal/harness/compose.go:492 — Pre-existing: child harnesses with no Security block inherit the base's config, potentially inheriting a weaker security posture. Not changed by this PR but agent registration increases base composition surface area.

• [doc-comment-completeness] internal/harness/compose.go — FetchAgentHarness doc comment references ADR 0038 but should also reference ADR 0058 since this function is introduced as part of agent registration.

• [doc-comment-completeness] internal/cli/orgconfig.go — tryLoadOrgConfig and requireOrgConfig doc comments could reference ADRs 0038/0045 for context on the remote resource validation they support.

Previous run (10)

Review

Findings

Medium

• [Path Traversal] internal/cli/run.go — In resolveAgentSource, when agent.Source is a local path, the code performs filepath.Join(fullsendDir, agent.Source) without runtime validation that the source path does not contain path traversal segments (..). The validateAgentEntries function checks for .. but is only invoked via OrgConfig.Validate() or PerRepoConfig.Validate() — neither of which is called by tryLoadOrgConfig (the runtime config loader). An attacker who can modify the org config YAML could insert a path-traversal source to load an arbitrary file as a harness.
  Remediation: Add a runtime containment check in resolveAgentSource for local-path agents. After filepath.Join, verify the resolved path is within fullsendDir using filepath.Rel.

Low

• [Missing Validation at Runtime] internal/cli/orgconfig.go — tryLoadOrgConfig calls config.ParseOrgConfig but does not call cfg.Validate(). The new Agents field is consumed at runtime in resolveAgentSource, which relies on structural invariants enforced by validateAgentEntries. This means all security checks — path traversal rejection, HTTPS enforcement, integrity hash requirement, allowlist membership — are bypassed at runtime. See also: [Path Traversal] finding above (same root cause).
  Remediation: Either call cfg.Validate() within tryLoadOrgConfig or add targeted validation of cfg.Agents entries in resolveAgentSource before using them.

• [Missing Absolute Path Check] internal/config/config.go — validateAgentEntries validates local-path agent sources by checking for backslashes and .. segments but does not reject absolute paths. While filepath.Join("/base", "/etc/passwd") does NOT escape in Go, an absolute path source would resolve to an unintended location relative to fullsendDir.
  Remediation: Add filepath.IsAbs(entry.Source) check to the local-path branch, consistent with validateBaseRelPath.

• [error handling] internal/cli/run.go — In resolveAgentSource, the error from scaffold.HarnessNames() is silently discarded: scaffoldNames, _ := scaffold.HarnessNames(). A corrupt binary would silently produce a merged agent set with only config agents.

• [stale-reference-renamed-method] docs/runtimes.md:47 — Documentation references AgentName() which was renamed to DerivedName() in the implementation plan.

• [stale-package-reference] docs/plans/universal-harness-access.md — References harness.IsURL() at lines 1078, 1104. While harness.IsURL() still exists as a thin wrapper delegating to urlutil.IsURL(), the canonical implementation moved to internal/urlutil. Also docs/plans/universal-harness-access-phase2.md:187.

• [stale-function-signature] docs/plans/adr-0045-forge-portable-harness-phase2.md:228 — Shows NewHarnessWrappersLayer with 5 parameters; the function now takes a 6th parameter configAgentNames []string.

• [function-naming-consistency] internal/cli/run.go — resolveAgentSource uses the 'resolve' prefix which the codebase also uses for resolveHarnessPath, so this is consistent. However, given that the function selects between config-driven and disk-based resolution, a name like lookupAgentHarness might be slightly clearer.

• [doc-comment-completeness] internal/harness/compose.go — FetchAgentHarness doc comment references ADR 0038 but should also reference ADR 0058 since this function is introduced as part of agent registration.

---

Labels: PR modifies internal/cli (run command), internal/config (agent schema), internal/harness (agent fetch), and internal/layers (wrapper generation) for config-driven agent resolution feature

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 4:12 AM UTC · Completed 4:35 AM UTC
Commit: 1ab3afe · View workflow run →

[github-actions]

Site preview

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

Commit: 9a39f2ec1888d71d091057109c1025f9f60640ad

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 11:38 AM UTC · Ended 11:54 AM UTC
Commit: 104508d · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 85.15625% with 19 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/run.go	81.25%	7 Missing and 5 partials ⚠️
internal/harness/compose.go	0.00%	5 Missing ⚠️
internal/cli/orgconfig.go	60.00%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:38 AM UTC · Completed 11:54 AM UTC
Commit: 825d21b · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 11:59 AM UTC · Completed 12:22 PM UTC
Commit: f360b5a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 12:31 PM UTC · Completed 12:49 PM UTC
Commit: 89c93d1 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:02 PM UTC · Completed 1:23 PM UTC
Commit: 9a39f2e · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 8:53 PM UTC · Completed 9:01 PM UTC
Commit: 9a39f2e · View workflow run →

[fullsend-ai-retro]

Retro: PR #2769 — Runtime agent resolution from config (ADR 0058 Phase 3)

Timeline

Time (UTC)	Event
03:28	PR created by ggallen (human-authored, 16 files, +773/-77)
03:32–03:47	Review run 1: ✅ Success — substantive review, verified prior findings addressed, identified 4 low findings
03:34	Qodo bot finds real security bug: filepath.Join ignores fullsendDir when agent.Source is absolute, enabling path traversal
04:07	Author fixes security issue with 3 layers of defense (absolute path rejection, traversal check, symlink escape check)
04:12–04:35	Review run 2: ❌ Failure
09:03	Human reviewer (rh-hemartin) approves — no comments
11:38–11:54	Review run 3: ⚠ Terminated
11:38–11:54	Review run 4: ✅ Success — low naming-coherence finding (out of scope)
11:59–12:22	Review run 5: ❌ Failure
12:31–12:49	Review run 6: ❌ Failure
13:02–13:23	Review run 7: ✅ Success — approved
20:48	Merged by ggallen

What went well

• Review quality: When review runs succeeded, the output was high quality. The bot correctly tracked prior medium findings as resolved, scoped the naming-coherence finding as out-of-PR, and identified defensible-but-surprising fail-fast behavior.
• Security fix: The author's response to Qodo's finding was thorough — three independent defense layers with dedicated tests for each.
• Test coverage: 25+ new tests covering merge logic, path containment, URL resolution, scaffold fallback, and wrapper skip behavior.

What could go better

1. Review run waste: 4 of 7 review runs produced no useful output (3 failures, 1 terminated). Without access to run logs (auth limitations prevented inspection), the root causes are unclear — likely a mix of concurrency cancellation and infrastructure issues. This is a known area with extensive existing coverage: #1422 (dedup on rapid rebases), #1014 (debounce dispatch), #2599 (per-PR review budget), #2711 (auto re-dispatch after infra failures).

2. Security finding gap: Qodo caught a path traversal vulnerability (filepath.Join ignoring base when source is absolute) that the fullsend review bot did not flag. The bot's first review noted that containedLocalPath already called filepath.EvalSymlinks (symlink defense), but missed the orthogonal absolute-path bypass vector. This is covered by #1622 (detect filesystem security anti-patterns including path traversal).

3. Effective review runs: Of the 3 successful runs, only 2 produced genuinely useful output (run 1: initial findings, run 7: approval). Run 4 flagged a naming issue explicitly out of scope for this PR.

Proposals

No new proposals. All identified improvement areas are covered by existing open issues:

• Review dispatch waste → Deduplicate review runs when PR is rebased multiple times in quick succession #1422, Debounce review dispatch on rapid synchronize events #1014, Add per-PR review budget to cap runaway review dispatches #2599, Auto re-dispatch review after consecutive infrastructure failures with backoff #2711
• Security pattern detection → Review agent should detect filesystem security anti-patterns (TOCTOU, symlink, path traversal) #1622
• Review on large architectural PRs → Review agent misses security-critical findings on large architectural PRs #898, Review agent: two-pass strategy for large PRs — triage security-critical files, then deep-review #2096

This PR represents a well-executed human-authored workflow with solid code quality. The main inefficiency was review dispatch volume, which is a systemic issue already being tracked across multiple fronts.