From b6680678684e1711d789eda83f8dae2e4d8b8ffb Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Fri, 6 Mar 2026 10:20:26 +0100 Subject: [PATCH 1/3] refactor: adopt NotBlankStr across all models (#108) Replace str + Field(min_length=1) + manual @model_validator whitespace checks with NotBlankStr type annotation from core.types across 15 source files. Migrate 80+ fields (scalar and tuple), delete 25+ redundant validators, rename validate_non_blank_unique_strings to validate_unique_strings. Net reduction of ~295 lines. Co-Authored-By: Claude Opus 4.6 --- DESIGN_SPEC.md | 4 +- src/ai_company/budget/config.py | 10 +- src/ai_company/budget/cost_record.py | 19 +--- src/ai_company/budget/hierarchy.py | 23 +--- src/ai_company/budget/spending_summary.py | 22 +--- src/ai_company/communication/channel.py | 6 +- src/ai_company/communication/config.py | 10 +- src/ai_company/core/__init__.py | 4 +- src/ai_company/core/agent.py | 78 +++---------- src/ai_company/core/artifact.py | 35 ++---- src/ai_company/core/company.py | 74 +++---------- src/ai_company/core/project.py | 30 ++--- src/ai_company/core/role.py | 105 +++--------------- src/ai_company/core/task.py | 50 +++------ src/ai_company/core/types.py | 11 +- src/ai_company/observability/config.py | 8 +- tests/unit/budget/test_config.py | 4 +- tests/unit/communication/test_channel.py | 8 +- tests/unit/communication/test_config.py | 12 +- tests/unit/core/test_agent.py | 12 +- tests/unit/core/test_artifact.py | 14 +-- tests/unit/core/test_company.py | 12 +- tests/unit/core/test_project.py | 10 +- tests/unit/core/test_role.py | 10 +- tests/unit/core/test_task.py | 28 ++--- tests/unit/core/test_types.py | 129 ++++++++++++++++++++++ tests/unit/observability/test_config.py | 2 +- 27 files changed, 282 insertions(+), 448 deletions(-) create mode 100644 tests/unit/core/test_types.py diff --git a/DESIGN_SPEC.md b/DESIGN_SPEC.md index c7ece84bd0..d1d16f04e8 100644 --- a/DESIGN_SPEC.md +++ b/DESIGN_SPEC.md @@ -106,7 +106,7 @@ Every agent has a comprehensive identity. At the design level, agent data splits - **Config (immutable)**: identity, personality, skills, model preferences, tool permissions, authority. Defined at hire time, changed only by explicit reconfiguration. Represented as frozen Pydantic models. - **Runtime state (mutable-via-copy)**: current status, active task, conversation history, execution metrics. Evolves during agent operation. Represented as Pydantic models using `model_copy(update=...)` for state transitions — never mutated in place. -> **Current state (M2):** Only the config layer exists as `AgentIdentity` (frozen Pydantic model in `core/agent.py`). The runtime state layer will be introduced in M3 when the agent execution engine is implemented. Non-optional identifier fields currently use `str` with `Field(min_length=1)` + a manual `@model_validator`; migration to `NotBlankStr` (from `core.types`) is planned. +> **Current state (M2):** Only the config layer exists as `AgentIdentity` (frozen Pydantic model in `core/agent.py`). The runtime state layer will be introduced in M3 when the agent execution engine is implemented. All identifier/name fields use `NotBlankStr` (from `core.types`) for automatic whitespace rejection; tuple fields use `tuple[NotBlankStr, ...]` for per-element validation. ```yaml # --- Current (M2): Config layer — AgentIdentity (frozen) --- @@ -1439,7 +1439,7 @@ These conventions were established during the M0–M2 review cycle. **Adopted** | **Immutability strategy** | Adopted | `copy.deepcopy()` at construction + `MappingProxyType` wrapping for non-Pydantic internal collections (registries, `BaseTool`). For Pydantic frozen models: `frozen=True` prevents field reassignment; `copy.deepcopy()` at system boundaries (tool execution, LLM provider serialization) prevents nested mutation. No MappingProxyType inside Pydantic models (serialization friction). | Deep-copy at construction fully isolates nested structures; `MappingProxyType` enforces read-only access. Boundary-copy for Pydantic models is simple, centralized, and Pydantic-native. A future CPython built-in immutable mapping type (e.g. `frozendict`) would provide zero-friction field-level immutability when available. | | **Config vs runtime split** | Adopted (M3) | Frozen models for config/identity; `model_copy(update=...)` for runtime state transitions | `TaskExecution` and `AgentContext` (in `engine/`) are frozen Pydantic models that use `model_copy(update=...)` for copy-on-write state transitions without re-running validators (per Pydantic `model_copy` semantics). Config layer (`AgentIdentity`, `Task`) remains unchanged. | | **Derived fields** | Adopted | `@computed_field` instead of stored + validated | Eliminates redundant storage and impossible-to-fail validators. `TokenUsage.total_tokens` migrated from stored `Field` + `@model_validator` to `@computed_field` property. | -| **String validation** | Planned | `NotBlankStr` type from `core.types` for all identifiers | Eliminates per-model `@model_validator` boilerplate for whitespace checks. `NotBlankStr` is defined but models still use `Field(min_length=1)` + manual validators. | +| **String validation** | Adopted | `NotBlankStr` type from `core.types` for all identifiers | Eliminates per-model `@model_validator` boilerplate for whitespace checks. All identifier/name fields use `NotBlankStr`; tuple fields use `tuple[NotBlankStr, ...]` for per-element validation. | | **Shared field groups** | Planned | Extract common field sets into base models (e.g. `_SpendingTotals`) | Prevents field duplication across spending summary models. Not yet implemented — each model independently defines fields. | | **Event constants** | Adopted (flat) | Single `events.py` module with domain-scoped naming (e.g. `PROVIDER_CALL_START`, `BUDGET_RECORD_ADDED`) | Current approach uses a single module. Splitting into per-domain submodules may be revisited when the file exceeds ~200 constants. | | **Parallel tool execution** | Planned | `asyncio.TaskGroup` in `ToolInvoker.invoke_all` | Structured concurrency with proper cancellation semantics. Currently sequential; migration planned for M3 when the agent engine needs concurrent tool calls. | diff --git a/src/ai_company/budget/config.py b/src/ai_company/budget/config.py index 86f3a9e4bd..22e67920a0 100644 --- a/src/ai_company/budget/config.py +++ b/src/ai_company/budget/config.py @@ -9,6 +9,8 @@ from pydantic import BaseModel, ConfigDict, Field, model_validator +from ai_company.core.types import NotBlankStr # noqa: TC001 + class BudgetAlertConfig(BaseModel): """Alert threshold configuration for budget monitoring. @@ -87,7 +89,7 @@ class AutoDowngradeConfig(BaseModel): strict=True, description="Budget percent triggering downgrade", ) - downgrade_map: tuple[tuple[str, str], ...] = Field( + downgrade_map: tuple[tuple[NotBlankStr, NotBlankStr], ...] = Field( default=(), description="Ordered pairs of (from_alias, to_alias)", ) @@ -110,12 +112,6 @@ def _validate_downgrade_map(self) -> Self: """Validate downgrade_map for correctness.""" sources: list[str] = [] for source, target in self.downgrade_map: - if not source: - msg = "Empty or whitespace-only source alias in downgrade_map" - raise ValueError(msg) - if not target: - msg = "Empty or whitespace-only target alias in downgrade_map" - raise ValueError(msg) if source == target: msg = f"Self-downgrade in downgrade_map: {source!r} -> {target!r}" raise ValueError(msg) diff --git a/src/ai_company/budget/cost_record.py b/src/ai_company/budget/cost_record.py index d531b71ca6..70fa1cc46f 100644 --- a/src/ai_company/budget/cost_record.py +++ b/src/ai_company/budget/cost_record.py @@ -8,6 +8,8 @@ from pydantic import AwareDatetime, BaseModel, ConfigDict, Field, model_validator +from ai_company.core.types import NotBlankStr # noqa: TC001 + class CostRecord(BaseModel): """Immutable record of a single API call's cost. @@ -29,24 +31,15 @@ class CostRecord(BaseModel): model_config = ConfigDict(frozen=True) - agent_id: str = Field(min_length=1, description="Agent identifier") - task_id: str = Field(min_length=1, description="Task identifier") - provider: str = Field(min_length=1, description="LLM provider name") - model: str = Field(min_length=1, description="Model identifier") + agent_id: NotBlankStr = Field(description="Agent identifier") + task_id: NotBlankStr = Field(description="Task identifier") + provider: NotBlankStr = Field(description="LLM provider name") + model: NotBlankStr = Field(description="Model identifier") input_tokens: int = Field(ge=0, description="Input token count") output_tokens: int = Field(ge=0, description="Output token count") cost_usd: float = Field(ge=0.0, description="Cost in USD") timestamp: AwareDatetime = Field(description="Timestamp of the API call") - @model_validator(mode="after") - def _validate_no_blank_strings(self) -> Self: - """Ensure string identifier fields are not whitespace-only.""" - for field_name in ("agent_id", "task_id", "provider", "model"): - if not getattr(self, field_name).strip(): - msg = f"{field_name} must not be whitespace-only" - raise ValueError(msg) - return self - @model_validator(mode="after") def _validate_token_consistency(self) -> Self: """Ensure positive cost implies at least one non-zero token count.""" diff --git a/src/ai_company/budget/hierarchy.py b/src/ai_company/budget/hierarchy.py index 3cc3f8edaa..03f7bd419c 100644 --- a/src/ai_company/budget/hierarchy.py +++ b/src/ai_company/budget/hierarchy.py @@ -11,6 +11,7 @@ from pydantic import BaseModel, ConfigDict, Field, model_validator from ai_company.constants import BUDGET_ROUNDING_PRECISION +from ai_company.core.types import NotBlankStr # noqa: TC001 class TeamBudget(BaseModel): @@ -23,8 +24,7 @@ class TeamBudget(BaseModel): model_config = ConfigDict(frozen=True) - team_name: str = Field( - min_length=1, + team_name: NotBlankStr = Field( description="Team name", ) budget_percent: float = Field( @@ -34,14 +34,6 @@ class TeamBudget(BaseModel): description="Percent of department budget", ) - @model_validator(mode="after") - def _validate_team_name_not_blank(self) -> Self: - """Ensure team_name is not whitespace-only.""" - if not self.team_name.strip(): - msg = "team_name must not be whitespace-only" - raise ValueError(msg) - return self - class DepartmentBudget(BaseModel): """Budget allocation for a department with nested team allocations. @@ -57,8 +49,7 @@ class DepartmentBudget(BaseModel): model_config = ConfigDict(frozen=True) - department_name: str = Field( - min_length=1, + department_name: NotBlankStr = Field( description="Department name", ) budget_percent: float = Field( @@ -72,14 +63,6 @@ class DepartmentBudget(BaseModel): description="Team budget allocations", ) - @model_validator(mode="after") - def _validate_department_name_not_blank(self) -> Self: - """Ensure department_name is not whitespace-only.""" - if not self.department_name.strip(): - msg = "department_name must not be whitespace-only" - raise ValueError(msg) - return self - @model_validator(mode="after") def _validate_unique_team_names(self) -> Self: """Ensure no duplicate team names within the department.""" diff --git a/src/ai_company/budget/spending_summary.py b/src/ai_company/budget/spending_summary.py index e22120b611..89be6786bf 100644 --- a/src/ai_company/budget/spending_summary.py +++ b/src/ai_company/budget/spending_summary.py @@ -13,6 +13,7 @@ from pydantic import BaseModel, ConfigDict, Field, model_validator from ai_company.budget.enums import BudgetAlertLevel +from ai_company.core.types import NotBlankStr # noqa: TC001 class PeriodSpending(BaseModel): @@ -77,7 +78,7 @@ class AgentSpending(BaseModel): model_config = ConfigDict(frozen=True) - agent_id: str = Field(min_length=1, description="Agent identifier") + agent_id: NotBlankStr = Field(description="Agent identifier") total_cost_usd: float = Field( default=0.0, ge=0.0, @@ -99,14 +100,6 @@ class AgentSpending(BaseModel): description="Number of cost records", ) - @model_validator(mode="after") - def _validate_agent_id_not_blank(self) -> Self: - """Ensure agent_id is not whitespace-only.""" - if not self.agent_id.strip(): - msg = "agent_id must not be whitespace-only" - raise ValueError(msg) - return self - class DepartmentSpending(BaseModel): """Spending aggregation for a department. @@ -121,8 +114,7 @@ class DepartmentSpending(BaseModel): model_config = ConfigDict(frozen=True) - department_name: str = Field( - min_length=1, + department_name: NotBlankStr = Field( description="Department name", ) total_cost_usd: float = Field( @@ -146,14 +138,6 @@ class DepartmentSpending(BaseModel): description="Number of cost records", ) - @model_validator(mode="after") - def _validate_department_name_not_blank(self) -> Self: - """Ensure department_name is not whitespace-only.""" - if not self.department_name.strip(): - msg = "department_name must not be whitespace-only" - raise ValueError(msg) - return self - class SpendingSummary(BaseModel): """Top-level spending summary combining all aggregation dimensions. diff --git a/src/ai_company/communication/channel.py b/src/ai_company/communication/channel.py index d5ae0dcc0a..af8541ff4a 100644 --- a/src/ai_company/communication/channel.py +++ b/src/ai_company/communication/channel.py @@ -7,7 +7,7 @@ from ai_company.communication.enums import ChannelType from ai_company.core.types import ( NotBlankStr, - validate_non_blank_unique_strings, + validate_unique_strings, ) @@ -27,7 +27,7 @@ class Channel(BaseModel): default=ChannelType.TOPIC, description="Channel delivery semantics", ) - subscribers: tuple[str, ...] = Field( + subscribers: tuple[NotBlankStr, ...] = Field( default=(), description="Agent IDs subscribed to this channel", ) @@ -35,5 +35,5 @@ class Channel(BaseModel): @model_validator(mode="after") def _validate_subscribers(self) -> Self: """Ensure subscriber entries are non-blank and unique.""" - validate_non_blank_unique_strings(self.subscribers, "subscribers") + validate_unique_strings(self.subscribers, "subscribers") return self diff --git a/src/ai_company/communication/config.py b/src/ai_company/communication/config.py index 05c9885bb0..5df876fe92 100644 --- a/src/ai_company/communication/config.py +++ b/src/ai_company/communication/config.py @@ -11,7 +11,7 @@ ) from ai_company.core.types import ( NotBlankStr, - validate_non_blank_unique_strings, + validate_unique_strings, ) # Default channels from DESIGN_SPEC Section 5.4. @@ -42,7 +42,7 @@ class MessageBusConfig(BaseModel): default=MessageBusBackend.INTERNAL, description="Transport backend", ) - channels: tuple[str, ...] = Field( + channels: tuple[NotBlankStr, ...] = Field( default=_DEFAULT_CHANNELS, description="Pre-defined channel names", ) @@ -50,7 +50,7 @@ class MessageBusConfig(BaseModel): @model_validator(mode="after") def _validate_channels(self) -> Self: """Ensure channel names are non-blank and unique.""" - validate_non_blank_unique_strings(self.channels, "channels") + validate_unique_strings(self.channels, "channels") return self @@ -79,7 +79,7 @@ class MeetingTypeConfig(BaseModel): default=None, description="Event trigger", ) - participants: tuple[str, ...] = Field( + participants: tuple[NotBlankStr, ...] = Field( default=(), description="Participant role or agent identifiers", ) @@ -103,7 +103,7 @@ def _validate_frequency_or_trigger(self) -> Self: @model_validator(mode="after") def _validate_participants(self) -> Self: """Ensure participant entries are non-blank and unique.""" - validate_non_blank_unique_strings(self.participants, "participants") + validate_unique_strings(self.participants, "participants") return self diff --git a/src/ai_company/core/__init__.py b/src/ai_company/core/__init__.py index 18bd4c56f8..f8bf49150d 100644 --- a/src/ai_company/core/__init__.py +++ b/src/ai_company/core/__init__.py @@ -50,7 +50,7 @@ ) from ai_company.core.task import AcceptanceCriterion, Task from ai_company.core.task_transitions import VALID_TRANSITIONS, validate_transition -from ai_company.core.types import NotBlankStr, validate_non_blank_unique_strings +from ai_company.core.types import NotBlankStr, validate_unique_strings __all__ = [ "BUILTIN_ROLES", @@ -96,6 +96,6 @@ "ToolPermissions", "get_builtin_role", "get_seniority_info", - "validate_non_blank_unique_strings", "validate_transition", + "validate_unique_strings", ] diff --git a/src/ai_company/core/agent.py b/src/ai_company/core/agent.py index 1e27697179..2b40ac066f 100644 --- a/src/ai_company/core/agent.py +++ b/src/ai_company/core/agent.py @@ -14,6 +14,7 @@ SeniorityLevel, ) from ai_company.core.role import Authority +from ai_company.core.types import NotBlankStr # noqa: TC001 class PersonalityConfig(BaseModel): @@ -29,13 +30,12 @@ class PersonalityConfig(BaseModel): model_config = ConfigDict(frozen=True) - traits: tuple[str, ...] = Field( + traits: tuple[NotBlankStr, ...] = Field( default=(), description="Personality traits", ) - communication_style: str = Field( + communication_style: NotBlankStr = Field( default="neutral", - min_length=1, description="Communication style description", ) risk_tolerance: RiskTolerance = Field( @@ -51,18 +51,6 @@ class PersonalityConfig(BaseModel): description="Extended personality description", ) - @model_validator(mode="after") - def _validate_no_empty_traits(self) -> Self: - """Ensure no empty or whitespace-only traits or communication_style.""" - if not self.communication_style.strip(): - msg = "communication_style must not be whitespace-only" - raise ValueError(msg) - for trait in self.traits: - if not trait.strip(): - msg = "Empty or whitespace-only entry in traits" - raise ValueError(msg) - return self - class SkillSet(BaseModel): """Primary and secondary skills for an agent. @@ -74,25 +62,15 @@ class SkillSet(BaseModel): model_config = ConfigDict(frozen=True) - primary: tuple[str, ...] = Field( + primary: tuple[NotBlankStr, ...] = Field( default=(), description="Primary skills", ) - secondary: tuple[str, ...] = Field( + secondary: tuple[NotBlankStr, ...] = Field( default=(), description="Secondary skills", ) - @model_validator(mode="after") - def _validate_no_empty_skills(self) -> Self: - """Ensure no empty or whitespace-only skill names.""" - for field_name in ("primary", "secondary"): - for skill in getattr(self, field_name): - if not skill.strip(): - msg = f"Empty or whitespace-only skill name in {field_name}" - raise ValueError(msg) - return self - class ModelConfig(BaseModel): """LLM model configuration for an agent. @@ -107,8 +85,8 @@ class ModelConfig(BaseModel): model_config = ConfigDict(frozen=True) - provider: str = Field(min_length=1, description="LLM provider name") - model_id: str = Field(min_length=1, description="Model identifier") + provider: NotBlankStr = Field(description="LLM provider name") + model_id: NotBlankStr = Field(description="Model identifier") temperature: float = Field( default=0.7, ge=0.0, @@ -120,22 +98,11 @@ class ModelConfig(BaseModel): gt=0, description="Maximum output tokens", ) - fallback_model: str | None = Field( + fallback_model: NotBlankStr | None = Field( default=None, - min_length=1, description="Fallback model identifier", ) - @model_validator(mode="after") - def _validate_non_blank_identifiers(self) -> Self: - """Ensure identifier fields are not whitespace-only.""" - for field_name in ("provider", "model_id", "fallback_model"): - value = getattr(self, field_name) - if value is not None and not value.strip(): - msg = f"{field_name} must not be whitespace-only" - raise ValueError(msg) - return self - class MemoryConfig(BaseModel): """Memory configuration for an agent. @@ -176,25 +143,15 @@ class ToolPermissions(BaseModel): model_config = ConfigDict(frozen=True) - allowed: tuple[str, ...] = Field( + allowed: tuple[NotBlankStr, ...] = Field( default=(), description="Explicitly allowed tools", ) - denied: tuple[str, ...] = Field( + denied: tuple[NotBlankStr, ...] = Field( default=(), description="Explicitly denied tools", ) - @model_validator(mode="after") - def _validate_no_empty_tools(self) -> Self: - """Ensure no empty or whitespace-only tool names.""" - for field_name in ("allowed", "denied"): - for tool in getattr(self, field_name): - if not tool.strip(): - msg = f"Empty or whitespace-only tool name in {field_name}" - raise ValueError(msg) - return self - @model_validator(mode="after") def _validate_no_overlap(self) -> Self: """Ensure no tool appears in both allowed and denied lists. @@ -236,9 +193,9 @@ class AgentIdentity(BaseModel): model_config = ConfigDict(frozen=True) id: UUID = Field(default_factory=uuid4, description="Unique agent identifier") - name: str = Field(min_length=1, description="Agent display name") - role: str = Field(min_length=1, description="Role name") - department: str = Field(min_length=1, description="Department name") + name: NotBlankStr = Field(description="Agent display name") + role: NotBlankStr = Field(description="Role name") + department: NotBlankStr = Field(description="Department name") level: SeniorityLevel = Field( default=SeniorityLevel.MID, description="Seniority level", @@ -269,12 +226,3 @@ class AgentIdentity(BaseModel): default=AgentStatus.ACTIVE, description="Current lifecycle status", ) - - @model_validator(mode="after") - def _validate_non_blank_identifiers(self) -> Self: - """Ensure name, role, and department are not whitespace-only.""" - for field_name in ("name", "role", "department"): - if not getattr(self, field_name).strip(): - msg = f"{field_name} must not be whitespace-only" - raise ValueError(msg) - return self diff --git a/src/ai_company/core/artifact.py b/src/ai_company/core/artifact.py index 498ed90a6a..3b44f58bbf 100644 --- a/src/ai_company/core/artifact.py +++ b/src/ai_company/core/artifact.py @@ -1,13 +1,13 @@ """Artifact domain models for task outputs and expected deliverables.""" from datetime import datetime # noqa: TC003 — required at runtime by Pydantic -from typing import Self -from pydantic import BaseModel, ConfigDict, Field, model_validator +from pydantic import BaseModel, ConfigDict, Field from ai_company.core.enums import ( ArtifactType, # noqa: TC001 — required at runtime by Pydantic ) +from ai_company.core.types import NotBlankStr # noqa: TC001 class ExpectedArtifact(BaseModel): @@ -23,19 +23,10 @@ class ExpectedArtifact(BaseModel): model_config = ConfigDict(frozen=True) type: ArtifactType = Field(description="Type of artifact expected") - path: str = Field( - min_length=1, + path: NotBlankStr = Field( description="File or directory path for the artifact", ) - @model_validator(mode="after") - def _validate_path_not_blank(self) -> Self: - """Ensure path is not whitespace-only.""" - if not self.path.strip(): - msg = "path must not be whitespace-only" - raise ValueError(msg) - return self - class Artifact(BaseModel): """A concrete artifact produced by an agent during task execution. @@ -55,18 +46,15 @@ class Artifact(BaseModel): model_config = ConfigDict(frozen=True) - id: str = Field(min_length=1, description="Unique artifact identifier") + id: NotBlankStr = Field(description="Unique artifact identifier") type: ArtifactType = Field(description="Artifact type") - path: str = Field( - min_length=1, + path: NotBlankStr = Field( description="File or directory path of the artifact", ) - task_id: str = Field( - min_length=1, + task_id: NotBlankStr = Field( description="ID of the task that produced this artifact", ) - created_by: str = Field( - min_length=1, + created_by: NotBlankStr = Field( description="Agent ID of the creator", ) description: str = Field( @@ -77,12 +65,3 @@ class Artifact(BaseModel): default=None, description="Timestamp when the artifact was created", ) - - @model_validator(mode="after") - def _validate_non_blank_strings(self) -> Self: - """Ensure string identifier fields are not whitespace-only.""" - for field_name in ("id", "path", "task_id", "created_by"): - if not getattr(self, field_name).strip(): - msg = f"{field_name} must not be whitespace-only" - raise ValueError(msg) - return self diff --git a/src/ai_company/core/company.py b/src/ai_company/core/company.py index 6e0707f946..9c55a262ee 100644 --- a/src/ai_company/core/company.py +++ b/src/ai_company/core/company.py @@ -8,6 +8,7 @@ from ai_company.constants import BUDGET_ROUNDING_PRECISION from ai_company.core.enums import CompanyType +from ai_company.core.types import NotBlankStr # noqa: TC001 class Team(BaseModel): @@ -24,24 +25,16 @@ class Team(BaseModel): model_config = ConfigDict(frozen=True) - name: str = Field(min_length=1, description="Team name") - lead: str = Field(min_length=1, description="Team lead agent name") - members: tuple[str, ...] = Field( + name: NotBlankStr = Field(description="Team name") + lead: NotBlankStr = Field(description="Team lead agent name") + members: tuple[NotBlankStr, ...] = Field( default=(), description="Team member agent names", ) @model_validator(mode="after") - def _validate_strings(self) -> Self: - """Ensure no empty or whitespace-only names in identifiers and members.""" - for field_name in ("name", "lead"): - if not getattr(self, field_name).strip(): - msg = f"{field_name} must not be whitespace-only" - raise ValueError(msg) - for member in self.members: - if not member.strip(): - msg = "Empty or whitespace-only entry in members" - raise ValueError(msg) + def _validate_no_duplicate_members(self) -> Self: + """Ensure no duplicate members.""" if len(self.members) != len(set(self.members)): dupes = sorted(m for m, c in Counter(self.members).items() if c > 1) msg = f"Duplicate members in team {self.name!r}: {dupes}" @@ -65,8 +58,8 @@ class Department(BaseModel): model_config = ConfigDict(frozen=True) - name: str = Field(min_length=1, description="Department name") - head: str = Field(min_length=1, description="Department head agent name") + name: NotBlankStr = Field(description="Department name") + head: NotBlankStr = Field(description="Department head agent name") budget_percent: float = Field( default=0.0, ge=0.0, @@ -78,15 +71,6 @@ class Department(BaseModel): description="Teams within this department", ) - @model_validator(mode="after") - def _validate_non_blank_identifiers(self) -> Self: - """Ensure name and head are not whitespace-only.""" - for field_name in ("name", "head"): - if not getattr(self, field_name).strip(): - msg = f"{field_name} must not be whitespace-only" - raise ValueError(msg) - return self - @model_validator(mode="after") def _validate_unique_team_names(self) -> Self: """Ensure no duplicate team names within a department.""" @@ -121,28 +105,15 @@ class CompanyConfig(BaseModel): ge=0.0, description="Monthly budget in USD", ) - communication_pattern: str = Field( + communication_pattern: NotBlankStr = Field( default="hybrid", - min_length=1, description="Default communication pattern", ) - tool_access_default: tuple[str, ...] = Field( + tool_access_default: tuple[NotBlankStr, ...] = Field( default=(), description="Default tool access for all agents", ) - @model_validator(mode="after") - def _validate_strings(self) -> Self: - """Ensure no whitespace-only identifiers or tool access entries.""" - if not self.communication_pattern.strip(): - msg = "communication_pattern must not be whitespace-only" - raise ValueError(msg) - for tool in self.tool_access_default: - if not tool.strip(): - msg = "Empty or whitespace-only entry in tool_access_default" - raise ValueError(msg) - return self - class HRRegistry(BaseModel): """Human resources registry for the company. @@ -158,27 +129,22 @@ class HRRegistry(BaseModel): model_config = ConfigDict(frozen=True) - active_agents: tuple[str, ...] = Field( + active_agents: tuple[NotBlankStr, ...] = Field( default=(), description="Currently active agent names", ) - available_roles: tuple[str, ...] = Field( + available_roles: tuple[NotBlankStr, ...] = Field( default=(), description="Roles available for hiring", ) - hiring_queue: tuple[str, ...] = Field( + hiring_queue: tuple[NotBlankStr, ...] = Field( default=(), description="Roles in the hiring pipeline", ) @model_validator(mode="after") - def _validate_entries(self) -> Self: - """Ensure no empty strings and no duplicate entries in active_agents.""" - for field_name in ("active_agents", "available_roles", "hiring_queue"): - for value in getattr(self, field_name): - if not value.strip(): - msg = f"Empty or whitespace-only entry in {field_name}" - raise ValueError(msg) + def _validate_no_duplicate_active_agents(self) -> Self: + """Ensure no duplicate entries in active_agents.""" agents = self.active_agents if len(agents) != len(set(agents)): dupes = sorted(a for a, c in Counter(agents).items() if c > 1) @@ -206,7 +172,7 @@ class Company(BaseModel): model_config = ConfigDict(frozen=True) id: UUID = Field(default_factory=uuid4, description="Company identifier") - name: str = Field(min_length=1, description="Company name") + name: NotBlankStr = Field(description="Company name") type: CompanyType = Field( default=CompanyType.CUSTOM, description="Company template type", @@ -224,14 +190,6 @@ class Company(BaseModel): description="HR registry", ) - @model_validator(mode="after") - def _validate_non_blank_name(self) -> Self: - """Ensure company name is not whitespace-only.""" - if not self.name.strip(): - msg = "name must not be whitespace-only" - raise ValueError(msg) - return self - @model_validator(mode="after") def _validate_departments(self) -> Self: """Validate department names are unique and budgets do not exceed 100%.""" diff --git a/src/ai_company/core/project.py b/src/ai_company/core/project.py index f511eeee19..401c69bfe8 100644 --- a/src/ai_company/core/project.py +++ b/src/ai_company/core/project.py @@ -7,6 +7,7 @@ from pydantic import BaseModel, ConfigDict, Field, model_validator from ai_company.core.enums import ProjectStatus +from ai_company.core.types import NotBlankStr # noqa: TC001 class Project(BaseModel): @@ -30,22 +31,21 @@ class Project(BaseModel): model_config = ConfigDict(frozen=True) - id: str = Field(min_length=1, description="Unique project identifier") - name: str = Field(min_length=1, description="Project display name") + id: NotBlankStr = Field(description="Unique project identifier") + name: NotBlankStr = Field(description="Project display name") description: str = Field( default="", description="Detailed project description", ) - team: tuple[str, ...] = Field( + team: tuple[NotBlankStr, ...] = Field( default=(), description="Agent IDs assigned to this project", ) - lead: str | None = Field( + lead: NotBlankStr | None = Field( default=None, - min_length=1, description="Agent ID of the project lead", ) - task_ids: tuple[str, ...] = Field( + task_ids: tuple[NotBlankStr, ...] = Field( default=(), description="IDs of tasks belonging to this project", ) @@ -64,15 +64,8 @@ class Project(BaseModel): ) @model_validator(mode="after") - def _validate_fields(self) -> Self: - """Validate string fields and deadline format.""" - for field_name in ("id", "name"): - if not getattr(self, field_name).strip(): - msg = f"{field_name} must not be whitespace-only" - raise ValueError(msg) - if self.lead is not None and not self.lead.strip(): - msg = "lead must not be whitespace-only" - raise ValueError(msg) + def _validate_deadline_format(self) -> Self: + """Validate deadline format if present.""" if self.deadline is not None: if not self.deadline.strip(): msg = "deadline must not be whitespace-only" @@ -86,12 +79,7 @@ def _validate_fields(self) -> Self: @model_validator(mode="after") def _validate_collections(self) -> Self: - """Validate collection entries and uniqueness.""" - for field_name in ("team", "task_ids"): - for value in getattr(self, field_name): - if not value.strip(): - msg = f"Empty or whitespace-only entry in {field_name}" - raise ValueError(msg) + """Validate collection uniqueness.""" if len(self.team) != len(set(self.team)): dupes = sorted(m for m, c in Counter(self.team).items() if c > 1) msg = f"Duplicate entries in team: {dupes}" diff --git a/src/ai_company/core/role.py b/src/ai_company/core/role.py index c6314c0f6f..a4f4e4acce 100644 --- a/src/ai_company/core/role.py +++ b/src/ai_company/core/role.py @@ -1,8 +1,6 @@ """Role and skill domain models.""" -from typing import Self - -from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator from ai_company.core.enums import ( DepartmentName, @@ -10,6 +8,7 @@ SeniorityLevel, SkillCategory, ) +from ai_company.core.types import NotBlankStr # noqa: TC001 class Skill(BaseModel): @@ -23,21 +22,13 @@ class Skill(BaseModel): model_config = ConfigDict(frozen=True) - name: str = Field(min_length=1, description="Skill name") + name: NotBlankStr = Field(description="Skill name") category: SkillCategory = Field(description="Skill category") proficiency: ProficiencyLevel = Field( default=ProficiencyLevel.INTERMEDIATE, description="Proficiency level", ) - @model_validator(mode="after") - def _validate_name_not_blank(self) -> Self: - """Ensure skill name is not whitespace-only.""" - if not self.name.strip(): - msg = "Skill name must not be whitespace-only" - raise ValueError(msg) - return self - class Authority(BaseModel): """Authority scope for an agent or role. @@ -51,16 +42,15 @@ class Authority(BaseModel): model_config = ConfigDict(frozen=True) - can_approve: tuple[str, ...] = Field( + can_approve: tuple[NotBlankStr, ...] = Field( default=(), description="Task types this role can approve", ) - reports_to: str | None = Field( + reports_to: NotBlankStr | None = Field( default=None, - min_length=1, description="Role this position reports to", ) - can_delegate_to: tuple[str, ...] = Field( + can_delegate_to: tuple[NotBlankStr, ...] = Field( default=(), description="Roles this position can delegate tasks to", ) @@ -70,19 +60,6 @@ class Authority(BaseModel): description="Maximum USD per task", ) - @model_validator(mode="after") - def _validate_no_empty_strings(self) -> Self: - """Ensure no whitespace-only entries in string tuples or reports_to.""" - for field_name in ("can_approve", "can_delegate_to"): - for value in getattr(self, field_name): - if not value.strip(): - msg = f"Empty or whitespace-only entry in {field_name}" - raise ValueError(msg) - if self.reports_to is not None and not self.reports_to.strip(): - msg = "reports_to must not be whitespace-only" - raise ValueError(msg) - return self - class SeniorityInfo(BaseModel): """Mapping from seniority level to authority and model configuration. @@ -97,28 +74,16 @@ class SeniorityInfo(BaseModel): model_config = ConfigDict(frozen=True) level: SeniorityLevel = Field(description="Seniority level") - authority_scope: str = Field( - min_length=1, + authority_scope: NotBlankStr = Field( description="Description of authority at this level", ) - typical_model_tier: str = Field( - min_length=1, + typical_model_tier: NotBlankStr = Field( description="Recommended model tier", ) - cost_tier: str = Field( - min_length=1, + cost_tier: NotBlankStr = Field( description="Cost tier identifier (built-in or user-defined)", ) - @model_validator(mode="after") - def _validate_non_blank_strings(self) -> Self: - """Ensure string fields are not whitespace-only.""" - for field_name in ("authority_scope", "typical_model_tier", "cost_tier"): - if not getattr(self, field_name).strip(): - msg = f"{field_name} must not be whitespace-only" - raise ValueError(msg) - return self - class Role(BaseModel): """A job definition within the organization. @@ -135,11 +100,11 @@ class Role(BaseModel): model_config = ConfigDict(frozen=True) - name: str = Field(min_length=1, description="Role name") + name: NotBlankStr = Field(description="Role name") department: DepartmentName = Field( description="Department this role belongs to", ) - required_skills: tuple[str, ...] = Field( + required_skills: tuple[NotBlankStr, ...] = Field( default=(), description="Skills required for this role", ) @@ -147,11 +112,11 @@ class Role(BaseModel): default=SeniorityLevel.MID, description="Default seniority level for this role", ) - tool_access: tuple[str, ...] = Field( + tool_access: tuple[NotBlankStr, ...] = Field( default=(), description="Tools available to this role", ) - system_prompt_template: str | None = Field( + system_prompt_template: NotBlankStr | None = Field( default=None, description="Template file for system prompt", ) @@ -160,25 +125,6 @@ class Role(BaseModel): description="Human-readable description", ) - @model_validator(mode="after") - def _validate_no_empty_strings(self) -> Self: - """Ensure no empty or whitespace-only entries in name and string tuples.""" - if not self.name.strip(): - msg = "name must not be whitespace-only" - raise ValueError(msg) - if ( - self.system_prompt_template is not None - and not self.system_prompt_template.strip() - ): - msg = "system_prompt_template must not be whitespace-only" - raise ValueError(msg) - for field_name in ("required_skills", "tool_access"): - for value in getattr(self, field_name): - if not value.strip(): - msg = f"Empty or whitespace-only entry in {field_name}" - raise ValueError(msg) - return self - class CustomRole(BaseModel): """User-defined custom role via configuration. @@ -198,15 +144,15 @@ class CustomRole(BaseModel): model_config = ConfigDict(frozen=True) - name: str = Field(min_length=1, description="Custom role name") + name: NotBlankStr = Field(description="Custom role name") department: DepartmentName | str = Field( description="Department (standard or custom name)", ) - required_skills: tuple[str, ...] = Field( + required_skills: tuple[NotBlankStr, ...] = Field( default=(), description="Required skills for this role", ) - system_prompt_template: str | None = Field( + system_prompt_template: NotBlankStr | None = Field( default=None, description="Template file for system prompt", ) @@ -214,7 +160,7 @@ class CustomRole(BaseModel): default=SeniorityLevel.MID, description="Default seniority level", ) - suggested_model: str | None = Field( + suggested_model: NotBlankStr | None = Field( default=None, description="Suggested model tier", ) @@ -230,20 +176,3 @@ def _department_not_empty(cls, v: DepartmentName | str) -> DepartmentName | str: msg = "Department name must not be empty" raise ValueError(msg) return stripped - - @model_validator(mode="after") - def _validate_no_empty_required_skills(self) -> Self: - """Ensure no whitespace-only name, optional fields, or skills.""" - if not self.name.strip(): - msg = "name must not be whitespace-only" - raise ValueError(msg) - for field_name in ("system_prompt_template", "suggested_model"): - value = getattr(self, field_name) - if value is not None and not value.strip(): - msg = f"{field_name} must not be whitespace-only" - raise ValueError(msg) - for value in self.required_skills: - if not value.strip(): - msg = "Empty or whitespace-only entry in required_skills" - raise ValueError(msg) - return self diff --git a/src/ai_company/core/task.py b/src/ai_company/core/task.py index d71079bd85..a219eeb098 100644 --- a/src/ai_company/core/task.py +++ b/src/ai_company/core/task.py @@ -9,6 +9,7 @@ from ai_company.core.artifact import ExpectedArtifact # noqa: TC001 from ai_company.core.enums import Complexity, Priority, TaskStatus, TaskType from ai_company.core.task_transitions import validate_transition +from ai_company.core.types import NotBlankStr # noqa: TC001 from ai_company.observability import get_logger from ai_company.observability.events import TASK_STATUS_CHANGED @@ -25,8 +26,7 @@ class AcceptanceCriterion(BaseModel): model_config = ConfigDict(frozen=True) - description: str = Field( - min_length=1, + description: NotBlankStr = Field( description="Criterion text", ) met: bool = Field( @@ -34,14 +34,6 @@ class AcceptanceCriterion(BaseModel): description="Whether this criterion has been satisfied", ) - @model_validator(mode="after") - def _validate_description_not_blank(self) -> Self: - """Ensure description is not whitespace-only.""" - if not self.description.strip(): - msg = "description must not be whitespace-only" - raise ValueError(msg) - return self - class Task(BaseModel): """A unit of work within the company. @@ -71,10 +63,9 @@ class Task(BaseModel): model_config = ConfigDict(frozen=True) - id: str = Field(min_length=1, description="Unique task identifier") - title: str = Field(min_length=1, description="Short task title") - description: str = Field( - min_length=1, + id: NotBlankStr = Field(description="Unique task identifier") + title: NotBlankStr = Field(description="Short task title") + description: NotBlankStr = Field( description="Detailed task description", ) type: TaskType = Field(description="Task work type") @@ -82,24 +73,21 @@ class Task(BaseModel): default=Priority.MEDIUM, description="Task priority", ) - project: str = Field( - min_length=1, + project: NotBlankStr = Field( description="Project ID this task belongs to", ) - created_by: str = Field( - min_length=1, + created_by: NotBlankStr = Field( description="Agent ID of the task creator", ) - assigned_to: str | None = Field( + assigned_to: NotBlankStr | None = Field( default=None, - min_length=1, description="Agent ID of the assignee", ) - reviewers: tuple[str, ...] = Field( + reviewers: tuple[NotBlankStr, ...] = Field( default=(), description="Agent IDs of designated reviewers", ) - dependencies: tuple[str, ...] = Field( + dependencies: tuple[NotBlankStr, ...] = Field( default=(), description="IDs of tasks this task depends on", ) @@ -130,15 +118,8 @@ class Task(BaseModel): ) @model_validator(mode="after") - def _validate_fields(self) -> Self: - """Validate string fields and deadline format.""" - for field_name in ("id", "title", "description", "project", "created_by"): - if not getattr(self, field_name).strip(): - msg = f"{field_name} must not be whitespace-only" - raise ValueError(msg) - if self.assigned_to is not None and not self.assigned_to.strip(): - msg = "assigned_to must not be whitespace-only" - raise ValueError(msg) + def _validate_deadline_format(self) -> Self: + """Validate deadline format if present.""" if self.deadline is not None: if not self.deadline.strip(): msg = "deadline must not be whitespace-only" @@ -152,12 +133,7 @@ def _validate_fields(self) -> Self: @model_validator(mode="after") def _validate_collections(self) -> Self: - """Validate collection entries, self-dependency, and uniqueness.""" - for field_name in ("reviewers", "dependencies"): - for value in getattr(self, field_name): - if not value.strip(): - msg = f"Empty or whitespace-only entry in {field_name}" - raise ValueError(msg) + """Validate self-dependency and uniqueness.""" if self.id in self.dependencies: msg = f"Task {self.id!r} cannot depend on itself" raise ValueError(msg) diff --git a/src/ai_company/core/types.py b/src/ai_company/core/types.py index 8b2e095aff..8dbbe63ac1 100644 --- a/src/ai_company/core/types.py +++ b/src/ai_company/core/types.py @@ -22,20 +22,15 @@ def _check_not_whitespace(value: str) -> str: """A string that must be non-empty and not consist solely of whitespace.""" -def validate_non_blank_unique_strings( +def validate_unique_strings( values: tuple[str, ...], field_name: str, ) -> None: - """Validate that every string in *values* is non-blank and unique. + """Validate that every string in *values* is unique. Raises: - ValueError: If any entry is empty/whitespace-only or if duplicates - are present. + ValueError: If duplicates are present. """ - for entry in values: - if not entry.strip(): - msg = f"Empty or whitespace-only entry in {field_name}" - raise ValueError(msg) if len(values) != len(set(values)): dupes = sorted(v for v, c in Counter(values).items() if c > 1) msg = f"Duplicate entries in {field_name}: {dupes}" diff --git a/src/ai_company/observability/config.py b/src/ai_company/observability/config.py index 8a55bb1a7d..ea3002f3e2 100644 --- a/src/ai_company/observability/config.py +++ b/src/ai_company/observability/config.py @@ -15,6 +15,7 @@ from pydantic import BaseModel, ConfigDict, Field, model_validator +from ai_company.core.types import NotBlankStr # noqa: TC001 from ai_company.observability.enums import LogLevel, RotationStrategy, SinkType @@ -140,7 +141,7 @@ class LogConfig(BaseModel): default=True, description="Whether to enable correlation ID tracking", ) - log_dir: str = Field( + log_dir: NotBlankStr = Field( default="logs", description="Directory for log files", ) @@ -181,10 +182,7 @@ def _validate_no_duplicate_file_paths(self) -> Self: @model_validator(mode="after") def _validate_log_dir_safe(self) -> Self: - """Ensure ``log_dir`` is not blank and has no path traversal.""" - if not self.log_dir.strip(): - msg = "log_dir must not be blank" - raise ValueError(msg) + """Ensure ``log_dir`` has no path traversal.""" path = PurePath(self.log_dir) if ".." in path.parts: msg = f"log_dir must not contain '..' components: {self.log_dir}" diff --git a/tests/unit/budget/test_config.py b/tests/unit/budget/test_config.py index 66becdfcdd..13cff53007 100644 --- a/tests/unit/budget/test_config.py +++ b/tests/unit/budget/test_config.py @@ -117,7 +117,7 @@ def test_custom_values(self) -> None: def test_empty_source_alias_rejected(self) -> None: """Reject whitespace-only source alias in downgrade_map.""" - with pytest.raises(ValidationError, match="source alias"): + with pytest.raises(ValidationError, match="at least 1 character"): AutoDowngradeConfig( enabled=True, downgrade_map=((" ", "sonnet"),), @@ -125,7 +125,7 @@ def test_empty_source_alias_rejected(self) -> None: def test_empty_target_alias_rejected(self) -> None: """Reject whitespace-only target alias in downgrade_map.""" - with pytest.raises(ValidationError, match="target alias"): + with pytest.raises(ValidationError, match="at least 1 character"): AutoDowngradeConfig( enabled=True, downgrade_map=(("opus", " "),), diff --git a/tests/unit/communication/test_channel.py b/tests/unit/communication/test_channel.py index 3915503d23..b635fa7b92 100644 --- a/tests/unit/communication/test_channel.py +++ b/tests/unit/communication/test_channel.py @@ -45,15 +45,11 @@ def test_whitespace_name_rejected(self) -> None: Channel(name=" ") def test_empty_subscriber_rejected(self) -> None: - with pytest.raises( - ValidationError, match="Empty or whitespace-only entry in subscribers" - ): + with pytest.raises(ValidationError, match="at least 1 character"): Channel(name="#test", subscribers=("agent-a", "")) def test_whitespace_subscriber_rejected(self) -> None: - with pytest.raises( - ValidationError, match="Empty or whitespace-only entry in subscribers" - ): + with pytest.raises(ValidationError, match="whitespace-only"): Channel(name="#test", subscribers=("agent-a", " ")) def test_duplicate_subscribers_rejected(self) -> None: diff --git a/tests/unit/communication/test_config.py b/tests/unit/communication/test_config.py index b3bc243d1d..9b560bad51 100644 --- a/tests/unit/communication/test_config.py +++ b/tests/unit/communication/test_config.py @@ -44,15 +44,11 @@ def test_custom_values(self) -> None: @pytest.mark.unit class TestMessageBusConfigValidation: def test_empty_channel_rejected(self) -> None: - with pytest.raises( - ValidationError, match="Empty or whitespace-only entry in channels" - ): + with pytest.raises(ValidationError, match="at least 1 character"): MessageBusConfig(channels=("#valid", "")) def test_whitespace_channel_rejected(self) -> None: - with pytest.raises( - ValidationError, match="Empty or whitespace-only entry in channels" - ): + with pytest.raises(ValidationError, match="whitespace-only"): MessageBusConfig(channels=("#valid", " ")) def test_duplicate_channels_rejected(self) -> None: @@ -159,7 +155,7 @@ def test_whitespace_trigger_rejected(self) -> None: def test_whitespace_participant_rejected(self) -> None: with pytest.raises( ValidationError, - match="Empty or whitespace-only entry in participants", + match="whitespace-only", ): MeetingTypeConfig( name="standup", frequency="daily", participants=("eng", " ") @@ -168,7 +164,7 @@ def test_whitespace_participant_rejected(self) -> None: def test_empty_participant_rejected(self) -> None: with pytest.raises( ValidationError, - match="Empty or whitespace-only entry in participants", + match="at least 1 character", ): MeetingTypeConfig( name="standup", frequency="daily", participants=("eng", "") diff --git a/tests/unit/core/test_agent.py b/tests/unit/core/test_agent.py index 07badbdc46..d559160ef3 100644 --- a/tests/unit/core/test_agent.py +++ b/tests/unit/core/test_agent.py @@ -74,12 +74,12 @@ def test_whitespace_communication_style_rejected(self) -> None: def test_empty_trait_rejected(self) -> None: """Reject empty string in traits tuple.""" - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="at least 1 character"): PersonalityConfig(traits=("analytical", "")) def test_whitespace_trait_rejected(self) -> None: """Reject whitespace-only trait entry.""" - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): PersonalityConfig(traits=(" ",)) def test_frozen(self) -> None: @@ -118,12 +118,12 @@ def test_custom_values(self) -> None: def test_empty_skill_name_rejected(self) -> None: """Reject empty string in primary skills.""" - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="at least 1 character"): SkillSet(primary=("python", "")) def test_whitespace_skill_name_rejected(self) -> None: """Reject whitespace-only skill name in secondary.""" - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): SkillSet(secondary=(" ",)) def test_empty_primary_error_mentions_primary(self) -> None: @@ -343,12 +343,12 @@ def test_case_insensitive_overlap_rejected(self) -> None: def test_empty_tool_name_rejected(self) -> None: """Reject empty string in allowed tools.""" - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="at least 1 character"): ToolPermissions(allowed=("git", "")) def test_whitespace_tool_name_rejected(self) -> None: """Reject whitespace-only tool name in denied.""" - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): ToolPermissions(denied=(" ",)) def test_frozen(self) -> None: diff --git a/tests/unit/core/test_artifact.py b/tests/unit/core/test_artifact.py index df6ce43ab6..b6cab08cdc 100644 --- a/tests/unit/core/test_artifact.py +++ b/tests/unit/core/test_artifact.py @@ -30,7 +30,7 @@ def test_empty_path_rejected(self) -> None: ExpectedArtifact(type=ArtifactType.CODE, path="") def test_whitespace_path_rejected(self) -> None: - with pytest.raises(ValidationError, match="path must not be whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): ExpectedArtifact(type=ArtifactType.CODE, path=" ") def test_frozen(self) -> None: @@ -89,7 +89,7 @@ def test_defaults(self) -> None: assert artifact.created_at is None def test_whitespace_id_rejected(self) -> None: - with pytest.raises(ValidationError, match="id must not be whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): Artifact( id=" ", type=ArtifactType.CODE, @@ -109,7 +109,7 @@ def test_empty_id_rejected(self) -> None: ) def test_whitespace_path_rejected(self) -> None: - with pytest.raises(ValidationError, match="path must not be whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): Artifact( id="artifact-1", type=ArtifactType.CODE, @@ -119,9 +119,7 @@ def test_whitespace_path_rejected(self) -> None: ) def test_whitespace_task_id_rejected(self) -> None: - with pytest.raises( - ValidationError, match="task_id must not be whitespace-only" - ): + with pytest.raises(ValidationError, match="whitespace-only"): Artifact( id="artifact-1", type=ArtifactType.CODE, @@ -131,9 +129,7 @@ def test_whitespace_task_id_rejected(self) -> None: ) def test_whitespace_created_by_rejected(self) -> None: - with pytest.raises( - ValidationError, match="created_by must not be whitespace-only" - ): + with pytest.raises(ValidationError, match="whitespace-only"): Artifact( id="artifact-1", type=ArtifactType.CODE, diff --git a/tests/unit/core/test_company.py b/tests/unit/core/test_company.py index ce85b7f935..93ab511a42 100644 --- a/tests/unit/core/test_company.py +++ b/tests/unit/core/test_company.py @@ -67,12 +67,12 @@ def test_whitespace_lead_rejected(self) -> None: def test_empty_member_name_rejected(self) -> None: """Reject empty string in members tuple.""" - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="at least 1 character"): Team(name="test", lead="lead", members=("dev", "")) def test_whitespace_member_name_rejected(self) -> None: """Reject whitespace-only member name.""" - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): Team(name="test", lead="lead", members=(" ",)) def test_duplicate_members_rejected(self) -> None: @@ -232,7 +232,7 @@ def test_whitespace_communication_pattern_rejected(self) -> None: def test_empty_tool_access_entry_rejected(self) -> None: """Reject empty string in tool_access_default tuple.""" - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="at least 1 character"): CompanyConfig(tool_access_default=("git", "")) def test_frozen(self) -> None: @@ -278,17 +278,17 @@ def test_duplicate_active_agents_rejected(self) -> None: def test_empty_active_agent_rejected(self) -> None: """Reject empty string in active_agents.""" - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="at least 1 character"): HRRegistry(active_agents=("",)) def test_empty_available_role_rejected(self) -> None: """Reject whitespace-only entry in available_roles.""" - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): HRRegistry(available_roles=(" ",)) def test_empty_hiring_queue_entry_rejected(self) -> None: """Reject empty string in hiring_queue.""" - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="at least 1 character"): HRRegistry(hiring_queue=("",)) def test_duplicate_available_roles_accepted(self) -> None: diff --git a/tests/unit/core/test_project.py b/tests/unit/core/test_project.py index 83f1600d31..c6bf42aadd 100644 --- a/tests/unit/core/test_project.py +++ b/tests/unit/core/test_project.py @@ -74,15 +74,15 @@ def test_empty_id_rejected(self) -> None: _make_project(id="") def test_whitespace_id_rejected(self) -> None: - with pytest.raises(ValidationError, match="id must not be whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): _make_project(id=" ") def test_whitespace_name_rejected(self) -> None: - with pytest.raises(ValidationError, match="name must not be whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): _make_project(name=" ") def test_whitespace_lead_rejected(self) -> None: - with pytest.raises(ValidationError, match="lead must not be whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): _make_project(lead=" ") def test_whitespace_deadline_rejected(self) -> None: @@ -104,11 +104,11 @@ def test_valid_iso_datetime_deadline_accepted(self) -> None: assert project.deadline == "2026-12-31T23:59:59" def test_empty_team_member_rejected(self) -> None: - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): _make_project(team=("agent-1", " ")) def test_empty_task_id_rejected(self) -> None: - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="at least 1 character"): _make_project(task_ids=("task-1", "")) diff --git a/tests/unit/core/test_role.py b/tests/unit/core/test_role.py index c536a49572..08543f3d28 100644 --- a/tests/unit/core/test_role.py +++ b/tests/unit/core/test_role.py @@ -84,11 +84,11 @@ def test_empty_reports_to_rejected(self) -> None: Authority(reports_to="") def test_empty_can_approve_entry_rejected(self) -> None: - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="at least 1 character"): Authority(can_approve=("code_review", "")) def test_whitespace_can_delegate_to_rejected(self) -> None: - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): Authority(can_delegate_to=(" ",)) def test_whitespace_reports_to_rejected(self) -> None: @@ -238,7 +238,7 @@ def test_invalid_department_rejected(self) -> None: Role(name="Test", department="not_a_department") # type: ignore[arg-type] def test_empty_required_skill_rejected(self) -> None: - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="at least 1 character"): Role( name="Dev", department=DepartmentName.ENGINEERING, @@ -246,7 +246,7 @@ def test_empty_required_skill_rejected(self) -> None: ) def test_whitespace_tool_access_rejected(self) -> None: - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): Role( name="Dev", department=DepartmentName.ENGINEERING, @@ -341,7 +341,7 @@ def test_whitespace_department_normalized(self) -> None: assert role.department == "blockchain" def test_empty_required_skill_rejected(self) -> None: - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="at least 1 character"): CustomRole( name="Dev", department="custom", diff --git a/tests/unit/core/test_task.py b/tests/unit/core/test_task.py index fefb6689de..60734ab194 100644 --- a/tests/unit/core/test_task.py +++ b/tests/unit/core/test_task.py @@ -54,9 +54,7 @@ def test_empty_description_rejected(self) -> None: AcceptanceCriterion(description="") def test_whitespace_description_rejected(self) -> None: - with pytest.raises( - ValidationError, match="description must not be whitespace-only" - ): + with pytest.raises(ValidationError, match="whitespace-only"): AcceptanceCriterion(description=" ") def test_frozen(self) -> None: @@ -144,35 +142,27 @@ def test_empty_id_rejected(self) -> None: _make_task(id="") def test_whitespace_id_rejected(self) -> None: - with pytest.raises(ValidationError, match="id must not be whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): _make_task(id=" ") def test_whitespace_title_rejected(self) -> None: - with pytest.raises(ValidationError, match="title must not be whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): _make_task(title=" ") def test_whitespace_description_rejected(self) -> None: - with pytest.raises( - ValidationError, match="description must not be whitespace-only" - ): + with pytest.raises(ValidationError, match="whitespace-only"): _make_task(description=" ") def test_whitespace_project_rejected(self) -> None: - with pytest.raises( - ValidationError, match="project must not be whitespace-only" - ): + with pytest.raises(ValidationError, match="whitespace-only"): _make_task(project=" ") def test_whitespace_created_by_rejected(self) -> None: - with pytest.raises( - ValidationError, match="created_by must not be whitespace-only" - ): + with pytest.raises(ValidationError, match="whitespace-only"): _make_task(created_by=" ") def test_whitespace_assigned_to_rejected(self) -> None: - with pytest.raises( - ValidationError, match="assigned_to must not be whitespace-only" - ): + with pytest.raises(ValidationError, match="whitespace-only"): _make_task( assigned_to=" ", status=TaskStatus.ASSIGNED, @@ -197,11 +187,11 @@ def test_valid_iso_datetime_deadline_accepted(self) -> None: assert task.deadline == "2026-12-31T23:59:59" def test_empty_reviewer_rejected(self) -> None: - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="whitespace-only"): _make_task(reviewers=("valid", " ")) def test_empty_dependency_rejected(self) -> None: - with pytest.raises(ValidationError, match="Empty or whitespace-only"): + with pytest.raises(ValidationError, match="at least 1 character"): _make_task(dependencies=("task-1", "")) diff --git a/tests/unit/core/test_types.py b/tests/unit/core/test_types.py new file mode 100644 index 0000000000..98cf610d9b --- /dev/null +++ b/tests/unit/core/test_types.py @@ -0,0 +1,129 @@ +"""Tests for core type annotations and validation helpers.""" + +import pytest +from pydantic import BaseModel, ConfigDict, ValidationError + +from ai_company.core.types import NotBlankStr, validate_unique_strings + +pytestmark = pytest.mark.timeout(30) + + +# ── Test models ───────────────────────────────────────────────── + + +class _ScalarModel(BaseModel): + model_config = ConfigDict(frozen=True) + value: NotBlankStr + + +class _OptionalModel(BaseModel): + model_config = ConfigDict(frozen=True) + value: NotBlankStr | None = None + + +class _TupleModel(BaseModel): + model_config = ConfigDict(frozen=True) + values: tuple[NotBlankStr, ...] + + +# ── NotBlankStr (scalar) ──────────────────────────────────────── + + +@pytest.mark.unit +class TestNotBlankStr: + def test_accepts_valid_string(self) -> None: + m = _ScalarModel(value="hello") + assert m.value == "hello" + + def test_rejects_empty_string(self) -> None: + with pytest.raises(ValidationError, match="at least 1 character"): + _ScalarModel(value="") + + def test_rejects_whitespace_only(self) -> None: + with pytest.raises(ValidationError, match="whitespace-only"): + _ScalarModel(value=" ") + + def test_rejects_tabs_only(self) -> None: + with pytest.raises(ValidationError, match="whitespace-only"): + _ScalarModel(value="\t\t") + + def test_rejects_newlines_only(self) -> None: + with pytest.raises(ValidationError, match="whitespace-only"): + _ScalarModel(value="\n\n") + + def test_accepts_string_with_spaces(self) -> None: + m = _ScalarModel(value=" hello ") + assert m.value == " hello " + + +# ── NotBlankStr | None ────────────────────────────────────────── + + +@pytest.mark.unit +class TestNotBlankStrOptional: + def test_accepts_none(self) -> None: + m = _OptionalModel(value=None) + assert m.value is None + + def test_accepts_valid_string(self) -> None: + m = _OptionalModel(value="hello") + assert m.value == "hello" + + def test_rejects_empty_string(self) -> None: + with pytest.raises(ValidationError, match="at least 1 character"): + _OptionalModel(value="") + + def test_rejects_whitespace_only(self) -> None: + with pytest.raises(ValidationError, match="whitespace-only"): + _OptionalModel(value=" ") + + +# ── tuple[NotBlankStr, ...] ───────────────────────────────────── + + +@pytest.mark.unit +class TestNotBlankStrTuple: + def test_accepts_valid_tuple(self) -> None: + m = _TupleModel(values=("a", "b", "c")) + assert m.values == ("a", "b", "c") + + def test_accepts_empty_tuple(self) -> None: + m = _TupleModel(values=()) + assert m.values == () + + def test_rejects_empty_element(self) -> None: + with pytest.raises(ValidationError, match="at least 1 character"): + _TupleModel(values=("valid", "")) + + def test_rejects_whitespace_element(self) -> None: + with pytest.raises(ValidationError, match="whitespace-only"): + _TupleModel(values=("valid", " ")) + + def test_per_element_validation(self) -> None: + """Each element is validated independently.""" + with pytest.raises(ValidationError): + _TupleModel(values=("", " ", "valid")) + + +# ── validate_unique_strings ───────────────────────────────────── + + +@pytest.mark.unit +class TestValidateUniqueStrings: + def test_accepts_unique(self) -> None: + validate_unique_strings(("a", "b", "c"), "test_field") + + def test_accepts_empty(self) -> None: + validate_unique_strings((), "test_field") + + def test_accepts_single(self) -> None: + validate_unique_strings(("a",), "test_field") + + def test_rejects_duplicates(self) -> None: + with pytest.raises(ValueError, match="Duplicate entries in test_field"): + validate_unique_strings(("a", "b", "a"), "test_field") + + def test_reports_all_duplicates(self) -> None: + with pytest.raises(ValueError, match="'a'") as exc_info: + validate_unique_strings(("a", "b", "a", "b"), "test_field") + assert "'b'" in str(exc_info.value) diff --git a/tests/unit/observability/test_config.py b/tests/unit/observability/test_config.py index 045deaf7b4..db8f15b165 100644 --- a/tests/unit/observability/test_config.py +++ b/tests/unit/observability/test_config.py @@ -231,7 +231,7 @@ def test_duplicate_file_paths_rejected(self) -> None: ) def test_blank_log_dir_rejected(self) -> None: - with pytest.raises(ValidationError, match="log_dir must not be blank"): + with pytest.raises(ValidationError, match="whitespace-only"): LogConfig(sinks=(_console_sink(),), log_dir=" ") def test_log_dir_traversal_rejected(self) -> None: From c6192d3450c0d8ac4e4abf9e237d91031c65ad00 Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Fri, 6 Mar 2026 10:33:05 +0100 Subject: [PATCH 2/3] refactor: fix stale docs and add missing tests from pre-PR review - Update CLAUDE.md: NotBlankStr convention from "Planned" to "Adopted", include optional and tuple variants in documentation - Fix 3 stale docstrings in communication/ that still said "non-blank and unique" after blank-check moved to NotBlankStr type annotation - Add empty-string deadline rejection tests for Project and Task - Simplify fragile assertion pattern in test_types.py Pre-reviewed by 9 agents, 6 findings addressed Co-Authored-By: Claude Opus 4.6 --- CLAUDE.md | 2 +- src/ai_company/communication/channel.py | 2 +- src/ai_company/communication/config.py | 4 ++-- tests/unit/core/test_project.py | 6 ++++++ tests/unit/core/test_task.py | 6 ++++++ tests/unit/core/test_types.py | 3 +-- 6 files changed, 17 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 36e4833348..cda760c616 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -68,7 +68,7 @@ src/ai_company/ - **Docstrings**: Google style, required on public classes/functions (enforced by ruff D rules) - **Immutability**: create new objects, never mutate existing ones. For non-Pydantic internal collections (registries, `BaseTool`), use `copy.deepcopy()` at construction + `MappingProxyType` wrapping for read-only enforcement. For `dict`/`list` fields in frozen Pydantic models, rely on `frozen=True` for field reassignment prevention and `copy.deepcopy()` at system boundaries (tool execution, LLM provider serialization, inter-agent delegation, serializing for persistence). - **Config vs runtime state**: frozen Pydantic models for config/identity; separate mutable-via-copy models (using `model_copy(update=...)`) for runtime state that evolves (e.g. agent execution state, task progress). Never mix static config fields with mutable runtime fields in one model. -- **Models**: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Adopted conventions: use `@computed_field` for derived values instead of storing + validating redundant fields (e.g. `TokenUsage.total_tokens`). Planned conventions for new code: use `NotBlankStr` (from `core.types`) for non-optional identifier/name fields instead of manual whitespace validators. Existing models are being migrated incrementally. +- **Models**: Pydantic v2 (`BaseModel`, `model_validator`, `computed_field`, `ConfigDict`). Adopted conventions: use `@computed_field` for derived values instead of storing + validating redundant fields (e.g. `TokenUsage.total_tokens`); use `NotBlankStr` (from `core.types`) for all identifier/name fields — including optional (`NotBlankStr | None`) and tuple (`tuple[NotBlankStr, ...]`) variants — instead of manual whitespace validators. - **Async concurrency**: prefer `asyncio.TaskGroup` for fan-out/fan-in parallel operations in new code (e.g. multiple tool invocations, parallel agent calls). Prefer structured concurrency over bare `create_task`. Existing code is being migrated incrementally. - **Line length**: 88 characters (ruff) - **Functions**: < 50 lines, files < 800 lines diff --git a/src/ai_company/communication/channel.py b/src/ai_company/communication/channel.py index af8541ff4a..34e5605d48 100644 --- a/src/ai_company/communication/channel.py +++ b/src/ai_company/communication/channel.py @@ -34,6 +34,6 @@ class Channel(BaseModel): @model_validator(mode="after") def _validate_subscribers(self) -> Self: - """Ensure subscriber entries are non-blank and unique.""" + """Ensure subscriber entries are unique.""" validate_unique_strings(self.subscribers, "subscribers") return self diff --git a/src/ai_company/communication/config.py b/src/ai_company/communication/config.py index 5df876fe92..ae1045fafc 100644 --- a/src/ai_company/communication/config.py +++ b/src/ai_company/communication/config.py @@ -49,7 +49,7 @@ class MessageBusConfig(BaseModel): @model_validator(mode="after") def _validate_channels(self) -> Self: - """Ensure channel names are non-blank and unique.""" + """Ensure channel names are unique.""" validate_unique_strings(self.channels, "channels") return self @@ -102,7 +102,7 @@ def _validate_frequency_or_trigger(self) -> Self: @model_validator(mode="after") def _validate_participants(self) -> Self: - """Ensure participant entries are non-blank and unique.""" + """Ensure participant entries are unique.""" validate_unique_strings(self.participants, "participants") return self diff --git a/tests/unit/core/test_project.py b/tests/unit/core/test_project.py index c6bf42aadd..ea00458b50 100644 --- a/tests/unit/core/test_project.py +++ b/tests/unit/core/test_project.py @@ -85,6 +85,12 @@ def test_whitespace_lead_rejected(self) -> None: with pytest.raises(ValidationError, match="whitespace-only"): _make_project(lead=" ") + def test_empty_deadline_rejected(self) -> None: + with pytest.raises( + ValidationError, match="deadline must not be whitespace-only" + ): + _make_project(deadline="") + def test_whitespace_deadline_rejected(self) -> None: with pytest.raises( ValidationError, match="deadline must not be whitespace-only" diff --git a/tests/unit/core/test_task.py b/tests/unit/core/test_task.py index 60734ab194..7f08844941 100644 --- a/tests/unit/core/test_task.py +++ b/tests/unit/core/test_task.py @@ -168,6 +168,12 @@ def test_whitespace_assigned_to_rejected(self) -> None: status=TaskStatus.ASSIGNED, ) + def test_empty_deadline_rejected(self) -> None: + with pytest.raises( + ValidationError, match="deadline must not be whitespace-only" + ): + _make_task(deadline="") + def test_whitespace_deadline_rejected(self) -> None: with pytest.raises( ValidationError, match="deadline must not be whitespace-only" diff --git a/tests/unit/core/test_types.py b/tests/unit/core/test_types.py index 98cf610d9b..85f6184507 100644 --- a/tests/unit/core/test_types.py +++ b/tests/unit/core/test_types.py @@ -124,6 +124,5 @@ def test_rejects_duplicates(self) -> None: validate_unique_strings(("a", "b", "a"), "test_field") def test_reports_all_duplicates(self) -> None: - with pytest.raises(ValueError, match="'a'") as exc_info: + with pytest.raises(ValueError, match=r"'a'.*'b'|'b'.*'a'"): validate_unique_strings(("a", "b", "a", "b"), "test_field") - assert "'b'" in str(exc_info.value) From fbe0ecd6ef3b3f58e6dcff8d07e62219f29083c2 Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Fri, 6 Mar 2026 10:57:27 +0100 Subject: [PATCH 3/3] fix: address 8 PR review items from local agents, CodeRabbit, and Copilot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Guard malformed downgrade_map entries in before-validator (CodeRabbit) - Clarify _normalize_downgrade_map docstring purpose (comment-analyzer) - Use NotBlankStr for logger_levels logger name (type-design-analyzer) - Add NotBlankStr | None optional variant to DESIGN_SPEC.md (CodeRabbit) - Fix CostRecord JSON example removing non-existent total_tokens (docs-consistency) - Rename misleading test names: empty → whitespace (pr-test-analyzer) - Remove redundant "(must be non-blank)" from ToolDefinition docstring (comment-analyzer) Co-Authored-By: Claude Opus 4.6 --- DESIGN_SPEC.md | 7 +++---- src/ai_company/budget/config.py | 21 +++++++++++++++++++-- src/ai_company/observability/config.py | 2 +- src/ai_company/providers/models.py | 2 +- tests/unit/core/test_project.py | 2 +- tests/unit/core/test_task.py | 2 +- 6 files changed, 26 insertions(+), 10 deletions(-) diff --git a/DESIGN_SPEC.md b/DESIGN_SPEC.md index d1d16f04e8..b3c05b206c 100644 --- a/DESIGN_SPEC.md +++ b/DESIGN_SPEC.md @@ -106,7 +106,7 @@ Every agent has a comprehensive identity. At the design level, agent data splits - **Config (immutable)**: identity, personality, skills, model preferences, tool permissions, authority. Defined at hire time, changed only by explicit reconfiguration. Represented as frozen Pydantic models. - **Runtime state (mutable-via-copy)**: current status, active task, conversation history, execution metrics. Evolves during agent operation. Represented as Pydantic models using `model_copy(update=...)` for state transitions — never mutated in place. -> **Current state (M2):** Only the config layer exists as `AgentIdentity` (frozen Pydantic model in `core/agent.py`). The runtime state layer will be introduced in M3 when the agent execution engine is implemented. All identifier/name fields use `NotBlankStr` (from `core.types`) for automatic whitespace rejection; tuple fields use `tuple[NotBlankStr, ...]` for per-element validation. +> **Current state (M2):** Only the config layer exists as `AgentIdentity` (frozen Pydantic model in `core/agent.py`). The runtime state layer will be introduced in M3 when the agent execution engine is implemented. All identifier/name fields use `NotBlankStr` (from `core.types`) for automatic whitespace rejection; optional identifier fields use `NotBlankStr | None`; tuple fields use `tuple[NotBlankStr, ...]` for per-element validation. ```yaml # --- Current (M2): Config layer — AgentIdentity (frozen) --- @@ -851,13 +851,12 @@ Every API call is tracked (illustrative schema): "model": "claude-sonnet-4-6", "input_tokens": 4500, "output_tokens": 1200, - "total_tokens": 5700, "cost_usd": 0.0315, "timestamp": "2026-02-27T10:30:00Z" } ``` -> **Implementation note:** `total_tokens` is a `@computed_field` property that returns `input_tokens + output_tokens` — no stored field or validator needed. Spending summary models (`AgentSpending`, `DepartmentSpending`, `PeriodSpending`) each independently define `total_cost_usd`, `total_input_tokens`, `total_output_tokens`, and `record_count` fields. Extracting a shared `_SpendingTotals` base is a planned convention (see §15.5). +> **Implementation note:** `CostRecord` stores `input_tokens` and `output_tokens`; `total_tokens` is not stored on `CostRecord` — it is a `@computed_field` property on `TokenUsage` (the model embedded in `CompletionResponse`). Spending summary models (`AgentSpending`, `DepartmentSpending`, `PeriodSpending`) each independently define `total_cost_usd`, `total_input_tokens`, `total_output_tokens`, and `record_count` fields. Extracting a shared `_SpendingTotals` base is a planned convention (see §15.5). ### 10.3 CFO Agent Responsibilities @@ -1439,7 +1438,7 @@ These conventions were established during the M0–M2 review cycle. **Adopted** | **Immutability strategy** | Adopted | `copy.deepcopy()` at construction + `MappingProxyType` wrapping for non-Pydantic internal collections (registries, `BaseTool`). For Pydantic frozen models: `frozen=True` prevents field reassignment; `copy.deepcopy()` at system boundaries (tool execution, LLM provider serialization) prevents nested mutation. No MappingProxyType inside Pydantic models (serialization friction). | Deep-copy at construction fully isolates nested structures; `MappingProxyType` enforces read-only access. Boundary-copy for Pydantic models is simple, centralized, and Pydantic-native. A future CPython built-in immutable mapping type (e.g. `frozendict`) would provide zero-friction field-level immutability when available. | | **Config vs runtime split** | Adopted (M3) | Frozen models for config/identity; `model_copy(update=...)` for runtime state transitions | `TaskExecution` and `AgentContext` (in `engine/`) are frozen Pydantic models that use `model_copy(update=...)` for copy-on-write state transitions without re-running validators (per Pydantic `model_copy` semantics). Config layer (`AgentIdentity`, `Task`) remains unchanged. | | **Derived fields** | Adopted | `@computed_field` instead of stored + validated | Eliminates redundant storage and impossible-to-fail validators. `TokenUsage.total_tokens` migrated from stored `Field` + `@model_validator` to `@computed_field` property. | -| **String validation** | Adopted | `NotBlankStr` type from `core.types` for all identifiers | Eliminates per-model `@model_validator` boilerplate for whitespace checks. All identifier/name fields use `NotBlankStr`; tuple fields use `tuple[NotBlankStr, ...]` for per-element validation. | +| **String validation** | Adopted | `NotBlankStr` type from `core.types` for all identifiers | Eliminates per-model `@model_validator` boilerplate for whitespace checks. All identifier/name fields use `NotBlankStr`; optional identifiers use `NotBlankStr \| None`; tuple fields use `tuple[NotBlankStr, ...]` for per-element validation. | | **Shared field groups** | Planned | Extract common field sets into base models (e.g. `_SpendingTotals`) | Prevents field duplication across spending summary models. Not yet implemented — each model independently defines fields. | | **Event constants** | Adopted (flat) | Single `events.py` module with domain-scoped naming (e.g. `PROVIDER_CALL_START`, `BUDGET_RECORD_ADDED`) | Current approach uses a single module. Splitting into per-domain submodules may be revisited when the file exceeds ~200 constants. | | **Parallel tool execution** | Planned | `asyncio.TaskGroup` in `ToolInvoker.invoke_all` | Structured concurrency with proper cancellation semantics. Currently sequential; migration planned for M3 when the agent engine needs concurrent tool calls. | diff --git a/src/ai_company/budget/config.py b/src/ai_company/budget/config.py index 22e67920a0..d1fdc66c76 100644 --- a/src/ai_company/budget/config.py +++ b/src/ai_company/budget/config.py @@ -97,13 +97,30 @@ class AutoDowngradeConfig(BaseModel): @model_validator(mode="before") @classmethod def _normalize_downgrade_map(cls, data: Any) -> Any: - """Strip whitespace from downgrade_map alias strings.""" + """Normalize downgrade_map aliases by stripping leading/trailing whitespace. + + Runs before NotBlankStr validation so that ``" gpt-4 "`` becomes + ``"gpt-4"`` rather than being kept with surrounding spaces. + Non-string or malformed entries are passed through unchanged so + that Pydantic can surface a proper field-level ``ValidationError``. + """ if isinstance(data, dict) and "downgrade_map" in data: raw_map = data["downgrade_map"] if isinstance(raw_map, (list, tuple)): + normalized: list[Any] = [] + for item in raw_map: + if ( + isinstance(item, (list, tuple)) + and len(item) == 2 # noqa: PLR2004 + and isinstance(item[0], str) + and isinstance(item[1], str) + ): + normalized.append((item[0].strip(), item[1].strip())) + else: + normalized.append(item) return { **data, - "downgrade_map": tuple((s.strip(), t.strip()) for s, t in raw_map), + "downgrade_map": tuple(normalized), } return data diff --git a/src/ai_company/observability/config.py b/src/ai_company/observability/config.py index ea3002f3e2..587f8e86c5 100644 --- a/src/ai_company/observability/config.py +++ b/src/ai_company/observability/config.py @@ -130,7 +130,7 @@ class LogConfig(BaseModel): default=LogLevel.DEBUG, description="Root logger level", ) - logger_levels: tuple[tuple[str, LogLevel], ...] = Field( + logger_levels: tuple[tuple[NotBlankStr, LogLevel], ...] = Field( default=(), description="Per-logger level overrides as (name, level) pairs", ) diff --git a/src/ai_company/providers/models.py b/src/ai_company/providers/models.py index 73e3610e96..a0e26bad78 100644 --- a/src/ai_company/providers/models.py +++ b/src/ai_company/providers/models.py @@ -78,7 +78,7 @@ class ToolDefinition(BaseModel): modify the schema. See DESIGN_SPEC.md section 15.5. Attributes: - name: Tool name (must be non-blank). + name: Tool name. description: Human-readable description of the tool. parameters_schema: JSON Schema dict describing the tool parameters. """ diff --git a/tests/unit/core/test_project.py b/tests/unit/core/test_project.py index ea00458b50..6f3b8e5c2b 100644 --- a/tests/unit/core/test_project.py +++ b/tests/unit/core/test_project.py @@ -109,7 +109,7 @@ def test_valid_iso_datetime_deadline_accepted(self) -> None: project = _make_project(deadline="2026-12-31T23:59:59") assert project.deadline == "2026-12-31T23:59:59" - def test_empty_team_member_rejected(self) -> None: + def test_whitespace_team_member_rejected(self) -> None: with pytest.raises(ValidationError, match="whitespace-only"): _make_project(team=("agent-1", " ")) diff --git a/tests/unit/core/test_task.py b/tests/unit/core/test_task.py index 7f08844941..4cec5eac48 100644 --- a/tests/unit/core/test_task.py +++ b/tests/unit/core/test_task.py @@ -192,7 +192,7 @@ def test_valid_iso_datetime_deadline_accepted(self) -> None: task = _make_task(deadline="2026-12-31T23:59:59") assert task.deadline == "2026-12-31T23:59:59" - def test_empty_reviewer_rejected(self) -> None: + def test_whitespace_reviewer_rejected(self) -> None: with pytest.raises(ValidationError, match="whitespace-only"): _make_task(reviewers=("valid", " "))