refactor(harness): remove scaffold agent fallback infrastructure

[ggallen]

Closes #5552

Summary

• Remove scaffold-based agent resolution (MergedAgents/LookupMergedAgent merge, HarnessWrappersLayer, disk fallback via resolveHarnessPath) from agent discovery
• Simplify resolveAgentSource() from 3-tier (config → agents-repo → disk) to 2-tier (config → agents-repo)
• Delete AgentEntryBuilder/DefaultAgentEntries (unused Phase 2 stubs), configAgentNames, and commitSHA from buildLayerStack signature

The agents-repo runtime fallback (tryAgentsRepoFallback) is retained because the agents key in config.yaml is optional — orgs without agent entries still need runtime resolution from fullsend-ai/agents.

Scaffold files themselves are not deleted (Step 8). The customize overlay is untouched (deprecated per ADR 0064, separate concern). migrate.go and baseurl.go are untouched.

Migration prerequisite (Step 7)

The prerequisite from docs/plans/agent-extraction-to-agents-repo.md Step 7 — "All fullsend customers must be upgraded to a version that supports config-driven agents and have registered agents-repo agents in their .fullsend/config.yaml" — has been verified. All customer installations now have config-driven agent entries, making the disk-based scaffold fallback unreachable.

Impact of disk fallback removal:

• Agents not in the 6-entry defaultAgentsRepoKnownAgents allowlist (triage, code, fix, review, retro, prioritize) require a config entry — custom agents already needed config registration for credentials and dispatch.
• --offline runs and token-less environments (where tryAgentsRepoFallback is unavailable) require agents to be in config — these scenarios previously fell through to disk but config registration is the supported path.
• Config resolution (findConfigAgentEntry) is purely local — no forge client, git token, or network access needed. The cited offline/token-less failure modes only apply to agents genuinely missing from config.

Known follow-up: fullsend lock and disk-based harness files

fullsend lock (internal/cli/lock.go) resolves harnesses from harness/*.yaml files on disk. With HarnessWrappersLayer removed from the install pipeline, new installs won't generate these wrapper files. This is expected:

• Existing installs: Wrapper files are committed to the .fullsend config repo (git-tracked) and persist. fullsend lock continues to work.
• New installs: Config-driven agents use URL sources with auto-pinning via fullsend agent add (ADR-0058), replacing the role fullsend lock served for disk-based harnesses. Updating lock.go to be config/agents-repo-aware is a separate follow-up.

Test plan

• go build ./... passes
• go test ./internal/config/... ./internal/layers/... ./internal/harness/... passes
• go test ./internal/cli/... — no new failures (8 pre-existing env-dependent failures on upstream/main unrelated to this change)
• grep -rn confirms no remaining references to MergedAgents, HarnessWrappersLayer, DefaultAgentEntries, AgentEntryBuilder, configAgentNames
• resolveHarnessPath no longer called from run.go (retained in lock.go for lock operations)
• CI passes

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Remove scaffold-based agent fallback infrastructure

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Remove scaffold/disk agent resolution; rely on config agents or agents-repo fallback.
• Drop HarnessWrappersLayer and commitSHA plumbing from CLI layer stack.
• Delete merged/default agent helper APIs and update tests for new behavior.

Diagram

graph TD
  A["CLI setup/install"] --> B["buildLayerStack()"] -.-> C["HarnessWrappersLayer (removed)"]
  D["resolveAgentSource()"] --> E["Config agents (config.yaml)"] --> F["ResolveRegisteredPath()"]
  D --> G["Agents-repo fallback"]
  D -.-> H["Disk scaffold lookup (removed)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep disk fallback behind an explicit flag

• ➕ Preserves offline/dev workflows that relied on local harness files
• ➕ Reduces breaking behavior changes for unknown-agent cases
• ➖ Adds another resolution tier to maintain and document
• ➖ Risk of silently re-enabling agents unless disable rules are carefully enforced

2. Replace wrappers with a one-time migration/command

• ➕ Keeps install-time repos clean while offering a guided transition for existing orgs
• ➕ Avoids runtime scaffold coupling while still helping users generate config entries
• ➖ Extra CLI surface area and maintenance
• ➖ Still requires deciding a long-term source of truth for defaults

3. Hard-deprecate with a transitional warning release

• ➕ Gives downstream users time to move from scaffold/disk to config/agents-repo
• ➕ Reduces surprise by surfacing telemetry/warnings before removal
• ➖ Slower cleanup; keeps dead code paths longer
• ➖ Requires additional release coordination

Recommendation: The PR’s approach (config + agents-repo fallback only, removing scaffold/disk lookup and wrapper generation) is the cleanest long-term model and reduces hidden/implicit behavior. If you expect users relying on local harness files, consider adding an explicit opt-in flag (or a migration helper) rather than reintroducing an implicit disk fallback.

Files changed (10) +72 / -306

Enhancement (1) +15 / -38

run.goSimplify resolveAgentSource to config + agents-repo only +15/-38

Simplify resolveAgentSource to config + agents-repo only

• Removes scaffold merge logic (MergedAgents/LookupMergedAgent) and eliminates disk-based harness lookup fallback. Adds clearer erroring when neither config nor agents-repo fallback can resolve an agent, while preserving explicit-disable blocking semantics.

internal/cli/run.go

Refactor (5) +7 / -135

admin.goRemove commitSHA param and drop HarnessWrappersLayer from stack +3/-5

Remove commitSHA param and drop HarnessWrappersLayer from stack

• Updates buildLayerStack call sites and signature to remove the commitSHA argument. Removes HarnessWrappersLayer from the layer stack construction, reflecting the deletion of install-time wrapper generation.

internal/cli/admin.go

github.goRemove commitSHA from GitHub setup layer stack creation +2/-2

Remove commitSHA from GitHub setup layer stack creation

• Adjusts GitHub setup flows to call buildLayerStack without commitSHA, matching the simplified stack and removal of wrapper generation.

internal/cli/github.go

orgconfig.goDelete configAgentNames helper +0/-9

Delete configAgentNames helper

• Removes configAgentNames, which existed to support HarnessWrappersLayer skipping config-driven agents. With wrappers removed, the helper is no longer needed.

internal/cli/orgconfig.go

agents.goRemove merged-agent helpers; keep explicit-disable check +2/-94

Remove merged-agent helpers; keep explicit-disable check

• Deletes MergedAgent, MergedAgents, and LookupMergedAgent, leaving IsAgentExplicitlyDisabled as the remaining config-level utility for disable semantics with last-writer-wins ordering.

internal/config/agents.go

config.goRemove DefaultAgentEntries and AgentEntryBuilder stubs +0/-25

Remove DefaultAgentEntries and AgentEntryBuilder stubs

• Deletes Phase-2 placeholder APIs for computing default scaffold-derived agent entries, reducing dead surface area now that scaffold-based resolution is removed.

internal/config/config.go

Tests (4) +50 / -133

admin_test.goAlign layer stack tests with new buildLayerStack signature +1/-3

Align layer stack tests with new buildLayerStack signature

• Updates unit tests to remove the commitSHA argument and removes HarnessWrappersLayer from the expected stack composition in scope checks.

internal/cli/admin_test.go

run_test.goUpdate agent resolution tests to reflect removed disk fallback +49/-58

Update agent resolution tests to reflect removed disk fallback

• Reworks tests to assert generic 'not found' failures when disk fallback is unavailable, and updates URL-base tests to require config presence for agent registration while still enforcing allowlist checks.

internal/cli/run_test.go

config_test.goRemove DefaultAgentEntries tests +0/-40

Remove DefaultAgentEntries tests

• Deletes unit tests that exclusively covered DefaultAgentEntries, since the underlying API has been removed.

internal/config/config_test.go

scaffold_integration_test.goRemove wrapper-format integration test tied to deleted layer +0/-32

Remove wrapper-format integration test tied to deleted layer

• Removes integration coverage for the HarnessWrappersLayer-generated wrapper YAML format, which is no longer produced now that the layer has been deleted.

internal/harness/scaffold_integration_test.go

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:34 PM UTC · Completed 10:50 PM UTC
Commit: e41d1db · View workflow run →

[github-actions]

Site preview

Preview: https://5482e845-site.fullsend-ai.workers.dev

Commit: 0ae41a945fc33496fc9d337cc6f5009af028dc95

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 54 rules

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

1. Opaque fallback error 🐞 Bug ◔ Observability

Description

When tryAgentsRepoFallback is skipped (offline/no forge client/unknown agent) it often returns false
without emitting a warning, and resolveAgentSource collapses all of those causes into the generic
“agents-repo fallback unavailable” error. This makes it difficult to determine whether the fix is to
provide a token, disable offline mode, update allowed_remote_resources, or choose a different agent
name.

Code

internal/cli/run.go[R2924-2930]

+	entry := findConfigAgentEntry(orgCfg.AgentEntries(), agentName)
+	if entry == nil {
		if path, deps, ok := tryAgentsRepoFallback(ctx, agentName, forgeClient, composeOpts, printer); ok {
			return path, deps, nil
		}
-		path, err := resolveHarnessPath(fullsendDir, agentName, printer)
-		return path, nil, err
-	}
-
-	entry := findConfigAgentEntry(orgCfg.AgentEntries(), agent.Name)
-	if entry == nil {
-		return "", nil, fmt.Errorf("config agent %s: entry not found", agent.Name)
+		return "", nil, fmt.Errorf("agent %q not found in config and agents-repo fallback unavailable", agentName)
	}

Relevance

⭐ Low

Team previously rejected adding extra warning diagnostics for fallback skips; prefers simpler
generic messaging.

PR-#1573

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code shows multiple skip paths that just return false, and resolveAgentSource turns that into a
generic error string, losing the specific cause.

internal/cli/run.go[2969-2999]
internal/cli/run.go[2902-2930]

Agent prompt

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

### Issue description
`resolveAgentSource()` now returns a hard error when agents-repo fallback doesn’t resolve, but the fallback function frequently “silently skips” (returns `false` without logging) for expected conditions like offline mode or missing forge client. This produces opaque user-facing errors.

### Issue Context
`tryAgentsRepoFallback()` has multiple early-return skip paths; only some of the later failure paths emit `StepWarn`. With disk fallback removed, these skips now need to be diagnosable.

### Fix Focus Areas
- internal/cli/run.go[2907-2930]
- internal/cli/run.go[2969-2999]

### Proposed fix
1. Change `tryAgentsRepoFallback` to return `(path string, deps []harness.Dependency, ok bool, reason string)` or `(path, deps, reasonErr)` where `reasonErr` is non-nil for “skipped” cases.
2. In `resolveAgentSource`, include the reason in the returned error message (e.g., `agents-repo fallback skipped: offline mode`, `...: no forge client (missing token)`, `...: URL not in allowed_remote_resources`).
3. Optionally add `printer.StepInfo/StepWarn` for the common skip cases that currently return `false` silently.

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

---

2. Disk fallback removed 🐞 Bug ☼ Reliability

Description

resolveAgentSource no longer attempts local harness lookup when config is absent/empty or the agent
isn’t registered, so agent runs now fail whenever agents-repo fallback is skipped (offline/no forge
client/allowlist mismatch), even if harness/<agent>.yaml exists locally. This is a behavioral
regression for local/tokenless/offline execution paths because tryAgentsRepoFallback returns false
in these cases and resolveAgentSource immediately errors.

Code

internal/cli/run.go[R2908-2913]

	if orgCfg == nil || len(orgCfg.AgentEntries()) == 0 {
		if path, deps, ok := tryAgentsRepoFallback(ctx, agentName, forgeClient, composeOpts, printer); ok {
			return path, deps, nil
		}
-		path, err := resolveHarnessPath(fullsendDir, agentName, printer)
-		return path, nil, err
+		return "", nil, fmt.Errorf("agent %q not found: no config and agents-repo fallback unavailable", agentName)
	}

Relevance

⭐ Low

PR intent removes disk fallback; tests updated to expect error when no config/client/offline.

PR-#2947

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new code returns an error when agents-repo fallback doesn’t succeed, and the fallback is skipped
when offline or when no forge client is available. In runAgent, the forge client is only created
when a token is present, making tokenless runs hit this failure path.

internal/cli/run.go[2902-2930]
internal/cli/run.go[2960-2999]
internal/cli/run.go[265-342]
internal/cli/lock.go[149-156]

Agent prompt

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

### Issue description
`resolveAgentSource()` used to fall back to local disk harness resolution when config/agents-repo resolution failed; this PR removes that last-resort path and returns a hard error instead. As a result, agent execution fails in common scenarios where agents-repo fallback is intentionally skipped (offline mode, missing forge client/token), even when a local `harness/{agent}.yaml` exists.

### Issue Context
- `runAgent()` only constructs a forge client for the agents-repo fallback when it has a git token; otherwise the forge client is `nil`.
- `tryAgentsRepoFallback()` immediately skips when `forgeClient == nil` or `Offline` is true.
- With disk fallback removed, these skip conditions now become user-visible hard failures.

### Fix Focus Areas
- internal/cli/run.go[2907-2930]
- internal/cli/run.go[2969-2979]
- internal/cli/run.go[324-342]

### Proposed fix
1. Restore disk-based lookup as the final fallback in `resolveAgentSource()` (e.g., call `resolveHarnessPath(fullsendDir, agentName, printer)`), at least for the `orgCfg == nil || len(orgCfg.AgentEntries()) == 0` path and the `entry == nil` path.
2. Preserve the “explicitly disabled blocks fallback” behavior by checking `config.IsAgentExplicitlyDisabled(orgCfg.Agents, agentName)` **before** any fallback that could re-enable an agent.
3. Update returned errors to reflect which resolution tiers were attempted (config vs agents-repo vs disk).

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

---

[fullsend-ai-review]

Review

Findings

Low

• [stale-code-comment] internal/config/config.go:24 — The AgentEntry.Enabled field comment references "merged agent set" and "built-in scaffold agents" — both concepts removed by this PR. MergedAgents() is deleted and the scaffold fallback is replaced by agents-repo fallback.
  Remediation: Update comment to: "Enabled controls whether the agent is included in resolution. When nil (omitted) the agent defaults to enabled. When explicitly set to false the agent is suppressed — this allows disabling agents-repo defaults or config-registered agents without removing their config entry."

• [stale-code-comment] internal/config/config.go:394 — The comment inside ValidateAgentEntries says "it exists solely to disable a scaffold default by name". After this PR, scaffold defaults no longer exist; suppression-only entries now disable agents-repo defaults or other config-registered agents by name.
  Remediation: Update comment to: "it exists solely to disable a default or config-registered agent by name."

• [stale-doc] docs/guides/user/customizing-agents.md:500 — References "built-in scaffold agents" which were part of the removed 3-tier resolution model. With this PR, agents resolve from config or agents-repo fallback — the scaffold concept no longer exists in the codebase.
  Remediation: Replace "including built-in scaffold agents" with "including first-party agents" or "including agents resolved from the agents-repo fallback".

• [stale-doc] docs/guides/user/bring-your-own-agent.md:397 — States "config-registered agents take precedence over built-in agents on name collision" (also repeated at line 504). The precedence behavior is still correct (config is checked before agents-repo fallback), but the "collision" framing implies an additive merge step that no longer exists.
  Remediation: Rewrite to describe direct config lookup: "Your agent in config is resolved directly, replacing the default agent of the same name."

Previous run

Review

Findings

Low

• [stale-doc] docs/guides/user/customizing-agents.md:500 — References "built-in scaffold agents" which were part of the removed 3-tier resolution model. This PR removes the scaffold fallback; agents now resolve from config entries or agents-repo fallback. The term "built-in scaffold agents" is misleading now that scaffold-based resolution no longer exists.
  Remediation: Replace "including built-in scaffold agents" with "including first-party agents" or "including agents resolved from the agents-repo fallback".

• [stale-doc] docs/guides/user/customizing-agents.md:527 — Uses "built-in" terminology ("The built-in agent names are:") which refers to the removed scaffold fallback concept. The list of agent names is still correct but the framing implies scaffold-embedded agents.
  Remediation: Replace "built-in" with "known first-party" or "default".

• [stale-doc] docs/guides/user/bring-your-own-agent.md:397 — States "config-registered agents take precedence over built-in agents on name collision", describing the removed additive merge model (MergedAgents). Agents are now looked up directly in config via findConfigAgentEntry or via agents-repo fallback; there is no collision resolution.
  Remediation: Rewrite to: "Your code agent in config is resolved directly, replacing the default agent of the same name."

• [stale-doc] docs/guides/user/bring-your-own-agent.md:504 — Repeats "On name collision, config-registered agents take precedence over built-in agents" — same stale additive merge concept as the finding above.
  Remediation: Remove this bullet point or rewrite to reflect direct config lookup without collision semantics.

• [stale-code-comment] internal/config/config.go:27 — The AgentEntry.Enabled field comment references "merged agent set" and "built-in scaffold agents" — both concepts removed by this PR (MergedAgents() deleted, scaffold fallback removed). The comment reads: "Enabled controls whether the agent participates in the merged agent set... this allows disabling built-in scaffold agents without removing their role."
  Remediation: Update comment to: "Enabled controls whether the agent is included in resolution. When explicitly set to false, the agent is suppressed — this allows disabling first-party agents without removing their config entry."

Previous run (2)

Review

Findings

Medium

• [logic-error] internal/cli/poll.go:99 — Iteration direction mismatch in buildRouter vs resolveAgentSource for duplicate config entries with conflicting enabled states. buildRouter iterates config entries forward and takes the first enabled entry for a given name, while IsAgentExplicitlyDisabled (used by resolveAgentSource) and findConfigAgentEntry both iterate in reverse using last-writer-wins. If config contains [{name: "code", enabled: true}, {name: "code", enabled: false}], buildRouter includes the agent in the poll router (dispatching events to it), but resolveAgentSource rejects it as "explicitly disabled." This causes the poller to dispatch work that will always fail at agent resolution time. Practical impact is limited to the edge case of duplicate config entries with conflicting enabled states.
  Remediation: Change buildRouter to iterate entries in reverse (last-writer-wins) matching IsAgentExplicitlyDisabled semantics, or call IsAgentExplicitlyDisabled for each config entry the same way the defaultAgentsRepoKnownAgents loop does.

• [stale-reference] docs/plans/agent-registration.md:250 — Stale reference to removed MergedAgents() in section 3b. Lines 250–251 describe the runtime resolution as "Build the merged agent set via MergedAgents()". Both MergedAgents() and the scaffold disk fallback have been removed by this PR, making this resolution description inaccurate. Also line 304 says "update it to use MergedAgents() so triage dispatch continues to work" — MergedAgents() no longer exists. The PR already annotated similar stale references elsewhere in this same file, so the omission of 3b and line 304 is a genuine oversight.
  Remediation: Update section 3b's resolution steps to reflect the current two-tier model: (1) config entry lookup via findConfigAgentEntry, (2) agents-repo fallback for known first-party agents. Add removal annotation to line 304.

Low

• [stale-doc] docs/guides/user/customizing-agents.md:500 — References "built-in scaffold agents" which were part of the removed 3-tier resolution model. This PR removes the scaffold fallback; agents now resolve from config entries or agents-repo fallback. The term is mildly misleading but not functionally incorrect — the disable mechanism (enabled: false) works identically regardless of agent source.
  Remediation: Replace "including built-in scaffold agents" with "including first-party agents" or "including agents resolved from the agents repo".

• [stale-doc] docs/guides/user/bring-your-own-agent.md:397 — States "config-registered agents take precedence over built-in agents on name collision" (lines 397 and 504), describing the removed additive merge model (MergedAgents). After this PR, agents are looked up directly from config by name via findConfigAgentEntry, with agents-repo fallback when not found — no collision/precedence logic exists. The practical user guidance ("register your agent, it works") remains correct; the stale terminology describes a mechanism that no longer exists.
  Remediation: Update to explain config-direct resolution without collision/precedence terminology.

Previous run (3)

Review

Findings

Medium

• [logic-error] internal/cli/poll.go:100 — Case-insensitive deduplication bug in buildRouter. The seen map is keyed by entry.DerivedName() (preserves original case), but defaultAgentsRepoKnownAgents uses lowercase keys. If a config entry has a mixed-case DerivedName (e.g., explicit Name: "Code"), the seen check will not detect the overlap with the lowercase "code" key from defaultAgentsRepoKnownAgents, causing the same agent to appear twice in the names slice. The old MergedAgents used strings.ToLower() throughout for consistent case-insensitive keying; findConfigAgentEntry and IsAgentExplicitlyDisabled in the same PR also lowercase consistently.
  Remediation: Lowercase the DerivedName() output when populating and checking the seen map: name := strings.ToLower(entry.DerivedName()).

• [stale-doc] docs/guides/user/customizing-agents.md:500 — References "built-in scaffold agents" which were part of the removed 3-tier resolution model. This PR removes the scaffold fallback; agents now resolve from config entries or agents-repo fallback. This user-facing guide was not updated.
  Remediation: Replace "including built-in scaffold agents" with "including agents resolved from the agents repo".

• [stale-doc] docs/guides/user/bring-your-own-agent.md:397 — States "config-registered agents take precedence over built-in agents on name collision" (lines 397 and 504), describing the removed additive merge model (MergedAgents). After this PR, agents are looked up directly from config by name via findConfigAgentEntry, with agents-repo fallback when not found — no collision/precedence logic exists.
  Remediation: Update to explain config-direct resolution without collision/precedence terminology.

Low

• [edge-case] internal/cli/poll.go:100 — Behavioral change for duplicate config entries with conflicting enabled states. In buildRouter, if config contains [{name: "code", enabled: true}, {name: "code", enabled: false}], the forward iteration takes the first enabled entry and ignores the later disabled entry. The old MergedAgents would suppress the agent entirely (last-writer-wins). Practical impact is limited since resolveAgentSource independently checks IsAgentExplicitlyDisabled.

• [incomplete-removal] internal/cli/migrate.go — scaffold.HarnessNames() is retained for migrate-customizations command. The PR body and updated plan docs note this is intentional ("migrate.go and baseurl.go are untouched"), with removal tracked as a follow-up.

• [documentation-coherence] docs/plans/agent-extraction-to-agents-repo.md:679 — Plan document annotates HarnessNames() retention inline but Step 7 scope section could be clearer about what is explicitly deferred.

Previous run (4)

Review

Findings

Medium

• [stale-doc] docs/guides/user/customizing-agents.md:500 — References "built-in scaffold agents" which were part of the removed 3-tier resolution model. This PR removes the scaffold fallback; agents now resolve from config entries or agents-repo fallback. This user-facing guide was not updated.
  Remediation: Replace "including built-in scaffold agents" with "including agents resolved from the agents repo".

• [stale-doc] docs/guides/user/bring-your-own-agent.md:397 — States "config-registered agents take precedence over built-in agents on name collision", describing the removed additive merge model (MergedAgents). After this PR, agents are looked up directly from config by name via findConfigAgentEntry, with agents-repo fallback when not found — no collision/precedence logic exists. Line 504 repeats the same stale claim.
  Remediation: Update to explain config-direct resolution without collision/precedence terminology.

Low

• [stale-comment] internal/cli/poll.go:58 — The inline comment says "Build the event router from config + scaffold agents" but buildRouter no longer uses scaffold agents — it now uses config entries plus defaultAgentsRepoKnownAgents.
  Remediation: Update the comment to: // Build the event router from config + agents-repo known agents.

Previous run (5)

Review

Reason: stale-head

The review agent reviewed commit 978b7178b3831df11800a5898252995390379311 but the PR HEAD is now 835adeafefde65252ad15e08e5a220d9626286f1. This review was discarded to avoid approving unreviewed code.

Previous run (6)

Review

Findings

Medium

• [missing-authorization] — This PR makes non-trivial structural changes (removes 3-tier resolution model, deletes layer infrastructure, modifies 28 files) without a linked GitHub issue.
  Remediation: Create a tracking issue documenting why Step 7 prerequisites are satisfied and link it to this PR.

Low

• [comment-consistency] internal/cli/run.go:3035 — The function comment for tryAgentsRepoFallback states "the caller returns a hard error when this fallback is unavailable" but this describes caller behavior rather than the function's contract.
  Remediation: Revise to focus on the function's return contract rather than caller behavior.

• [scope-mismatch] — The PR implements Step 7 items 2 and 4 but omits items 1, 5-8. The remaining items (scaffold file deletion, Makefile updates, test updates) are cleanup work that logically follows this PR.
  Remediation: File a follow-up issue to track remaining Step 7 items.

Previous run (7)

Review

Findings

Medium

• [unrelated-deletion] docs/contributing/shell-scripting.md — The "yq/jq pitfalls" and "Fail-open error suppression" sections (previously lines 29–64) are deleted in this PR, but they contain substantive shell-scripting review guidance that is entirely unrelated to scaffold agent fallback removal. This content is unique to this file and not duplicated elsewhere in the repo. The deletion removes actionable review guidance about security-relevant patterns (fail-open gates in workflow dispatch steps) and common yq/jq function mismatches.
  Remediation: Restore the deleted sections. If the deletion was intentional, split it into a separate PR with its own rationale.

Previous run (8)

Review

Findings

Low

• [stale-reference] docs/ADRs/0061-harness-cel-dispatch.md:48,85 — Lines 48 and 85 state "config agents entries merged with scaffold discovery" but scaffold discovery (MergedAgents/LookupMergedAgent) was removed in this PR. Current behavior is config entries with agents-repo fallback for known first-party agents, not a scaffold discovery merge.
  Remediation: Update both references to reflect current two-tier resolution: config entries, then agents-repo fallback. Add a removal annotation consistent with the pattern used in other ADRs and plan documents updated in this PR.

Previous run (9)

Review

Findings

Low

• [stale-reference] docs/plans/vertex-inference-provisioning.md:130 — Prose says InferenceLayer runs at "position 4, before DispatchTokenLayer" but the numbered layer stack (updated in this PR at lines 148–155) shows InferenceLayer at position 5 after HarnessWrappersLayer removal.
  Remediation: Update to "position 5" or remove the position number.

• [stale-reference] docs/plans/vertex-inference-provisioning.md:233 — Says "Update to include InferenceLayer at position 4" but the stack string on the next line (updated in this PR) shows InferenceLayer as the 5th element.
  Remediation: Update to "position 5" or remove the position number.

• [stale-reference] docs/plans/agent-registration.md:276 — Section 3c references internal/layers/harnesswrappers.go and describes HarnessWrappersLayer behavior without a removal annotation. Other references in plan documents are annotated with (removed — PR refactor(harness): remove scaffold agent fallback infrastructure #5425).
  Remediation: Add a removal annotation consistent with other plan documents.

• [stale-reference] docs/plans/agent-registration.md:293 — Test expectation lists "Scaffold fallback works when agent is not in config" but the test was renamed to TestRunAgent_AgentNotInConfig and now asserts resolution failure.
  Remediation: Update test description to reflect the new behavior.

• [stale-doc] docs/ADRs/0045-forge-portable-harness-schema.md:407 — States "A fresh fullsend install generates thin harness wrappers" — behavior removed by HarnessWrappersLayer deletion. ADR is a point-in-time record; an annotation would help readers understand the current state.
  Remediation: Add a historical note indicating wrapper generation was subsequently removed.

• [stale-doc] docs/ADRs/0045-forge-portable-harness-schema.md:513 — Phase 2 description says wrappers are generated without noting this was later removed.
  Remediation: Add an annotation noting the mechanism was later removed.

Previous run (10)

Review

Findings

Medium

• [stale-reference] docs/plans/deprecate-per-org-install.md:331 — This plan document references internal/layers/harnesswrappers.go and harnesswrappers_test.go (lines 331–334), both of which are deleted in this PR. Other plan documents in the diff are annotated with (removed — PR refactor(harness): remove scaffold agent fallback infrastructure #5425) to mark deleted items, but this file is not in the diff and its references to the deleted files remain stale.
  Remediation: Add a removal annotation or strikethrough to lines 331–334 in docs/plans/deprecate-per-org-install.md, consistent with how other plan documents in this PR handle deleted references.

Previous run (11)

Review

Findings

Low

• [stale-reference] docs/plans/deprecate-customized-directory-overlay.md:32,178,196 — Three references to the deleted internal/layers/harnesswrappers.go file and its wrapperHeader constant. The PR deleted the harness wrappers layer but did not update this plan document, which describes updating wrapperHeader as future work (lines 178, 196) and references the file in a relationship table (line 32).
  Remediation: Annotate the affected lines to indicate the file was removed by PR refactor(harness): remove scaffold agent fallback infrastructure #5425, consistent with the pattern used in other plan documents.

Previous run (12)

Review

Findings

Low

• [stale-doc] docs/plans/adr-0045-forge-portable-harness-phase3.md:29 — Phase 3 plan relationship table references HarnessWrappersLayer dual-write behavior as a Phase 2 artifact. HarnessWrappersLayer was deleted by this PR. The PR annotated other HarnessWrappersLayer references in phase2, agent-extraction, agent-registration, and vertex-inference plan docs, but missed this file at this line.
  Remediation: Add annotation consistent with the pattern used elsewhere in this PR.

• [stale-doc] docs/plans/adr-0045-forge-portable-harness-phase3.md:334 — Phase 3 plan "Future: Phase 4" section describes removing dual-write from HarnessWrappersLayer as future work. HarnessWrappersLayer was deleted entirely by this PR, making this description moot.
  Remediation: Add strikethrough annotation consistent with the pattern used in agent-registration.md.

• [stale-doc] docs/plans/adr-0045-forge-portable-harness-phase4.md:38 — Phase 4 plan relationship table states HarnessWrappersLayer "remains unchanged — remains the sole source of agent identity." HarnessWrappersLayer was deleted by this PR.
  Remediation: Add annotation indicating HarnessWrappersLayer was removed.

• [stale-doc] docs/plans/adr-0045-forge-portable-harness-phase4.md:158 — Phase 4 plan PR 2 scope states "The HarnessWrappersLayer remains unchanged" — factually incorrect after this PR deletes the layer.
  Remediation: Add annotation indicating HarnessWrappersLayer was removed.

Previous run (13)

Review

Findings

High

• [rebase-conflict] internal/scaffold/fullsend-repo/.github/workflows/code.yml:43 — PR feat(telemetry): forward OTEL env vars to all managed workflow agents #5531 (merged on main as commit 3c57dda) added OTEL_EXPORTER_OTLP_TRACES_HEADERS secret forwarding to all scaffold caller workflows. This PR removes that forwarding from code.yml, fix.yml, review.yml, retro.yml, and prioritize.yml, while keeping it in triage.yml. The corresponding reusable workflows (.github/workflows/reusable-{code,fix,review,retro,prioritize}.yml) still declare and consume this secret (required: false). After this PR, newly scaffolded installations will not forward the OTEL secret from non-triage callers to the reusable workflows. Since the secret is optional, the workflow will not fail — but the OTEL header will arrive empty, silently breaking authenticated tracing backends with a 401. The PR title and description make no mention of this OTEL scope change, and no rationale is documented for why OTEL was narrowed back to triage-only less than one commit after it was expanded to all stages.
  Remediation: Either (a) preserve the OTEL forwarding in all scaffold callers to maintain the all-stage OTEL support from PR feat(telemetry): forward OTEL env vars to all managed workflow agents #5531, or (b) if the narrowing is intentional, state this explicitly in the PR description and add a comment in each removed caller explaining the rationale.

Medium

• [stale-reference] docs/guides/infrastructure/distributed-tracing.md:98 — The documentation change narrows OTEL scope to triage-only: "Only the triage stage forwards OTEL configuration in this release." The reusable workflows for all stages still declare and consume the OTEL secret. Whether the scope narrowing is intentional or a rebase artifact, the documentation should accurately reflect the final state.
  Remediation: If OTEL narrowing is intentional, acknowledge it in the PR description. If it is a rebase artifact, revert both the doc change and the scaffold caller changes.

• [test-weakened] internal/scaffold/workflow_call_alignment_test.go:274 — Two test changes reduce OTEL coverage: (1) TestReusableWorkflowsShareCommonInputs removes OTEL_EXPORTER_OTLP_TRACES_HEADERS from commonSecrets, meaning the test no longer verifies that all reusable workflows declare this secret — though they still do. (2) TestOTELHeadersSecretThreading narrows from checking all stages to triage only. Removing OTEL_EXPORTER_OTLP_TRACES_HEADERS from commonSecrets is arguably wrong even if the narrowing is intentional: the reusable workflows still declare this secret, so the assertion remains true and removing it weakens coverage without justification.

Low

• [stale-doc] docs/plans/agent-extraction-to-agents-repo.md:678 — Plan document describes future cleanup steps including removal of MergedAgents() and HarnessWrappersLayer which have been completed by this PR.
  Remediation: Annotate the completed steps in the plan document.

• [stale-doc] docs/plans/agent-registration.md — Plan document extensively describes MergedAgents(), DefaultAgentEntries(), HarnessWrappersLayer, and the additive merge model across multiple sections. All removed by this PR.
  Remediation: Annotate completed phases in the plan document.

• [stale-doc] docs/plans/adr-0045-forge-portable-harness-phase2.md — Plan document provides detailed implementation specification for HarnessWrappersLayer which has been removed.
  Remediation: Add a note indicating the layer was removed after completion of config-driven agent migration.

Previous run (14)

Review

Findings

High

• [rebase-conflict] internal/scaffold/fullsend-repo/.github/workflows/code.yml:43 — PR feat(telemetry): forward OTEL env vars to all managed workflow agents #5531 (merged on main as commit 3c57dda) added OTEL_EXPORTER_OTLP_TRACES_HEADERS secret forwarding to all scaffold caller workflows. This PR removes that forwarding from code.yml, fix.yml, review.yml, retro.yml, and prioritize.yml, while keeping it in triage.yml. The corresponding reusable workflows (.github/workflows/reusable-{code,fix,review,retro,prioritize}.yml) still declare and consume this secret (required: false). After this PR, newly scaffolded installations will not forward the OTEL secret from non-triage callers to the reusable workflows. Since the secret is optional, the workflow will not fail — but the OTEL header will arrive empty, silently breaking authenticated tracing backends with a 401. The PR title and description make no mention of this OTEL scope change, and no rationale is documented for why OTEL was narrowed back to triage-only less than one commit after it was expanded to all stages.
  Remediation: Either (a) preserve the OTEL forwarding in all scaffold callers to maintain the all-stage OTEL support from PR feat(telemetry): forward OTEL env vars to all managed workflow agents #5531, or (b) if the narrowing is intentional, state this explicitly in the PR description and add a comment in each removed caller explaining the rationale.

Medium

• [stale-reference] docs/guides/infrastructure/distributed-tracing.md:98 — The documentation change narrows OTEL scope to triage-only: "Only the triage stage forwards OTEL configuration in this release." The reusable workflows for all stages still declare and consume the OTEL secret. Whether the scope narrowing is intentional or a rebase artifact, the documentation should accurately reflect the final state.
  Remediation: If OTEL narrowing is intentional, acknowledge it in the PR description. If it is a rebase artifact, revert both the doc change and the scaffold caller changes.

• [test-weakened] internal/scaffold/workflow_call_alignment_test.go:274 — Two test changes reduce OTEL coverage: (1) TestReusableWorkflowsShareCommonInputs removes OTEL_EXPORTER_OTLP_TRACES_HEADERS from commonSecrets, meaning the test no longer verifies that all reusable workflows declare this secret — though they still do. (2) TestOTELHeadersSecretThreading narrows from checking all stages to triage only. Removing OTEL_EXPORTER_OTLP_TRACES_HEADERS from commonSecrets is arguably wrong even if the narrowing is intentional: the reusable workflows still declare this secret, so the assertion remains true and removing it weakens coverage without justification.

Low

• [stale-doc] docs/plans/agent-extraction-to-agents-repo.md:678 — Plan document describes future cleanup steps including removal of MergedAgents() and HarnessWrappersLayer which have been completed by this PR.
  Remediation: Annotate the completed steps in the plan document.

• [stale-doc] docs/plans/agent-registration.md — Plan document extensively describes MergedAgents(), DefaultAgentEntries(), HarnessWrappersLayer, and the additive merge model across multiple sections. All removed by this PR.
  Remediation: Annotate completed phases in the plan document.

• [stale-doc] docs/plans/adr-0045-forge-portable-harness-phase2.md — Plan document provides detailed implementation specification for HarnessWrappersLayer which has been removed.
  Remediation: Add a note indicating the layer was removed after completion of config-driven agent migration.

Previous run (15)

Review

Findings

High

• [protected-path] .github/workflows/reusable-code.yml, .github/workflows/reusable-dispatch.yml, .github/workflows/reusable-fix.yml, .github/workflows/reusable-prioritize.yml, .github/workflows/reusable-retro.yml, .github/workflows/reusable-review.yml — Six files under the .github/ protected path are modified. The PR has no linked issue authorizing modifications to governance/infrastructure files. The changes remove OTEL trace export forwarding (OTEL_EXPORTER_OTLP_TRACES_HEADERS secret and OTEL_EXPORTER_OTLP_TRACES_ENDPOINT/OTEL_RESOURCE_ATTRIBUTES env vars) from non-triage agent stages (code, fix, review, retro, prioritize), narrowing OTEL export to the triage stage only. Human approval is required for all protected-path changes regardless of context.
  Remediation: Link a tracking issue that authorizes the .github/workflows/ changes, or obtain explicit human approval for the OTEL forwarding removal.

Previous run (16)

Review

Findings

Low

• [error-message-consistency] internal/cli/run.go — Error message quoting inconsistency in resolveAgentSource. Three new error messages use %q for the agent name (resolving agent %q: no config..., resolving agent %q: not in config..., agent %q is explicitly disabled...), but one surviving error uses unquoted %s: fmt.Errorf("resolving config agent %s: %w", agentName, err).
  Remediation: Change %s to %q for consistency with the other error messages in the function.

Previous run (17)

Review

Findings

Low

• [stale-reference] docs/plans/agent-registration.md — Plan document references removed functions MergedAgents(), HarnessWrappersLayer, harnessesForRole(), DefaultAgentEntries(), and describes scaffold fallback behavior that was removed by this PR. Phases 4 and 5 describe work that this PR has now completed.

• [stale-reference] docs/plans/agent-extraction-to-agents-repo.md — Step 7 references MergedAgents() and HarnessWrappersLayer as items to remove. This PR has already removed both.

• [stale-reference] docs/plans/adr-0045-forge-portable-harness-phase2.md — Plan extensively describes HarnessWrappersLayer, harnessesForRole(), and configAgentNames as the mechanism for generating harness wrappers during install. These were removed in this PR.

Previous run (18)

Review

Findings

Low

• [stale-reference] docs/plans/agent-registration.md, docs/plans/agent-extraction-to-agents-repo.md — Plan documents reference removed functions DefaultAgentEntries(), MergedAgents(), and HarnessWrappersLayer across multiple sections (agent-registration.md lines 93, 230, 278, 400, 416, 423; agent-extraction-to-agents-repo.md Step 7 at line 678). These describe phased work that this PR completes — marking completed phases is good housekeeping but not blocking.

Previous run (19)

Review

Findings

Low

• [stale-reference] docs/plans/agent-registration.md — Plan document references removed functions DefaultAgentEntries(), MergedAgents(), and HarnessWrappersLayer across multiple sections (lines 93, 230, 278, 400, 416, 423). These describe phased work that this PR completes — marking completed phases is good housekeeping but not blocking.

• [stale-reference] docs/plans/adr-0045-forge-portable-harness-phase2.md, phase3.md, phase4.md, agent-extraction-to-agents-repo.md — Plan documents contain references to HarnessWrappersLayer and related infrastructure removed in this PR. These are historical records of the phased work that culminated in this removal.

Previous run (20)

Review

Findings

Medium

• [stale-reference] docs/plans/vertex-inference-provisioning.md:234 — The PR updates the numbered layer stack list at lines 147-155 to remove HarnessWrappersLayer, but misses a second occurrence of the layer stack on line 234: config-repo → workflows → harness-wrappers → vendor-binary → secrets → inference → dispatch-token → enrollment. This line still references the removed harness-wrappers layer, creating an inconsistency within the same file.
  Remediation: Update line 234 to: config-repo → workflows → vendor-binary → secrets → inference → dispatch-token → enrollment

Low

• [behavioral-change] internal/cli/run.go — The simplified resolveAgentSource returns a hard error when agents-repo fallback is unavailable, instead of falling back to disk via resolveHarnessPath. This is the intended behavior change (removing the third resolution tier), and the ready-for-merge label suggests migration prerequisites are satisfied.

• [missing-authorization] No linked issue authorizes this work. The PR body references ADR 0064 and a phased plan but no issue number is cited. Well-scoped refactor with clear intent — linking the tracking issue is recommended for traceability.

Previous run (21)

Review

Findings

Low

• [missing-authorization] No linked issue authorizes this work. The PR body references ADR 0064 and a phased plan but no issue number is cited. Well-scoped refactor with clear intent — linking the tracking issue is recommended for traceability.

• [stale-doc] docs/plans/vertex-inference-provisioning.md:150 — Plan document lists HarnessWrappersLayer at position 3 in the layer stack, which has been removed in this PR. The PR already updates ADR 0006 and docs/architecture.md but missed this plan doc.
  Remediation: Update line 150 to remove HarnessWrappersLayer from the layer ordering.

• [missing-step-reference] PR description references "Step 8" but this does not correspond to a step in the agent-extraction plan (Steps 1–7) or agent-registration plan (Phases 1–5). The code changes implement agent-registration.md Phase 5 (steps 5b and 5d).
  Remediation: Clarify the plan reference in the PR description.

• [architectural-coherence] internal/cli/run.go — The simplified resolveAgentSource returns a hard error when agents-repo fallback is unavailable, instead of falling back to disk. This is the intended behavior change per agent-registration Phase 5, but represents a breaking change for installations without config-driven agents. The ready-for-merge label suggests migration prerequisites are satisfied.

Previous run (22)

Review

Findings

Low

• [stale-doc] docs/ADRs/0006-ordered-layer-model.md:28 — The ADR states the current layer stack order as "config-repo → workflows → harness-wrappers → vendor-binary → secrets → inference → dispatch-token → enrollment" using present tense, but HarnessWrappersLayer has been removed from buildLayerStack. The PR updated the same text in docs/architecture.md (line 45) but this ADR was not annotated. ADRs are point-in-time records per project convention; a minor annotation removing harness-wrappers from the list would keep the factual claim accurate.
  Remediation: Add a minor annotation to line 28 removing harness-wrappers from the stack order, or add a cross-reference note linking to this PR.

• [stale-doc] Plan documents docs/plans/vertex-inference-provisioning.md, docs/plans/agent-registration.md, and docs/plans/adr-0045-forge-portable-harness-phase2.md reference removed identifiers (MergedAgents, HarnessWrappersLayer, configAgentNames, harnessesForRole). These are historical records of phased work — the references describe infrastructure that was planned, built, and now superseded by this PR. No action required.

Previous run (23)

Review

Findings

Medium

• [stale-doc] docs/architecture.md:290 — The Agent Registry section still describes a 3-tier resolution model: "(1) config entries from OrgConfig.Agents (highest priority), (2) runtime fallback to the fullsend-ai/agents repository for known first-party agents not in config, (3) scaffold-embedded harnesses on disk." This PR removes tier 3 (disk fallback from resolveAgentSource), but these lines were not updated. The document explicitly states it is "where the current truth lives," so this is the authoritative source developers consult for the runtime resolution algorithm.
  Remediation: Update line 290 to describe a 2-tier model: (1) config entries, (2) agents-repo fallback. Remove the reference to scaffold-embedded harnesses on disk.

• [stale-doc] docs/architecture.md:291 — The additive-merge bullet says "config entries overlay scaffold-discovered agents (config wins on name collision)" but the MergedAgents function that implemented this overlay has been deleted. Config entries are now looked up directly via findConfigAgentEntry and the agents-repo fallback is independent, not merged.
  Remediation: Remove or rewrite the additive-merge bullet to reflect the new 2-tier model where config entries are looked up directly and the agents-repo fallback operates independently.

Low

• [stale-doc] docs/ADRs/0006-ordered-layer-model.md:28 — The ADR still lists the layer stack as "config-repo → workflows → harness-wrappers → vendor-binary → secrets → inference → dispatch-token → enrollment" using present tense ("The current stack order is:"), but HarnessWrappersLayer has been removed from buildLayerStack. ADRs are point-in-time records per project convention, but the present-tense phrasing is misleading. The PR already updated the authoritative reference in docs/architecture.md:45.

• [stale-doc] docs/plans/agent-extraction-to-agents-repo.md:678 — The agent extraction plan lists removal of MergedAgents() and HarnessWrappersLayer as future steps; this PR implements those removals. Annotating the plan with completion status would help future readers.

• [missing-authorization] No linked issue authorizes this work. The PR body references "Step 8" and ADR 0064 but no issue number is cited. Well-scoped refactor with clear intent — linking the tracking issue is recommended for traceability.

Previous run (24)

Review

Findings

High

• [stale-doc] docs/architecture.md:45 — The installation layer stack lists "harness-wrappers" as a current layer: config-repo → workflows → harness-wrappers → vendor-binary → secrets → inference → dispatch → enrollment. This PR removes HarnessWrappersLayer from buildLayerStack, making the documented stack incorrect. A reader consulting this architecture doc would expect a harness-wrappers layer that no longer exists.
  Remediation: Update line 45 to remove "harness-wrappers" from the sequence: config-repo → workflows → vendor-binary → secrets → inference → dispatch → enrollment.

Low

• [missing-authorization] No linked issue authorizes this work. The PR body references "Step 8" and ADR 0064 but no issue number is cited. Well-scoped refactor with clear intent — linking the tracking issue is recommended for traceability.

Previous run (25)

Review

Findings

Medium

• [test-coverage] internal/config/agents_test.go — Deleting this file removes 11 unit tests for IsAgentExplicitlyDisabled (covering case-insensitivity, nil Enabled, DerivedName-based matching, disable-then-enable ordering, enable-then-disable ordering, empty config, and not-found scenarios). The function is retained in internal/config/agents.go and actively called by resolveAgentSource in run.go. Only indirect coverage remains via TestResolveAgentSource_DisabledAgentBlocksFallback, which tests a single scenario.
  Remediation: Move the IsAgentExplicitlyDisabled tests to a surviving test file (e.g., config_test.go or a new agents_disabled_test.go).

Low

• [missing-authorization] No linked issue authorizes this work. The PR body references a phased plan ("Step 8", ADR 0064) but no issue number is cited. Well-scoped refactor with clear intent — linking the tracking issue is recommended for traceability.

• [behavioral-change] internal/cli/run.go — Removing the disk fallback simplifies resolution from 3-tier (config → agents-repo → disk) to 2-tier (config → agents-repo). The refactor(harness): prefix correctly implies non-breaking since the disk fallback resolved files from the .fullsend config repo's harness/ directory (not arbitrary user directories), and any custom agent needs config registration for credentials and dispatch.

• [stale-documentation] Five plan documents reference removed identifiers (MergedAgents, HarnessWrappersLayer, configAgentNames, harnessesForRole): docs/plans/agent-extraction-to-agents-repo.md, docs/plans/agent-registration.md, docs/plans/adr-0045-forge-portable-harness-phase2.md, docs/plans/adr-0045-forge-portable-harness-phase3.md, docs/plans/adr-0045-forge-portable-harness-phase4.md. These are plan documents (historical records of phased work) rather than user-facing docs — the references describe infrastructure that was planned for removal and has now been removed. Marking completed phases is good housekeeping but not blocking.

Previous run

Review

Findings

High

• [commit-message-convention] PR title uses feat: prefix for internal restructuring. Per COMMITS.md: "feat is wrong for: Restructuring internals (extracting a sub-agent, splitting a package) → refactor" and "When in doubt, prefer refactor or chore over feat or fix." Removing scaffold fallback infrastructure (disk-based harness resolution, HarnessWrappersLayer, MergedAgents functions) is internal restructuring — no new user-facing capability is added. If removing the disk fallback is a breaking change for users who relied on it, the title additionally needs a ! suffix and a BREAKING CHANGE: trailer.
  Remediation: Change title to refactor(harness): remove scaffold agent fallback infrastructure. If breaking, use refactor(harness)!: remove scaffold agent fallback infrastructure with a BREAKING CHANGE: trailer explaining migration to config-driven agent registration.

Medium

• [stale-reference] internal/cli/run.go:336 — Comment says "config agents take precedence, then agents repo fallback, then disk harnesses" but this PR removes the disk-based fallback from resolveAgentSource. The 3-tier resolution no longer exists.
  Remediation: Update to: // Resolve agent source: config agents take precedence, then agents repo fallback.

• [stale-reference] internal/cli/run.go:2970 — The tryAgentsRepoFallback doc comment says "the caller falls through to disk-based lookup" but disk-based lookup via resolveHarnessPath has been removed from resolveAgentSource. Both call sites now return a hard error when fallback fails.
  Remediation: Update to: "All errors are non-fatal — the caller returns a hard error when this fallback is unavailable."

• [behavioral-change] internal/cli/run.go:2910 — Removing the disk fallback is a behavioral change. The agents-repo fallback only covers 6 first-party agents (defaultAgentsRepoKnownAgents: triage, code, fix, review, retro, prioritize). Custom agents on disk that aren't in config will stop resolving.
  Remediation: Confirm the agents-repo fallback is sufficient for all first-party agents. If any users rely on disk-based custom agents, document the migration path.

Low

• [error-message-style] internal/cli/run.go:2914 — New error messages use agent %q not found: ... format, but the established pattern in this file uses gerund verb forms (resolving ..., loading ...). Minor inconsistency.
  Remediation: Consider gerund form: fmt.Errorf("resolving agent %q: not in config and agents-repo fallback unavailable", agentName)

• [test-coverage] internal/cli/run_test.go — TestRunAgent_ScaffoldFallback renamed to TestRunAgent_AgentNotInConfig correctly reflects new behavior. Consider adding an integration test verifying agents-repo fallback works for all known first-party agents when config is absent.

---

Labels: PR removes harness wrapper generation layer and simplifies agent resolution infrastructure

Previous run (26)

Review

Findings

Medium

• [test-coverage] internal/config/agents_test.go — Deleting this file removes 11 unit tests for IsAgentExplicitlyDisabled (covering case-insensitivity, nil Enabled, DerivedName-based matching, disable-then-enable ordering, enable-then-disable ordering, empty config, and not-found scenarios). The function is retained in internal/config/agents.go and actively called by resolveAgentSource in run.go. Only indirect coverage remains via TestResolveAgentSource_DisabledAgentBlocksFallback, which tests a single scenario.
  Remediation: Move the IsAgentExplicitlyDisabled tests to a surviving test file (e.g., config_test.go or a new agents_disabled_test.go).

Low

• [missing-authorization] No linked issue authorizes this work. The PR body references a phased plan ("Step 8", ADR 0064) but no issue number is cited. Well-scoped refactor with clear intent — linking the tracking issue is recommended for traceability.

• [behavioral-change] internal/cli/run.go — Removing the disk fallback simplifies resolution from 3-tier (config → agents-repo → disk) to 2-tier (config → agents-repo). The refactor(harness): prefix correctly implies non-breaking since the disk fallback resolved files from the .fullsend config repo's harness/ directory (not arbitrary user directories), and any custom agent needs config registration for credentials and dispatch.

• [stale-documentation] Five plan documents reference removed identifiers (MergedAgents, HarnessWrappersLayer, configAgentNames, harnessesForRole): docs/plans/agent-extraction-to-agents-repo.md, docs/plans/agent-registration.md, docs/plans/adr-0045-forge-portable-harness-phase2.md, docs/plans/adr-0045-forge-portable-harness-phase3.md, docs/plans/adr-0045-forge-portable-harness-phase4.md. These are plan documents (historical records of phased work) rather than user-facing docs — the references describe infrastructure that was planned for removal and has now been removed. Marking completed phases is good housekeeping but not blocking.

Previous run (27)

Review

Findings

High

• [commit-message-convention] PR title uses feat: prefix for internal restructuring. Per COMMITS.md: "feat is wrong for: Restructuring internals (extracting a sub-agent, splitting a package) → refactor" and "When in doubt, prefer refactor or chore over feat or fix." Removing scaffold fallback infrastructure (disk-based harness resolution, HarnessWrappersLayer, MergedAgents functions) is internal restructuring — no new user-facing capability is added. If removing the disk fallback is a breaking change for users who relied on it, the title additionally needs a ! suffix and a BREAKING CHANGE: trailer.
  Remediation: Change title to refactor(harness): remove scaffold agent fallback infrastructure. If breaking, use refactor(harness)!: remove scaffold agent fallback infrastructure with a BREAKING CHANGE: trailer explaining migration to config-driven agent registration.

Medium

• [stale-reference] internal/cli/run.go:336 — Comment says "config agents take precedence, then agents repo fallback, then disk harnesses" but this PR removes the disk-based fallback from resolveAgentSource. The 3-tier resolution no longer exists.
  Remediation: Update to: // Resolve agent source: config agents take precedence, then agents repo fallback.

• [stale-reference] internal/cli/run.go:2970 — The tryAgentsRepoFallback doc comment says "the caller falls through to disk-based lookup" but disk-based lookup via resolveHarnessPath has been removed from resolveAgentSource. Both call sites now return a hard error when fallback fails.
  Remediation: Update to: "All errors are non-fatal — the caller returns a hard error when this fallback is unavailable."

• [behavioral-change] internal/cli/run.go:2910 — Removing the disk fallback is a behavioral change. The agents-repo fallback only covers 6 first-party agents (defaultAgentsRepoKnownAgents: triage, code, fix, review, retro, prioritize). Custom agents on disk that aren't in config will stop resolving.
  Remediation: Confirm the agents-repo fallback is sufficient for all first-party agents. If any users rely on disk-based custom agents, document the migration path.

Low

• [error-message-style] internal/cli/run.go:2914 — New error messages use agent %q not found: ... format, but the established pattern in this file uses gerund verb forms (resolving ..., loading ...). Minor inconsistency.
  Remediation: Consider gerund form: fmt.Errorf("resolving agent %q: not in config and agents-repo fallback unavailable", agentName)

• [test-coverage] internal/cli/run_test.go — TestRunAgent_ScaffoldFallback renamed to TestRunAgent_AgentNotInConfig correctly reflects new behavior. Consider adding an integration test verifying agents-repo fallback works for all known first-party agents when config is absent.

---

Labels: PR removes harness wrapper generation layer and simplifies agent resolution infrastructure

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 11:56 PM UTC · Completed 12:32 AM UTC
Commit: 8e16c5b · View workflow run →

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 1:39 AM UTC · Ended 1:55 AM UTC
Commit: 82d754b · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/github.go	50.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:39 AM UTC · Completed 1:55 AM UTC
Commit: 82d754b · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:01 AM UTC · Completed 2:15 AM UTC
Commit: cfeda1e · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:21 AM UTC · Completed 2:37 AM UTC
Commit: 52059b2 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:11 PM UTC · Completed 9:25 PM UTC
Commit: 46575a6 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:31 PM UTC · Completed 9:45 PM UTC
Commit: c1ff53b · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:49 PM UTC · Completed 10:06 PM UTC
Commit: 43703b0 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:10 PM UTC · Completed 10:22 PM UTC
Commit: 978b717 · View workflow run →

[fullsend-ai-review]

/fs-review

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 10:24 PM UTC · Ended 10:32 PM UTC
Commit: 835adea · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:34 PM UTC · Completed 10:48 PM UTC
Commit: 31888ba · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:56 PM UTC · Completed 11:17 PM UTC
Commit: c76d241 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:27 PM UTC · Completed 11:42 PM UTC
Commit: ba18b28 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:54 PM UTC · Completed 12:12 AM UTC
Commit: 41ace04 · View workflow run →

[waynesun09]

Final verification pass (2 agents) after the extensive prior review history on this PR (5+ rounds, all threads resolved, CI green, 2 prior approvals).

Re-verified the one MEDIUM finding without a written reply — internal/cli/poll.go:101's claimed case-insensitive dedup bug in buildRouter. Confirmed false positive: both the first loop (seen[lower], lowercased) and the second loop's lookup against defaultAgentsRepoKnownAgents's already-lowercase keys correctly key seen by lowercase form throughout — traced through a concrete "Code" example and independently verified with a standalone test of the exact loop logic (["Code", "triage"], no duplicate).

Fresh full-diff sweep — no dangling references to removed symbols (MergedAgents, LookupMergedAgent, HarnessWrappersLayer, AgentEntryBuilder, DefaultAgentEntries, configAgentNames), go build/go vet clean, retained disk-adjacent helpers (resolveHarnessPath, scaffold.HarnessNames()) correctly untouched for lock.go/migrate.go, no downstream tooling depends on the old error-message wording, and test coverage for the tryAgentsRepoFallback-as-sole-fallback paths is adequate.

Migration-safety claim ("all customer installations verified migrated") — spot-checked real consuming-org configs; corroborated in practice (orgs without an agents: block still resolve via the retained agents-repo fallback for first-party agents, not the removed disk path). No formal audit artifact backs the claim in the PR/tracking issue, but the actual blast radius of it being wrong is narrow, the failure mode is a loud non-silent error (never a silent fallback to stale behavior), and rollback is a trivial single-commit revert with no dependent work merged yet.

No blocking issues found. Approving.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 5:10 PM UTC · Ended 5:23 PM UTC
Commit: f9c17c0 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:25 PM UTC · Completed 5:41 PM UTC
Commit: 0ae41a9 · View workflow run →

[fullsend-ai-review]

Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:

• internal/config/config.go (file-level): Line 24 · [low] stale-code-comment

The AgentEntry.Enabled field comment references 'merged agent set' and 'built-in scaffold agents' — both concepts removed by this PR. MergedAgents() is deleted and the scaffold fallback is replaced by agents-repo fallback.

Suggested fix: Update comment to: 'Enabled controls whether the agent is included in resolution. When nil (omitted) the agent defaults to enabled. When explicitly set to false the agent is suppressed — this allows disabling agents-repo defaults or config-registered agents without removing their config entry.'

• internal/config/config.go (file-level): Line 394 · [low] stale-code-comment

The comment inside ValidateAgentEntries says 'it exists solely to disable a scaffold default by name'. After this PR, scaffold defaults no longer exist; suppression-only entries now disable agents-repo defaults or other config-registered agents by name.

Suggested fix: Update comment to: 'it exists solely to disable a default or config-registered agent by name.'

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 5:47 PM UTC · Completed 6:00 PM UTC
Commit: 0ae41a9 · View workflow run →

[fullsend-ai-retro]

Retro: PR #5425 -- Remove scaffold agent fallback infrastructure

Timeline

Human-authored refactoring PR (ggallen) removing 3-tier agent resolution down to 2-tier. +219/-1,391 across 29 files. Created July 21, merged July 24 (~67 hours). 25 force-pushes triggered 28 review bot runs (22 review submissions, 35 issue comments). Three human reviewers: waynesun09 (4 reviews with substantive findings that drove code fixes), rh-hemartin (1 approval), ggallen (7 response comments). Agents repo: fullsend-ai/agents at e7a3660d.

What worked well

• Stale-doc sweeping: The review bot consistently found stale references to removed concepts (MergedAgents, HarnessWrappersLayer, scaffold fallback) across docs, ADRs, and plan files -- breadth that would be tedious for humans.
• Commit convention enforcement: Bot correctly flagged feat: vs refactor: prefix mismatch.
• Rebase conflict detection: Bot caught an OTEL secret forwarding reversion from a rebase artifact.
• Iterative collaboration: waynesun09's reviews drove concrete fixes to test assertions, doc annotations, error comments, and test determinism.

Review quality gap

waynesun09 found three HIGH issues the bot missed across 28 runs:

1. fullsend lock orphaning -- removing HarnessWrappersLayer from the install pipeline without updating lock.go (untouched file still consuming the removed layer's output). New installs would silently produce no wrapper files, causing fullsend lock to no-op.
2. Missing ! breaking-change marker -- the PR description acknowledged breakage for --offline and token-less environments, but neither the title nor commit carried the conventional ! suffix.
3. Migration prerequisite verification -- the bot noted "behavioral-change" at LOW severity but never escalated to verify whether the plan doc's Step 7 prerequisite was actually satisfied.

waynesun09 also caught 4 MEDIUM issues: test assertions weakened to generic substrings (~8 tests), non-deterministic test setup (missing useFakeOpenshell(t)), generic error conflating three failure modes, and a stale plan doc annotation.

The bot produced one confirmed false positive: a medium-severity "logic-error" about case-insensitive deduplication in poll.go that waynesun09's final review pass refuted.

Evidence for existing issues

• agents#421 (check PR title for ! marker): Direct evidence -- bot caught prefix mismatch but missed the ! breaking-change marker that waynesun09 flagged.
• agents#420 (verify assertions before posting): The poll.go false positive -- bot asserted a case-sensitivity mismatch in buildRouter without verifying the actual data flow through DerivedName().
• fullsend#4960, Debounce review dispatch on rapid synchronize events #1014 (debounce force-pushes): 28 review runs from 25 force-pushes on one PR, with 12 no-finding APPROVED submissions.
• fullsend#5249 (detect weakened test assertions): waynesun09 caught ~8 tests changed from specific to generic error assertions.
• fullsend#2959 (finding deduplication): Same stale-doc findings re-raised across many iterations without progress signal.
• fullsend#971 (comment consolidation): 35 bot issue comments on one PR; status comments posted as new comments rather than updated in place.

Proposals filed

• Correctness sub-agent should trace deleted symbol references to untouched callers (in fullsend-ai/agents)