feat: subagent agent_ref resolution + agent description field

[Kaiohz]

Changes

Subagent selection from existing agents

• SubAgentConfig gains optional agent_ref field to reference an existing agent by name
• Factory resolves the referenced agent's config (model, system_prompt, mcp_servers, tools, response_format) at runner-build time
• Explicit values on the SubAgentConfig override the referenced agent's values
• One level only — referenced agent's own subagents are ignored

Agent description field

• AgentConfig gains optional description field
• AgentConfigMetadata gains description (persisted in PostgreSQL)
• Migration 010_add_description_to_agent_configs adds description VARCHAR(500) column

Validation & cache invalidation

• Self-reference (agent_ref == agent name) rejected with ConfigError
• Non-existent agent_ref references rejected with ConfigError
• On update/delete, dependent agents (those referencing the updated/deleted agent) are invalidated in the registry

Tests

• 469 unit tests pass (19 new tests for agent_ref/description)
• Ruff clean, mypy 0 new errors
• Trivy 0 vulnerabilities
• QA: 18/18 E2E scenarios passed (API + UI)

[Kaiohz]

Code Review — PR #38

Scope: +766 / -7 across 17 files. 1 commit. Hexagonal boundaries respected (domain / application / infrastructure). New Alembic migration 010_add_description_to_agent_configs. 19 new unit tests covering the new behavior. Ruff clean, mypy 0 new errors, Trivy 0 vulns, 18/18 E2E green per the PR description.

Score: 8/10 — Solid, well-tested, well-documented, and respects the existing hexagonal layout. A few design / nitpicks below, but nothing blocking. I'd be happy to approve after the nits are addressed (or if the author pushes back with a justified reason for any one of them).

---

👍 Highlights

• Clear separation of concerns. The new _subagent_ref_utils.py module extracts validate_subagent_refs and invalidate_dependent_agents so both create and update use cases share one validation path. Good DRY.
• Explicit-override semantics for agent_ref are well-defined and tested (test_subagent_agent_ref_explicit_override_wins).
• Defense-in-depth at the port level. validate_subagent_refs is called inside the use case before put/save, so a bad reference never reaches the store. Test mock_agent_config_store.put.assert_not_awaited() confirms this.
• Cache invalidation is correct and complete. On update/delete of agent X, both X and all dependents (other agents whose YAML lists agent_ref: X) are invalidated. invalidate_dependent_agents correctly skips agent_name itself (avoids double-invalidation) and gracefully handles corrupted YAML via try/except + log + continue so a single broken config can't poison the rest.
• Alembic migration is reversible (proper downgrade()).
• README + CONTRIBUTING updated — easy to undervalue, but this is the kind of doc update that makes the new feature actually discoverable.

---

🟡 Suggestions (non-blocking)

1. SubAgentConfig.description and agent_ref are mutually compatible but documentation says "required"

In tests/unit/test_agent_config.py:

def test_subagent_with_agent_ref_still_requires_description(self):

This is enforced by the existing description: str = Field(..., min_length=1), which is fine — but the README's new agent_ref row says "when set, the backend resolves… explicit values on the SubAgentConfig override the inherited ones", which implies the rest of the SubAgentConfig is optional. It might be worth a one-liner in the README clarifying that name + description are still required even with agent_ref, since a user naturally tries to keep their YAML minimal when referencing.

2. No cycle detection beyond self-reference

The PR description claims "one level only — referenced agent's own subagents are ignored" and the test test_subagent_agent_ref_ignores_referenced_subagents covers it. But the validation only blocks direct self-reference (sa.agent_ref == config.name). A 2-cycle is still possible:

# agent A.yaml
subagents:
  - { name: subA, description: d, agent_ref: B }
# agent B.yaml
subagents:
  - { name: subB, description: d, agent_ref: A }

Both pass validation (no self-ref), but they reference each other. At runtime, _resolve_referenced_subagent will only resolve one level, so it's safe by construction (no infinite loop), but a user who renames one of the two will get a confusing "agent not found" at the moment a runner is built. Consider either:

• documenting the limitation in the README ("transitive references are not validated"), or
• adding a cheap DFS in validate_subagent_refs that walks ref.subagents[].agent_ref one extra level to fail fast on cycles.

The first option is enough for this PR.

3. _subagent_ref_utils.invalidate_dependent_agents re-parses every YAML on every update/delete

For each update/delete, we list_all → get → yaml.safe_load every stored agent. With 10s of agents this is fine, at 1000s this will get slow. Two cheap optimizations if you ever scale:

• Index dependents in a Map<agent_name, Set<referenced_by>> populated on create (denormalized); invalidate by union.
• Or store the dependent set in the existing agent_configs table as a separate column maintained at write time.

Not a blocker — flag for the day the registry grows.

4. _resolve_referenced_subagent mixes await calls with or expressions — readability nit

instructions = await _resolve_subagent_instructions(sa, prompt_manager)
system_prompt = instructions if instructions is not None else ref.system_prompt

local_tools = _resolve_tools_list(sa.tools) if sa.tools else None
ref_tools = _resolve_tools_list(ref.tools) if ref.tools else None
mcp_tools: list = []
servers = list(sa.mcp_servers) + list(ref.mcp_servers)
if servers and mcp_tool_loader:
    mcp_tools = await mcp_tool_loader.load_tools(servers)
all_tools = (local_tools or []) + (ref_tools or []) + mcp_tools if (local_tools or ref_tools or mcp_tools) else None

The last line is hard to read because the conditional if (...) controls whether the concatenation evaluates at all. If local_tools=[], ref_tools=[], mcp_tools=['x'], you correctly get ['x']. But the precedence is subtle: a + b + c if cond else None parses as (a + b + c) if cond else None, not a + b + (c if cond else None). A future refactor is very likely to break this. A small helper or an explicit if-block would be safer.

5. The validate_subagent_refs function takes a Callable[[str], Awaitable[bool]] — leaky abstraction

AgentConfigStore and AgentConfigRepository both expose exists semantics, but the use case takes a raw callable instead of a port. That's OK for testability (the tests pass a custom side_effect), but the production wiring in create_agent_config.py passes self._config_repository.exists — so the function is implicitly coupled to a single port. If a future test wants to validate against the store vs. the repository, it'll be awkward. Consider passing a port-typed object instead, or just inlining the check in the use case (it's 5 lines).

6. assert sa.agent_ref is None after the None-check

In _resolve_referenced_subagent:

if config_resolver is None:
    raise ConfigError(...)
assert sa.agent_ref is not None
ref = await config_resolver(sa.agent_ref)

assert is stripped when Python runs with -O. Better to use a typed assertion that survives, or restructure the function to narrow the type for mypy.

7. README mentions the feature but the new validation rules are buried in the table

The "Validation" section is added at the bottom of the table description, which is good. But the bullet about "On update/delete, dependents are invalidated" is missing from the README — it's in CONTRIBUTING.md only. I'd move that bullet to the README too, since the user-facing impact (a referenced agent being silently rebuilt on update) is non-obvious.

---

🟢 Nits (purely cosmetic)

• tests/unit/test_agent_crud.py has a exists_side_effect that always returns False (return False if name == "ghost" else False) — the ternary is a no-op. A plain return False reads better.
• agent_memories_dir slice in factory.py was reformatted to item.key[len(agent_memories_dir) :] — nice PEP-8 spacing, but unrelated to the PR. Consider splitting that into a separate commit next time so the diff stays scoped to the feature.
• _metadata test helper in test_postgres_repository.py now takes description=None — keep it, but consider asserting description is None in the existing tests to lock the default behavior.

---

✅ What's Good to Ship

• All assertions in the new tests are meaningful (no assert True).
• The model_copy(update=…) pattern in update_agent_config.py is the right way to evolve the frozen Pydantic metadata.
• The use of ConfigError (domain error) rather than ValueError keeps the error mapping consistent with the rest of the codebase.
• The PR description accurately reflects the diff (no "TODO" or "fix later" hidden in the changes).

---

Verdict: Approve with optional nits. Happy to merge once the 2-cycle doc note and the assert hardening are addressed (the rest can wait).

[Kaiohz]

Code Review — PR #38 (SoluBot review)

Scope: +853 / -7 across 20 files. 2 commits. Solid implementation, well-tested (19 new tests, all meaningful), and the hexagonal boundaries are respected. I read through Kaiohz's own review (8/10) — we largely agree, so I'll focus on what I think that review missed or under-weighted, plus a few additional nits.

Score: 7.5/10 — feature itself is shippable, but the PR's title/body/diff alignment is a real problem that needs to be resolved before merge (see Major #1). After that's clarified I'd bump to 8.5/10.

---

🔴 Major — must address before merge

1. Migrations 008 and 009 (mcp_servers table + source_type/openapi_url columns) do not belong in this PR

This is the most important finding. The PR's title is "feat: subagent agent_ref resolution + agent description field" and the body explicitly says:

The MCP server registry (CRUD API, OpenAPI→MCP generation, Swagger 2.0 conversion, Fernet encryption, mounter, startup rehydration) used to live in this brick. It has been moved to [mcp-raganything], which now owns the mcp_servers PostgreSQL table and the /api/v1/mcp/servers REST surface.

…and:

SECRET_ENCRYPTION_KEY is now optional in composable-agents (it is no longer used here); it must still be set on the mcp-raganything service that owns the registry.

So the story is: the mcp_servers table is owned by mcp-raganything, and composable-agents is just a consumer. Yet the PR adds two new Alembic migrations:

• 008_create_mcp_servers_table.py — creates mcp_servers from scratch with Fernet-encrypted columns (headers_encrypted, env_encrypted, auth_token_encrypted).
• 009_alter_mcp_servers_add_openapi.py — adds source_type and openapi_url columns.

Three concrete problems with this:

1. Schema ownership ambiguity. If both composable-agents and mcp-raganything ship a migration creating/altering the same mcp_servers table, the table will get created twice on environments that happen to apply both stacks against the same DB — and any drift (column types, defaults, indexes) will bite later. A migration that creates a table this brick no longer owns is, by definition, the wrong migration to ship here.
2. README claim mismatch. The README block just added says the registry "has been moved to mcp-raganything", but the PR itself ships a migration to create that very registry's table. If the table is created here, then either (a) the README is wrong and the registry hasn't actually been moved, or (b) the migration will conflict with the migration that already exists in mcp-raganything's main branch. Either way, the PR contradicts its own narrative.
3. CI test coverage is impossible to validate. composable-agents's test suite can't run 008/009 against a real mcp_servers table without dragging the whole registry in — but the body of the PR claims Trivy is clean and 18/18 E2E scenarios pass. If those E2E scenarios include a fresh DB migration, they must be either skipping the new migrations or running against a DB that already has the table from somewhere else. The test story isn't being told honestly.

What I think happened: these two migrations were likely authored as part of the "move the registry out" work, lived in this branch first, and got accidentally swept into the squash before the "agent_ref + description" feature was rebased on top. The 2-commit history (7ced7ce = agent_ref+description, a69a640 = MCP server migrations + ruff per-file-ignores) actually proves this — the migrations are a separate commit but on the same branch and ended up in the same PR.

Suggested fix (pick one):

• Preferred: move 008_create_mcp_servers_table.py and 009_alter_mcp_servers_add_openapi.py into the mcp-raganything repo (where the table is now owned) and drop them from this PR. Keep 010_add_description_to_agent_configs here, and rebase the branch on top of the latest mcp-raganything migration chain so the head revision still descends correctly. If the down-revision 009 is needed because 010 descends from it, just point 010's down_revision at whatever the last real composable-agents migration is (presumably 007).
• Acceptable but requires justification: keep them, and amend the PR body + README to explain why this brick still owns the mcp_servers table despite the narrative saying otherwise. The current state is the worst of both worlds.

Either way, the pyproject.toml change (tests/** = ["ARG002"] ruff per-file-ignores) is a third, unrelated thing — that's a 3rd commit's worth of change riding along. The whole thing reads as a "scrub the branch of accumulated junk" PR and the title doesn't reflect it.

2. validate_subagent_refs runs after ConfigError(AGENT_NAME_MISMATCH) — fine, but its place in the use case is fragile

In create_agent_config.py:

if config.name != name:
    raise ConfigError(ErrorMessage.AGENT_NAME_MISMATCH.format(...))
await validate_subagent_refs(config, self._config_repository.exists)
if await self._config_repository.exists(name):
    raise AgentConfigAlreadyExistsError(...)

…and in update_agent_config.py, validate_subagent_refs runs after the name-mismatch check, before put, before save. That's defensible ordering, but note that the function takes self._config_repository.exists as the exists_fn callback, which couples the use case to the SQL repository's notion of "exists". If a future refactor pushes YAML configs only into the store (MinIO) and the metadata into Postgres, exists semantics will diverge between the two and validate_subagent_refs will be wrong by construction. Kaiohz's review already noted the Callable is leaky; I want to flag that the wiring is fragile too — pick a single source of truth for "does this agent exist?" (the registry, the store, or the metadata repo) and inject that port, not a method on one specific port.

3. invalidate_dependent_agents silently swallows errors twice

try:
    all_names = await config_store.list_all()
except Exception:
    logger.warning("Failed to list stored agent configs during dependent invalidation for '%s'", agent_name)
    return
…
try:
    yaml_content = await config_store.get(other_name)
    …
except Exception:
    logger.warning("Failed to parse YAML for agent '%s' during dependent invalidation", other_name)
    continue

The first try/except is fine — if we can't list, abort silently. But the second swallows all exceptions including ConfigError raised by the loader, yaml.YAMLError from safe_load, and a hypothetical KeyError if get ever returns None. Two specific risks:

• A typo in a referenced agent's YAML (e.g. agent_ref: "resercher" in a 4th agent) will be silently skipped during the next update of X and the user will see "registry rebuilt X but the dependent A is still serving a stale runner". By the time they notice, they'll have lost track of which update "broke" which dependent.
• If the YAML for an unrelated agent is corrupted (say, a half-written file from a botched kubectl exec), no admin gets a paged alert — it just gets a logger.warning that nobody reads.

Suggestion: at minimum, surface a logger.error (not warning) for the per-agent failure, and increment a counter / metric so this is observable. If you want to be safer, collect failed agents and re-raise a ConfigError listing them at the end of the loop, after all the valid ones have been invalidated. The current "log and continue" is the right default for robustness, but "log nothing" is the wrong default for observability.

---

🟡 Things I agree with Kaiohz on (re-flagged for emphasis)

These are well covered in the existing review but I want to second them because they're the next-most-impactful items:

• 2-cycle detection (A→B→A) — agree, document the limitation in the README for now, add a DFS later. This is fine to ship.
• assert sa.agent_ref is not None after the None check — assert is stripped under -O. Use a typed narrowing pattern (if not isinstance(...) raise ConfigError(...)) or restructure. Cheap fix, real foot-gun.
• all_tools = … if cond else None precedence — the operator precedence is correct today but very fragile. Split into an explicit if-block.
• The unrelated factory.py PEP-8 whitespace fix in _prepare_agent_namespace is a 3rd, unrelated change riding the branch. Split it out next time.

---

🟢 Nits

4. exists_side_effect ternary is a no-op

async def exists_side_effect(name: str) -> bool:
    return False if name == "ghost" else False

in tests/unit/test_agent_crud.py. Both branches return False. Replace with return False.

5. _metadata test helper now silently accepts description=None

The helper signature change is fine, but the 4 existing call sites that don't pass description will now produce a metadata object whose description is None — and the 3 new tests assume description round-trips through _model_to_metadata and save. Add a one-liner assertion in the existing _metadata callers (assert metadata.description is None) so the default behavior is locked. Otherwise someone will remove description from the model and not notice the helper is now lying.

6. README's "one level only" warning is missing from the agent_ref row

Kaiohz flagged the inverse (the subagents invalidation bullet missing from README). Mine is the other side: the agent_ref row in the table doesn't repeat the "one level only — referenced agent's own subagents are ignored" line. It appears in the prose below, but a user scanning just the table will not see it. Either add a column or a (⚠ one level only) suffix.

7. CONTRIBUTING.md line wrapping

The new subagents[*].agent_ref bullet is one very long line (~80 words). It will wrap poorly on most renderers. Consider breaking it into 2-3 lines.

8. Migration filename 010_add_description_to_agent_configs.py and the 008/009 downgrade branch

If we end up keeping 008 and 009 here (per Major #1, we shouldn't), note that 009's downgrade DROP COLUMN openapi_url, source_type is order-sensitive. PostgreSQL accepts this order fine, but if anyone reorders the columns in 009 later, the downgrade will need a matching reorder. A comment in the migration noting that would help.

9. name field validator

name: str = Field(..., min_length=1, max_length=100) — fine. But description: str | None = None accepts None and any length. Adding a max_length=500 Pydantic constraint (matching the DB column) would catch YAML typos at parse time instead of at DB write time. Tiny improvement, but consistent with the rest of the model.

10. Test naming inconsistency

test_subagent_with_agent_ref_still_requires_description is the only new test that doesn't follow the test_<thing>_<expected> convention. Most of the file uses that pattern. Cosmetic only.

---

🟢 What's Good to Ship

• The _subagent_ref_utils.py extraction is the right call. Both create/update go through the same validation and same invalidation logic. If you ever need a third call site (e.g. a validate_agent_yaml dry-run endpoint), it's already factored out.
• The config_resolver injection in factory.py keeps the deepagent layer free of repository knowledge. The wiring in persistent_registry/adapter.py (config_resolver = self._config_loader.load_from_string ∘ self._config_store.get) is the cleanest place for it.
• The defense-in-depth ordering in create_agent_config.py (validate name, validate refs, check existence, store) means a bad agent_ref can never reach MinIO/Postgres. Good.
• README + CONTRIBUTING are both updated. This is the most-skipped step in real PRs; not skipped here.
• try/except in _resolve_referenced_subagent for ref.subagents logs a warning instead of silently dropping the field. A user who intended a multi-level chain gets a visible signal.

---

Verdict

Request changes — primarily because of Major #1 (the migrations 008/009 inclusion). The agent_ref feature itself is solid and I would approve it in a PR that contained only the agent_ref + description + relevant tests/migration/docs changes. The unrelated registry migrations and the pyproject.toml ruff change dilute what should be a focused, easy-to-review PR.

After Major #1 is addressed (move the MCP registry migrations out, rebase the description migration chain), the remaining items are nits and the 8.5/10 score is justified. As-is, 7.5/10.

---

Note: Posted as COMMENT (not REQUEST_CHANGES) because I'm authenticated as the PR author on this token. The Verdict: Request changes line above reflects the substance of the review; the GitHub event is downgraded to COMMENT accordingly.

[Kaiohz]

Review — PR #38: feat: subagent agent_ref resolution + agent description field

Overall: 7.5/10 — solid, well-tested feature with a clean API surface, but a few correctness/performance sharp edges worth addressing before merge.

---

✅ What works well

• Clean separation of concerns. agent_ref resolution lives in _resolve_referenced_subagent in the factory, validation lives in _subagent_ref_utils.py, and the use cases just wire them in. The new helpers are easy to unit-test in isolation (and you do).
• Good DI. validate_subagent_refs accepts an exists_fn callback instead of hard-coupling to AgentConfigRepository. Easy to mock, easy to reuse.
• Override semantics are explicit and tested. sa.model if sa.model else ref.model, etc. Tests test_subagent_agent_ref_explicit_override_wins and test_subagent_agent_ref_falls_back_to_referenced_response_format lock the precedence.
• Self-reference + non-existent reference both rejected with ConfigError at create and update time (not just at build time), which prevents stale YAMLs from sneaking in.
• One-level-only restriction is documented and tested. No silent recursion.
• Dependent invalidation on update/delete — a real correctness concern (cached runners of agents referencing a deleted agent) that most refactor PRs miss. Good catch.
• Migration 010 is minimal and reversible — clean add/drop column, no data backfill needed.
• Tests are well-structured (Arrange/Act/Assert, descriptive names, mock-as-pinned-side-effect rather than return_value when needed).

---

🟡 Should fix before merge

1. _resolve_subagent_instructions uses the alias name, not the referenced agent's name — silent prompt-miss bug.

In factory.py (line ~305 in _resolve_referenced_subagent):

instructions = await _resolve_subagent_instructions(sa, prompt_manager)
system_prompt = instructions if instructions is not None else ref.system_prompt

_resolve_subagent_instructions calls prompt_manager.get_prompt_content(sa.name), where sa.name is the alias in the parent agent, not the referenced agent's name. If the user has a Phoenix prompt registered for the referenced agent, the lookup will miss and the code silently falls back to ref.system_prompt. That's probably what you want in the success path, but if a user intentionally registered a prompt under the alias name, the referenced agent's Phoenix prompt is shadowed with no warning.

Suggestion: either (a) document the alias-vs-referenced precedence in the docstring + CONTRIBUTING, or (b) try sa.name first and then fall back to ref.name explicitly. At minimum, log when the alias lookup fails so the user understands the resolution.

2. invalidate_dependent_agents is O(N) YAML re-parses on every update/delete, with no parallelism and overly broad exception swallowing.

for other_name in all_names:
    ...
    try:
        yaml_content = await config_store.get(other_name)
        data = yaml.safe_load(yaml_content) or {}
        ...
    except Exception:
        logger.warning(...)
        continue

Two concerns:

• For a tenant with hundreds of stored agents, every update re-reads + re-parses every YAML. Wrap the loop in asyncio.gather (with a semaphore) so dependent-invalidation scales with concurrency, not with N.
• Catching bare Exception masks real bugs (e.g. a malformed agent_config_store raising AttributeError). Tighten to yaml.YAMLError + KeyError and let the rest propagate, or at least include the exception in the log line.

3. The same referenced agent is loaded N times when a parent has N subagents pointing to it.

In persistent_registry/adapter.py:

async def config_resolver(name: str) -> AgentConfig:
    referenced_yaml = await self._config_store.get(name)
    return self._config_loader.load_from_string(referenced_yaml)

If parent A has 5 subagents all agent_ref: researcher, config_resolver("researcher") is called 5 times → 5 store reads + 5 YAML parses for the same data. Either memoize within one create_agent_from_config call (functools.lru_cache won't work on an async callable — use a local dict keyed by name inside the call) or refactor _resolve_subagents to resolve references up front and pass the resolved AgentConfig down.

4. Cache invalidation does not propagate when a referencing agent's YAML is itself updated to start referencing a new agent.

Only the updated agent's dependents are invalidated. That's correct as written, but it means: if I update agent A to add subagents[0].agent_ref: B, and B already has a cached runner, nothing forces B to be re-checked. The next build of A will resolve B fresh, so functionally fine, but if A's runner was already cached, the cached runner might be stale relative to B. Probably OK in practice, but worth a comment in the docstring acknowledging this asymmetry.

---

🟢 Nitpicks (non-blocking)

5. Falsy response_format is treated as "no value" in _resolve_referenced_subagent:

"response_format": sa.response_format if sa.response_format else ref.response_format,

If someone sets sa.response_format = {} (empty dict) they'd silently get the referenced agent's format. Pydantic validation likely rejects empty schemas, but use is not None for clarity and to make the intent explicit.

6. SubAgentConfig.description is still required even when agent_ref is set — the user has to describe their alias even though the model/system_prompt are inherited. Test test_subagent_with_agent_ref_still_requires_description enforces this. Consider making description optional when agent_ref is set, and defaulting it to ref.description at resolution time. (Low priority — current behavior is fine, just slightly redundant.)

7. New migration files 008 and 009 are listed as "added" in the PR but are MCP-server-registry migrations (referenced in the README update as "moved to mcp-raganything"). These are likely already applied to the target branch — confirm they don't re-run on a DB that's already at rev 007. If they're additive and the target DB has never reached 008, fine. If you can collapse them into a single forward migration (010) you avoid the awkward Revises: 009 chain on a feature that has nothing to do with MCP.

8. CI change dropping continue-on-error on the SonarQube step is unrelated to the feature and bundled into the same PR. SonarQube failures will now block merge — which is probably what you want, but worth calling out in the PR description so reviewers don't miss it. If it was a flake-fix, say so.

9. pyproject.toml per-file-ignores ARG002 for tests/** is also unrelated to the feature. If it's the same flake-fix, fine; otherwise, split into a separate PR so the agent_ref/description review stays focused.

10. README has a stray "Registry migration" blockquote (line ~330) that describes the removal of the MCP registry. If this PR doesn't actually remove the registry (the factory still imports McpServerConfig, McpToolLoader, etc.), this blockquote is documenting work done in a different PR. Either link that PR or move the blockquote to the other PR's description.

---

🧪 Test coverage gaps

• No test for "create referenced agent first, then create referencing agent" end-to-end. You have separate tests for the create path and the resolution path, but not a single scenario that uses the full persistent_registry.get_runner flow with two agents.
• No test for mcp_servers merging in _resolve_referenced_subagent (servers = list(sa.mcp_servers) + list(ref.mcp_servers)). Worth a unit test to lock the merge order.
• No test for the empty-list case in invalidate_dependent_agents (zero stored agents → should no-op cleanly).
• No test that the description field propagates through the full create→read round-trip at the HTTP route level. The Postgres + use-case tests are fine, but an E2E confirmation would be nice for parity with the existing QA scenario count.

---

📊 Score breakdown

Criterion	Score
Code quality / readability	8/10
Architecture / separation of concerns	8/10
Test coverage	7/10
Performance considerations	5/10 (O(N) scan + N×resolution, no parallelism)
Migration safety	8/10
Documentation	8/10
CI hygiene (bundled unrelated changes)	5/10

Weighted total: ~7.5/10

Solid PR, ship it after addressing #1 and #2. #3, #5, #6 are quick wins. The unrelated CI/config changes (#7, #8, #9) are worth either splitting or calling out in the PR description.

---

Review posted by SoluBot on behalf of the SoluDevTech maintainers. CI ✅ (test=success). 21 files, +853/-8. Mergeable.

[Kaiohz]

Review — feat: subagent agent_ref resolution + agent description field

Verdict: APPROVE (avec nits)

Globalement, c'est un PR propre, bien testé, bien documenté, qui suit l'architecture hexagonale existante. Le découpage en commits est propre (feat: × 2, Fail CI on SonarQube, fix(review):), le dernier commit a corrigé deux points remontés en review précédente (injection du port AgentConfigRepository au lieu d'une closure, et narration README), ce qui montre une vraie réactivité.

Score: 8.5/10

---

✅ Points forts

• Architecture hexagonale respectée — la résolution passe par un config_resolver: Callable[[str], Awaitable[AgentConfig]] injecté dans le factory, ce qui évite de coupler la couche infrastructure à un store concret. Très propre.
• Validation au bon endroit — validate_subagent_refs est appelé dans create_agent_config et update_agent_config (pas seulement à l'exécution), donc les agents cassés sont rejetés avant d'être persistés. Le ConfigError est l'exception adaptée.
• Cache invalidation correcte — l'invalidation des dépendants sur update/delete évite les stale runners. Le pattern de scan YAML avec yaml.safe_load (validation légère) + log d'erreur sur parse failures + liste récapitulative finale est robuste face aux configs corrompues.
• README + CONTRIBUTING.md mis à jour — y compris la nouvelle section dédiée à agent_ref avec les règles de résolution/override, et le bloc "Registry migration" qui clarifie l'ownership partagé du schéma mcp_servers avec mcp-raganything. Excellent pour les nouveaux contributeurs.
• Migrations propres — 010_add_description_to_agent_configs utilise ADD COLUMN ... VARCHAR(500) (nullable, pas de default), pas de IF NOT EXISTS car Alembic protège déjà. Idempotent en pratique. Les migrations 008/009 (table mcp_servers) sont ré-importées ici depuis mcp-raganything (ownership du schéma), bien commenté.
• Couverture de tests — 19 nouveaux tests (validation self-ref, non-existent, override explicite, response_format hérité, subagents de la ref ignorés, dependent invalidation sur create/update/delete, persistance description). Le test critique test_subagent_agent_ref_ignores_referenced_subagents valide bien la règle "one level only".
• Ruff per-file-ignores (tests/** = ["ARG002"]) — pragmatique et bien scopé, pas de pollution du lint en prod.
• Fail CI on SonarQube errors — bonne hygiène CI, on ne peut plus merger silencieusement avec un scan cassé.

---

🔸 Nits (à corriger ou à noter)

1. Test test_rejects_subagent_agent_ref_pointing_to_nonexistent_agent — side_effect sans valeur ajoutée
   Dans tests/unit/test_agent_crud.py, le side_effect est :

   async def exists_side_effect(name: str) -> bool:
       return False if name == "ghost" else False  # toujours False

   Le name == "ghost" else False est un copier-coller accidentel — la condition est redondante. Le test passe quand même (parce que return False matche tous les cas), mais c'est du code trompeur qui ne teste pas vraiment le chemin "ghost" spécifiquement. Soit supprimer la condition (return False), soit la réécrire en l'inversant pour rendre l'intention explicite :

   async def exists_side_effect(name: str) -> bool:
       return name != "ghost"  # seul ghost "n'existe pas"

   Bonus : ça documenterait pourquoi on a besoin du side_effect (au lieu du return_value = False par défaut).

2. Migration 010 : pas de IF NOT EXISTS sur ADD COLUMN
   Alembic protège déjà contre le double-run, donc ce n'est pas un bug. Mais par cohérence avec 009 qui utilise ADD COLUMN IF NOT EXISTS, ajouter le guard rendrait la migration ré-exécutable à la main (utile en recovery post-incident). Pas bloquant.

3. Factory : all_tools calculé via une expression booléenne fragile
   Dans factory.py :

   all_tools = (local_tools or []) + (ref_tools or []) + mcp_tools if (local_tools or ref_tools or mcp_tools) else None

   La condition est non-DRY (on répète l'OR 3 fois) et l'opérateur de précédence est piégeux (le if ... else None ne s'applique qu'à la dernière concaténation). Suggestion :

   combined = (local_tools or []) + (ref_tools or []) + mcp_tools
   all_tools = combined if combined else None

   Pareil dans le chemin non-ref (lignes 248-255) — c'est pré-existant, donc hors scope, mais c'est une bonne occasion de fixer les deux.

4. invalidate_dependent_agents : N+1 latence sur grands parcs
   Sur une base de 500 agents, invalider un seul agent déclenche 499 config_store.get() séquentiels (boucle for ... await). Pour la plupart des cas OK, mais à noter si le parc grossit. Une option : asyncio.gather sur la lecture (les écritures invalidate doivent rester séquentielles pour garder l'ordre du log, mais les lectures peuvent être parallélisées). Pas urgent.

5. SubAgentConfig sans extra="forbid"
AgentConfig a model_config = ConfigDict(frozen=True, extra="forbid") mais SubAgentConfig n'a que frozen=True. Conséquence : un typo (mcp_server au lieu de mcp_servers) sur un subagent passe silencieusement. Pré-existant, pas une régression de cette PR — mais le nouveau champ agent_ref aurait été une bonne occasion d'ajouter le guard. À ouvrir dans une PR séparée si vous voulez resserrer.

6. Pas de test pour le warning "Referenced agent has its own subagents; ignoring them"
   Le log logger.warning(...) dans _resolve_referenced_subagent n'est pas couvert. Ajouter un test qui vérifie que le warning est émis (avec caplog) et que la résolution continue de fonctionner.

7. Pas de test pour la chaîne mcp_servers combinée (sa + ref)
   Le code fait servers = list(sa.mcp_servers) + list(ref.mcp_servers) mais aucun test ne vérifie (a) que les deux listes sont bien concaténées, (b) que les doublons ne sont pas dédupliqués (si c'est intentionnel, le confirmer ; sinon ajouter une dédup).

---

📋 Questions ouvertes

• Override de mcp_servers : merge ou replace ? Le code fait servers = list(sa.mcp_servers) + list(ref.mcp_servers) (merge additif), tandis que model / response_format font du replace (sa.X si sa.X sinon ref.X). Est-ce volontaire ? C'est asymétrique et mériterait d'être documenté dans le README ("Agent references" section) — un user qui set mcp_servers sur le subagent s'attendrait probablement à remplacer, pas à merger.
• Cycle detection — validate_subagent_refs détecte la self-reference, mais un cycle A → B → A passerait au create de A (au moment de A, B existe déjà, donc OK), puis au create de B (au moment de B, A existe déjà, mais B n'a pas encore été créé — donc OK aussi). Le code factory ne résout qu'un niveau, donc l'exécution n'explose pas, mais c'est un trou de validation. À ouvrir en follow-up.

---

✅ Verdict

• Pas de bug bloquant.
• 19 tests ajoutés, 469 passent, ruff clean, mypy 0 nouvelles erreurs, trivy 0 vuln, QA E2E 18/18 OK.
• Le commit fix(review): ... montre que la review précédente a été traitée.
• PR prête à merger après nit Configure Renovate #1 (side_effect) et idéalement un test pour le warning du subagent ignoré.

Score final : 8.5/10.

🤖 Generated by SoluBot

[Kaiohz]

Re-Review — PR #38 (post-fix, head a1bd30e)

Scope now: +793 / -8 across 19 files, 5 commits, mergeable ✅
Previous score: 7.5/10 → New score: 8.5/10 — recommend merge after the two nits below.

The two new commits since my last review (2c32ab6 and a1bd30e) directly address the two blockers I flagged. That's the right move and it shows the review feedback loop is working.

---

✅ What got fixed (delta vs. my previous review)

1. validate_subagent_refs now takes a port, not a callable — fixed (2c32ab6)

Before:

async def validate_subagent_refs(
    config: AgentConfig,
    repository: Callable[[str], Awaitable[bool]],   # ← leaky
) -> None:

After:

async def validate_subagent_refs(
    config: AgentConfig,
    repository: AgentConfigRepository,              # ← port-typed
) -> None:

Clean. The docstring's argument for why a port beats a callable is also a useful comment for future maintainers ("avoids drift if the store/repository split is refactored later"). My concern #2 from the previous review is fully addressed.

2. invalidate_dependent_agents error handling is now observable — mostly fixed (2c32ab6)

Before: per-agent failures → logger.warning(...) + continue, end of story.
After:

• Per-agent failures escalate to logger.error(..., exc_info=True).
• Failed agents are accumulated in failed_agents: list[str].
• A summary logger.error line at the end lists every skipped agent with the count.

That's a real observability win — an SRE grepping the logs for "Dependent invalidation" will now get a clear summary of what's broken instead of N individual lines buried in noise. My concern #3 from the previous review is fully addressed.

3. Migrations 008/009 are out of the PR — fixed (a1bd30e)

The mcp_servers table ownership is now explicitly in mcp-raganything (commit message: "refactor(alembic): move mcp_servers table ownership to mcp-raganything"). The diff in this PR is back to a clean 19 files: only the description migration (010) remains, and it correctly down-revs from 007 (no orphan chain). My Major #1 from the previous review is fully addressed.

4. README narrative corrected — fixed (2c32ab6)

The "Registry migration" blockquote now matches reality (the registry is actually in mcp-raganything on this branch), and the description-vs-mcp story is internally consistent.

---

🟡 Should still address before merge (low effort)

A. description is str | None with no max_length=500 constraint on the Pydantic model

src/domain/entities/agent_config.py:

description: str | None = None

The DB column is VARCHAR(500) and the migration enforces that, but a YAML with a 10k-character description will pass Pydantic validation and only fail when the SQL INSERT happens — which is the wrong place to discover a user typo. One-line fix, same shape as the existing name: str = Field(..., min_length=1, max_length=100):

description: str | None = Field(default=None, max_length=500)

This is the one substantive item left. Cheap to fix, makes the Pydantic model honest about its own DB schema.

B. _resolve_referenced_subagent mixes operator-precedence ternaries — still a foot-gun

From factory.py:

all_tools = (local_tools or []) + (ref_tools or []) + mcp_tools if (local_tools or ref_tools or mcp_tools) else None

This is correct today but reads as (a + b + c) if cond else None, not a + b + (c if cond else None). A future "let me just add a small thing" PR will break the precedence silently. The fix is a 4-line explicit block:

if local_tools or ref_tools or mcp_tools:
    all_tools = (local_tools or []) + (ref_tools or []) + mcp_tools
else:
    all_tools = None

Not blocking, but it's the kind of nit that becomes a bug in 6 months. Worth a one-minute refactor while the file is open.

---

🟢 Remaining nits (cosmetic, ship as-is if you want)

C. description falsy-coalesce — same pattern as B, same fix shape

"response_format": sa.response_format if sa.response_format else ref.response_format,

Should be is not None for the same reason: an empty {} would silently get overwritten by the referenced agent's format. Pydantic probably rejects empty schemas, but the intent reads cleaner with is not None.

D. The factory.py PEP-8 whitespace fix on item.key[len(agent_memories_dir) :] is still in this PR

It's a one-byte change and I won't block on it, but for next time: please split unrelated style nits into their own commit (or a separate PR) so feature reviews stay focused. The CI is now flagging it would be visible if it were alone.

E. agent_ref row in README still doesn't repeat "one level only" inline

The warning is in the prose block below the table, but a user scanning the table won't see it. Add (⚠ one level only) to the agent_ref cell or to the description column. 5-second fix.

F. assert sa.agent_ref is not None in _resolve_referenced_subagent is still an assert

This was on my previous list. assert is stripped under python -O. Easy to swap to a typed narrowing, but since the only caller gates on sa.agent_ref is not None immediately above, the assert is essentially defensive — fine to keep if you don't care about -O users (no one does for FastAPI services).

G. No memoization in config_resolver for repeated references in the same get_runner call

Still an issue (noted in the previous review): if parent A has 5 subagents all agent_ref: researcher, config_resolver("researcher") is called 5 times → 5 store reads + 5 YAML parses for the same data. A local dict[str, AgentConfig] populated on first call would fix it. Performance is fine for the current scale (handful of subagents), but if you ever onboard a tenant with a "library" of 50 reusable agents, this becomes the bottleneck. Not blocking.

---

🧪 Test coverage delta

The new commits didn't add new tests, but the existing 19 new tests are still well-structured (Arrange/Act/Assert, descriptive names, side-effect-based mocks). The gaps I flagged previously (full E2E get_runner flow, mcp_servers merge order, empty-list case) remain unaddressed, but I agree they're not blockers for this PR.

One micro-coverage suggestion: add a test that asserts description round-trips through the full _model_to_metadata → save → get cycle on the same db_session — the existing test_save_persists_description and test_maps_description_from_model_to_metadata are split across two tests, which is fine, but a single integration test would lock the contract at the seam.

---

📊 Updated score breakdown

Criterion	Previous	Now	Notes
Code quality / readability	8	8	Same — item A & B are 5-min fixes
Architecture / separation	8	9	Port injection fix is a real improvement
Test coverage	7	7	Same — no new tests in the 2 fix commits
Performance	5	5	Memoization still missing (#G)
Migration safety	8	9	Ownership now clean, single 010
Documentation	8	8	Same — item E is a 5-sec fix
CI hygiene (bundled unrelated)	5	6	MCP migrations out, but the factory.py whitespace & pyproject.toml per-file-ignores still in

Weighted total: ~8.5/10 (up from 7.5).

---

Verdict

Approve. The two new commits turned a 7.5 into a clean 8.5. Ship it after addressing items A and B (10 minutes total); items C–G are nits that can wait for the next PR.

Posted as COMMENT (not REQUEST_CHANGES) — no remaining blockers, just polish items. CI ✅ (test=success). Mergeable ✅. 5 commits, 19 files, +793/-8.

---

Review posted by SoluBot on behalf of the SoluDevTech maintainers. Re-review of the same author's previous version; based on a1bd30ecab7b09c799dba89c5d53868e69bc5b42 (HEAD of feature/subagent-ref-and-description).