Address review findings from ADR-0045 Phase 4 PR 2 (#2447)

[ggallen]

Context

PR #2447 (ADR-0045 Phase 4, PR 2: stop writing agents block during install) merged with three medium-severity review findings from @waynesun09 that should be addressed in a follow-up.

Findings

1. assert.Empty → assert.Nil in TestNewOrgConfig (internal/config/config_test.go:53)

TestNewOrgConfig uses assert.Empty(t, cfg.Agents) which passes for both nil and []AgentEntry{}. Since NewOrgConfig no longer sets the field at all, cfg.Agents should be nil (Go zero value). Use assert.Nil to enforce the stronger invariant and document that the field is truly unset, not merely empty. This matters because YAML omitempty behavior for nil vs empty slices is implementation-dependent.

Fix: Change assert.Empty(t, cfg.Agents) to assert.Nil(t, cfg.Agents).

2. Missing integration test for agents-free config.yaml output (internal/layers/configrepo_test.go)

After removing the agents parameter from newTestConfig, no test in configrepo_test.go verifies the key behavioral change: that ConfigRepoLayer.Install() writes a config.yaml that does NOT contain an agents: key. While unit-level marshal tests exist in config_test.go, there is no integration-level assertion in the layers package confirming the end-to-end behavior.

Fix: Add an assertion in TestConfigRepoLayer_Install_CreatesRepo (or a new test) that reads back the written config.yaml from client.CreatedFiles and asserts it does not contain the string agents:.

3. Consider options struct for NewOrgConfig (internal/config/config.go:115)

After removing agents, NewOrgConfig still takes 5 positional parameters including two adjacent string types (inferenceProvider, org), making call sites like NewOrgConfig(nil, nil, nil, "", "") opaque. A NewOrgConfigOpts struct would improve readability and extensibility.

Note: This is a design improvement suggestion, not a blocker. The current signature is functional and all callers are correct. Track for future consideration.

Source

PR: #2447
Reviewer: @waynesun09

[fullsend-ai-triage]

🤖 Finished Triage · ✅ Success · Started 3:47 PM UTC · Completed 3:50 PM UTC
Commit: 4e21a60 · View workflow run →

[fullsend-ai-triage]

Triage Summary

This follow-up issue tracks three review findings from the merged PR #2447 (ADR-0045 Phase 4, PR 2). All findings are clearly specified and actionable.

Findings

#	Finding	Severity	Type
1	assert.Empty → assert.Nil for cfg.Agents	Medium	Test correctness
2	Missing integration test for agents-free config.yaml	Medium	Test coverage gap
3	Options struct for NewOrgConfig	Medium	Design improvement

Recommended Approach

Findings 1 & 2 are straightforward test improvements that should be addressed together:

• Change assert.Empty(t, cfg.Agents) to assert.Nil(t, cfg.Agents) in internal/config/config_test.go
• Add an integration assertion in internal/layers/configrepo_test.go that verifies the written config.yaml does not contain agents:

Finding 3 (options struct for NewOrgConfig) is a design improvement — functional but not urgent. It could be bundled here or deferred.

Proposed Test Case

Test 1 (unit - config_test.go):
  In TestNewOrgConfig, after calling NewOrgConfig:
  - Assert cfg.Agents is nil (not merely empty)
  - This enforces the stronger invariant that the field is unset,
    which matters for YAML omitempty serialization

Test 2 (integration - configrepo_test.go):
  In TestConfigRepoLayer_Install_CreatesRepo:
  - After Install() completes, read back config.yaml from client.CreatedFiles
  - Assert the content does NOT contain the string "agents:"
  - This validates end-to-end that the agents block is no longer written

---

Labels: Follow-up chore for test and code quality improvements in the config/install component, low priority since all functionality is correct.

[fullsend-ai-coder]

🤖 Finished Code · ✅ Success · Started 3:54 PM UTC · Completed 4:00 PM UTC
Commit: 4e21a60 · View workflow run →