refactor(config): remove OrgConfig.Agents field (ADR-0045 Phase 4 PR 4)

[ggallen]

Summary

• Remove AgentEntry type, OrgConfig.Agents field, AgentSlugs(), and HasAgentsBlock() from internal/config/
• Inline Role, Name, Slug fields directly into layers.AgentCredentials (previously embedded via config.AgentEntry)
• Update all construction sites in admin.go, github.go, mint_test.go, and layer tests
• Remove all Agents-related tests: TestOrgConfigAgentSlugs, TestParseOrgConfig_WithoutAgentsBlock, TestParseOrgConfig_EmptyAgentsList, TestHasAgentsBlock, TestOrgConfigMarshal_NilAgentsOmitted, TestOrgConfigMarshal_EmptyAgentsOmitted
• Remove agents: [] from test fixture YAML

Context

This is Phase 4, PR 4 of ADR-0045 (Forge-Portable Harness Schema) — the final PR. PRs 1-3 required role in Validate(), stopped writing the agents: block during install, and removed legacy discovery fallbacks. This PR removes the field itself, completing the migration: agent identity now lives exclusively in harness wrapper files.

PR dependency graph (Phase 4)

PR 1 (require role in Validate)   [independent, still open as #2446]
PR 2 (#2447, merged) ──────────────> PR 4 (this)
PR 3 (#2448, merged) ──────────────> PR 4 (this)

Test plan

• go build ./... — clean
• go vet ./... — clean
• go test ./internal/config/... ./internal/layers/... ./internal/cli/... — all pass
• Pre-commit hooks pass (gofmt, yaml, etc.)
• grep -rn 'AgentEntry' --include='*.go' returns no results
• grep -rn 'agents:' --include='*.yaml' returns no results in config fixtures
• No remaining callers of AgentSlugs() or HasAgentsBlock()

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Refactor: remove OrgConfig agents block and inline AgentCredentials identity
✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Description

• Remove legacy agents config schema (AgentEntry, OrgConfig.Agents, helpers) per ADR-0045
 Phase 4.
• Inline agent identity fields (Role, Name, Slug) into layers.AgentCredentials.
• Update CLI/layer call sites and fixtures; delete tests that validated deprecated agents behavior.

Diagram

graph TD
  A["OrgConfig (defaults)"] --> B["CLI setup"] --> C(["AgentCredentials"]) --> D(["Layers (secrets/wrappers)"]) --> E{{"Forge client"}} --> F[("Repo files")]
  G["Web config fixtures"] --> A
  subgraph Legend
    direction LR
    _cfg["Config"] ~~~ _cli["CLI"] ~~~ _layer(["Layer"]) ~~~ _ext{{"External"}} ~~~ _repo[("Storage")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep a deprecated type alias for one release

• ➕ Allows downstream/internal callers to migrate gradually (e.g., type AgentEntry = layers.AgentCredentials or a shim struct).
• ➕ Reduces risk of hidden compile breaks in less-traveled packages/plugins.
• ➖ Prolongs the presence of legacy terminology/schema after ADR completion.
• ➖ Can create ambiguity about the source of truth for agent identity (config vs harness).

2. Provide an explicit migration/validation error message

• ➕ If any configs still contain agents:, users get a targeted remediation message.
• ➕ Avoids silent ignoring if YAML contains unknown fields (depending on decoder behavior).
• ➖ Adds extra code solely for a transitional state that ADR Phase 4 aims to remove.
• ➖ May be unnecessary if parsing already rejects/flags unknown keys in this repo.

Recommendation: The PR’s approach (fully removing OrgConfig.Agents and inlining identity into layers.AgentCredentials) is the cleanest end-state for ADR-0045 Phase 4 and reduces future maintenance. If compatibility concerns remain, consider a short-lived type alias/shim, but otherwise the hard removal is appropriate for completing the migration.

Files changed (10) +67 / -219

Refactor (4) +10 / -35

admin.goConstruct AgentCredentials without config.AgentEntry embedding +5/-7

Construct AgentCredentials without config.AgentEntry embedding

• Updates dry-run, app setup, and analyze flows to set 'Role/Name/Slug' directly on 'layers.AgentCredentials' instead of populating the removed 'config.AgentEntry' field.

internal/cli/admin.go

github.goUpdate GitHub setup to use direct Role field +1/-1

Update GitHub setup to use direct Role field

• Replaces 'config.AgentEntry{Role: ...}' construction with direct 'Role' assignment on 'layers.AgentCredentials'.

internal/cli/github.go

config.goRemove AgentEntry and OrgConfig.Agents schema and helpers +0/-24

Remove AgentEntry and OrgConfig.Agents schema and helpers

• Deletes the 'AgentEntry' type, the 'OrgConfig.Agents' field, and the 'AgentSlugs()' / 'HasAgentsBlock()' helper methods. This completes the removal of the legacy 'agents:' org config block.

internal/config/config.go

secrets.goInline agent identity fields into AgentCredentials +4/-3

Inline agent identity fields into AgentCredentials

• Replaces the embedded 'config.AgentEntry' with explicit 'Role', 'Name', and 'Slug' fields on 'AgentCredentials', clarifying ownership of agent identity within layers.

internal/layers/secrets.go

Tests (5) +57 / -183

admin_test.goUpdate admin CLI tests to use direct Role field +3/-3

Update admin CLI tests to use direct Role field

• Adjusts installation-related tests to build 'layers.AgentCredentials' with 'Role' directly, matching the new struct shape.

internal/cli/admin_test.go

mint_test.goUpdate mint tests to return AgentCredentials with direct Slug +2/-2

Update mint tests to return AgentCredentials with direct Slug

• Removes the nested 'AgentEntry' initialization in mocked return values, setting 'Slug' directly on 'layers.AgentCredentials'.

internal/cli/mint_test.go

config_test.goDelete agents-block tests and remove agents from fixtures +9/-141

Delete agents-block tests and remove agents from fixtures

• Removes assertions and tests that validated 'agents' parsing/marshaling and deprecation behavior, and updates embedded YAML test data to omit the 'agents:' block entirely.

internal/config/config_test.go

harnesswrappers_test.goUpdate harness wrapper tests to remove config dependency +23/-24

Update harness wrapper tests to remove config dependency

• Drops the 'internal/config' import and constructs 'AgentCredentials' with 'Role/Name/Slug' fields directly throughout the harness wrapper layer tests.

internal/layers/harnesswrappers_test.go

secrets_test.goUpdate secrets layer tests for new AgentCredentials shape +20/-13

Update secrets layer tests for new AgentCredentials shape

• Removes 'config.AgentEntry' usage in test fixtures and sets identity fields directly on 'AgentCredentials' instances.

internal/layers/secrets_test.go

Other (1) +0 / -1

config-valid.yamlRemove empty agents block from config fixture +0/-1

Remove empty agents block from config fixture

• Updates the web/admin configrepo fixture YAML to remove the now-invalid 'agents: []' entry.

web/admin/src/lib/layers/fixtures/configrepo/config-valid.yaml

[github-actions]

Site preview

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

Commit: b76d35d85d76f0a6da71b16ba808c2dbba26413d

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:09 PM UTC · Completed 4:20 PM UTC
Commit: edcc268 · View workflow run →

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 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. PEM blocks hardcoded in tests 📘 Rule violation ⛨ Security

Description

internal/layers/secrets_test.go hardcodes RSA private key PEM blocks (and related identifiers)
directly in source. This can trip secret-scanning and risks accidental reuse of sensitive material,
violating the ban on hardcoded secrets/sensitive identifiers.

Code

internal/layers/secrets_test.go[R24-39]

func twoAgents() []AgentCredentials {
	return []AgentCredentials{
		{
-			AgentEntry: config.AgentEntry{Role: "fullsend", Name: "FullsendBot", Slug: "fullsend-bot"},
-			PEM:        "----***********************************************************************************************************************************************************************************************************************************************--\nfullsend-key\n-----END RSA PRIVATE KEY-----",
+			ClientID: "Iv1.abc111",
		},
		{
-			AgentEntry: config.AgentEntry{Role: "triage", Name: "TriageBot", Slug: "triage-bot"},
-			PEM:        "----***************************************************************************************************************************************************************************************************************************************--\ntriage-key\n-----END RSA PRIVATE KEY-----",
+			ClientID: "Iv1.abc222",
		},

Relevance

⭐⭐⭐ High

Repo emphasizes secret-scanning/redaction; hardcoded PRIVATE KEY blocks likely flagged and removed
even in tests.

PR-#1448
PR-#337

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062040 disallows hardcoded secrets/cryptographic material (including PEM blocks).
The modified test code embeds RSA private key PEM blocks directly in string literals.

Rule 1062040: Disallow hardcoded secrets and sensitive environment-specific identifiers in source code
internal/layers/secrets_test.go[24-39]
internal/layers/secrets_test.go[83-99]

Agent prompt

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

## Issue description
`internal/layers/secrets_test.go` includes hardcoded PEM private key blocks (e.g., strings containing `-----BEGIN RSA PRIVATE KEY-----`). Even if intended as test data, this matches disallowed cryptographic material patterns and can trigger secret scanners.

## Issue Context
The tests only need a stable dummy string to verify it is threaded into secret creation; they do not appear to require real PEM formatting.

## Fix Focus Areas
- internal/layers/secrets_test.go[24-39]
- internal/layers/secrets_test.go[83-99]

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

---

2. Admin UI schema drift 🐞 Bug ≡ Correctness

Description

After removing OrgConfig.Agents, the web admin code still derives agent roles exclusively from
cfg.agents, so configs produced by the current schema will result in an empty agent list and the
secrets-layer analysis will report "installed" without checking any secrets/variables. Additionally,
the web secrets analyzer checks FULLSEND_{ROLE}_APP_ID variables while the backend uses
FULLSEND_{ROLE}_CLIENT_ID, so UI status can be wrong even when roles are provided.

Code

internal/config/config.go[86]

-	Agents                 []AgentEntry          `yaml:"agents,omitempty"`

Relevance

⭐⭐⭐ High

Team recently migrated APP_ID→CLIENT_ID and built admin analyzers; UI/backend drift likely treated
as correctness bug.

PR-#318
PR-#617

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The backend OrgConfig struct no longer includes an Agents field, but the web admin parser still
models/parses agents and uses it to build the agent list passed into secrets analysis; when
agents is absent, the secrets analyzer iterates over an empty list and reports installed. The
web secrets analyzer also checks for ..._APP_ID variables, while the backend constructs variable
names with ..._CLIENT_ID.

internal/config/config.go[72-87]
web/admin/src/lib/layers/orgConfigParse.ts[3-14]
web/admin/src/lib/layers/orgConfigParse.ts[122-140]
web/admin/src/lib/layers/orgConfigParse.ts[164-200]
web/admin/src/lib/orgs/orgListRow.ts[84-113]
web/admin/src/lib/layers/secrets.ts[5-12]
web/admin/src/lib/layers/secrets.ts[32-53]
internal/layers/secrets.go[183-189]

Agent prompt

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

## Issue description
The Go org config schema removed the `agents` block (`OrgConfig.Agents`), but the web admin YAML parser and layer analyzers still:
- model/validate an `agents:` list,
- use it as the only input to secrets-layer analysis,
- and check the wrong repo variable name suffix (`APP_ID` vs backend `CLIENT_ID`).

This causes the admin UI to incorrectly mark the secrets layer as installed (no checks performed) and/or to report incorrect missing variables.

## Issue Context
Backend schema (`internal/config/config.go`) no longer contains `agents`, so configs written by the CLI will omit it. Web admin must derive roles from the remaining config fields (at minimum `defaults.roles`) and match backend secret/variable naming.

## Fix Focus Areas
- web/admin/src/lib/layers/orgConfigParse.ts[3-14]
- web/admin/src/lib/layers/orgConfigParse.ts[164-200]
- web/admin/src/lib/orgs/orgListRow.ts[84-113]
- web/admin/src/lib/layers/secrets.ts[5-12]
- web/admin/src/lib/layers/secrets.ts[32-69]

### Concrete fix direction
1. Update `OrgConfigYaml` to remove (or stop relying on) `agents`, and replace `agentsFromConfig` with a function that derives agent roles from `defaults.roles` (and/or harness wrapper discovery if that’s the new source of truth).
2. Update `analyzeOrgForOrgList` to pass the derived roles to `analyzeSecretsLayer`.
3. Update `variableNameForRole` to match the backend (`FULLSEND_{ROLE}_CLIENT_ID`).
4. Update/extend role validation to match backend `ValidRoles()` (or otherwise ensure the UI doesn’t reject configs that the backend accepts).

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

---

[codecov]

Codecov Report

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

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

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Low

• [missing-authorization] No linked issue. This non-trivial change (367 lines across 13 files) is architecturally authorized by ADR-0045 and the Phase 4 plan, with dependencies on merged PRs refactor(config): stop writing agents block during install (ADR-0045 Phase 4, PR 2) #2447 and refactor(config): remove legacy agent discovery fallbacks (ADR-0045 Phase 4 PR 3) #2448. A tracking issue would formalize authorization but is not blocking.

• [stale-reference] web/admin/src/lib/layers/orgConfigParse.ts:196 — The doc comment says "mirrors config.OrgConfig.Agents" but this PR removes OrgConfig.Agents from Go. The TypeScript code still functions correctly (cfg.agents ?? [] handles absence), but the comment becomes factually incorrect post-merge.
  Remediation: File a follow-up issue to update the comment on line 196.

---

Previous review fixed: The prior review's high finding about docs/plans/adr-0045-forge-portable-harness-phase4.md:55 (self-contradiction in "What Phase 4 does NOT do" section) is resolved — line 55 now uses strikethrough with a "Done" annotation. The prior low finding about the inventory table lacking checkmarks is also resolved — rows now show ✅ with PR references.

Previous run

Review

Findings

High

• [stale-reference] docs/plans/adr-0045-forge-portable-harness-phase4.md:55 — The "What Phase 4 does NOT do" section states "Does NOT remove AgentEntry from config.go — it is still used by AgentCredentials in internal/layers/secrets.go". This PR does exactly that — it removes AgentEntry and inlines its fields into AgentCredentials. The diff updates line 293 (changing "Keep AgentEntry" to "Remove AgentEntry — fields inlined") but does not update line 55, creating a direct self-contradiction within the same document.
  Remediation: Remove the bullet at line 55 or rewrite it to state that AgentEntry was removed and its fields were inlined into AgentCredentials.

Low

• [stale-reference] web/admin/src/lib/layers/orgConfigParse.ts:196 — The doc comment on line 196 says "mirrors config.OrgConfig.Agents" but this PR removes OrgConfig.Agents from Go. The TypeScript code still functions correctly (cfg.agents ?? [] handles absence), but the comment becomes factually incorrect post-merge.
  Remediation: File a follow-up issue to clean up the TypeScript config parser, or at minimum update the comment on line 196.

• [stale-reference] docs/plans/adr-0045-forge-portable-harness-phase4.md:78 — The inventory table references AgentEntry, AgentSlugs(), and HasAgentsBlock() with pre-PR locations and behaviors. After this PR executes these removals, the table rows lack completion checkmarks.

---

Previous review fixed: The prior review's high finding about toAgentCredentials splitting the filterSlugsByAppSet doc comment is resolved — a blank line separates the function from the comment, preserving correct Go doc association.

Previous run

Review

Findings

High

• [logic-error] internal/cli/admin.go:1990 — The toAgentCredentials function is inserted between the first line of the filterSlugsByAppSet doc comment (// filterSlugsByAppSet returns a new map containing only entries whose slug) and the continuation of that comment (// matches the convention...). After the patch, Go associates the first comment line with toAgentCredentials (wrong function name in godoc), and filterSlugsByAppSet loses the first line of its doc comment, leaving it with an orphaned fragment.
  Remediation: Move toAgentCredentials (with its own doc comment) either before the filterSlugsByAppSet comment block or after the filterSlugsByAppSet function body. Do not split the existing comment block.

Low

• [stale-reference] web/admin/src/lib/layers/orgConfigParse.ts:197 — The agentsFromConfig() function, the agents field on OrgConfigYaml (line 12), validation logic (lines 122-139 and 188-192), and the comment on line 196 (mirrors config.OrgConfig.Agents) still reference the agents config block that this PR removes from Go's OrgConfig. The TypeScript parses raw YAML independently so this won't break at runtime, but the comment will be factually incorrect post-merge.
  Remediation: File a follow-up issue to clean up the TypeScript config parser, or at minimum update the comment on line 196.

• [stale-reference] docs/plans/adr-0045-forge-portable-harness-phase4.md:293 — The phase 4 plan states "Keep AgentEntry type (lines 20-24) — it is still used by layers.AgentCredentials". This is now factually incorrect since this PR removes AgentEntry and inlines its fields.
  Remediation: Update the plan document to note that PR 4 removed AgentEntry and inlined its fields.

• [stale-open-question] docs/ADRs/0045-forge-portable-harness-schema.md:689 — The Open Questions section says "The current OrgConfig.Agents field uses yaml:"agents" without omitempty" in present tense. Phase 3 already added omitempty, and this PR removes the field entirely — the statement is doubly outdated.
  Remediation: Update to past tense to reflect that Phase 4 has removed the field.

---

Labels: PR is a Go refactoring chore removing deprecated config types as part of ADR-0045 migration.

---

Labels: PR is a Go refactoring chore removing deprecated config types as part of ADR-0045 migration.

Previous run

Review

Findings

High

• [stale-reference] docs/plans/adr-0045-forge-portable-harness-phase4.md:55 — The "What Phase 4 does NOT do" section states "Does NOT remove AgentEntry from config.go — it is still used by AgentCredentials in internal/layers/secrets.go". This PR does exactly that — it removes AgentEntry and inlines its fields into AgentCredentials. The diff updates line 293 (changing "Keep AgentEntry" to "Remove AgentEntry — fields inlined") but does not update line 55, creating a direct self-contradiction within the same document.
  Remediation: Remove the bullet at line 55 or rewrite it to state that AgentEntry was removed and its fields were inlined into AgentCredentials.

Low

• [stale-reference] web/admin/src/lib/layers/orgConfigParse.ts:196 — The doc comment on line 196 says "mirrors config.OrgConfig.Agents" but this PR removes OrgConfig.Agents from Go. The TypeScript code still functions correctly (cfg.agents ?? [] handles absence), but the comment becomes factually incorrect post-merge.
  Remediation: File a follow-up issue to clean up the TypeScript config parser, or at minimum update the comment on line 196.

• [stale-reference] docs/plans/adr-0045-forge-portable-harness-phase4.md:78 — The inventory table references AgentEntry, AgentSlugs(), and HasAgentsBlock() with pre-PR locations and behaviors. After this PR executes these removals, the table rows lack completion checkmarks.

---

Previous review fixed: The prior review's high finding about toAgentCredentials splitting the filterSlugsByAppSet doc comment is resolved — a blank line separates the function from the comment, preserving correct Go doc association.

---

Labels: PR is a Go refactoring chore removing deprecated config types as part of ADR-0045 migration.

Previous run (2)

Review

Findings

High

• [logic-error] internal/cli/admin.go:1990 — The toAgentCredentials function is inserted between the first line of the filterSlugsByAppSet doc comment (// filterSlugsByAppSet returns a new map containing only entries whose slug) and the continuation of that comment (// matches the convention...). After the patch, Go associates the first comment line with toAgentCredentials (wrong function name in godoc), and filterSlugsByAppSet loses the first line of its doc comment, leaving it with an orphaned fragment.
  Remediation: Move toAgentCredentials (with its own doc comment) either before the filterSlugsByAppSet comment block or after the filterSlugsByAppSet function body. Do not split the existing comment block.

Low

• [stale-reference] web/admin/src/lib/layers/orgConfigParse.ts:197 — The agentsFromConfig() function, the agents field on OrgConfigYaml (line 12), validation logic (lines 122-139 and 188-192), and the comment on line 196 (mirrors config.OrgConfig.Agents) still reference the agents config block that this PR removes from Go's OrgConfig. The TypeScript parses raw YAML independently so this won't break at runtime, but the comment will be factually incorrect post-merge.
  Remediation: File a follow-up issue to clean up the TypeScript config parser, or at minimum update the comment on line 196.

• [stale-reference] docs/plans/adr-0045-forge-portable-harness-phase4.md:293 — The phase 4 plan states "Keep AgentEntry type (lines 20-24) — it is still used by layers.AgentCredentials". This is now factually incorrect since this PR removes AgentEntry and inlines its fields.
  Remediation: Update the plan document to note that PR 4 removed AgentEntry and inlined its fields.

• [stale-open-question] docs/ADRs/0045-forge-portable-harness-schema.md:689 — The Open Questions section says "The current OrgConfig.Agents field uses yaml:"agents" without omitempty" in present tense. Phase 3 already added omitempty, and this PR removes the field entirely — the statement is doubly outdated.
  Remediation: Update to past tense to reflect that Phase 4 has removed the field.

---

Labels: PR is a Go refactoring chore removing deprecated config types as part of ADR-0045 migration.

Previous run (3)

Review

Findings

High

• [logic-error] internal/cli/admin.go:1990 — The toAgentCredentials function is inserted between the first line of the filterSlugsByAppSet doc comment (// filterSlugsByAppSet returns a new map containing only entries whose slug) and the continuation of that comment (// matches the convention...). After the patch, Go associates the first comment line with toAgentCredentials (wrong function name in godoc), and filterSlugsByAppSet loses the first line of its doc comment, leaving it with an orphaned fragment.
  Remediation: Move toAgentCredentials (with its own doc comment) either before the filterSlugsByAppSet comment block or after the filterSlugsByAppSet function body. Do not split the existing comment block.

Low

• [stale-reference] web/admin/src/lib/layers/orgConfigParse.ts:197 — The agentsFromConfig() function, the agents field on OrgConfigYaml (line 12), validation logic (lines 122–139 and 188–192), and the comment on line 196 (mirrors config.OrgConfig.Agents) still reference the agents config block that this PR removes from Go's OrgConfig. The TypeScript parses raw YAML independently so this won't break at runtime, but the comment will be factually incorrect post-merge.
  Remediation: File a follow-up issue to clean up the TypeScript config parser, or at minimum update the comment on line 196.

• [stale-reference] docs/plans/adr-0045-forge-portable-harness-phase4.md:293 — The phase 4 plan states "Keep AgentEntry type (lines 20-24) — it is still used by layers.AgentCredentials". This is now factually incorrect since this PR removes AgentEntry and inlines its fields.
  Remediation: Update the plan document to note that PR 4 removed AgentEntry and inlined its fields.

• [stale-open-question] docs/ADRs/0045-forge-portable-harness-schema.md:689 — The Open Questions section says "The current OrgConfig.Agents field uses yaml:\"agents\" without omitempty" in present tense. Phase 3 already added omitempty, and this PR removes the field entirely — the statement is doubly outdated.
  Remediation: Update to past tense to reflect that Phase 4 has removed the field.

---

Labels: PR is a Go refactoring chore removing deprecated config types as part of ADR-0045 migration.

Previous run (4)

Review

Findings

Medium

• [stale-reference] web/admin/src/lib/layers/orgConfigParse.ts:197 — The agentsFromConfig() function, the agents field on OrgConfigYaml (line 12), validation logic (lines 122–139), and validateOrgConfig iteration (lines 188–192) still reference the agents config block that this PR removes from Go's OrgConfig. The comment on line 196 (mirrors config.OrgConfig.Agents) will be factually incorrect post-merge, and agentsFromConfig() is called from production code in orgListRow.ts:92.
  Remediation: Either remove the agents handling from the TypeScript config parser in this PR, or file a follow-up issue. At minimum, update the comment on line 196.

Low

• [stale-reference] docs/superpowers/plans/2026-06-11-triage-prerequisites.md:58 — Go code snippets on lines 58, 74 reference Agents: []AgentEntry{} on OrgConfig. After this PR, AgentEntry no longer exists. These are illustrative examples in a plan document (not executable code), so risk of confusion is low.
  Remediation: Remove the Agents: []AgentEntry{} lines or note them as outdated.

• [stale-example] docs/ADRs/0045-forge-portable-harness-schema.md:35 — The ADR Context section labels a YAML example # config.yaml (current) showing the agents: block. Now that Phase 4 removes this field, the label is mildly imprecise (though ADR Context sections are inherently point-in-time records).
  Remediation: Update the label to # config.yaml (before ADR-0045) or # config.yaml (legacy).

• [struct-comment-clarity] internal/layers/secrets.go:13 — The updated comment "holds agent identity and app credentials for layer operations" could enumerate the identity fields for clarity.
  Remediation: Consider: "holds agent identity (role, name, slug) and app credentials for layer operations."

---

Labels: PR removes config types and modifies install/harness layer code

[fullsend-ai-review]

See the review comment for full details.

[waynesun09]

Review Squad Report — 8 agents (Claude, Gemini, Codex)

Overall: LGTM — Go-side changes are clean, complete, and correct. Safe to merge.

LOW — Plan deviation: AgentEntry inlined (improvement over plan)

The Phase 4 plan explicitly says "Does NOT remove AgentEntry" but this PR removes it and inlines Role, Name, Slug directly into AgentCredentials. This is a better outcome — cleaner decoupling, no cross-package dependency. Consider updating the plan doc to reflect what shipped.

Flagged by 3 agents (consensus)

LOW — Missing explicit backward-compatibility test for legacy agents: YAML

The Phase 4 plan recommends: "Add a test: parse config YAML that has an agents: block → verify it parses without error." No such dedicated test exists. Backward compat is implicitly verified by residual agents: [] in other test fixtures, but there's no named test documenting the contract. Consider adding TestParseOrgConfig_IgnoresLegacyAgentsBlock.

Flagged by 3 agents (consensus)

LOW — Web admin findings (stalled, recommend separate issues)

The web admin frontend has three stale code paths now diverged from the backend. Since web admin development has stalled, these are LOW priority and should be tracked as separate issues rather than blocking this PR:

1. agentsFromConfig() reads removed agents block — orgConfigParse.ts:197-198 returns (cfg.agents ?? []).map(...) which will always be [], causing analyzeSecretsLayer to vacuously report "installed." Should derive roles from defaults.roles instead.

2. variableNameForRole wrong suffix — secrets.ts:10 generates FULLSEND_{ROLE}_APP_ID but Go backend uses FULLSEND_{ROLE}_CLIENT_ID (internal/layers/secrets.go:187). Pre-existing bug.

3. VALID_ROLES has 4 roles vs Go's 8 — orgConfigParse.ts:18 defines ["fullsend", "triage", "coder", "review"] but Go ValidRoles() includes fix, retro, prioritize, e2e. Pre-existing.

These three should each get a tracking issue so they aren't lost.

Flagged by 5 agents (strong consensus)

---

Assisted-by: Claude (review), Gemini (review), Codex (review)

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 7:19 PM UTC · Completed 7:31 PM UTC
Commit: b5e4f87 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 7:45 PM UTC · Completed 7:58 PM UTC
Commit: e4cf824 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 8:05 PM UTC · Ended 8:39 PM UTC
Commit: 4e21a60 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:43 PM UTC · Completed 8:59 PM UTC
Commit: b76d35d · View workflow run →

[ralphbean]

LGTM.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 10:34 PM UTC · Completed 10:43 PM UTC
Commit: b76d35d · View workflow run →

[fullsend-ai-retro]

Retro: PR #2517 — refactor(config): remove OrgConfig.Agents field

Overall assessment: This human-authored refactoring PR had a smooth review cycle with good-quality feedback from both automated reviewers and the human review squad. The main inefficiency was two wasted review runs caused by a known bug (GitHub API 422 when posting inline comments outside the PR diff), and four merge queue attempts before successful merge.

Timeline

1. 16:05 — PR created by ggallen (13 files, net -113 lines)
2. 16:11–16:20 — Automated reviews (qodo-code-review, fullsend-ai-review, codecov) ran successfully with low-severity findings
3. 17:52 — Review squad (8 agents: Claude/Gemini/Codex) posted LGTM with 3 LOW findings (plan deviation, missing backward-compat test, web admin drift)
4. 19:10 — ggallen responded to all findings, filed follow-up issues #2526, #2527, #2528 for pre-existing web admin TS drift
5. 19:15 — Force-push with comment clarity fix, triggering re-review
6. 19:19, 19:45 — Two review runs failed with 422 Unprocessable Entity when submitting inline comments on lines outside the PR diff
7. 20:05 — Third review run cancelled (superseded by new push)
8. 20:43 — Fourth review run succeeded on final commit
9. 20:58 — Human approval from ralphbean
10. 21:49–22:29 — Four merge queue attempts before successful merge (~40 min delay)

Review quality

Review quality was strong across all sources. The fullsend-ai-review bot, qodo-code-review bot, and waynesun09's review squad all identified real (if low-severity) issues. The human author responded thoughtfully — filing follow-up issues for pre-existing problems rather than blocking the PR, and fixing the actionable comment clarity suggestion.

Existing issue coverage

The primary inefficiency (422 errors on inline comment submission) is already tracked by #1067 ("post-review: handle GitHub API 422 on inline comment submission gracefully"), which remains open. A related issue #1349 suggests it was partially addressed by PR #1348, but this PR demonstrates the fix is incomplete — the review agent still fails when it attempts to post REQUEST_CHANGES reviews with inline comments referencing lines not in the diff. The log shows the agent detected the problem ("2 inline comment(s) omitted (file not in PR diff)") but the submission still failed, suggesting the filtering logic has gaps.

No new proposals

No new proposals are warranted because:

• The 422 inline comment bug is already covered by post-review: handle GitHub API 422 on inline comment submission gracefully #1067
• The merge queue flakiness (4 attempts) appears to be CI/infrastructure-related rather than a fullsend platform issue, and without access to check statuses the root cause cannot be determined
• Review quality, rework rate, and time-to-resolution were all reasonable for a 13-file refactoring PR