diff --git a/docs/reference/protocols-audit.md b/docs/reference/protocols-audit.md index 5e0873c8e9..0bb4ac513a 100644 --- a/docs/reference/protocols-audit.md +++ b/docs/reference/protocols-audit.md @@ -37,13 +37,13 @@ The counts are deliberately approximate: they're a triage tool, not a removal lo The 2026-05-10 snapshot counts in the original table were aggregated by hand and undercounted both the REMOVE and REVIEW rows. The table below reflects (a) the per-area row totals as they actually appear in the snapshot tables, and (b) the post-2026-05-11 cleanup outcome captured in the "Post-cleanup status" section. -| Total | Recommendation (2026-05-10 snapshot row counts) | After 2026-05-11 cleanup (#1864) | -|---|---|---| -| Protocol classes inventoried | 250 | 248 (2 deleted) | -| KEEP | 193 | 237 (44 audit re-flagged, plus pre-existing 193) | -| MAKE-RUNTIME-CHECKABLE | 0 (no `isinstance` sites surveyed) | 0 | -| REMOVE candidates (flag-only; must be re-verified at cleanup time) | 46 | 0 (2 deleted, 44 reclassified KEEP) | -| REVIEW (`_PrivatePrefixed` typing seams; intent inspection needed) | 11 | 11 (unchanged; #1865 owns these) | +| Total | Snapshot (2026-05-10) | After #1864 (2026-05-11) | After #1865 (2026-05-11) | +|---|---|---|---| +| Protocol classes inventoried | 250 | 248 (2 deleted) | 241 (7 more deleted) | +| KEEP | 193 | 237 (44 audit re-flagged, plus pre-existing 193) | 241 (5 `_PrivatePrefixed` retained with rationale; 1 fold-deletion drops conflict_detection.py's KEEP) | +| MAKE-RUNTIME-CHECKABLE | 0 (no `isinstance` sites surveyed) | 0 | 0 | +| REMOVE candidates (flag-only; must be re-verified at cleanup time) | 46 | 0 (2 deleted, 44 reclassified KEEP) | 0 | +| REVIEW (`_PrivatePrefixed` typing seams; intent inspection needed) | 11 | 11 (unchanged; #1865 owns these) | 0 (12 protocol names across the 11 snapshot rows: 5 retained with rationale, 5 deleted, 2 fold-deleted) | `@runtime_checkable` decoration: 209 of 250 (84%). The 41 without the decorator are listed inline below; absence is not a defect on its own; only a few need it. @@ -405,6 +405,34 @@ Categories of re-flag rationale: 5. **Multi-impl injection points missed by the audit doc's own duplicate detection**. `ParticipantResolver`@`meeting/participant.py:56` was flagged as a "dead duplicate" of an alleged twin in `meeting/protocol.py`; the twin does not exist. The participant.py copy has 2 impls (`PassthroughParticipantResolver`, `RegistryParticipantResolver`) in the same file and 3 test files. +### Issue #1865 outcomes (REVIEW pass + duplicate folds) + +Issue [#1865](https://github.com/Aureliolo/synthorg/issues/1865) closed out the 11 REVIEW rows, which cover 12 protocol names: 9 `_PrivatePrefixed` typing seams (the `engine/coordination/dispatcher_types.py` row lists 2 names in one snapshot entry), the non-prefixed `IsDuplicate` predicate in `persistence/_shared/audit.py`, and the 2 duplicate-named-pair REVIEW rows. The 5 retained protocols each carry a one-line `# ` rationale comment naming the consumer that justifies the seam; the 7 deletions (5 single-consumer seam collapses plus 2 duplicate-pair folds) collapsed single-consumer seams or removed pure dead code. + +#### Retained with rationale (5 protocols) + +Each gets a one-line `# ` design-rationale comment immediately above the `class` line. + +| Path | Line | Name | Rationale | +|---|---|---|---| +| api/lifecycle.py | 86 | `_AsyncStartStop` | Structural seam over the optional `synthorg[distributed]` `JetStreamTaskQueue`; consumed by `_cleanup_on_failure` and `_safe_shutdown`. | +| core/state_machine.py | 36 | `_HasValue` | Generic bound for `StateMachine[S]`; 4 structural users: `TaskStatus`, `RequestStatus`, `KanbanColumn`, `SprintStatus`. | +| persistence/_shared/audit.py | 160 | `IsDuplicate` | Driver-abstraction predicate; impls: `sqlite._shared.is_unique_constraint_error`, `postgres.audit_repository._postgres_is_duplicate`. | +| templates/_inheritance.py | 24 | `_RenderToDictFn` | Self-referential recursive callback for `render_parent_config`; consumer: `templates.renderer._render_to_dict` passes itself in to walk the parent chain. | +| providers/management/_capabilities_mixin.py | 55 | `_ServiceProtocol` | Narrows the mixin's `self`-type to the 3 attrs + 3 methods consumed; host: `ProviderManagementService`. | + +#### Deleted (7 protocols) + +| Path | Line | Name | Outcome | +|---|---|---|---| +| engine/quality/graders/heuristic.py | 27 | `_HeuristicGraderBridge` | Deleted. Collapsed to `bridge: EngineBridgeConfig` with a `TYPE_CHECKING` import; the single structural consumer was already `EngineBridgeConfig`. | +| engine/routing/scorer.py | 40 | `_RoutingScorerBridge` | Deleted. Same collapse as `_HeuristicGraderBridge`. | +| templates/model_matcher.py | 370 | `_ModelMatcherBridge` | Deleted. Same collapse as `_HeuristicGraderBridge`. | +| engine/coordination/attribution.py | 32 | `_ExecutionResultLike` | Deleted. Defined inside `if TYPE_CHECKING:` but never referenced as an annotation; runtime code already used `getattr()` on untyped values. Duck-type contract documented inline at the call site. | +| engine/coordination/attribution.py | 35 | `_AgentRunResultLike` | Deleted. Same as `_ExecutionResultLike`. | +| communication/meeting/conflict_detection.py | 39 | `ConflictDetector` (fold duplicate) | Deleted. Folded into the canonical definition at `communication/meeting/protocol.py:41`. The 5 importers already pointed at `protocol.py`; the `conflict_detection.py` copy was unimported dead code. | +| meta/signals/protocol.py | 26 | `SignalAggregator` (fold duplicate) | Deleted. File removed entirely (zero importers; sole content was this dead protocol). Folded into the canonical definition at `meta/protocol.py:38`. | + ### Recommended next pass for `scripts/protocol_audit.py` A follow-up issue should teach the audit script to (a) consult a manual override file (e.g. `data/protocols_audit_overrides.yaml`) keyed on `path:line:name` so KEEP decisions made here survive future regenerations, and (b) detect structural impls heuristically (same-file `class X:` siblings whose method names match the Protocol's, factory return type annotations) to reduce false-positive REMOVE flags. diff --git a/src/synthorg/api/lifecycle.py b/src/synthorg/api/lifecycle.py index dcec43ff67..510b7d59f1 100644 --- a/src/synthorg/api/lifecycle.py +++ b/src/synthorg/api/lifecycle.py @@ -83,6 +83,8 @@ """ +# Structural seam over the optional synthorg[distributed] JetStreamTaskQueue; +# consumers: _cleanup_on_failure, _safe_shutdown. class _AsyncStartStop(Protocol): """Minimal async lifecycle Protocol used by the distributed task queue hook. diff --git a/src/synthorg/communication/meeting/conflict_detection.py b/src/synthorg/communication/meeting/conflict_detection.py index fbd95d8e8c..e16f24c153 100644 --- a/src/synthorg/communication/meeting/conflict_detection.py +++ b/src/synthorg/communication/meeting/conflict_detection.py @@ -12,7 +12,7 @@ """ import re -from typing import Any, Protocol, runtime_checkable +from typing import Any from synthorg.core.json_parsing import extract_json_from_llm_response from synthorg.observability import get_logger @@ -35,26 +35,6 @@ _DEFAULT_SIMILARITY_THRESHOLD: float = 0.7 -@runtime_checkable -class ConflictDetector(Protocol): - """Protocol for pluggable conflict detection strategies. - - All conflict detectors share this interface so consumers can - type against the pluggable API under mypy strict mode. - """ - - def detect(self, response_content: str) -> bool: - """Determine whether the response indicates conflicts. - - Args: - response_content: The conflict-check agent response text. - - Returns: - True if conflicts detected, False otherwise. - """ - ... - - _MIN_POSITIONS_FOR_CONFLICT: int = 2 # Identity/metadata keys to skip during structured field comparison. diff --git a/src/synthorg/core/state_machine.py b/src/synthorg/core/state_machine.py index 7aa3079845..17943bb294 100644 --- a/src/synthorg/core/state_machine.py +++ b/src/synthorg/core/state_machine.py @@ -33,6 +33,8 @@ def validate_transition(current: TaskStatus, target: TaskStatus) -> None: logger = get_logger(__name__) +# Generic bound for StateMachine[S]; structural users: TaskStatus, +# RequestStatus, KanbanColumn, SprintStatus. class _HasValue(Protocol): """Structural type for enum-like states (e.g. ``StrEnum`` members).""" diff --git a/src/synthorg/engine/coordination/attribution.py b/src/synthorg/engine/coordination/attribution.py index 0e637999e6..700fd6179e 100644 --- a/src/synthorg/engine/coordination/attribution.py +++ b/src/synthorg/engine/coordination/attribution.py @@ -6,7 +6,7 @@ with attribution data built from routing decisions and wave outcomes. """ -from typing import TYPE_CHECKING, Literal, Protocol, Self +from typing import Final, Literal, Self from pydantic import BaseModel, ConfigDict, Field, computed_field, model_validator @@ -27,17 +27,6 @@ logger = get_logger(__name__) -if TYPE_CHECKING: - - class _ExecutionResultLike(Protocol): - error_message: str | None - - class _AgentRunResultLike(Protocol): - is_success: bool - termination_reason: TerminationReason | None - execution_result: _ExecutionResultLike | None - - FailureAttribution = Literal[ "direct", "upstream_contamination", @@ -48,7 +37,7 @@ class _AgentRunResultLike(Protocol): # Internal constant by design: defensive truncation of evidence text # attached to failure attributions; prevents oversized error-evidence # payloads. Not exposed to the settings registry. -_MAX_EVIDENCE_LENGTH = 500 +_MAX_EVIDENCE_LENGTH: Final[int] = 500 # Map FailureCategory -> FailureAttribution for error-based outcomes. _CATEGORY_TO_ATTRIBUTION: dict[FailureCategory, FailureAttribution] = { @@ -256,9 +245,12 @@ def _score_outcome( evidence=outcome_error[:_MAX_EVIDENCE_LENGTH], ) - # Case 2: Execution completed -- check if successful. - # outcome_result conforms to _AgentRunResultLike; typed as object - # at runtime to avoid circular import. + # Case 2: Execution completed -- check if successful. The + # ``outcome_result`` is duck-typed (an ``AgentRunResult`` in practice) + # so this module avoids importing ``engine.run_result`` and the cycle + # it would pull in; we expect ``.is_success: bool``, + # ``.termination_reason: TerminationReason | None``, and + # ``.execution_result: | None``. if outcome_result is not None: is_success = getattr(outcome_result, "is_success", False) if is_success: diff --git a/src/synthorg/engine/quality/graders/heuristic.py b/src/synthorg/engine/quality/graders/heuristic.py index 43598bcf31..7001ace442 100644 --- a/src/synthorg/engine/quality/graders/heuristic.py +++ b/src/synthorg/engine/quality/graders/heuristic.py @@ -1,7 +1,7 @@ """Heuristic rubric grader -- rule-based, deterministic.""" from datetime import UTC, datetime -from typing import TYPE_CHECKING, Protocol +from typing import TYPE_CHECKING from pydantic import BaseModel, ConfigDict, Field @@ -20,32 +20,11 @@ from synthorg.core.types import NotBlankStr from synthorg.engine.quality.verification import AtomicProbe from synthorg.engine.workflow.handoff import HandoffArtifact + from synthorg.settings.bridge_configs import EngineBridgeConfig logger = get_logger(__name__) -class _HeuristicGraderBridge(Protocol): - """Structural type for the EngineBridgeConfig quality_heuristic_* fields. - - Local Protocol declaration keeps the grader module free of a - runtime import of ``settings.bridge_configs``. The fields are - exposed as ``@property`` so this Protocol matches frozen Pydantic - models (whose attributes are read-only); mypy validates the - attribute mapping in ``from_bridge_config``. - """ - - @property - def quality_heuristic_pass_threshold(self) -> float: ... - @property - def quality_heuristic_pass_grade(self) -> float: ... - @property - def quality_heuristic_fail_grade(self) -> float: ... - @property - def quality_heuristic_confidence_ceiling(self) -> float: ... - @property - def quality_heuristic_confidence_bias(self) -> float: ... - - class HeuristicGraderConfig(BaseModel): """Operator-tunable thresholds + per-criterion grades for the heuristic grader. @@ -70,15 +49,13 @@ class HeuristicGraderConfig(BaseModel): confidence_bias: float = Field(default=0.1, ge=0.0, le=1.0) @classmethod - def from_bridge_config( - cls, bridge: _HeuristicGraderBridge - ) -> HeuristicGraderConfig: + def from_bridge_config(cls, bridge: EngineBridgeConfig) -> HeuristicGraderConfig: """Project the heuristic-grader subset out of an ``EngineBridgeConfig``. - Typed via the local ``_HeuristicGraderBridge`` Protocol so mypy - validates the attribute mapping without a runtime import of - ``settings.bridge_configs`` (which would cycle through the - engine namespace). + ``EngineBridgeConfig`` is imported under ``TYPE_CHECKING`` so the + grader module stays free of a runtime dependency on + ``settings.bridge_configs`` (which would cycle through the engine + namespace). """ return cls( pass_threshold=bridge.quality_heuristic_pass_threshold, diff --git a/src/synthorg/engine/routing/scorer.py b/src/synthorg/engine/routing/scorer.py index ee5ae8806b..671f6cf45c 100644 --- a/src/synthorg/engine/routing/scorer.py +++ b/src/synthorg/engine/routing/scorer.py @@ -7,7 +7,7 @@ :class:`RoutingScorerConfig` (resolved at construction time). """ -from typing import TYPE_CHECKING, Final, Protocol +from typing import TYPE_CHECKING, Final from pydantic import BaseModel, ConfigDict, Field, model_validator @@ -23,6 +23,7 @@ if TYPE_CHECKING: from synthorg.core.agent import AgentIdentity from synthorg.engine.decomposition.models import SubtaskDefinition + from synthorg.settings.bridge_configs import EngineBridgeConfig logger = get_logger(__name__) @@ -37,33 +38,6 @@ _WEIGHT_SUM_WARN_CEILING: Final[float] = 1.3 # lint-allow: magic-numbers -- ceiling -class _RoutingScorerBridge(Protocol): - """Structural type for the EngineBridgeConfig fields the scorer reads. - - Declared locally to break the import cycle that would otherwise - pull ``settings.bridge_configs`` into the engine routing module. - The fields are exposed as ``@property`` so this Protocol matches - frozen Pydantic models (whose attributes are read-only). Mypy - validates the attribute mapping in ``from_bridge_config`` against - this Protocol; any caller passing an object missing one of the - fields gets a static type error rather than a runtime - ``AttributeError``. - """ - - @property - def routing_weight_primary_skill(self) -> float: ... - @property - def routing_weight_secondary_skill(self) -> float: ... - @property - def routing_weight_tag_match_bonus(self) -> float: ... - @property - def routing_weight_role_match_bonus(self) -> float: ... - @property - def routing_weight_seniority_alignment_bonus(self) -> float: ... - @property - def routing_min_score(self) -> float: ... - - class RoutingScorerConfig(BaseModel): """Operator-tunable configuration for :class:`AgentTaskScorer`. @@ -116,13 +90,13 @@ def _check_weight_sum(self) -> RoutingScorerConfig: return self @classmethod - def from_bridge_config(cls, bridge: _RoutingScorerBridge) -> RoutingScorerConfig: + def from_bridge_config(cls, bridge: EngineBridgeConfig) -> RoutingScorerConfig: """Project the routing-scorer subset out of an ``EngineBridgeConfig``. - Typed via the local ``_RoutingScorerBridge`` Protocol so mypy - validates the attribute mapping without forcing a runtime - import of ``settings.bridge_configs`` (which would create a - cycle through the engine namespace). + ``EngineBridgeConfig`` is imported under ``TYPE_CHECKING`` so the + routing module stays free of a runtime dependency on + ``settings.bridge_configs`` (which would cycle through the engine + namespace). """ return cls( primary_skill_weight=bridge.routing_weight_primary_skill, diff --git a/src/synthorg/meta/signals/protocol.py b/src/synthorg/meta/signals/protocol.py deleted file mode 100644 index 870bf66a8a..0000000000 --- a/src/synthorg/meta/signals/protocol.py +++ /dev/null @@ -1,57 +0,0 @@ -"""Protocol for signal aggregators. - -Each aggregator wraps one existing subsystem and produces a typed -summary model for consumption by the rule engine and strategies. -""" - -from typing import TYPE_CHECKING, Protocol, runtime_checkable - -from synthorg.core.types import NotBlankStr # noqa: TC001 - -if TYPE_CHECKING: - from datetime import datetime - - from synthorg.meta.models import ( - OrgBudgetSummary, - OrgCoordinationSummary, - OrgErrorSummary, - OrgEvolutionSummary, - OrgPerformanceSummary, - OrgScalingSummary, - OrgTelemetrySummary, - ) - - -@runtime_checkable -class SignalAggregator(Protocol): - """Aggregates raw signals from a subsystem into a typed summary.""" - - @property - def domain(self) -> NotBlankStr: - """Signal domain name (e.g. 'performance', 'budget').""" - ... - - async def aggregate( - self, - *, - since: datetime, - until: datetime, - ) -> ( - OrgPerformanceSummary - | OrgBudgetSummary - | OrgCoordinationSummary - | OrgScalingSummary - | OrgErrorSummary - | OrgEvolutionSummary - | OrgTelemetrySummary - ): - """Collect and aggregate signals for the time window. - - Args: - since: Start of the observation window (UTC). - until: End of the observation window (UTC). - - Returns: - Domain-specific typed summary model. - """ - ... diff --git a/src/synthorg/persistence/_shared/audit.py b/src/synthorg/persistence/_shared/audit.py index a9d43acf57..2beaaf49f5 100644 --- a/src/synthorg/persistence/_shared/audit.py +++ b/src/synthorg/persistence/_shared/audit.py @@ -157,6 +157,8 @@ def row_to_audit_entry(row: dict[str, object]) -> AuditEntry: raise MalformedRowError(msg) from exc +# Driver-abstraction predicate; impls: sqlite._shared.is_unique_constraint_error, +# postgres.audit_repository._postgres_is_duplicate. class IsDuplicate(Protocol): """Driver-specific predicate that classifies an INSERT failure. diff --git a/src/synthorg/providers/management/_capabilities_mixin.py b/src/synthorg/providers/management/_capabilities_mixin.py index 8aa4ae5cd1..359e1a228d 100644 --- a/src/synthorg/providers/management/_capabilities_mixin.py +++ b/src/synthorg/providers/management/_capabilities_mixin.py @@ -52,6 +52,8 @@ logger = get_logger(__name__) +# Narrows the mixin's self-type to the 3 attrs + 3 methods consumed; +# host: ProviderManagementService (composed via MRO in service.py). class _ServiceProtocol(Protocol): """Subset of ``ProviderManagementService`` accessed by the mixin. diff --git a/src/synthorg/templates/_inheritance.py b/src/synthorg/templates/_inheritance.py index 45eca68889..234e027e40 100644 --- a/src/synthorg/templates/_inheritance.py +++ b/src/synthorg/templates/_inheritance.py @@ -5,7 +5,7 @@ Extracted from ``renderer.py`` to keep file sizes under 800 lines. """ -from typing import TYPE_CHECKING, Any, Protocol +from typing import TYPE_CHECKING, Any, Final, Protocol from synthorg.observability import get_logger from synthorg.observability.events.template import ( @@ -21,6 +21,8 @@ from synthorg.templates.schema import CompanyTemplate +# Self-referential callback for render_parent_config; consumer: +# templates.renderer._render_to_dict passes itself in to walk the parent chain. class _RenderToDictFn(Protocol): """Callback protocol for ``_render_to_dict``.""" @@ -39,7 +41,7 @@ def __call__( logger = get_logger(__name__) # Maximum inheritance chain depth. -_MAX_INHERITANCE_DEPTH = 10 +_MAX_INHERITANCE_DEPTH: Final[int] = 10 def _validate_inheritance_chain( diff --git a/src/synthorg/templates/model_matcher.py b/src/synthorg/templates/model_matcher.py index e39877215e..a3e601084c 100644 --- a/src/synthorg/templates/model_matcher.py +++ b/src/synthorg/templates/model_matcher.py @@ -7,7 +7,7 @@ """ from types import MappingProxyType -from typing import TYPE_CHECKING, Any, Protocol +from typing import TYPE_CHECKING, Any from pydantic import BaseModel, ConfigDict, Field, ValidationError @@ -25,6 +25,7 @@ from collections.abc import Mapping from synthorg.config.schema import ProviderModelConfig + from synthorg.settings.bridge_configs import EngineBridgeConfig from synthorg.templates.model_requirements import ModelRequirement logger = get_logger(__name__) @@ -367,28 +368,6 @@ def _rank_by_priority( return min(models, key=lambda m: abs(m.cost_per_1k_input - mid)) -class _ModelMatcherBridge(Protocol): - """Structural type for the EngineBridgeConfig matcher_* fields. - - Declared locally to keep ``model_matcher`` free of a top-level - import of ``settings.bridge_configs``; the fields are exposed as - ``@property`` so this Protocol matches frozen Pydantic models - (whose attributes are read-only). Mypy validates the attribute - mapping in ``from_bridge_config`` against this Protocol. - """ - - @property - def matcher_tier_base_score(self) -> float: ... - @property - def matcher_headroom_max_bonus(self) -> float: ... - @property - def matcher_priority_max_bonus(self) -> float: ... - @property - def matcher_headroom_ratio_cap(self) -> float: ... - @property - def matcher_balanced_partial_credit(self) -> float: ... - - class ModelMatcherConfig(BaseModel): """Operator-tunable score weights for the model matcher. @@ -418,8 +397,14 @@ class ModelMatcherConfig(BaseModel): balanced_partial_credit: float = Field(default=0.125, ge=0.0, le=1.0) @classmethod - def from_bridge_config(cls, bridge: _ModelMatcherBridge) -> ModelMatcherConfig: - """Project the matcher subset out of an ``EngineBridgeConfig``.""" + def from_bridge_config(cls, bridge: EngineBridgeConfig) -> ModelMatcherConfig: + """Project the matcher subset out of an ``EngineBridgeConfig``. + + ``EngineBridgeConfig`` is imported under ``TYPE_CHECKING`` to keep + ``model_matcher`` free of a top-level dependency on + ``settings.bridge_configs`` (which already imports several engine + types). + """ return cls( tier_base_score=bridge.matcher_tier_base_score, headroom_max_bonus=bridge.matcher_headroom_max_bonus,