fix(agents): isolate swarm agent configs to prevent fallback resolution cross-contamination#1237
Conversation
🤖 Dual-Model PR ReviewPipeline: Qwen3.6-35B-APEX (broad review) → Gemma-4-26B (adversarial verify + blind-spot) PR Review: fix(agents): isolate swarm agent configs to prevent fallback resolution cross-contamination🔍 PR Intent
📦 Implementation SummaryThe PR replaces a module-level singleton ✅ /
|
| Obligation | Status | Evidence (file:line) |
|---|---|---|
| O-001 | SUPPORTED |
src/agents/index.ts:46-49 |
| O-002 | SUPPORTED |
src/agents/index.ts:78-91 |
| O-003 | SUPPORTED |
src/agents/index.ts:121-128 |
| O-004 | SUPPORTED |
src/hooks/guardrails.ts:3669 and src/hooks/guardrails.ts:3726 |
| O-005 | SUPPORTED |
src/agents/index.ts:125 |
| O-006 | SUPPORTED |
src/agents/index.ts:335 |
🚨 Confirmed Findings
[HIGH] Regex edge case: _coder extracts empty string as swarm ID
- Location:
src/agents/index.ts:89 - Why it matters: The regex
/^([^_]+)_/matches an agent name starting with an underscore (e.g.,_coder) and captures an empty string as the first group. This results inextractSwarmIdFromAgentNamereturning"". WhengetSwarmAgents("")is called, it returnsundefined, causing fallback resolution to fail for any agent intended to be in the "default" swarm but accidentally prefixed with an underscore. - Evidence:
For
const match = agentName.match(/^([^_]+)_/); return match ? match[1] : undefined;
_coder,match[1]is"". Since""is truthy in the ternary, it returns"". - Fix direction: Use a regex that requires at least one non-underscore character before the underscore, e.g.,
/^([^_{}]+)_/or validate that the result is not an empty string.
[MEDIUM] Test coverage gap: Guardrails integration
- Location:
tests/unit/agents/multi-swarm-fallback.test.ts - Why it matters: The PR's primary goal is to fix fallback resolution in the
guardrails.tshook. However, the new tests only verify the utility functions (extractSwarmIdFromAgentNameandgetSwarmAgents) in isolation. There is no test verifying that thecreateGuardrailsHookslogic actually uses the extractedswarmIdto fetch the correct fallback models. - Evidence:
tests/unit/agents/multi-swarm-fallback.test.tscontains no imports or assertions related tosrc/hooks/guardrails.ts. - Fix direction: Add an integration test that simulates a model failure within a specific swarm and verifies that the fallback model is pulled from that swarm's specific config.
[LOW] Inconsistent swarm ID extraction logic
- Location:
src/hooks/guardrails.ts:3674andsrc/hooks/guardrails.ts:3731 - Why it matters: The guardrails hook uses inline regex
replace(/^[^_]+[_]/, '')to strip the prefix, whilesrc/agents/index.tsusesmatch(/^([^_]+)_/). While functionally similar for the intended use case, this duplication increases maintenance surface area. - Evidence:
src/hooks/guardrails.ts:3674:session.agentName.replace(/^[^_]+[_]/, '')src/agents/index.ts:89:agentName.match(/^([^_]+)_/)
- Fix direction: Use the exported
extractSwarmIdFromAgentNameto determine the prefix, then use a shared utility to strip it, ensuring consistency.
🔬 Unverified but Plausible Risks
-
Risk: Module-level state leakage in tests: The
_swarmAgentsMapis a module-levelMapthat is not cleared. If tests are run in a single process without a reset mechanism, configuration from one test could leak into another.- Note: The provided test file does not show a
beforeEachthat clears the map.
- Note: The provided test file does not show a
-
Risk: Session
agentNameformat assumptions: The logic assumessession.agentNamealways follows theswarmId_agentNamepattern. If a session is initialized with justagentName(no prefix),extractSwarmIdFromAgentNamereturnsundefined, which defaults to"default". This is correct for the default swarm, but if the system evolves to have multiple "unprefixed" swarms, this will fail.
🔁 Cross-model verification
- CONFIRMED:
Regex edge case: _coder extracts empty string(Verified viasrc/agents/index.ts:89). - CONFIRMED:
Test assertion: getSwarmAgents('default') returns {}(Refuted: This is actually expected behavior for uninitialized swarms in this implementation, not a bug, though it is a design choice). - CONFIRMED:
No test coverage for guardrails hook integration(Verified: Tests only coversrc/agents/index.ts). - CONFIRMED:
Inconsistent swarm ID extraction logic(Verified: Inline regex vs helper regex).
📝 Merge Recommendation
APPROVE_WITH_FIXES
The architectural change to use a Map is correct and necessary. However, the following must be addressed:
- Fix the regex in
src/agents/index.ts:89to prevent empty string swarm IDs. - Add integration tests in
tests/unit/agents/multi-swarm-fallback.test.tsthat actually invoke the guardrails logic to ensure the fix works in the real execution path.
🔒 Reviewed locally by two on-prem models for blind-spot coverage. Findings are advisory — verify before acting.
- F-001 (HIGH): Fix extractSwarmIdFromAgentName('_coder') test assertion
(expect undefined, not empty string)
- F-002 (HIGH): Pre-seed _swarmAgentsMap with 'default' entry so
getSwarmAgents('default') always returns defined value
- F-003 (MEDIUM): Reject underscore-containing swarm IDs in schema
validation (regex /^[^_]+$/)
- F-005 (LOW): Add negative isolation assertion verifying
fastAgents !== preciseAgents (cross-contamination guard)
- Plus biome formatting fixes
Closure ledger for review feedbackCommit \3d25e575\ addresses the following findings: Fixed
Advisory (non-blocking, noted for follow-up)
CI Status (commit \3d25e575)
|
🤖 Dual-Model PR ReviewPipeline: Qwen3.6-35B-APEX (broad review) → Gemma-4-26B (adversarial verify + blind-spot) 🔍 PR Intent
📦 Implementation SummaryThe PR replaces a module-level singleton ✅ /
|
| Obligation | Status | Evidence (file:line) |
|---|---|---|
| O-001 | SUPPORTED |
src/agents/index.ts:46-52 |
| O-002 | SUPPORTED |
src/agents/index.ts:81-99 |
| O-003 | SUPPORTED |
src/agents/index.ts:193-203 |
| O-004 | SUPPORTED |
src/hooks/guardrails.ts:3673-3680, 3732-3739 |
| O-005 | SUPPORTED |
src/config/schema.ts:1820-1823 |
| O-006 | SUPPORTED |
src/agents/index.ts:778 |
| O-007 | SUPPORTED |
tests/unit/agents/multi-swarm-fallback.test.ts |
🚨 Confirmed Findings
[HIGH] Breaking Change: Schema validation rejects existing underscore-based swarm IDs
- Location:
src/config/schema.ts:1820-1823 - Why it matters: The PR introduces a regex constraint
/^[^_]+$/on swarm IDs. The PR documentation (docs/releases/pending/multi-swarm-agent-config-fallback.md:35) explicitly claims "Migration notes: None required — this is a pure bugfix. Existing configs continue to work." This is false. Any user currently using a swarm ID containing an underscore (e.g.,{"swarms": {"local_dev": ...}}) will experience a schema validation failure on startup. - Fix direction: Either document this as a breaking change or allow underscores in swarm IDs (and adjust the agent name prefixing logic to be more robust, e.g., using a different delimiter).
[MEDIUM] Logic Flaw: Agents in non-default swarms without prefixes resolve to 'default' config
- Location:
src/agents/index.ts:81-99andsrc/hooks/guardrails.ts:3673 - Why it matters: The
extractSwarmIdFromAgentNamefunction returnsundefinedif no underscore is found. Inguardrails.ts, ifswarmIdisundefined,getSwarmAgentsdefaults to'default'. If a user has a swarm namedproductionand an agent namedcoder(without theproduction_prefix), the system will attempt to resolve fallbacks from thedefaultswarm instead of theproductionswarm. While the prefixing convention is intended, the current implementation fails to provide a way to associate an unprefixed agent with a specific non-default swarm. - Fix direction: Ensure the system can explicitly map unprefixed agents to specific swarms, or strictly enforce prefixing in documentation/validation.
[MEDIUM] STEALTH_CHANGE: Schema validation added without PR mention
- Location:
src/config/schema.ts:1820-1823 - Why it matters: The PR title and description focus on the fallback resolution bug. The addition of a new validation rule that breaks existing configurations is a significant change that was omitted from the PR summary and migration notes.
🔬 Unverified but Plausible Risks
- Risk:
session.agentNamemight not be populated at the timecreateGuardrailsHooksis called.- Why suspicious: If the session is initialized before the agent name is set, the
swarmIdextraction will always returnundefined, causing all agents to fall back to thedefaultswarm.
- Why suspicious: If the session is initialized before the agent name is set, the
🧪 Test / Coverage Gaps
- Gap: No unit tests for the new schema validation logic.
- Evidence:
tests/unit/agents/multi-swarm-fallback.test.tstests the logic of the helper functions but does not verify thatPluginConfigSchemacorrectly rejects underscores in swarm IDs.
- Evidence:
- Gap: No integration tests for the
guardrails.tshook.- Evidence: The new tests only cover the
agents/index.tslogic. The actual runtime interaction whereguardrails.tscallsgetSwarmAgentsis not exercised in a full integration test.
- Evidence: The new tests only cover the
📋 Shipped-vs-Claimed Gaps
- Gap: PR claims "Existing configs continue to work" but the schema change breaks them.
- Evidence:
docs/releases/pending/multi-swarm-agent-config-fallback.md:35vssrc/config/schema.ts:1820.
- Evidence:
📝 Merge Recommendation
[APPROVE_WITH_FIXES]
The core logic for isolating swarm configurations is sound and the implementation of the Map is correct. However, the PR contains a breaking change (schema validation) that is explicitly denied in the documentation, and it introduces a potential configuration mismatch for non-default swarms.
Required Fixes:
- Update
docs/releases/pending/multi-swarm-agent-config-fallback.mdto reflect the breaking change regarding underscores in swarm IDs. - Add unit tests for the schema validation to prevent regressions.
- (Optional but recommended) Add integration tests for the guardrails hook to ensure the
swarmIdis correctly extracted and used during fallback resolution.
🔁 Cross-model verification
- CONFIRMED: Schema validation is a breaking change (Underscores in swarm IDs).
- CONFIRMED: Agent name without prefix in non-default swarm resolves to wrong config.
- CONFIRMED: Stealth change (Schema validation not mentioned in PR description).
- CONFIRMED: Shipped-vs-Claimed gap (Migration notes claim no breaking changes).
- REFUTED: None.
🔒 Reviewed locally by two on-prem models for blind-spot coverage. Findings are advisory — verify before acting.
In multi-swarm configurations, fallback model resolution was consulting only the last-processed swarm's agent config for ALL swarms due to a single-slot module variable being overwritten on each iteration. This caused silent failures where swarms with different
fallback_modelswould resolve to wrong or missing models.Changes
Replace single-slot singleton with swarm-keyed map
_swarmAgentsfrom a single variable to_swarmAgentsMap: Map<swarmId, config>Extract swarm prefix from agent names
extractSwarmIdFromAgentName(agentName)helper function"local_coder"→"local")undefinedfor unprefixed names (default swarm)Update getSwarmAgents() to support swarm-specific lookups
getSwarmAgents(swarmId?: string)(defaults to"default")Wire swarmId through guardrails fallback resolution
swarmIdfromsession.agentNamebefore config lookupswarmIdtogetSwarmAgents(swarmId)in both fallback locationsExample
{ "swarms": { "fast": { "agents": { "coder": { "fallback_models": ["fast/fallback"] } } }, "precise": { "agents": { "coder": { "fallback_models": ["precise/fallback"] } } } } }Before fix:
fast_coderhitting a model error would resolvefast/fallbackonly iffastwas the last swarm in iteration order; otherwise it would useprecise/fallback(wrong).After fix:
fast_coderalways resolvesfast/fallbackbecauseextractSwarmIdFromAgentName("fast_coder")returns"fast", which is passed togetSwarmAgents("fast").