feat(config): add agent registration schema (ADR 0058 Phase 1)

[ggallen]

Summary

Implements Phase 1 of ADR 0058 — config schema for agent registration.

• Adds AgentEntry type with custom YAML unmarshaler supporting string shorthand (source-url) and object form ({name, source})
• Adds Agents and AllowedRemoteResources fields to both OrgConfig and PerRepoConfig
• Validates URL entries require #sha256= integrity fragment, HTTPS scheme, and allowlist prefix
• Validates local path entries reject .. traversal
• Validates agent names (derived from filename) are unique
• Rejects legacy role/name/slug agent entries with clear error (previously silently ignored)
• Seeds DefaultAllowedRemoteResources() in NewPerRepoConfig
• Adds DefaultAgentEntries() builder for install-time default URL computation
• 40+ new tests covering parse/marshal round-trip, validation rules, name derivation, and defaults

This is the foundation PR — Phase 2 (CLI) and Phase 3 (runtime resolution) can begin in parallel after this merges.

Test plan

• All existing internal/config tests pass (including updated legacy agents test)
• All existing internal/harness tests pass
• Full go build ./... succeeds
• New tests cover: string shorthand parsing, object form parsing, mixed forms, AgentName derivation (explicit, filename, URL), marshal round-trip, duplicate name rejection, missing hash rejection, non-HTTPS rejection, allowlist enforcement, path traversal rejection, empty source rejection, invalid hash length/chars, per-repo agents/allowlist, DefaultAgentEntries builder

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Add agent registration config schema with validation and defaults (ADR 0058 P1)

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Introduce agents config schema for org and per-repo installs (string or object forms).
• Enforce secure/pinned agent sources (https, sha256 fragment, allowlist, no path traversal).
• Add defaults and extensive round-trip/validation test coverage for agent entries.

Diagram

graph TD
  A["config.yaml"] --> B["ParseOrg/PerRepoConfig"] --> C["AgentEntry YAML unmarshal"] --> D["validateAgentEntries"] --> E["Validated Org/Repo config"] --> F["Harness compose (allowlist)"]
  G["DefaultAllowedRemoteResources"] --> B
  H["DefaultAgentEntries (builder)"] --> B

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use net/url parsing + explicit fragment handling

• ➕ More robust URL validation than prefix/string checks (handles edge cases consistently)
• ➕ Makes it clearer which parts are scheme/host/path/fragment for future phases
• ➖ Slightly more code and test updates now
• ➖ Still need custom allowlist-prefix semantics (path prefix checks)

2. Normalize local paths via filepath.Clean and then validate

• ➕ Simplifies traversal detection and normalizes redundant separators
• ➕ Avoids manual split/segment iteration
• ➖ Care needed to avoid inadvertently accepting absolute paths or OS-specific quirks
• ➖ May change behavior for some currently-accepted relative paths

Recommendation: The PR’s approach is a good Phase-1 foundation: it introduces a minimal schema and enforces critical security constraints (pinned hashes + allowlist) close to parsing. Consider migrating the URL/path checks to net/url + path normalization in a follow-up if Phase 2/3 expands supported source formats, but there’s no strong reason to block this PR on that refactor.

Files changed (3) +664 / -18

Enhancement (1) +172 / -12

config.goAdd AgentEntry schema, defaults, and validation for agent registration +172/-12

Add AgentEntry schema, defaults, and validation for agent registration

• Introduces 'AgentEntry' with a custom YAML unmarshaler supporting string shorthand and mapping form, while rejecting legacy role/name/slug agent entries. Adds 'agents' and 'allowed_remote_resources' support to both OrgConfig and PerRepoConfig, seeds defaults, and enforces validation (https + #sha256 fragment, allowlist prefix, no path traversal, unique derived names). Also adds a 'DefaultAgentEntries' helper for building pinned default agent URLs via an injected builder.

internal/config/config.go

Tests (1) +488 / -4

config_test.goAdd comprehensive AgentEntry parsing/validation/defaults test coverage +488/-4

Add comprehensive AgentEntry parsing/validation/defaults test coverage

• Updates legacy agents behavior test to assert rejection rather than silent ignore. Adds extensive tests for YAML unmarshal/marshal round-trip, name derivation, validation rules (hash requirements, scheme enforcement, allowlist matching, traversal prevention, duplicates), per-repo allowlist/agents behavior, and default helper functions.

internal/config/config_test.go

Documentation (1) +4 / -2

compose.goClarify allowlist semantics to include agent source URLs +4/-2

Clarify allowlist semantics to include agent source URLs

• Updates 'ComposeOpts.OrgAllowlist' documentation to reflect that allowlists apply to both base composition URLs and agent source URLs, and that callers may need to merge org and per-repo allowlists.

internal/harness/compose.go

[github-actions]

Site preview

Preview: https://4edce2f5-site.fullsend-ai.workers.dev

Commit: 86ab28491ba1c07c0c3b976f9543fd19868173f0

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:56 AM UTC · Completed 1:07 AM UTC
Commit: 4677afb · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 94.44444% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/urlutil/urlutil.go	86.20%	4 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

[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. Agent URL checks bypassable 🐞 Bug ⛨ Security

Description

validateAgentEntries() treats URLs via case-sensitive "https://" prefix matching, so a
mixed/upper-case scheme (e.g. "HTTPS://...") is misclassified as a local path and bypasses the
required #sha256 and allowlist checks. It also allowlists using raw strings.HasPrefix() without URL
path normalization/percent-decoding, which can incorrectly accept URLs that normalize outside the
allowlisted prefix (e.g. via encoded dot segments).

Code

internal/config/config.go[R317-365]

+		if strings.HasPrefix(entry.Source, "https://") {
+			// URL entry: require #sha256= fragment and allowlist membership.
+			hashIdx := strings.Index(entry.Source, "#sha256=")
+			if hashIdx < 0 {
+				return fmt.Errorf("agents[%d] (%s): URL source must include #sha256=<hash> integrity fragment", i, name)
+			}
+			hashVal := entry.Source[hashIdx+len("#sha256="):]
+			if len(hashVal) != 64 {
+				return fmt.Errorf("agents[%d] (%s): integrity hash must be 64 hex characters, got %d", i, name, len(hashVal))
+			}
+			if !isValidHex(hashVal) {
+				return fmt.Errorf("agents[%d] (%s): integrity hash is not valid hex", i, name)
+			}
+			cleanURL := entry.Source[:hashIdx]
+			if !hasAllowlistPrefix(cleanURL, allowlist) {
+				return fmt.Errorf("agents[%d] (%s): URL %q is not covered by allowed_remote_resources", i, name, cleanURL)
+			}
+		} else if strings.HasPrefix(entry.Source, "http://") {
+			return fmt.Errorf("agents[%d] (%s): URL scheme must be https, got http", i, name)
+		} else {
+			// Local path: reject traversal.
+			for _, seg := range strings.Split(entry.Source, "/") {
+				if seg == ".." {
+					return fmt.Errorf("agents[%d] (%s): local path must not contain path traversal (..)", i, name)
+				}
+			}
+		}
+	}
+	return nil
+}
+
+// isValidHex checks whether s is a valid hexadecimal string.
+func isValidHex(s string) bool {
+	for _, c := range s {
+		if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F')) {
+			return false
+		}
+	}
+	return true
+}
+
+// hasAllowlistPrefix checks if a URL matches any prefix in the allowlist.
+func hasAllowlistPrefix(rawURL string, allowlist []string) bool {
+	for _, prefix := range allowlist {
+		if strings.HasPrefix(rawURL, prefix) {
+			return true
+		}
+	}
+	return false

Relevance

⭐⭐⭐ High

Prior work adds case-insensitive URL allowlist matching + path percent-decoding/normalization to
prevent traversal bypasses (PR #1555).

PR-#1555
PR-#1095
PR-#2525

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The config validator currently uses string-prefix checks for URL detection and allowlist
enforcement, while the harness/resolve layers use URL parsing plus normalization specifically to
prevent encoded path traversal and other bypasses. This means the same agent URL can pass config
validation while violating (or later being rejected by) the actual runtime allowlist logic.

internal/config/config.go[301-366]
internal/harness/url.go[9-28]
internal/harness/url.go[41-63]
internal/harness/harness.go[814-869]
internal/resolve/resolve.go[165-203]

Agent prompt

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

### Issue description
`validateAgentEntries()` is implementing its own URL detection, integrity-hash parsing, and allowlist matching using naive string operations. This creates bypasses (mixed-case schemes) and mismatches the stronger normalization logic used elsewhere in the repo to prevent encoded path traversal.

### Issue Context
The harness/resolve path already has hardened helpers for:
- Determining whether a string is a valid HTTPS URL (`harness.IsURL`)
- Extracting/validating `#sha256=` fragments (`harness.ParseIntegrityHash`)
- Allowlist matching with percent-decoding + dot-segment cleaning (`harness.MatchingAllowedPrefixInList`)

Config validation should align with these semantics so the same URL is either allowed or rejected consistently across validation and runtime resolution.

### Fix Focus Areas
- internal/config/config.go[304-366]

### Implementation notes
- Detect URL sources using a case-insensitive scheme check (e.g. `strings.HasPrefix(strings.ToLower(src), "https://")`) or by calling `harness.IsURL(src)`.
- Parse and validate integrity hashes via `harness.ParseIntegrityHash(entry.Source)` instead of `strings.Index` + custom `isValidHex`.
- Enforce allowlisting via `harness.MatchingAllowedPrefixInList(cleanURL, allowlist) != ""` (this normalizes/decodes paths and prevents traversal bypasses).
- Consider removing `isValidHex` / `hasAllowlistPrefix` to avoid future drift from the canonical implementations.

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

---

[fullsend-ai-review]

Review

Findings

Medium

• [stale-doc] docs/plans/adr-0045-forge-portable-harness-phase4.md:55 — Strikethrough note states AgentEntry was removed and its fields (Role, Name, Slug) inlined into layers.AgentCredentials. This PR re-introduces AgentEntry in internal/config/config.go with entirely different semantics (Source, Name fields for URL/path-based agent registration per ADR 0058). While the note is historically accurate about the old AgentEntry, the type name reuse could confuse readers navigating between plan documents.
  Remediation: Add a brief annotation to the strikethrough note clarifying that a new AgentEntry type with different semantics was later introduced in ADR 0058 Phase 1.

Low

• [fail-open] internal/config/config.go — PerRepoConfig.Validate() validates agent entries against the per-repo config's own AllowedRemoteResources. Both fields are repo-contributor-controlled, so a contributor could add a permissive prefix to pass validation for any URL. The org-level containment check (harness.ValidateAllowedRemoteResources) enforces org-level containment at compose time, and this is Phase 1 (no runtime resolution), so current risk is low. Org-level enforcement of agent source URLs must be added before Phase 2 enables runtime fetching.

• [code-organization] internal/config/config.go — DefaultAgentEntries takes an AgentEntryBuilder function parameter, unlike other Default* functions in this file which return simple values. The code comment explains the co-location rationale (Phase 2 caller in install/scaffold). Consider renaming to BuildDefaultAgentEntries to distinguish it from pure-default getters.

• [naming-convention] internal/config/config.go — validConfigAgentName uses a context-prefixed naming pattern unlike the established validAgentName, validModelName etc. in internal/harness/harness.go. The existing code comment already explains the stricter semantics (requires alphanumeric first character).

• [error-message-consistency] internal/config/config.go — Legacy-format rejection error message references "ADR 0045 Phase 4". While useful as a migration aid, other validation errors in this file do not reference ADRs.

• [validation-consistency] internal/config/config.go — Agent name duplicate check uses case-insensitive comparison (strings.ToLower) while the validConfigAgentName regex is case-sensitive. Both are correct for their purposes (format vs uniqueness), but a brief comment explaining the case-insensitive duplicate rationale would aid readability.

• [error-message-format] internal/config/config.go — Validation errors include the agent name in parentheses when available (e.g., agents[%d] (%s): ...) but omit it for the empty-source error (where no name can be derived). The inconsistency is justified by the control flow but may look uneven at a glance.

Previous run

Review

Findings

Medium

• [stale-reference] docs/runtimes.md:47 — The PR changes AgentName() to DerivedName() on line 47, but this reference describes runtime.BootstrapInput.AgentName() (defined at internal/runtime/bootstrap.go:15), not the renamed AgentEntry.DerivedName(). The runtime sandbox filename derivation is performed by agentDestName(input.AgentName(), agentPath) at internal/runtime/claude.go:49, using the BootstrapInput interface method — which was not renamed by this PR. The new AgentEntry.DerivedName() is a config-level method on a different type. The doc change incorrectly conflates these two distinct methods.
  Remediation: Revert line 47 from DerivedName() back to AgentName(), or update to clarify that the sandbox filename is derived from BootstrapInput.AgentName() (which may use AgentEntry.DerivedName() upstream in Phase 2/3).

Low

• [fail-open] internal/config/config.go — PerRepoConfig.Validate() validates agent entries against the per-repo config's own AllowedRemoteResources. Both fields are repo-contributor-controlled, so a contributor could add a permissive prefix to pass validation for any URL. The org-level containment check (harness.ValidateAllowedRemoteResources) enforces org-level containment at compose time, and this is Phase 1 (no runtime resolution), so current risk is low. Must be enforced before Phase 2 enables runtime fetching.

• [missing-authorization] No linked GitHub issue. ADR 0058 (accepted) provides architectural authorization, and the PR title and labels are consistent with Phase 1 implementation. Consider linking a tracking issue for traceability.

Previous run (2)

Review

Findings

Low

• [fail-open] internal/config/config.go — PerRepoConfig.Validate() validates agent entries against the per-repo config's own AllowedRemoteResources. Both fields are repo-contributor-controlled. The org-level containment check (harness.ValidateAllowedRemoteResources) enforces org-level containment at compose time, and this is Phase 1 (no runtime resolution), so current risk is low. Must be enforced before Phase 2 enables runtime fetching.

• [error-message-format] internal/config/config.go — Error messages in validateAgentEntries use inconsistent formatting: some use semicolons vs colons as separators, some use parenthetical (%s) context while others don't. Minor inconsistency with validation error style in the rest of the file and the harness package.

• [stale-doc] docs/plans/universal-harness-access-phase1.md:29, docs/plans/universal-harness-access.md:558, docs/plans/adr-0045-forge-portable-harness-phase1.md:22 — Plan documents reference internal/harness/url.go as the location for URL utilities. The canonical implementations now live in internal/urlutil/urlutil.go; harness/url.go remains as a backward-compatible wrapper.

Previous run (3)

Review

Findings

Low

• [edge-case] internal/config/config.go:71 — UnmarshalYAML old-format detection loop uses i < len(value.Content)-1 with step 2. If a MappingNode had odd Content items, the last key would be skipped. In practice, yaml.v3 guarantees even-length Content for MappingNodes, so this is theoretical.

• [edge-case] internal/config/config.go:97 — DerivedName on a bare fragment source like #sha256=abc... strips to empty, path.Base("") returns ".", which fails regex validation. Behavior is correct (rejects degenerate input) and tested, but the error message ("invalid name") doesn't explain the root cause (missing path component).

• [fail-open] internal/config/config.go:471 — PerRepoConfig.Validate() validates agent entries against the per-repo config's own AllowedRemoteResources. Both fields are repo-contributor-controlled. The org-level containment check (harness.ValidateAllowedRemoteResources) enforces org-level containment at compose time, and this is Phase 1 (no runtime resolution), so current risk is low. Must be enforced before Phase 2 enables runtime fetching.

• [path-traversal] internal/config/config.go:339 — Local-path traversal check rejects .. segments and backslashes but does not reject absolute paths (e.g. /etc/passwd). Acceptable for Phase 1 since paths are not resolved during validation.

• [scope-tier-mismatch] internal/config/config.go:131 — DefaultAgentEntries() and AgentEntryBuilder are defined but uncalled. Code comments explain they are "defined here in Phase 1 so the type and validation are co-located" and called by install/scaffold in Phase 2. Standard incremental-delivery pattern.

• [missing-authorization] No linked GitHub issue. ADR 0058 (accepted) provides architectural authorization. Consider linking a tracking issue for traceability.

• [naming-convention] internal/config/config.go:48 — validConfigAgentName uses a similar base name to harness.validAgentName but with stricter semantics (requires alphanumeric first char). The code comment explains the distinction.

• [error-message-format] internal/config/config.go:198 — Error messages use inconsistent formatting: semicolons vs colons as separators, parenthetical (%s) context in some messages but not others.

• [stale-doc] docs/plans/universal-harness-access-phase1.md:29, docs/plans/universal-harness-access.md:558, docs/plans/adr-0045-forge-portable-harness-phase1.md:22 — Plan documents reference internal/harness/url.go as the location for URL utilities. The canonical implementations now live in internal/urlutil/urlutil.go; harness/url.go remains as a backward-compatible wrapper.

Previous run (4)

Review

Findings

Low

• [edge-case] internal/config/config.go:71 — UnmarshalYAML old-format detection loop uses i < len(value.Content)-1 with step 2. If a MappingNode had odd Content items, the last key would be skipped. In practice, yaml.v3 guarantees even-length Content for MappingNodes, so this is theoretical.

• [edge-case] internal/config/config.go:97 — DerivedName on a bare fragment source like #sha256=abc... strips to empty, path.Base("") returns ".", which fails regex validation. Behavior is correct (rejects degenerate input) and tested, but the error message ("invalid name") doesn't explain the root cause (missing path component).

• [fail-open] internal/config/config.go:471 — PerRepoConfig.Validate() validates agent entries against the per-repo config's own AllowedRemoteResources. Both fields are repo-contributor-controlled. The org-level containment check (harness.ValidateAllowedRemoteResources) enforces org-level containment at compose time, and this is Phase 1 (no runtime resolution), so current risk is low. Must be enforced before Phase 2 enables runtime fetching.

• [path-traversal] internal/config/config.go:339 — Local-path traversal check rejects .. segments and backslashes but does not reject absolute paths (e.g. /etc/passwd). Acceptable for Phase 1 since paths are not resolved during validation.

• [scope-tier-mismatch] internal/config/config.go:131 — DefaultAgentEntries() and AgentEntryBuilder are defined but uncalled. Code comments explain they are "defined here in Phase 1 so the type and validation are co-located" and called by install/scaffold in Phase 2. Standard incremental-delivery pattern.

• [missing-authorization] No linked GitHub issue. ADR 0058 (accepted) provides architectural authorization. Consider linking a tracking issue for traceability.

• [naming-convention] internal/config/config.go:48 — validConfigAgentName uses a similar base name to harness.validAgentName but with stricter semantics (requires alphanumeric first char). The code comment explains the distinction.

• [error-message-format] internal/config/config.go:198 — Error messages use inconsistent formatting: semicolons vs colons as separators, parenthetical (%s) context in some messages but not others.

• [stale-doc] docs/plans/universal-harness-access-phase1.md:29, docs/plans/universal-harness-access.md:558, docs/plans/adr-0045-forge-portable-harness-phase1.md:22 — Plan documents reference internal/harness/url.go as the location for URL utilities. The canonical implementations now live in internal/urlutil/urlutil.go; harness/url.go remains as a backward-compatible wrapper.

Previous run (5)

Review

Findings

Medium

• [stale-doc] docs/runtimes.md:47 — References AgentName() method which was renamed to DerivedName() in this PR. The doc still says "filename derived from AgentName()" which no longer matches the implementation.
  Remediation: Update line 47 from AgentName() to DerivedName().

• [naming-convention] internal/config/config.go:32 — validAgentName regex ^[a-zA-Z0-9][a-zA-Z0-9_-]*$ differs from the harness package's validAgentName at harness.go:18 which uses ^[a-zA-Z0-9_-]+$ (allows starting with underscore/hyphen). Using the same variable name with different semantics across packages is confusing.
  Remediation: Document why the config regex is stricter (requires alphanumeric first char), or align the patterns. Consider extracting to a shared constant if both validate the same concept.

• [scope-tier-mismatch] internal/config/config.go — DefaultAgentEntries() builder function (lines 121–134) has no callers in this PR. The PR description says this is a Phase 1 foundation, but shipping tested-but-uncalled code blurs the boundary between Phase 1 (schema) and Phase 1d (install seeding).
  Remediation: Consider deferring DefaultAgentEntries() and AgentEntryBuilder to the PR that adds the install-time caller, or note explicitly in the PR description that this is intentional pre-work.

Low

• [fail-open] internal/config/config.go:469 — PerRepoConfig.Validate() validates agent entries against the per-repo config's own AllowedRemoteResources. Both fields are repo-contributor-controlled. The existing harness.ValidateAllowedRemoteResources(orgAllowlist) enforces org-level containment at compose time, and this is Phase 1 (no runtime resolution yet), so current risk is low. Worth noting for Phase 3 when runtime agent resolution is added.

• [edge-case] internal/config/config.go — UnmarshalYAML old-format detection loop uses len(value.Content)-1 bound. If a MappingNode had odd Content items, the last key would be skipped. In practice, yaml.v3 guarantees even-length Content for MappingNodes, so this is theoretical.

• [architectural-coherence] internal/urlutil/urlutil.go — New package extracted to avoid circular dependencies. The package doc explains the rationale, but the intended scope and future consumers are not documented.

• [schema-coherence] internal/config/config.go:64 — Error message references "ADR 0045 Phase 4" for legacy format removal. The reference is technically correct (old YAML files may persist on disk after the Go struct field was removed), but the timeline between ADR 0045 Phase 4 and ADR 0058 re-introduction could be clearer.

• [code-organization] internal/harness/url.go — Now contains only trivial wrappers delegating to urlutil. These maintain backward compatibility for existing callers, which is a standard migration pattern. Consider documenting this as intentional or migrating callers in a follow-up.

• [stale-doc] docs/plans/universal-harness-access-phase1.md:29 — References internal/harness/url.go as the location for IsURL and ParseIntegrityHash. The canonical implementations now live in internal/urlutil/urlutil.go (wrappers remain in harness).

• [error-message-format] internal/config/config.go — Several error messages use inconsistent formatting: semicolons vs colons as separators (line 181), negative vs positive phrasing (line 204), and (..) parenthetical that could be confused with the (%s) name context (line 207). Minor inconsistencies with the rest of the validation error style.

Previous run (6)

Review

Findings

Low

• [stale-reference] docs/plans/agent-registration.md:55 — Plan document references AgentEntry.AgentName() but the implementation uses DerivedName() (renamed per prior review feedback). Plan documents describe intent at planning time and are routinely superseded by implementation decisions, so this is low-impact. Consider updating the plan for consistency.
  Remediation: Update the reference from AgentName() to DerivedName().

Previous run

Review

Findings

Low

• [edge-case] internal/config/config.go — Agent name uniqueness check in validateAgentEntries is case-sensitive (seen[name]), meaning entries that derive names differing only by case (e.g., "Triage" vs "triage") would pass validation as distinct agents. If downstream consumers use these names in case-insensitive contexts (file system paths, case-insensitive map lookups, or YAML keys), collisions could occur at runtime. Currently there are no downstream consumers (Phase 1), so the impact is latent.
  Remediation: Consider normalizing names to lowercase before the uniqueness check (e.g., seen[strings.ToLower(name)]), or document that agent names are case-sensitive.

Previous run

Review

Findings

Medium

• [method-naming] internal/config/config.go — AgentEntry.AgentName() repeats the type name in the method name. The codebase convention is to use concise method names that read naturally with the receiver type (e.g., OrgConfig.DefaultRoles() not OrgConfig.OrgDefaultRoles()). Since the struct already has a Name field, the method name AgentName() disambiguates from the field, but the Go convention would be to use a different approach — e.g., DerivedName() or EffectiveName() — to avoid the stutter while also avoiding collision with the Name field.

Low

• [edge-case] internal/config/config.go — AgentName() can return semantically invalid names like "." for edge-case Source values. For example, a source consisting only of a fragment (#sha256=abc...) strips to an empty string, and path.Base("") returns ".". The validateAgentEntries check guards against empty-string names but not these degenerate values.
  Remediation: Add a pattern check that the derived agent name is alphanumeric (with hyphens/underscores).

• [input-validation-gap] internal/config/config.go — Non-HTTPS URL schemes (file://, ftp://, data:) pass validation as local paths since urlutil.IsURL only recognizes https:// and the explicit check only catches http://. In practice these would fail at resolution time, but rejecting them early provides a clearer error.
  Remediation: In the local-path branch of validateAgentEntries, reject any source containing ://.

• [path-traversal] internal/config/config.go — The path traversal check splits on / only. On Windows, a source like ..\\..\\etc\\passwd would bypass the segment check. Mitigated by Linux deployment target and by the runtime path resolution in harness/compose.go which uses filepath.Clean/filepath.Rel containment checks.
  Remediation: Consider also rejecting backslashes in local path sources for defense-in-depth.

• [missing-authorization] No linked GitHub issue. ADR 0058 (accepted 2026-06-29) authorizes this work architecturally, and the PR title and labels are consistent with Phase 1 implementation. Consider linking an issue for traceability.

Previous run

Review

Findings

Medium

• [api-contract] internal/config/config.go — AgentName() uses filepath.Base and filepath.Ext to parse filenames from Source, which may be a URL. The filepath package uses OS-specific path separators (backslash on Windows), so filepath.Base on a URL like https://example.com/agents/triage.yaml#sha256=... would return the entire URL on Windows rather than just triage.yaml#sha256=.... The correct functions for URL paths are path.Base and path.Ext (from the path package), which always use forward slashes. While the current deployment target is Linux, this is a latent correctness bug for local CLI usage on Windows/macOS.
  Remediation: Replace filepath.Base and filepath.Ext with path.Base and path.Ext in AgentName(), and update the import from path/filepath to path.

Low

• [edge-case] internal/config/config.go — AgentName() could return an empty string for edge-case sources like .yaml or a URL ending in /.yaml#sha256=.... A single entry with an empty derived name passes validation silently (the duplicate-name check only catches two empties). No downstream consumer guards against empty agent names.
  Remediation: Add a check in validateAgentEntries that the derived agent name (from AgentName()) is non-empty.

---

Labels: PR adds agent registration config schema (Go feature) touching harness and install-related config code.

Previous run (7)

Review

Findings

Low

• [stale-reference] docs/plans/agent-registration.md:55 — Plan document references AgentEntry.AgentName() but the implementation uses DerivedName() (renamed per prior review feedback). Plan documents describe intent at planning time and are routinely superseded by implementation decisions, so this is low-impact. Consider updating the plan for consistency.
  Remediation: Update the reference from AgentName() to DerivedName().

Previous run (8)

Review

Findings

Low

• [edge-case] internal/config/config.go — Agent name uniqueness check in validateAgentEntries is case-sensitive (seen[name]), meaning entries that derive names differing only by case (e.g., "Triage" vs "triage") would pass validation as distinct agents. If downstream consumers use these names in case-insensitive contexts (file system paths, case-insensitive map lookups, or YAML keys), collisions could occur at runtime. Currently there are no downstream consumers (Phase 1), so the impact is latent.
  Remediation: Consider normalizing names to lowercase before the uniqueness check (e.g., seen[strings.ToLower(name)]), or document that agent names are case-sensitive.

Previous run

Review

Findings

Medium

• [method-naming] internal/config/config.go — AgentEntry.AgentName() repeats the type name in the method name. The codebase convention is to use concise method names that read naturally with the receiver type (e.g., OrgConfig.DefaultRoles() not OrgConfig.OrgDefaultRoles()). Since the struct already has a Name field, the method name AgentName() disambiguates from the field, but the Go convention would be to use a different approach — e.g., DerivedName() or EffectiveName() — to avoid the stutter while also avoiding collision with the Name field.

Low

• [edge-case] internal/config/config.go — AgentName() can return semantically invalid names like "." for edge-case Source values. For example, a source consisting only of a fragment (#sha256=abc...) strips to an empty string, and path.Base("") returns ".". The validateAgentEntries check guards against empty-string names but not these degenerate values.
  Remediation: Add a pattern check that the derived agent name is alphanumeric (with hyphens/underscores).

• [input-validation-gap] internal/config/config.go — Non-HTTPS URL schemes (file://, ftp://, data:) pass validation as local paths since urlutil.IsURL only recognizes https:// and the explicit check only catches http://. In practice these would fail at resolution time, but rejecting them early provides a clearer error.
  Remediation: In the local-path branch of validateAgentEntries, reject any source containing ://.

• [path-traversal] internal/config/config.go — The path traversal check splits on / only. On Windows, a source like ..\\..\\etc\\passwd would bypass the segment check. Mitigated by Linux deployment target and by the runtime path resolution in harness/compose.go which uses filepath.Clean/filepath.Rel containment checks.
  Remediation: Consider also rejecting backslashes in local path sources for defense-in-depth.

• [missing-authorization] No linked GitHub issue. ADR 0058 (accepted 2026-06-29) authorizes this work architecturally, and the PR title and labels are consistent with Phase 1 implementation. Consider linking an issue for traceability.

Previous run

Review

Findings

Medium

• [api-contract] internal/config/config.go — AgentName() uses filepath.Base and filepath.Ext to parse filenames from Source, which may be a URL. The filepath package uses OS-specific path separators (backslash on Windows), so filepath.Base on a URL like https://example.com/agents/triage.yaml#sha256=... would return the entire URL on Windows rather than just triage.yaml#sha256=.... The correct functions for URL paths are path.Base and path.Ext (from the path package), which always use forward slashes. While the current deployment target is Linux, this is a latent correctness bug for local CLI usage on Windows/macOS.
  Remediation: Replace filepath.Base and filepath.Ext with path.Base and path.Ext in AgentName(), and update the import from path/filepath to path.

Low

• [edge-case] internal/config/config.go — AgentName() could return an empty string for edge-case sources like .yaml or a URL ending in /.yaml#sha256=.... A single entry with an empty derived name passes validation silently (the duplicate-name check only catches two empties). No downstream consumer guards against empty agent names.
  Remediation: Add a check in validateAgentEntries that the derived agent name (from AgentName()) is non-empty.

---

Labels: PR adds agent registration config schema (Go feature) touching harness and install-related config code.

Previous run (9)

Review

Findings

Low

• [edge-case] internal/config/config.go — Agent name uniqueness check in validateAgentEntries is case-sensitive (seen[name]), meaning entries that derive names differing only by case (e.g., "Triage" vs "triage") would pass validation as distinct agents. If downstream consumers use these names in case-insensitive contexts (file system paths, case-insensitive map lookups, or YAML keys), collisions could occur at runtime. Currently there are no downstream consumers (Phase 1), so the impact is latent.
  Remediation: Consider normalizing names to lowercase before the uniqueness check (e.g., seen[strings.ToLower(name)]), or document that agent names are case-sensitive.

Previous run (10)

Review

Findings

Medium

• [method-naming] internal/config/config.go — AgentEntry.AgentName() repeats the type name in the method name. The codebase convention is to use concise method names that read naturally with the receiver type (e.g., OrgConfig.DefaultRoles() not OrgConfig.OrgDefaultRoles()). Since the struct already has a Name field, the method name AgentName() disambiguates from the field, but the Go convention would be to use a different approach — e.g., DerivedName() or EffectiveName() — to avoid the stutter while also avoiding collision with the Name field.

Low

• [edge-case] internal/config/config.go — AgentName() can return semantically invalid names like "." for edge-case Source values. For example, a source consisting only of a fragment (#sha256=abc...) strips to an empty string, and path.Base("") returns ".". The validateAgentEntries check guards against empty-string names but not these degenerate values.
  Remediation: Add a pattern check that the derived agent name is alphanumeric (with hyphens/underscores).

• [input-validation-gap] internal/config/config.go — Non-HTTPS URL schemes (file://, ftp://, data:) pass validation as local paths since urlutil.IsURL only recognizes https:// and the explicit check only catches http://. In practice these would fail at resolution time, but rejecting them early provides a clearer error.
  Remediation: In the local-path branch of validateAgentEntries, reject any source containing ://.

• [path-traversal] internal/config/config.go — The path traversal check splits on / only. On Windows, a source like ..\\..\\etc\\passwd would bypass the segment check. Mitigated by Linux deployment target and by the runtime path resolution in harness/compose.go which uses filepath.Clean/filepath.Rel containment checks.
  Remediation: Consider also rejecting backslashes in local path sources for defense-in-depth.

• [missing-authorization] No linked GitHub issue. ADR 0058 (accepted 2026-06-29) authorizes this work architecturally, and the PR title and labels are consistent with Phase 1 implementation. Consider linking an issue for traceability.

Previous run

Review

Findings

Medium

• [api-contract] internal/config/config.go — AgentName() uses filepath.Base and filepath.Ext to parse filenames from Source, which may be a URL. The filepath package uses OS-specific path separators (backslash on Windows), so filepath.Base on a URL like https://example.com/agents/triage.yaml#sha256=... would return the entire URL on Windows rather than just triage.yaml#sha256=.... The correct functions for URL paths are path.Base and path.Ext (from the path package), which always use forward slashes. While the current deployment target is Linux, this is a latent correctness bug for local CLI usage on Windows/macOS.
  Remediation: Replace filepath.Base and filepath.Ext with path.Base and path.Ext in AgentName(), and update the import from path/filepath to path.

Low

• [edge-case] internal/config/config.go — AgentName() could return an empty string for edge-case sources like .yaml or a URL ending in /.yaml#sha256=.... A single entry with an empty derived name passes validation silently (the duplicate-name check only catches two empties). No downstream consumer guards against empty agent names.
  Remediation: Add a check in validateAgentEntries that the derived agent name (from AgentName()) is non-empty.

---

Labels: PR adds agent registration config schema (Go feature) touching harness and install-related config code.

Previous run (11)

Review

Findings

Medium

• [method-naming] internal/config/config.go — AgentEntry.AgentName() repeats the type name in the method name. The codebase convention is to use concise method names that read naturally with the receiver type (e.g., OrgConfig.DefaultRoles() not OrgConfig.OrgDefaultRoles()). Since the struct already has a Name field, the method name AgentName() disambiguates from the field, but the Go convention would be to use a different approach — e.g., DerivedName() or EffectiveName() — to avoid the stutter while also avoiding collision with the Name field.

Low

• [edge-case] internal/config/config.go — AgentName() can return semantically invalid names like "." for edge-case Source values. For example, a source consisting only of a fragment (#sha256=abc...) strips to an empty string, and path.Base("") returns ".". The validateAgentEntries check guards against empty-string names but not these degenerate values.
  Remediation: Add a pattern check that the derived agent name is alphanumeric (with hyphens/underscores).

• [input-validation-gap] internal/config/config.go — Non-HTTPS URL schemes (file://, ftp://, data:) pass validation as local paths since urlutil.IsURL only recognizes https:// and the explicit check only catches http://. In practice these would fail at resolution time, but rejecting them early provides a clearer error.
  Remediation: In the local-path branch of validateAgentEntries, reject any source containing ://.

• [path-traversal] internal/config/config.go — The path traversal check splits on / only. On Windows, a source like ..\\..\\etc\\passwd would bypass the segment check. Mitigated by Linux deployment target and by the runtime path resolution in harness/compose.go which uses filepath.Clean/filepath.Rel containment checks.
  Remediation: Consider also rejecting backslashes in local path sources for defense-in-depth.

• [missing-authorization] No linked GitHub issue. ADR 0058 (accepted 2026-06-29) authorizes this work architecturally, and the PR title and labels are consistent with Phase 1 implementation. Consider linking an issue for traceability.

Previous run (12)

Review

Findings

Medium

• [api-contract] internal/config/config.go — AgentName() uses filepath.Base and filepath.Ext to parse filenames from Source, which may be a URL. The filepath package uses OS-specific path separators (backslash on Windows), so filepath.Base on a URL like https://example.com/agents/triage.yaml#sha256=... would return the entire URL on Windows rather than just triage.yaml#sha256=.... The correct functions for URL paths are path.Base and path.Ext (from the path package), which always use forward slashes. While the current deployment target is Linux, this is a latent correctness bug for local CLI usage on Windows/macOS.
  Remediation: Replace filepath.Base and filepath.Ext with path.Base and path.Ext in AgentName(), and update the import from path/filepath to path.

Low

• [edge-case] internal/config/config.go — AgentName() could return an empty string for edge-case sources like .yaml or a URL ending in /.yaml#sha256=.... A single entry with an empty derived name passes validation silently (the duplicate-name check only catches two empties). No downstream consumer guards against empty agent names.
  Remediation: Add a check in validateAgentEntries that the derived agent name (from AgentName()) is non-empty.

---

Labels: PR adds agent registration config schema (Go feature) touching harness and install-related config code.

[ascerra]

Review

Findings

Medium

• [security / defense-in-depth] internal/config/config.go — validateAgentEntries() uses case-sensitive strings.HasPrefix(entry.Source, "https://") for URL detection. A mixed-case scheme like HTTPS:// falls through to the local-path branch, bypassing integrity hash and allowlist checks at validation time. Not a blocker because config validation is a UX layer, not a security boundary — Phase 3 runtime resolution uses harness.IsURL() (via url.Parse) which normalizes the scheme, so a malicious URL would be caught or rejected at load time regardless. Should be fixed before Phase 3 ships, ideally by delegating to harness.IsURL() or lowercasing before the prefix check.

• [security / defense-in-depth] internal/config/config.go — The new isValidHex(), hasAllowlistPrefix(), and manual #sha256= parsing duplicate weaker versions of existing hardened helpers (harness.ParseIntegrityHash uses LastIndex + lowercase normalization; harness.MatchingAllowedPrefixInList does percent-decoding and dot-segment cleaning). Again, not a blocker because the runtime harness loader enforces the hardened checks — validation disagreeing with runtime just means a bad entry could be stored in config but would fail at load time rather than at config parse time. Worth consolidating before Phase 3 to avoid user confusion (entry passes validation, fails at runtime). If config can't import harness due to circular deps, extracting the shared logic into a leaf package like internal/urlutil would work.

Low

• [correctness] AgentName() uses filepath.Base() which is OS-dependent (\ separator on Windows). For URL sources, path.Base() would be more correct. Low risk since the CLI is predominantly run on Linux, and Phase 2's fullsend agent add will construct entries programmatically rather than relying on name derivation from raw user input.

Overall: clean Phase 1 foundation. Schema matches ADR 0058, legacy format detection is a nice touch, test coverage is thorough (96.5% patch). The medium findings are defense-in-depth improvements that should land before Phase 3 runtime resolution ships.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 1:44 AM UTC · Ended 1:48 AM UTC
Commit: 104508d · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:51 AM UTC · Completed 2:04 AM UTC
Commit: d90110a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:11 AM UTC · Completed 2:23 AM UTC
Commit: 5816301 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:29 AM UTC · Completed 2:48 AM UTC
Commit: 7f1d1da · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 3:31 AM UTC · Completed 3:45 AM UTC
Commit: 4c09de0 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:50 AM UTC · Completed 4:02 AM UTC
Commit: f5eb2e1 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:09 AM UTC · Completed 4:23 AM UTC
Commit: 9978306 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:29 AM UTC · Completed 4:44 AM UTC
Commit: 86ab284 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 11:34 AM UTC · Completed 11:46 AM UTC
Commit: 86ab284 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2768 — feat(config): add agent registration config schema

Workflow type: Human-authored PR by ggallen implementing Phase 1 of ADR-0058. Review-only agent involvement (no code/fix agent runs).

Timeline

1. 00:53 UTC — PR opened. Review agent dispatched.
2. 00:56–01:07 UTC — First review cycle. qodo-code-review[bot] flagged a security bug (case-sensitive URL scheme bypass). fullsend-ai-review[bot] posted a comprehensive review with ~7 findings (naming, docs, edge cases) but did not catch the security issue.
3. 01:20 UTC — Human reviewer ascerra approved with 2 medium security/defense-in-depth findings: the same case-sensitive scheme bypass plus duplicated weaker validation logic vs existing hardened harness helpers. Both high-impact.
4. 01:30–04:29 UTC — Author pushed 8 fixup commits addressing review feedback, triggering 9 more review agent runs (8 succeeded, 1 failed, 1 cancelled).
5. 03:28 UTC — Review run 28418221360 failed with a GitHub API 422 when the post-review script tried to submit inline comments referencing positions outside the PR diff.
6. 09:01 UTC — Second human approval from rh-hemartin.
7. 11:30 UTC — PR merged.

Review quality analysis

What the review agent did well:

• Found ~17 distinct findings across runs: naming stutter (AgentName → DerivedName), stale doc references, filepath.Base OS-dependency, edge cases in YAML unmarshaling, path traversal gaps, error message consistency. Several drove real code improvements.
• Correctly flagged fail-open risk in per-repo allowlist validation on every run.

What the review agent missed (human caught):

• Case-sensitive URL scheme bypass — strings.HasPrefix(source, "https://") allows mixed-case schemes to bypass integrity hash and allowlist checks. Both ascerra and qodo-code-review caught this; fullsend-ai-review missed it across 13 runs.
• Duplicated weaker validation — New isValidHex(), hasAllowlistPrefix(), and manual hash parsing duplicated weaker versions of hardened helpers in internal/harness. The review agent never cross-referenced new validation code against existing implementations.

Token cost: 10 review dispatches over ~10 hours is high. Most were triggered by incremental fixup commits.

Existing issues that cover identified gaps

All significant improvement areas are already tracked by open issues:

• Post-review 422 failure: #2569 (422 regression in diff-hunk validation)
• Review agent missing security findings: #898 (misses security-critical findings on large PRs), #2644 (deeper security analysis on auth/privilege code)
• Parallel implementation detection: #2651 (escalate detected parallel implementations into unification suggestions) — directly covers the "new code duplicated existing hardened helpers" pattern
• Review run debouncing: #1014, #1422, #1418 (debounce/deduplicate on rapid pushes)
• Redundant review comments: #1285 (should not regenerate unchanged inline comments on re-reviews)

No new proposals are warranted — existing issues cover the gaps with sufficient specificity. Prioritizing #2651 and #898 would have the highest impact on preventing the class of miss observed here.