feat: expand template presets, rosters, and add inheritance (#80, #81, #84)#162
feat: expand template presets, rosters, and add inheritance (#80, #81, #84)#162
Conversation
…#84) Phase A — Personality Presets (#80): - Add 5 new presets: user_advocate, process_optimizer, growth_hacker, technical_communicator, systems_thinker (total: 20) - Add 12 auto-name pools covering all BUILTIN_ROLES Phase B — Agent Rosters (#81): - Expand all 7 template YAMLs to target sizes (2→25 agents) - Add min_agents/max_agents metadata passthrough in loader - full_company uses Jinja2 {% for %} loops for configurable num_backend_devs/num_frontend_devs variables Phase C — Template Inheritance (#84): - Add `extends` field to CompanyTemplate schema - New merge.py module with agent merge by (role, department) key, department merge by name, _remove directive support - Renderer resolves inheritance after Jinja2 rendering with circular detection and depth limiting (max 10 levels) - New TemplateInheritanceError exception - 5 new observability event constants for inheritance tracing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… docs Pre-reviewed by 9 agents, 26 findings addressed: - Add logging before all raise paths in renderer.py (9 paths) and loader.py (11 paths) using structured event constants - Extract _resolve_agent_personality helper from _expand_single_agent to keep functions under 50 lines - Replace list[list[Any]] with _ParentEntry dataclass in merge.py - Add DEFAULT_MERGE_DEPARTMENT shared constant between merge/renderer - Add field_validator to normalize extends field (lowercase/stripped) - Narrow except Exception to except (ValidationError, TypeError) in presets.py - Remove dead MULTI_LEVEL_CHILD_YAML from test conftest - Tighten >= assertions to exact counts in inheritance tests - Add 10 new tests: extends/normalize, _remove alias, parent variable collection, escalation_paths fallback, None child scalar merge - Document template inheritance in DESIGN_SPEC.md §14.1 - Add merge.py to §15.3 project structure - Add inheritance and alias conventions to §15.5 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds template inheritance with an Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant Renderer as TemplateRenderer
participant Loader as TemplateLoader
participant Merge as MergeEngine
participant Validator as SchemaValidator
Client->>Renderer: render(template_name)
Renderer->>Loader: load_template(child_name)
Loader-->>Renderer: TemplateSource (child)
alt child.extends present
Renderer->>Loader: load_template(parent_name)
Loader-->>Renderer: TemplateSource (parent)
Renderer->>Renderer: render Jinja → child_dict / parent_dict
Renderer->>Merge: merge_template_configs(parent_dict, child_dict)
Merge-->>Renderer: merged_dict
else no extends
Renderer->>Renderer: render Jinja → child_dict
Note right of Renderer: merged_dict = child_dict
end
Renderer->>Validator: validate(merged_dict) -> RootConfig
Validator-->>Renderer: RootConfig
Renderer-->>Client: RootConfig
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a powerful template inheritance system, allowing for more modular and reusable company configurations. By enabling templates to extend others and defining clear merge strategies, it significantly enhances flexibility in defining agent rosters and company structures. The changes also expand the range of pre-defined company templates and personality presets, making it easier to set up diverse simulations. Alongside these core features, the PR includes robust logging improvements and code quality enhancements, ensuring a more stable and maintainable system. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a powerful template inheritance feature. A thorough security audit confirmed no significant vulnerabilities, with the implementation following best practices such as SSTI Mitigation, Path Traversal Protection, Safe Merging, Input Validation, Recursion Control, and Secure Defaults. The implementation is well-structured with a new merge.py module and clean refactoring in renderer.py, and includes comprehensive tests, documentation updates, and logging hardening. I have one minor suggestion for improving code clarity.
src/ai_company/templates/renderer.py
Outdated
| return [ | ||
| _expand_single_agent(agent, idx, used_names) | ||
| for idx, agent in enumerate(raw_agents) | ||
| ] |
There was a problem hiding this comment.
Using a list comprehension with a function (_expand_single_agent) that has side effects on a shared object (used_names) can be less clear than an explicit for loop. For better readability and to make the state mutation more obvious, consider reverting to a standard loop as was used previously.
| return [ | |
| _expand_single_agent(agent, idx, used_names) | |
| for idx, agent in enumerate(raw_agents) | |
| ] | |
| expanded_agents: list[dict[str, Any]] = [] | |
| for idx, agent in enumerate(raw_agents): | |
| expanded_agents.append(_expand_single_agent(agent, idx, used_names)) | |
| return expanded_agents |
There was a problem hiding this comment.
Pull request overview
This PR implements template inheritance (extends directive), expands personality presets and agent rosters for all 7 built-in company templates, and adds a dedicated merge module for template config composition. It closes issues #80 (personality presets), #81 (full agent rosters), and #84 (template inheritance).
Changes:
- Template inheritance via
extendsfield onCompanyTemplate, with circular detection (frozenset chain tracking, max depth 10), and merge semantics for agents (by role+department), departments (by name), config (deep merge), and scalars (child wins). - New
merge.pymodule with_ParentEntrydataclass,_removedirective support, andmerge_template_configsas the public merge entry point. Schema additions include_removealias onTemplateAgentConfigandextendsfield with normalization validator. - Expanded personality presets (5 new: user_advocate, process_optimizer, growth_hacker, technical_communicator, systems_thinker), expanded auto-name pools, full agent rosters for all 7 templates with min/max agent constraints, and structured logging before all raise paths.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/ai_company/templates/merge.py |
New module for template config merging with agent/department/scalar merge strategies |
src/ai_company/templates/renderer.py |
Inheritance resolution (_render_to_dict, _resolve_inheritance, _collect_parent_variables), extracted _resolve_agent_personality, added structured logging |
src/ai_company/templates/schema.py |
Added extends field with normalization, remove alias field, agent count validation skip for extending templates |
src/ai_company/templates/loader.py |
Added min_agents/max_agents passthrough, extends passthrough, structured logging on error paths |
src/ai_company/templates/presets.py |
5 new personality presets, expanded auto-name pools, narrowed exception catches |
src/ai_company/templates/errors.py |
New TemplateInheritanceError exception class |
src/ai_company/templates/__init__.py |
Exported TemplateInheritanceError |
src/ai_company/observability/events/template.py |
11 new event constants for inheritance and logging hardening |
src/ai_company/templates/builtins/*.yaml |
Full agent rosters, min/max constraints, personality presets for all 7 templates |
tests/unit/templates/test_inheritance.py |
Comprehensive tests for merge, inheritance, circular detection, depth limits |
tests/unit/templates/test_renderer.py |
Roster count range tests, variable override test |
tests/unit/templates/test_schema.py |
Tests for _remove alias, extends field, normalization |
tests/unit/templates/test_presets.py |
Tests for new presets validation, auto-name coverage |
tests/unit/templates/test_loader.py |
Tests for min/max passthrough |
tests/unit/templates/conftest.py |
Test YAML fixtures for inheritance scenarios |
DESIGN_SPEC.md |
Documented template inheritance in §14.1, merge.py in §15.3, conventions in §15.5 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/ai_company/templates/schema.py
Outdated
| def _normalize_extends(cls, value: str | None) -> str | None: | ||
| """Normalize extends to lowercase stripped form.""" | ||
| if value is None: | ||
| return None |
There was a problem hiding this comment.
The _normalize_extends validator with mode="before" could receive non-string types from malformed YAML (e.g., extends: 123 produces an int). Calling .strip() on a non-string would raise AttributeError instead of a clean ValidationError. Consider adding a type guard: if the value is not a string, either coerce it with str(value) or return the value as-is to let Pydantic's type validation reject it.
| def _normalize_extends(cls, value: str | None) -> str | None: | |
| """Normalize extends to lowercase stripped form.""" | |
| if value is None: | |
| return None | |
| def _normalize_extends(cls, value: Any) -> Any: | |
| """Normalize extends to lowercase stripped form.""" | |
| if value is None: | |
| return None | |
| if not isinstance(value, str): | |
| # Let Pydantic's type system handle or reject non-string values. | |
| return value |
| extends: NotBlankStr | None = Field( | ||
| default=None, | ||
| description="Parent template name for inheritance", | ||
| ) |
There was a problem hiding this comment.
The extends field is missing from the CompanyTemplate docstring's Attributes section. The docstring lists all other attributes but omits extends, which is inconsistent. Please add an entry like:
extends: Parent template name for inheritance (``None`` for standalone).
Similarly, the TemplateAgentConfig docstring above (line 83-93) is missing the remove attribute.
DESIGN_SPEC.md
Outdated
| - role: "full_stack_developer" | ||
| department: "engineering" | ||
| _remove: true # removes matching parent agent by (role, department) |
There was a problem hiding this comment.
The example uses role: "full_stack_developer" (snake_case) but the actual templates and role catalog use "Full-Stack Developer" (Title-Case with hyphens). Since the merge key uses role.lower(), this role name wouldn't match the parent's "Full-Stack Developer" ("full-stack developer" vs "full_stack_developer"). The example should use "Full-Stack Developer" to accurately illustrate how _remove works.
Greptile SummaryThis PR delivers template inheritance ( One critical logic bug remains unfixed:
Confidence Score: 2/5
Sequence DiagramsequenceDiagram
participant U as User
participant R as renderer.py
participant L as loader.py
participant M as merge.py
U->>R: render_template(loaded, variables)
R->>R: _render_to_dict(loaded, _chain=∅)
R->>R: _collect_variables(template, variables)
R->>R: Jinja2 render → YAML parse
R->>R: _build_config_dict() → child_config<br/>[_expand_agents drops merge_id ⚠️]
alt template.extends is None
R-->>U: child_config (no inheritance)
else template.extends = "parent_name"
R->>R: _validate_inheritance_chain(child_id, parent_name, _chain)<br/>• circular check: parent_name ∈ _chain?<br/>• depth check: len(_chain) ≥ 10?
R->>L: load_template(parent_name)
L-->>R: parent_loaded
R->>R: _collect_parent_variables(parent, child_vars)
R->>R: _render_to_dict(parent_loaded,<br/>_chain=_chain ∪ {parent_name})
note over R: Recursive — handles multi-level chains
R->>M: merge_template_configs(parent_config, child_config)
M->>M: scalars: child wins if non-None
M->>M: config dict: deep_merge()
M->>M: agents: _merge_agents() by (role, dept, merge_id)<br/>[merge_id always "" due to renderer bug ⚠️]
M->>M: departments: _merge_departments() by name
M->>M: workflow_handoffs / escalation_paths: child replaces
M-->>R: merged_config
R->>R: deep_merge(defaults, merged_config)
R->>R: _validate_as_root_config()
R-->>U: RootConfig
end
|
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ai_company/templates/loader.py (1)
427-439:⚠️ Potential issue | 🟠 MajorPass through the new handoff and escalation fields during normalization.
CompanyTemplatehas dedicatedworkflow_handoffsandescalation_pathsfields, but_normalize_template_data()drops both. That means pass-1 validation and inheritance resolution never see YAML that defines them.🛠️ Proposed fix
result: dict[str, Any] = { "metadata": metadata, "variables": data.get("variables", ()), "agents": data.get("agents", ()), "departments": data.get("departments", ()), "workflow": data.get("workflow", "agile_kanban"), "communication": data.get("communication", "hybrid"), "budget_monthly": _to_float(company.get("budget_monthly", 50.0)), "autonomy": _to_float(company.get("autonomy", 0.5)), + "workflow_handoffs": data.get("workflow_handoffs", ()), + "escalation_paths": data.get("escalation_paths", ()), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ai_company/templates/loader.py` around lines 427 - 439, The normalization currently drops workflow_handoffs and escalation_paths so definitions never reach CompanyTemplate; update _normalize_template_data (the function building result: dict[str, Any]) to include "workflow_handoffs": data.get("workflow_handoffs", ()) and "escalation_paths": data.get("escalation_paths", ()) in the result dict (same style as "agents"/"departments") so pass-1 validation and inheritance see these fields.src/ai_company/templates/renderer.py (1)
417-421:⚠️ Potential issue | 🟡 MinorDon't mask invalid
companyvalues.Line 417 converts any falsy value into
{}, socompany: [],company: "", orcompany: 0bypasses the mapping check below and silently falls back to defaults. Only substitute an empty mapping when the field is actually missing ornull.🐛 Proposed fix
- company = rendered_data.get("company") or {} + company = rendered_data.get("company") + if company is None: + company = {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ai_company/templates/renderer.py` around lines 417 - 421, The current code masks invalid non-mapping `company` values by using `rendered_data.get("company") or {}`; change the logic in the renderer so you only default to an empty mapping when the `company` key is absent or its value is explicitly null, otherwise preserve the original value and then validate its type; specifically, replace the `company = rendered_data.get("company") or {}` pattern with a check like "if 'company' not in rendered_data or rendered_data['company'] is None: company = {} else: company = rendered_data['company']" and keep the existing type check that logs TEMPLATE_RENDER_YAML_ERROR and raises TemplateRenderError when `company` is present but not a dict.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ai_company/templates/builtins/agency.yaml`:
- Around line 79-103: The duplicate engineering entries must be made uniquely
addressable so template inheritance can target by level; update the built-in
agent specs (e.g., the two "Full-Stack Developer" entries) to include a unique
identifier or include "level" in the merge key used by the inheritance logic so
`_merge_agents()` can distinguish them (for example add a unique "id" or "seat"
field or modify the merge key to combine role+level); ensure the change is
applied to the entries with role "Full-Stack Developer" (the senior and mid
entries) so a child template can override or remove a specific level without
restating duplicates.
In `@src/ai_company/templates/builtins/product_team.yaml`:
- Around line 59-68: Two Backend Developer entries collide under the new
inheritance merge key (which uses (role, department)), so child templates cannot
target or override one entry selectively; update the YAML to make each agent
uniquely addressable by either consolidating the two entries into a single
Backend Developer agent that contains both level variants, or add a distinct
identifier field (e.g., id, rank or include level in the inheritance key) so
(role, department, id) or (role, department, level) becomes unique; ensure any
references using extends are updated to the chosen unique key and that the YAML
keys mentioned (role, department, level, extends) reflect the new uniqueness.
In `@src/ai_company/templates/builtins/research_lab.yaml`:
- Around line 53-67: The two entries with role "Data Analyst" and department
"data_analytics" produce identical YAML merge keys and must be made unique so
descendants can override them individually; modify one of the Data Analyst
entries (e.g., the one with model "small" or "medium") to include a distinct
identifier used as the merge key—either add an explicit unique field such as
"id" or "seat_id" (e.g., "seat_id: data_analyst_small") or change the role to a
unique value (e.g., "Data Analyst (small)") so that the entries for role "Data
Analyst" in this template can be surgically overridden instead of colliding.
In `@src/ai_company/templates/builtins/startup.yaml`:
- Around line 5-6: The startup template currently sets min_agents: 4 and
max_agents: 7 which violates the agreed 3–5 agent contract; update the
template's startup bounds by changing min_agents to 3 and max_agents to 5 so the
startup configuration (min_agents and max_agents) enforces the required 3–5
agent constraint.
In `@src/ai_company/templates/loader.py`:
- Around line 404-412: The code masks falsy non-mapping values by using company
= data.get("company") or {}; change this to company = data.get("company", {}) so
the default empty dict is only used when the key is missing, and keep the
subsequent isinstance(company, dict) check (and the existing logging/TypeError)
to ensure values like "", 0, False, or [] are properly treated as malformed;
locate the assignment to company and the following isinstance check in loader.py
to make this change.
In `@src/ai_company/templates/merge.py`:
- Around line 107-116: _current implementation uses (role.lower(),
department.lower()) as the merge key which is unstable for duplicate-role
rosters; update _apply_child_agent to support a stable per-agent identity by
using an explicit merge id (e.g., agent.get("merge_id") or "slot") as part of
the index key: build parent index by (role.lower(), department.lower(),
merge_id) where merge_id defaults to None, and when matching a child entry
prefer exact merge_id matches (consume that specific parent) and fall back to
the first unmatched parent only if child has no merge_id; strip the merge_id
from replacement/appended dicts and apply the same change to the other merge
functions referenced (the blocks currently around the second and third
occurrences of agent merging) so all matching/consumption logic consistently
honors merge_id when present.
- Around line 90-94: The merged result is copying references from parent/child
into result (e.g., the loop assigning result[key] for "workflow_handoffs" and
"escalation_paths"), causing downstream mutations to leak into source templates;
fix by importing copy and types.MappingProxyType, and when assigning collection
values use result[key] = copy.deepcopy(child[key]) or copy.deepcopy(parent[key])
as appropriate, and ensure any final returned merged collections (agents,
departments, handoffs, workflow_handoffs, escalation_paths, etc.) are wrapped in
MappingProxyType for read-only enforcement; apply the same deep-copy +
MappingProxyType pattern to the other merge sites referenced (the other merge
blocks handling agents/departments/handoffs).
In `@src/ai_company/templates/renderer.py`:
- Around line 218-225: The child config being passed to merge_template_configs
is already materialized with synthesized defaults, causing it to clobber parent
scalar values; before calling merge_template_configs in the block that builds
parent_config (where _render_to_dict(parent_loaded, parent_vars, _chain=_chain |
{parent_name}) is used and merged with child_config), strip out scalar keys from
child_config that the child did not explicitly supply. Implement this by having
_render_to_dict (or the rendering call site) return or expose which keys were
explicitly set by the child (e.g., a provided_keys set) and then filter
child_config to remove scalar entries (e.g., company_name and other simple
scalars) unless they appear in that provided_keys set; then call
merge_template_configs with the filtered child_config so parent scalars are
preserved.
- Around line 151-230: The _resolve_inheritance function is too long and mixes
concerns; refactor it into small helpers: extract the circular check and depth
enforcement (using _MAX_INHERITANCE_DEPTH and raising
TemplateInheritanceError/TEMPLATE_INHERIT_CIRCULAR or
TEMPLATE_INHERIT_DEPTH_EXCEEDED) into a helper like
_validate_inheritance_chain(child_id, parent_name, _chain), extract template
loading into a helper like _load_parent_template(parent_name) that wraps
load_template and returns parent_loaded, and extract the parent
render/merge/logging into another helper like
_render_and_merge_parent(parent_loaded, child_config, vars_dict, _chain) which
calls _collect_parent_variables, _render_to_dict, merge_template_configs and
emits TEMPLATE_INHERIT_RESOLVE_START / TEMPLATE_INHERIT_RESOLVE_SUCCESS logs;
then have _resolve_inheritance orchestrate these helpers (call validation, load,
render/merge) so each helper is small and testable while preserving the same
exceptions and log messages.
In `@src/ai_company/templates/schema.py`:
- Around line 277-283: The validator _normalize_extends currently assumes the
incoming YAML value is a string and calls .strip(), which will raise an
AttributeError for non-string inputs; change _normalize_extends (the
`@field_validator` for "extends") to explicitly check the input type: if value is
None return None; if not isinstance(value, str) raise a TypeError or ValueError
with a clear message (so Pydantic converts it to a ValidationError); otherwise
return value.strip().lower().
In `@tests/unit/templates/test_renderer.py`:
- Around line 516-554: The tests duplicate agent-count bounds and a validity
check; replace the hardcoded _EXPECTED_AGENT_COUNTS and the redundant
test_all_templates_render_to_valid_root_config by sourcing min/max from each
template's metadata: in TestRosterCounts (functions
test_template_agent_count_in_range and test_full_company_variable_override) call
load_template(name) once, read bounds from loaded.template.metadata.min_agents
and .max_agents, assert len(config.agents) falls within those bounds, and remove
or fold the duplicate RootConfig validity assertion (you may still assert
isinstance(config, RootConfig) where you already load/render to ensure the
object type); keep using load_template, render_template, BUILTIN_TEMPLATES, and
the full_company override logic but compare against metadata-derived bounds
instead of _EXPECTED_AGENT_COUNTS.
---
Outside diff comments:
In `@src/ai_company/templates/loader.py`:
- Around line 427-439: The normalization currently drops workflow_handoffs and
escalation_paths so definitions never reach CompanyTemplate; update
_normalize_template_data (the function building result: dict[str, Any]) to
include "workflow_handoffs": data.get("workflow_handoffs", ()) and
"escalation_paths": data.get("escalation_paths", ()) in the result dict (same
style as "agents"/"departments") so pass-1 validation and inheritance see these
fields.
In `@src/ai_company/templates/renderer.py`:
- Around line 417-421: The current code masks invalid non-mapping `company`
values by using `rendered_data.get("company") or {}`; change the logic in the
renderer so you only default to an empty mapping when the `company` key is
absent or its value is explicitly null, otherwise preserve the original value
and then validate its type; specifically, replace the `company =
rendered_data.get("company") or {}` pattern with a check like "if 'company' not
in rendered_data or rendered_data['company'] is None: company = {} else: company
= rendered_data['company']" and keep the existing type check that logs
TEMPLATE_RENDER_YAML_ERROR and raises TemplateRenderError when `company` is
present but not a dict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc1df9f6-1fd1-4b8b-a40f-c97df078ed8e
📒 Files selected for processing (22)
DESIGN_SPEC.mdsrc/ai_company/observability/events/template.pysrc/ai_company/templates/__init__.pysrc/ai_company/templates/builtins/agency.yamlsrc/ai_company/templates/builtins/dev_shop.yamlsrc/ai_company/templates/builtins/full_company.yamlsrc/ai_company/templates/builtins/product_team.yamlsrc/ai_company/templates/builtins/research_lab.yamlsrc/ai_company/templates/builtins/solo_founder.yamlsrc/ai_company/templates/builtins/startup.yamlsrc/ai_company/templates/errors.pysrc/ai_company/templates/loader.pysrc/ai_company/templates/merge.pysrc/ai_company/templates/presets.pysrc/ai_company/templates/renderer.pysrc/ai_company/templates/schema.pytests/unit/templates/conftest.pytests/unit/templates/test_inheritance.pytests/unit/templates/test_loader.pytests/unit/templates/test_presets.pytests/unit/templates/test_renderer.pytests/unit/templates/test_schema.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use native lazy annotations (PEP 649) — do not usefrom __future__ import annotations
Use PEP 758 except syntax:except A, B:without parentheses for multiple exception types
Maintain line length of 88 characters (enforced by ruff)
Files:
tests/unit/templates/conftest.pytests/unit/templates/test_presets.pysrc/ai_company/observability/events/template.pysrc/ai_company/templates/__init__.pysrc/ai_company/templates/renderer.pytests/unit/templates/test_inheritance.pysrc/ai_company/templates/merge.pysrc/ai_company/templates/loader.pytests/unit/templates/test_schema.pysrc/ai_company/templates/presets.pytests/unit/templates/test_renderer.pysrc/ai_company/templates/schema.pytests/unit/templates/test_loader.pysrc/ai_company/templates/errors.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e, and@pytest.mark.slowto categorize tests
Prefer@pytest.mark.parametrizefor testing similar test cases
Use generic vendor-agnostic names in test fixtures and configuration:test-provider,test-small-001,test-medium-001,test-large-001
Files:
tests/unit/templates/conftest.pytests/unit/templates/test_presets.pytests/unit/templates/test_inheritance.pytests/unit/templates/test_schema.pytests/unit/templates/test_renderer.pytests/unit/templates/test_loader.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: All public functions and classes must have type hints and Google-style docstrings (enforced by ruff D rules)
Never useimport loggingorlogging.getLogger()in application code — usefrom ai_company.observability import get_loggerwith variable namelogger
Use structured logging with constants fromai_company.observability.events.<domain>— always log aslogger.info(EVENT_CONSTANT, key=value)never with format strings
For all identifier/name fields in Pydantic models, useNotBlankStrtype fromcore.typesinstead of manual whitespace validators (including optional and tuple variants)
Use@computed_fieldfor derived values in Pydantic models instead of storing and validating redundant fields
Enforce immutability: create new objects instead of mutating. For non-Pydantic internal collections usecopy.deepcopy()at construction andMappingProxyTypefor read-only enforcement
For config/identity data use frozen Pydantic models; for runtime state that evolves use separate mutable-via-copy models withmodel_copy(update=...)
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code instead of barecreate_task
Functions must be under 50 lines, files must be under 800 lines
Handle errors explicitly — never silently swallow exceptions
Never use real vendor names (Anthropic, OpenAI, Claude, GPT) in project-owned code, docstrings, comments, or tests — use generic names:example-provider,example-large-001,test-provider,test-small-001
Files:
src/ai_company/observability/events/template.pysrc/ai_company/templates/__init__.pysrc/ai_company/templates/renderer.pysrc/ai_company/templates/merge.pysrc/ai_company/templates/loader.pysrc/ai_company/templates/presets.pysrc/ai_company/templates/schema.pysrc/ai_company/templates/errors.py
DESIGN_SPEC.md
📄 CodeRabbit inference engine (CLAUDE.md)
Update
DESIGN_SPEC.mdafter approved deviations to reflect the new reality
Files:
DESIGN_SPEC.md
🧠 Learnings (4)
📚 Learning: 2026-03-07T17:28:05.391Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T17:28:05.391Z
Learning: Applies to tests/**/*.py : Use generic vendor-agnostic names in test fixtures and configuration: `test-provider`, `test-small-001`, `test-medium-001`, `test-large-001`
Applied to files:
tests/unit/templates/test_presets.py
📚 Learning: 2026-03-07T17:28:05.391Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T17:28:05.391Z
Learning: Applies to src/**/*.py : Use structured logging with constants from `ai_company.observability.events.<domain>` — always log as `logger.info(EVENT_CONSTANT, key=value)` never with format strings
Applied to files:
src/ai_company/observability/events/template.py
📚 Learning: 2026-03-07T17:28:05.391Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T17:28:05.391Z
Learning: Applies to DESIGN_SPEC.md : Update `DESIGN_SPEC.md` after approved deviations to reflect the new reality
Applied to files:
DESIGN_SPEC.md
📚 Learning: 2026-03-07T17:28:05.391Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T17:28:05.391Z
Learning: Applies to src/ai_company/{api,config,providers}/**/*.py : Validate at system boundaries (user input, external APIs, config files)
Applied to files:
src/ai_company/templates/schema.py
🧬 Code graph analysis (8)
tests/unit/templates/test_presets.py (2)
src/ai_company/core/agent.py (1)
PersonalityConfig(25-122)src/ai_company/templates/presets.py (1)
get_personality_preset(405-427)
src/ai_company/templates/__init__.py (1)
src/ai_company/templates/errors.py (2)
TemplateError(6-7)TemplateInheritanceError(23-28)
src/ai_company/templates/renderer.py (6)
src/ai_company/templates/errors.py (2)
TemplateInheritanceError(23-28)TemplateRenderError(14-20)src/ai_company/templates/merge.py (1)
merge_template_configs(35-97)src/ai_company/config/utils.py (1)
deep_merge(37-61)src/ai_company/config/defaults.py (1)
default_config_dict(6-32)src/ai_company/templates/loader.py (1)
LoadedTemplate(77-88)src/ai_company/templates/schema.py (1)
CompanyTemplate(206-328)
src/ai_company/templates/merge.py (3)
src/ai_company/config/utils.py (1)
deep_merge(37-61)src/ai_company/observability/_logger.py (1)
get_logger(8-28)src/ai_company/templates/errors.py (1)
TemplateInheritanceError(23-28)
src/ai_company/templates/loader.py (3)
src/ai_company/templates/errors.py (2)
TemplateRenderError(14-20)TemplateValidationError(31-67)src/ai_company/config/errors.py (1)
ConfigLocation(7-21)src/ai_company/tools/registry.py (1)
get(71-96)
tests/unit/templates/test_schema.py (1)
src/ai_company/templates/schema.py (2)
TemplateAgentConfig(77-132)CompanyTemplate(206-328)
tests/unit/templates/test_renderer.py (3)
src/ai_company/templates/loader.py (2)
load_template(158-214)load_template_file(217-238)src/ai_company/templates/renderer.py (1)
render_template(69-102)src/ai_company/config/schema.py (1)
RootConfig(361-504)
tests/unit/templates/test_loader.py (2)
tests/unit/templates/conftest.py (2)
tmp_template_file(211-219)TemplateFileFactory(12-15)src/ai_company/templates/loader.py (1)
load_template_file(217-238)
🪛 YAMLlint (1.38.0)
src/ai_company/templates/builtins/full_company.yaml
[warning] 128-128: comment not indented like content
(comments-indentation)
[error] 129-129: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (10)
src/ai_company/templates/builtins/solo_founder.yaml (1)
5-6: LGTM!The
min_agents: 2andmax_agents: 3constraints are consistent with the template's 2 defined agents (CEO and Full-Stack Developer). This aligns well with the "Solo Founder" minimal setup concept.src/ai_company/templates/builtins/full_company.yaml (4)
5-27: LGTM!The new
min_agents/max_agentsbounds and configurable developer count variables are well-designed. Themin_agents: 1provides flexibility for inheritance scenarios where child templates may remove agents, whilemax_agents: 50aligns with the PR objectives for full_company templates (20-50+ agents).
34-61: LGTM!Department budget allocations correctly sum to 100% (10+30+10+8+10+8+8+8+8=100). The new departments (design, data_analytics, security, creative_marketing) with appropriate head roles provide comprehensive enterprise coverage.
128-155: Static analysis hints are false positives (Jinja2 template context).The YAMLlint warnings about line 128 comment indentation and line 129
%character are false positives—this is a Jinja2 template file, not pure YAML. The{% for %}syntax is valid Jinja2 that renders to proper YAML.The Jinja2 loops are well-structured with appropriate
| default() | intfilters and sensible seniority progression (first dev is senior, subsequent devs are mid-level).Minor style note: The comment on line 128 uses 4-space indentation while the Jinja2 control block at line 129 has no indentation. Consider aligning the comment with the Jinja2 block for visual consistency, though this is purely cosmetic.
[approve_code_changes, suggest_optional_refactor]
156-215: LGTM!The expanded agent roster covers all departments with appropriate roles, levels, models, and personality presets. The structure properly reflects enterprise team composition with specialized roles across QA, data analytics, operations, security, and marketing.
tests/unit/templates/test_presets.py (1)
120-129: Nice drift check for auto-name coverage.This will fail as soon as
BUILTIN_ROLESgrows without a matching_AUTO_NAMESentry.src/ai_company/observability/events/template.py (1)
18-28: Good event-surface expansion.Centralizing the new inheritance and load-failure event names here keeps the renderer/loader logging string-free and consistent.
src/ai_company/templates/presets.py (1)
431-437: Current syntax is correct—no change needed.The parentheses in
except (ValidationError, TypeError) as _exc:are required. PEP 758's parenthesis-free syntax only applies when there is noasclause; withas, parentheses must be used per PEP 758. The code is already compliant with the style guide.src/ai_company/templates/loader.py (1)
356-358: The code at line 356 is already compliant with PEP 758. When catching multiple exception types and binding to an alias usingas exc, parentheses are required by PEP 758. The current syntaxexcept (ValidationError, ValueError, TypeError) as exc:is correct and does not need modification.> Likely an incorrect or invalid review comment.tests/unit/templates/test_inheritance.py (1)
41-512: Nice coverage of the new inheritance surface.This exercises merge semantics, parent-variable propagation, cycle/depth failures, and end-to-end rendering paths, which should keep the new renderer flow from regressing.
src/ai_company/templates/renderer.py
Outdated
| parent_config = _render_to_dict( | ||
| parent_loaded, | ||
| parent_vars, | ||
| _chain=_chain | {parent_name}, | ||
| ) | ||
|
|
||
| merged = merge_template_configs(parent_config, child_config) | ||
| logger.info( |
There was a problem hiding this comment.
Let the parent keep its scalar defaults when the child didn't set them.
At this merge point, child_config has already been materialized as a full standalone config, so the parent-fallback branch in merge_template_configs() becomes unreachable for render-time inheritance. In practice, a child template that only adds agents will still override inherited scalars like company_name with its own synthesized defaults. Please omit those scalar keys unless the child explicitly provided them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ai_company/templates/renderer.py` around lines 218 - 225, The child
config being passed to merge_template_configs is already materialized with
synthesized defaults, causing it to clobber parent scalar values; before calling
merge_template_configs in the block that builds parent_config (where
_render_to_dict(parent_loaded, parent_vars, _chain=_chain | {parent_name}) is
used and merged with child_config), strip out scalar keys from child_config that
the child did not explicitly supply. Implement this by having _render_to_dict
(or the rendering call site) return or expose which keys were explicitly set by
the child (e.g., a provided_keys set) and then filter child_config to remove
scalar entries (e.g., company_name and other simple scalars) unless they appear
in that provided_keys set; then call merge_template_configs with the filtered
child_config so parent scalars are preserved.
…type safety, and function split - Add merge_id field to TemplateAgentConfig for stable per-agent identity during inheritance merge (prevents extra="forbid" rejection) - Add merge_id to all 6 YAML templates with duplicate-role agents - Update merge key from (role, department) to (role, department, merge_id) - Enforce deep-copy at all merge boundaries to prevent reference sharing - Fix or-falsy anti-pattern: explicit `if value is None` checks - Fix _normalize_extends validator: guard non-string inputs - Narrow TemplateVariable.default from Any to str|int|float|bool|None - Split _resolve_inheritance into 3 focused functions (<50 lines each) - Wrap BUILTIN_TEMPLATES in MappingProxyType for read-only enforcement - Fix startup.yaml min_agents: 4 → 3 to match actual roster - Update DESIGN_SPEC.md §14.1: Title-Case roles, merge_id, 3-tuple key - Replace hardcoded agent count dict with dynamic min/max bounds in tests - Fix logger.error → logger.exception in loader.py exception handler - Fix ruff lint: Union→pipe syntax, assert noqa, line length Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ai_company/templates/renderer.py (1)
426-478: 🧹 Nitpick | 🔵 Trivial
_build_config_dictslightly exceeds the 50-line limit.This function is 53 lines. Consider extracting the
workflow_handoffs/escalation_pathshandling (lines 474-476) into a small helper to stay within the guideline.♻️ Suggested extraction
+def _extract_optional_lists( + rendered_data: dict[str, Any], + result: dict[str, Any], +) -> None: + """Extract optional list fields from rendered data into result.""" + for key in ("workflow_handoffs", "escalation_paths"): + if key in rendered_data and rendered_data[key] is not None: + result[key] = _validate_list(rendered_data, key) + + def _build_config_dict( rendered_data: dict[str, Any], template: CompanyTemplate, variables: dict[str, Any], ) -> dict[str, Any]: # ... existing code until line 472 ... - for key in ("workflow_handoffs", "escalation_paths"): - if key in rendered_data and rendered_data[key] is not None: - result[key] = _validate_list(rendered_data, key) + _extract_optional_lists(rendered_data, result) return resultAs per coding guidelines,
Functions must be under 50 lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ai_company/templates/renderer.py` around lines 426 - 478, The _build_config_dict function is over the 50-line limit; extract the post-processing that adds "workflow_handoffs" and "escalation_paths" into a small helper to shorten the function. Create a helper (e.g., _attach_optional_list_keys or _add_optional_lists) that accepts the result dict and rendered_data, iterates over the tuple ("workflow_handoffs", "escalation_paths"), checks key presence and non-None, calls _validate_list(rendered_data, key) and assigns into result[key]; then replace the final for-loop in _build_config_dict with a single call to that helper. Ensure the helper uses the same _validate_list symbol and preserves behavior and return types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DESIGN_SPEC.md`:
- Around line 2218-2222: Update the "agents" merge-key description to explicitly
state that when merge_id is omitted the implementation uses an empty string as
the default (i.e., the key becomes the tuple (role, department, "") at runtime),
and mention this matches the current behavior of the merge logic for `agents`
and `merge_id`; adjust the sentence that currently reads "`(role, department,
"")` when `merge_id` is omitted" to clearly say this is the implementation
default used by the `agents` merging code.
In `@src/ai_company/templates/builtins/startup.yaml`:
- Around line 5-6: The startup template's max_agents value violates the 3–5
agent contract; update the startup template's max_agents from 7 to 5 so the
min_agents/min and max_agents constraints (min_agents: 3, max_agents: 5) enforce
the required 3–5 range in the startup template configuration (look for
min_agents and max_agents entries in the startup template YAML).
In `@src/ai_company/templates/presets.py`:
- Around line 434-437: Replace the current except clause that uses parentheses
with the PEP 758 style: change "except (ValidationError, TypeError) as _exc:" to
use the comma form "except ValidationError, TypeError as _exc:" in the block
that constructs msg and raises ValueError (the block referencing _preset_name
and _preset_dict) so the except handles both exception types using the requested
syntax and then re-raises ValueError from _exc; leave the rest of the logic (msg
creation and raise ValueError(msg) from _exc) unchanged.
In `@tests/unit/templates/test_renderer.py`:
- Around line 530-538: The assertion in test_full_company_variable_override is
brittle because it assumes a +2 delta; update the test to assert a clear,
deterministic expectation instead of an unexplained delta: load_template and
render_template as before, compute the previous and new counts of backend
developer agents by inspecting default_config.agents and override_config.agents
(e.g., count by role or name for "backend_dev"), and assert that the backend dev
count increased from the default to the expected value (or assert the explicit
total agent count equals a documented expected number). Also add a short comment
in test_full_company_variable_override explaining why the expected increase is
what it is so future template changes are easier to reason about.
---
Outside diff comments:
In `@src/ai_company/templates/renderer.py`:
- Around line 426-478: The _build_config_dict function is over the 50-line
limit; extract the post-processing that adds "workflow_handoffs" and
"escalation_paths" into a small helper to shorten the function. Create a helper
(e.g., _attach_optional_list_keys or _add_optional_lists) that accepts the
result dict and rendered_data, iterates over the tuple ("workflow_handoffs",
"escalation_paths"), checks key presence and non-None, calls
_validate_list(rendered_data, key) and assigns into result[key]; then replace
the final for-loop in _build_config_dict with a single call to that helper.
Ensure the helper uses the same _validate_list symbol and preserves behavior and
return types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0543d695-79da-4192-84f0-d067d544f9ee
📒 Files selected for processing (13)
DESIGN_SPEC.mdsrc/ai_company/templates/builtins/agency.yamlsrc/ai_company/templates/builtins/dev_shop.yamlsrc/ai_company/templates/builtins/full_company.yamlsrc/ai_company/templates/builtins/product_team.yamlsrc/ai_company/templates/builtins/research_lab.yamlsrc/ai_company/templates/builtins/startup.yamlsrc/ai_company/templates/loader.pysrc/ai_company/templates/merge.pysrc/ai_company/templates/presets.pysrc/ai_company/templates/renderer.pysrc/ai_company/templates/schema.pytests/unit/templates/test_renderer.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use native lazy annotations (PEP 649) — do not usefrom __future__ import annotations
Use PEP 758 except syntax:except A, B:without parentheses for multiple exception types
Maintain line length of 88 characters (enforced by ruff)
Files:
tests/unit/templates/test_renderer.pysrc/ai_company/templates/renderer.pysrc/ai_company/templates/merge.pysrc/ai_company/templates/presets.pysrc/ai_company/templates/loader.pysrc/ai_company/templates/schema.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest markers@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.e2e, and@pytest.mark.slowto categorize tests
Prefer@pytest.mark.parametrizefor testing similar test cases
Use generic vendor-agnostic names in test fixtures and configuration:test-provider,test-small-001,test-medium-001,test-large-001
Files:
tests/unit/templates/test_renderer.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: All public functions and classes must have type hints and Google-style docstrings (enforced by ruff D rules)
Never useimport loggingorlogging.getLogger()in application code — usefrom ai_company.observability import get_loggerwith variable namelogger
Use structured logging with constants fromai_company.observability.events.<domain>— always log aslogger.info(EVENT_CONSTANT, key=value)never with format strings
For all identifier/name fields in Pydantic models, useNotBlankStrtype fromcore.typesinstead of manual whitespace validators (including optional and tuple variants)
Use@computed_fieldfor derived values in Pydantic models instead of storing and validating redundant fields
Enforce immutability: create new objects instead of mutating. For non-Pydantic internal collections usecopy.deepcopy()at construction andMappingProxyTypefor read-only enforcement
For config/identity data use frozen Pydantic models; for runtime state that evolves use separate mutable-via-copy models withmodel_copy(update=...)
Preferasyncio.TaskGroupfor fan-out/fan-in parallel operations in new code instead of barecreate_task
Functions must be under 50 lines, files must be under 800 lines
Handle errors explicitly — never silently swallow exceptions
Never use real vendor names (Anthropic, OpenAI, Claude, GPT) in project-owned code, docstrings, comments, or tests — use generic names:example-provider,example-large-001,test-provider,test-small-001
Files:
src/ai_company/templates/renderer.pysrc/ai_company/templates/merge.pysrc/ai_company/templates/presets.pysrc/ai_company/templates/loader.pysrc/ai_company/templates/schema.py
DESIGN_SPEC.md
📄 CodeRabbit inference engine (CLAUDE.md)
Update
DESIGN_SPEC.mdafter approved deviations to reflect the new reality
Files:
DESIGN_SPEC.md
🧠 Learnings (7)
📚 Learning: 2026-03-07T17:28:05.391Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T17:28:05.391Z
Learning: Applies to tests/**/*.py : Use generic vendor-agnostic names in test fixtures and configuration: `test-provider`, `test-small-001`, `test-medium-001`, `test-large-001`
Applied to files:
tests/unit/templates/test_renderer.py
📚 Learning: 2026-03-07T17:28:05.391Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T17:28:05.391Z
Learning: Applies to src/**/*.py : Functions must be under 50 lines, files must be under 800 lines
Applied to files:
src/ai_company/templates/renderer.py
📚 Learning: 2026-03-07T17:28:05.391Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T17:28:05.391Z
Learning: Applies to src/ai_company/{providers,tools,engine,communication}/**/*.py : Use `copy.deepcopy()` at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, persistence serialization) to prevent unintended mutations
Applied to files:
src/ai_company/templates/merge.pysrc/ai_company/templates/builtins/product_team.yaml
📚 Learning: 2026-03-07T17:28:05.391Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T17:28:05.391Z
Learning: Applies to src/**/*.py : Enforce immutability: create new objects instead of mutating. For non-Pydantic internal collections use `copy.deepcopy()` at construction and `MappingProxyType` for read-only enforcement
Applied to files:
src/ai_company/templates/merge.py
📚 Learning: 2026-03-07T17:28:05.391Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T17:28:05.391Z
Learning: Applies to DESIGN_SPEC.md : Update `DESIGN_SPEC.md` after approved deviations to reflect the new reality
Applied to files:
DESIGN_SPEC.md
📚 Learning: 2026-03-07T17:28:05.391Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T17:28:05.391Z
Learning: Applies to src/**/*.py : Use structured logging with constants from `ai_company.observability.events.<domain>` — always log as `logger.info(EVENT_CONSTANT, key=value)` never with format strings
Applied to files:
DESIGN_SPEC.md
📚 Learning: 2026-03-07T17:28:05.391Z
Learnt from: CR
Repo: Aureliolo/ai-company PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-07T17:28:05.391Z
Learning: Applies to src/ai_company/{api,config,providers}/**/*.py : Validate at system boundaries (user input, external APIs, config files)
Applied to files:
src/ai_company/templates/schema.py
🧬 Code graph analysis (3)
src/ai_company/templates/renderer.py (6)
src/ai_company/templates/errors.py (2)
TemplateInheritanceError(23-28)TemplateRenderError(14-20)src/ai_company/templates/merge.py (1)
merge_template_configs(36-98)src/ai_company/config/utils.py (1)
deep_merge(37-61)src/ai_company/config/defaults.py (1)
default_config_dict(6-32)src/ai_company/templates/loader.py (1)
LoadedTemplate(79-90)src/ai_company/templates/schema.py (1)
CompanyTemplate(218-344)
src/ai_company/templates/merge.py (3)
src/ai_company/config/utils.py (1)
deep_merge(37-61)src/ai_company/observability/_logger.py (1)
get_logger(8-28)src/ai_company/templates/errors.py (1)
TemplateInheritanceError(23-28)
src/ai_company/templates/loader.py (1)
src/ai_company/templates/errors.py (3)
TemplateNotFoundError(10-11)TemplateRenderError(14-20)TemplateValidationError(31-67)
🪛 YAMLlint (1.38.0)
src/ai_company/templates/builtins/full_company.yaml
[warning] 128-128: comment not indented like content
(comments-indentation)
[error] 129-129: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (28)
src/ai_company/templates/presets.py (1)
263-342: Well-structured preset expansion with proper immutability.The new personality presets (
user_advocate,process_optimizer,growth_hacker,technical_communicator,systems_thinker) are validated at import time and properly frozen via nestedMappingProxyType. The expanded_AUTO_NAMESpool provides good role coverage for auto-name generation.src/ai_company/templates/builtins/dev_shop.yaml (1)
37-80: Well-structured roster with proper merge_id disambiguation.The template correctly uses
merge_idto distinguish multiple agents with the same role (three Backend Developers with distinct identities). Agent count (8) is within the declared range [6, 10], and department budgets sum to 100%.src/ai_company/templates/builtins/research_lab.yaml (1)
53-69: Past review concern addressed with distinct merge_ids.The two Data Analyst entries now have unique merge identifiers (
analyst-primaryandanalyst-secondary), resolving the previous concern about duplicate merge keys that would prevent surgical overrides in child templates.tests/unit/templates/test_renderer.py (1)
516-528: Good implementation using metadata bounds instead of hardcoded values.The test now reads bounds from
loaded.template.metadata.min_agentsandmax_agents, keeping the test in sync with template definitions and addressing the previous review concern about duplicate data.src/ai_company/templates/builtins/startup.yaml (1)
49-64: Proper merge_id usage for duplicate Full-Stack Developer roles.The two Full-Stack Developer entries have distinct merge identifiers (
fullstack-senior,fullstack-mid), enabling child templates to target specific agents during inheritance.src/ai_company/templates/merge.py (4)
36-98: Well-designed merge semantics with proper immutability.The
merge_template_configsfunction implements clear merge strategies: scalars (child wins), config (deep-merged), agents (by role/department/merge_id key), departments (by name). Deep-copy is correctly applied at merge boundaries (lines 93, 95) to prevent reference sharing, addressing the previous review concern.
101-144: Agent merge now uses stable three-part key.The
(role, department, merge_id)key addresses the previous review concern about unstable merge keys when rosters contain duplicate roles. The_ParentEntrydataclass properly tracks match state, and agents are deep-copied at index time (line 136).
146-181: Proper _remove directive handling with clear error reporting.The
_apply_child_agentfunction correctly handles the_removedirective by marking matched parent entries for removal, and raisesTemplateInheritanceErrorwith structured logging when no matching parent is found.
208-263: Department merge with deep-copy enforcement.The
_merge_departmentsfunction properly deep-copies all department entries (lines 236, 248, 254, 261), preventing reference sharing across inheritance levels. Warning logs for nameless departments provide useful debugging context.src/ai_company/templates/loader.py (2)
415-425: Past review concern addressed: proper handling of falsy company values.The code now explicitly checks
if company is Nonebefore defaulting to{}, ensuring that invalid falsy values like"",0, or[]are properly caught by theisinstance(company, dict)check and raise aTypeError.
48-58: Proper immutability for built-in templates registry.
BUILTIN_TEMPLATESis correctly wrapped inMappingProxyTypeto prevent accidental mutation of the template registry.src/ai_company/templates/schema.py (3)
291-299: Past review concern addressed: proper type guard in extends normalizer.The
_normalize_extendsvalidator now explicitly checksisinstance(value, str)before calling.strip(), preventingAttributeErroron non-string inputs. Non-string values are passed through for Pydantic's type validation to reject.
125-133: Proper merge directive fields with alias support.The
merge_idfield provides stable identity for inheritance merging, andremovewithalias="_remove"enables the removal directive in YAML while maintaining Pythonic attribute naming.
301-324: Correct skip of agent count validation during inheritance.Skipping
_validate_agent_count_in_rangewhenextendsis set is correct because child templates may define zero agents (inheriting all from parent). The docstring clearly explains the rationale.src/ai_company/templates/renderer.py (5)
69-102: LGTM — Clean separation of concerns in render flow.The refactored
render_templatenow delegates inheritance resolution to_render_to_dictand keeps the top-level function focused on merging with defaults and validation. This is a good improvement.
151-199: Refactor addressed — function is now within the 50-line limit.The
_resolve_inheritancefunction has been split into smaller helpers (_validate_inheritance_chain,_render_and_merge_parent) as suggested in past reviews, bringing it to 49 lines.
235-254: Verify that circular import is acceptable here.The
load_templateimport inside_render_and_merge_parentis a deferred import to break a circular dependency. This is a valid pattern, but ensure it doesn't introduce import-time performance issues in hot paths.
596-600: Good addition — preserving_removedirective for inheritance.The
_removemerge directive is correctly preserved through the agent expansion, enabling child templates to remove parent agents.
604-649: Clean extraction of personality resolution logic.The
_resolve_agent_personalityhelper properly handles both inline personality configs and preset lookups with appropriate error handling and logging.src/ai_company/templates/builtins/full_company.yaml (2)
128-142: Static analysis false positive — Jinja2 syntax is valid.YAMLlint reports a syntax error on line 129 because it doesn't recognize Jinja2 template syntax (
{% for ... %}). This is expected and not a real issue — the template will be processed by Jinja2 before YAML parsing.The
merge_id: "backend-{{ i }}"pattern correctly ensures each dynamically-generated agent is uniquely addressable for inheritance, addressing the collision concerns raised in past reviews of other templates.
5-6: Well-structured enterprise roster with proper agent differentiation.The template now includes:
- Dynamic developer generation with
merge_idfor unique addressability- Comprehensive department coverage (9 departments, budgets sum to 100%)
- Clear role differentiation with appropriate personality presets
Also applies to: 63-217
src/ai_company/templates/builtins/product_team.yaml (2)
59-70: Collision issue resolved — agents now have unique merge keys.The two Backend Developer entries now have distinct
merge_idvalues (backend-seniorandbackend-mid), making them uniquely addressable for inheritance. This addresses the concern raised in past reviews.
26-41: Department structure is well-balanced.Budget percentages sum to 100% (25+35+15+15+10), and each department has an appropriate head_role assigned.
src/ai_company/templates/builtins/agency.yaml (2)
79-84: Collision issue resolved — Full-Stack Developers now uniquely addressable.The two Full-Stack Developer entries now have distinct
merge_idvalues (fullstack-senioron line 80,fullstack-midon line 101), enabling child templates to selectively override or remove individual agents.Also applies to: 100-105
26-41: Budget and department structure correct.Budget percentages sum to 100% (20+35+20+10+15), with appropriate head_role assignments. The addition of
quality_assuranceandcreative_marketingdepartments rounds out the agency structure.DESIGN_SPEC.md (3)
2203-2225: Excellent documentation of template inheritance semantics.The inheritance documentation clearly explains:
- Scalar merge behavior (child wins)
- Config deep-merge
- Agent merge by
(role, department, merge_id)key with_removesupport- Department merge by name
- Circular inheritance detection
This aligns well with the implementation in
merge.pyandrenderer.py.
2573-2574: Engineering conventions table updated appropriately.The new conventions for template inheritance and Pydantic alias usage are well-documented with clear rationale. The
_ParentEntrydataclass andDEFAULT_MERGE_DEPARTMENTconstant are properly referenced.
2514-2514: Project structure updated with merge.py.The addition of
merge.pyto the templates directory in the project structure is correctly documented.
src/ai_company/templates/presets.py
Outdated
| except (ValidationError, TypeError) as _exc: | ||
| msg = f"Invalid personality preset {_preset_name!r}: {_exc}" | ||
| raise ValueError(msg) from _exc | ||
| # Clean up loop variables only if the loop body executed (non-empty dict). | ||
| if PERSONALITY_PRESETS: | ||
| del _preset_name, _preset_dict | ||
| del _preset_name, _preset_dict |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use PEP 758 except syntax without parentheses.
As per coding guidelines, use except ValidationError, TypeError: instead of parentheses.
♻️ Suggested fix
- except (ValidationError, TypeError) as _exc:
+ except ValidationError, TypeError as _exc:As per coding guidelines: "Use PEP 758 except syntax: except A, B: without parentheses for multiple exception types"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except (ValidationError, TypeError) as _exc: | |
| msg = f"Invalid personality preset {_preset_name!r}: {_exc}" | |
| raise ValueError(msg) from _exc | |
| # Clean up loop variables only if the loop body executed (non-empty dict). | |
| if PERSONALITY_PRESETS: | |
| del _preset_name, _preset_dict | |
| del _preset_name, _preset_dict | |
| except ValidationError, TypeError as _exc: | |
| msg = f"Invalid personality preset {_preset_name!r}: {_exc}" | |
| raise ValueError(msg) from _exc | |
| del _preset_name, _preset_dict |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ai_company/templates/presets.py` around lines 434 - 437, Replace the
current except clause that uses parentheses with the PEP 758 style: change
"except (ValidationError, TypeError) as _exc:" to use the comma form "except
ValidationError, TypeError as _exc:" in the block that constructs msg and raises
ValueError (the block referencing _preset_name and _preset_dict) so the except
handles both exception types using the requested syntax and then re-raises
ValueError from _exc; leave the rest of the logic (msg creation and raise
ValueError(msg) from _exc) unchanged.
…n extraction - Reject _remove directive in standalone templates (no extends) with error - Add TEMPLATE_LOAD_INVALID_NAME event for path-traversal rejections - Strip _remove from surviving parent agents in merge output - Extract _attach_optional_lists helper from _build_config_dict (<50 lines) - Restore empty-dict guard on del _preset_name at module level - Tighten startup.yaml max_agents: 7 → 5 per issue #81 contract - Clarify DESIGN_SPEC.md merge_id defaults-to-empty-string wording - Make test_full_company_variable_override assert on role counts not delta Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | **State coordination** | Planned (M4) | Centralized single-writer: `TaskEngine` owns all task/project mutations via `asyncio.Queue`. Agents submit requests, engine applies `model_copy(update=...)` sequentially and publishes snapshots. `version: int` field on state models for future optimistic concurrency if multi-process scaling is needed. | Prevents lost updates by design. Trivial in single-threaded asyncio (no locks). Perfect audit trail. Industry consensus: MetaGPT, CrewAI, AutoGen all use prevention-by-design, not conflict resolution. See §6.8 State Coordination table. | | ||
| | **Workspace isolation** | Planned (M4) | Pluggable `WorkspaceIsolationStrategy` protocol. Default: planner + git worktrees. Each agent works in an isolated worktree; sequential merge on completion. Textual conflicts detected by git; semantic conflicts reviewed by agent or human. | Industry standard (Codex, Cursor, Claude Code, VS Code). Maximum parallelism. Leverages mature git infrastructure. See §6.8. | | ||
| | **Graceful shutdown** | Adopted (M3) | Pluggable `ShutdownStrategy` protocol. Default: cooperative with 30s timeout. Agents check shutdown event at turn boundaries. Force-cancel after timeout. `INTERRUPTED` status for force-cancelled tasks. M4/M5: upgrade to checkpoint-and-stop. | Cross-platform (Windows `signal.signal()` fallback). Bounded shutdown time. Mirrors cooperative shutdown in §6.7. | | ||
| | **Template inheritance** | Adopted (M2.5) | `extends` field on `CompanyTemplate` triggers parent resolution at render time. `merge.py` merges configs by field type: scalars (child wins), config dicts (deep merge), agents (by `(role, department)` key with `_remove` support), departments (by name). `_ParentEntry` dataclass tracks merge state. `DEFAULT_MERGE_DEPARTMENT = "engineering"` shared between merge and renderer. Circular chains detected via `frozenset` tracking; max depth = 10. | Enables template composition without copy-paste. Merge-by-key preserves parent order. `_remove` directive enables clean agent removal without workarounds. | |
There was a problem hiding this comment.
The conventions table entry for Template inheritance describes the agent merge key as (role, department) but the actual code and the detailed documentation in §14.1 (line 2221) correctly describe it as (role, department, merge_id). This row in the conventions table should be updated to mention merge_id for consistency with the actual merge key.
| | **Template inheritance** | Adopted (M2.5) | `extends` field on `CompanyTemplate` triggers parent resolution at render time. `merge.py` merges configs by field type: scalars (child wins), config dicts (deep merge), agents (by `(role, department)` key with `_remove` support), departments (by name). `_ParentEntry` dataclass tracks merge state. `DEFAULT_MERGE_DEPARTMENT = "engineering"` shared between merge and renderer. Circular chains detected via `frozenset` tracking; max depth = 10. | Enables template composition without copy-paste. Merge-by-key preserves parent order. `_remove` directive enables clean agent removal without workarounds. | | |
| | **Template inheritance** | Adopted (M2.5) | `extends` field on `CompanyTemplate` triggers parent resolution at render time. `merge.py` merges configs by field type: scalars (child wins), config dicts (deep merge), agents (by `(role, department, merge_id)` key with `_remove` support), departments (by name). `_ParentEntry` dataclass tracks merge state. `DEFAULT_MERGE_DEPARTMENT = "engineering"` shared between merge and renderer. Circular chains detected via `frozenset` tracking; max depth = 10. | Enables template composition without copy-paste. Merge-by-key preserves parent order. `_remove` directive enables clean agent removal without workarounds. | |
| agent_dict: dict[str, Any] = { | ||
| "name": name, | ||
| "role": role, | ||
| "department": agent.get("department", _DEFAULT_DEPARTMENT), | ||
| "level": agent.get("level", "mid"), | ||
| } | ||
|
|
||
| _resolve_agent_personality(agent, name, agent_dict) | ||
|
|
||
| model_tier = agent.get("model", "medium") | ||
| agent_dict["model"] = {"provider": _DEFAULT_PROVIDER, "model_id": model_tier} | ||
|
|
||
| # Preserve _remove merge directive for inheritance. | ||
| if agent.get("_remove"): | ||
| if not has_extends: | ||
| msg = ( | ||
| f"Agent {name!r} uses '_remove' but the template " | ||
| "has no 'extends' — directive has no effect" | ||
| ) | ||
| logger.warning( | ||
| TEMPLATE_RENDER_VARIABLE_ERROR, | ||
| agent=name, | ||
| field="_remove", | ||
| ) | ||
| raise TemplateRenderError(msg) | ||
| agent_dict["_remove"] = True | ||
|
|
||
| return agent_dict |
There was a problem hiding this comment.
_expand_single_agent does not propagate merge_id from the raw agent dict into the expanded agent_dict. Since _build_config_dict calls _expand_agents before the merge happens, the merge_id key is lost. When _merge_agents later computes _agent_key, it always sees merge_id="" for all agents.
This makes the merge_id field in YAML templates (e.g., startup.yaml line 50/56, dev_shop.yaml line 44/50) ineffective for inheritance merges. Two Full-Stack Developers with different merge_id values will both get key ("full-stack developer", "engineering", ""), defeating the purpose of merge_id as a disambiguation tool for child template overrides.
The fix is to include merge_id in the returned agent_dict when it is present in the input agent, e.g., adding if agent.get("merge_id"): agent_dict["merge_id"] = agent["merge_id"] after building the base dict.
| clean = copy.deepcopy( | ||
| {k: v for k, v in child_agent.items() if k not in ("_remove", "merge_id")} | ||
| ) | ||
|
|
||
| if is_remove: | ||
| if matched_entry is None: | ||
| msg = f"Cannot remove agent with key {key}: no matching parent agent found" | ||
| logger.error( | ||
| TEMPLATE_INHERIT_MERGE_ERROR, | ||
| action="remove_failed", | ||
| key=key, | ||
| ) | ||
| raise TemplateInheritanceError(msg) | ||
| matched_entry.matched = True | ||
| matched_entry.agent = None # mark for removal |
There was a problem hiding this comment.
When is_remove is True and a matching parent is found, the clean dict (line 161-163) is computed via copy.deepcopy but never used — the code immediately sets matched_entry.agent = None on line 175. Consider moving the clean computation after the is_remove check to avoid the unnecessary deep copy in the remove path.
Address 13 findings from post-merge bot reviews on PRs #157-#162: Communication layer (PR #157): - Wake blocked receive() callers on unsubscribe via None sentinel - Distinguish shutdown vs timeout in receive() logging - Add AgentMessenger.receive() facade method - Validate MessageHandler.handle() is async at registration - Await cancelled tasks in _await_with_shutdown to prevent warnings Observability (PR #158): - Add log-before-raise to all validators missing it (company.py, schema.py) — 14 raise sites across 11 validators Parallel execution (PR #161): - Log suppressed ExceptionGroup instead of silent pass - Add PARALLEL_AGENT_CANCELLED structured event for cancellations - Fix progress.in_progress semantics (increment after semaphore) - Use PARALLEL_LOCK_RELEASE_ERROR for lock release failures - Remove duplicate plan_parsing.py from DESIGN_SPEC file tree Template inheritance (PR #162): - Update DESIGN_SPEC merge key docs to include merge_id - Preserve merge_id in _expand_single_agent (confirmed bug fix) - Defer deepcopy in _apply_child_agent past _remove early-return Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
extendsother templates, with merge semantics for agents (by role+department key), departments (by name), config (deep merge), and scalars (child wins). Circular detection via frozenset chain tracking, max depth 10.merge.py): New dedicated module for template config merging with_ParentEntrydataclass for type-safe agent tracking,_removedirective support, and department merge by name.extendsfield with normalization validator,_removealias onTemplateAgentConfig, agent count validation skip when extending._resolve_agent_personalityhelper, narrowed exception catches, removed dead test fixtures, tightened test assertions to exact counts.merge.pyto §15.3 project structure, added inheritance and alias conventions to §15.5._removealias,_collect_parent_variables, escalation_paths fallback, None child scalar merge.Closes #80, closes #81, closes #84
Test plan
uv run ruff check src/ tests/— all checks passeduv run ruff format src/ tests/— all formatteduv run mypy src/ tests/— no issues (318 files)uv run pytest tests/ -n auto --cov=ai_company --cov-fail-under=80— 3002 passed, 96.49% coverageReview coverage
Pre-reviewed by 9 agents (code-reviewer, python-reviewer, pr-test-analyzer, silent-failure-hunter, comment-analyzer, type-design-analyzer, logging-audit, resilience-audit, docs-consistency). 26 findings identified, all 26 implemented. Polish pass applied by code-simplifier agent.
🤖 Generated with Claude Code