From 1772fd8c0e6d92a01673e88f568794520ef85b7f Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Wed, 10 Jun 2026 16:58:24 -0700 Subject: [PATCH 01/18] Add hardening roadmap from 2026-06-10 project review Co-Authored-By: Claude Fable 5 --- plans/fcli-hardening-roadmap.md | 391 ++++++++++++++++++++++++++++++++ 1 file changed, 391 insertions(+) create mode 100644 plans/fcli-hardening-roadmap.md diff --git a/plans/fcli-hardening-roadmap.md b/plans/fcli-hardening-roadmap.md new file mode 100644 index 0000000..a387e2e --- /dev/null +++ b/plans/fcli-hardening-roadmap.md @@ -0,0 +1,391 @@ +# FCLI Hardening Roadmap + +## Purpose + +Close the gaps found in the 2026-06-10 full-project review (architecture, +code-quality, and test-depth passes). The theme of the findings: the project +is stricter about what gets in (policy, approval, validation) than about +noticing when its own internals fail (stripped asserts, swallowed exceptions, +unvalidated migrations). This roadmap fixes that, one stage at a time. + +Stages are ordered by risk: runtime correctness first, then audit integrity, +then test depth, then structural cleanup, then docs. Each stage is small +enough to land as one PR. + +## Source Inputs + +- 2026-06-10 review findings (architecture 7/10, code quality 7/10, + tests 7.5/10, docs 8/10). +- Current local verification results on `main` (commit `d00ab98`). + +## Current Baseline + +Verified on 2026-06-10: + +- `./scripts/uv run pytest` passes: 460 tests. +- `./scripts/uv run ruff check src tests` passes. +- `./scripts/uv run ruff format --check src tests` passes. +- `./scripts/uv run mypy` passes (strict, 44 source files). +- `./scripts/uv run foundation doctor` passes; all capabilities healthy. + +Note: `plans/fcli-fixes-roadmap.md` stages appear fully shipped (command-usage +classifier in `gap_handoff.py`/`orchestrator.py`, `LivePhase` in +`live_turn.py`, static gates green) but the roadmap was never marked complete. +Stage 9 closes that loop. + +## Reviewed And Rejected + +Findings from the review that were checked against the code and dropped: + +- "Scope grants persist without expiration" — wrong. `ScopeGrantStore` + (`services/scope_grants.py`) is session-scoped, in-memory, read-only, and + never persisted. No action needed. + +## Out Of Scope + +- Beekeeper Queen/worker architecture. +- New capabilities, providers, or UX surfaces. +- Full discriminated-union rewrite of the plan wire format (stage 2 adds + validation without changing the schema providers emit). + +## Stage 1: Executor Invariants Fail Loudly + +### Goal + +Replace production-path `assert` statements with typed errors. `python -O` +strips asserts entirely; this code mutates files and runs shell commands, so +its invariant guards must not be removable by an interpreter flag. + +### Findings + +16 asserts in `services/executor.py`: + +- Action-union guards: lines 485 (`action.question`), 501 (`action.tool_call`), + 515 and 862 (`action.shell`). +- Service-injection guards: lines 612–672 (`self._file_service`, + `self._git_service` not None, 12 occurrences). + +### Tasks + +1. Add a small module-level helper in `executor.py` that raises a typed + internal error (reuse `ToolExecutionError` or add an + `InternalExecutorError`) instead of asserting. Keep it private to the + module — no new abstractions. +2. Replace the 4 action-union asserts: a mismatch between `action.kind` and + its payload becomes a FAILED `ExecutionResult` with a clear error message, + not a crash. +3. Replace the 12 service-injection asserts: a `builtin.file.*` / + `builtin.git.*` dispatch without the service wired becomes a typed error + naming the missing service. +4. Grep the rest of `src/foundation/` for asserts on real code paths and apply + the same treatment (tests may keep asserts). + +### Tests + +Add before implementation: + +1. An action with `kind=TOOL_CALL` but `tool_call=None` produces a FAILED + result with the typed error message — under both normal and `-O` execution + semantics (simulate by calling the guard helper directly). +2. Dispatching `builtin.file.read` with `file_service=None` produces a typed + error, not `AttributeError` or `AssertionError`. +3. Existing 460 tests still pass. + +### Done Criteria + +- `grep -n "assert " src/foundation/services/executor.py` returns nothing. +- Invariant violations surface as FAILED execution results in the trace, not + interpreter crashes. + +## Stage 2: Plan-Action Union Validation At The Boundary + +### Goal + +Make invalid action shapes unrepresentable at validation time instead of +crash-time. Today Pydantic accepts an action whose `kind` says one thing and +whose payload fields say another; every consumer then re-checks by hand. + +### Tasks + +1. Add a `model_validator(mode="after")` to the planned-action model in + `models/` enforcing: the payload field matching `kind` is present, and + payload fields for other kinds are absent. +2. Keep the wire format unchanged — providers still emit `kind` + optional + payload fields. This is validation, not a schema migration. +3. Route validation failures through the existing plan-repair path (the + planner already retries on malformed plans), so a model that emits a + mismatched action gets one repair attempt rather than a hard stop. +4. Remove the per-consumer `isinstance`/None re-checks in `planner.py` + (~lines 476–498) that the validator now makes redundant. Stage 1's typed + guards in the executor stay — defense in depth at the execution boundary. + +### Tests + +1. `kind=TOOL_CALL` with `shell` payload set and `tool_call=None` fails + validation with a message naming both fields. +2. A mismatched action from the provider triggers one plan-repair round trip. +3. Valid plans for every `ActionKind` still validate. + +### Done Criteria + +- A mismatched kind/payload can no longer reach the executor. +- Plan-repair handles the new validation failure shape. + +## Stage 3: One Source Of Truth For Git Mutation Subcommands + +### Goal + +`planner.py:91` (`_GIT_MUTATION_SUBCOMMANDS`) and `guardrails.py:52` +(`_WRITE_GIT_SUBCOMMANDS`) define the same 16-entry set independently. If +they diverge, the planner will permit what policy blocks — or policy will +miss what the planner emits. + +### Tasks + +1. Define `GIT_MUTATION_SUBCOMMANDS: frozenset[str]` once, in `models/git.py` + (it is domain knowledge, not service logic). +2. Import it in both `planner.py` and `guardrails.py`; delete the local + copies. Keep local aliases if it keeps diffs small. + +### Tests + +1. Both modules reference the shared constant (identity check: + `planner module set is guardrails module set`). +2. Existing planner-validation and guardrails tests pass unchanged. + +### Done Criteria + +- Exactly one definition of the set exists in `src/foundation/`. + +## Stage 4: Audit-Trail Failures Become Visible + +### Goal + +The trace/event pipeline is the project's accountability story, but failures +in it are currently invisible: `observer.py:64` suppresses all event-sink +exceptions, and `gap_handoff.py:300` silently falls back when provider +phrasing fails. Keep the "never break the turn" property; lose the silence. + +### Tasks + +1. In `ObserverService`: count sink failures per session. On the first + failure, emit a WARNING the user can see (stderr notice via the existing + notice path, not just `logger.exception`). After N consecutive failures + (suggest N=3), disable that sink for the rest of the session and say so + once — a flapping sink should not spam. +2. Record sink degradation in the session's NDJSON index entry + (`sessions.jsonl`) so monitors can tell a complete event log from a + truncated one. +3. In `gap_handoff.py`: when `make_provider_phraser` falls back (exception or + `_sanitize_phrased_message` rejection), log the reason at WARNING with the + rejection category (exception / json-shaped / fenced / empty / too-long). + The user-facing fallback behavior stays the same. + +### Tests + +1. A sink that raises once: turn completes, WARNING notice emitted, sink + stays enabled. +2. A sink that raises 3 times consecutively: sink disabled, one + disabled-notice, subsequent events do not call it. +3. Sink degradation appears in the sessions index entry. +4. Phraser returning JSON-shaped output: fallback message used, WARNING + logged with category `json-shaped`. + +### Done Criteria + +- No silent audit-trail loss: every suppressed failure leaves a user-visible + or index-visible trace. +- A turn still never fails because of a sink or phrasing failure. + +## Stage 5: Migration Safety Rails + +### Goal + +`history.py:1382` (`_migrate_to_v6`) rebuilds `assistant_plans` to change a +unique constraint. Migrations run automatically against the user's real +history DB with no backup and no post-check. Add safety rails for v6 and +every future migration. + +### Tasks + +1. Before running any migration chain, copy the SQLite file to + `.pre-v.bak` (cheap, file-level). Remove or rotate old + backups; keep the most recent one only. +2. Wrap the whole migration chain in one transaction so a mid-chain failure + cannot leave a half-migrated schema. +3. After `_migrate_to_v6`'s table rebuild, validate row counts: rebuilt table + row count must equal the source count. On mismatch, roll back and raise + with the backup path in the message. +4. Apply the same count-validation pattern to any future rebuild-style + migration (note it in a comment at the migration dispatcher). + +### Tests + +1. Synthetic v5 DB fixture migrates to v6 with all rows preserved (count and + spot-check content). +2. A sabotaged rebuild (fixture with a row the new constraint rejects) rolls + back, raises with the backup path, and leaves the original DB readable. +3. Backup file exists after a successful migration. + +### Done Criteria + +- A failed migration can never destroy history: either it completes verified, + or the original DB and a backup survive. + +## Stage 6: Diff Applier Strictness Decision + +### Goal + +The unified-diff applier (`file_service.py` ~118–257) is lenient: bare lines +parse as context, and newline normalization lets CRLF/LF mismatches succeed +silently. Some leniency is deliberate (model-generated diffs are imperfect); +the problem is that it is undocumented and unbounded. Decide the contract, +then enforce it at parse time. + +### Tasks + +1. Decide and document per quirk: bare lines as context (keep — models drop + the leading space often — but count them), CRLF/LF normalization (keep, + but only as a fallback after an exact match fails), anything else found + while reading the parser. +2. Reject at parse time what is never valid: hunks whose declared counts + disagree with their body, hunks with no `+`/`-` lines at all. +3. Surface leniency: when a diff applies only via a fallback (normalized + newlines, bare-line context), include that fact in the execution artifact + so it lands in the trace. + +### Tests + +1. Hunk with wrong declared counts → parse-time `FileOperationError`, not a + match-time failure. +2. CRLF file + LF diff → applies, artifact notes normalized matching. +3. Existing apply_diff tests pass unchanged. + +### Done Criteria + +- The applier's leniency is documented in the module docstring, bounded, and + visible in traces when exercised. + +## Stage 7: Test Depth Where It Is Thin + +### Goal + +Coverage is strong on the orchestrator and providers but thin exactly where +failures are most likely in daily use: planner prompt/validation logic, the +Codex provider's failure paths, and live rendering edge cases. + +### Tasks + +1. New `tests/test_planner.py` exercising `PlannerService` in isolation with + `StubProvider`: observation injection (iteration, remaining-actions), + plan-time endpoint validation (`builtin.file.*`/`builtin.git.*` rejection + of unknown endpoints), zero-action plan repair, deferred-write + materialization, and the stage-2 union-validation repair path. +2. Codex provider failure paths in `tests/test_provider.py`: `codex` binary + missing, login/auth-expired stderr shape, malformed/non-JSON output, and + timeout — each mapping to the right `ProviderErrorCode`. +3. Live rendering edges in `tests/test_live_turn.py`: narrow terminal widths + (20 cols), spinner animation state preserved across `Live` refreshes, + stale-phase rendering, and ANSI-bearing event text. + +### Tests + +This stage is tests; the gate is meaningfulness, not count. Each new test +must assert behavior (output shape, error code, rendered text), not +implementation strings. + +### Done Criteria + +- `planner.py` edge cases are debuggable without running the orchestrator. +- Every `ProviderErrorCode` the Codex adapter can emit has a test. +- Full suite still passes; ruff and mypy stay green. + +## Stage 8: Orchestrator Slimming + +### Goal + +`RequestOrchestrator` (`orchestrator.py`) takes 14 constructor parameters and +owns ~40 methods, several of which are presentation or provider-salvage +concerns. Shrink it incrementally — no behavior change, no big-bang rewrite. + +### Tasks + +1. Group the 14 constructor parameters into one (or two) frozen context + objects (e.g. `OrchestratorRuntime` holding services, policy, stores). + Update call sites and test factories; keep keyword compatibility shims out + — fix the call sites instead. +2. Move provider-output salvage (`_unwrap_generated_file_body`, ~290–321) + into `planner.py` next to the other plan-repair logic. +3. Move presentation helpers (`_tool_result_preview` ~344, + `_format_tool_call_log_entry` ~443–463) into the rendering layer + (`cli_rendering.py`) or a small observation-formatting module — wherever + their only callers live. +4. Stop if any step requires changing behavior to proceed; this stage is + strictly mechanical. + +### Tests + +1. No new tests required; the gate is the existing suite passing unchanged + plus mypy strict on the new context objects. +2. Test factories (`tests/` orchestrator fixtures) updated to build the + context object once instead of threading 14 kwargs. + +### Done Criteria + +- `RequestOrchestrator.__init__` takes ≤ 4 parameters. +- No plan-salvage or presentation code remains in `orchestrator.py`. +- Diff shows moves and signature changes only — no logic edits. + +## Stage 9: Docs And Plans Hygiene + +### Goal + +Make `plans/` trustworthy again: a reader should be able to tell what is +done, what is pending, and what was abandoned. + +### Tasks + +1. Mark `plans/fcli-fixes-roadmap.md` complete (its stages shipped: command + error recovery, static gates, live phases). Add a one-line status header + with the verifying commit. +2. Add status headers to `plans/v2`, `plans/v3`, `plans/v4` roadmaps where + missing (v3/v4 are done). +3. Add a `CHANGELOG.md` entry for this hardening batch as stages land + (typed invariants, union validation, audit visibility, migration rails, + diff strictness). +4. Update `docs/TECHNICAL.md` only where stage 4 (visible degradation + notices) and stage 5 (migration backups) change user-visible behavior. + +### Done Criteria + +- Every file in `plans/` states whether it is shipped, in progress, or + superseded. +- CHANGELOG names the hardening changes without overpromising. + +## Cross-Stage Rules + +1. One stage per PR; the maintainer merges (agents prepare and hand off). +2. Tests are written before implementation within each stage. +3. Stages 1–5 are strictly ordered. Stages 6–7 may land in any order after 5. + Stage 8 lands only after 1–7 (it moves code the earlier stages touch). + Stage 9 closes the batch. +4. No stage adds dependencies, new capabilities, or new UX surfaces. + +## Final Completion Gate + +The hardening batch is complete when all of these pass on `main`: + +```bash +./scripts/uv run ruff check src tests +./scripts/uv run ruff format --check src tests +./scripts/uv run mypy +./scripts/uv run pytest +./scripts/uv run foundation doctor +``` + +…and additionally: + +- `grep -rn "assert " src/foundation/services/executor.py` is empty. +- Exactly one git-mutation subcommand set exists. +- A sabotaged-migration test proves history survives a failed migration. +- `plans/` has no unmarked completed roadmaps. From 3b849ff92b68de7eb506ab62ea1a0f620c88b50e Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Wed, 10 Jun 2026 17:21:06 -0700 Subject: [PATCH 02/18] Stage 1: replace executor asserts with typed invariant errors python -O strips assert statements, so the executor's 16 invariant guards (kind/payload narrowing and file/git service wiring) could silently vanish in optimized runs. Replace them with a _require helper raising ExecutorInvariantError; ActionExecutor.execute() converts violations into typed FAILED ExecutionResults instead of interpreter crashes. Co-Authored-By: Claude Fable 5 --- plans/fcli-hardening-roadmap.md | 11 ++ src/foundation/services/executor.py | 117 ++++++++++++++------- tests/test_executor.py | 157 ++++++++++++++++++++++++++++ 3 files changed, 245 insertions(+), 40 deletions(-) create mode 100644 tests/test_executor.py diff --git a/plans/fcli-hardening-roadmap.md b/plans/fcli-hardening-roadmap.md index a387e2e..7965ffa 100644 --- a/plans/fcli-hardening-roadmap.md +++ b/plans/fcli-hardening-roadmap.md @@ -50,6 +50,17 @@ Findings from the review that were checked against the code and dropped: ## Stage 1: Executor Invariants Fail Loudly +**Status: shipped 2026-06-10 (branch `feat/fcli-hardening`).** All 16 executor +asserts replaced with `ExecutorInvariantError` via a `_require` helper; one +catch site in `ActionExecutor.execute()` converts violations to FAILED +results. Task 4 sweep outcome: 26 asserts remain across 9 other source files; +they are plan-time payload narrowing (already enforced by `PlannedAction`'s +model validator), subprocess API narrowing (`process.stdout` after +`Popen(..., stdout=PIPE)`), or loop-invariant narrowing (`last_error` after a +retry loop). None sits on the side-effect execution path the way the executor +asserts did; converting them is deferred unless a later stage touches those +files anyway. + ### Goal Replace production-path `assert` statements with typed errors. `python -O` diff --git a/src/foundation/services/executor.py b/src/foundation/services/executor.py index 623bef1..8c53c9e 100644 --- a/src/foundation/services/executor.py +++ b/src/foundation/services/executor.py @@ -101,6 +101,20 @@ _SHELL_OUTPUT_PREVIEW_LIMIT = 240 +class ExecutorInvariantError(RuntimeError): + """An internal executor invariant was violated. + + Raised instead of ``assert`` so the guard survives ``python -O`` and + surfaces as a typed FAILED result rather than an interpreter crash. + """ + + +def _require[T](value: T | None, *, description: str) -> T: + if value is None: + raise ExecutorInvariantError(f"Internal executor invariant violated: {description}") + return value + + def _utcnow() -> str: return datetime.now(tz=UTC).replace(microsecond=0).isoformat().replace("+00:00", "Z") @@ -195,15 +209,25 @@ def execute( ) -> ActionExecutionEnvelope: started_at = _utcnow() started_monotonic = time.monotonic() - execution_result, approval_request, approval_resolution = self._handle_action( - action, - decision, - policy_evaluation=policy_evaluation, - plan_only=plan_only, - request_cwd=request_cwd, - request_id=request_id, - session_id=session_id, - ) + try: + execution_result, approval_request, approval_resolution = self._handle_action( + action, + decision, + policy_evaluation=policy_evaluation, + plan_only=plan_only, + request_cwd=request_cwd, + request_id=request_id, + session_id=session_id, + ) + except ExecutorInvariantError as exc: + execution_result = ExecutionResult( + action_id=action.id, + status=ExecutionStatus.FAILED, + summary=f"Internal error: {exc}", + error=str(exc), + ) + approval_request = None + approval_resolution = None completed_at = _utcnow() return ActionExecutionEnvelope( execution_result=execution_result, @@ -482,11 +506,14 @@ def _handle_action( ) if action.kind is ActionKind.QUESTION: - assert action.question is not None + question = _require( + action.question, + description=f"QUESTION action {action.id!r} is missing its question payload", + ) return ( self._handle_question( action.id, - action.question, + question, request_id=request_id, session_id=session_id, ), @@ -498,11 +525,14 @@ def _handle_action( self._policy_engine.register_invocation(policy_evaluation) if action.kind is ActionKind.TOOL_CALL: - assert action.tool_call is not None + tool_call = _require( + action.tool_call, + description=f"TOOL_CALL action {action.id!r} is missing its tool_call payload", + ) return ( self._execute_tool_call( action, - action.tool_call, + tool_call, policy_evaluation=policy_evaluation, request_cwd=request_cwd, request_id=request_id, @@ -512,7 +542,6 @@ def _handle_action( approval_resolution, ) - assert action.shell is not None return ( self._execute_shell_action( action, @@ -525,6 +554,18 @@ def _handle_action( approval_resolution, ) + def _require_file_service(self, endpoint: str) -> FileService: + return _require( + self._file_service, + description=f"{endpoint} dispatched without a file service wired", + ) + + def _require_git_service(self, endpoint: str) -> GitService: + return _require( + self._git_service, + description=f"{endpoint} dispatched without a git service wired", + ) + def _execute_tool_call( self, action: PlannedAction, @@ -609,68 +650,62 @@ def _execute_tool_call( ) artifact_type = ExecutionArtifactType.TLDR elif manifest.runtime_endpoint == "builtin.file.read": - assert self._file_service is not None - result = self._file_service.read( + result = self._require_file_service("builtin.file.read").read( FileReadRequest.model_validate(tool_call.arguments) ) artifact_type = ExecutionArtifactType.FILE_READ elif manifest.runtime_endpoint == "builtin.file.read_chunk": - assert self._file_service is not None - result = self._file_service.read_chunk( + result = self._require_file_service("builtin.file.read_chunk").read_chunk( FileReadChunkRequest.model_validate(tool_call.arguments) ) artifact_type = ExecutionArtifactType.FILE_READ_CHUNK elif manifest.runtime_endpoint == "builtin.file.write": - assert self._file_service is not None - result = self._file_service.write( + result = self._require_file_service("builtin.file.write").write( FileWriteRequest.model_validate(tool_call.arguments) ) artifact_type = ExecutionArtifactType.FILE_WRITE elif manifest.runtime_endpoint == "builtin.file.edit": - assert self._file_service is not None - result = self._file_service.edit( + result = self._require_file_service("builtin.file.edit").edit( FileEditRequest.model_validate(tool_call.arguments) ) artifact_type = ExecutionArtifactType.FILE_EDIT elif manifest.runtime_endpoint == "builtin.file.apply_diff": - assert self._file_service is not None - result = self._file_service.apply_diff( + result = self._require_file_service("builtin.file.apply_diff").apply_diff( FileApplyDiffRequest.model_validate(tool_call.arguments) ) artifact_type = ExecutionArtifactType.FILE_APPLY_DIFF elif manifest.runtime_endpoint == "builtin.git.status": - assert self._git_service is not None - result = self._git_service.status( + result = self._require_git_service("builtin.git.status").status( GitStatusRequest.model_validate(tool_call.arguments) ) artifact_type = ExecutionArtifactType.GIT_STATUS elif manifest.runtime_endpoint == "builtin.git.diff": - assert self._git_service is not None - result = self._git_service.diff(GitDiffRequest.model_validate(tool_call.arguments)) + result = self._require_git_service("builtin.git.diff").diff( + GitDiffRequest.model_validate(tool_call.arguments) + ) artifact_type = ExecutionArtifactType.GIT_DIFF elif manifest.runtime_endpoint == "builtin.git.show": - assert self._git_service is not None - result = self._git_service.show(GitShowRequest.model_validate(tool_call.arguments)) + result = self._require_git_service("builtin.git.show").show( + GitShowRequest.model_validate(tool_call.arguments) + ) artifact_type = ExecutionArtifactType.GIT_SHOW elif manifest.runtime_endpoint == "builtin.git.log": - assert self._git_service is not None - result = self._git_service.log(GitLogRequest.model_validate(tool_call.arguments)) + result = self._require_git_service("builtin.git.log").log( + GitLogRequest.model_validate(tool_call.arguments) + ) artifact_type = ExecutionArtifactType.GIT_LOG elif manifest.runtime_endpoint == "builtin.git.stage": - assert self._git_service is not None - result = self._git_service.stage( + result = self._require_git_service("builtin.git.stage").stage( GitStageRequest.model_validate(tool_call.arguments) ) artifact_type = ExecutionArtifactType.GIT_STAGE elif manifest.runtime_endpoint == "builtin.git.unstage": - assert self._git_service is not None - result = self._git_service.unstage( + result = self._require_git_service("builtin.git.unstage").unstage( GitUnstageRequest.model_validate(tool_call.arguments) ) artifact_type = ExecutionArtifactType.GIT_UNSTAGE elif manifest.runtime_endpoint == "builtin.git.commit": - assert self._git_service is not None - result = self._git_service.commit( + result = self._require_git_service("builtin.git.commit").commit( GitCommitRequest.model_validate(tool_call.arguments) ) artifact_type = ExecutionArtifactType.GIT_COMMIT @@ -859,8 +894,10 @@ def _execute_shell_action( request_id: str, session_id: str | None, ) -> ExecutionResult: - assert action.shell is not None - shell_action = action.shell + shell_action = _require( + action.shell, + description=f"SHELL action {action.id!r} is missing its shell payload", + ) shell_cwd = request_cwd if shell_action.cwd is None else Path(shell_action.cwd) command_preview = shlex.join([shell_action.command, *shell_action.args]) effective_timeout = shell_action.timeout_seconds diff --git a/tests/test_executor.py b/tests/test_executor.py new file mode 100644 index 0000000..b6e8ace --- /dev/null +++ b/tests/test_executor.py @@ -0,0 +1,157 @@ +"""Invariant guards in the action executor (hardening stage 1). + +These tests deliberately bypass model validation (``model_construct``) to +simulate the states the executor's invariant guards protect against: a +kind/payload mismatch that slipped past planning, or a builtin endpoint +dispatched without its backing service wired. The guards must surface these +as typed FAILED results, never as ``AssertionError`` (which ``python -O`` +strips entirely). +""" + +from __future__ import annotations + +from pathlib import Path +from types import SimpleNamespace +from typing import Any + +import pytest + +from foundation.models import ( + ActionKind, + ExecutionStatus, + PlannedAction, + PolicyDecision, + PolicyDecisionType, + ToolCall, +) +from foundation.services import executor as executor_module +from foundation.services.executor import ActionExecutor + + +class _NullObserver: + def emit(self, *args: Any, **kwargs: Any) -> None: + pass + + +class _StubRegistry: + """Resolves every capability to a manifest with a fixed runtime endpoint.""" + + def __init__(self, endpoint: str) -> None: + self._endpoint = endpoint + + def resolve(self, capability_id: str, version: str | None) -> Any: + return SimpleNamespace(runtime_endpoint=self._endpoint) + + +def _make_executor(tmp_path: Path, *, registry: Any = None) -> ActionExecutor: + return ActionExecutor( + workspace_root=tmp_path, + shell_runtime=None, # type: ignore[arg-type] + tool_service=None, # type: ignore[arg-type] + policy_engine=None, # type: ignore[arg-type] + approval_service=None, # type: ignore[arg-type] + capability_registry=registry, + observer=_NullObserver(), # type: ignore[arg-type] + file_service=None, + git_service=None, + ) + + +def _allow(action_id: str) -> PolicyDecision: + return PolicyDecision( + action_id=action_id, + decision=PolicyDecisionType.ALLOW, + reason="allowed for invariant test", + ) + + +def _invalid_action(kind: ActionKind, *, tool_call: ToolCall | None = None) -> PlannedAction: + """Build a kind/payload-mismatched action, bypassing model validation.""" + return PlannedAction.model_construct( + id="a1", + kind=kind, + summary="invariant test action", + requires_approval=False, + approval_reason=None, + explanation=None, + shell=None, + tool_call=tool_call, + question=None, + ) + + +def _execute(executor: ActionExecutor, action: PlannedAction, tmp_path: Path) -> Any: + return executor.execute( + action, + _allow(action.id), + policy_evaluation=None, + plan_only=False, + request_cwd=tmp_path, + request_id="req-1", + session_id=None, + ) + + +class TestRequireHelper: + def test_returns_value_when_present(self) -> None: + assert executor_module._require("value", description="anything") == "value" + + def test_raises_typed_error_when_missing(self) -> None: + with pytest.raises(executor_module.ExecutorInvariantError, match="missing thing"): + executor_module._require(None, description="missing thing") + + +class TestKindPayloadInvariants: + def test_question_action_missing_payload_fails_typed(self, tmp_path: Path) -> None: + action = _invalid_action(ActionKind.QUESTION) + envelope = _execute(_make_executor(tmp_path), action, tmp_path) + result = envelope.execution_result + assert result.status is ExecutionStatus.FAILED + assert result.error is not None + assert "invariant" in result.error.lower() + assert "question" in result.error.lower() + + def test_tool_call_action_missing_payload_fails_typed(self, tmp_path: Path) -> None: + action = _invalid_action(ActionKind.TOOL_CALL) + envelope = _execute(_make_executor(tmp_path), action, tmp_path) + result = envelope.execution_result + assert result.status is ExecutionStatus.FAILED + assert result.error is not None + assert "invariant" in result.error.lower() + assert "tool_call" in result.error.lower() + + def test_shell_action_missing_payload_fails_typed(self, tmp_path: Path) -> None: + envelope = _execute(_make_executor(tmp_path), _invalid_action(ActionKind.SHELL), tmp_path) + result = envelope.execution_result + assert result.status is ExecutionStatus.FAILED + assert result.error is not None + assert "invariant" in result.error.lower() + assert "shell" in result.error.lower() + + +class TestServiceWiringInvariants: + def _tool_action(self, capability_id: str) -> PlannedAction: + return PlannedAction( + id="a1", + kind=ActionKind.TOOL_CALL, + summary="invariant test action", + tool_call=ToolCall(capability_id=capability_id, arguments={}), + ) + + def test_file_endpoint_without_file_service_fails_typed(self, tmp_path: Path) -> None: + executor = _make_executor(tmp_path, registry=_StubRegistry("builtin.file.read")) + envelope = _execute(executor, self._tool_action("foundation.file.read"), tmp_path) + result = envelope.execution_result + assert result.status is ExecutionStatus.FAILED + assert result.error is not None + assert "invariant" in result.error.lower() + assert "file service" in result.error.lower() + + def test_git_endpoint_without_git_service_fails_typed(self, tmp_path: Path) -> None: + executor = _make_executor(tmp_path, registry=_StubRegistry("builtin.git.status")) + envelope = _execute(executor, self._tool_action("foundation.git.status"), tmp_path) + result = envelope.execution_result + assert result.status is ExecutionStatus.FAILED + assert result.error is not None + assert "invariant" in result.error.lower() + assert "git service" in result.error.lower() From 7971c86ecdba43a16f606adfe5355f2c70b1b531 Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Wed, 10 Jun 2026 17:27:42 -0700 Subject: [PATCH 03/18] Stage 2: close cross-payload holes in PlannedAction validation The kind/payload validator already enforced payload presence but accepted a stray question payload on EXPLANATION/SHELL/TOOL_CALL actions and a stray explanation on QUESTION actions. Every cross-kind payload is now rejected at validation time; failures route through the existing plan repair loop. Co-Authored-By: Claude Fable 5 --- plans/fcli-hardening-roadmap.md | 11 ++++ src/foundation/models/orchestration.py | 28 +++++++--- tests/test_orchestration_models.py | 71 ++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 8 deletions(-) create mode 100644 tests/test_orchestration_models.py diff --git a/plans/fcli-hardening-roadmap.md b/plans/fcli-hardening-roadmap.md index 7965ffa..9f39f4b 100644 --- a/plans/fcli-hardening-roadmap.md +++ b/plans/fcli-hardening-roadmap.md @@ -110,6 +110,17 @@ Add before implementation: ## Stage 2: Plan-Action Union Validation At The Boundary +**Status: shipped 2026-06-10, with a smaller scope than planned.** The review +finding this stage was based on was outdated: `PlannedAction` already has a +`model_validator` enforcing kind/payload presence and most cross-payload +rejections, and plan validation failures already route through the repair +loop (`planner.py` catches `(ValidationError, PlanningError)` and retries +with repair messages). What actually shipped: closing the validator's +remaining holes — a stray `question` payload was accepted on +EXPLANATION/SHELL/TOOL_CALL actions, and a stray `explanation` on QUESTION +actions. Task 4 (removing per-consumer None re-checks) was **rejected**: those +checks are mypy-strict Optional narrowing, not redundant validation. + ### Goal Make invalid action shapes unrepresentable at validation time instead of diff --git a/src/foundation/models/orchestration.py b/src/foundation/models/orchestration.py index c08b16b..d9416d5 100644 --- a/src/foundation/models/orchestration.py +++ b/src/foundation/models/orchestration.py @@ -266,23 +266,35 @@ def _validate_payload_shape(self) -> PlannedAction: if self.kind is ActionKind.EXPLANATION: if not self.explanation: raise ValueError("Explanation actions require the explanation field") - if self.shell is not None or self.tool_call is not None: - raise ValueError("Explanation actions cannot include shell or tool payloads") + if self.shell is not None or self.tool_call is not None or self.question is not None: + raise ValueError( + "Explanation actions cannot include shell, tool, or question payloads" + ) elif self.kind is ActionKind.SHELL: if self.shell is None: raise ValueError("Shell actions require the shell field") - if self.explanation is not None or self.tool_call is not None: - raise ValueError("Shell actions cannot include explanation or tool payloads") + if ( + self.explanation is not None + or self.tool_call is not None + or self.question is not None + ): + raise ValueError( + "Shell actions cannot include explanation, tool, or question payloads" + ) elif self.kind is ActionKind.TOOL_CALL: if self.tool_call is None: raise ValueError("Tool-call actions require the tool_call field") - if self.explanation is not None or self.shell is not None: - raise ValueError("Tool-call actions cannot include explanation or shell payloads") + if self.explanation is not None or self.shell is not None or self.question is not None: + raise ValueError( + "Tool-call actions cannot include explanation, shell, or question payloads" + ) elif self.kind is ActionKind.QUESTION: if self.question is None: raise ValueError("Question actions require the question field") - if self.shell is not None or self.tool_call is not None: - raise ValueError("Question actions cannot include shell or tool payloads") + if self.shell is not None or self.tool_call is not None or self.explanation is not None: + raise ValueError( + "Question actions cannot include shell, tool, or explanation payloads" + ) if self.requires_approval and not self.approval_reason: raise ValueError("Approval-required actions must include approval_reason") diff --git a/tests/test_orchestration_models.py b/tests/test_orchestration_models.py new file mode 100644 index 0000000..4ef868c --- /dev/null +++ b/tests/test_orchestration_models.py @@ -0,0 +1,71 @@ +"""Kind/payload shape validation on PlannedAction (hardening stage 2). + +The model validator must reject every cross-kind payload combination, so a +mismatched action can never reach policy evaluation or execution. +""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError + +from foundation.models import ( + ActionKind, + PlannedAction, + QuestionAction, + ShellAction, + ToolCall, +) + +_QUESTION = QuestionAction(prompt="Which file should I edit?") +_SHELL = ShellAction(command="ls") +_TOOL_CALL = ToolCall(capability_id="foundation.file.read", arguments={}) + + +def _action(kind: ActionKind, **payloads: object) -> PlannedAction: + return PlannedAction.model_validate( + { + "id": "a1", + "kind": kind, + "summary": "payload shape test", + **payloads, + } + ) + + +class TestValidShapes: + def test_explanation_action_validates(self) -> None: + action = _action(ActionKind.EXPLANATION, explanation="done") + assert action.kind is ActionKind.EXPLANATION + + def test_shell_action_validates(self) -> None: + action = _action(ActionKind.SHELL, shell=_SHELL) + assert action.shell is not None + + def test_tool_call_action_validates(self) -> None: + action = _action(ActionKind.TOOL_CALL, tool_call=_TOOL_CALL) + assert action.tool_call is not None + + def test_question_action_validates(self) -> None: + action = _action(ActionKind.QUESTION, question=_QUESTION) + assert action.question is not None + + +class TestStrayQuestionPayload: + def test_explanation_action_rejects_question_payload(self) -> None: + with pytest.raises(ValidationError, match="question"): + _action(ActionKind.EXPLANATION, explanation="done", question=_QUESTION) + + def test_shell_action_rejects_question_payload(self) -> None: + with pytest.raises(ValidationError, match="question"): + _action(ActionKind.SHELL, shell=_SHELL, question=_QUESTION) + + def test_tool_call_action_rejects_question_payload(self) -> None: + with pytest.raises(ValidationError, match="question"): + _action(ActionKind.TOOL_CALL, tool_call=_TOOL_CALL, question=_QUESTION) + + +class TestStrayExplanationPayload: + def test_question_action_rejects_explanation_payload(self) -> None: + with pytest.raises(ValidationError, match="explanation"): + _action(ActionKind.QUESTION, question=_QUESTION, explanation="stray") From 73e4873a5f958efcc7df638e2d11932d1ffc1d3e Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Wed, 10 Jun 2026 17:35:04 -0700 Subject: [PATCH 04/18] Stage 3: single source of truth for git mutation subcommands Planner and guardrails each maintained an identical 16-entry set; a divergence would let the planner permit what policy blocks. The set now lives once in models/git.py with both services aliasing it, plus an identity test. Co-Authored-By: Claude Fable 5 --- plans/fcli-hardening-roadmap.md | 4 ++++ src/foundation/models/git.py | 25 +++++++++++++++++++++++++ src/foundation/services/guardrails.py | 20 ++------------------ src/foundation/services/planner.py | 22 ++-------------------- tests/test_policy.py | 9 +++++++++ 5 files changed, 42 insertions(+), 38 deletions(-) diff --git a/plans/fcli-hardening-roadmap.md b/plans/fcli-hardening-roadmap.md index 9f39f4b..337f328 100644 --- a/plans/fcli-hardening-roadmap.md +++ b/plans/fcli-hardening-roadmap.md @@ -155,6 +155,10 @@ whose payload fields say another; every consumer then re-checks by hand. ## Stage 3: One Source Of Truth For Git Mutation Subcommands +**Status: shipped 2026-06-10.** `GIT_MUTATION_SUBCOMMANDS` defined once in +`models/git.py`; planner and guardrails alias it. Identity test added in +`tests/test_policy.py`. + ### Goal `planner.py:91` (`_GIT_MUTATION_SUBCOMMANDS`) and `guardrails.py:52` diff --git a/src/foundation/models/git.py b/src/foundation/models/git.py index 9f35870..cdbbf3d 100644 --- a/src/foundation/models/git.py +++ b/src/foundation/models/git.py @@ -13,6 +13,31 @@ class StrictModel(BaseModel): model_config = ConfigDict(extra="forbid") +# Git subcommands that mutate the working tree, index, or history. The planner +# (preflight review gating) and the guardrail policy engine (write-risk +# classification) must agree on this set, so it is defined exactly once here. +GIT_MUTATION_SUBCOMMANDS: frozenset[str] = frozenset( + { + "add", + "apply", + "checkout", + "cherry-pick", + "clean", + "commit", + "merge", + "mv", + "rebase", + "reset", + "restore", + "revert", + "rm", + "stash", + "switch", + "tag", + } +) + + # --------------------------------------------------------------------------- # Error types # --------------------------------------------------------------------------- diff --git a/src/foundation/services/guardrails.py b/src/foundation/services/guardrails.py index 79da9a8..68ed170 100644 --- a/src/foundation/services/guardrails.py +++ b/src/foundation/services/guardrails.py @@ -35,6 +35,7 @@ ToolCall, TrustTier, ) +from foundation.models.git import GIT_MUTATION_SUBCOMMANDS from foundation.services.capabilities import SHELL_CAPABILITY_ID, CapabilityRegistry from foundation.services.scope_grants import ScopeGrantStore from foundation.settings import ApprovalMode @@ -49,24 +50,7 @@ _PERMISSION_COMMANDS = {"chmod", "chown"} _UNKNOWN_RISK_COMMANDS = {"python", "python3"} _READONLY_GIT_SUBCOMMANDS = {"branch", "diff", "log", "rev-parse", "show", "status"} -_WRITE_GIT_SUBCOMMANDS = { - "add", - "apply", - "checkout", - "cherry-pick", - "clean", - "commit", - "merge", - "mv", - "rebase", - "reset", - "restore", - "revert", - "rm", - "stash", - "switch", - "tag", -} +_WRITE_GIT_SUBCOMMANDS = GIT_MUTATION_SUBCOMMANDS _NETWORK_GIT_SUBCOMMANDS = {"clone", "fetch", "pull", "push", "submodule"} _UNSAFE_GIT_OPTIONS = { "-C", diff --git a/src/foundation/services/planner.py b/src/foundation/services/planner.py index 2a30ed8..0c7c9e9 100644 --- a/src/foundation/services/planner.py +++ b/src/foundation/services/planner.py @@ -29,6 +29,7 @@ FileWriteRequest, ) from foundation.models.git import ( + GIT_MUTATION_SUBCOMMANDS, GitCommitRequest, GitDiffRequest, GitLogRequest, @@ -88,26 +89,7 @@ } ) _SHELL_MUTATION_COMMANDS = frozenset({*_RELATIVE_PATH_MUTATION_COMMANDS, "git"}) -_GIT_MUTATION_SUBCOMMANDS = frozenset( - { - "add", - "apply", - "checkout", - "cherry-pick", - "clean", - "commit", - "merge", - "mv", - "rebase", - "reset", - "restore", - "revert", - "rm", - "stash", - "switch", - "tag", - } -) +_GIT_MUTATION_SUBCOMMANDS = GIT_MUTATION_SUBCOMMANDS class PlanningError(RuntimeError): diff --git a/tests/test_policy.py b/tests/test_policy.py index f94f307..fcbe794 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -504,3 +504,12 @@ def test_auto_except_commit_gated_helper_passes_workspace_write() -> None: side_effects=["workspace_write"], ) assert _auto_except_commit_gated(request) is None + + +def test_git_mutation_subcommands_have_single_source_of_truth() -> None: + """Planner and guardrails must share one definition (hardening stage 3).""" + from foundation.models.git import GIT_MUTATION_SUBCOMMANDS + from foundation.services import guardrails, planner + + assert planner._GIT_MUTATION_SUBCOMMANDS is GIT_MUTATION_SUBCOMMANDS + assert guardrails._WRITE_GIT_SUBCOMMANDS is GIT_MUTATION_SUBCOMMANDS From 8270739584d496498ae1abe5f0df7a224ecbac7e Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Wed, 10 Jun 2026 17:49:44 -0700 Subject: [PATCH 05/18] Stage 4: make audit-trail failures visible Event sinks must never break a turn, but their failures were invisible: the observer swallowed sink exceptions, compose_event_sink warned per event forever on a flapping sink, and a crash escaping EventLogWriter.write_event left the sessions index claiming a complete log. Sink failures are now counted and warned about once, a sink is disabled after 3 consecutive failures, writer crashes poison the index row to write_truncated, and gap-phraser fallbacks log their reason with a category instead of failing silently. Co-Authored-By: Claude Fable 5 --- plans/fcli-hardening-roadmap.md | 13 +++ src/foundation/monitor/__init__.py | 35 ++++-- src/foundation/monitor/event_log.py | 47 +++++--- src/foundation/observability.py | 5 + src/foundation/services/gap_handoff.py | 30 +++-- src/foundation/services/observer.py | 48 +++++++- tests/test_gap_handoff.py | 26 +++++ tests/test_sink_failures.py | 156 +++++++++++++++++++++++++ 8 files changed, 322 insertions(+), 38 deletions(-) create mode 100644 tests/test_sink_failures.py diff --git a/plans/fcli-hardening-roadmap.md b/plans/fcli-hardening-roadmap.md index 337f328..554d4b6 100644 --- a/plans/fcli-hardening-roadmap.md +++ b/plans/fcli-hardening-roadmap.md @@ -185,6 +185,19 @@ miss what the planner emits. ## Stage 4: Audit-Trail Failures Become Visible +**Status: shipped 2026-06-10.** Scope notes vs the plan: `compose_event_sink` +(the production fanout) already logged per-sink failures at WARNING, so the +work focused on what was missing — failure counting and a disable-after-3 +breaker in both `ObserverService._dispatch_to_sink` and `compose_event_sink` +(shared constant `SINK_DISABLE_AFTER_CONSECUTIVE_FAILURES` in +`observability.py`), a catch-all in `EventLogWriter.write_event` that marks +the session `write_truncated` in `sessions.jsonl` when the writer itself +crashes, and WARNING-with-category logging for gap-phraser fallbacks +(provider-error / empty / json-or-fenced / plan-shaped). Degradation is +exposed via `ObserverService.sink_failure_count` / `.sink_disabled` for +future CLI notices; a stderr notice was not wired into the turn result (kept +out of scope to avoid touching the presenter mid-batch). + ### Goal The trace/event pipeline is the project's accountability story, but failures diff --git a/src/foundation/monitor/__init__.py b/src/foundation/monitor/__init__.py index c749824..affe368 100644 --- a/src/foundation/monitor/__init__.py +++ b/src/foundation/monitor/__init__.py @@ -22,6 +22,7 @@ TransportStartError, UnixSocketTransport, ) +from foundation.observability import SINK_DISABLE_AFTER_CONSECUTIVE_FAILURES EventSink = Callable[[str, Mapping[str, Any]], None] @@ -33,21 +34,39 @@ def compose_event_sink(*sinks: EventSink | None) -> EventSink: ``None`` entries are skipped. A sink raising an exception is logged at WARNING and the remaining sinks still receive the event — orchestration - must never break because of a misbehaving observer. + must never break because of a misbehaving observer. A sink that fails on + several consecutive events is disabled for the rest of the session (with + one final warning) so a flapping sink cannot flood the log. """ active: list[EventSink] = [sink for sink in sinks if sink is not None] + consecutive_failures = [0] * len(active) + disabled = [False] * len(active) def _fanout(event_name: str, payload: Mapping[str, Any]) -> None: - for sink in active: + for index, sink in enumerate(active): + if disabled[index]: + continue try: sink(event_name, payload) except Exception: # noqa: BLE001 - sink errors must not propagate - logger.warning( - "event_sink_failed event=%s sink=%r", - event_name, - sink, - exc_info=True, - ) + consecutive_failures[index] += 1 + if consecutive_failures[index] >= SINK_DISABLE_AFTER_CONSECUTIVE_FAILURES: + disabled[index] = True + logger.warning( + "event_sink_disabled sink=%r after %d consecutive failures", + sink, + consecutive_failures[index], + exc_info=True, + ) + else: + logger.warning( + "event_sink_failed event=%s sink=%r", + event_name, + sink, + exc_info=True, + ) + else: + consecutive_failures[index] = 0 return _fanout diff --git a/src/foundation/monitor/event_log.py b/src/foundation/monitor/event_log.py index 51cd972..c9ed32f 100644 --- a/src/foundation/monitor/event_log.py +++ b/src/foundation/monitor/event_log.py @@ -104,24 +104,35 @@ def events_dir(self) -> Path: def write_event(self, event_name: str, payload: Mapping[str, Any]) -> None: """Sink callback wired into ``ObserverService.event_sink``.""" - with self._lock: - if event_name == EVENT_USER_REQUEST and self._request_summary == "": - summary = payload.get("request_text") or "" - self._request_summary = str(summary)[:200] - if event_name == EVENT_SESSION_START: - session_id = self._coerce_session_id(payload) - if session_id is not None: - self._open_session( - session_id=session_id, - request_id=str(payload.get("request_id") or ""), - timestamp_payload=payload, - ) - envelope = build_envelope(event_name, payload) - self._write_envelope(envelope) - if event_name == EVENT_SESSION_END: - ended_at = envelope["ts"] - status = str(payload.get("status") or "completed") - self._close_session(ended_at=ended_at, status=status) + try: + with self._lock: + if event_name == EVENT_USER_REQUEST and self._request_summary == "": + summary = payload.get("request_text") or "" + self._request_summary = str(summary)[:200] + if event_name == EVENT_SESSION_START: + session_id = self._coerce_session_id(payload) + if session_id is not None: + self._open_session( + session_id=session_id, + request_id=str(payload.get("request_id") or ""), + timestamp_payload=payload, + ) + envelope = build_envelope(event_name, payload) + self._write_envelope(envelope) + if event_name == EVENT_SESSION_END: + ended_at = envelope["ts"] + status = str(payload.get("status") or "completed") + self._close_session(ended_at=ended_at, status=status) + except Exception: + # An unexpected bug must not break the turn, but the index row + # must not claim a complete event log either. + self._truncated = True + logger.warning( + "event_log_write_event_failed event=%s session_id=%s", + event_name, + self._session_id, + exc_info=True, + ) def close(self, *, status: str | None = None) -> None: """Force-close any open session (e.g. on KeyboardInterrupt).""" diff --git a/src/foundation/observability.py b/src/foundation/observability.py index 55eee62..0d3dfb7 100644 --- a/src/foundation/observability.py +++ b/src/foundation/observability.py @@ -19,6 +19,11 @@ STRUCTURED_LOG_SCHEMA_VERSION = "1.0.0" +# After this many consecutive failures an event sink is disabled for the rest +# of the session (with one final warning) so a flapping sink cannot spam logs. +# Shared by ObserverService and compose_event_sink. +SINK_DISABLE_AFTER_CONSECUTIVE_FAILURES = 3 + EVENT_SESSION_START = "session_start" EVENT_SESSION_END = "session_end" EVENT_USER_REQUEST = "user_request" diff --git a/src/foundation/services/gap_handoff.py b/src/foundation/services/gap_handoff.py index 73c86f0..f738585 100644 --- a/src/foundation/services/gap_handoff.py +++ b/src/foundation/services/gap_handoff.py @@ -295,12 +295,25 @@ def phrase( ) -> str | None: try: response = provider.complete(_build_phrasing_prompt(kind, request, detail, fallback)) - except ProviderError: + except ProviderError as exc: + logger.warning( + "gap-message phrasing failed (provider error: %s); using deterministic fallback", + exc, + ) return None except Exception: # pragma: no cover - defensive on the recovery path - logger.debug("gap-message phrasing failed", exc_info=True) + logger.warning( + "gap-message phrasing failed; using deterministic fallback", + exc_info=True, + ) return None - return _sanitize_phrased_message(response.content) + sanitized, rejection = _sanitize_phrased_message(response.content) + if rejection is not None: + logger.warning( + "gap-message phrasing rejected (%s); using deterministic fallback", + rejection, + ) + return sanitized return phrase @@ -335,19 +348,20 @@ def _build_phrasing_prompt( ) -def _sanitize_phrased_message(content: str | None) -> str | None: +def _sanitize_phrased_message(content: str | None) -> tuple[str | None, str | None]: + """Return ``(sanitized_text, rejection_category)``; exactly one is None.""" text = (content or "").strip() if not text: - return None + return None, "empty" # Reject anything that looks like a JSON plan or fenced code rather than prose. if text[0] in "{[" or text.startswith("```"): - return None + return None, "json-or-fenced" if '"actions"' in text or '"assistant_message"' in text: - return None + return None, "plan-shaped" text = " ".join(text.split()) if len(text) > _MAX_PHRASED_CHARS: text = text[:_MAX_PHRASED_CHARS].rsplit(" ", 1)[0].rstrip() + "…" - return text + return text, None def build_issue_body(report: CapabilityGapReport) -> str: diff --git a/src/foundation/services/observer.py b/src/foundation/services/observer.py index 10dfcf2..76c8df3 100644 --- a/src/foundation/services/observer.py +++ b/src/foundation/services/observer.py @@ -28,7 +28,12 @@ TraceEdgeKind, ) from foundation.models.trace import ExecutionStep, PlanningStep -from foundation.observability import emit_event, emit_exception, redact_payload +from foundation.observability import ( + SINK_DISABLE_AFTER_CONSECUTIVE_FAILURES, + emit_event, + emit_exception, + redact_payload, +) from foundation.services.capabilities import SHELL_CAPABILITY_ID, CapabilityRegistry from foundation.services.guardrails import POLICY_SNAPSHOT_VERSION from foundation.services.history import HistoryStore @@ -51,18 +56,53 @@ def __init__( self._history_store = history_store self._capability_registry = capability_registry self._event_sink: EventSink | None = event_sink + self._sink_failure_count = 0 + self._sink_consecutive_failures = 0 + self._sink_disabled = False def set_event_sink(self, event_sink: EventSink | None) -> None: """Replace the event sink callback (or clear it with ``None``).""" self._event_sink = event_sink + self._sink_consecutive_failures = 0 + self._sink_disabled = False + + @property + def sink_failure_count(self) -> int: + """Total sink failures suppressed so far (for surfacing degradation).""" + return self._sink_failure_count + + @property + def sink_disabled(self) -> bool: + """Whether the sink was disabled after repeated consecutive failures.""" + return self._sink_disabled def _dispatch_to_sink(self, event_name: str, payload: Mapping[str, Any]) -> None: - if self._event_sink is None: + if self._event_sink is None or self._sink_disabled: return try: self._event_sink(event_name, payload) - except Exception: # pragma: no cover - sink errors must not break runtime - logger.exception("event_sink raised; suppressing", extra={"event": event_name}) + except Exception: + # A sink failure must never break the turn, but it must not be + # silent either: events are the audit surface. + self._sink_failure_count += 1 + self._sink_consecutive_failures += 1 + if self._sink_consecutive_failures >= SINK_DISABLE_AFTER_CONSECUTIVE_FAILURES: + self._sink_disabled = True + logger.warning( + "event sink disabled after %d consecutive failures; further " + "events will not reach monitor surfaces (event=%s)", + self._sink_consecutive_failures, + event_name, + exc_info=True, + ) + else: + logger.warning( + "event sink failed; monitor surfaces may be missing events (event=%s)", + event_name, + exc_info=True, + ) + else: + self._sink_consecutive_failures = 0 def emit( self, diff --git a/tests/test_gap_handoff.py b/tests/test_gap_handoff.py index 081264f..a558311 100644 --- a/tests/test_gap_handoff.py +++ b/tests/test_gap_handoff.py @@ -259,3 +259,29 @@ def test_write_gap_report_round_trips(tmp_path) -> None: # Deterministic id: a second write of the same report reuses the same file. again = write_gap_report(handoff.report, gaps_dir=tmp_path / "gaps") assert again == path + + +def test_make_provider_phraser_logs_provider_error_at_warning( + caplog, +) -> None: + """Hardening stage 4: a phrasing fallback must say why it happened.""" + import logging + + phraser = make_provider_phraser(_StubProvider(error=True)) + with caplog.at_level(logging.WARNING, logger="foundation.services.gap_handoff"): + assert phraser(CapabilityGapKind.STUCK_NO_PROGRESS, "r", "", "fallback") is None + assert any("phrasing" in record.message for record in caplog.records) + + +def test_make_provider_phraser_logs_rejection_category( + caplog, +) -> None: + """Hardening stage 4: sanitizer rejections are logged with a category.""" + import logging + + provider = _StubProvider(body='{"assistant_message": "Done.", "actions": []}') + phraser = make_provider_phraser(provider) + with caplog.at_level(logging.WARNING, logger="foundation.services.gap_handoff"): + assert phraser(CapabilityGapKind.MISSING_CAPABILITY, "r", "d", "fallback") is None + messages = [record.message for record in caplog.records] + assert any("rejected" in message for message in messages) diff --git a/tests/test_sink_failures.py b/tests/test_sink_failures.py new file mode 100644 index 0000000..dc5abe1 --- /dev/null +++ b/tests/test_sink_failures.py @@ -0,0 +1,156 @@ +"""Event-sink failure visibility and circuit-breaking (hardening stage 4). + +A failing sink must never break the turn, but it must not fail silently +either: failures are counted and warned about, a flapping sink is disabled +after three consecutive failures, and a crash inside the event-log writer +marks the session's index row as truncated instead of claiming a complete +log. +""" + +from __future__ import annotations + +import json +import logging +from pathlib import Path +from typing import Any + +import pytest + +from foundation.monitor import compose_event_sink +from foundation.monitor.event_log import EventLogWriter +from foundation.observability import EVENT_SESSION_END, EVENT_SESSION_START +from foundation.services.observer import ObserverService + + +def _observer(event_sink: Any) -> ObserverService: + return ObserverService( + history_store=None, + capability_registry=None, # type: ignore[arg-type] + event_sink=event_sink, + ) + + +class TestObserverSinkBreaker: + def test_single_failure_warns_and_keeps_sink_enabled( + self, caplog: pytest.LogCaptureFixture + ) -> None: + calls: list[str] = [] + + def flaky(event_name: str, payload: Any) -> None: + calls.append(event_name) + if len(calls) == 1: + raise RuntimeError("boom") + + observer = _observer(flaky) + with caplog.at_level(logging.WARNING): + observer.emit("event_one", payload={}) + observer.emit("event_two", payload={}) + + assert len(calls) == 2 + assert observer.sink_failure_count == 1 + assert not observer.sink_disabled + assert any("sink" in record.message for record in caplog.records) + + def test_disabled_after_three_consecutive_failures( + self, caplog: pytest.LogCaptureFixture + ) -> None: + calls: list[str] = [] + + def broken(event_name: str, payload: Any) -> None: + calls.append(event_name) + raise RuntimeError("boom") + + observer = _observer(broken) + with caplog.at_level(logging.WARNING): + for index in range(5): + observer.emit(f"event_{index}", payload={}) + + assert observer.sink_disabled + assert len(calls) == 3 + assert observer.sink_failure_count == 3 + disabled_warnings = [r for r in caplog.records if "disabled" in r.message] + assert len(disabled_warnings) == 1 + + def test_success_resets_consecutive_count(self) -> None: + outcomes = iter([True, True, False, True, True, False]) + + def sometimes(event_name: str, payload: Any) -> None: + if next(outcomes): + raise RuntimeError("boom") + + observer = _observer(sometimes) + for index in range(6): + observer.emit(f"event_{index}", payload={}) + + assert not observer.sink_disabled + assert observer.sink_failure_count == 4 + + def test_replacing_sink_resets_breaker(self) -> None: + def broken(event_name: str, payload: Any) -> None: + raise RuntimeError("boom") + + observer = _observer(broken) + for index in range(3): + observer.emit(f"event_{index}", payload={}) + assert observer.sink_disabled + + replacement_calls: list[str] = [] + observer.set_event_sink(lambda event_name, payload: replacement_calls.append(event_name)) + observer.emit("after_replacement", payload={}) + assert not observer.sink_disabled + assert replacement_calls == ["after_replacement"] + + +class TestComposedSinkBreaker: + def test_flapping_sink_disabled_but_others_keep_receiving( + self, caplog: pytest.LogCaptureFixture + ) -> None: + bad_calls: list[str] = [] + good_calls: list[str] = [] + + def bad(event_name: str, payload: Any) -> None: + bad_calls.append(event_name) + raise RuntimeError("boom") + + def good(event_name: str, payload: Any) -> None: + good_calls.append(event_name) + + fanout = compose_event_sink(bad, good) + with caplog.at_level(logging.WARNING): + for index in range(5): + fanout(f"event_{index}", {}) + + assert len(bad_calls) == 3 + assert len(good_calls) == 5 + disabled_warnings = [ + r for r in caplog.records if r.message.startswith("event_sink_disabled") + ] + assert len(disabled_warnings) == 1 + + +class TestEventLogWriterDegradation: + def test_sink_crash_marks_index_row_truncated( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + writer = EventLogWriter(events_dir=tmp_path, install_signal_handlers=False) + writer.write_event( + EVENT_SESSION_START, + {"session_id": "s1", "request_id": "r1"}, + ) + + def boom(event_name: str, payload: Any) -> Any: + raise RuntimeError("envelope bug") + + monkeypatch.setattr("foundation.monitor.event_log.build_envelope", boom) + # Must not raise out of the sink, and must poison the session status. + writer.write_event("some_event", {"session_id": "s1"}) + monkeypatch.undo() + + writer.write_event( + EVENT_SESSION_END, + {"session_id": "s1", "status": "completed"}, + ) + index_lines = (tmp_path / "sessions.jsonl").read_text().splitlines() + rows = [json.loads(line) for line in index_lines if line.strip()] + assert rows[-1]["session_id"] == "s1" + assert rows[-1]["status"] == "write_truncated" From ec56d6e17571d6548d3ecec504696a27d87084f3 Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Wed, 10 Jun 2026 17:55:34 -0700 Subject: [PATCH 06/18] Stage 5: migration safety rails for the history store Schema migrations ran automatically against the user's real history DB with no backup and no post-check, and the v6 executescript rebuild dropped the source table without validating the copy. Migrations now back up the DB file first (keeping the newest backup), the v6 rebuild validates row counts before dropping anything, and failures raise HistoryMigrationError naming the backup path with the original data intact. Co-Authored-By: Claude Fable 5 --- plans/fcli-hardening-roadmap.md | 10 +++ src/foundation/services/history.py | 115 +++++++++++++++++++++++------ tests/test_history.py | 100 +++++++++++++++++++++++++ 3 files changed, 201 insertions(+), 24 deletions(-) diff --git a/plans/fcli-hardening-roadmap.md b/plans/fcli-hardening-roadmap.md index 554d4b6..e3f6820 100644 --- a/plans/fcli-hardening-roadmap.md +++ b/plans/fcli-hardening-roadmap.md @@ -238,6 +238,16 @@ phrasing fails. Keep the "never break the turn" property; lose the silence. ## Stage 5: Migration Safety Rails +**Status: shipped 2026-06-10.** Pre-migration file backup +(`.pre-v.bak`, newest kept, older rotated out), the v6 rebuild +now validates row counts and raises `HistoryMigrationError` (naming the +backup) *before* dropping the source table, and any sqlite error during the +chain rolls back and re-raises as `HistoryMigrationError`. One deliberate +deviation: the rebuild was switched from `executescript` (which has implicit +commit semantics) to individual `execute()` calls so validation happens +inside the transaction — the validate-before-drop ordering, not the +transaction, is the real guarantee that history survives. + ### Goal `history.py:1382` (`_migrate_to_v6`) rebuilds `assistant_plans` to change a diff --git a/src/foundation/services/history.py b/src/foundation/services/history.py index f825def..bcabad4 100644 --- a/src/foundation/services/history.py +++ b/src/foundation/services/history.py @@ -4,6 +4,7 @@ import json import logging +import shutil import sqlite3 from datetime import UTC, datetime, timedelta from pathlib import Path @@ -39,6 +40,17 @@ _SCHEMA_VERSION = 6 _DEFAULT_MAX_BLOB_BYTES = 64 * 1024 + +class HistoryMigrationError(RuntimeError): + """A schema migration failed; the original database was left untouched.""" + + +def _backup_hint(backup_path: Path | None) -> str: + if backup_path is None: + return "" + return f" Pre-migration backup: {backup_path}" + + _SCHEMA_SQL = """ PRAGMA foreign_keys = ON; @@ -1340,16 +1352,44 @@ def _connect(self) -> sqlite3.Connection: return connection def _ensure_schema(self) -> None: - with self._connect() as connection: - current_version = connection.execute("PRAGMA user_version").fetchone()[0] - connection.executescript(_SCHEMA_SQL) - if current_version < 4: - self._migrate_to_v4(connection) - if current_version < 5: - self._migrate_to_v5(connection) - if current_version < 6: - self._migrate_to_v6(connection) - connection.execute(f"PRAGMA user_version = {_SCHEMA_VERSION}") + probe = self._connect() + try: + current_version = probe.execute("PRAGMA user_version").fetchone()[0] + finally: + probe.close() + backup_path: Path | None = None + if current_version < _SCHEMA_VERSION: + backup_path = self._backup_before_migration() + try: + with self._connect() as connection: + connection.executescript(_SCHEMA_SQL) + if current_version < 4: + self._migrate_to_v4(connection) + if current_version < 5: + self._migrate_to_v5(connection) + if current_version < 6: + self._migrate_to_v6(connection, backup_path=backup_path) + connection.execute(f"PRAGMA user_version = {_SCHEMA_VERSION}") + except HistoryMigrationError: + raise + except sqlite3.Error as exc: + raise HistoryMigrationError( + f"History schema migration to v{_SCHEMA_VERSION} failed and was " + f"rolled back: {exc}.{_backup_hint(backup_path)}" + ) from exc + + def _backup_before_migration(self) -> Path | None: + """Copy the DB file aside before migrating; keep only the newest backup.""" + if not self._database_path.exists() or self._database_path.stat().st_size == 0: + return None + backup_path = self._database_path.with_name( + f"{self._database_path.name}.pre-v{_SCHEMA_VERSION}.bak" + ) + shutil.copy2(self._database_path, backup_path) + for stale in self._database_path.parent.glob(f"{self._database_path.name}.pre-v*.bak"): + if stale != backup_path: + stale.unlink(missing_ok=True) + return backup_path @staticmethod def _migrate_to_v4(connection: sqlite3.Connection) -> None: @@ -1379,7 +1419,11 @@ def _migrate_to_v5(connection: sqlite3.Connection) -> None: ) @staticmethod - def _migrate_to_v6(connection: sqlite3.Connection) -> None: + def _migrate_to_v6( + connection: sqlite3.Connection, + *, + backup_path: Path | None = None, + ) -> None: """Migrate v5 → v6: ensure ``assistant_plans`` is keyed per iteration. Pre-v6 databases that were upgraded from v3 may carry the older @@ -1388,6 +1432,11 @@ def _migrate_to_v6(connection: sqlite3.Connection) -> None: latest iteration's plan was preserved per session. v6 rebuilds the table with ``UNIQUE(session_id, iteration)`` so per-iteration plans are inspectable for the first time. + + Rebuild-style migrations must validate the copy before dropping the + source table (see this method for the pattern): the original table is + only dropped after the rebuilt row count matches, so a failure can + never destroy history. """ # Cheapest probe: try to insert a duplicate sentinel pair under the # same session_id with a different iteration. If the existing schema @@ -1411,7 +1460,9 @@ def _migrate_to_v6(connection: sqlite3.Connection) -> None: if already_correct: return - connection.executescript( + source_count = connection.execute("SELECT COUNT(*) FROM assistant_plans").fetchone()[0] + connection.execute("DROP TABLE IF EXISTS assistant_plans_new") + connection.execute( """ CREATE TABLE assistant_plans_new ( id INTEGER PRIMARY KEY AUTOINCREMENT, @@ -1423,21 +1474,37 @@ def _migrate_to_v6(connection: sqlite3.Connection) -> None: planning_metadata_json TEXT NOT NULL, created_at TEXT NOT NULL, UNIQUE(session_id, iteration) - ); - - INSERT INTO assistant_plans_new ( - id, session_id, iteration, assistant_message, - context_json, plan_json, planning_metadata_json, created_at ) - SELECT - id, session_id, iteration, assistant_message, - context_json, plan_json, planning_metadata_json, created_at - FROM assistant_plans; - - DROP TABLE assistant_plans; - ALTER TABLE assistant_plans_new RENAME TO assistant_plans; """ ) + try: + connection.execute( + """ + INSERT INTO assistant_plans_new ( + id, session_id, iteration, assistant_message, + context_json, plan_json, planning_metadata_json, created_at + ) + SELECT + id, session_id, iteration, assistant_message, + context_json, plan_json, planning_metadata_json, created_at + FROM assistant_plans + """ + ) + except sqlite3.IntegrityError as exc: + raise HistoryMigrationError( + "History migration to v6 failed: assistant_plans contains rows " + "that violate UNIQUE(session_id, iteration). The original table " + f"was left untouched.{_backup_hint(backup_path)}" + ) from exc + rebuilt_count = connection.execute("SELECT COUNT(*) FROM assistant_plans_new").fetchone()[0] + if rebuilt_count != source_count: + raise HistoryMigrationError( + f"History migration to v6 failed: rebuilt assistant_plans has " + f"{rebuilt_count} rows but the source has {source_count}. The " + f"original table was left untouched.{_backup_hint(backup_path)}" + ) + connection.execute("DROP TABLE assistant_plans") + connection.execute("ALTER TABLE assistant_plans_new RENAME TO assistant_plans") def _encode_json_blob(self, payload: object) -> str: raw = _json_dumps(payload) diff --git a/tests/test_history.py b/tests/test_history.py index 474b2b2..c2e4bd3 100644 --- a/tests/test_history.py +++ b/tests/test_history.py @@ -248,3 +248,103 @@ def test_schema_v6_migration_keys_assistant_plans_per_iteration( assert [row["iteration"] for row in rows] == [1, 2] finally: connection.close() + + +def _build_v5_database(database_path: Path, *, duplicate_iteration_rows: bool = False) -> None: + """Create a v5-shaped DB with the legacy assistant_plans constraint. + + With ``duplicate_iteration_rows`` the table is built without any unique + constraint and seeded with two rows sharing (session_id, iteration) — a + corrupt shape the v6 rebuild must refuse to destroy (hardening stage 5). + """ + constraint = "" if duplicate_iteration_rows else ", UNIQUE(session_id)" + connection = sqlite3.connect(database_path) + try: + connection.executescript( + f""" + CREATE TABLE sessions ( + id TEXT PRIMARY KEY, + kind TEXT NOT NULL, + status TEXT NOT NULL, + workspace_root TEXT NOT NULL, + request_cwd TEXT NOT NULL, + approval_mode TEXT NOT NULL, + plan_only INTEGER NOT NULL DEFAULT 0, + command_preview TEXT, + started_at TEXT NOT NULL, + ended_at TEXT + ); + CREATE TABLE assistant_plans ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + session_id TEXT NOT NULL REFERENCES sessions(id) ON DELETE CASCADE, + iteration INTEGER NOT NULL DEFAULT 1, + assistant_message TEXT NOT NULL, + context_json TEXT, + plan_json TEXT NOT NULL, + planning_metadata_json TEXT NOT NULL, + created_at TEXT NOT NULL{constraint} + ); + INSERT INTO sessions (id, kind, status, workspace_root, request_cwd, + approval_mode, started_at) + VALUES ('sess-legacy', 'chat', 'completed', '/ws', '/ws', 'prompt', + '2026-01-01T00:00:00Z'); + INSERT INTO assistant_plans ( + session_id, iteration, assistant_message, plan_json, + planning_metadata_json, created_at + ) VALUES ( + 'sess-legacy', 1, 'iter-1', '{{}}', '{{}}', '2026-01-01T00:00:00Z' + ); + """ + ) + if duplicate_iteration_rows: + connection.execute( + "INSERT INTO assistant_plans (session_id, iteration, " + "assistant_message, plan_json, planning_metadata_json, created_at) " + "VALUES ('sess-legacy', 1, 'iter-1-dup', '{}', '{}', " + "'2026-01-01T00:00:01Z')" + ) + connection.execute("PRAGMA user_version = 5") + connection.commit() + finally: + connection.close() + + +def test_migration_writes_backup_before_running(tmp_path: Path) -> None: + """Hardening stage 5: a schema migration backs up the DB file first.""" + database_path = tmp_path / "history.sqlite3" + _build_v5_database(database_path) + + HistoryStore(database_path=database_path) + + backup_path = tmp_path / "history.sqlite3.pre-v6.bak" + assert backup_path.exists() + backup = sqlite3.connect(backup_path) + try: + assert backup.execute("PRAGMA user_version").fetchone()[0] == 5 + count = backup.execute("SELECT COUNT(*) FROM assistant_plans").fetchone()[0] + assert count == 1 + finally: + backup.close() + + +def test_sabotaged_v6_rebuild_raises_and_preserves_original(tmp_path: Path) -> None: + """Hardening stage 5: a failing rebuild must not destroy history.""" + import pytest + + from foundation.services.history import HistoryMigrationError + + database_path = tmp_path / "history.sqlite3" + _build_v5_database(database_path, duplicate_iteration_rows=True) + + with pytest.raises(HistoryMigrationError, match="pre-v6.bak"): + HistoryStore(database_path=database_path) + + # Original database untouched and readable at the old version. + connection = sqlite3.connect(database_path) + try: + assert connection.execute("PRAGMA user_version").fetchone()[0] == 5 + count = connection.execute("SELECT COUNT(*) FROM assistant_plans").fetchone()[0] + assert count == 2 + finally: + connection.close() + assert (tmp_path / "history.sqlite3.pre-v6.bak").exists() From 7ccc613a97b3296690e93e5cae08551dfe2198d5 Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Wed, 10 Jun 2026 18:03:04 -0700 Subject: [PATCH 07/18] Stage 6: bound and surface diff-applier leniency The unified-diff applier silently tolerated bare context lines and always compared with trailing newlines stripped, and never checked a hunk's declared source count against its body. Declared-count mismatches and no-op hunks are now parse-time DIFF_REJECTED errors; bare-context and newline-normalized matching remain as deliberate tolerances but are reported through FileMutationResult.leniency_notes so they land in the trace. Co-Authored-By: Claude Fable 5 --- plans/fcli-hardening-roadmap.md | 8 +++ src/foundation/models/file.py | 3 + src/foundation/services/file_service.py | 84 ++++++++++++++++++++++--- tests/test_file_service.py | 58 +++++++++++++++++ 4 files changed, 143 insertions(+), 10 deletions(-) diff --git a/plans/fcli-hardening-roadmap.md b/plans/fcli-hardening-roadmap.md index e3f6820..2fb3439 100644 --- a/plans/fcli-hardening-roadmap.md +++ b/plans/fcli-hardening-roadmap.md @@ -283,6 +283,14 @@ every future migration. ## Stage 6: Diff Applier Strictness Decision +**Status: shipped 2026-06-10.** Decisions made: bare context lines kept +(counted and reported), newline normalization demoted to a fallback after an +exact match fails (reported when used), declared-count mismatches and no-op +hunks rejected at parse time with `DIFF_REJECTED`. Tolerances surface via a +new `FileMutationResult.leniency_notes` field, which flows into the trace +artifact automatically; the contract is documented on +`_parse_and_apply_diff`'s docstring. + ### Goal The unified-diff applier (`file_service.py` ~118–257) is lenient: bare lines diff --git a/src/foundation/models/file.py b/src/foundation/models/file.py index bf0529e..0b233d2 100644 --- a/src/foundation/models/file.py +++ b/src/foundation/models/file.py @@ -154,3 +154,6 @@ class FileMutationResult(StrictModel): size_bytes: int = Field(ge=0) diff_summary: str created: bool = False + # Tolerances the diff applier exercised (bare context lines, newline + # normalization); empty when the diff applied exactly. + leniency_notes: list[str] = Field(default_factory=list) diff --git a/src/foundation/services/file_service.py b/src/foundation/services/file_service.py index 08d1c58..4d22c5e 100644 --- a/src/foundation/services/file_service.py +++ b/src/foundation/services/file_service.py @@ -115,9 +115,27 @@ def _diff_summary(old_content: str | None, new_content: str) -> str: # --------------------------------------------------------------------------- -def _parse_and_apply_diff(original: str, diff_text: str, *, file_path: str) -> str: +def _norm_line(line: str) -> str: + return line.rstrip("\n\r") + + +def _parse_and_apply_diff( + original: str, diff_text: str, *, file_path: str +) -> tuple[str, list[str]]: """Parse a unified diff and apply it atomically to *original*. + Returns ``(new_content, leniency_notes)``. The notes name every tolerance + that was exercised so callers can surface them in the execution artifact. + + Deliberate tolerances for model-generated diffs (each recorded when used): + - body lines without a leading ``+``/``-``/space are treated as context; + - a hunk that only matches after trailing-newline normalization (CRLF vs + LF) is accepted as a fallback when the exact match fails. + + Never-valid shapes are rejected at parse time: hunks whose declared + source-line count disagrees with their body, hunks containing no + additions or removals, rename-style and delete-only diffs. + Raises FileServiceError on malformed diffs, context mismatches, or policy violations (delete-only, rename-style). """ @@ -183,6 +201,43 @@ def _parse_and_apply_diff(original: str, diff_text: str, *, file_path: str) -> s path=file_path, ) + # Parse-time validation: each hunk's body must agree with its declared + # source-line count and actually change something. + leniency_notes: list[str] = [] + for hunk_idx, (_old_start, old_count, hunk_lines) in enumerate(hunks): + old_side_lines = 0 + has_change = False + bare_lines = 0 + for hl in hunk_lines: + if hl.startswith("+"): + has_change = True + elif hl.startswith("-"): + has_change = True + old_side_lines += 1 + elif hl.startswith(" "): + old_side_lines += 1 + else: + bare_lines += 1 + old_side_lines += 1 + if not has_change: + _raise( + FileErrorCode.DIFF_REJECTED, + f"Hunk {hunk_idx + 1} contains no additions or removals.", + path=file_path, + ) + if old_side_lines != old_count: + _raise( + FileErrorCode.DIFF_REJECTED, + f"Hunk {hunk_idx + 1} declares {old_count} source lines " + f"but its body has {old_side_lines}.", + path=file_path, + ) + if bare_lines: + leniency_notes.append( + f"hunk {hunk_idx + 1}: {bare_lines} line(s) without a diff " + "prefix treated as context" + ) + # Reject delete-only diffs (all hunks contain only removals, no additions) has_addition = False for _, _, hunk_lines in hunks: @@ -221,18 +276,26 @@ def _parse_and_apply_diff(original: str, diff_text: str, *, file_path: str) -> s src_start = old_start - 1 src_slice = original_lines[src_start : src_start + len(expected)] - # Normalise trailing newlines for comparison - def _norm(s: str) -> str: - return s.rstrip("\n\r") - - if len(src_slice) != len(expected) or any( - _norm(a) != _norm(b) for a, b in zip(src_slice, expected, strict=True) - ): + if len(src_slice) != len(expected): _raise( FileErrorCode.DIFF_APPLY_FAILED, f"Hunk {hunk_idx + 1} does not match the source file at line {old_start}.", path=file_path, ) + if all(a == b for a, b in zip(src_slice, expected, strict=True)): + continue + # Fallback: accept the hunk when only trailing newlines (CRLF vs LF, + # missing final newline) differ — but say so. + if all(_norm_line(a) == _norm_line(b) for a, b in zip(src_slice, expected, strict=True)): + leniency_notes.append( + f"hunk {hunk_idx + 1} matched only after trailing-newline normalization" + ) + continue + _raise( + FileErrorCode.DIFF_APPLY_FAILED, + f"Hunk {hunk_idx + 1} does not match the source file at line {old_start}.", + path=file_path, + ) # Apply hunks in reverse order to preserve line indices result_lines = list(original_lines) @@ -254,7 +317,7 @@ def _norm(s: str) -> str: src_start = old_start - 1 result_lines[src_start : src_start + remove_count] = new_lines - return "".join(result_lines) + return "".join(result_lines), leniency_notes # --------------------------------------------------------------------------- @@ -510,7 +573,7 @@ def apply_diff(self, request: FileApplyDiffRequest) -> FileMutationResult: if not resolved.exists(): _raise_not_found(request.path, resolved) old_content, _ = self._read_raw(resolved) - new_content = _parse_and_apply_diff( + new_content, leniency_notes = _parse_and_apply_diff( old_content, request.diff, file_path=request.path, @@ -522,4 +585,5 @@ def apply_diff(self, request: FileApplyDiffRequest) -> FileMutationResult: line_count=_line_count(new_content), size_bytes=len(new_content.encode("utf-8")), diff_summary=_diff_summary(old_content, new_content), + leniency_notes=leniency_notes, ) diff --git a/tests/test_file_service.py b/tests/test_file_service.py index b038065..e67b542 100644 --- a/tests/test_file_service.py +++ b/tests/test_file_service.py @@ -743,3 +743,61 @@ def test_read_not_found_lists_sibling_files(tmp_path: Path) -> None: # The error names the real sibling so the model can self-correct. assert "anmolnoor-github-report.md" in exc.value.error.message assert exc.value.error.suggestion is not None + + +# =================================================================== +# file.apply_diff — strictness contract (hardening stage 6) +# =================================================================== + + +class TestFileApplyDiffStrictness: + def test_declared_count_mismatch_rejected_at_parse_time(self, tmp_path: Path) -> None: + svc, ws = _make_service(tmp_path) + (ws / "file.txt").write_text("a\nb\nc\n", encoding="utf-8") + + # Header declares 3 source lines; body carries only 2. + diff = "@@ -1,3 +1,3 @@\n a\n-b\n+B\n" + with pytest.raises(FileServiceError) as exc_info: + svc.apply_diff(FileApplyDiffRequest(path="file.txt", diff=diff)) + assert exc_info.value.error.code == FileErrorCode.DIFF_REJECTED + assert "declares" in exc_info.value.error.message + + def test_noop_hunk_rejected(self, tmp_path: Path) -> None: + svc, ws = _make_service(tmp_path) + (ws / "file.txt").write_text("a\nb\nc\nd\n", encoding="utf-8") + + diff = "@@ -1,2 +1,2 @@\n a\n-b\n+B\n@@ -3,2 +3,2 @@\n c\n d\n" + with pytest.raises(FileServiceError) as exc_info: + svc.apply_diff(FileApplyDiffRequest(path="file.txt", diff=diff)) + assert exc_info.value.error.code == FileErrorCode.DIFF_REJECTED + assert "no additions or removals" in exc_info.value.error.message + + def test_crlf_file_with_lf_diff_applies_and_reports_normalization(self, tmp_path: Path) -> None: + svc, ws = _make_service(tmp_path) + (ws / "file.txt").write_bytes(b"a\r\nb\r\nc\r\n") + + diff = "@@ -1,3 +1,3 @@\n a\n-b\n+B\n c\n" + result = svc.apply_diff(FileApplyDiffRequest(path="file.txt", diff=diff)) + + assert "B" in (ws / "file.txt").read_text(encoding="utf-8") + assert any("normaliz" in note for note in result.leniency_notes) + + def test_bare_context_lines_apply_and_are_reported(self, tmp_path: Path) -> None: + svc, ws = _make_service(tmp_path) + (ws / "file.txt").write_text("a\nb\nc\n", encoding="utf-8") + + # "a" and "c" lack the leading space a strict unified diff requires. + diff = "@@ -1,3 +1,3 @@\na\n-b\n+B\nc\n" + result = svc.apply_diff(FileApplyDiffRequest(path="file.txt", diff=diff)) + + assert (ws / "file.txt").read_text(encoding="utf-8") == "a\nB\nc\n" + assert any("without a diff prefix" in note for note in result.leniency_notes) + + def test_exact_match_produces_no_leniency_notes(self, tmp_path: Path) -> None: + svc, ws = _make_service(tmp_path) + (ws / "file.txt").write_text("a\nb\nc\n", encoding="utf-8") + + diff = "@@ -1,3 +1,3 @@\n a\n-b\n+B\n c\n" + result = svc.apply_diff(FileApplyDiffRequest(path="file.txt", diff=diff)) + + assert result.leniency_notes == [] From b57b9130da429adcc48e25a4a28772577581491a Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Wed, 10 Jun 2026 18:14:21 -0700 Subject: [PATCH 08/18] Stage 7: add planner, Codex failure-path, and live-rendering tests 41 new tests where coverage was thinnest: PlannerService unit tests (prompt injection, validation, repair retries), Codex adapter failure paths mapped to ProviderErrorCodes, and live renderer edge cases (narrow widths, spinner identity, markup/ANSI payloads). Co-Authored-By: Claude Fable 5 --- plans/fcli-hardening-roadmap.md | 17 + tests/test_live_turn.py | 144 ++++++- tests/test_planner.py | 718 ++++++++++++++++++++++++++++++++ tests/test_provider.py | 196 +++++++++ 4 files changed, 1073 insertions(+), 2 deletions(-) create mode 100644 tests/test_planner.py diff --git a/plans/fcli-hardening-roadmap.md b/plans/fcli-hardening-roadmap.md index 2fb3439..a416e7c 100644 --- a/plans/fcli-hardening-roadmap.md +++ b/plans/fcli-hardening-roadmap.md @@ -325,6 +325,23 @@ then enforce it at parse time. ## Stage 7: Test Depth Where It Is Thin +**Status: shipped 2026-06-10.** 41 new tests: `tests/test_planner.py` (21 +isolated PlannerService cases: observation/iteration prompt injection, +endpoint validation, repair retries for missing/invalid/truncated output, +commit-intent zero-action guard, shell guards, deferred-write rules, +preflight reject), 10 Codex failure-path cases in `tests/test_provider.py` +(missing binary, timeout, auth stderr, usage-limit, nonzero-exit stderr +preservation, malformed/empty output → ProviderErrorCode mapping), 10 live +rendering edge cases in `tests/test_live_turn.py` (narrow widths, spinner +identity across refreshes, markup-literal activity lines, ANSI payloads, +empty payloads). The sweep found two real bugs, fixed separately right after +this stage: a hallucinated capability id escaped the plan-repair loop as an +unwrapped `ValueError`, and `render_detail_panel` parsed user request text +as Rich markup (crash + styling injection). Known oddities documented in +tests, not changed: Codex `rc>=2 → retryable` heuristic; stale banners were +deliberately removed in #12, so `render_status_line`'s `now` parameter is +vestigial. + ### Goal Coverage is strong on the orchestrator and providers but thin exactly where diff --git a/tests/test_live_turn.py b/tests/test_live_turn.py index cf5abe2..6ad0733 100644 --- a/tests/test_live_turn.py +++ b/tests/test_live_turn.py @@ -6,6 +6,7 @@ from typing import Any from rich.console import Console +from rich.spinner import Spinner from foundation.live_turn import ( LivePhase, @@ -195,8 +196,8 @@ def test_state_reducer_approval_pending_path(): assert state.approval_summary is None -def _render_to_text(renderable) -> str: - console = Console(file=io.StringIO(), force_terminal=False, width=80, record=True) +def _render_to_text(renderable, *, width: int = 80) -> str: + console = Console(file=io.StringIO(), force_terminal=False, width=width, record=True) console.print(renderable) return console.export_text() @@ -349,3 +350,142 @@ def test_renderer_pause_resume_safe_when_unmounted(tmp_path): # Both should be no-ops when the Live widget hasn't been entered. renderer.pause() renderer.resume() + + +# --- Stage 7 hardening: renderer edge cases ------------------------------ + + +def test_render_status_line_narrow_width_stays_single_line(): + cases = [ + TurnLiveState(), # Starting + TurnLiveState(phase=LivePhase.THINKING), + TurnLiveState(current_action_id="a1", current_action_tool="foundation.file.read"), + TurnLiveState(finished=True, final_status="completed"), + ] + for state in cases: + text = _render_to_text(render_status_line(state, elapsed_seconds=2.0), width=20) + lines = [line for line in text.splitlines() if line.strip()] + assert len(lines) == 1 + assert len(lines[0].rstrip()) <= 20 + + +def test_render_collapsed_narrow_width_renders_without_crash(): + state = TurnLiveState() + state.fold( + EVENT_SHELL_EXECUTION_STARTED, + { + "action_id": "test", + "command_preview": "pytest tests/test_live_turn.py -q --maxfail=1 -k narrow", + }, + ) + text = _render_to_text(render_collapsed(state, elapsed_seconds=0.3), width=20) + assert "Working" in text + assert "pytest" in text + # Long activity lines wrap within the console width instead of overflowing. + assert all(len(line.rstrip()) <= 20 for line in text.splitlines()) + + +def test_render_collapsed_reuses_provided_spinner_instance(): + state = TurnLiveState(iteration=1) + spinner = Spinner("dots", style="cyan") + first = render_collapsed(state, elapsed_seconds=0.1, spinner=spinner) + second = render_collapsed(state, elapsed_seconds=0.2, spinner=spinner) + # The same Spinner object must be embedded each time so its animation + # clock is not reset between refreshes (regression for 51645f5). + assert first.renderables[0] is spinner + assert second.renderables[0] is spinner + + +def test_renderer_keeps_one_spinner_object_across_renders(): + renderer = LiveTurnRenderer( + console=Console(file=io.StringIO(), force_terminal=False, width=80), + enable_keypress=False, + ) + renderer.on_event(EVENT_SESSION_START, {"request_id": "r"}) + first = renderer._render() + second = renderer._render() + assert first.renderables[0] is renderer._spinner + assert second.renderables[0] is renderer._spinner + + +def test_render_status_line_terminal_states_never_render_stale(): + finished = TurnLiveState(finished=True, final_status="completed", last_event_at=1.0) + text = _render_to_text(render_status_line(finished, elapsed_seconds=500.0, now=1000.0)) + assert "completed" in text + assert "No live events" not in text + + failed = TurnLiveState(phase=LivePhase.FAILED, final_status="failed", last_event_at=1.0) + text = _render_to_text(render_status_line(failed, elapsed_seconds=500.0, now=1000.0)) + assert "failed" in text + assert "No live events" not in text + + +def test_render_status_line_running_tool_at_hard_stale_age_keeps_plain_label(): + # Staleness banners were removed in 635b496; even far past the old hard + # threshold the active phase label renders unchanged and `now` is ignored. + state = TurnLiveState( + phase=LivePhase.RUNNING_TOOL, + current_action_id="a1", + current_action_tool="shell", + last_event_at=10.0, + ) + text = _render_to_text(render_status_line(state, elapsed_seconds=120.0, now=130.0)) + assert text.strip() == "Working" + assert "No live events" not in text + + +def test_collapsed_activity_renders_markup_brackets_literally(): + state = TurnLiveState() + state.fold(EVENT_TOOL_CALL_STARTED, {"action_id": "a1", "tool": "[bold]not markup[/bold]"}) + text = _render_to_text(render_collapsed(state, elapsed_seconds=0.1)) + # Activity lines go through Text(), so Rich markup is rendered literally. + assert "next: [bold]not markup[/bold]" in text + + +def test_collapsed_activity_with_ansi_escapes_renders_without_error(): + state = TurnLiveState() + state.fold( + EVENT_SHELL_EXECUTION_STARTED, + {"action_id": "a1", "command_preview": "echo \x1b[31mred\x1b[0m"}, + ) + state.fold( + EVENT_SHELL_EXECUTION_FINISHED, + {"action_id": "a1", "stdout_preview": "\x1b[31mred\x1b[0m"}, + ) + text = _render_to_text(render_collapsed(state, elapsed_seconds=0.1)) + assert "red" in text + + +def test_detail_panel_with_ansi_error_text_renders_without_error(): + state = TurnLiveState(request_text="plain request") + state.fold(EVENT_TOOL_CALL_STARTED, {"action_id": "a1", "tool": "shell"}) + state.fold(EVENT_TOOL_CALL_FAILED, {"action_id": "a1", "error": "\x1b[31mboom\x1b[0m"}) + text = _render_to_text(render_detail_panel(state, elapsed_seconds=0.5)) + assert "boom" in text + assert "plain request" in text + + +def test_fold_and_render_with_empty_payloads_does_not_crash(): + state = TurnLiveState() + for event in ( + EVENT_USER_REQUEST, + EVENT_SESSION_START, + EVENT_ITERATION_STARTED, + EVENT_PLAN_STARTED, + EVENT_PLAN_FINISHED, + EVENT_TOOL_CALL_STARTED, + EVENT_TOOL_CALL_FINISHED, + EVENT_SHELL_EXECUTION_STARTED, + EVENT_SHELL_EXECUTION_FINISHED, + EVENT_APPROVAL_REQUESTED, + EVENT_APPROVAL_RESOLVED, + EVENT_ITERATION_COMPLETED, + EVENT_SESSION_END, + ): + state.fold(event, {}) + status = _render_to_text(render_status_line(state, elapsed_seconds=0.1)) + collapsed = _render_to_text(render_collapsed(state, elapsed_seconds=0.1)) + detail = _render_to_text(render_detail_panel(state, elapsed_seconds=0.1)) + assert status.strip() + assert collapsed.strip() + assert "(no request)" in detail diff --git a/tests/test_planner.py b/tests/test_planner.py new file mode 100644 index 0000000..d0d126a --- /dev/null +++ b/tests/test_planner.py @@ -0,0 +1,718 @@ +"""Hardening stage 7: isolated unit tests for PlannerService. + +These tests exercise the planner directly (observation injection, plan-time +endpoint validation, plan repair, and the `_validate_supported_actions` +guards) without going through the orchestrator loop. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from typing import Any + +import pytest + +from foundation.models import ( + AssistantPlan, + CapabilityId, + CapabilityInstallSource, + CapabilityKind, + CapabilityManifest, + CapabilityTransport, + CapabilityVersion, + ContextSnapshot, + ProviderMessage, + ProviderMessageRole, + ProviderPrompt, + ProviderResponse, + ProviderResponseMetadata, + RiskClass, + TrustTier, + UserRequest, +) +from foundation.services import LocalToolService +from foundation.services.capabilities import CapabilityRegistry, CapabilityStore +from foundation.services.planner import PlannerService, PlanningError +from foundation.services.provider import ProviderError, ProviderErrorCode +from foundation.settings import ApprovalMode + + +def _provider_response(payload: dict[str, Any]) -> ProviderResponse: + return ProviderResponse( + content=json.dumps(payload), + structured_output=payload, + metadata=ProviderResponseMetadata( + provider="stub", + model="stub-model", + latency_seconds=0.01, + ), + ) + + +def _text_response(body: str) -> ProviderResponse: + return ProviderResponse( + content=body, + structured_output=None, + metadata=ProviderResponseMetadata( + provider="stub", + model="stub-model", + latency_seconds=0.01, + ), + ) + + +class StubProvider: + """Queue-backed provider stub mirroring the orchestrator test pattern.""" + + def __init__(self, responses: list[ProviderResponse]) -> None: + self._responses = list(responses) + self.calls: list[ProviderPrompt] = [] + + def complete(self, prompt: ProviderPrompt) -> ProviderResponse: + if prompt.schema_name == "assistant_plan_review" and not ( + self._responses + and isinstance(self._responses[0].structured_output, dict) + and "decision" in self._responses[0].structured_output + ): + return _provider_response( + { + "decision": "accept", + "reason": "Stub preflight accepted the candidate plan.", + } + ) + self.calls.append(prompt) + if not self._responses: + return _provider_response({"assistant_message": "Done.", "actions": []}) + return self._responses.pop(0) + + +class ErrorThenPlanProvider: + """Raise one ProviderError on the first call, then return the queued plan.""" + + def __init__(self, error: ProviderError, response: ProviderResponse) -> None: + self._error: ProviderError | None = error + self._response = response + self.calls: list[ProviderPrompt] = [] + + def complete(self, prompt: ProviderPrompt) -> ProviderResponse: + self.calls.append(prompt) + if self._error is not None: + error, self._error = self._error, None + raise error + return self._response + + +def _build_planner( + tmp_path: Path, + provider: Any, + *, + max_plan_attempts: int = 2, +) -> tuple[PlannerService, CapabilityRegistry, Path]: + workspace_root = tmp_path / "workspace" + workspace_root.mkdir(exist_ok=True) + tool_service = LocalToolService( + workspace_root=workspace_root, + default_timeout_seconds=5, + capture_limit_kb=64, + ) + registry = CapabilityRegistry( + store=CapabilityStore(tmp_path / "capabilities"), + tool_service=tool_service, + ) + planner = PlannerService( + workspace_root=str(workspace_root), + approval_mode=ApprovalMode.PROMPT, + provider=provider, + tool_service=tool_service, + capability_registry=registry, + max_plan_attempts=max_plan_attempts, + ) + return planner, registry, workspace_root + + +def _context( + workspace_root: Path, + *, + git_context: dict[str, Any] | None = None, +) -> ContextSnapshot: + return ContextSnapshot( + workspace_root=str(workspace_root), + request_cwd=str(workspace_root), + approval_mode="prompt", + git_context=git_context, + ) + + +def _plan(actions: list[dict[str, Any]], message: str = "Working on it.") -> AssistantPlan: + return AssistantPlan.model_validate({"assistant_message": message, "actions": actions}) + + +def _shell_action( + action_id: str, + command: str, + args: list[str] | None = None, +) -> dict[str, Any]: + return { + "id": action_id, + "kind": "shell", + "summary": f"Run {command}", + "shell": {"command": command, "args": args or []}, + } + + +def _tool_action( + action_id: str, + capability_id: str, + arguments: dict[str, Any], + *, + requires_approval: bool = False, + approval_reason: str | None = None, +) -> dict[str, Any]: + return { + "id": action_id, + "kind": "tool_call", + "summary": f"Call {capability_id}", + "requires_approval": requires_approval, + "approval_reason": approval_reason, + "tool_call": {"capability_id": capability_id, "arguments": arguments}, + } + + +def _register_bogus_capability(registry: CapabilityRegistry) -> None: + registry.register( + CapabilityManifest( + capability_id=CapabilityId(root="foundation.bogus"), + version=CapabilityVersion(root="1.0.0"), + kind=CapabilityKind.TOOL, + name="Bogus Tool", + description="Test-only capability whose runtime endpoint is unsupported.", + transport=CapabilityTransport.BUILTIN_TOOL, + runtime_endpoint="builtin.bogus", + input_schema={"type": "object"}, + install_source=CapabilityInstallSource(kind="test", location="test://bogus"), + owner="tests", + risk_class=RiskClass.LOW, + trust_tier=TrustTier.FOUNDATION, + ) + ) + + +# --------------------------------------------------------------------------- +# Observation injection +# --------------------------------------------------------------------------- + + +def test_observation_and_iteration_details_reach_planning_prompt(tmp_path: Path) -> None: + provider = StubProvider( + [_provider_response({"assistant_message": "Investigation complete.", "actions": []})] + ) + planner, _registry, workspace_root = _build_planner(tmp_path, provider) + + plan, metadata = planner.request_plan( + UserRequest(message="fix the failing test"), + _context(workspace_root), + request_id="req-1", + observation_messages=[ + ProviderMessage( + role=ProviderMessageRole.ASSISTANT, + content="Observation block: pytest exited 1 in iteration 2.", + ), + ProviderMessage( + role=ProviderMessageRole.DEVELOPER, + content="Remaining budget is shrinking.", + ), + ], + iteration=3, + remaining_actions=12, + ) + + assert plan.assistant_message == "Investigation complete." + assert metadata.provider == "stub" + assert len(provider.calls) == 1 + prompt = provider.calls[0] + assert prompt.schema_name == "assistant_plan" + developer = prompt.messages[0] + assert developer.role is ProviderMessageRole.DEVELOPER + # Observation messages are folded into the developer instructions, joined + # by blank lines, rather than appended as extra conversation turns. + assert "Observation block: pytest exited 1 in iteration 2." in developer.content + assert "Remaining budget is shrinking." in developer.content + assert "This is iteration 3" in developer.content + assert "Return at most 12 actions" in developer.content + user = prompt.messages[-1] + assert user.role is ProviderMessageRole.USER + assert "fix the failing test" in user.content + + +# --------------------------------------------------------------------------- +# Plan-time endpoint validation +# --------------------------------------------------------------------------- + + +def test_valid_typed_file_and_git_plan_passes_validation(tmp_path: Path) -> None: + provider = StubProvider( + [ + _provider_response( + { + "assistant_message": "Inspecting the workspace.", + "actions": [ + _tool_action("read_app", "foundation.file.read", {"path": "src/app.py"}), + _tool_action("repo_status", "foundation.git.status", {}), + ], + } + ) + ] + ) + planner, _registry, workspace_root = _build_planner(tmp_path, provider) + + plan, _metadata = planner.request_plan( + UserRequest(message="what changed?"), + _context(workspace_root), + request_id="req-1", + ) + + assert len(provider.calls) == 1 + assert [action.tool_call.capability_id for action in plan.actions if action.tool_call] == [ + "foundation.file.read", + "foundation.git.status", + ] + # Defaults: iteration 1, full action budget capped at the plan bound. + developer = provider.calls[0].messages[0] + assert "This is iteration 1" in developer.content + assert "Return at most 40 actions" in developer.content + + +def test_unknown_runtime_endpoint_is_rejected(tmp_path: Path) -> None: + provider = StubProvider( + [ + _provider_response( + { + "assistant_message": "Calling a capability with no executor.", + "actions": [_tool_action("bogus_call", "foundation.bogus", {})], + } + ) + ] + ) + planner, registry, workspace_root = _build_planner(tmp_path, provider, max_plan_attempts=1) + _register_bogus_capability(registry) + + with pytest.raises(PlanningError) as excinfo: + planner.request_plan( + UserRequest(message="do something"), + _context(workspace_root), + request_id="req-1", + ) + + assert "Unsupported capability id: foundation.bogus" in str(excinfo.value) + assert "after 1 attempt(s)" in str(excinfo.value) + + +# --------------------------------------------------------------------------- +# Plan repair +# --------------------------------------------------------------------------- + + +def test_missing_structured_output_triggers_repair_retry(tmp_path: Path) -> None: + provider = StubProvider( + [ + _text_response("plain prose, not the requested JSON object"), + _provider_response({"assistant_message": "Recovered.", "actions": []}), + ] + ) + planner, _registry, workspace_root = _build_planner(tmp_path, provider) + + plan, _metadata = planner.request_plan( + UserRequest(message="say hello"), + _context(workspace_root), + request_id="req-1", + ) + + assert plan.assistant_message == "Recovered." + assert len(provider.calls) == 2 + retry = provider.calls[1] + assert len(retry.messages) == len(provider.calls[0].messages) + 1 + repair = retry.messages[-1] + assert repair.role is ProviderMessageRole.DEVELOPER + assert "omitted the required JSON object" in repair.content + assert "Return a corrected JSON object only" in repair.content + # First attempt decodes deterministically; the retry nudges temperature. + assert provider.calls[0].temperature is None + assert retry.temperature == 0.4 + + +def test_invalid_action_shape_triggers_repair_with_validation_feedback(tmp_path: Path) -> None: + bad_payload = { + "assistant_message": "Doing work.", + "actions": [ + { + "id": "broken", + "kind": "shell", + "summary": "Shell kind with tool payload", + "tool_call": { + "capability_id": "foundation.file.read", + "arguments": {"path": "x"}, + }, + } + ], + } + provider = StubProvider( + [ + _provider_response(bad_payload), + _provider_response({"assistant_message": "Fixed.", "actions": []}), + ] + ) + planner, _registry, workspace_root = _build_planner(tmp_path, provider) + + plan, _metadata = planner.request_plan( + UserRequest(message="read a file"), + _context(workspace_root), + request_id="req-1", + ) + + assert plan.assistant_message == "Fixed." + assert len(provider.calls) == 2 + retry = provider.calls[1] + # The invalid output is echoed back as an assistant turn before the + # developer repair instruction. + assert retry.messages[-2].role is ProviderMessageRole.ASSISTANT + assert retry.messages[-2].content == json.dumps(bad_payload) + assert retry.messages[-1].role is ProviderMessageRole.DEVELOPER + assert "The previous JSON failed validation" in retry.messages[-1].content + + +def test_truncated_response_repair_requests_content_brief(tmp_path: Path) -> None: + partial = '{"assistant_message":"writing","actions":[{"id":"w"' + provider = ErrorThenPlanProvider( + ProviderError( + "Provider response was truncated before completion.", + code=ProviderErrorCode.TRUNCATED, + response_text=partial, + ), + _provider_response({"assistant_message": "Shorter plan.", "actions": []}), + ) + planner, _registry, workspace_root = _build_planner(tmp_path, provider) + + plan, _metadata = planner.request_plan( + UserRequest(message="write a big file"), + _context(workspace_root), + request_id="req-1", + ) + + assert plan.assistant_message == "Shorter plan." + assert len(provider.calls) == 2 + retry = provider.calls[1] + assert retry.messages[-2].role is ProviderMessageRole.ASSISTANT + assert retry.messages[-2].content == partial + assert "truncated before the JSON closed" in retry.messages[-1].content + assert "content_brief" in retry.messages[-1].content + assert retry.temperature == 0.4 + + +def test_non_repairable_provider_error_propagates_unwrapped(tmp_path: Path) -> None: + provider = ErrorThenPlanProvider( + ProviderError("connection refused", code=ProviderErrorCode.NETWORK), + _provider_response({"assistant_message": "Never reached.", "actions": []}), + ) + planner, _registry, workspace_root = _build_planner(tmp_path, provider) + + with pytest.raises(ProviderError) as excinfo: + planner.request_plan( + UserRequest(message="say hello"), + _context(workspace_root), + request_id="req-1", + ) + + assert excinfo.value.code is ProviderErrorCode.NETWORK + assert len(provider.calls) == 1 + + +def test_planning_error_after_exhausted_attempts(tmp_path: Path) -> None: + provider = StubProvider( + [ + _text_response("still not JSON"), + _text_response("again not JSON"), + ] + ) + planner, _registry, workspace_root = _build_planner(tmp_path, provider) + + with pytest.raises(PlanningError) as excinfo: + planner.request_plan( + UserRequest(message="say hello"), + _context(workspace_root), + request_id="req-1", + ) + + assert "after 2 attempt(s)" in str(excinfo.value) + assert len(provider.calls) == 2 + + +# --------------------------------------------------------------------------- +# Zero-action commit-intent guard +# --------------------------------------------------------------------------- + + +def test_zero_action_plan_with_commit_intent_and_staged_changes_is_rejected( + tmp_path: Path, +) -> None: + planner, _registry, workspace_root = _build_planner(tmp_path, StubProvider([])) + context = _context( + workspace_root, + git_context={"status": [{"index_status": "M", "path": "src/app.py"}]}, + ) + + with pytest.raises(PlanningError) as excinfo: + planner._validate_supported_actions( + _plan([], message="All done."), + request=UserRequest(message="Please commit the staged changes."), + context=context, + ) + + assert "Zero-action completion is invalid" in str(excinfo.value) + assert "foundation.git.commit" in str(excinfo.value) + + +def test_zero_action_plan_allowed_when_nothing_is_staged(tmp_path: Path) -> None: + planner, _registry, workspace_root = _build_planner(tmp_path, StubProvider([])) + # Untracked-only status entries do not count as staged changes. + context = _context( + workspace_root, + git_context={"status": [{"index_status": "?", "path": "scratch.txt"}], "staged_diff": []}, + ) + + planner._validate_supported_actions( + _plan([], message="Nothing to commit."), + request=UserRequest(message="Please commit the staged changes."), + context=context, + ) + + +# --------------------------------------------------------------------------- +# Shell action guards +# --------------------------------------------------------------------------- + + +def test_gh_api_raw_output_flag_is_rejected(tmp_path: Path) -> None: + planner, _registry, workspace_root = _build_planner(tmp_path, StubProvider([])) + plan = _plan( + [_shell_action("fetch_readme", "gh", ["api", "repos/x/y/readme", "--jq", ".content", "-r"])] + ) + + with pytest.raises(PlanningError) as excinfo: + planner._validate_supported_actions( + plan, + request=UserRequest(message="fetch my GitHub README"), + context=_context(workspace_root), + ) + + assert "gh api" in str(excinfo.value) + assert "does not support `-r`" in str(excinfo.value) + + +@pytest.mark.parametrize( + ("command", "expected_equivalent"), + [ + ("cat", "foundation.file.read"), + ("grep", "foundation.search"), + ("printf", "foundation.file.write"), + ("/bin/cat", "foundation.file.read"), + ], +) +def test_shell_commands_with_typed_equivalents_are_rejected( + tmp_path: Path, + command: str, + expected_equivalent: str, +) -> None: + planner, _registry, workspace_root = _build_planner(tmp_path, StubProvider([])) + plan = _plan([_shell_action("use_shell", command, ["some-target"])]) + + with pytest.raises(PlanningError) as excinfo: + planner._validate_supported_actions( + plan, + request=UserRequest(message="inspect the file"), + context=_context(workspace_root), + ) + + assert expected_equivalent in str(excinfo.value) + + +# --------------------------------------------------------------------------- +# Tool-call guards +# --------------------------------------------------------------------------- + + +def test_git_commit_action_must_require_approval(tmp_path: Path) -> None: + planner, _registry, workspace_root = _build_planner(tmp_path, StubProvider([])) + plan = _plan( + [_tool_action("commit_work", "foundation.git.commit", {"message": "feat: add thing"})] + ) + + with pytest.raises(PlanningError) as excinfo: + planner._validate_supported_actions( + plan, + request=UserRequest(message="commit the staged work"), + context=_context(workspace_root), + ) + + assert "requires_approval=true" in str(excinfo.value) + + +def test_deferred_write_following_earlier_actions_is_rejected(tmp_path: Path) -> None: + planner, _registry, workspace_root = _build_planner(tmp_path, StubProvider([])) + plan = _plan( + [ + _shell_action("list_dir", "ls", []), + _tool_action( + "write_report", + "foundation.file.write", + {"path": "report.md", "content_brief": "a report based on the listing"}, + ), + ] + ) + + with pytest.raises(PlanningError) as excinfo: + planner._validate_supported_actions( + plan, + request=UserRequest(message="summarize the directory"), + context=_context(workspace_root), + ) + + assert "content_brief cannot follow earlier" in str(excinfo.value) + + +def test_file_write_with_both_content_and_brief_is_rejected(tmp_path: Path) -> None: + planner, _registry, workspace_root = _build_planner(tmp_path, StubProvider([])) + plan = _plan( + [ + _tool_action( + "write_notes", + "foundation.file.write", + {"path": "notes.md", "content": "hello", "content_brief": "greeting"}, + ) + ] + ) + + with pytest.raises(PlanningError) as excinfo: + planner._validate_supported_actions( + plan, + request=UserRequest(message="write the notes file"), + context=_context(workspace_root), + ) + + assert "either content or content_brief, not both" in str(excinfo.value) + + +def test_request_plan_truncates_plan_before_deferred_write(tmp_path: Path) -> None: + provider = StubProvider( + [ + _provider_response( + { + "assistant_message": "Listing first, then writing.", + "actions": [ + _shell_action("list_dir", "ls", []), + _tool_action( + "write_report", + "foundation.file.write", + {"path": "report.md", "content_brief": "a directory report"}, + ), + ], + } + ) + ] + ) + planner, _registry, workspace_root = _build_planner(tmp_path, provider) + + plan, _metadata = planner.request_plan( + UserRequest(message="summarize the directory"), + _context(workspace_root), + request_id="req-1", + ) + + # Rather than rejecting outright, request_plan keeps the executable prefix + # and drops the deferred write so it can be replanned with observations. + assert len(provider.calls) == 1 + assert plan.assistant_message == "Listing first, then writing." + assert [action.id for action in plan.actions] == ["list_dir"] + + +# --------------------------------------------------------------------------- +# Preflight review and payload normalization +# --------------------------------------------------------------------------- + + +def test_preflight_reject_returns_zero_action_answer(tmp_path: Path) -> None: + provider = StubProvider( + [ + _provider_response( + { + "assistant_message": "Writing the notes file.", + "actions": [ + _tool_action( + "write_notes", + "foundation.file.write", + {"path": "notes.md", "content": "hello"}, + ) + ], + } + ), + _provider_response( + { + "decision": "reject", + "reason": "Execution should not proceed.", + } + ), + ] + ) + planner, _registry, workspace_root = _build_planner(tmp_path, provider) + + plan, _metadata = planner.request_plan( + UserRequest(message="write the notes file"), + _context(workspace_root), + request_id="req-1", + ) + + assert plan.actions == [] + assert plan.assistant_message == "Execution should not proceed." + assert len(provider.calls) == 2 + review_prompt = provider.calls[1] + assert review_prompt.schema_name == "assistant_plan_review" + assert "write the notes file" in review_prompt.messages[-1].content + + +def test_file_write_note_is_normalized_into_content_brief(tmp_path: Path) -> None: + provider = StubProvider( + [ + _provider_response( + { + "assistant_message": "Writing notes.", + "actions": [ + { + "id": "write_notes", + "kind": "tool_call", + "summary": "Write the notes file", + "tool_call": { + "capability_id": "foundation.file.write", + "_file_write_note": "content_brief: a short summary of the run", + "arguments": {"path": "notes.md"}, + }, + } + ], + } + ) + ] + ) + planner, _registry, workspace_root = _build_planner(tmp_path, provider) + + plan, _metadata = planner.request_plan( + UserRequest(message="write the notes file"), + _context(workspace_root), + request_id="req-1", + ) + + assert len(plan.actions) == 1 + tool_call = plan.actions[0].tool_call + assert tool_call is not None + assert tool_call.arguments == { + "path": "notes.md", + "content_brief": "a short summary of the run", + } diff --git a/tests/test_provider.py b/tests/test_provider.py index c70509c..72dd7c4 100644 --- a/tests/test_provider.py +++ b/tests/test_provider.py @@ -89,6 +89,45 @@ def run( ) +class ScriptedCodexRunner: + """Codex runner fake for failure paths: raises or returns a scripted result.""" + + def __init__( + self, + *, + exception: Exception | None = None, + returncode: int = 0, + stdout: str = "", + stderr: str = "", + final_message: str | None = None, + ) -> None: + self._exception = exception + self._returncode = returncode + self._stdout = stdout + self._stderr = stderr + self._final_message = final_message + + def run( + self, + args: list[str], + *, + cwd: Path, + input_text: str, + timeout_seconds: int, + ) -> subprocess.CompletedProcess[str]: + if self._exception is not None: + raise self._exception + if self._final_message is not None: + output_path = Path(args[args.index("--output-last-message") + 1]) + output_path.write_text(self._final_message, encoding="utf-8") + return subprocess.CompletedProcess( + args=args, + returncode=self._returncode, + stdout=self._stdout, + stderr=self._stderr, + ) + + def _structured_prompt() -> ProviderPrompt: return ProviderPrompt( messages=[ @@ -223,6 +262,163 @@ def test_codex_adapter_omits_output_schema_for_open_ended_json_schema( assert '"additionalProperties": true' in runner.calls[0]["input_text"] +def _codex_adapter( + tmp_path: Path, + runner: ScriptedCodexRunner, + *, + timeout_seconds: int = 60, +) -> CodexExecAdapter: + return CodexExecAdapter( + model="gpt-5.5", + workspace_root=tmp_path, + timeout_seconds=timeout_seconds, + runner=runner, + ) + + +def test_codex_adapter_missing_binary_maps_to_bad_request(tmp_path: Path) -> None: + runner = ScriptedCodexRunner(exception=FileNotFoundError("codex")) + adapter = _codex_adapter(tmp_path, runner) + + with pytest.raises(ProviderError) as exc_info: + adapter.complete(_structured_prompt()) + + assert exc_info.value.code is ProviderErrorCode.BAD_REQUEST + assert exc_info.value.retryable is False + assert "Codex CLI was not found on PATH" in str(exc_info.value) + + +def test_codex_adapter_timeout_maps_to_retryable_network_error(tmp_path: Path) -> None: + runner = ScriptedCodexRunner( + exception=subprocess.TimeoutExpired(cmd=["codex", "exec"], timeout=5) + ) + adapter = _codex_adapter(tmp_path, runner, timeout_seconds=5) + + with pytest.raises(ProviderError) as exc_info: + adapter.complete(_structured_prompt()) + + assert exc_info.value.code is ProviderErrorCode.NETWORK + assert exc_info.value.retryable is True + assert "timed out after 5s" in str(exc_info.value) + + +def test_codex_adapter_launch_oserror_maps_to_retryable_network_error( + tmp_path: Path, +) -> None: + runner = ScriptedCodexRunner(exception=OSError("argument list too long")) + adapter = _codex_adapter(tmp_path, runner) + + with pytest.raises(ProviderError) as exc_info: + adapter.complete(_structured_prompt()) + + assert exc_info.value.code is ProviderErrorCode.NETWORK + assert exc_info.value.retryable is True + assert "failed to start" in str(exc_info.value) + assert "argument list too long" in str(exc_info.value) + + +def test_codex_adapter_auth_failure_stderr_maps_to_authentication( + tmp_path: Path, +) -> None: + stderr = "Error: not logged in. Run `codex login` and sign in with ChatGPT." + runner = ScriptedCodexRunner(returncode=1, stderr=stderr) + adapter = _codex_adapter(tmp_path, runner) + + with pytest.raises(ProviderError) as exc_info: + adapter.complete(_structured_prompt()) + + assert exc_info.value.code is ProviderErrorCode.AUTHENTICATION + assert exc_info.value.retryable is False + assert str(exc_info.value) == stderr + + +def test_codex_adapter_usage_limit_stderr_maps_to_retryable_rate_limit( + tmp_path: Path, +) -> None: + stderr = "You've hit your usage limit. Try again later." + runner = ScriptedCodexRunner(returncode=1, stderr=stderr) + adapter = _codex_adapter(tmp_path, runner) + + with pytest.raises(ProviderError) as exc_info: + adapter.complete(_structured_prompt()) + + assert exc_info.value.code is ProviderErrorCode.RATE_LIMIT + assert exc_info.value.retryable is True + assert str(exc_info.value) == stderr + + +@pytest.mark.parametrize( + ("returncode", "expected_retryable"), + [(1, False), (2, True)], +) +def test_codex_adapter_nonzero_exit_preserves_stderr_in_server_error( + tmp_path: Path, + returncode: int, + expected_retryable: bool, +) -> None: + stderr = "stream disconnected before completion: unexpected status" + runner = ScriptedCodexRunner(returncode=returncode, stderr=stderr) + adapter = _codex_adapter(tmp_path, runner) + + with pytest.raises(ProviderError) as exc_info: + adapter.complete(_structured_prompt()) + + assert exc_info.value.code is ProviderErrorCode.SERVER_ERROR + assert exc_info.value.retryable is expected_retryable + assert str(exc_info.value) == stderr + + +def test_codex_adapter_nonzero_exit_prefers_error_event_from_stdout( + tmp_path: Path, +) -> None: + runner = ScriptedCodexRunner( + returncode=1, + stdout='{"type":"error","message":"model stream closed unexpectedly"}\n', + stderr="exit status 1", + ) + adapter = _codex_adapter(tmp_path, runner) + + with pytest.raises(ProviderError) as exc_info: + adapter.complete(_structured_prompt()) + + assert exc_info.value.code is ProviderErrorCode.SERVER_ERROR + assert str(exc_info.value) == "model stream closed unexpectedly" + assert "exit status 1" in (exc_info.value.response_text or "") + + +def test_codex_adapter_malformed_json_output_maps_to_invalid_response( + tmp_path: Path, +) -> None: + runner = ScriptedCodexRunner(final_message="Sorry, I cannot produce JSON for that request.") + adapter = _codex_adapter(tmp_path, runner) + + with pytest.raises(ProviderError) as exc_info: + adapter.complete(_structured_prompt()) + + assert exc_info.value.code is ProviderErrorCode.INVALID_RESPONSE + assert "invalid JSON" in str(exc_info.value) + assert "Sorry, I cannot produce JSON" in str(exc_info.value) + + +def test_codex_adapter_empty_output_maps_to_invalid_response(tmp_path: Path) -> None: + runner = ScriptedCodexRunner(returncode=0, stdout="") + adapter = _codex_adapter(tmp_path, runner) + prompt = ProviderPrompt( + messages=[ + ProviderMessage( + role=ProviderMessageRole.USER, + content="Say hello.", + ) + ], + ) + + with pytest.raises(ProviderError) as exc_info: + adapter.complete(prompt) + + assert exc_info.value.code is ProviderErrorCode.INVALID_RESPONSE + assert "no final assistant message" in str(exc_info.value) + + def test_openai_adapter_parses_structured_output_and_usage() -> None: transport = FakeTransport( [ From 3c863ee0349309249942d5f90c6c4237f9445f9a Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Wed, 10 Jun 2026 18:17:14 -0700 Subject: [PATCH 09/18] Fix two bugs found by the stage 7 test sweep A plan naming a nonexistent capability id raised a plain ValueError from CapabilityRegistry.resolve that escaped both the plan-repair loop and the orchestrator's PlanningError handling, crashing the turn; it is now wrapped as PlanningError so the model gets a repair retry. And render_detail_panel passed user request text to Rich as raw markup, allowing styling injection and a MarkupError crash mid-Live.update; it is now rendered literally via Text(). Co-Authored-By: Claude Fable 5 --- src/foundation/live_turn.py | 4 +++- src/foundation/services/planner.py | 8 +++++++- tests/test_live_turn.py | 15 +++++++++++++++ tests/test_planner.py | 30 ++++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 2 deletions(-) diff --git a/src/foundation/live_turn.py b/src/foundation/live_turn.py index c55e2d4..e801802 100644 --- a/src/foundation/live_turn.py +++ b/src/foundation/live_turn.py @@ -346,7 +346,9 @@ def render_detail_panel(state: TurnLiveState, *, elapsed_seconds: float) -> Rend table.add_column(no_wrap=False) request = _truncate(state.request_text or "(no request)", limit=80) - table.add_row("request", request) + # Text() keeps user-controlled request text literal — raw strings would be + # parsed as Rich markup (styling injection, MarkupError on stray tags). + table.add_row("request", Text(request)) phase_elapsed = max(now - state.phase_started_at, 0.0) if state.phase_started_at else 0.0 table.add_row("phase", f"{state.phase.value} · {_format_duration(phase_elapsed)}") table.add_row("last event", state.last_event_name or "(none)") diff --git a/src/foundation/services/planner.py b/src/foundation/services/planner.py index 0c7c9e9..b6abfc3 100644 --- a/src/foundation/services/planner.py +++ b/src/foundation/services/planner.py @@ -803,7 +803,13 @@ def _validated_tool_request( version: str | None, arguments: dict[str, object], ) -> None: - manifest = self._capability_registry.resolve(capability_id, version) + try: + manifest = self._capability_registry.resolve(capability_id, version) + except ValueError as exc: + # The registry raises a plain ValueError for unknown capability + # ids; wrap it so the plan-repair loop (and the orchestrator's + # PlanningError handling) can route it instead of crashing. + raise PlanningError(str(exc)) from exc endpoint = manifest.runtime_endpoint if endpoint == "builtin.search": from foundation.services.tools import SearchRequest diff --git a/tests/test_live_turn.py b/tests/test_live_turn.py index 6ad0733..8d4992e 100644 --- a/tests/test_live_turn.py +++ b/tests/test_live_turn.py @@ -489,3 +489,18 @@ def test_fold_and_render_with_empty_payloads_does_not_crash(): assert status.strip() assert collapsed.strip() assert "(no request)" in detail + + +def test_detail_panel_request_text_is_not_parsed_as_markup(): + """Found by hardening stage 7: raw request text reached Rich as markup. + + A request containing markup-like brackets must render literally, and a + malformed closing tag must not raise MarkupError mid-Live.update. + """ + state = TurnLiveState(request_text="[bold]not markup[/bold]") + text = _render_to_text(render_detail_panel(state, elapsed_seconds=0.1)) + assert "[bold]not markup[/bold]" in text + + state_malformed = TurnLiveState(request_text="[/bold]oops") + text = _render_to_text(render_detail_panel(state_malformed, elapsed_seconds=0.1)) + assert "oops" in text diff --git a/tests/test_planner.py b/tests/test_planner.py index d0d126a..61c0f94 100644 --- a/tests/test_planner.py +++ b/tests/test_planner.py @@ -716,3 +716,33 @@ def test_file_write_note_is_normalized_into_content_brief(tmp_path: Path) -> Non "path": "notes.md", "content_brief": "a short summary of the run", } + + +def test_hallucinated_capability_id_routes_through_plan_repair(tmp_path: Path) -> None: + """A plan naming a nonexistent capability must repair, not crash. + + Found by hardening stage 7: CapabilityRegistry.resolve raises a plain + ValueError, which used to escape both the repair loop and the + orchestrator's PlanningError handling. + """ + provider = StubProvider( + [ + _provider_response( + { + "assistant_message": "Using a made-up tool.", + "actions": [_tool_action("a1", "foundation.does-not-exist", {"path": "x"})], + } + ), + _provider_response({"assistant_message": "Recovered.", "actions": []}), + ] + ) + planner, _registry, workspace_root = _build_planner(tmp_path, provider) + + plan, _metadata = planner.request_plan( + UserRequest(message="do a thing"), + _context(workspace_root), + request_id="req-1", + ) + + assert plan.assistant_message == "Recovered." + assert len(provider.calls) == 2 From 8dbc7d3f1e714946d37faaf02febcd392ded663f Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Wed, 10 Jun 2026 18:21:39 -0700 Subject: [PATCH 10/18] Stage 9: docs and plans hygiene Mark completed roadmaps (fixes roadmap, v0.1, v2 stages, v4) with status headers, add the hardening-batch CHANGELOG section, and document the write_truncated semantics, sink circuit-breaker, and pre-migration backups in TECHNICAL.md. Stage 8 (orchestrator slimming) is recorded as evaluated-and-descoped: the helpers' only callers live inside the orchestrator, and the constructor params are deliberate DI seams. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 52 +++++++++++++++++++ docs/TECHNICAL.md | 12 +++++ plans/00-roadmap.md | 4 ++ plans/fcli-fixes-roadmap.md | 9 ++++ plans/fcli-hardening-roadmap.md | 24 +++++++++ ...rsational-brain-and-persistent-sessions.md | 2 + .../01-capability-registry-and-local-store.md | 2 + .../02-capability-policies-and-governance.md | 2 + plans/v2/03-trace-audit-and-runtime-split.md | 2 + ...iet-chat-surface-and-audit-first-output.md | 2 + plans/v4/00-roadmap.md | 5 ++ 11 files changed, 116 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67a126f..911990e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,58 @@ All notable changes to Foundation CLI are documented here. This project follows semantic-ish versioning: feature releases bump the minor version; bug fixes and small enhancements land on patch releases. +## [Unreleased] — hardening batch (2026-06) + +Closes the gaps found in the 2026-06-10 full-project review (see +`plans/fcli-hardening-roadmap.md` for stage-by-stage detail and the findings +that were checked and rejected). + +### Changed + +- **Executor invariants fail loudly.** All 16 `assert` statements in the + action executor (kind/payload narrowing, file/git service wiring) were + replaced with a typed `ExecutorInvariantError`; violations now surface as + FAILED execution results in the trace instead of interpreter crashes, and + survive `python -O`. +- **Plan-action validation closed its last holes.** A stray `question` + payload on EXPLANATION/SHELL/TOOL_CALL actions (and a stray `explanation` + on QUESTION actions) is now rejected at validation time and routed through + the existing plan-repair retry. +- **One source of truth for git mutation subcommands.** + `GIT_MUTATION_SUBCOMMANDS` lives in `models/git.py`; the planner and the + guardrail policy engine both alias it, so they can no longer diverge. +- **Audit-trail failures are visible.** Event-sink failures are counted and + warned about; a sink failing 3 consecutive times is disabled with one + final warning instead of spamming. A crash inside the NDJSON event-log + writer now marks the session `write_truncated` in `sessions.jsonl` instead + of letting the index claim a complete log. Gap-message phrasing fallbacks + log their reason (provider-error / empty / json-or-fenced / plan-shaped). +- **History migrations have safety rails.** Before any schema migration the + database file is backed up to `.pre-v.bak` (newest kept). The + v6 rebuild validates row counts before dropping the source table; failures + raise `HistoryMigrationError` naming the backup, with the original data + intact. +- **Diff applier leniency is bounded and reported.** Hunks whose declared + source-line count disagrees with their body, and hunks with no additions + or removals, are rejected at parse time. Bare context lines and + newline-normalized matching remain accepted but are reported through + `FileMutationResult.leniency_notes` into the trace. + +### Fixed + +- A plan naming a nonexistent capability id crashed the turn with an + unwrapped `ValueError`; it now routes through the plan-repair retry. +- The live detail panel parsed the user's request text as Rich markup, + allowing styling injection and a `MarkupError` crash; it renders literally + now. + +### Added + +- 100+ new tests, including isolated `PlannerService` unit tests + (`tests/test_planner.py`), Codex provider failure-path coverage, live + rendering edge cases, sink failure/circuit-breaker tests, and migration + backup/sabotage tests. + ## [0.2.0] — unreleased (v3) v3 makes `foundation` behave like a real coding-agent shell on top of the v2 diff --git a/docs/TECHNICAL.md b/docs/TECHNICAL.md index df78a80..036f41a 100644 --- a/docs/TECHNICAL.md +++ b/docs/TECHNICAL.md @@ -243,6 +243,18 @@ Retention defaults to 200 sessions / 500 MB; oldest sessions are pruned automatically on session end. Configure under `[monitor]` in `config.toml`. +**Degradation is recorded, never silent.** A session whose event log lost +writes — whether from an I/O error or a crash inside the writer — closes +with `status=write_truncated` in `sessions.jsonl`, so consumers can tell a +complete log from a partial one. An event sink that fails on 3 consecutive +events is disabled for the rest of the session with one final warning. + +**History migrations back up first.** Before any schema migration runs, the +SQLite history database is copied to `.pre-v.bak` next to the +original (only the newest backup is kept). A failed migration raises +`HistoryMigrationError` naming the backup and leaves the original data +untouched. + **Opt-out:** pass `--no-monitor` for one invocation, set `FOUNDATION_MONITOR=0`, or `monitor.enabled = false` in `config.toml`. Override the directory with `--events-dir `. diff --git a/plans/00-roadmap.md b/plans/00-roadmap.md index 1c54dd5..d60de9b 100644 --- a/plans/00-roadmap.md +++ b/plans/00-roadmap.md @@ -1,5 +1,9 @@ # Foundation CLI v0.1 Roadmap +## Status + +**v0.1 shipped** — superseded by the v2/v3/v4 roadmaps and the fix/hardening batches. + ## Purpose This planning set is the stage-zero baseline for Foundation CLI. It turns the MVP direction into an execution sequence with explicit gates so implementation can move from an empty repository to a usable v0.1 without scope drift. diff --git a/plans/fcli-fixes-roadmap.md b/plans/fcli-fixes-roadmap.md index 9ff58e0..7f54ed1 100644 --- a/plans/fcli-fixes-roadmap.md +++ b/plans/fcli-fixes-roadmap.md @@ -1,5 +1,14 @@ # FCLI Fixes Roadmap +## Status + +**Complete (verified 2026-06-10).** All five stages shipped: command error +recovery (`_COMMAND_USAGE_ERROR_PATTERNS` in `gap_handoff.py` + repair +notices in `orchestrator.py`), static gates restored (ruff check/format and +strict mypy all green), and live loading UX (`LivePhase` model in +`live_turn.py`, landed via #11/#12). Superseded by +`plans/fcli-hardening-roadmap.md` for the next fix batch. + ## Purpose Track the near-term fixes needed to get Foundation CLI back to a clean, diff --git a/plans/fcli-hardening-roadmap.md b/plans/fcli-hardening-roadmap.md index a416e7c..67b3ffb 100644 --- a/plans/fcli-hardening-roadmap.md +++ b/plans/fcli-hardening-roadmap.md @@ -376,6 +376,23 @@ implementation strings. ## Stage 8: Orchestrator Slimming +**Status: evaluated and descoped 2026-06-10.** Inspection refuted both +premises. (1) The helpers are not misplaced: `_tool_result_preview` and +`_format_tool_call_log_entry` are called only from inside `orchestrator.py` +(observation/log building at lines ~1081 and ~1467), and +`_unwrap_generated_file_body` is called by `_generate_file_body`, which +itself lives in the orchestrator — moving any of them would separate code +from its only caller. (2) The 14 constructor parameters are mostly optional +dependency-injection seams (policy engine, approval service, history store, +registry, callbacks) that default sensibly and that tests inject through; +bundling them into a frozen context object moves the same kwargs one level +down without removing any responsibility from the class. The real +god-object concern (≈40 methods spanning context gathering, deferred-write +generation, classification, and observation formatting) is a behavioral +redesign, not a mechanical move, and is out of scope for a hardening batch +per this stage's own "strictly mechanical" rule. Revisit only if/when the +deferred-write generation is extracted into its own service. + ### Goal `RequestOrchestrator` (`orchestrator.py`) takes 14 constructor parameters and @@ -412,6 +429,13 @@ concerns. Shrink it incrementally — no behavior change, no big-bang rewrite. ## Stage 9: Docs And Plans Hygiene +**Status: shipped 2026-06-10.** `fcli-fixes-roadmap.md` marked complete; +status headers added to the v0.1 roadmap, all five v2 stage plans, and the +v4 roadmap (v3 already had one); CHANGELOG gained an "Unreleased — hardening +batch" section covering stages 1–7 and the two bug fixes; TECHNICAL.md +documents the `write_truncated` degradation semantics, the sink +circuit-breaker, and pre-migration backups. + ### Goal Make `plans/` trustworthy again: a reader should be able to tell what is diff --git a/plans/v2/00-conversational-brain-and-persistent-sessions.md b/plans/v2/00-conversational-brain-and-persistent-sessions.md index a400287..8fdead9 100644 --- a/plans/v2/00-conversational-brain-and-persistent-sessions.md +++ b/plans/v2/00-conversational-brain-and-persistent-sessions.md @@ -1,5 +1,7 @@ # Stage 00: Conversational Brain and Persistent Sessions +**Status: shipped (v2 complete; see git history).** + ## Goal Turn `foundation chat` into a real terminal-first agent shell on top of the existing v1 runtime. This stage should let a user talk to Foundation continuously, use the current v1 tools and approvals during the conversation, and persist memory and session state across turns and restarts in a way that feels closer to modern terminal coding agents. diff --git a/plans/v2/01-capability-registry-and-local-store.md b/plans/v2/01-capability-registry-and-local-store.md index d00bab2..05ba6db 100644 --- a/plans/v2/01-capability-registry-and-local-store.md +++ b/plans/v2/01-capability-registry-and-local-store.md @@ -1,5 +1,7 @@ # Stage 1: Capability Registry and Local Store +**Status: shipped (v2 complete; see git history).** + ## Goal Replace the hardcoded tool surface with a first-class capability system that can represent built-in tools, future skills, and user-created extensions through one typed registry. This stage establishes the local store and metadata model that every later v2 policy, audit, and execution path will depend on. diff --git a/plans/v2/02-capability-policies-and-governance.md b/plans/v2/02-capability-policies-and-governance.md index 1b326fb..8ca68e4 100644 --- a/plans/v2/02-capability-policies-and-governance.md +++ b/plans/v2/02-capability-policies-and-governance.md @@ -1,5 +1,7 @@ # Stage 2: Capability Policies and Governance +**Status: shipped (v2 complete; see git history).** + ## Goal Move policy enforcement from shell-specific guardrails to a capability-wide governance layer. This stage ensures that every tool, skill, and shell-backed capability is evaluated through the same policy engine before execution, with explicit approval and audit behavior based on capability metadata and invocation context. diff --git a/plans/v2/03-trace-audit-and-runtime-split.md b/plans/v2/03-trace-audit-and-runtime-split.md index 9860eea..6b118d6 100644 --- a/plans/v2/03-trace-audit-and-runtime-split.md +++ b/plans/v2/03-trace-audit-and-runtime-split.md @@ -1,5 +1,7 @@ # Stage 3: Trace, Audit, and Runtime Split +**Status: shipped (v2 complete; see git history).** + ## Goal Split the runtime into explicit subsystems and persist a full causal trace for each request so users can inspect why a capability was chosen, why policy allowed or blocked it, and what side effects followed. This stage prioritizes audit-first traceability while storing enough detail to support future replay-oriented features without redesigning persistence later. diff --git a/plans/v2/04-quiet-chat-surface-and-audit-first-output.md b/plans/v2/04-quiet-chat-surface-and-audit-first-output.md index c710680..58a5ff5 100644 --- a/plans/v2/04-quiet-chat-surface-and-audit-first-output.md +++ b/plans/v2/04-quiet-chat-surface-and-audit-first-output.md @@ -1,5 +1,7 @@ # Stage 4: Quiet Chat Surface and Audit-First Output +**Status: shipped (v2 complete; see git history).** + ## Goal Make `foundation chat` feel like a normal terminal assistant: the user asks a question, the assistant answers, and the next prompt appears. Internal orchestration detail such as planned actions, policy tables, execution summaries, provider metadata, token counts, and raw structured log lines should stop dominating the default terminal view. Those details must still be preserved in logs, history, and audit surfaces. diff --git a/plans/v4/00-roadmap.md b/plans/v4/00-roadmap.md index 377a3cc..11013d8 100644 --- a/plans/v4/00-roadmap.md +++ b/plans/v4/00-roadmap.md @@ -1,5 +1,10 @@ # Foundation CLI v4 Roadmap +## Status + +**v4 complete** — all three stages shipped (see the stage table below); +verified 2026-05-27 with the full suite green. + ## Purpose v4 turns Foundation CLI from a fast-to-use coding agent into a **transparent, observable one**. The v3 runtime already has the event plumbing (22 `EVENT_*` constants across orchestrator, executor, shell, provider, observer) and a structured audit trail. What's missing is a live UX that tells the user what's happening *while* a turn runs, and an external stream so a separate monitoring tool — GUI or terminal — can subscribe to the agent's activity in real time. From 8ab664d41b2230258d4379cb0ec0e8cad66637e3 Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Wed, 10 Jun 2026 18:32:27 -0700 Subject: [PATCH 11/18] Fix MarkupError crash on model output in both chat renderers Found by the codex smoke test: an assistant message starting with a stray Rich closing tag (e.g. '[/bold]') crashed the concise renderer's console.print and the verbose Assistant panel with MarkupError after the turn had already completed. Model-generated text now renders literally via Text() in both paths, matching the earlier detail-panel fix. Co-Authored-By: Claude Fable 5 --- src/foundation/cli_rendering.py | 8 ++++++-- tests/test_cli.py | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/foundation/cli_rendering.py b/src/foundation/cli_rendering.py index 94543f5..7ca23f5 100644 --- a/src/foundation/cli_rendering.py +++ b/src/foundation/cli_rendering.py @@ -293,7 +293,9 @@ def _render_help_lookup(result: HelpLookupResult) -> None: def _render_assistant_message(result: OrchestrationResult) -> None: - console.print(Panel.fit(result.assistant_message.content, title="Assistant")) + # Text() keeps model-generated content literal — raw strings would be + # parsed as Rich markup (styling injection, MarkupError on stray tags). + console.print(Panel.fit(Text(result.assistant_message.content), title="Assistant")) def _chat_surface_policy(render_mode: RenderMode) -> ChatSurfacePolicy: @@ -789,7 +791,9 @@ def _render_concise_chat_turn( primary_text = presentation.primary_text if interactive: primary_text = _format_interactive_concise_text(primary_text) - console.print(primary_text) + # Text() keeps model-generated content literal — raw strings would be + # parsed as Rich markup (styling injection, MarkupError on stray tags). + console.print(Text(primary_text)) style_map = { PresentationNoticeLevel.INFO: "cyan", PresentationNoticeLevel.WARNING: "yellow", diff --git a/tests/test_cli.py b/tests/test_cli.py index 77911c9..ac82103 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2355,3 +2355,21 @@ def test_approval_required_notice_fires_only_for_pending_approval_stop() -> None stop_reason=LoopStopReason.ZERO_ACTION_PLAN, ) assert _approval_required_notice(completed) is None + + +def test_chat_rendering_keeps_model_markup_literal() -> None: + """Found by the codex smoke test (hardening batch): assistant text that + starts with a stray Rich closing tag crashed both chat renderers with + MarkupError. Model output must render literally. + """ + from foundation.cli_rendering import _render_chat_turn, console + from foundation.models import AssistantMessage + + result = _chat_result("say something").model_copy( + update={"assistant_message": AssistantMessage(content="[/bold]oops [red]x[/red]")} + ) + with console.capture() as capture: + _render_chat_turn(result, render_mode=RenderMode.CONCISE) + _render_chat_turn(result, render_mode=RenderMode.VERBOSE) + output = capture.get() + assert "[/bold]oops" in output From a5ed228cabe99f011f2fe369f4aa579445242f07 Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Wed, 10 Jun 2026 21:36:07 -0700 Subject: [PATCH 12/18] Fix non-TTY approval aborts and misleading recovery summaries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the codex smoke test. First, a piped one-shot run hitting an approval prompt died with a bare 'Aborted.' (exit 1) when typer.confirm got EOF; the prompt callback now returns None on non-TTY EOF and ApprovalService resolves the action as PENDING, producing the graceful loop stop. A TTY Ctrl-C still aborts. Second, a turn that failed, repaired itself, and completed naturally reported 'stopped: tool failed' plus 'no verification command ran': custom test scripts now classify as verification, cross-iteration verification is decided by the latest attempt instead of worst-wins, and the summary only uses stop framing for abnormal stops — recovered turns read 'Executed N actions, recovered from M earlier failures'. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 16 ++- src/foundation/cli_runtime.py | 20 +++- src/foundation/services/approval.py | 24 ++++- src/foundation/services/orchestrator.py | 35 +++++- tests/test_orchestrator.py | 137 ++++++++++++++++++++++++ tests/test_policy.py | 73 +++++++++++++ 6 files changed, 294 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 911990e..fb171fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,9 +45,19 @@ that were checked and rejected). - A plan naming a nonexistent capability id crashed the turn with an unwrapped `ValueError`; it now routes through the plan-repair retry. -- The live detail panel parsed the user's request text as Rich markup, - allowing styling injection and a `MarkupError` crash; it renders literally - now. +- Model-generated text was parsed as Rich markup in the live detail panel + and both chat renderers, allowing styling injection and a `MarkupError` + crash; it renders literally now. +- In piped, non-interactive runs an approval-gated action died with a bare + "Aborted." (exit 1) when the prompt hit EOF; it now resolves as + PENDING_APPROVAL with the graceful stop notice. A TTY user pressing + Ctrl-C at the prompt still aborts. +- A turn that failed, repaired itself, and completed no longer reports + "stopped: tool failed"; the summary reads "Executed N actions, recovered + from M earlier failure(s)". Custom test scripts (any command with "test" + in its basename, e.g. `./run_tests.sh`) now count as verification, and + the verification outcome across iterations is decided by the latest + attempt instead of worst-wins, so a repaired test run reports "passed". ### Added diff --git a/src/foundation/cli_runtime.py b/src/foundation/cli_runtime.py index 39e437f..ab3684e 100644 --- a/src/foundation/cli_runtime.py +++ b/src/foundation/cli_runtime.py @@ -4,6 +4,7 @@ import os import shlex +import sys import threading from contextlib import suppress from pathlib import Path @@ -107,7 +108,7 @@ def _build_session_manager(settings: AppSettings) -> SessionManager: ) -def _prompt_for_approval(request: ApprovalRequest) -> bool: +def _prompt_for_approval(request: ApprovalRequest) -> bool | None: risk_text = ", ".join(request.risk_categories) if request.risk_categories else "unknown" lines = [ f"Action: [cyan]{escape(request.action_id)}[/cyan]", @@ -159,7 +160,22 @@ def _prompt_for_approval(request: ApprovalRequest) -> bool: renderer.pause() try: console.print(Panel.fit(panel_text, title="Approval Required")) - return typer.confirm("Approve this action?", default=False) + try: + return typer.confirm("Approve this action?", default=False) + except click.Abort: + # A TTY user pressing Ctrl-C/Ctrl-D keeps aborting; EOF on piped + # stdin (one-shot non-interactive run) must not kill the turn — + # the action resolves as PENDING instead. + if sys.stdin.isatty(): + raise + console.print( + Text( + "No interactive input available to answer the approval prompt; " + "leaving the action pending approval.", + style="dim", + ) + ) + return None finally: if renderer is not None: renderer.resume() diff --git a/src/foundation/services/approval.py b/src/foundation/services/approval.py index 67d0c52..f0a6ea3 100644 --- a/src/foundation/services/approval.py +++ b/src/foundation/services/approval.py @@ -18,7 +18,9 @@ ) from foundation.settings import ApprovalMode -ApprovalPrompt = Callable[[ApprovalRequest], bool] +# Returns True/False for an explicit user decision, or None when no +# interactive input is available (the action then resolves as PENDING). +ApprovalPrompt = Callable[[ApprovalRequest], bool | None] def _utcnow() -> str: @@ -136,7 +138,25 @@ def resolve( requested_side_effects=list(request.requested_side_effects), ) - approved = bool(self._prompt_callback(request)) + answer = self._prompt_callback(request) + if answer is None: + return request, CapabilityApprovalResolution( + action_id=action.id, + capability_id=request.capability_id, + mode=self._mode.value, + status=ApprovalDecisionStatus.PENDING, + reason=( + "Approval is required but no interactive input was available; " + "re-run interactively to approve." + ), + requested_at=requested_at, + resolved_at=_utcnow(), + risk_categories=list(request.risk_categories), + reason_codes=list(request.reason_codes), + command_preview=request.command_preview, + requested_side_effects=list(request.requested_side_effects), + ) + approved = bool(answer) return request, CapabilityApprovalResolution( action_id=action.id, capability_id=request.capability_id, diff --git a/src/foundation/services/orchestrator.py b/src/foundation/services/orchestrator.py index 09f3d8f..9cf8b5b 100644 --- a/src/foundation/services/orchestrator.py +++ b/src/foundation/services/orchestrator.py @@ -158,6 +158,22 @@ def _worst_verification_outcome( return a +def _combine_iteration_verification( + previous: VerificationOutcome, + latest: VerificationOutcome, +) -> VerificationOutcome: + """Combine verification outcomes across iterations: the latest attempt wins. + + Each iteration is a repair attempt, so a later PASSED supersedes an earlier + FAILED (and vice versa). An iteration that attempted no verification keeps + the previous outcome. Within a single iteration worst-wins still applies — + see ``_classify_results``. + """ + if latest is VerificationOutcome.NOT_ATTEMPTED: + return previous + return latest + + _CODE_CHANGING_ARTIFACT_TYPES = frozenset( { ExecutionArtifactType.FILE_WRITE, @@ -1048,7 +1064,7 @@ def _run_replan_loop( execution_results, attempted_actions ) had_code_changes = had_code_changes or iter_code_change - verification_outcome = _worst_verification_outcome( + verification_outcome = _combine_iteration_verification( verification_outcome, iter_outcome, ) @@ -1595,7 +1611,9 @@ def _classify_results( if action.kind is ActionKind.SHELL and action.shell: cmd_basename = action.shell.command.split("/")[-1] - if cmd_basename in _VERIFICATION_COMMANDS: + # Custom test scripts (./run_tests.sh, scripts/test.py, …) + # count as verification alongside the known tool names. + if cmd_basename in _VERIFICATION_COMMANDS or "test" in cmd_basename: display = " ".join([action.shell.command, *action.shell.args]) verify_cmds.append(display) cmd_outcome = _verification_outcome_for_result(result) @@ -2103,7 +2121,11 @@ def _build_summary( if skipped: stop_parts.append(f"{skipped} skipped") - if blocked or failed: + # A ZERO_ACTION_PLAN stop means the loop completed naturally; + # failures along the way were repaired, not the stop cause. + completed_naturally = stop_reason is LoopStopReason.ZERO_ACTION_PLAN + use_stop_framing = (blocked or failed) and not completed_naturally + if use_stop_framing: cause = RequestOrchestrator._stop_cause_summary( execution_results, actions_by_id, @@ -2125,10 +2147,15 @@ def _build_summary( parts = [f"Executed {RequestOrchestrator._action_count(executed, 'action')}"] if pending: parts.append(RequestOrchestrator._approval_count(pending)) + if failed: + suffix = "" if failed == 1 else "s" + parts.append(f"recovered from {failed} earlier failure{suffix}") + if blocked: + parts.append(f"{blocked} blocked by policy") if skipped: parts.append(f"{skipped} skipped") text = ", ".join(parts) + "." - if len(iterations) > 1 and not (blocked or failed): + if len(iterations) > 1 and not use_stop_framing: text += f" ({len(iterations)} iterations)" return OrchestrationSummary( diff --git a/tests/test_orchestrator.py b/tests/test_orchestrator.py index 13c6667..0c5a1c4 100644 --- a/tests/test_orchestrator.py +++ b/tests/test_orchestrator.py @@ -4132,3 +4132,140 @@ def test_repeated_successful_file_read_stops_as_no_progress( if call.response_format is ProviderResponseFormat.JSON_OBJECT ] assert len(plan_calls) == 2 + + +def test_classify_results_recognizes_custom_test_scripts(tmp_path: Path) -> None: + """Codex smoke finding: ./run_tests.sh was not counted as verification.""" + from foundation.models import ( + ActionKind, + ExecutionResult, + ExecutionStatus, + PlannedAction, + ShellAction, + VerificationOutcome, + ) + from foundation.services.orchestrator import RequestOrchestrator + + action = PlannedAction( + id="v1", + kind=ActionKind.SHELL, + summary="run the project test script", + shell=ShellAction(command="./run_tests.sh"), + ) + result = ExecutionResult(action_id="v1", status=ExecutionStatus.EXECUTED, summary="ok") + + _changed, _code, outcome, commands = RequestOrchestrator._classify_results([result], [action]) + + assert commands == ["./run_tests.sh"] + assert outcome is VerificationOutcome.PASSED + + +def test_latest_iteration_verification_wins_across_iterations() -> None: + """Codex smoke finding: an early failing test run masked the later pass.""" + from foundation.models import VerificationOutcome + from foundation.services.orchestrator import _combine_iteration_verification + + failed = VerificationOutcome.FAILED + passed = VerificationOutcome.PASSED + not_attempted = VerificationOutcome.NOT_ATTEMPTED + + assert _combine_iteration_verification(failed, passed) is passed + assert _combine_iteration_verification(passed, failed) is failed + assert _combine_iteration_verification(failed, not_attempted) is failed + assert _combine_iteration_verification(not_attempted, passed) is passed + + +def _summary_fixtures(tmp_path: Path): + from foundation.models import ( + ActionKind, + AssistantPlan, + ContextSnapshot, + ExecutionResult, + ExecutionStatus, + OrchestrationIteration, + PlannedAction, + ProviderResponseMetadata, + ShellAction, + ToolCall, + ) + + context = ContextSnapshot( + workspace_root=str(tmp_path), + request_cwd=str(tmp_path), + approval_mode="prompt", + ) + metadata = ProviderResponseMetadata(provider="stub", model="stub", latency_seconds=0.0) + run_tests = PlannedAction( + id="t1", + kind=ActionKind.SHELL, + summary="run tests", + shell=ShellAction(command="./run_tests.sh"), + ) + fix = PlannedAction( + id="f1", + kind=ActionKind.TOOL_CALL, + summary="fix the bug", + tool_call=ToolCall(capability_id="foundation.file.edit", arguments={}), + ) + rerun = PlannedAction( + id="t2", + kind=ActionKind.SHELL, + summary="re-run tests", + shell=ShellAction(command="./run_tests.sh"), + ) + failed = ExecutionResult( + action_id="t1", status=ExecutionStatus.FAILED, summary="tests failed", error="exit 1" + ) + fixed = ExecutionResult(action_id="f1", status=ExecutionStatus.EXECUTED, summary="edited") + passed = ExecutionResult(action_id="t2", status=ExecutionStatus.EXECUTED, summary="passed") + iterations = [ + OrchestrationIteration( + iteration=1, + context=context, + plan=AssistantPlan(assistant_message="try the tests", actions=[run_tests]), + planning_metadata=metadata, + execution_results=[failed], + ), + OrchestrationIteration( + iteration=2, + context=context, + plan=AssistantPlan(assistant_message="fix and re-run", actions=[fix, rerun]), + planning_metadata=metadata, + execution_results=[fixed, passed], + ), + ] + return iterations, [failed, fixed, passed] + + +def test_summary_does_not_blame_recovered_failures_on_natural_completion( + tmp_path: Path, +) -> None: + """Codex smoke finding: a successful turn said 'stopped: tool failed'.""" + from foundation.models import LoopStopReason + from foundation.services.orchestrator import RequestOrchestrator + + iterations, results = _summary_fixtures(tmp_path) + summary = RequestOrchestrator._build_summary( + iterations, + results, + plan_only=False, + stop_reason=LoopStopReason.ZERO_ACTION_PLAN, + ) + assert "stopped" not in summary.text.lower() + assert summary.text.startswith("Executed 2") + assert "recovered" in summary.text + assert "(2 iterations)" in summary.text + + +def test_summary_keeps_stop_framing_for_abnormal_stops(tmp_path: Path) -> None: + from foundation.models import LoopStopReason + from foundation.services.orchestrator import RequestOrchestrator + + iterations, results = _summary_fixtures(tmp_path) + summary = RequestOrchestrator._build_summary( + iterations, + results, + plan_only=False, + stop_reason=LoopStopReason.FATAL_EXECUTION_FAILURE, + ) + assert "stopped" in summary.text.lower() diff --git a/tests/test_policy.py b/tests/test_policy.py index fcbe794..a5d9a15 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -513,3 +513,76 @@ def test_git_mutation_subcommands_have_single_source_of_truth() -> None: assert planner._GIT_MUTATION_SUBCOMMANDS is GIT_MUTATION_SUBCOMMANDS assert guardrails._WRITE_GIT_SUBCOMMANDS is GIT_MUTATION_SUBCOMMANDS + + +def test_prompt_callback_none_answer_resolves_pending( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Codex smoke finding: EOF at the approval prompt must mean PENDING.""" + registry, workspace_root = _registry(tmp_path, monkeypatch) + engine = CapabilityPolicyEngine( + workspace_root=workspace_root, + capability_registry=registry, + ) + action = PlannedAction( + id="touch_file", + kind=ActionKind.SHELL, + summary="Create a file", + shell=ShellAction(command="touch", args=["note.txt"]), + ) + evaluation = engine.evaluate( + action, + request_cwd=workspace_root, + approval_mode=ApprovalMode.PROMPT, + ) + assert evaluation is not None + + from foundation.models import ApprovalDecisionStatus + + service = ApprovalService(mode=ApprovalMode.PROMPT, prompt_callback=lambda request: None) + _request, resolution = service.resolve(action, evaluation, request_cwd=workspace_root) + + assert resolution.status is ApprovalDecisionStatus.PENDING + assert "no interactive input" in resolution.reason.lower() + + +def test_prompt_for_approval_with_eof_stdin_returns_none( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Non-TTY EOF (piped one-shot run) must not abort the whole turn.""" + import io + import sys as sys_module + + from foundation.cli_runtime import _prompt_for_approval + + request = _make_approval_request( + capability_id="foundation.shell.command", + side_effects=["subprocess"], + ) + monkeypatch.setattr(sys_module, "stdin", io.StringIO()) + assert _prompt_for_approval(request) is None + + +def test_prompt_for_approval_tty_abort_still_raises( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A real user pressing Ctrl-C/Ctrl-D at a TTY prompt keeps aborting.""" + import io + import sys as sys_module + + import click + + from foundation.cli_runtime import _prompt_for_approval + + class _TtyEof(io.StringIO): + def isatty(self) -> bool: + return True + + request = _make_approval_request( + capability_id="foundation.shell.command", + side_effects=["subprocess"], + ) + monkeypatch.setattr(sys_module, "stdin", _TtyEof()) + with pytest.raises(click.Abort): + _prompt_for_approval(request) From e03d417cf033896dad91f63b000a8cb0d303dbef Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Thu, 11 Jun 2026 00:36:05 -0700 Subject: [PATCH 13/18] Stop the live keypress reader from eating prompt input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a real TTY the live status line's '?'-toggle reader kept its byte-at-a-time stdin loop running while approval/question prompts were shown — renderer.pause() only stopped the Rich Live widget. The reader raced typer.confirm for every byte, swallowed the user's 'y', and the prompt resolved to its default 'n', blocking the action and stopping the turn. pause() now tears down the reader (thread stopped, termios restored, type-ahead flushed) and resume() reinstalls it. Pinned by a pty-backed regression test. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 7 ++++++ src/foundation/live_turn.py | 21 ++++++++++++++-- tests/test_live_turn.py | 48 +++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb171fd..2c0b080 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,13 @@ that were checked and rejected). "Aborted." (exit 1) when the prompt hit EOF; it now resolves as PENDING_APPROVAL with the graceful stop notice. A TTY user pressing Ctrl-C at the prompt still aborts. +- Typing "y" at an interactive approval prompt no longer resolves as "n": + the live status line's `?`-toggle keypress reader kept reading stdin + byte-by-byte during prompts, eating the user's answer so `typer.confirm` + saw only the Enter and fell back to the default. `pause()` now stops the + reader thread, restores canonical terminal mode, and flushes type-ahead; + `resume()` reinstalls the reader. Affected every mid-turn prompt + (approvals, agent questions, out-of-scope read grants) on a real TTY. - A turn that failed, repaired itself, and completed no longer reports "stopped: tool failed"; the summary reads "Executed N actions, recovered from M earlier failure(s)". Custom test scripts (any command with "test" diff --git a/src/foundation/live_turn.py b/src/foundation/live_turn.py index e801802..957d7e6 100644 --- a/src/foundation/live_turn.py +++ b/src/foundation/live_turn.py @@ -536,7 +536,14 @@ def drain_until_finished( self.tick() def pause(self) -> None: - """Stop the Live widget so other prompts can render normally.""" + """Stop the Live widget and fully release stdin for other prompts. + + The keypress reader competes with ``input()`` for stdin bytes — left + running, it eats the user's answer at approval/question prompts and + the prompt resolves to its default. Pausing must stop the reader + thread, restore canonical terminal mode, and flush any type-ahead + that the raw-mode reader may have left behind. + """ if self._live is None or self._paused: return self._paused = True @@ -544,12 +551,22 @@ def pause(self) -> None: self._live.stop() except Exception: # pragma: no cover - defensive pass + stdin_fd = self._stdin_fd + self._teardown_keypress_reader() + if stdin_fd is not None: + try: + import termios + + termios.tcflush(stdin_fd, termios.TCIFLUSH) + except Exception: # pragma: no cover - defensive + pass def resume(self) -> None: - """Re-enter the Live widget after a paused prompt.""" + """Re-enter the Live widget (and the keypress reader) after a prompt.""" if self._live is None or not self._paused: return self._paused = False + self._install_keypress_reader() try: self._live.start(refresh=True) except Exception: # pragma: no cover - defensive diff --git a/tests/test_live_turn.py b/tests/test_live_turn.py index 8d4992e..74d2b17 100644 --- a/tests/test_live_turn.py +++ b/tests/test_live_turn.py @@ -504,3 +504,51 @@ def test_detail_panel_request_text_is_not_parsed_as_markup(): state_malformed = TurnLiveState(request_text="[/bold]oops") text = _render_to_text(render_detail_panel(state_malformed, elapsed_seconds=0.1)) assert "oops" in text + + +def test_pause_releases_stdin_and_resume_reinstalls_keypress_reader(monkeypatch): + """Approval prompts and the '?'-toggle reader must not compete for stdin. + + Found live: with the keypress thread still running during pause(), the + user's 'y' at the approval prompt was eaten byte-by-byte by the reader + and typer.confirm saw only the Enter, resolving to the default 'n'. + pause() must fully release stdin (thread stopped, termios restored); + resume() must reinstall the reader. + """ + import os + import pty + import sys + import termios + + from foundation import live_turn + + master_fd, slave_fd = pty.openpty() + + class _PtyStdin: + def fileno(self) -> int: + return slave_fd + + def isatty(self) -> bool: + return True + + monkeypatch.setattr(sys, "stdin", _PtyStdin()) + console = Console(file=io.StringIO(), force_terminal=True, width=80) + renderer = live_turn.LiveTurnRenderer(console=console, enable_keypress=True) + try: + with renderer: + assert renderer._keypress_thread is not None + running_lflag = termios.tcgetattr(slave_fd)[3] + assert not (running_lflag & termios.ICANON) # cbreak active + + renderer.pause() + assert renderer._keypress_thread is None + paused_lflag = termios.tcgetattr(slave_fd)[3] + assert paused_lflag & termios.ICANON # canonical mode restored + + renderer.resume() + assert renderer._keypress_thread is not None + resumed_lflag = termios.tcgetattr(slave_fd)[3] + assert not (resumed_lflag & termios.ICANON) # reader reinstalled + finally: + os.close(master_fd) + os.close(slave_fd) From 447ee0720f1ebf0e43bed1b625b227eeae24389c Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Thu, 11 Jun 2026 01:51:33 -0700 Subject: [PATCH 14/18] Add headless worker mode speaking the agent-task-contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit foundation run --headless --task-file task.json --out result.json drives the existing replan loop noninteractively: contract task envelope in, stamped NDJSON event stream (/.events/.ndjson) and CodingWorkerResult out. Approval mode is forced to manual so approval- requiring actions terminate as pending_approval without ever touching stdin; version skew rejects via task.rejected; budgets map onto new additive orchestrator bounds (max_loop_iterations/max_total_actions, defaults unchanged); a 10s heartbeat thread reports the current phase; the patch artifact comes from git add -N + git diff with .events/ .artifacts excluded — no commit, no push, ever, in headless mode. agent-task-contract is a local path dependency until published. Verified: ruff check/format clean, mypy src clean (45 files), full suite 554 passed (7 new headless tests; zero regressions). Co-Authored-By: Claude Fable 5 --- pyproject.toml | 4 + src/foundation/cli.py | 31 ++ src/foundation/headless.py | 596 ++++++++++++++++++++++++ src/foundation/services/orchestrator.py | 18 +- tests/test_headless.py | 373 +++++++++++++++ uv.lock | 20 + 6 files changed, 1035 insertions(+), 7 deletions(-) create mode 100644 src/foundation/headless.py create mode 100644 tests/test_headless.py diff --git a/pyproject.toml b/pyproject.toml index 4b40271..05231d3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,6 +31,7 @@ dependencies = [ "prompt-toolkit>=3.0,<4.0", "rich>=13.7,<14.0", "typer>=0.12,<1.0", + "agent-task-contract", ] [project.optional-dependencies] @@ -91,3 +92,6 @@ strict = true warn_unused_configs = true mypy_path = ["src"] packages = ["foundation"] + +[tool.uv.sources] +agent-task-contract = { path = "../agent-task-contract", editable = true } diff --git a/src/foundation/cli.py b/src/foundation/cli.py index 569b133..8e36d0e 100644 --- a/src/foundation/cli.py +++ b/src/foundation/cli.py @@ -446,9 +446,40 @@ def run( help="Set NAME=VALUE in the command environment. May be repeated.", ), ] = None, + headless: Annotated[ + bool, + typer.Option( + "--headless", + help="Run one contract task noninteractively (worker mode).", + ), + ] = False, + task_file: Annotated[ + Path | None, + typer.Option( + "--task-file", + help="Path to the contract task.json envelope (requires --headless).", + ), + ] = None, + out: Annotated[ + Path | None, + typer.Option( + "--out", + help="Path to write the contract result.json (requires --headless).", + ), + ] = None, ) -> None: """Execute a shell command inside the configured workspace.""" settings = _load_runtime_settings(ctx) + if headless or task_file is not None or out is not None: + if not headless or task_file is None or out is None: + console.print( + "[bold red]Execution error:[/bold red] headless mode requires " + "--headless, --task-file, and --out together." + ) + raise typer.Exit(code=2) + from foundation.headless import run_headless_task + + raise typer.Exit(code=run_headless_task(task_file, out, settings=settings)) history_store = _build_history_store(settings) command_argv = list(ctx.args) if command_argv and command_argv[0] == "--": diff --git a/src/foundation/headless.py b/src/foundation/headless.py new file mode 100644 index 0000000..c281e3a --- /dev/null +++ b/src/foundation/headless.py @@ -0,0 +1,596 @@ +"""Headless worker mode: contract task.json in, NDJSON events + result.json out. + +Implements the worker side of the agent-task-contract v0.1 spec: the existing +plan→execute→observe→replan loop is driven from a ``CodingWorkerTask`` envelope, +every observer event is stamped onto the contract event stream at +``/.events/.ndjson``, and a ``CodingWorkerResult`` is written +on exit. Headless mode never prompts a terminal: approval-requiring actions stay +pending and the run terminates with status ``pending_approval``; the worker never +commits and never pushes. +""" + +from __future__ import annotations + +import hashlib +import json +import os +import subprocess +import threading +import uuid +from collections.abc import Mapping +from datetime import UTC, datetime +from pathlib import Path +from typing import Any + +from agent_task_contract import ( + CONTRACT_VERSION, + Artifact, + CodingWorkerResult, + CodingWorkerTask, + CommandRecord, + EventType, + TaskState, + Verification, + check_supported, +) +from agent_task_contract import ( + VerificationOutcome as ContractVerificationOutcome, +) +from pydantic import ValidationError + +from foundation import __version__ as WORKER_VERSION +from foundation.models import LoopStopReason, OrchestrationResult, UserRequest +from foundation.observability import ( + EVENT_APPROVAL_REQUESTED, + EVENT_ITERATION_COMPLETED, + EVENT_PLAN_FINISHED, + EVENT_PLAN_STARTED, + EVENT_SHELL_EXECUTION_FAILED, + EVENT_SHELL_EXECUTION_FINISHED, + EVENT_SHELL_EXECUTION_STARTED, + EVENT_TOOL_EXECUTION_STARTED, +) +from foundation.services import LocalToolService, ShellRuntime +from foundation.services.capabilities import CapabilityRegistry, CapabilityStore +from foundation.services.history import TraceStore +from foundation.services.orchestrator import RequestOrchestrator +from foundation.services.provider import ProviderAdapter, build_provider_adapter +from foundation.settings import ApprovalMode, AppSettings + +SUPPORTED_CONTRACT_RANGE = ">=0.1,<0.2" +DEFAULT_HEARTBEAT_SECONDS = 10.0 + +# Exit codes mirror the terminal state (plan Stage 4 relies on this mapping). +EXIT_COMPLETED = 0 +EXIT_INVOCATION_ERROR = 2 +EXIT_FAILED = 3 +EXIT_PENDING_APPROVAL = 4 +EXIT_REJECTED = 5 + +_EXIT_BY_STATUS: dict[TaskState, int] = { + TaskState.COMPLETED: EXIT_COMPLETED, + TaskState.FAILED: EXIT_FAILED, + TaskState.PENDING_APPROVAL: EXIT_PENDING_APPROVAL, + TaskState.REJECTED: EXIT_REJECTED, +} + + +def _utc_now_rfc3339() -> str: + return datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%SZ") + + +def _sha256_file(path: Path) -> str: + digest = hashlib.sha256() + with path.open("rb") as handle: + for chunk in iter(lambda: handle.read(65536), b""): + digest.update(chunk) + return digest.hexdigest() + + +class ContractEventStream: + """Thread-safe, contract-stamped NDJSON event writer (spec §4).""" + + def __init__(self, path: Path, *, task_id: str, trace_id: str) -> None: + self.path = path + self._task_id = task_id + self._trace_id = trace_id + self._seq = 0 + self._lock = threading.Lock() + path.parent.mkdir(parents=True, exist_ok=True) + # Truncate any stale stream from a previous run of the same task id. + path.write_text("") + + def emit(self, event_type: EventType, payload: dict[str, Any]) -> None: + with self._lock: + self._seq += 1 + envelope = { + "contract_version": CONTRACT_VERSION, + "event_id": str(uuid.uuid4()), + "seq": self._seq, + "task_id": self._task_id, + "trace_id": self._trace_id, + "ts": _utc_now_rfc3339(), + "type": event_type.value, + "payload": payload, + } + with self.path.open("a", encoding="utf-8") as handle: + handle.write(json.dumps(envelope, ensure_ascii=True) + "\n") + handle.flush() + os.fsync(handle.fileno()) + + +class ContractEventSink: + """Map fcli observer events onto the contract event stream. + + Receives payloads already passed through the observability redaction + pipeline (ObserverService redacts before dispatching to sinks), so + redaction is preserved as-is. + """ + + def __init__(self, stream: ContractEventStream) -> None: + self._stream = stream + self.phase = "starting" + self._command_purposes: dict[str, str] = {} + self.commands: list[CommandRecord] = [] + self._lock = threading.Lock() + + def __call__(self, event_name: str, payload: Mapping[str, Any]) -> None: + if event_name == EVENT_PLAN_STARTED: + self.phase = "planning" + elif event_name in (EVENT_SHELL_EXECUTION_STARTED, EVENT_TOOL_EXECUTION_STARTED): + self.phase = "executing" + elif event_name == EVENT_ITERATION_COMPLETED: + self.phase = "observing" + + if event_name == EVENT_PLAN_FINISHED: + iteration = payload.get("iteration") + action_count = payload.get("action_count") + self._stream.emit( + EventType.PLAN_CREATED, + {"steps": [f"iteration {iteration}: {action_count} action(s) planned"]}, + ) + elif event_name == EVENT_SHELL_EXECUTION_STARTED: + command = str(payload.get("command_preview", "")) + action_id = str(payload.get("action_id", "")) + purpose = f"shell action {action_id}" + with self._lock: + self._command_purposes[action_id] = purpose + self._stream.emit( + EventType.COMMAND_START, + {"command": command, "purpose": purpose}, + ) + elif event_name in (EVENT_SHELL_EXECUTION_FINISHED, EVENT_SHELL_EXECUTION_FAILED): + action_id = str(payload.get("action_id", "")) + command = str(payload.get("command_preview", "")) + exit_code = int(payload.get("exit_code") or 0) + duration_seconds = float(payload.get("duration_seconds") or 0.0) + record = CommandRecord( + command=command, + exit_code=exit_code, + purpose=self._command_purposes.get(action_id, f"shell action {action_id}"), + ) + with self._lock: + self.commands.append(record) + self._stream.emit( + EventType.COMMAND_RESULT, + { + "command": command, + "exit_code": exit_code, + "duration_ms": int(duration_seconds * 1000), + "stdout_tail": str(payload.get("stdout_preview") or ""), + "stderr_tail": str(payload.get("stderr_preview") or ""), + }, + ) + elif event_name == EVENT_APPROVAL_REQUESTED: + action_id = str(payload.get("action_id", "")) + risk = ", ".join(str(r) for r in payload.get("risk_categories", []) or []) + self._stream.emit( + EventType.APPROVAL_REQUESTED, + { + "action": action_id, + "reason": f"approval required (risk: {risk or 'unspecified'}); " + "headless mode never prompts — stopping as pending_approval", + }, + ) + + +class _Heartbeat: + """Emit a contract heartbeat with the current phase every N seconds (Q3).""" + + def __init__( + self, + stream: ContractEventStream, + sink: ContractEventSink, + interval_seconds: float, + ) -> None: + self._stream = stream + self._sink = sink + self._interval = interval_seconds + self._stop = threading.Event() + self._thread = threading.Thread(target=self._run, name="contract-heartbeat", daemon=True) + + def _run(self) -> None: + while not self._stop.wait(self._interval): + self._stream.emit(EventType.HEARTBEAT, {"phase": self._sink.phase}) + + def start(self) -> None: + self._thread.start() + + def stop(self) -> None: + self._stop.set() + + +def _git(workspace: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", "-C", str(workspace), *args], + capture_output=True, + text=True, + check=False, + ) + + +def _is_git_repo(workspace: Path) -> bool: + return _git(workspace, "rev-parse", "--is-inside-work-tree").returncode == 0 + + +def _collect_patch(workspace: Path, task_id: str) -> tuple[Artifact | None, list[str]]: + """Write the patch artifact (Q5) and return it plus the changed-file list. + + ``git add -N`` records intent-to-add for untracked files so the diff covers + them; nothing is ever committed or pushed in headless mode. + """ + if not _is_git_repo(workspace): + return None, [] + # Contract bookkeeping (.events/, .artifacts/) must never appear in the patch. + excludes = (":(exclude).events", ":(exclude).artifacts") + _git(workspace, "add", "-N", "--", ".", *excludes) + changed_proc = _git(workspace, "diff", "--name-only", "--", ".", *excludes) + changed_files = [line for line in changed_proc.stdout.splitlines() if line.strip()] + diff_proc = _git(workspace, "diff", "--", ".", *excludes) + if not diff_proc.stdout.strip(): + return None, changed_files + artifacts_dir = workspace / ".artifacts" + artifacts_dir.mkdir(parents=True, exist_ok=True) + patch_path = artifacts_dir / f"{task_id}.patch" + patch_path.write_text(diff_proc.stdout, encoding="utf-8") + artifact = Artifact( + kind="patch", + path=str(patch_path.relative_to(workspace)), + sha256=_sha256_file(patch_path), + ) + return artifact, changed_files + + +def _registry_manifest_fingerprint(registry: CapabilityRegistry) -> str: + """SHA-256 over the full capability-manifest set (G7 surfacing).""" + digest = hashlib.sha256() + manifests = sorted( + registry.list_capabilities(), + key=lambda manifest: (str(manifest.capability_id), str(manifest.version)), + ) + for manifest in manifests: + payload = json.dumps( + manifest.model_dump(mode="json"), + ensure_ascii=True, + sort_keys=True, + ).encode("utf-8") + digest.update(payload) + return digest.hexdigest() + + +class _Finalizer: + """Single-shot guard so the deadline thread and the main path never both finalize.""" + + def __init__(self) -> None: + self._lock = threading.Lock() + self._done = False + + def claim(self) -> bool: + with self._lock: + if self._done: + return False + self._done = True + return True + + +def _status_for(result: OrchestrationResult) -> TaskState: + if result.stop_reason is LoopStopReason.PENDING_APPROVAL: + return TaskState.PENDING_APPROVAL + notice = result.verification_notice + verified = notice is not None and notice.outcome.value == "passed" + if verified and result.stop_reason is LoopStopReason.ZERO_ACTION_PLAN: + return TaskState.COMPLETED + return TaskState.FAILED + + +def _verification_for(result: OrchestrationResult) -> Verification: + notice = result.verification_notice + if notice is None: + return Verification( + outcome=ContractVerificationOutcome.NOT_ATTEMPTED, + details="no verification notice produced", + ) + commands = ", ".join(notice.verification_commands_run) or "none" + details = notice.reason or f"verification commands run: {commands}" + # fcli's taxonomy maps 1:1 onto the contract's (Keep List #5). + return Verification( + outcome=ContractVerificationOutcome(notice.outcome.value), + details=details, + ) + + +def _write_result(out_path: Path, result: CodingWorkerResult) -> None: + out_path.parent.mkdir(parents=True, exist_ok=True) + out_path.write_text(result.model_dump_json(indent=2) + "\n", encoding="utf-8") + + +def _finalize( + *, + task: CodingWorkerTask, + out_path: Path, + stream: ContractEventStream, + sink: ContractEventSink, + status: TaskState, + summary: str, + verification: Verification, + changed_files: list[str], + extra_artifacts: list[Artifact], + terminal_reason: str | None = None, +) -> int: + workspace = Path(task.workspace) + terminal_payload: dict[str, Any] = {"status": status.value, "summary": summary} + if terminal_reason is not None: + terminal_payload["reason"] = terminal_reason + stream.emit(EventType.TASK_TERMINAL, terminal_payload) + event_log = Artifact( + kind="event_log", + path=str(stream.path.relative_to(workspace)) + if stream.path.is_relative_to(workspace) + else str(stream.path), + sha256=_sha256_file(stream.path), + ) + result = CodingWorkerResult( + contract_version=CONTRACT_VERSION, + task_id=task.task_id, + trace_id=task.trace_id, + status=status, + summary=summary, + changed_files=changed_files, + commands=list(sink.commands), + verification=verification, + artifacts=[event_log, *extra_artifacts], + ) + _write_result(out_path, result) + return _EXIT_BY_STATUS.get(status, EXIT_FAILED) + + +def run_headless_task( + task_path: Path, + out_path: Path, + *, + settings: AppSettings, + provider: ProviderAdapter | None = None, + heartbeat_seconds: float = DEFAULT_HEARTBEAT_SECONDS, +) -> int: + """Run one contract task end-to-end. Returns the process exit code.""" + try: + raw_text = task_path.read_text(encoding="utf-8") + raw = json.loads(raw_text) + except (OSError, json.JSONDecodeError) as exc: + print( + f"headless: cannot read task file {task_path}: {exc}. " + "Remediation: pass --task-file pointing at a contract task.json.", + flush=True, + ) + return EXIT_INVOCATION_ERROR + + task_id = str(raw.get("task_id") or "") + trace_id = str(raw.get("trace_id") or "") + workspace_raw = str(raw.get("workspace") or "") + if not task_id or not trace_id or not workspace_raw: + print( + "headless: task envelope is missing task_id/trace_id/workspace; " + "cannot open an event stream. Remediation: fix the dispatching supervisor.", + flush=True, + ) + return EXIT_INVOCATION_ERROR + + workspace = Path(workspace_raw).expanduser() + if not workspace.is_dir(): + print( + f"headless: workspace {workspace} does not exist or is not a directory. " + "Remediation: the supervisor must create the worktree before dispatch.", + flush=True, + ) + return EXIT_INVOCATION_ERROR + + stream = ContractEventStream( + workspace / ".events" / f"{task_id}.ndjson", + task_id=task_id, + trace_id=trace_id, + ) + sink = ContractEventSink(stream) + + def _reject(reason: str) -> int: + stream.emit( + EventType.TASK_REJECTED, + { + "reason": reason, + "worker_version": WORKER_VERSION, + "supported_range": SUPPORTED_CONTRACT_RANGE, + }, + ) + stream.emit( + EventType.TASK_TERMINAL, + {"status": TaskState.REJECTED.value, "summary": reason, "reason": "rejected"}, + ) + result = CodingWorkerResult( + contract_version=CONTRACT_VERSION, + task_id=task_id, + trace_id=trace_id, + status=TaskState.REJECTED, + summary=reason, + changed_files=[], + commands=[], + verification=Verification( + outcome=ContractVerificationOutcome.NOT_ATTEMPTED, + details="task rejected before execution", + ), + artifacts=[ + Artifact( + kind="event_log", + path=str(stream.path.relative_to(workspace)), + sha256=_sha256_file(stream.path), + ) + ], + ) + _write_result(out_path, result) + return EXIT_REJECTED + + task_version = str(raw.get("contract_version") or "") + try: + skew = check_supported(task_version, SUPPORTED_CONTRACT_RANGE) + except ValueError: + return _reject( + f"task contract_version {task_version!r} is not a valid semver string; " + f"worker supports {SUPPORTED_CONTRACT_RANGE}." + ) + if skew is not None: + return _reject(str(skew)) + + try: + task = CodingWorkerTask.model_validate(raw) + except ValidationError as exc: + return _reject(f"task envelope failed validation: {exc}") + + workspace = Path(task.workspace).expanduser().resolve() + tool_service = LocalToolService( + workspace_root=workspace, + default_timeout_seconds=min(settings.shell.default_timeout_seconds, 30), + capture_limit_kb=settings.shell.capture_limit_kb, + pass_through_foundation_env=settings.shell.pass_through_foundation_env, + ) + shell_runtime = ShellRuntime( + workspace_root=workspace, + default_timeout_seconds=settings.shell.default_timeout_seconds, + max_timeout_seconds=settings.shell.max_timeout_seconds, + allow_pty=False, + capture_limit_kb=settings.shell.capture_limit_kb, + enforce_workspace_boundary=True, + ) + capability_registry = CapabilityRegistry( + store=CapabilityStore(settings.app.data_dir / "capabilities"), + tool_service=tool_service, + ) + history_store = TraceStore( + database_path=settings.history.database_path, + retention_days=settings.history.retention_days, + max_entries=settings.history.max_entries, + ) + resolved_provider = provider if provider is not None else build_provider_adapter(settings) + + orchestrator = RequestOrchestrator( + workspace_root=workspace, + approval_mode=ApprovalMode.MANUAL, + provider=resolved_provider, + shell_runtime=shell_runtime, + tool_service=tool_service, + history_store=history_store, + capability_registry=capability_registry, + event_sink=sink, + question_callback=None, + max_loop_iterations=task.budget.max_iterations, + max_total_actions=task.budget.max_actions, + ) + + stream.emit( + EventType.TASK_START, + { + "worker_version": WORKER_VERSION, + "manifest_fingerprint": _registry_manifest_fingerprint(capability_registry), + }, + ) + + finalizer = _Finalizer() + heartbeat = _Heartbeat(stream, sink, heartbeat_seconds) + heartbeat.start() + + def _self_deadline() -> None: + # Defense-in-depth (Q3/Q4): the supervisor backstop also enforces this. + if not finalizer.claim(): + return + heartbeat.stop() + exit_code = _finalize( + task=task, + out_path=out_path, + stream=stream, + sink=sink, + status=TaskState.FAILED, + summary=( + f"budget wall_clock_seconds={task.budget.wall_clock_seconds} exceeded; " + "worker self-terminated" + ), + verification=Verification( + outcome=ContractVerificationOutcome.NOT_ATTEMPTED, + details="wall-clock budget exceeded before verification", + ), + changed_files=[], + extra_artifacts=[], + terminal_reason="wall_clock_exceeded", + ) + os._exit(exit_code) + + deadline = threading.Timer(float(task.budget.wall_clock_seconds), _self_deadline) + deadline.daemon = True + deadline.start() + + try: + request = UserRequest(message=task.instructions, cwd=workspace) + result = orchestrator.orchestrate(request) + except Exception as exc: # noqa: BLE001 — any crash must still produce evidence + deadline.cancel() + if not finalizer.claim(): + return EXIT_FAILED + heartbeat.stop() + return _finalize( + task=task, + out_path=out_path, + stream=stream, + sink=sink, + status=TaskState.FAILED, + summary=f"worker error: {exc}", + verification=Verification( + outcome=ContractVerificationOutcome.NOT_ATTEMPTED, + details="run aborted before verification", + ), + changed_files=[], + extra_artifacts=[], + terminal_reason="exception", + ) + + deadline.cancel() + if not finalizer.claim(): + return EXIT_FAILED + heartbeat.stop() + sink.phase = "finishing" + + verification = _verification_for(result) + stream.emit( + EventType.VERIFY_RESULT, + {"outcome": verification.outcome.value, "details": verification.details}, + ) + + patch_artifact, changed_files = _collect_patch(workspace, task.task_id) + status = _status_for(result) + extra_artifacts = [patch_artifact] if patch_artifact is not None else [] + return _finalize( + task=task, + out_path=out_path, + stream=stream, + sink=sink, + status=status, + summary=result.assistant_message.content, + verification=verification, + changed_files=changed_files, + extra_artifacts=extra_artifacts, + ) diff --git a/src/foundation/services/orchestrator.py b/src/foundation/services/orchestrator.py index 9cf8b5b..ce1efc7 100644 --- a/src/foundation/services/orchestrator.py +++ b/src/foundation/services/orchestrator.py @@ -594,9 +594,13 @@ def __init__( max_plan_attempts: int = 2, event_sink: EventSink | None = None, question_callback: Callable[[QuestionAction], str | None] | None = None, + max_loop_iterations: int = _MAX_LOOP_ITERATIONS, + max_total_actions: int = _MAX_TOTAL_ACTIONS, ) -> None: self._workspace_root = Path(workspace_root).expanduser().resolve() self._approval_mode = approval_mode + self._max_loop_iterations = max_loop_iterations + self._max_total_actions = max_total_actions self._provider = provider self._shell_runtime = shell_runtime self._tool_service = tool_service @@ -905,7 +909,7 @@ def _run_replan_loop( progress_detector = NoProgressDetector() prev_last_step_id: str | None = None - for iteration_index in range(1, _MAX_LOOP_ITERATIONS + 1): + for iteration_index in range(1, self._max_loop_iterations + 1): self._observer.emit( EVENT_ITERATION_STARTED, payload={ @@ -921,7 +925,7 @@ def _run_replan_loop( context = self._planner.gather_context(request_cwd=str(resolved_request_cwd)) # 2. Request plan - remaining_actions = _MAX_TOTAL_ACTIONS - total_actions_executed + remaining_actions = self._max_total_actions - total_actions_executed planning_started_at = _utcnow() planning_started_monotonic = time.monotonic() self._observer.emit( @@ -1027,7 +1031,7 @@ def _run_replan_loop( break # 4. Enforce action budget - budget = _MAX_TOTAL_ACTIONS - total_actions_executed + budget = self._max_total_actions - total_actions_executed actions_to_execute = plan.actions[:budget] # 4b. Materialize deferred file bodies (content_brief -> content) via a @@ -1127,9 +1131,9 @@ def _run_replan_loop( stop_reason = LoopStopReason.BLOCKED elif has_fatal: stop_reason = LoopStopReason.FATAL_EXECUTION_FAILURE - elif total_actions_executed >= _MAX_TOTAL_ACTIONS: + elif total_actions_executed >= self._max_total_actions: stop_reason = LoopStopReason.MAX_ACTIONS - elif iteration_index >= _MAX_LOOP_ITERATIONS: + elif iteration_index >= self._max_loop_iterations: stop_reason = LoopStopReason.MAX_ITERATIONS # 8. Build observation @@ -1138,8 +1142,8 @@ def _run_replan_loop( execution_results, attempted_actions, iter_changed, - remaining_iterations=_MAX_LOOP_ITERATIONS - iteration_index, - remaining_actions=_MAX_TOTAL_ACTIONS - total_actions_executed, + remaining_iterations=self._max_loop_iterations - iteration_index, + remaining_actions=self._max_total_actions - total_actions_executed, ) iterations.append( diff --git a/tests/test_headless.py b/tests/test_headless.py new file mode 100644 index 0000000..5f63c8a --- /dev/null +++ b/tests/test_headless.py @@ -0,0 +1,373 @@ +"""Hermetic tests for headless worker mode (contract v0.1). + +No network, no live LLM: the provider is a scripted stub, verification commands +are fake binaries on PATH, and the workspace is a throwaway git repo. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +import textwrap +from pathlib import Path +from typing import Any + +import pytest +from agent_task_contract import ( + CONTRACT_VERSION, + CodingWorkerResult, + Event, + TaskState, +) + +from foundation.headless import ( + EXIT_COMPLETED, + EXIT_FAILED, + EXIT_PENDING_APPROVAL, + EXIT_REJECTED, + run_headless_task, +) +from foundation.models import ProviderPrompt, ProviderResponse, ProviderResponseMetadata +from foundation.settings import AppSection, AppSettings, MonitorSection + + +def _provider_response(payload: dict[str, Any]) -> ProviderResponse: + return ProviderResponse( + content=json.dumps(payload), + structured_output=payload, + metadata=ProviderResponseMetadata( + provider="stub", + model="stub-model", + latency_seconds=0.01, + ), + ) + + +class StubProvider: + """Scripted provider; auto-accepts plan-review preflights (mirrors test_orchestrator).""" + + def __init__(self, responses: list[ProviderResponse]) -> None: + self._responses = list(responses) + self.calls: list[ProviderPrompt] = [] + + def complete(self, prompt: ProviderPrompt) -> ProviderResponse: + if prompt.schema_name == "assistant_plan_review" and not ( + self._responses + and isinstance(self._responses[0].structured_output, dict) + and "decision" in self._responses[0].structured_output + ): + return _provider_response( + {"decision": "accept", "reason": "Stub preflight accepted the plan."} + ) + self.calls.append(prompt) + if not self._responses: + return _provider_response({"assistant_message": "Done.", "actions": []}) + return self._responses.pop(0) + + +def _settings(tmp_path: Path) -> AppSettings: + return AppSettings( + app=AppSection( + workspace_root=tmp_path / "workspace", + data_dir=tmp_path / "data", + state_dir=tmp_path / "state", + log_dir=tmp_path / "logs", + ), + monitor=MonitorSection(enabled=False, events_dir=tmp_path / "monitor-events"), + ) + + +def _git(workspace: Path, *args: str) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", "-C", str(workspace), *args], + capture_output=True, + text=True, + check=True, + ) + + +def _git_workspace(tmp_path: Path) -> Path: + workspace = tmp_path / "workspace" + workspace.mkdir() + _git(workspace, "init", "-q") + _git(workspace, "config", "user.email", "test@example.com") + _git(workspace, "config", "user.name", "Test") + (workspace / "existing.py").write_text("value = 0\n") + _git(workspace, "add", "existing.py") + _git(workspace, "commit", "-qm", "seed") + return workspace + + +def _task_envelope(workspace: Path, **overrides: Any) -> dict[str, Any]: + envelope: dict[str, Any] = { + "contract_version": CONTRACT_VERSION, + "task_id": "01976e10-0000-7000-8000-0000000000aa", + "trace_id": "01976e10-0000-7000-8000-0000000000bb", + "worker_kind": "coding", + "workspace": str(workspace), + "instructions": "Fix the failing test and make the suite pass.", + "permissions": {"read": ["workspace"], "write": ["workspace"], "env_allowlist": []}, + "budget": { + "wall_clock_seconds": 120, + "max_iterations": 5, + "max_actions": 20, + "max_provider_calls": 20, + }, + } + envelope.update(overrides) + return envelope + + +def _install_fake_pytest(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + bin_dir = tmp_path / "bin" + bin_dir.mkdir(exist_ok=True) + script = bin_dir / "pytest" + script.write_text( + f"#!{sys.executable}\n" + + textwrap.dedent("""\ + import sys + print("4 passed") + sys.exit(0) + """) + ) + script.chmod(0o755) + monkeypatch.setenv("PATH", f"{bin_dir}:{os.environ.get('PATH', '')}") + + +def _run( + tmp_path: Path, + provider: StubProvider, + envelope: dict[str, Any], +) -> tuple[int, Path, Path]: + task_path = tmp_path / "task.json" + out_path = tmp_path / "result.json" + task_path.write_text(json.dumps(envelope)) + code = run_headless_task( + task_path, + out_path, + settings=_settings(tmp_path), + provider=provider, # type: ignore[arg-type] + heartbeat_seconds=600.0, + ) + return code, out_path, Path(envelope["workspace"]) + + +def _happy_provider() -> StubProvider: + return StubProvider( + [ + _provider_response( + { + "assistant_message": "Fixing the bug and verifying.", + "actions": [ + { + "id": "a1", + "kind": "tool_call", + "summary": "Apply the fix", + "tool_call": { + "capability_id": "foundation.file.write", + "arguments": { + "path": "existing.py", + "content": "value = 1\n", + "overwrite": True, + }, + }, + }, + { + "id": "a2", + "kind": "shell", + "summary": "Run the test suite", + "shell": {"command": "pytest"}, + }, + ], + } + ), + _provider_response( + {"assistant_message": "Fixed and verified; suite passes.", "actions": []} + ), + ] + ) + + +def _read_events(workspace: Path, task_id: str) -> list[Event]: + lines = (workspace / ".events" / f"{task_id}.ndjson").read_text().splitlines() + return [Event.model_validate(json.loads(line)) for line in lines] + + +def test_happy_path_produces_contract_result_and_patch( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + workspace = _git_workspace(tmp_path) + _install_fake_pytest(tmp_path, monkeypatch) + head_before = _git(workspace, "rev-parse", "HEAD").stdout.strip() + + code, out_path, _ = _run(tmp_path, _happy_provider(), _task_envelope(workspace)) + + assert code == EXIT_COMPLETED + result = CodingWorkerResult.model_validate_json(out_path.read_text()) + assert result.status is TaskState.COMPLETED + assert result.verification.outcome.value == "passed" + assert result.changed_files == ["existing.py"] + kinds = {artifact.kind for artifact in result.artifacts} + assert kinds == {"event_log", "patch"} + assert result.commands and result.commands[0].command == "pytest" + assert result.commands[0].exit_code == 0 + + patch = next(a for a in result.artifacts if a.kind == "patch") + patch_path = workspace / patch.path + assert patch_path.is_file() + assert "value = 1" in patch_path.read_text() + + # Repo HEAD untouched: no commit, no push, ever (Q5 / Keep List #9). + assert _git(workspace, "rev-parse", "HEAD").stdout.strip() == head_before + + events = _read_events(workspace, "01976e10-0000-7000-8000-0000000000aa") + assert events[0].type.value == "task.start" + assert events[0].payload["worker_version"] + assert events[0].payload["manifest_fingerprint"] + assert events[-1].type.value == "task.terminal" + assert events[-1].payload["status"] == "completed" + seqs = [event.seq for event in events] + assert seqs == sorted(seqs) and len(set(seqs)) == len(seqs) + types = [event.type.value for event in events] + assert "verify.result" in types + assert "command.result" in types + + # Event-log artifact hash covers the file as written. + event_log = next(a for a in result.artifacts if a.kind == "event_log") + import hashlib + + digest = hashlib.sha256((workspace / event_log.path).read_bytes()).hexdigest() + assert digest == event_log.sha256 + + +def test_headless_never_reads_stdin(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + class ExplodingStdin: + def __getattr__(self, name: str) -> Any: + raise AssertionError(f"headless mode read stdin (.{name})") + + workspace = _git_workspace(tmp_path) + _install_fake_pytest(tmp_path, monkeypatch) + monkeypatch.setattr(sys, "stdin", ExplodingStdin()) + + code, out_path, _ = _run(tmp_path, _happy_provider(), _task_envelope(workspace)) + + assert code == EXIT_COMPLETED + assert CodingWorkerResult.model_validate_json(out_path.read_text()).status is ( + TaskState.COMPLETED + ) + + +def test_approval_required_action_ends_pending_approval( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + workspace = _git_workspace(tmp_path) + head_before = _git(workspace, "rev-parse", "HEAD").stdout.strip() + provider = StubProvider( + [ + _provider_response( + { + "assistant_message": "Committing the change.", + "actions": [ + { + "id": "a1", + "kind": "tool_call", + "summary": "Commit the fix", + "requires_approval": True, + "approval_reason": "git commit always requires approval", + "tool_call": { + "capability_id": "foundation.git.commit", + "arguments": {"message": "fix"}, + }, + } + ], + } + ), + ] + ) + + code, out_path, _ = _run(tmp_path, provider, _task_envelope(workspace)) + + assert code == EXIT_PENDING_APPROVAL + result = CodingWorkerResult.model_validate_json(out_path.read_text()) + assert result.status is TaskState.PENDING_APPROVAL + assert _git(workspace, "rev-parse", "HEAD").stdout.strip() == head_before + + events = _read_events(workspace, "01976e10-0000-7000-8000-0000000000aa") + types = [event.type.value for event in events] + assert "approval.requested" in types + assert events[-1].type.value == "task.terminal" + assert events[-1].payload["status"] == "pending_approval" + + +def test_contract_version_skew_rejects_task(tmp_path: Path) -> None: + workspace = _git_workspace(tmp_path) + provider = StubProvider([]) + envelope = _task_envelope(workspace, contract_version="9.9.9") + + code, out_path, _ = _run(tmp_path, provider, envelope) + + assert code == EXIT_REJECTED + result = CodingWorkerResult.model_validate_json(out_path.read_text()) + assert result.status is TaskState.REJECTED + assert "9.9.9" in result.summary + assert provider.calls == [] + + events = _read_events(workspace, "01976e10-0000-7000-8000-0000000000aa") + types = [event.type.value for event in events] + assert types[0] == "task.rejected" + assert events[-1].payload["status"] == "rejected" + + +def test_unknown_worker_kind_rejects_task(tmp_path: Path) -> None: + workspace = _git_workspace(tmp_path) + envelope = _task_envelope(workspace, worker_kind="curator") + + code, out_path, _ = _run(tmp_path, StubProvider([]), envelope) + + assert code == EXIT_REJECTED + result = CodingWorkerResult.model_validate_json(out_path.read_text()) + assert result.status is TaskState.REJECTED + assert "worker_kind" in result.summary + + +def test_budget_max_iterations_bounds_the_loop( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + workspace = _git_workspace(tmp_path) + _install_fake_pytest(tmp_path, monkeypatch) + # Provider always plans another action; budget must stop it after 1 iteration. + looping_plan = { + "assistant_message": "Still working.", + "actions": [ + { + "id": "a1", + "kind": "shell", + "summary": "Run the suite again", + "shell": {"command": "pytest"}, + } + ], + } + provider = StubProvider([_provider_response(looping_plan) for _ in range(4)]) + envelope = _task_envelope(workspace) + envelope["budget"]["max_iterations"] = 1 + + code, out_path, _ = _run(tmp_path, provider, envelope) + + assert code == EXIT_FAILED + result = CodingWorkerResult.model_validate_json(out_path.read_text()) + assert result.status is TaskState.FAILED + events = _read_events(workspace, "01976e10-0000-7000-8000-0000000000aa") + plan_events = [event for event in events if event.type.value == "plan.created"] + assert len(plan_events) == 1 + + +def test_missing_task_file_is_invocation_error(tmp_path: Path) -> None: + code = run_headless_task( + tmp_path / "absent.json", + tmp_path / "result.json", + settings=_settings(tmp_path), + provider=StubProvider([]), # type: ignore[arg-type] + ) + assert code == 2 diff --git a/uv.lock b/uv.lock index 34982e4..ed55f92 100644 --- a/uv.lock +++ b/uv.lock @@ -2,6 +2,24 @@ version = 1 revision = 3 requires-python = "==3.12.*" +[[package]] +name = "agent-task-contract" +version = "0.1.0" +source = { editable = "../agent-task-contract" } +dependencies = [ + { name = "pydantic" }, +] + +[package.metadata] +requires-dist = [{ name = "pydantic", specifier = ">=2" }] + +[package.metadata.requires-dev] +dev = [ + { name = "mypy", specifier = ">=1.13" }, + { name = "pytest", specifier = ">=8" }, + { name = "ruff", specifier = ">=0.8" }, +] + [[package]] name = "annotated-doc" version = "0.0.4" @@ -148,6 +166,7 @@ name = "foundation-cli" version = "0.2.0" source = { editable = "." } dependencies = [ + { name = "agent-task-contract" }, { name = "click" }, { name = "keyring" }, { name = "pathspec" }, @@ -170,6 +189,7 @@ dev = [ [package.metadata] requires-dist = [ + { name = "agent-task-contract", editable = "../agent-task-contract" }, { name = "click", specifier = ">=8.0,<9.0" }, { name = "coverage", extras = ["toml"], marker = "extra == 'dev'", specifier = ">=7.6,<8.0" }, { name = "keyring", specifier = ">=25.5,<26.0" }, From 3c9af1cac1e9241bd0fea1561fb19eede6185ec0 Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Thu, 11 Jun 2026 02:13:44 -0700 Subject: [PATCH 15/18] Add deterministic mock provider (Q10: the gate never depends on a live LLM) provider.name = "mock" + provider.scenario_file = replays a scripted scenario JSON: plan entries consumed in order, plan-review preflights auto-accepted (or scripted via review entries), zero-action completion when exhausted. Directive entries simulate worker failure modes for the golden smoke: crash (os._exit, no terminal event) and hang (SIGSTOP, heartbeats stop so the supervisor death path has something real to kill). Verified: ruff check/format clean, mypy src clean (46 files), full suite 559 passed (5 new mock-provider tests; zero regressions). Co-Authored-By: Claude Fable 5 --- src/foundation/doctor.py | 4 +- src/foundation/services/mock_provider.py | 128 +++++++++++++++++++++++ src/foundation/services/provider.py | 20 +++- src/foundation/settings.py | 2 + tests/test_mock_provider.py | 87 +++++++++++++++ 5 files changed, 237 insertions(+), 4 deletions(-) create mode 100644 src/foundation/services/mock_provider.py create mode 100644 tests/test_mock_provider.py diff --git a/src/foundation/doctor.py b/src/foundation/doctor.py index a9d222f..36125aa 100644 --- a/src/foundation/doctor.py +++ b/src/foundation/doctor.py @@ -159,12 +159,12 @@ def _provider_readiness_check(settings: AppSettings) -> DoctorCheck: summary="Provider name is missing.", detail="Set [provider].name to a supported provider.", ) - if provider_name not in {"codex", "openai", "ollama"}: + if provider_name not in {"codex", "openai", "ollama", "mock"}: return DoctorCheck( name="Provider readiness", status=DoctorStatus.FAIL, summary=f"Provider {settings.provider.name!r} is not supported.", - detail="Supported providers: codex, openai, ollama.", + detail="Supported providers: codex, openai, ollama, mock.", ) if not settings.provider.model.strip(): return DoctorCheck( diff --git a/src/foundation/services/mock_provider.py b/src/foundation/services/mock_provider.py new file mode 100644 index 0000000..53b3223 --- /dev/null +++ b/src/foundation/services/mock_provider.py @@ -0,0 +1,128 @@ +"""Deterministic scripted provider — the acceptance gate never depends on a live LLM. + +The mock is a first-class provider implementation (decision Q10): the golden +smoke and CI run against it with zero secrets, and a full headless run is +byte-deterministic because every "model" response comes from a scenario file. + +Scenario file format (JSON): + +```json +{ + "responses": [ + {"plan": {"assistant_message": "...", "actions": [...]}}, + {"review": {"decision": "accept", "reason": "..."}}, + {"directive": "crash", "exit_code": 13}, + {"directive": "hang"} + ] +} +``` + +- ``plan`` entries are consumed, in order, by planning calls; once the script is + exhausted every further planning call gets a zero-action completion. +- ``review`` entries answer plan-review preflights; without one queued, reviews + are auto-accepted (and consume nothing). +- ``directive`` entries simulate worker failure modes for the golden smoke: + ``crash`` exits the process immediately (no terminal event, no result); + ``hang`` SIGSTOPs the process so heartbeats stop and the supervisor's + death path (Q3) has something real to kill. +""" + +from __future__ import annotations + +import json +import os +import signal +import time +from pathlib import Path +from typing import Any + +from foundation.models import ( + ProviderPrompt, + ProviderResponse, + ProviderResponseMetadata, +) + +MOCK_PROVIDER_NAME = "mock" + + +class MockScenarioError(Exception): + """The scenario file is missing or malformed (doctor-style message).""" + + +def _response(payload: dict[str, Any], *, scenario: str) -> ProviderResponse: + return ProviderResponse( + content=json.dumps(payload, sort_keys=True), + structured_output=payload, + metadata=ProviderResponseMetadata( + provider=MOCK_PROVIDER_NAME, + model=scenario, + latency_seconds=0.0, + ), + ) + + +class MockProvider: + """Replay a scripted scenario file; deterministic by construction.""" + + def __init__(self, scenario_file: Path) -> None: + if not scenario_file.is_file(): + raise MockScenarioError( + f"mock provider scenario file not found: {scenario_file}. " + "Remediation: set provider.scenario_file to an existing scenario " + "JSON (see foundation.services.mock_provider for the format)." + ) + try: + document = json.loads(scenario_file.read_text(encoding="utf-8")) + except json.JSONDecodeError as exc: + raise MockScenarioError( + f"mock provider scenario file {scenario_file} is not valid JSON: {exc}." + ) from exc + responses = document.get("responses") + if not isinstance(responses, list): + raise MockScenarioError( + f"mock provider scenario file {scenario_file} must contain a " + "top-level 'responses' array." + ) + self._entries: list[dict[str, Any]] = list(responses) + self._scenario_name = scenario_file.stem + + def complete(self, prompt: ProviderPrompt) -> ProviderResponse: + if prompt.schema_name == "assistant_plan_review": + if self._entries and "review" in self._entries[0]: + entry = self._entries.pop(0) + return _response(dict(entry["review"]), scenario=self._scenario_name) + return _response( + {"decision": "accept", "reason": "Mock preflight accepted the plan."}, + scenario=self._scenario_name, + ) + + if not self._entries: + return _response( + {"assistant_message": "Done.", "actions": []}, + scenario=self._scenario_name, + ) + + entry = self._entries.pop(0) + if "plan" in entry: + return _response(dict(entry["plan"]), scenario=self._scenario_name) + if "directive" in entry: + return self._run_directive(entry) + raise MockScenarioError( + f"mock scenario entry must contain 'plan', 'review', or 'directive': {entry!r}" + ) + + def _run_directive(self, entry: dict[str, Any]) -> ProviderResponse: + directive = str(entry["directive"]) + if directive == "crash": + # Simulate a worker that dies mid-run: no terminal event, no result. + os._exit(int(entry.get("exit_code", 13))) + if directive == "hang": + # Freeze the whole process (all threads, heartbeats included) so the + # supervisor's heartbeat-loss detection has something real to kill. + os.kill(os.getpid(), signal.SIGSTOP) + time.sleep(3600) # unreachable unless resumed; the supervisor kills us + return _response( + {"assistant_message": "Resumed after hang.", "actions": []}, + scenario=self._scenario_name, + ) + raise MockScenarioError(f"unknown mock directive {directive!r}; supported: crash, hang.") diff --git a/src/foundation/services/provider.py b/src/foundation/services/provider.py index 14caf01..7c9001c 100644 --- a/src/foundation/services/provider.py +++ b/src/foundation/services/provider.py @@ -1160,15 +1160,31 @@ def build_provider_adapter( ) -> ProviderAdapter: """Build the configured provider adapter for Stage 5.""" provider_name = settings.provider.normalized_name() - if provider_name not in {"codex", "openai", "ollama"}: + if provider_name not in {"codex", "openai", "ollama", "mock"}: raise ProviderError( ( f"Provider {settings.provider.name!r} is not supported in Foundation CLI v0.1. " - "Supported providers: codex, openai, ollama." + "Supported providers: codex, openai, ollama, mock." ), code=ProviderErrorCode.UNSUPPORTED_PROVIDER, ) + if provider_name == "mock": + from foundation.services.mock_provider import MockProvider, MockScenarioError + + if settings.provider.scenario_file is None: + raise ProviderError( + "Provider 'mock' requires provider.scenario_file to point at a scenario JSON file.", + code=ProviderErrorCode.UNSUPPORTED_PROVIDER, + ) + try: + return MockProvider(settings.provider.scenario_file) + except MockScenarioError as exc: + raise ProviderError( + str(exc), + code=ProviderErrorCode.UNSUPPORTED_PROVIDER, + ) from exc + if provider_name == "codex": return CodexExecAdapter( model=settings.provider.model, diff --git a/src/foundation/settings.py b/src/foundation/settings.py index 0f3625c..abff120 100644 --- a/src/foundation/settings.py +++ b/src/foundation/settings.py @@ -209,6 +209,8 @@ class ProviderSection(BaseModel): num_ctx: PositiveInt | None = None api_key_env_var: str | None = OPENAI_DEFAULT_API_KEY_ENV_VAR api_key_keychain: KeychainSecretRef | None = Field(default_factory=KeychainSecretRef) + # Used only by the deterministic mock provider (provider.name = "mock"). + scenario_file: Path | None = None def normalized_name(self) -> str: """Return the normalized provider name.""" diff --git a/tests/test_mock_provider.py b/tests/test_mock_provider.py new file mode 100644 index 0000000..d1527b3 --- /dev/null +++ b/tests/test_mock_provider.py @@ -0,0 +1,87 @@ +"""Mock provider: deterministic scripted responses, no network, no secrets.""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from foundation.models import ProviderPrompt +from foundation.services.mock_provider import MockProvider, MockScenarioError +from foundation.services.provider import ProviderError, build_provider_adapter +from foundation.settings import AppSettings, ProviderSection + + +def _scenario(tmp_path: Path, responses: list[dict[str, object]]) -> Path: + path = tmp_path / "scenario.json" + path.write_text(json.dumps({"responses": responses})) + return path + + +def _plan_prompt() -> ProviderPrompt: + return ProviderPrompt( + messages=[{"role": "user", "content": "do the thing"}], + schema_name="assistant_plan", + ) + + +def _review_prompt() -> ProviderPrompt: + return ProviderPrompt( + messages=[{"role": "user", "content": "review the plan"}], + schema_name="assistant_plan_review", + ) + + +def test_scripted_plans_replay_in_order_and_deterministically(tmp_path: Path) -> None: + plan_one = {"assistant_message": "First.", "actions": []} + plan_two = {"assistant_message": "Second.", "actions": []} + scenario = _scenario(tmp_path, [{"plan": plan_one}, {"plan": plan_two}]) + + first_run = [MockProvider(scenario).complete(_plan_prompt()).content for _ in range(1)] + provider = MockProvider(scenario) + assert provider.complete(_plan_prompt()).structured_output == plan_one + assert provider.complete(_plan_prompt()).structured_output == plan_two + # Exhausted scripts settle into zero-action completion. + assert provider.complete(_plan_prompt()).structured_output == { + "assistant_message": "Done.", + "actions": [], + } + # Determinism: a fresh provider on the same scenario yields identical bytes. + assert MockProvider(scenario).complete(_plan_prompt()).content == first_run[0] + + +def test_review_prompts_auto_accept_without_consuming(tmp_path: Path) -> None: + plan = {"assistant_message": "Only plan.", "actions": []} + provider = MockProvider(_scenario(tmp_path, [{"plan": plan}])) + review = provider.complete(_review_prompt()) + assert review.structured_output == { + "decision": "accept", + "reason": "Mock preflight accepted the plan.", + } + assert provider.complete(_plan_prompt()).structured_output == plan + + +def test_scripted_review_entry_is_consumed(tmp_path: Path) -> None: + provider = MockProvider( + _scenario(tmp_path, [{"review": {"decision": "revise", "reason": "nope"}}]) + ) + review = provider.complete(_review_prompt()) + assert review.structured_output == {"decision": "revise", "reason": "nope"} + + +def test_missing_scenario_file_fails_loudly(tmp_path: Path) -> None: + with pytest.raises(MockScenarioError, match="Remediation"): + MockProvider(tmp_path / "absent.json") + + +def test_factory_builds_mock_and_requires_scenario_file(tmp_path: Path) -> None: + scenario = _scenario(tmp_path, []) + settings = AppSettings( + provider=ProviderSection(name="mock", scenario_file=scenario), + ) + adapter = build_provider_adapter(settings) + assert isinstance(adapter, MockProvider) + + with pytest.raises(ProviderError, match="scenario_file"): + build_provider_adapter(AppSettings(provider=ProviderSection(name="mock"))) From 8585d867782556f56116dbf420ed0afa053d03c7 Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Thu, 11 Jun 2026 02:31:06 -0700 Subject: [PATCH 16/18] Make headless patch artifacts appliable with git apply --index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git diff --binary for the patch artifact: intent-added untracked binary files (e.g. __pycache__ from a verification run in a repo without a .gitignore) previously produced text-format binary stubs that git apply rejects — and applying the patch is the approval action (Q5). Found by the Stage 4 review-approve smoke; covered there. Co-Authored-By: Claude Fable 5 --- src/foundation/headless.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/foundation/headless.py b/src/foundation/headless.py index c281e3a..f5a1fab 100644 --- a/src/foundation/headless.py +++ b/src/foundation/headless.py @@ -246,7 +246,9 @@ def _collect_patch(workspace: Path, task_id: str) -> tuple[Artifact | None, list _git(workspace, "add", "-N", "--", ".", *excludes) changed_proc = _git(workspace, "diff", "--name-only", "--", ".", *excludes) changed_files = [line for line in changed_proc.stdout.splitlines() if line.strip()] - diff_proc = _git(workspace, "diff", "--", ".", *excludes) + # --binary keeps the patch appliable (git apply --index) even when untracked + # binary files were intent-added; applying the patch is the approval action (Q5). + diff_proc = _git(workspace, "diff", "--binary", "--", ".", *excludes) if not diff_proc.stdout.strip(): return None, changed_files artifacts_dir = workspace / ".artifacts" From 84c4da741141c3e201cf339495f4a27ccb8fca9a Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Thu, 11 Jun 2026 06:12:50 -0700 Subject: [PATCH 17/18] Headless setup failures produce a failed result, not a bare crash Provider/services construction errors (e.g. a misconfigured provider) now emit a terminal failed event with reason=worker_setup_failed and write a valid result envelope, so the supervisor ingests evidence instead of synthesizing worker_crashed. Found running the real-provider smoke. Co-Authored-By: Claude Fable 5 --- src/foundation/headless.py | 93 ++++++++++++++++++++++---------------- 1 file changed, 55 insertions(+), 38 deletions(-) diff --git a/src/foundation/headless.py b/src/foundation/headless.py index f5a1fab..150b29b 100644 --- a/src/foundation/headless.py +++ b/src/foundation/headless.py @@ -466,44 +466,61 @@ def _reject(reason: str) -> int: return _reject(f"task envelope failed validation: {exc}") workspace = Path(task.workspace).expanduser().resolve() - tool_service = LocalToolService( - workspace_root=workspace, - default_timeout_seconds=min(settings.shell.default_timeout_seconds, 30), - capture_limit_kb=settings.shell.capture_limit_kb, - pass_through_foundation_env=settings.shell.pass_through_foundation_env, - ) - shell_runtime = ShellRuntime( - workspace_root=workspace, - default_timeout_seconds=settings.shell.default_timeout_seconds, - max_timeout_seconds=settings.shell.max_timeout_seconds, - allow_pty=False, - capture_limit_kb=settings.shell.capture_limit_kb, - enforce_workspace_boundary=True, - ) - capability_registry = CapabilityRegistry( - store=CapabilityStore(settings.app.data_dir / "capabilities"), - tool_service=tool_service, - ) - history_store = TraceStore( - database_path=settings.history.database_path, - retention_days=settings.history.retention_days, - max_entries=settings.history.max_entries, - ) - resolved_provider = provider if provider is not None else build_provider_adapter(settings) - - orchestrator = RequestOrchestrator( - workspace_root=workspace, - approval_mode=ApprovalMode.MANUAL, - provider=resolved_provider, - shell_runtime=shell_runtime, - tool_service=tool_service, - history_store=history_store, - capability_registry=capability_registry, - event_sink=sink, - question_callback=None, - max_loop_iterations=task.budget.max_iterations, - max_total_actions=task.budget.max_actions, - ) + try: + tool_service = LocalToolService( + workspace_root=workspace, + default_timeout_seconds=min(settings.shell.default_timeout_seconds, 30), + capture_limit_kb=settings.shell.capture_limit_kb, + pass_through_foundation_env=settings.shell.pass_through_foundation_env, + ) + shell_runtime = ShellRuntime( + workspace_root=workspace, + default_timeout_seconds=settings.shell.default_timeout_seconds, + max_timeout_seconds=settings.shell.max_timeout_seconds, + allow_pty=False, + capture_limit_kb=settings.shell.capture_limit_kb, + enforce_workspace_boundary=True, + ) + capability_registry = CapabilityRegistry( + store=CapabilityStore(settings.app.data_dir / "capabilities"), + tool_service=tool_service, + ) + history_store = TraceStore( + database_path=settings.history.database_path, + retention_days=settings.history.retention_days, + max_entries=settings.history.max_entries, + ) + resolved_provider = provider if provider is not None else build_provider_adapter(settings) + + orchestrator = RequestOrchestrator( + workspace_root=workspace, + approval_mode=ApprovalMode.MANUAL, + provider=resolved_provider, + shell_runtime=shell_runtime, + tool_service=tool_service, + history_store=history_store, + capability_registry=capability_registry, + event_sink=sink, + question_callback=None, + max_loop_iterations=task.budget.max_iterations, + max_total_actions=task.budget.max_actions, + ) + except Exception as exc: # noqa: BLE001 — setup failures must still leave evidence + return _finalize( + task=task, + out_path=out_path, + stream=stream, + sink=sink, + status=TaskState.FAILED, + summary=f"worker setup failed: {exc}", + verification=Verification( + outcome=ContractVerificationOutcome.NOT_ATTEMPTED, + details="failed before execution (provider/services construction)", + ), + changed_files=[], + extra_artifacts=[], + terminal_reason="worker_setup_failed", + ) stream.emit( EventType.TASK_START, From d7ac8accc0d09c0a7aa9415812449cbc0bc7e948 Mon Sep 17 00:00:00 2001 From: Smoke Test Date: Thu, 11 Jun 2026 06:37:12 -0700 Subject: [PATCH 18/18] Resolve agent-task-contract from its published repo (pinned v0.1.0) No local path dependencies: the contract package now comes from github.com/Anmolnoor/agent-task-contract at tag v0.1.0. Verified: full suite 559 passed after re-sync. Co-Authored-By: Claude Fable 5 --- pyproject.toml | 2 +- uv.lock | 14 ++------------ 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 05231d3..da7c73b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -94,4 +94,4 @@ mypy_path = ["src"] packages = ["foundation"] [tool.uv.sources] -agent-task-contract = { path = "../agent-task-contract", editable = true } +agent-task-contract = { git = "https://github.com/Anmolnoor/agent-task-contract", tag = "v0.1.0" } diff --git a/uv.lock b/uv.lock index ed55f92..a9f94d2 100644 --- a/uv.lock +++ b/uv.lock @@ -5,21 +5,11 @@ requires-python = "==3.12.*" [[package]] name = "agent-task-contract" version = "0.1.0" -source = { editable = "../agent-task-contract" } +source = { git = "https://github.com/Anmolnoor/agent-task-contract?tag=v0.1.0#f1caec7db4fddca347fa4e64e36fb464ebf706ea" } dependencies = [ { name = "pydantic" }, ] -[package.metadata] -requires-dist = [{ name = "pydantic", specifier = ">=2" }] - -[package.metadata.requires-dev] -dev = [ - { name = "mypy", specifier = ">=1.13" }, - { name = "pytest", specifier = ">=8" }, - { name = "ruff", specifier = ">=0.8" }, -] - [[package]] name = "annotated-doc" version = "0.0.4" @@ -189,7 +179,7 @@ dev = [ [package.metadata] requires-dist = [ - { name = "agent-task-contract", editable = "../agent-task-contract" }, + { name = "agent-task-contract", git = "https://github.com/Anmolnoor/agent-task-contract?tag=v0.1.0" }, { name = "click", specifier = ">=8.0,<9.0" }, { name = "coverage", extras = ["toml"], marker = "extra == 'dev'", specifier = ">=7.6,<8.0" }, { name = "keyring", specifier = ">=25.5,<26.0" },