refactor(config): stop writing agents block during install (ADR-0045 Phase 4, PR 2)

[ggallen]

Summary

• Remove agents parameter from NewOrgConfig() — config.yaml no longer contains an agents: block
• Remove agent entry construction code from runInstall, runGitHubSetup (both dry-run and real paths)
• Update all callers across admin.go, github.go, and test files to match the new signature
• Agent identity now lives exclusively in harness wrapper files via HarnessWrappersLayer (unchanged)

Context

This is Phase 4, PR 2 of ADR-0045 (Forge-Portable Harness Schema). Phase 3 made OrgConfig.Agents optional with omitempty and migrated all consumers to harness-first discovery. This PR completes the write-side migration: NewOrgConfig() no longer accepts or sets agents, so fullsend install and fullsend github setup produce config.yaml without an agents: block.

PR dependency graph (Phase 4)

PR 1 (require role in Validate)   [independent]
PR 2 (this) ──────────────────────> PR 4 (remove OrgConfig.Agents field)
PR 3 (remove legacy discovery) ──> PR 4

Test plan

• make go-test — all packages pass
• make go-vet — clean
• make lint — passes
• NewOrgConfig no longer accepts agents parameter
• Config output does not contain agents: key (existing tests TestOrgConfigMarshal_NilAgentsOmitted and TestOrgConfigMarshal_EmptyAgentsOmitted verify this)
• TestNewOrgConfig updated to verify cfg.Agents is empty

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Refactor config: stop persisting agents in config.yaml during install
✨ Enhancement ⚙️ Configuration changes 🕐 20-40 Minutes

Description

• Remove the agents parameter from NewOrgConfig so generated config omits agents:.
• Stop constructing agent entries in install and github setup paths.
• Update CLI/tests to rely on harness wrapper files for agent identity.

Diagram

graph TD
  CLI["CLI install/setup"] --> NOC["config.NewOrgConfig"] --> OC["OrgConfig"] --> CRL["ConfigRepoLayer"] --> REPO[(".fullsend repo")] --> CFG["config.yaml"]
  CLI --> HWL["HarnessWrappersLayer"] --> REPO --> HWR["harness/*.yaml"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep old constructor (deprecated) and ignore agents internally

• ➕ Avoids touching all call sites at once
• ➕ Allows gradual migration for any downstream/internal packages
• ➖ Continues to suggest agents are part of config.yaml write path
• ➖ Risk of regressions reintroducing agents: in output via future call sites

2. Functional-options constructor (e.g., NewOrgConfig(..., opts...))

• ➕ More extensible API for future config defaults without signature churn
• ➕ Can explicitly forbid writing agents while still supporting other optional knobs
• ➖ Higher implementation overhead for a phased ADR migration
• ➖ Adds indirection for a currently simple constructor

Recommendation: The PR’s approach (remove the agents parameter entirely) is the clearest enforcement mechanism for ADR-0045 Phase 4: it makes it structurally harder to reintroduce agents: into config.yaml during install/setup. The alternatives reduce churn but leave a misleading API surface area that implies agents belong in the write-side org config.

Files changed (7) +16 / -41

Refactor (3) +8 / -24

admin.goStop passing agents into NewOrgConfig during dry-run/install/analyze +4/-11

Stop passing agents into NewOrgConfig during dry-run/install/analyze

• Updates 'runDryRun', 'runInstall', 'runUninstall', and 'runAnalyze' to call 'config.NewOrgConfig' without an agents slice. Removes the now-dead code that built 'config.AgentEntry' lists for config.yaml generation.

internal/cli/admin.go

github.goRemove dummy/real agent entry construction in GitHub setup flow +2/-10

Remove dummy/real agent entry construction in GitHub setup flow

• Eliminates building agent entries solely for config.yaml in both the dry-run and real credential paths of 'runGitHubSetupPerOrg'. Updates config creation to rely on the new 'NewOrgConfig' signature.

internal/cli/github.go

config.goRemove agents parameter from NewOrgConfig and stop populating OrgConfig.Agents +2/-3

Remove agents parameter from NewOrgConfig and stop populating OrgConfig.Agents

• Changes the 'NewOrgConfig' constructor signature to no longer accept '[]AgentEntry'. Stops setting 'OrgConfig.Agents' during default config creation so marshaled YAML omits the 'agents:' key when empty.

internal/config/config.go

Tests (4) +8 / -17

admin_test.goUpdate admin CLI tests for NewOrgConfig signature change +1/-3

Update admin CLI tests for NewOrgConfig signature change

• Adjusts helper/config construction in tests to match the new 'NewOrgConfig' signature. Removes now-unneeded 'nil' agents arguments in layer-stack related tests.

internal/cli/admin_test.go

github_test.goUpdate GitHub CLI tests for NewOrgConfig signature change +1/-1

Update GitHub CLI tests for NewOrgConfig signature change

• Updates test config creation to call 'NewOrgConfig' without an agents parameter while keeping existing status/report behavior intact.

internal/cli/github_test.go

config_test.goAlign config tests with agents-less NewOrgConfig behavior +6/-12

Align config tests with agents-less NewOrgConfig behavior

• Updates 'TestNewOrgConfig' and other constructor tests to use the new signature and assert 'cfg.Agents' is empty by default. Keeps existing marshal tests that ensure 'agents' is omitted when nil/empty.

internal/config/config_test.go

configrepo_test.goUpdate ConfigRepoLayer test config setup for NewOrgConfig signature +0/-1

Update ConfigRepoLayer test config setup for NewOrgConfig signature

• Removes the explicit agent entry passed into 'NewOrgConfig' in test helpers, matching the new constructor contract and expected config.yaml output.

internal/layers/configrepo_test.go

[github-actions]

Site preview

Preview: https://443a2ea0-site.fullsend-ai.workers.dev

Commit: 902ab8f3af912e4e9ad7b194cf54d0fbcecb56f8

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:46 PM UTC · Completed 9:58 PM UTC
Commit: 0eab249 · View workflow run →

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Tickets: 🎫 Shared Feature Toggle Strategy for HCC

✅ Compliance rules (platform): 51 rules

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

1. Renamed app slugs lost 🐞 Bug ☼ Reliability

Description

NewOrgConfig no longer persists agent role→slug metadata in config.yaml, leaving harness wrapper
files as the only durable source for non-standard/renamed GitHub App slugs. But HarnessWrappersLayer
skips generating wrappers when commitSHA is empty/"dev", so appsetup can’t rediscover renamed app
slugs and may create duplicate apps or fail to reuse existing installations.

Code

internal/config/config.go[R132-135]

			MaxImplementationRetries: 2,
			AutoMerge:                false,
		},
-		Agents: agents,
-		Repos:  repos,
+		Repos: repos,

Evidence

The PR removes Agents initialization from NewOrgConfig, so config.yaml can no longer carry
role→slug mappings. The only remaining durable discovery path for renamed apps is via harness
wrapper files, but those are explicitly skipped when commitSHA is "dev"/empty, which is the
default in local builds; without knownSlugs, findExistingInstallation falls back to the
conventional slug and won’t match renamed apps.

internal/config/config.go[116-140]
internal/cli/root.go[9-20]
internal/layers/harnesswrappers.go[67-76]
internal/cli/admin.go[1319-1340]
internal/cli/admin.go[2019-2067]
internal/appsetup/appsetup.go[152-156]
internal/appsetup/appsetup.go[363-398]

Agent prompt

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

## Issue description
`NewOrgConfig()` no longer writes `OrgConfig.Agents`, removing the legacy persistence mechanism for role→slug mappings. When harness wrapper generation is skipped (commitSHA unset/"dev"), there is no remaining durable source for renamed/non-conventional GitHub App slugs, so app setup can’t reliably reuse existing apps.

## Issue Context
- Local/dev builds default `commitSHA` to `"dev"`, and `HarnessWrappersLayer.Install` returns early in that case.
- `runAppSetup()` uses `loadKnownSlugs()` to provide `WithKnownSlugs()` overrides so `findExistingInstallation()` can match renamed apps; without wrappers and without config agents, those overrides disappear.

## Fix Focus Areas
- internal/config/config.go[116-153]
- internal/cli/root.go[9-20]
- internal/layers/harnesswrappers.go[67-71]
- internal/cli/admin.go[1319-1340]
- internal/cli/admin.go[2019-2067]
- internal/appsetup/appsetup.go[152-156]
- internal/appsetup/appsetup.go[363-398]

## Implementation direction
Choose one durable source for role→slug that does *not* depend on a 40-char commit SHA:
1. Write a small managed mapping file in the config repo (e.g., `harness/agent_slugs.yaml` or similar) during install/setup using the actual slugs from `agentCreds`, and have `loadKnownSlugs()` read it before falling back to legacy `config.yaml`.
2. Alternatively, if ADR constraints permit, keep writing a minimal role→slug block somewhere in `config.yaml` *only* when harness wrappers cannot be generated (commitSHA invalid), to preserve behavior for dev/local builds.

Update `loadKnownSlugs()` / `discoverAgentSlugs()` to include this new source so `Setup.WithKnownSlugs()` continues to work for renamed apps even without harness wrappers.

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

---

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% 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!

[fullsend-ai-review]

Looks good to me — clean removal of the agents parameter from NewOrgConfig() with all call sites and tests updated consistently. The change aligns with ADR-0045 Phase 4, and both prior findings (stale references in triage-prerequisites doc, future-tense wording in phase4 plan) are resolved in this revision.

Previous run

Looks good to me — clean removal of the agents parameter from NewOrgConfig() with all call sites and tests updated consistently. The change aligns with ADR-0045 Phase 4.

Low

• [stale-reference] docs/superpowers/plans/2026-06-11-triage-prerequisites.md — The PR updates the NewOrgConfig signature at line 229 but leaves numerous call-site examples (lines 159, 334, 339, 344, 349, 354, 359, 379, 389, 396) still passing the removed agents argument. The document is internally inconsistent after this change.
• [internal-consistency] docs/plans/adr-0045-forge-portable-harness-phase4.md:11 — Item 2 summary still uses future tense ("will no longer accept") despite being marked as shipped elsewhere in the document.

Previous run

Looks good to me — clean removal of the agents parameter from NewOrgConfig() with all call sites and tests updated consistently. The change aligns with ADR-0045 Phase 4.

Low

• [stale-function-signature] docs/superpowers/plans/2026-06-11-triage-prerequisites.md:229 — Plan document still shows the old NewOrgConfig signature with the agents []AgentEntry parameter. After this PR merges, the snippet is stale.
• [stale-function-signature] docs/plans/adr-0045-forge-portable-harness-phase4.md:11 — Phase 4 plan describes removing the agents parameter as future work; after merge, mark PR 2 as completed.
• [stale-function-signature] docs/plans/adr-0045-forge-portable-harness-phase4.md:78 — Consumer audit table entry for NewOrgConfig agents param should be marked completed after merge.
• [stale-code-reference] docs/plans/adr-0045-forge-portable-harness-phase4.md:161 — Implementation instructions show the new signature as a future change; mark as completed after merge.
• [stale-code-reference] docs/plans/adr-0045-forge-portable-harness-phase4.md:170 — Caller site list should be marked as completed after merge.
• [missing-authorization] No linked issue, but authorization is established via ADR-0045 Phase 4. Consider adding an explicit issue link for traceability.

---

Labels: Refactoring PR modifies CLI install/setup flows and config construction as part of ADR-0045 migration.

Previous run

Looks good to me — clean removal of the agents parameter from NewOrgConfig() with all call sites and tests updated consistently. The change aligns with ADR-0045 Phase 4.

Low

• [stale-reference] docs/superpowers/plans/2026-06-11-triage-prerequisites.md — The PR updates the NewOrgConfig signature at line 229 but leaves numerous call-site examples (lines 159, 334, 339, 344, 349, 354, 359, 379, 389, 396) still passing the removed agents argument. The document is internally inconsistent after this change.
• [internal-consistency] docs/plans/adr-0045-forge-portable-harness-phase4.md:11 — Item 2 summary still uses future tense ("will no longer accept") despite being marked as shipped elsewhere in the document.

Previous run (2)

Looks good to me — clean removal of the agents parameter from NewOrgConfig() with all call sites and tests updated consistently. The change aligns with ADR-0045 Phase 4.

Low

• [stale-function-signature] docs/superpowers/plans/2026-06-11-triage-prerequisites.md:229 — Plan document still shows the old NewOrgConfig signature with the agents []AgentEntry parameter. After this PR merges, the snippet is stale.
• [stale-function-signature] docs/plans/adr-0045-forge-portable-harness-phase4.md:11 — Phase 4 plan describes removing the agents parameter as future work; after merge, mark PR 2 as completed.
• [stale-function-signature] docs/plans/adr-0045-forge-portable-harness-phase4.md:78 — Consumer audit table entry for NewOrgConfig agents param should be marked completed after merge.
• [stale-code-reference] docs/plans/adr-0045-forge-portable-harness-phase4.md:161 — Implementation instructions show the new signature as a future change; mark as completed after merge.
• [stale-code-reference] docs/plans/adr-0045-forge-portable-harness-phase4.md:170 — Caller site list should be marked as completed after merge.
• [missing-authorization] No linked issue, but authorization is established via ADR-0045 Phase 4. Consider adding an explicit issue link for traceability.

---

Labels: Refactoring PR modifies CLI install/setup flows and config construction as part of ADR-0045 migration.

[ggallen]

Renamed app slugs lost (Qodo)

Not a real issue. loadKnownSlugs calls DiscoverRemoteAgents, which reads harness wrappers from the remote config repo — the commitSHA == "dev" skip in HarnessWrappersLayer.Install only affects writing wrappers during the current install, not reading existing ones. On re-install, wrappers from the previous install are already in the remote repo and will be discovered.

The only scenario where this could matter is a first-ever install done with a dev build, followed by an app rename, followed by another dev-build install — that is not a supported workflow (dev builds are for local testing, not production app setup).

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:19 PM UTC · Completed 10:30 PM UTC
Commit: 3935090 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:40 PM UTC · Completed 10:49 PM UTC
Commit: 902ab8f · View workflow run →

[ralphbean]

LGTM.

[waynesun09]

ADR-0045 Review Squad — 3 findings (medium+ severity). Reviewed for correctness, security, and ADR alignment.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 3:43 PM UTC · Completed 3:51 PM UTC
Commit: 902ab8f · View workflow run →

[fullsend-ai-retro]

Retro Analysis: PR #2447

Workflow outcome: Successful. Human-authored refactoring PR (9 files, +47/-107) received 3 agent review cycles (all approved) and 2 human reviews (1 approval, 1 commented with non-blocking suggestions). Merged in ~4 days.

What went well

• Review agent found 2 valid low-severity issues (stale doc reference, tense wording) that the author fixed promptly.
• Agent reviews completed quickly (~15 min from PR open to first approval).
• Human reviewer waynesun09 added 3 medium-severity non-blocking suggestions (assertion quality, missing integration test, design improvement), which the author tracked in issue Address review findings from ADR-0045 Phase 4 PR 2 (#2447) #2511 for follow-up.

Review quality gap

The agent's correctness sub-agent has explicit instructions to evaluate whether "tests actually constrain the code's behavior, or merely assert it runs" — yet it missed a case where assert.Empty was used instead of assert.Nil (a weaker assertion for nil-checking). This appears to be an execution gap rather than an instruction gap. The challenger sub-agent may have removed borderline test-quality findings as "speculative."

Existing issue coverage

The identified gaps are already well-covered by open issues:

• Test adequacy detection: Review agent should flag bug-fix PRs that lack tests for the fixed behavior #1354, Review agent should flag new helper functions lacking dedicated unit tests #2066, Elevate test-adequacy findings to medium for bug-fix PRs lacking regression tests #2313, Review agent should check CLAUDE.md path-based test requirements #2036
• Design/approach review depth: Add 'strategic fitness' evaluation dimension to review agent #849, Review agent should evaluate approach soundness, not just implementation correctness #1999 (recently closed), Review agent misses cross-file impact analysis that Review Squad catches #1525, Review agent should validate implementation approach against issue requirements #1472
• Redundant review dispatches: Reduce redundant review dispatches triggered by non-code events #893, Review agent accumulates redundant internal iterations on clean diffs #1370, Review-fix race condition causes duplicate review cycles and wasted tokens #1557
• Approval gating on medium-severity findings: Review agent: configurable severity threshold for reported findings #2333, Review agent should alert when human approves with unresolved Medium+ findings #2099, Review agent should withhold approval for medium-severity architectural/UX-consistency findings #2404 (recently closed)

No new proposals are warranted — the improvement opportunities from this workflow are already tracked across multiple existing issues.