refactor(domain): replace literal string types with typed enums - #152
refactor(domain): replace literal string types with typed enums#152morluto wants to merge 8 commits into
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| @model_validator(mode="after") | ||
| def tool_readiness_is_coherent(self) -> _InferenceReplayPlan: | ||
| if self.argv and self.tool_executable is None: | ||
| raise ValueError("an available tool requires an executable") | ||
| if self.argv and (self.tool_compatibility_reason is not None or self.tool_remediation): | ||
| raise ValueError("an available tool cannot carry incompatibility recovery") | ||
| return self | ||
|
|
||
| @computed_field # type: ignore[prop-decorator] | ||
| @property | ||
| def tool_available(self) -> bool: | ||
| return bool(self.argv) | ||
|
|
||
| @computed_field # type: ignore[prop-decorator] | ||
| @property | ||
| def tool_compatible(self) -> bool: | ||
| return self.tool_available |
There was a problem hiding this comment.
🔴 Planning an inference replay crashes when the benchmarking tool is installed but the wrong version
The replay plan now decides that the tool is usable purely from the presence of a command line (tool_available computed at src/flameox/application/inference.py:180-188) and then refuses any plan that also carries an incompatibility explanation, so planning against an installed-but-unsupported tool aborts with an internal error instead of returning the plan that explains the problem.
Impact: A user with, for example, an out-of-range AIPerf or SGLang installed can no longer plan a replay at all; instead of a clear "tool unsupported, install version X" answer they get an opaque internal validation failure.
How an unavailable-but-present discovery reaches the new coherence validator
discover_inference_tool returns UnavailableInferenceToolDiscovery with a non-None executable plus compatibility_reason/remediation when the executable exists but the version is out of range (src/flameox/application/inference_providers.py:404-431); discover_sglang does the same when benchmark_python exists but does not provide sglang 0.5.16 (src/flameox/application/inference_providers.py:363-373).
In InferenceReplayService.plan, the argv-producing request is built whenever discovery.executable is not None (src/flameox/application/inference.py:371-379), so argv is non-empty for those unavailable discoveries. Previously the plan recorded tool_available=discovery.available (False) and tool_compatible=discovery.compatible (False) while preserving tool_compatibility_reason and tool_remediation, and execution later raised a structured CAPABILITY_UNAVAILABLE DomainError with remediation.
Now tool_available is derived from bool(self.argv) and the after-validator rejects the combination:
if self.argv and (self.tool_compatibility_reason is not None or self.tool_remediation):
raise ValueError("an available tool cannot carry incompatibility recovery")so parse_inference_replay_plan raises ValidationError out of plan(), which callers only translate for DomainError. Either the plan should omit argv when the discovery is unavailable, or the model should keep an explicit availability field rather than inferring it from argv.
Prompt for agents
In src/flameox/application/inference.py, `_InferenceReplayPlan` now derives `tool_available`/`tool_compatible` from whether `argv` is non-empty, and `tool_readiness_is_coherent` rejects any plan that has argv together with `tool_compatibility_reason` or `tool_remediation`. However `InferenceReplayService.plan` builds the execution request (and therefore argv) whenever `discovery.executable is not None`, and `discover_inference_tool` / `discover_sglang` in src/flameox/application/inference_providers.py return UnavailableInferenceToolDiscovery objects that carry BOTH a real executable and a compatibility_reason/remediation (e.g. AIPerf installed but outside >=0.12,<0.13, or benchmark_python present without sglang 0.5.16). In that case parse_inference_replay_plan raises a pydantic ValidationError out of plan(), replacing the previous behaviour where a plan was returned with tool_available=False plus the remediation text (and execution later raised a structured CAPABILITY_UNAVAILABLE DomainError). Fix so that an incompatible-but-present tool still yields an actionable outcome: either do not build argv when the discovery is unavailable, or stop inferring availability from argv and keep it as an explicit field driven by discovery.available. Add a regression test covering plan() with an unavailable discovery whose executable is not None.
Was this helpful? React with 👍 or 👎 to provide feedback.
Promote loose `Literal[...]` and `str` status fields in the domain models to proper `StrEnum` types so invalid evidence, experiment, oracle, finding, capability, preflight, and capture states cannot be represented. New enums in `domain/models.py`: ExperimentOutcomeMethod, ExperimentOutcomeGoal, ExperimentOutcomeDisposition, TrialFailureClass, OracleStatus, FindingConfidence, EvidenceReferenceType, EvidenceRelation, MetricPolarity, PreflightMode, PreflightDisposition, ProbeKind, ProcessCancellationCause, CaptureContainment, CapabilityPermissionStatus, CapabilitySetupVerification. `evidence_status.py` replaces the single `EvidenceAvailability` model with a discriminated union (AvailableEvidence | EmptyEvidence | PartialEvidence | UnknownEvidence | UnavailableEvidence | RecoverableUnavailableEvidence) so `next_tool`/`next_arguments` are only representable on recoverable-unavailable states; adds `parse_evidence_availability` for typed validation. `config.py` adds `ContainmentPolicy` and `NetworkPolicy` enums. `execution.py` adopts `ProcessCancellationCause` from the domain. `models.py` adds `Mapping`/`Self` imports for the new typed helpers. Includes direct unit tests for the new domain enums and the evidence-status union, and registers the new test path in `tests/ownership.toml`. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Update the storage layer to construct and persist the new domain `StrEnum` types instead of plain strings for execution status, validation status, evidence level, finding confidence/assessment, and experiment outcome fields. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Update all application services (capture, experiments, comparisons, capabilities, preflight, recovery, integrity, records, summaries, drilldown, imports, inference, viewers, workloads, etc.) to construct and accept the new domain `StrEnum` types instead of plain strings. Covers status fields for execution, validation, evidence level, finding confidence/assessment/lifecycle, experiment outcome, oracle status, preflight mode/disposition, probe kind, capture containment, capability permission/setup, and evidence availability with recovery routing. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…erence protocol Update the analysis layer to use the new domain `StrEnum` types for evidence level, oracle status, experiment outcome, metric polarity, and comparison validity fields across recipe models, comparison logic, and the inference protocol. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Update adapter modules (kernel build, kernel validation, pytest, setup runtime) and their JSON schemas to use the new domain `StrEnum` types for validation status, execution status, and probe kind fields. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Update CLI and MCP server receipts and tool parameters to carry the new domain `StrEnum` types (ExecutionStatus, ValidationStatus, ExperimentOutcomeDisposition/Method, PreflightMode, EvidenceReferenceType) instead of `str` and inline `Literal` aliases. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Update golden, evidence publication, and shared test support code to construct the new domain `StrEnum` types (FindingConfidence, FindingAssessment, EvidenceLevel, ExecutionStatus, ValidationStatus) instead of plain strings. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
c03684f to
8e9aed7
Compare
| if initial_phase is None: | ||
| self._record_setup_completed(requested) | ||
| else: | ||
| self._record_setup_progress( | ||
| requested, | ||
| completed=already_available, | ||
| phase=initial_phase, | ||
| ) |
There was a problem hiding this comment.
🟡 Capability setup is durably recorded as finished for every requested tool before any of them are installed
The durable setup record is written as fully finished for all requested tools (_record_setup_completed(requested) at src/flameox/application/capabilities.py:1158-1159) before the tools have actually been prepared, so anyone polling setup progress can be told the work succeeded while it is still running or about to fail.
Impact: An agent polling capability setup can see a "completed" receipt naming tools that were never actually made available.
Why the pre-work receipt now claims every requested adapter
This code path is reached only when pending is non-empty (the if not pending: early return is above at src/flameox/application/capabilities.py:1126-1136), i.e. some requested adapters are still unavailable. Previously the receipt written here used completed=already_available with phase="completed", so it never claimed pending adapters were done. CapabilitySetupCompletedReceipt requires completed == requested (src/flameox/application/capabilities.py:94-98), so _record_setup_completed necessarily overstates progress.
The same overstatement occurs mid-flow after package installation at src/flameox/application/capabilities.py:1229-1230, where the previous code recorded completed=self._available_requested(requested); the flow can still fail afterwards via the not_ready check, which then rewrites a failure receipt.
Prompt for agents
In `CapabilityService` (src/flameox/application/capabilities.py), the refactor replaced the intermediate `_record_setup_receipt(..., phase="completed")` calls (which recorded `completed=already_available` / `self._available_requested(requested)`) with `_record_setup_completed(requested)`, which by construction records every requested adapter as completed. Both occurrences (the initial-phase branch and the post-install branch) happen before the final availability verification, so a concurrent reader can observe a completed receipt listing adapters that are not yet available. Consider only emitting a completed receipt once the adapters are verified available (i.e. at the end of the flow), and emitting a progress/failed receipt otherwise, or model an intermediate phase that carries the truthful `completed` subset.
Was this helpful? React with 👍 or 👎 to provide feedback.
| @model_validator(mode="after") | ||
| def availability_is_a_partition(self) -> WorkloadDependencySetupResult: | ||
| if len(set(self.requested)) != len(self.requested): | ||
| raise ValueError("requested requirements must be unique") | ||
| available = set(self.already_available) | ||
| if tuple(item for item in self.requested if item in available) != self.already_available: | ||
| raise ValueError("already-available requirements must be an ordered requested subset") | ||
| return self |
There was a problem hiding this comment.
🔍 New uniqueness invariants can reject caller-supplied duplicates
WorkloadDependencySetupResult.availability_is_a_partition raises when requested contains duplicates. requested is built from config.requirements.python_distributions without deduplication (src/flameox/application/dependencies.py:141-144), so a workload that lists the same distribution twice in flameox.toml would now fail the whole prepare_workload_dependencies call with a validation error rather than installing it once. The capability path is safe because it dedupes via dict.fromkeys before building the result.
Was this helpful? React with 👍 or 👎 to provide feedback.
| @staticmethod | ||
| def _canonical(manifest: RunManifest) -> RunManifest: | ||
| try: | ||
| canonical = parse_run_manifest(manifest.model_dump(mode="python")) | ||
| except ValueError as exc: | ||
| raise DomainError( | ||
| ErrorCode.WORKSPACE_INVALID, | ||
| "Run manifest is invalid and cannot be persisted.", | ||
| ) from exc | ||
| if type(canonical) is not type(manifest): | ||
| raise DomainError( | ||
| ErrorCode.WORKSPACE_INVALID, | ||
| "A run revision cannot change its run type.", | ||
| ) | ||
| return canonical |
There was a problem hiding this comment.
🔍 Run-store canonicalization changes the error surface for invalid manifests
_canonical re-parses every manifest before persistence and converts any validation failure into ErrorCode.WORKSPACE_INVALID. Combined with the new ExecutionRunManifest.lifecycle_is_coherent invariants (planned/running/terminal coupling of started_at, finished_at, process, and capture_status), any existing call site that produces a transiently inconsistent revision — for example writing a terminal execution status without finished_at — will now fail at append time instead of silently persisting. This is a broad behavioral tightening across every capture, import, inference, and recovery path; the added tests cover only the direct model paths, so it would be worth confirming the full capture lifecycle (including the timeout, resource-policy, and pipeline-failure branches in capture.py) always sets a finish timestamp.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ent tools Address three review findings on the typed-enum refactor: 1. Inference replay planning crashed with ValidationError when the tool was installed but incompatible (e.g. AIPerf wrong version). The plan built argv from `discovery.executable is not None`, but the coherence validator rejects argv + compatibility_reason. Fix: build argv only when `discovery.available` so the plan returns with empty argv, remediation, and compatibility_reason; execution later raises CAPABILITY_UNAVAILABLE DomainError. 2. Summary trial row parsing dropped the NULL fallback for failure_class. `TrialFailureClass(str(row[2]))` raised ValueError when row[2] was None (str(None) is "None", not a valid enum member). Fix: fall back to TrialFailureClass.NONE for NULL column values from externally written rows. 3. Legacy CapabilityList payloads with next_tool="list_capabilities" became unparseable because the computed field no longer includes that value. Fix: the legacy validator treats "list_capabilities" as None (no setup action needed) before comparing against the computed projection. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
1 participant
Summary
Promotes loose
Literal[...]andstrstatus fields across the domain, application, adapters, MCP, and storage layers to properStrEnumtypes and discriminated unions, continuing the direction set by #128 ("Make invalid evidence and MCP states unrepresentable").src/flameox/domain/models.py:ExperimentOutcomeMethod,ExperimentOutcomeGoal,ExperimentOutcomeDisposition,TrialFailureClass,OracleStatus,FindingConfidence,EvidenceReferenceType,EvidenceRelation,MetricPolarity,PreflightMode,PreflightDisposition,ProbeKind,ProcessCancellationCause,CaptureContainment,CapabilityPermissionStatus,CapabilitySetupVerification.EvidenceAvailabilitymodel insrc/flameox/evidence_status.pywith a discriminated union (AvailableEvidence | EmptyEvidence | PartialEvidence | UnknownEvidence | UnavailableEvidence | RecoverableUnavailableEvidence) sonext_tool/next_argumentsare only representable on recoverable-unavailable states; addsparse_evidence_availabilityfor typed validation.ContainmentPolicyandNetworkPolicyenums insrc/flameox/config.py.CaptureReceipt,ExperimentReceipt, tool parameters) now carry the enum types instead ofstr, andLiteralparameter aliases are replaced by the exported enums.tests/evidence/test_evidence_status.py(renamed fromtest_status.pyto avoid a module-name collision withtests/application/test_status.py), andtests/ownership.tomlis updated accordingly.No behavioral contracts changed; this is a type-safety refactor that makes previously invalid states unrepresentable at the schema boundary.
Test plan
uv run ruff check src testsuv run mypy src testsuv run pytest -q— 907 passed, 216 deselectedGenerated with Devin