feat: decouple worker concurrency from coordinator uniqueness - #229

Actionable comments posted: 2

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/config/autonomy_test.go`:
- Around line 49-69: Restructure TestCoordinatorConfigValidatesMaxActiveSessions
into three t.Run subtests (e.g., "Should validate default", "Should error when
max_active_sessions_per_workspace too low", "Should validate higher") and call
t.Parallel() inside each subtest; locate the logic around
DefaultCoordinatorConfig(), modifications to MaxActiveSessionsPerWorkspace and
Validate("autonomy.coordinator", nil). For the invalid case assert the error is
non-nil AND that the error message contains the expected text (e.g.,
"max_active_sessions_per_workspace" or the specific validation phrase) using an
ErrorContains-style check rather than only err != nil; keep the valid-case
checks identical but wrapped in their own subtests.

In `@internal/config/autonomy.go`:
- Around line 25-29: Replace the hardcoded literal 5 used by
DefaultCoordinatorMaxActiveSessionsPerWorkspace with a value retrieved from the
project's configuration/environment system: read the int option (e.g., via the
existing config package or environment variable like
COORDINATOR_MAX_ACTIVE_SESSIONS_PER_WORKSPACE) and fall back to 5 if unset;
update the initializer for DefaultCoordinatorMaxActiveSessionsPerWorkspace to
call that config lookup instead of embedding the literal so the default is
driven by the config layer.

there are some conflicts to solve @richardfogaca

The fix is functionally correct and solves a real, confirmed blocker — the coordinator counts itself in validateSpawnCaps (internal/session/spawn.go:284-307, since its normalized lineage sets RootSessionID = coordinator.ID via store/session_lineage.go:71-73), so with the budget pinned to 1 no worker can ever spawn. Repro and root cause check out, and the fix is targeted at the right line (the budget flows through spawnLineage → effectiveSpawnBudget, which doesn't touch MaxActivePerWorkspace).

That said, I don't think this is the right shape for the change, and there are gaps. Requesting changes on the following:

1. The core rationale is inaccurate

The PR argues:

Simply allowing max_active_per_workspace > 1 would instead permit multiple coordinators, which is why a separate field is the correct shape.

This isn't true. Coordinator uniqueness is enforced solely by the daemon singleton check — activeCoordinator + coordinator.HealthySession under a mutex (internal/daemon/coordinator_runtime.go:296-307,404-416). That check never consults the spawn budget. The coordinator is created via Create (startCoordinatorSession), not via Spawn, so max_active_per_workspace has never gated coordinator creation. Bumping it would only ever allow more workers — which is exactly the goal here. The premise that a second field is needed to protect uniqueness doesn't hold.

2. This leaves max_active_per_workspace as a vestigial field

After this change, max_active_per_workspace is still validated to be exactly 1 (internal/config/autonomy.go:161) and still exposed over the API, but it no longer feeds any runtime enforcement. That's a validated-but-no-op knob. Per the repo's "Greenfield Alpha — Zero Legacy Tolerance" rules (no dual fields, hard cuts, delete obsolete code instead of bridging), the cleaner fix is to repurpose the existing field to mean the session cap (default 5, drop the "must be 1" pin) and lean on the daemon singleton for uniqueness — rather than adding a parallel field and advertising "Fully backward compatible."

3. Near-duplicate naming is a footgun

max_active_per_workspace (coordinators, =1) vs max_active_sessions_per_workspace (all managed sessions, =5) differ by a single word. This is very easy to confuse for both operators and agents. If we do keep two fields, they need names that don't collide on read.

4. Partial-surface gap (agent-manageability)

The new knob materially governs worker concurrency, but it's not plumbed into CoordinatorConfigPayload (internal/api/core/agent_contracts.go:16-26), which is served at GET /api/agent/coordinator/config over both HTTP and UDS (httpapi/routes.go:143, udsapi/routes.go:166) and is part of the agent Situation Surface. As-is, an agent inspecting its coordinator config sees the now-meaningless max_active_per_workspace but not the field that actually controls its worker budget. This violates "No partial-surface completions" / "Agent-manageable by default." Please plumb the new field through the contract payload (and regenerate the OpenAPI/TS if the contract DTO changes).

5. Missing AGH Impact Audit

The PR body is missing the required four-row AGH Impact Audit (native tools / extensibility & hooks / workspace data isolation / official AGH skill). Please add it.

Suggested direction

I'd prefer the hard-cut version: rename/repurpose to a single max_active_sessions_per_workspace (default 5), delete the must be 1 validation, document that coordinator uniqueness is owned by the daemon singleton, and expose the field in the agent-facing payload. If you want to keep an explicit uniqueness invariant as defense-in-depth, that's fine — but then fix the rationale, keep it out of the spawn-budget seed entirely, and resolve the naming collision.

Also: Verify (fmt, lint, unit, build) was still pending at review time — let's get that green before merge.