feat(#4026): support disabling agents via config.yaml enabled field - #4049
Conversation
E2E tests are runningAuthorization passed for this commit. See the E2E Tests workflow for results. |
Site previewPreview: https://5d1c8cd7-site.fullsend-ai.workers.dev Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ascerra
left a comment
ascerra
left a comment
There was a problem hiding this comment.
Review: enabled: false does not stop agents from running
Problem
This PR filters disabled agents out of MergedAgents(), but that is not enough to disable them at runtime. Two independent gates still allow the agent to run.
Gap 1 — resolveAgentSource treats “missing” as “fall back”
In internal/cli/run.go:
agent := config.LookupMergedAgent(merged, agentName)
if agent == nil || !agent.IsConfig {
// tryAgentsRepoFallback → resolveHarnessPath
}Deleting an agent from MergedAgents makes Lookup return nil, which takes the same path as a normal scaffold agent. First-party names (triage, code, fix, review, retro, prioritize) are in defaultAgentsRepoKnownAgents and will still resolve via agents-repo fallback (or disk).
So for the stated use case (disable built-in scaffold agents), filtering in MergedAgents alone is a no-op at runtime. The PR claim that no run.go changes are needed is incorrect.
Gap 2 — dispatch ignores agents: entirely
Org dispatch.yml gates only on defaults.roles (yq '.defaults.roles[]'). It never reads agents[].enabled. With retro still in defaults.roles, dispatch still fires even if config says enabled: false.
What works today vs what this PR needs
| Goal | Works with this PR? |
|---|---|
| Disable custom (non-first-party) agent | Partially — may error on missing harness, not clean skip |
Disable built-in (retro, etc.) |
No — fallback + dispatch still run it |
Required fix (for another agent)
resolveAgentSource: Distinguish “explicitly disabled” from “not in merged set.” If config has{name: X, enabled: false}, return a clear skip/error — do not calltryAgentsRepoFallback/resolveHarnessPath.- Dispatch (scaffold
dispatch.yml+reusable-dispatch.yml): Honor disabled agents (or document that operators must also remove the role fromdefaults.roles— but that contradicts the issue goal of keeping workflow infra). - Tests: Add
resolveAgentSourcecoverage provingenabled: falseonretro/triagedoes not resolve a harness via fallback. Current tests only coverMergedAgentsin isolation.
Minimal repro config
defaults:
roles: [fullsend, triage, coder, review, retro, prioritize]
agents:
- name: retro
enabled: falseExpected after fix: retro does not dispatch / does not resolve a harness.
Actual with this PR: MergedAgents drops retro; dispatch + fallback still run it.
Refs: internal/cli/run.go (resolveAgentSource, defaultAgentsRepoKnownAgents), internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml (Check role is enabled).
|
/fs-fix rebase and fix the issues raised in #4049 (review) and also #4049 (comment) |
|
🤖 Finished Fix · ✅ Success · Started 2:50 PM UTC · Completed 3:02 PM UTC |
🔧 Fix agent — iteration 1 (human-triggered)Addressed 4 of 5 review findings. Fixed resolveAgentSource to block disabled agents from resolving via fallback paths, added IsAgentExplicitlyDisabled helper, and added 11 tests. Disagreed with dispatch workflow changes (deferred to follow-up PR since resolveAgentSource now enforces the safety guarantee). Fixed (4):
Disagreed (1):
Tests: passed Decision points
Updated by fullsend fix agent |
Add IsAgentExplicitlyDisabled() to distinguish 'explicitly disabled in config' from 'not in config at all'. Update resolveAgentSource() to check this before falling through to agents-repo fallback or disk lookup — without this, setting enabled: false on a built-in agent (triage, retro, etc.) was a no-op because the fallback paths still resolved the harness. Add unit tests for IsAgentExplicitlyDisabled and integration tests for resolveAgentSource proving that enabled: false blocks all fallback resolution paths. Addresses review feedback on #4049
|
/fs-fix Rebase first. Add a dispatch-side check for agents[].enabled in internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml. After the "Check role is enabled" step (line 340), add a new step "Check agent is enabled" that reads agents[].enabled from config.yaml for the matched stage. If the agent for that stage has enabled: false, skip dispatch (set skipped=true the same way the role check does). Use yq to check — something like: look up the agent entry whose name matches $STAGE and check if its enabled field is explicitly false. If there is no matching agent entry, or if enabled is omitted/true, proceed normally (default-enabled, backward compatible). Gate subsequent steps on this new step's skipped output the same way they gate on role-check.outputs.skipped. The resolveAgentSource check in run.go is correct and must stay — it's the safety net for direct fullsend run invocations. The dispatch check is the efficiency gate that prevents wasting a runner on an agent that will just fail at resolution time. |
|
🤖 Finished Fix · ❌ Failure · Started 3:49 PM UTC · Completed 3:59 PM UTC |
553b42f to
5fd307b
Compare
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 5:32 PM UTC · Completed 5:49 PM UTC |
ReviewFindingsMedium
Low
Labels: PR adds e2e dispatch scenarios for disabled agent gating Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsHigh
Medium
Low
Labels: PR modifies dispatch workflows, e2e tests, and documentation in addition to config/harness code Previous run (3)ReviewFindingsMedium
Low
Labels: PR modifies dispatch workflows, harness registry, documentation, and e2e tests Previous run (4)ReviewReason: stale-head The review agent reviewed commit Previous run (5)ReviewRe-review from FindingsMedium
Low
Previous run (6)ReviewRe-review of same SHA This PR adds an No code changes since prior review. All prior findings remain applicable at their anchored severities. FindingsMedium
Low
Labels: PR modifies agent configuration (internal/config/), dispatch workflows (.github/workflows/, internal/scaffold/), and documentation (docs/) Labels: PR modifies agent configuration (internal/config/) and dispatch workflows. Labels: PR modifies agent configuration (internal/config/) and dispatch workflows. Previous run (7)ReviewRe-review of force-push from This PR adds an Improvement since prior review: The yq queries in both dispatch workflows now use FindingsMedium
Low
Labels: PR modifies agent configuration (internal/config/), dispatch workflows (.github/workflows/, internal/scaffold/), and documentation (docs/) Labels: PR modifies agent configuration (internal/config/) and dispatch workflows. Previous run (8)ReviewRe-review of force-push from FindingsMedium
Low
Labels: PR modifies agent configuration (internal/config/) and dispatch workflows (.github/workflows/, internal/scaffold/) Previous run (9)ReviewRe-review of force-push from This PR adds an No new issues since prior review. The force-push from FindingsMedium
Previous run (10)ReviewRe-review of force-push from This PR adds an No new issues since prior review. The force-push from FindingsMedium
Previous run (11)ReviewRe-review of force-push from This PR adds an Improvement since prior review: The force-push addressed the prior review's case-sensitivity finding — both dispatch workflows now use FindingsMedium
Previous runReviewRe-review of force-push from This PR adds an FindingsMedium
Low
Previous runReviewRe-review of force-push from This PR adds an Improvement since prior review: The force-push addressed four documentation findings from the prior review — ADR 0058 is now annotated with the FindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (12)ReviewRe-review of force-push from This PR adds an Improvement since prior review: The force-push addressed the prior review's case-sensitivity finding — both dispatch workflows now use FindingsMedium
Previous run (13)ReviewRe-review of force-push from This PR adds an FindingsMedium
Low
Previous runReviewRe-review of force-push from This PR adds an Improvement since prior review: The force-push addressed four documentation findings from the prior review — ADR 0058 is now annotated with the FindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (14)ReviewRe-review of force-push from This PR adds an FindingsMedium
Low
Previous run (15)ReviewRe-review of force-push from This PR adds an Improvement since prior review: The force-push addressed four documentation findings from the prior review — ADR 0058 is now annotated with the FindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (16)ReviewRe-review of force-push from This PR adds an Improvement since prior review: The force-push addressed four documentation findings from the prior review — ADR 0058 is now annotated with the FindingsMedium
Low
|
fullsend-ai-review
Bot
added
the
requires-manual-review
5fd307b to
99148e2
Compare
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 6:02 PM UTC · Completed 6:17 PM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
99148e2 to
43ec352
Compare
ascerra
left a comment
ascerra
left a comment
There was a problem hiding this comment.
approve as long as testing passes
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 6:42 PM UTC · Completed 6:57 PM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
github-actions
Bot
removed
the
ok-to-test
ggallen
added
the
ok-to-test
cb01b21 to
f5b887a
Compare
github-actions
Bot
removed
the
ok-to-test
ifireball
left a comment
ifireball
left a comment
There was a problem hiding this comment.
Thanks for adding BT coverage!
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Follow-up review-squad pass (3 agents: Claude ×2, Grok), scoped specifically to verifying previously-claimed "Fixed" replies against the current head (f5b887a0), since I noticed discrepancies before dispatching.
A systemic pattern surfaced: at least 3 "Fixed" replies from the 2026-07-20 18:29-20:44 UTC window do not match the current code — likely lost during the rebase onto PR #3820's agent-listing refactor mentioned in this thread. Most seriously, the HIGH-severity stale-harness-resolution bug (findConfigAgentEntry / ValidateAgentEntries alternating-chain cap) that was explicitly claimed fixed is fully reproducible right now — I confirmed it behaviorally with a live repro against the branch, not just by reading the diff. Two MEDIUM findings (the exactly-once dispatch assertion, the workflow duplicate-name yq check) show the same pattern. A fourth MEDIUM (AGENTS_TYPE malformed-value handling) was never claimed fixed and remains accurately flagged as open. Replies posted inline on each existing thread with re-verification evidence rather than as new duplicate comments.
Separately, I did forensic verification on round 6's "behaviour test never executed in CI" concern and found it's actually resolved/moot: the new scenario genuinely ran and passed at commit cb01b217 (all 8 scenarios, including this PR's), and every PR-relevant file is byte-identical between that commit and current head — the final push was a content-free rebase onto newer main. Current head's behaviour: SKIPPED status is an unrelated CI-gate race (two workflows concurrently deleting the same stale ok-to-test label), not evidence of untested code, and behaviour isn't a required status check.
One previously-discussed HIGH (workflow yq gate vs. unnamed override entries) was independently re-derived by Grok this round but isn't re-posted — it's the same issue as the existing, already-accepted-as-narrow-risk thread from an earlier round.
Recommend the PR author diff their local fix commits (if still available) against current head to recover the 3 lost fixes rather than re-deriving them from scratch.
f5b887a to
a247c32
Compare
|
@waynesun09 Re: review 4746818102 — good catch flagging the discrepancy, but the 3 "lost" fixes were never actually lost from the code. Here's what happened:
Resolution: I've now pushed directly to the upstream branch ( |
ggallen
added
the
ok-to-test
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Round-8 review-squad pass (3 agents: Claude ×2, Grok) on the latest commit a247c321, which addresses the 3 issues reopened in the previous round.
Verified fixed and correct: findConfigAgentEntry now iterates in reverse, ValidateAgentEntries correctly caps alternating chains at 2 entries per name (confirmed with a live repro — rejects 3+ entry chains, still accepts the legitimate 2-entry disable-then-enable pattern), the exactly-once dispatch assertion is wired into the e2e scenario, and the AGENTS_TYPE case statement properly distinguishes absent from malformed. Good regression test coverage was added across all three call sites (internal/cli/dispatch_test.go, internal/config/config_test.go, internal/harness/registry_test.go). Go build/vet/tests all clean.
However, the new duplicate-name check itself introduces a regression (posted inline above): it doesn't account for enabled state, so it rejects the exact disable-then-enable pattern documented in customizing-agents.md and covered by TestValidateAgentEntries_DisableThenEnable_Accepted. Confirmed independently with a live yq repro against the documented example — it hard-fails with duplicate agent name(s): retro for a config the Go layer explicitly validates as correct. This would break dispatch for any repo using the documented disable-then-enable-with-override pattern.
Not re-flagging: the workflow yq-gate-vs-bare-source-reenable divergence re-derived by one agent this round — that's the same already-discussed, already-accepted-as-narrow-risk issue from an earlier round, not new.
Recommendation: hold off on approval until the DUPES check is fixed to be state-aware (group by name+enabled-state rather than a flat check across all entries) — this is a one-line yq fix, should be quick to land.
Signed-off-by: Greg Allen <greg@fullsend.ai> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>
a247c32 to
4ee9899
Compare
github-actions
Bot
removed
the
ok-to-test
ggallen
added
the
ok-to-test
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Verified at head 4ee98995 — the round-8 blocker and all previously reopened findings are resolved:
- DUPES check is now state-aware in both workflow copies (separate enabled/disabled buckets). Live yq repro: the documented disable-then-enable pattern from
customizing-agents.mdpasses; a true case-insensitive duplicate is still caught. AGENTS_TYPEcase statement distinguishes absent (!!null) from malformed non-sequence values in both copies.- Exactly-once dispatch assertion wired into the e2e scenario (
dispatch.feature:119). findConfigAgentEntryreverse iteration +IsEnabled()filter and theValidateAgentEntriesalternating-chain cap are intact this time (both survived the squash).internal/configandinternal/harnesstests pass locally at head; CI build/test/gate green,e2eandfunctional-testsgreen.behaviouris still running at approval time — the scenario already passed atcb01b217on identical content, so no concerns, but worth a glance once it completes.
Approving — the state-aware DUPES fix was the last open item.
|
🤖 Finished Retro · ✅ Success · Started 7:46 PM UTC · Completed 8:04 PM UTC |
Retro: PR #4049 -- Support disabling agents via config.yaml
|
| Time | Event |
|---|---|
| Jul 10 13:00 | Issue #4026 opened by ggallen |
| Jul 10 13:04 | Triage agent completed -- listed 3 files to change, echoed issue's claim that run.go needs no changes |
| Jul 10 14:13 | Code agent opened PR #4049 (19 files ultimately changed, 1027 additions) |
| Jul 10 14:35 | ascerra (human) CHANGES_REQUESTED -- identified 2 fundamental gaps: resolveAgentSource fallback still resolves disabled agents, dispatch workflows ignore agents[].enabled entirely |
| Jul 10 17:49 | Review bot posted findings: 1 MEDIUM (protected-path), 5 LOW (fail-open behavior, edge cases) |
| Jul 10 22:09 | waynesun09 (human, multi-model squad) found CRITICAL bug: ascii_downcase is not a yq function -- the agent-check gate was entirely non-functional. Error was hidden by 2>/dev/null || echo "" |
| Jul 11 | Fix agent iteration 1: addressed 4/5 findings, disagreed with dispatch workflow changes (deferred). Iteration 2: fixed validation bug, docs gap |
| Jul 12 | ifireball CHANGES_REQUESTED: wait for PR #2889 agent listing refactor to merge first |
| Jul 20 | ggallen rebased on merged #3820, added behaviour tests. waynesun09 ran 4 more review rounds (rounds 4-6), finding 2 HIGH + 10 MEDIUM issues including findConfigAgentEntry stale-harness resolution and duplicate-name detection gaps |
| Jul 21 05:47 | ifireball APPROVED |
| Jul 21 16:34 | waynesun09 round 7: 3 fixes silently lost during rebase (GitHub update-branch API dropped fork commits) |
| Jul 21 18:31 | waynesun09 final APPROVED after all issues resolved (round 8, commit 4ee98995) |
| Jul 21 19:44 | PR merged |
Key findings
1. Code agent produced a non-functional feature gate. The initial PR used ascii_downcase (not a real yq function) in the workflow agent-check step, and 2>/dev/null || echo "" silently swallowed the error. The entire feature gate would never have fired. This was caught by waynesun09's multi-model review squad (Claude + Gemini + Codex), not by the automated review agent.
2. Automated review agent missed critical bugs across 19 runs. The review bot ran 19 times on this PR but never detected that ascii_downcase is invalid, that resolveAgentSource would still resolve disabled built-in agents via fallback, or that dispatch workflows didn't honor agents[].enabled. It found 1 MEDIUM and 5 LOW findings. Meanwhile, human reviewer ascerra identified both architectural gaps within 22 minutes of PR creation.
3. Fix agent disagreed with valid review findings. The fix agent's first iteration explicitly deferred dispatch workflow changes that ascerra had identified as required. This forced manual human intervention and added a review cycle.
4. Triage agent echoed incorrect scope assumption. The issue description claimed internal/cli/run.go -- no changes expected. The triage agent accepted this and listed only 3 files to change. The final PR changed 19 files including run.go, both dispatch workflows, 6 test files, and 4 documentation files.
5. Fixes lost during rebase. Round 7 discovered that 3 already-accepted fixes (reverse iteration in findConfigAgentEntry, seenState tracking, duplicate-name yq check) were silently dropped when the GitHub update-branch API performed a server-side rebase.
Metrics
- Time to merge: 11 days (Jul 10-21), of which ~8 days waiting for dependency PR feat(dispatch): add harness-dispatch job with ADR 54 auth, kill switch, and custom-agent behaviour test #2889/feat(dispatch): harness CEL dispatch for custom agents (#2889) #3820
- Active review rounds: 8+ human rounds, 19 automated review runs
- Total workflow runs: 72 (42
pull_request_review, 28pull_request_target, 2issue_comment) - Rework iterations: 8+ fix-review cycles
Evidence for existing open issues
- Code agent: analyze all usage sites before implementing, not just the one in the issue #1214 (code agent should analyze all usage sites): Code agent didn't propagate
enabledfield to dispatch workflows orresolveAgentSource, despite AGENTS.md's explicit dispatch sync rule. - Improve fix agent finding triage to reduce disagreement churn #1391 (fix agent disagreement churn): Fix agent deferred dispatch workflow changes that were architecturally required.
- Reduce review dispatch frequency during rapid fix-commit iterations #4069 (reduce review dispatch during rapid fixes): 72 workflow runs, with rapid fix-commit iterations on Jul 20 triggering multiple review dispatches.
- Filter bot-submitted pull_request_review events at dispatch shim to reduce unnecessary workflow runs #4984 / Filter bot-submitted pull_request_review events from review dispatch #2994 (filter bot-submitted review events): Bot reviews triggered additional review dispatches, contributing to the 42
pull_request_reviewruns. - Review agent should carry forward unresolved medium+ findings across commit iterations #4956 (carry forward unresolved findings across iterations): 3 fixes lost during rebase would have been caught if the review agent tracked findings across commits.
- code agent: invoke review agent in a pre-PR self-review loop (max 3 iterations) #1088 (code agent pre-PR self-review loop): A self-review pass would have caught the
ascii_downcasebug if it tested the yq expression. - Add circuit breaker to review-fix feedback loop #902 / Add per-PR circuit breaker: stop dispatching after N cycles without human interaction #3034 (circuit breaker for review-fix loop): 8 review rounds with diminishing returns in later rounds.
Proposals filed
Two proposals below target the specific review gap that allowed the most critical bugs through: the automated review agent's inability to validate yq/shell expressions in workflow YAML files.
Proposals filed
- AGENTS.md: add yq function validation and fail-open gate severity guidance to shell scripting section (in
fullsend-ai/fullsend) - Review agent should validate yq/jq expression correctness in workflow YAML files (in
fullsend-ai/agents)
5 participants
Summary
Enabled *boolfield toAgentEntrystruct so operators can selectively disable agents (including built-in scaffold agents) without removing them from configurationMergedAgents()so they are excluded from the merged result setresolveAgentSourceagents[].enabledin both scaffold and reusable dispatch workflowsenabled: falsewith nosource) that exist solely to disable scaffold defaults by nameRelated Issue
Closes #4026
ADR Edits
This PR edits the Decision section of ADR 0058 (Agent Registration) (status: Accepted, 2026-06-29) to add an
enabled: falseexample to theAgentEntrystruct documentation. The edit extends the existing struct definition with the new field — it does not change the decision or its rationale.Changes
internal/config/config.goEnabled *boolfield withyaml:"enabled,omitempty"tag toAgentEntryIsEnabled()helper method that returnstruewhenEnabledis nil (default) or explicitlytrueValidateAgentEntries()to accept suppression-only entries (no source,enabled: false) as long as they have an explicit nameinternal/config/agents.goMergedAgents()to skip disabled config entries and remove any scaffold entries they overrideinOrderset to prevent duplicate entries when a disabled entry is followed by an enabled one with a different nameIsAgentExplicitlyDisabled()to distinguish "explicitly disabled" from "not in config"internal/cli/run.goresolveAgentSourcebefore agents-repo/disk fallback to prevent disabled agents from resolving via other sourcesinternal/scaffold/fullsend-repo/.github/workflows/dispatch.yml::warning::on failure instead of silently swallowing errors.github/workflows/reusable-dispatch.yml.fullsend/config.yamlpath and file-existence guarddocs/guides/user/customizing-agents.mdnamemust match agent/harness name (code,fix, etc.), not role name (coder)docs/ADRs/0058-agent-registration.mdenabled: falseexample to Decision section (see ADR Edits above)docs/plans/agent-registration.md,docs/plans/agent-extraction-to-agents-repo.mdEnabled *boolfield andenabled: falseexampleTests
internal/config/agents_test.go: 7 new tests — disabled scaffold/custom agents, suppression-only, enabled defaults, duplicate-name-rejected-by-validation,IsAgentExplicitlyDisabled(7 cases)internal/config/config_test.go: Suppression validation, disabled-with-source,IsEnabledunit tests, YAML round-trip testsinternal/cli/run_test.go: 4 tests — disabled/suppression blocks fallback, enabled still resolvesTesting
internal/config/...tests passinternal/cli/...resolveAgentSource tests passgo vet ./...cleango build ./...cleanChecklist
downcase(yq), notascii_downcase(jq)