refactor(config): make OrgConfig.Agents optional and add Phase 4 plan (ADR-0045 Phase 3 PR 6)

[ggallen]

Summary

• Adds omitempty to OrgConfig.Agents yaml tag so config.yaml can omit the agents: block entirely without parse errors
• Adds HasAgentsBlock() method for CLI deprecation checks
• Adds tests covering nil/empty agents parsing, marshaling, and HasAgentsBlock behavior
• Writes the Phase 4 implementation plan (docs/plans/adr-0045-forge-portable-harness-phase4.md) documenting 4 PRs to complete the ADR-0045 migration

This is the final PR in Phase 3 of ADR-0045. The loadKnownSlugs function (merged in Phase 3 PR 4) already emits a deprecation warning when falling back to the legacy agents: block, so no additional runtime warning was added.

Phase 4 plan highlights

The plan decomposes Phase 4 into 4 PRs, 3 of which can run in parallel:

• PR 1 (independent): Require role in Validate() — promote Lint warning to hard error
• PR 2 (independent): Stop writing agents: block during install — remove agents param from NewOrgConfig()
• PR 3 (independent): Remove legacy discovery fallbacks — delete loadKnownSlugsLegacy, simplify discoverAgentSlugs
• PR 4 (depends on 2+3): Remove OrgConfig.Agents field, AgentSlugs(), HasAgentsBlock()

Config schema stays at v1 (removal is backward-compatible). Full consumer audit covering all 16 call sites is included.

Test plan

• TestParseOrgConfig_WithoutAgentsBlock — config without agents: key parses, AgentSlugs() returns empty map
• TestParseOrgConfig_EmptyAgentsList — agents: [] parses, AgentSlugs() returns empty map
• TestHasAgentsBlock — true with entries, false when nil, false when empty slice
• TestOrgConfigMarshal_NilAgentsOmitted — nil agents omits key from YAML output
• TestOrgConfigMarshal_EmptyAgentsOmitted — empty slice also omitted (yaml.v3 behavior locked in)
• make go-test passes
• make lint passes

🤖 Generated with Claude Code

[github-actions]

Site preview

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

Commit: 10772424c255ed430a13efab6355f6f3f4479715

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:48 AM UTC · Completed 1:58 AM UTC
Commit: 343c155 · View workflow run →

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Low

• [stale_reference] docs/ADRs/0045-forge-portable-harness-schema.md:689 — Line 689 states the OrgConfig.Agents field uses yaml:"agents" without omitempty, which is now contradicted by this same PR adding omitempty to the field. The PR adds a clarifying note at lines 695–698 but does not amend the original sentence at line 689. Since accepted ADRs are point-in-time records and should not be substantially rewritten, this is informational only.

Info

• [missing-authorization] This PR does not have a linked GitHub issue. The work is authorized by the accepted ADR-0045 and is the final PR in a multi-PR Phase 3 sequence.

Previous run

Review

Findings

Low

• [incomplete implementation] internal/config/config.go:269 — HasAgentsBlock() is added but not called anywhere in production code. The existing deprecation warning in discover_slugs.go uses len(cfg.Agents) > 0 directly rather than the new method. The method is tested and correct, but it is dead code until a subsequent PR integrates it.

• [stale_reference] docs/ADRs/0045-forge-portable-harness-schema.md:689 — Line 689 still describes the field as using yaml:"agents" without omitempty, which is now stale after this PR adds omitempty. The new annotation at lines 695–698 partially addresses this, but the original sentence remains contradictory within the same paragraph. Per ADR policy, accepted ADRs are point-in-time records and should not be substantially rewritten, so this is informational.

Info

• [missing-authorization] This PR does not have a linked GitHub issue. The work is authorized by the accepted ADR-0045 and is the final PR in a multi-PR Phase 3 sequence.

Previous run

Review

Findings

Low

• [incomplete implementation] internal/config/config.go:269 — HasAgentsBlock() is added but not called anywhere in production code. The existing deprecation warning in discover_slugs.go uses len(cfg.Agents) > 0 directly rather than the new method. The method is tested and correct, but it is dead code until a subsequent PR integrates it.

• [stale_reference] docs/ADRs/0045-forge-portable-harness-schema.md:689 — Line 689 still describes the field as using yaml:"agents" without omitempty, which is now stale after this PR adds omitempty. The new annotation at lines 695–698 partially addresses this, but the original sentence remains contradictory within the same paragraph. Per ADR policy, accepted ADRs are point-in-time records and should not be substantially rewritten, so this is informational.

Info

• [missing-authorization] This PR does not have a linked GitHub issue. The work is authorized by the accepted ADR-0045 and is the final PR in a multi-PR Phase 3 sequence.

Previous run (2)

Review

Findings

Low

• [incomplete implementation] internal/config/config.go:269 — HasAgentsBlock() is added but not yet called in production code. The deprecation notice for the legacy agents: block is already functional via loadKnownSlugs() in internal/cli/admin.go, which emits a deprecation warning when falling back to the legacy block. HasAgentsBlock() provides a cleaner public API for future callers but is not yet wired up.

• [stale_reference] docs/ADRs/0045-forge-portable-harness-schema.md:689 — ADR-0045 line 689 describes OrgConfig.Agents as using yaml:"agents" without omitempty. This PR adds omitempty to the field tag. However, per AGENTS.md policy, accepted ADRs are point-in-time records and should not be substantially rewritten. The ADR already anticipates this change at lines 690-691. A minor cross-reference annotation would be acceptable.

Info

• [missing-authorization] This PR does not have a linked GitHub issue. The work is authorized by the accepted ADR-0045 and is the final PR in a multi-PR Phase 3 sequence.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:08 AM UTC · Completed 2:20 AM UTC
Commit: c8710f1 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:12 PM UTC · Completed 1:25 PM UTC
Commit: 1077242 · View workflow run →

[ralphbean]

LGTM.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 7:26 PM UTC · Completed 7:35 PM UTC
Commit: 1077242 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2369 — refactor(config): make OrgConfig.Agents optional and add Phase 4 plan

Workflow outcome: Good. Human-authored PR reviewed 3 times by the review agent (all APPROVED), approved by a human reviewer, and merged cleanly.

Timeline: PR opened Jun 17 01:44 UTC → 3 review agent runs (01:48, 02:08, 13:12) → human approval Jun 18 15:29 → merged Jun 18 19:20.

One pattern observed — already tracked:

The review agent raised the same [incomplete implementation] finding (HasAgentsBlock() is dead code) across two consecutive review iterations, despite the author explicitly dismissing it in a reply explaining it was intentionally staged for Phase 4. The author had to respond to duplicate inline conversation threads with the same explanation.

This is directly covered by existing open issues:

• #1672 — Review agent should honor explicit author dismissals of findings
• #1013 — Review agent should deduplicate findings across iterations
• #1500 — Review agent should not re-request changes for unchanged findings

Recently closed #1272 and #1799 confirm this area is under active improvement.

No new proposals filed — the improvement opportunity is already well-tracked. The review findings themselves were reasonable in nature (stale ADR reference, missing linked issue), and the overall workflow performed well with minimal rework and a clean merge.