diff --git a/.harness/docs/ARCHITECTURE.md b/.harness/docs/ARCHITECTURE.md index 946bab5..a8bb662 100644 --- a/.harness/docs/ARCHITECTURE.md +++ b/.harness/docs/ARCHITECTURE.md @@ -24,7 +24,9 @@ l5 is a level 3 agentic harness: a story execution system. The workflow defines A stage that writes to the repository declares an optional `changed_files` key naming its changed-files record: the implementer declares `changed-files.json`, the tester declares `tester-changed-files.json`. After any stage with this declaration completes, the coordinator checks that record against `blocked_paths` and escalates on violation — enforcement is driven by the workflow definition, with no stage names hard-coded in the coordinator. The documenter declares no record and is intentionally unchecked; enabling it later is a one-line workflow change. Both records share one schema definition (`modified`/`created`/`deleted` arrays), not two copies. -A stage may also declare an optional `may_not_create` key, a list of repository-relative path prefixes it is not allowed to add files under. The implementer declares `["tests/"]`; no other stage does. After a stage that declares both `changed_files` and `may_not_create`, the coordinator reads that stage's own record and escalates when any entry in its **`created`** array falls under a declared prefix. `modified` and `deleted` are not examined — the rule is about independence, not about directories: an implementer must be able to update an existing test whose call site its own signature change broke, but validation it authors itself checks what it built rather than what was asked. As with blocked paths, no stage name and no prefix appears in orchestration code; both are read off the stage dict. +A stage may also declare an optional `may_not_create` key, a list of repository-relative path prefixes it is not allowed to add files under. The implementer declares `["tests/"]`; no other stage does. After a stage that declares both `changed_files` and `may_not_create`, the coordinator reads that stage's own record and escalates when any entry in its **`created`** array falls under a declared prefix. `modified` and `deleted` are not examined by *this* check — the rule is about independence, not about directories: an implementer must be able to update an existing test whose call site its own signature change broke, but validation it authors itself checks what it built rather than what was asked. Since story-017 those two arrays are decided by the revert check below rather than left unexamined. As with blocked paths, no stage name and no prefix appears in orchestration code; both are read off the stage dict. + +A stage may also declare an optional `revert_check` key naming the artifact that check writes; the implementer declares `revert-check-result.json`. The key is what turns the check on, exactly as `clean_clone` does on the verifier: the coordinator reads `stage.get("revert_check")` and does nothing when it is absent, so removing the declaration disables the check with no change to orchestration code and the artifact name never appears there. The check runs immediately after the ownership check, inside the same `changed_files` block, on the same record and the **same enforced prefix list** — the stage's `may_not_create` declarations with the story's granted prefixes already subtracted and each grant already logged. A story holding a `stage_exception` for a prefix is therefore not subject to the revert check on it either, from reuse rather than from a second subtraction. The verifier stage declares an optional `clean_clone` key naming the artifact the clean-clone check writes (`clean-clone-result.json`). The key is what turns the check on: the coordinator reads `stage.get("clean_clone")` and does nothing when it is absent, so removing the declaration disables the check with no change to orchestration code, and the artifact name never appears there. It sits on the verifier because the check runs on the verifier's passing verdict, in that stage's branch, before routing. @@ -32,13 +34,13 @@ A stage may also declare an optional `schemas` map from artifact filename to sch ### Artifact schemas (`schemas/`) -One JSON Schema (draft 2020-12) per structured artifact at the harness root: `changed-files`, `test-results`, `verification-result`, `retry-guidance`, `story`, `execution-history`, `clean-clone-result`, `retry-history`. The first five are the single source of truth for the shapes the harness routes on; each is injected into the prompt that asks an agent to produce the artifact *and* read by the coordinator to check what the agent produced, so the two can never drift. +One JSON Schema (draft 2020-12) per structured artifact at the harness root: `changed-files`, `test-results`, `verification-result`, `retry-guidance`, `story`, `execution-history`, `clean-clone-result`, `retry-history`, `revert-check-result`. The first five are the single source of truth for the shapes the harness routes on; each is injected into the prompt that asks an agent to produce the artifact *and* read by the coordinator to check what the agent produced, so the two can never drift. -`execution-history.schema.json` is the exception that clarifies the rule, and `clean-clone-result.schema.json` and `retry-history.schema.json` follow it. Their artifacts are coordinator-written rather than stage-written, so they appear in no stage's `schemas` map and no stage is asked to satisfy them, and the coordinator does not validate its own write at run time — a self-check against a shape the same code just produced buys nothing. They get schemas anyway, because a schema is how this harness defines a shape it routes on *or hands to an agent*, and because `schema_context` makes any file in `schemas/` an injectable placeholder for the consumers that come later. The schema is the definition; the story's tests are what check conformance. `clean-clone-result` has a second consumer already: the retried implementer reads the artifact through `{{clean_clone_result}}`. +`execution-history.schema.json` is the exception that clarifies the rule, and `clean-clone-result.schema.json`, `retry-history.schema.json` and `revert-check-result.schema.json` follow it. Their artifacts are coordinator-written rather than stage-written, so they appear in no stage's `schemas` map and no stage is asked to satisfy them, and the coordinator does not validate its own write at run time — a self-check against a shape the same code just produced buys nothing. They get schemas anyway, because a schema is how this harness defines a shape it routes on *or hands to an agent*, and because `schema_context` makes any file in `schemas/` an injectable placeholder for the consumers that come later. The schema is the definition; the story's tests are what check conformance. `clean-clone-result` has a second consumer already: the retried implementer reads the artifact through `{{clean_clone_result}}`. `revert-check-result.schema.json` carries one thing the others do not: its top-level `description` states the check's **granularity limit** in full, because the artifact is where a reader meets the verdict and the place a false reading of it would be formed. The schemas directory is an *inventory*, and since story-013 the inventory is **declared in `schemas/manifest.json`** — a flat JSON object with one `schemas` key holding the sorted kebab-case stems, the same names `load_schema` takes. `schema_validator.shipped_schemas(harness_root=None)` is the only reader; both `tests/test_schema_validator.py` and `tests/test_story_004_validation.py` call it instead of holding a copy, so the fact is stored once and the tests are where it is *checked* rather than where it lives. They still assert **exact set equality** between the manifest and the directory, in both directions — a schema file with no manifest entry fails, a manifest entry with no schema file fails — and that must not be relaxed to a subset or a containment check. Adding a schema is therefore a two-line edit under `schemas/` and nothing else: the directory glob is `*.schema.json` so the manifest is not counted as a schema, with a companion assertion that `schemas/` holds nothing but `*.schema.json` files plus `manifest.json`, which is what the older `glob("*")` gave for free. -That move is why the standing "the implementer touches no file under `tests/`" requirement can hold verbatim. Before story-013 the two inventories lived in test files, so shipping a schema forced the implementer into `tests/`; story-011 collided with exactly that and closed it as a recorded deviation. The manifest is written and read by the same stage, which is weaker than a list a different stage maintains — accepted deliberately, and bought down by the diff being one line of filenames and by every name it declares still having to pass the parametrized draft-2020-12, unsupported-keyword and no-`additionalProperties` checks. `shipped_schemas` raises rather than returning an empty or partial tuple on a missing, unparseable, or wrong-shaped manifest, because a degraded return would make those parametrized checks silently vacuous instead of red. +That move is why the "the implementer touches no file under `tests/`" requirement could hold verbatim for the stories between story-013 and story-017; story-017 retired the requirement itself, and the paragraph is kept for the reason the move was still right. Before story-013 the two inventories lived in test files, so shipping a schema forced the implementer into `tests/`; story-011 collided with exactly that and closed it as a recorded deviation. The manifest is written and read by the same stage, which is weaker than a list a different stage maintains — accepted deliberately, and bought down by the diff being one line of filenames and by every name it declares still having to pass the parametrized draft-2020-12, unsupported-keyword and no-`additionalProperties` checks. `shipped_schemas` raises rather than returning an empty or partial tuple on a missing, unparseable, or wrong-shaped manifest, because a degraded return would make those parametrized checks silently vacuous instead of red. Schemas ship with the harness code (like the orchestration modules), not with per-repository `.harness/` config, so `schema_validator` resolves them relative to its own module rather than a caller-supplied root. `load_schema`, `schemas_dir` and `shipped_schemas` all accept the same optional `harness_root` override for callers that need one; `context_assembler` still globs the `harness_root` it is passed. Nothing in orchestration routes on the manifest, and `schema_context` globs `*.schema.json`, so `manifest.json` is no injectable placeholder and no `{{manifest_schema}}` appears in any rendered prompt. @@ -62,11 +64,11 @@ Templates carry no inline JSON artifact bodies. A stage that must produce a stru `planner.md` follows the same rule for the artifact it *asks for* rather than produces: it injects `{{story_schema}}` and states no required section and no required field of its own. What survives around the injection is deliberate and of two kinds. The skeleton stays, labeled an illustration rather than the contract, because the planner writes the story dialect and a shape teaches indentation, block scalars, and dash-prefixed items in a way a schema cannot; it names no field absent from `schemas/story.schema.json`. The `stage_exceptions` ask-first instruction also stays — "do not add one without asking the developer first" is planner role guidance, not schema content, and removing it would be an over-application of the injection rule. story-007 is why this matters: it changed the story contract twice, `planner.md` was in that story's `do_not_modify` list, and between the merge and the follow-up patches the planner wrote stories `l5-run` refused at pre-flight. -The drift source that paragraph used to name is closed: `planner.md` no longer states the workflow's `may_not_create` rule in prose. A `[Workflow facts]` section carries three injected placeholders — `{{workflow_stages}}` (the stage list, placed beside the `likely_file_changes` guidance so plans name only stages the workflow defines), `{{stage_create_restrictions}}` (one line per stage/prefix pair read off the workflow's `may_not_create` declarations), and `{{blocked_paths}}` (the rules' repository-wide list, stated as enforced repository-wide rather than per story, placed beside the scope guidance). The template itself names no stage and no restricted prefix — grep for `implementer|tester|verifier|documenter|tests/` over `planner.md` returns nothing; the skeleton's `stage: ` is a field description, not a stage name. What stays in prose is, as before, role guidance: the `stage_exceptions` ask-the-developer-first rule and the explanation of what an exception is for. +The drift source that paragraph used to name is closed: `planner.md` no longer states the workflow's `may_not_create` rule in prose. A `[Workflow facts]` section carries three injected placeholders — `{{workflow_stages}}` (the stage list, placed beside the `likely_file_changes` guidance so plans name only stages the workflow defines), `{{stage_create_restrictions}}` (one line per stage/prefix pair read off the workflow's `may_not_create` declarations), and `{{blocked_paths}}` (the rules' repository-wide list, stated as enforced repository-wide rather than per story, placed beside the scope guidance). Beside `{{stage_create_restrictions}}` sits one paragraph story-017 added, and it is role guidance rather than a restatement: restate an injected restriction exactly as the workflow declares it, or not at all, because a plan that tightens one has written an unenforced rule the harness cannot see broken and that a legitimate change can make impossible to satisfy. It is a courtesy, not the enforcement — the revert check is. The template itself names no stage and no restricted prefix — grep for `implementer|tester|verifier|documenter|tests/` over `planner.md` returns nothing; the skeleton's `stage: ` is a field description, not a stage name. What stays in prose is, as before, role guidance: the `stage_exceptions` ask-the-developer-first rule and the explanation of what an exception is for. ### Orchestration (`orchestration/`) -- `story_coordinator.py` — the Story Coordinator. Loads the workflow definition, story artifact, and rules; creates the story branch and run directory; loops: determine stage → assemble context → render prompt → invoke agent → save artifacts → update state → route (advance, retry, or escalate). Post-stage checks run in a fixed order: required artifacts present → declared artifacts match their schemas → changed-files record clear of blocked paths → stage output ownership. Schema validation sits in the middle deliberately, so a malformed `changed-files.json` escalates with a validation error naming the field rather than raising out of the blocked-paths check that reads the same file. Ownership runs last, on the same record, after that record is known to be well-formed and clear of blocked paths. `_ownership_violation` returns a frozen `OwnershipViolation(path, prefix)`, and the escalation reason names stage, path, and prefix in both `events.log` and `escalation-summary.md`. `_granted_prefixes` subtracts the story's grants from the enforced list at check time, and each applied grant is appended to `events.log` as `stage exception applied: may create `, so routing stays reconstructable from the log alone. Pre-flight story reading is one function, `read_story(story_text)`: load `story.schema.json`, parse with it, validate against it, and return a frozen `StoryReading` carrying both the `parsed` story (`None` when parsing failed) and the `problems` list. It runs above the run-directory creation and the branch checkout, so a rejection is an exit-1 refusal leaving no run directory, no `state.json`, no log, and no new branch — and no agent invoked. It is called exactly once per run, and the parse it returns is the run's only reading of the artifact: `reading.parsed` is threaded into every `build_context` call and into `_complete`, which takes the completion-report title and commit-message subject from `story["story"]["title"]` rather than scanning lines. A missing title is a loud `KeyError`; the schema marks it required and the run cannot reach `_complete` without having validated. `read_story` stays schema conformance only. Whether a story's `stage_exceptions` mean anything against the workflow *this run loaded* is a separate question the schema cannot answer, so it is a separate function — `stage_exception_problems(story, stages)`, called from `run_story` beside `read_story` and above the run-directory creation. It refuses an exception naming a stage the workflow does not define, and one granting a prefix that stage was never restricted on: an exception that grants nothing is a planning error, not a harmless one. Matching is exact — a grant must name a prefix appearing verbatim in the stage's `may_not_create` list, so `create: tests` against a declared `tests/` refuses rather than silently granting part of it. Both refusals print through one extracted `_refuse(story_path, problems)`, so the refusal shape (exit 1, one message per problem, nothing created) is a single code path rather than two copies of one. The retry branch archives before it increments: `archive_attempt(run_dir, archivable_artifacts(stages), state.retry_count + 1)` copies the superseded attempt's artifacts under `attempts/attempt-N/` — see the archive decisions below. `append_event(run_dir, message, *, kind, stage, artifacts, duration_seconds, verifier_outcome, retry_decision, retry_reason)` is the run's single event write path: the prose message stays positional and is what the `events.log` line is built from, and the *same call* appends one structured entry to `execution-history.json`. `load_history(run_dir)` is the read side, called only by `append_event` for the next sequence number. `run_story` captures `stage_started_at = time.monotonic()` at the stage-started event and reads it through a local `elapsed()` at every event that ends a stage, so a completed stage's entry carries a duration the log only made derivable; `_escalate` forwards whatever structured fields an escalation has and tags its entry `escalated`. The clean-clone check is the last thing the verifier branch does on a passing verdict: `clean_clone_check(run_dir, target_root, config, artifact)` builds a scratch clone with `tempfile.mkdtemp`, runs `run_clean_clone`, removes the scratch directory in a `finally` whatever the result, and writes the returned `CleanCloneResult.as_record()` to the run directory under the declared artifact name. `_build_clone` does `git clone --no-hardlinks` from the target's filesystem path, applies the target's tracked edits as `git diff --binary HEAD` piped to `git apply`, copies the untracked-but-not-ignored files from `git ls-files --others --exclude-standard`, then commits inside the clone; the target repository is only read. `_link_interpreter_roots` links the top-level directory of each configured interpreter path into the clone and appends those names to the clone's `.git/info/exclude`, because a virtualenv is gitignored and therefore absent from a fresh clone, and a `.gitignore` entry for a directory does not cover a symlink standing in its place. Zero exit appends `_clean_clone_passed` and falls through to the existing advance; non-zero takes the retry path the verification-failed branch already takes — `archive_attempt` above the increment, then increment, save, `_clean_clone_failed`, and `index = stage_names.index(stage["on_failure"]["retry_stage"])` — or the existing escalation path at the ceiling, with `_clean_clone_failures` collapsing the output's `FAILED` lines into the one-line reason. Both events are module-level helpers rather than inline calls, for a reason worth keeping: `tests/test_story_011_validation.py` proves its own non-vacuity by deleting the first `retry_decision="retry",` line at the verification-failed branch's indentation, and an inline clean-clone branch nests deeper and sits earlier in the file, so its line *contains* that indented text and the mutation lands there instead of where it was aimed. +- `story_coordinator.py` — the Story Coordinator. Loads the workflow definition, story artifact, and rules; creates the story branch and run directory; loops: determine stage → assemble context → render prompt → invoke agent → save artifacts → update state → route (advance, retry, or escalate). Post-stage checks run in a fixed order: required artifacts present → declared artifacts match their schemas → changed-files record clear of blocked paths → stage output ownership → the revert check. Schema validation sits in the middle deliberately, so a malformed `changed-files.json` escalates with a validation error naming the field rather than raising out of the blocked-paths check that reads the same file. Ownership runs last, on the same record, after that record is known to be well-formed and clear of blocked paths. `_ownership_violation` returns a frozen `OwnershipViolation(path, prefix)`, and the escalation reason names stage, path, and prefix in both `events.log` and `escalation-summary.md`. `_granted_prefixes` subtracts the story's grants from the enforced list at check time, and each applied grant is appended to `events.log` as `stage exception applied: may create `, so routing stays reconstructable from the log alone. The revert check sits immediately after, in the same block, reusing that already-narrowed `enforced` list rather than recomputing it: `governed_edits(run_dir, record_name, prefixes)` returns a frozen `GovernedEdits(paths, prefixes)` holding the sorted `modified` and `deleted` entries under any of them plus the prefixes that matched, and it names no stage, no prefix and no artifact. When `paths` is empty nothing at all happens — no clone, no suite, no artifact — because a check that can say nothing should not run. Otherwise `revert_check(run_dir, target_root, config, artifact, paths)` is shaped exactly like `clean_clone_check`: `tempfile.mkdtemp` scratch, the shared `run_clean_clone` with `revert` set to the governed paths, `shutil.rmtree` in a `finally` whatever the result, and `RevertCheckResult.as_record()` written under the declared artifact name. `permitted` is `result.exit_code != 0`. A check that did not run escalates naming the reason; `permitted` false escalates naming the stage, the prefixes and the paths; `permitted` true appends `_revert_check_permitted` and falls through to the existing advance. Both escalations go through `_escalate`, which does not touch `retry_count`. `_build_clone` and `run_clean_clone` gained one keyword-only `revert` parameter for this, defaulting to reverting nothing: when non-empty, `git checkout HEAD -- ` runs inside the clone *after* the working-tree diff is applied and the untracked files are copied and *before* `git add -A`, so the clone commits those files as HEAD has them while every other change is present, and a failed checkout raises `RuntimeError` naming the paths. Pre-flight story reading is one function, `read_story(story_text)`: load `story.schema.json`, parse with it, validate against it, and return a frozen `StoryReading` carrying both the `parsed` story (`None` when parsing failed) and the `problems` list. It runs above the run-directory creation and the branch checkout, so a rejection is an exit-1 refusal leaving no run directory, no `state.json`, no log, and no new branch — and no agent invoked. It is called exactly once per run, and the parse it returns is the run's only reading of the artifact: `reading.parsed` is threaded into every `build_context` call and into `_complete`, which takes the completion-report title and commit-message subject from `story["story"]["title"]` rather than scanning lines. A missing title is a loud `KeyError`; the schema marks it required and the run cannot reach `_complete` without having validated. `read_story` stays schema conformance only. Whether a story's `stage_exceptions` mean anything against the workflow *this run loaded* is a separate question the schema cannot answer, so it is a separate function — `stage_exception_problems(story, stages)`, called from `run_story` beside `read_story` and above the run-directory creation. It refuses an exception naming a stage the workflow does not define, and one granting a prefix that stage was never restricted on: an exception that grants nothing is a planning error, not a harmless one. Matching is exact — a grant must name a prefix appearing verbatim in the stage's `may_not_create` list, so `create: tests` against a declared `tests/` refuses rather than silently granting part of it. Both refusals print through one extracted `_refuse(story_path, problems)`, so the refusal shape (exit 1, one message per problem, nothing created) is a single code path rather than two copies of one. The retry branch archives before it increments: `archive_attempt(run_dir, archivable_artifacts(stages), state.retry_count + 1)` copies the superseded attempt's artifacts under `attempts/attempt-N/` — see the archive decisions below. `append_event(run_dir, message, *, kind, stage, artifacts, duration_seconds, verifier_outcome, retry_decision, retry_reason)` is the run's single event write path: the prose message stays positional and is what the `events.log` line is built from, and the *same call* appends one structured entry to `execution-history.json`. `load_history(run_dir)` is the read side, called only by `append_event` for the next sequence number. `run_story` captures `stage_started_at = time.monotonic()` at the stage-started event and reads it through a local `elapsed()` at every event that ends a stage, so a completed stage's entry carries a duration the log only made derivable; `_escalate` forwards whatever structured fields an escalation has and tags its entry `escalated`. The clean-clone check is the last thing the verifier branch does on a passing verdict: `clean_clone_check(run_dir, target_root, config, artifact)` builds a scratch clone with `tempfile.mkdtemp`, runs `run_clean_clone`, removes the scratch directory in a `finally` whatever the result, and writes the returned `CleanCloneResult.as_record()` to the run directory under the declared artifact name. `_build_clone` does `git clone --no-hardlinks` from the target's filesystem path, applies the target's tracked edits as `git diff --binary HEAD` piped to `git apply`, copies the untracked-but-not-ignored files from `git ls-files --others --exclude-standard`, then commits inside the clone; the target repository is only read. `_link_interpreter_roots` links the top-level directory of each configured interpreter path into the clone and appends those names to the clone's `.git/info/exclude`, because a virtualenv is gitignored and therefore absent from a fresh clone, and a `.gitignore` entry for a directory does not cover a symlink standing in its place. Zero exit appends `_clean_clone_passed` and falls through to the existing advance; non-zero takes the retry path the verification-failed branch already takes — `archive_attempt` above the increment, then increment, save, `_clean_clone_failed`, and `index = stage_names.index(stage["on_failure"]["retry_stage"])` — or the existing escalation path at the ceiling, with `_clean_clone_failures` collapsing the output's `FAILED` lines into the one-line reason. Both events are module-level helpers rather than inline calls, for a reason worth keeping: `tests/test_story_011_validation.py` proves its own non-vacuity by deleting the first `retry_decision="retry",` line at the verification-failed branch's indentation, and an inline clean-clone branch nests deeper and sits earlier in the file, so its line *contains* that indented text and the mutation lands there instead of where it was aimed. - `story_parser.py` — lexer plus schema-directed interpreter for the story artifact. **The story dialect is not YAML**; see the module docstring before reaching for `yaml.safe_load`, which reads committed artifacts differently and wrongly. The lexer produces line/indent/content records, drops blank lines and full-line comments, consumes a `key: |` block scalar body whole (so blank and `#`-shaped lines *inside* it survive), and rejects tab indentation. The interpreter dispatches on the schema node's `type`, consulting structure only where the schema is silent. Under `items.type == "string"` a `- ` item is the verbatim remainder of its line, colons included; under `items.type == "object"` the same syntax parses into key/value pairs. Scalars are never coerced — every value is a `str`. A single `StoryParseError` carries line, expectation, and finding, rendering as `line 12: expected …, found …`. - `schema_validator.py` — `schemas_dir`, `load_schema`, `shipped_schemas`, `unsupported_keywords`, and `validate(instance, schema) -> list[str]`. `shipped_schemas(harness_root=None) -> tuple[str, ...]` reads `schemas/manifest.json` through `schemas_dir`, so the override behaves identically to `load_schema`'s, and raises `ValueError` on anything short of a well-formed non-empty list of strings. A deliberately small JSON Schema subset — `type`, `required`, `properties`, `items`, `enum` — because the harness is standard library only. `validate` walks the whole schema first and raises `ValueError` if any keyword outside that subset appears anywhere in it, so a schema can never claim a constraint the validator silently drops. Errors carry a tracked JSON path, the expectation, and the found value: `$.blocking_issues[0].severity: expected one of ["high", "medium", "low"], found string ("critical")`. - `context_assembler.py` — builds each stage's runtime context from the story artifact, prior stage artifacts, retry state, and architecture documents, and renders it into the prompt template. `build_context` takes both the raw `story_text` and the required keyword-only `story` (the parsed artifact from `read_story`); it never reads the artifact itself. `{{story}}` is `story_text` verbatim, and `{{acceptance_criteria}}` comes from the parsed list via `_dashed_lines`, which renders one `- `-prefixed criterion per line and returns `None` for an absent or empty list. `{{stage_exceptions}}` follows the same convention through `_exception_lines`: one dash-prefixed line per grant naming the stage, the granted path, and the reason, `None` when the story declares none. `render()` is single-pass: `re.sub` does not re-scan substituted text, so a placeholder injected by one substitution is not itself resolved. `build_context` therefore resolves the shared `prompts/harness-layer.md` partial as a **two-pass render** — it renders that partial (including the partial's own `{{blocked_paths}}` placeholder) against the assembled context first, then stores the already-resolved text as the `harness_layer` context value for injection into stage templates. When the partial is absent, `harness_layer` is left unset and renders as `None`. The schema placeholders come from `schema_context(harness_root) -> dict[str, str]`, a public function of the same module: it globs `harness_root/schemas/*.schema.json` and exposes each file's text under the stem with hyphens replaced by underscores plus `_schema` (`verification-result.schema.json` → `{{verification_result_schema}}`). `build_context` merges it with `update` at the point the inline loop used to run, before the two-pass render, so the values are available to any template. A new schema file becomes an injectable placeholder with no code change. The glob appears exactly once in the module because it has two callers: `build_context` for workflow stages, and `l5-plan` for the planner template, which no coordinator renders. `workflow_context(workflow, rules) -> dict[str, str | None]` sits beside `schema_context` for the same reason: it maps the loaded workflow's stage names to `{{workflow_stages}}`, each stage's `may_not_create` declarations to `{{stage_create_restrictions}}` (`" may not create files under "`, one line per pair), and the rules' `blocked_paths` to `{{blocked_paths}}`, all through the shared `_dashed_lines` helper — `build_context`'s own `blocked_paths` rendering goes through the same helper, so the harness-layer partial and the planner render the list identically. `_dashed_lines` returns `None` for an empty list, and `render()` maps `None` to the literal `None`, so the empty-list edge changes no rendered prompt. @@ -80,7 +82,7 @@ Headless agents cannot answer permission prompts, so `.harness/config.yaml` carr ### Rules (`rules/`) -`execution-rules.json` — `max_retries`, `blocked_paths`, `require_verifier_pass`. The coordinator refuses to advance past verification without a passing `verification-result.json`, stops retrying at the ceiling, and fails a stage that modified a blocked path. Blocked paths are checked after every stage that declares a `changed_files` record in the workflow definition, each stage against its own record only. Stage output ownership (`may_not_create`) is checked against the same record but declared in the *workflow*, not here: blocked paths are a property of the repository and apply to every stage, while ownership is a property of one stage's role in one workflow. +`execution-rules.json` — `max_retries`, `blocked_paths`, `require_verifier_pass`. The coordinator refuses to advance past verification without a passing `verification-result.json`, stops retrying at the ceiling, and fails a stage that modified a blocked path. Blocked paths are checked after every stage that declares a `changed_files` record in the workflow definition, each stage against its own record only. Stage output ownership (`may_not_create`) is checked against the same record but declared in the *workflow*, not here: blocked paths are a property of the repository and apply to every stage, while ownership is a property of one stage's role in one workflow. The revert check reads nothing from here either: it reuses the same workflow-declared prefixes and the target repository's configured `test_command`. ### Scripts (`scripts/`) @@ -123,6 +125,8 @@ Thin entry points only; no orchestration logic. `l5-init`, `l5-plan`, `l5-run`, verification/iteration-1.json retry-guidance.json written by the verifier on failure clean-clone-result.json the clean-clone check's record, coordinator-written + revert-check-result.json the revert check's record, coordinator-written; + absent when the stage's record named no governed path attempts/attempt-1/ superseded attempt's artifacts, canonical filenames retry-history.json one entry per retry taken; absent when none was completion-report.md or escalation-summary.md @@ -133,13 +137,13 @@ The files at the root always describe the *current* attempt. `attempts/attempt-N Three artifacts describe a retry and none of them substitutes for another. `retry-guidance.json` looks *forward*: the verifier writes it for the next attempt, saying what to fix. `attempts/attempt-N/` is the *evidence*: the actual artifacts that attempt produced. `retry-history.json` is the backward-looking *record*: one entry per retry actually taken, naming the attempt that ended, the blocking issues that failed it as the verifier recorded them (field for field, never a prose summary), the stage execution was rerouted to, the `attempts/attempt-N/` directory holding that attempt's evidence, and the guidance that attempt's failure produced — optional, because a retry can be routed without any being written. Its named consumers are the documenter, the assist agent during recovery, and the adjudicator later; they read it directly rather than filtering the full event stream. Overlap with `execution-history.json`, which records each retry decision as one event among all events, is expected rather than a defect: that artifact is the chronological stream, this one is the retry-scoped record. Rendering it into `escalation-summary.md` belongs to the escalation-summary story, which consumes this artifact. -`clean-clone-result.json` is absent from the archive for a different reason, and it is a known cost rather than a decision. `archivable_artifacts` reads the three places a stage names artifacts — `outputs`, `changed_files`, and the keys of `schemas` — and the clean-clone declaration is none of those, so a second attempt's record overwrites the first's. The event stream still carries that a first clean-clone failure happened; only its captured output is lost. Widening the declaration is the fix if that ever costs a debugging session. +`clean-clone-result.json` is absent from the archive for a different reason, and it is a known cost rather than a decision. `archivable_artifacts` reads the three places a stage names artifacts — `outputs`, `changed_files`, and the keys of `schemas` — and the clean-clone declaration is none of those, so a second attempt's record overwrites the first's. The event stream still carries that a first clean-clone failure happened; only its captured output is lost. Widening the declaration is the fix if that ever costs a debugging session. `revert-check-result.json` is absent from the archive for exactly the same reason and on the same terms — a known cost carried deliberately, not an oversight — though it bites less: the revert check escalates rather than retrying, so a run that writes the record twice is a run that was permitted the first time. ## Decisions and constraints - Story IDs are `story-NNN`, assigned sequentially by `l5-plan`. - Branch per story: `story/`, created from the current branch by the coordinator. -- The implementer runs existing tests locally as implementation discipline; the tester creates and runs new validation; the verifier evaluates evidence only. That split is enforced by `may_not_create`, not trusted to the prompt. story-006 is why: the story artifact named a test file in its plan, the implementer created it because injected story state is authoritative, and the tester — arriving second and forbidden to delete an existing test — wrote its own alongside it, leaving two files covering one plan with 17 of 19 tests duplicated. Every stage did what it was told; nothing in the harness could see the aggregate. A rule only a prompt states is a rule the harness cannot see broken. +- The implementer runs existing tests locally as implementation discipline; the tester creates and runs new validation; the verifier evaluates evidence only. That split is enforced by `may_not_create` on creations and, since story-017, by the revert check on modifications and deletions — not trusted to the prompt. story-006 is why: the story artifact named a test file in its plan, the implementer created it because injected story state is authoritative, and the tester — arriving second and forbidden to delete an existing test — wrote its own alongside it, leaving two files covering one plan with 17 of 19 tests duplicated. Every stage did what it was told; nothing in the harness could see the aggregate. A rule only a prompt states is a rule the harness cannot see broken. - An ownership violation escalates immediately without incrementing `retry_count`, matching a blocked-path violation. The stage did not fail at its work; it produced an output that is not its to produce, and a retry of the same instructions would produce it again. No new retry axis and no new `RunState` field. - A `stage_exception` is the pressure valve, and it is deliberately narrow: exact prefix match, required `reason`, cross-checked against the loaded workflow at pre-flight, and recorded in `events.log` when applied. A story whose deliverable is the regression suite lifts the filesystem rule; it does not lift independence, because the tester still validates what the implementer wrote. - Every writing stage keeps its own changed-files record, and the verifier receives them injected separately: the implementer's `{{changed_files}}` is held to the approved story scope, while `{{tester_changed_files}}` lists test files that are expected additions of a later stage, not scope violations (`None` when absent, e.g. before the tester has run). Requiring the record in the stage's `outputs` list makes the existing required-artifacts check escalate when it is missing — no separate code path. @@ -170,7 +174,7 @@ Three artifacts describe a retry and none of them substitutes for another. `retr - The coordinator's output contract is stated directly, in `tests/test_coordinator_contract.py`, not as equality with a past implementation. That file is the standing home for what a run must write: `state.json`'s exact field set (read from `dataclasses.fields(RunState)` rather than typed out, so it cannot disagree with the definition it describes), each field's type, the statuses a run can end in (exercised by runs that reach them), the frozen `events.log` line pattern, the escalation summary's five parts, and the run directory's **required subset** of artifacts (derived from the loaded workflow's `outputs`, per the same read-it-off-the-definition rule as `archivable_artifacts`). It is not named for a story and is not one story's evidence — a later story that means to change one of these shapes edits it deliberately, which is the point. - A differential test against a frozen implementation is an instrument with a shelf life, and it should be retired when the constraint it was built for has landed. story-011's six comparisons — same `events.log`, same `state.json`, same escalation summary, same run-directory contents as the pre-story-011 coordinator — were exactly right while the requirement was "adding `execution-history.json` changes nothing else". Once merged they asserted something nobody holds: that the coordinator's output may never differ from what one implementation produced on one day. story-012 (`retry-history.json`) and story-014 (`clean-clone-result.json`) each legitimately add an artifact and an event; story-014 escalated on nine failures, all of them this comparison, with the file in its `do_not_modify`. The instrument also decays independently — the historical coordinator is run against today's workflow, schemas and config, a pairing that grows more artificial until the old code cannot run at all. story-016 replaced the comparisons with the direct shape assertions above. Removing a test is not weakening it when every guarantee it carried is restated as a shape and shown to fail on violation; the corollary is that the restatement must land *before* the removal, and each new assertion must be demonstrated red. What survives in `tests/test_story_011_validation.py` is everything not resolved out of git history: the log-line-to-history correspondence, the ordering and retry-stream checks, the schema conformance of a run's history, and the prompt-scope assertion with its baseline resolution intact. No module under `tests/` loads a coordinator implementation out of git history any longer. - **When a rule and a declaration collide, move the declaration rather than weakening the rule or re-routing the edit.** Two correct rules — "the schema inventory must be a deliberate, noticed act" and "the implementer touches no file under `tests/`" — could not both hold while the inventory lived in test files. story-013 resolved it by asking where the fact belongs, not who should be allowed to edit it: the inventory is a fact about *what the harness ships*, so it moved to `schemas/manifest.json` beside the schemas it names, and the tests kept their assertions. Routing the edit to the tester was rejected because it leaves the suite red between stages and treats a misplaced source of truth as a scheduling problem. The contrasting case is `FIRST_SCHEMA_ERA_STORY`, deliberately left under `tests/`: it is a fact about which committed artifacts the corpus tests validate — test-only — so `tests/` is its correct home, and the open request to consolidate its two copies there stays open and unblocked. Different classes of fact, different homes; a duplicated definition usually dissolves as a side effect once the home is right, as both inventory copies did here. -- A story whose deliverable is *moving a definition out of `tests/`* cannot carry the `tests/`-independence requirement it is protecting. story-013 wrote the narrower true thing into its verification requirements instead — the implementer's edits under `tests/` are confined to deleting the two inventory definitions and repointing their assertions, with no test file created, no test function added, and no unrelated assertion changed. `may_not_create` permits modifications, so the coordinator does not escalate; the discipline has to come from the verification requirements. Do not reword the standing sentence for future stories, and expect one further ripple: an existing per-story test that asserted *where* a declaration lives goes red on landing and should be repointed, not weakened (story-014's inventory assertion moved from grepping both test sources for a literal name to asserting membership in `shipped_schemas()`, keeping its anti-subset guards intact). The ripple ran wider than one assertion and is not spent: story-012, the next story to ship a schema, found five assertions in `tests/test_story_013_validation.py` resolving their "after" state from the *working tree* rather than from story-013's own endpoint, so every one of them went red on `schemas/retry-history.schema.json` — a file story-013 has nothing to say about. They were repointed to `_endpoint_listing` (falling back to the working tree while a story is in flight), and the recovered-inventory comparison repointed to the schema stems listed *at the baseline revision*, with two non-emptiness guards added so no repointed comparison can pass on an empty set. This is the same trap as the `HEAD`-baseline bullets above wearing a different hat: an assertion about what a story did must be bounded at both ends of that story's commit range, and "today's working tree" is never the upper bound. The cost landed on story-012 as a stated-requirement deviation — its implementer had to modify a file under `tests/` to keep the suite green, which its own verification requirements forbade, and reverting the edit re-breaks the suite. Recorded here so the next story that ships a schema inherits the lesson rather than the failure. +- A story whose deliverable is *moving a definition out of `tests/`* cannot carry the `tests/`-independence requirement it is protecting. story-013 wrote the narrower true thing into its verification requirements instead — the implementer's edits under `tests/` are confined to deleting the two inventory definitions and repointing their assertions, with no test file created, no test function added, and no unrelated assertion changed. `may_not_create` permits modifications, so the coordinator did not escalate; the discipline had to come from the verification requirements. story-017 replaced that discipline with the revert check — the standing sentence is retired, not reworded, and a story should now state the restriction exactly as the workflow declares it or not at all. Expect one further ripple: an existing per-story test that asserted *where* a declaration lives goes red on landing and should be repointed, not weakened (story-014's inventory assertion moved from grepping both test sources for a literal name to asserting membership in `shipped_schemas()`, keeping its anti-subset guards intact). The ripple ran wider than one assertion and is not spent: story-012, the next story to ship a schema, found five assertions in `tests/test_story_013_validation.py` resolving their "after" state from the *working tree* rather than from story-013's own endpoint, so every one of them went red on `schemas/retry-history.schema.json` — a file story-013 has nothing to say about. They were repointed to `_endpoint_listing` (falling back to the working tree while a story is in flight), and the recovered-inventory comparison repointed to the schema stems listed *at the baseline revision*, with two non-emptiness guards added so no repointed comparison can pass on an empty set. This is the same trap as the `HEAD`-baseline bullets above wearing a different hat: an assertion about what a story did must be bounded at both ends of that story's commit range, and "today's working tree" is never the upper bound. The cost landed on story-012 as a stated-requirement deviation — its implementer had to modify a file under `tests/` to keep the suite green, which its own verification requirements forbade, and reverting the edit re-breaks the suite. Recorded here so the next story that ships a schema inherits the lesson rather than the failure. - Do not assert a run directory as an exact set. Every story that adds an artifact would fail an assertion about something else, which is the friction story-016 exists to remove; require the artifacts a completed run must produce and permit others. The schemas *inventory* (above) is the deliberate exception — there the point is that a new shape cannot appear unnoticed. - The `HEAD`-baseline trap catches validation files too, not only differential tests of orchestration code. story-016's tester resolved its "before" copy of `tests/test_story_011_validation.py` as `git show HEAD:...`; in the working tree the diff was real, and in a clean clone with the story committed the baseline *was* the story's own file, so the removal assertion compared the file against itself. One test failed and two neighbouring diff assertions had gone silently vacuous — the more dangerous outcome. The fix is the same walk story-011 uses: `git log --format=%H -- ` newest-first, taking the first blob that still carries what the story removed, raising loudly when none does, plus a positive guard asserting the resolved baseline differs from the working tree and really contains it. Read the bullet above as applying to any test that resolves a baseline out of git, whatever it is a baseline of. The same trap has a quieter form: an assertion that a story left a path alone written as `git diff HEAD -- ` empty. That asks whether the working tree is dirty there, which is a question about whoever is working *now* — vacuously green once the story commits, and red for every later story that legitimately edits the path. `tests/test_story_009_validation.py` and `tests/test_story_010_validation.py` carried six of these; story-014 rewrote them to diff the story's own commit against its parent, resolving the commit by walking a marker file's history for the first revision carrying the feature the story introduced, and story-015 replaced that with the shared resolution below. Five stories in total (007, 008, 009, 010 and 013) shipped the idiom, which is why the next two bullets exist: it is a standing pattern the harness permitted, not one story's lapse. - **The baseline a per-story assertion compares against is resolved in exactly one place**: `story_commit_range(validation_file, repo=HARNESS_ROOT)` in `tests/conftest.py`, with `story_diff(paths, validation_file=..., diff_filter=..., options=...)` on top of it. A story's own run commit is the **oldest** commit that *added* that story's validation file (`git log --diff-filter=A`), and the baseline is that commit's parent. Oldest-addition is what makes a planning or hotfix commit on the same story — which *modifies* the file — unable to be mistaken for the run, and the pair of bounds is what keeps the comparison fixed as later stories accumulate: it survives a commit, a rebase and a squash, where a pinned SHA would not, and it needs no marker string to be chosen and kept true. While the story is in flight the file is uncommitted and the range degrades to HEAD against the working tree, which is the correct pre-story baseline then. When the file is in `HEAD` but no adding commit is visible (a shallow clone) or the adding commit is the root, it raises `NothingToCompareAgainst` rather than returning a baseline that makes the caller vacuous — the discrimination is a `git cat-file -e HEAD:` probe, so the uncommitted fallback can never mask a truncated history. The `repo` parameter exists so the same code path can be exercised against a synthetic history in which the story *is* committed, the state the repository under test cannot be in while these tests decide whether it commits. No repaired file carries its own copy of the resolution. @@ -183,6 +187,14 @@ Three artifacts describe a retry and none of them substitutes for another. `retr - A clone, not a tree copy. A copy would carry `.venv/` and `.harness/runs/`, and it would not have the story as a *commit* — which is the whole point, since a test resolving a baseline as `git show HEAD:…` only misbehaves once the story *is* `HEAD`. Cloning from the local filesystem path also makes "no network access" true by construction rather than by observation, and letting `.gitignore` do the filtering is what makes the clone's contents the same set `_complete`'s `git add -A` would commit, without a second exclusion list to keep in sync. The target repository is never mutated: no commit, no branch, no index change, no stash. - A clean-clone failure reroutes rather than escalating. The suite failing where the code ships is a defect in the implementation, which is what a retry addresses, so it reuses the verification-failed branch's whole sequence — archive above the increment, increment `retry_count`, save, one event, reroute to the stage named by the verifier's own `on_failure.retry_stage` — and the existing escalation path at the ceiling. No new `RunState` field and no second retry axis, matching how every other routing decision here is kept to one. - A configured `clean_clone_python` that does not resolve escalates naming it, rather than falling back to the harness's interpreter. A check that quietly tests the wrong version is worse than one that refuses. The key exists because nothing local otherwise exercises a version CI does — the developer's venv is 3.14 while CI tests 3.10, 3.11 and 3.12 — and the record carries the interpreter and the version it reported so a reader can tell which Python the check exercised rather than assuming. A *fallback* interpreter reporting no recognizable version is not an error: the configured test command need not be Python at all, so the record simply carries no `python_version` there. -- story-014's own run was not governed by the check it adds, for the reason story-007 hit with `may_not_create`: the coordinator loads the workflow definition at run start, before the `clean_clone` key existed in it. Enforcement begins with the next story. Expect this of any story that adds an enforcement rule, and say so in its constraints rather than treating the gap as a defect. +- story-014's own run was not governed by the check it adds, for the reason story-007 hit with `may_not_create`: the coordinator loads the workflow definition at run start, before the `clean_clone` key existed in it. Enforcement begins with the next story. Expect this of any story that adds an enforcement rule, and say so in its constraints rather than treating the gap as a defect. story-017 is the third instance, with `revert_check`; three is enough to call it the standing shape rather than a recurrence. +- **The line the harness draws under `tests/` is not a path prefix, it is reverting.** Four story artifacts asserted in prose that the implementer's record lists nothing under `tests/`. The harness enforced something narrower — `may_not_create`, creation only — and never escalated on the difference, so the prose added no enforcement while being sometimes impossible to satisfy: a legitimate implementer change can break an existing test and the suite has to stay green. Every finding the sentence produced was a deviation from prose rather than a defect (story-011 had to add a schema to two inventories asserting exact set equality, story-013 could not carry the rule at all because deleting those inventories was its deliverable, story-012 shipped a schema that turned five story-013 assertions red and recorded the deviation as not clearable by a retry). Three separate causes, each removed by its own story, each followed by another, and nobody could enumerate the next. The distinction being reached for was never "must not touch `tests/`" but "must not author its own validation", and a path prefix conflates two acts: authoring coverage, which must not happen in the implementer, and keeping existing validation runnable, which is maintenance. Reverting separates them exactly and with no judgement — **maintenance is by definition the edit without which the suite fails** — so an edit under a governed prefix is permitted iff reverting it makes the suite fail. Do not reintroduce the prefix rule in prose beside the check. +- The revert check is story-014's clone operation with the governed paths *restored from HEAD* instead of applied, and there is deliberately no second clone builder. `run_clean_clone` stays the single build-a-clone-and-run-the-suite path and both checks go through it. The added `revert` parameter defaults to reverting nothing, so the clean-clone check's artifact, events and routing are unchanged — which is the property to re-establish after any future edit to that path, because two clone builders would drift the same way two write paths for the run's history would. +- **The check decides on the whole set of governed paths in one run of the suite, and the limit is stated where the check is defined** — in `story_coordinator.py`'s module docstring and in `revert-check-result.schema.json`'s own `description`, following the narrowness `tests/test_baseline_honesty.py` states about itself. A set containing one forced repair is therefore permitted *in full*, added coverage in the other files of that set included, and a single file mixing a forced repair with added coverage is not caught at all. The record's `paths` field names exactly what was reverted, so the artifact reports what it checked rather than claiming discrimination it does not have. Per-file or per-hunk reversion was considered and is not the fix to reach for first: the negative-control case it would leave open — an assertion weakened as part of an otherwise-forced repair, where reverting still fails — is not closable by any granularity of reverting, and belongs to `prompts/verifier.md` and the verifier reading the diff, which it has done correctly in every recorded instance. +- A revert-check refusal escalates immediately without incrementing `retry_count`, matching the ownership violation it sits beside and for the same reason: the stage did not fail at its work, it produced something that is not its to produce, and rerunning the same instructions would produce it again. The reason names the stage, the prefixes and the governed paths in both `events.log` and `escalation-summary.md`. A permission is not silent either — one `revert-check-permitted` event is appended, so a run shows *why* an implementer was allowed into a governed prefix rather than only that it was. +- A check that cannot run escalates naming why; it never permits by default. A governed path with no version at HEAD makes the checkout fail, and `revert_check` records `ran: false` with the reason rather than treating an unbuildable clone as evidence of maintenance — the same stance the clean-clone check takes toward an unresolvable configured interpreter. `permitted` is *absent* from the record in that case rather than `false`, because a check that could not run permitted nothing and refused nothing; the validator subset has no union keyword, so absence is the honest encoding, as it is for the optional history fields. +- `revert-check-result.json` is evidence, never state. Nothing in orchestration reads it back — the routing is driven by the returned `RevertCheckResult`, exactly as `clean-clone-result.json` and `retry-history.json` and everything under `attempts/` route nothing. No stage name, no prefix and no artifact name introduced by this check appears in `orchestration/story_coordinator.py`; all three come off the loaded workflow definition and the story, per the standing rule. +- **Making a bad requirement harmless is not the same as stopping it being written, and the two are separate stories.** story-017 makes the over-strict sentence inert: once reverting decides permission, a verification requirement restating the stricter rule has nothing to adjudicate, because the coordinator has already permitted or refused the edit before the verifier is asked. The offending sentence is composed fresh by the planner into each story and is stored nowhere, so there was nothing to delete. Catching it *at plan time*, when the artifact is written, is what `.harness/requests/11-plan-time-validation.md` proposes and is not done here. The one line added to `planner.md`'s `[Workflow facts]` is a courtesy toward that day, not the enforcement — a written rule in an injected document has already been tried in this repository and failed, which is what `tests/test_baseline_honesty.py` exists because of. +- **Adding a stage declaration to the shipped workflow breaks the end-to-end tests of every check already declared on that stage, and the repair is to mirror the workflow, not to weaken the test.** `tests/test_story_007_validation.py`'s two ownership tests drive real `run_story` calls whose subject is that the ownership rule reads `created` alone; against the shipped workflow they now also pick up the new `revert_check` key, and their synthetic records name paths with no version at HEAD, so the revert check correctly escalates with *could not run* and the runs no longer complete. story-017 repaired them with an `ownership_only` helper built on that file's existing `mirror_harness`, which runs each against a copy of the workflow with the implementer's `revert_check` popped; every assertion and every record is unchanged and the docstring says why. That is the pattern to reuse: a test whose subject is one check is run against a workflow declaring only that check, so the next declaration does not silently retarget it. The general form is the story-013 ripple bullet above — a per-story test that goes red on landing gets repointed, never relaxed — and demonstrating the repair was *forced* is the same revert the check itself performs. - Verification rules never change between retries; retries narrow scope, they do not restart the workflow. - Capacity exhaustion (rate limits) is a reason to wait, not to fail; budget ceilings are a reason to stop. diff --git a/.harness/stories/story-017.yaml b/.harness/stories/story-017.yaml new file mode 100644 index 0000000..699d9f7 --- /dev/null +++ b/.harness/stories/story-017.yaml @@ -0,0 +1,179 @@ +story: + id: story-017 + title: Decide an implementer's test edits by reverting them + description: | + Four story artifacts assert that the implementer's changed-files record + lists nothing under tests/. The harness enforces something narrower - + may_not_create, creation only - and has never escalated on the + difference. The prose rule adds no enforcement and is sometimes + impossible to satisfy, because a legitimate implementer change can break + an existing test and the suite has to stay green. Every finding it has + produced was a deviation from prose: story-011 had to add a schema to two + inventories asserting exact set equality, story-013 could not carry the + rule at all because deleting those inventories was its deliverable, and + story-012 shipped a schema that turned five story-013 assertions red and + recorded the deviation as not clearable by a retry. + + Three separate causes - a schema inventory, a git baseline, a + working-tree endpoint - each removed by its own story, each followed by + another. Nobody can enumerate the next one. + + The distinction the rule was reaching for is not "the implementer must + not touch tests/" but "the implementer must not author its own + validation". Two acts get conflated by a path prefix: authoring coverage, + which must not happen in the implementer, and keeping existing validation + runnable, which is maintenance. Reverting separates them exactly, with no + judgement: maintenance is by definition the edit without which the suite + fails. + + So this story enforces that. After the implementer stage, an edit under a + prefix that stage declared it may not create is permitted iff reverting + it makes the suite fail. A forced repair breaks the suite when reverted; + new coverage does not. The machinery already exists - story-014's + clean-clone check clones the repository, applies the working tree, + commits, and runs the configured test command - so this is that same + operation with the governed paths restored from HEAD rather than applied. + + The prose rule is not stored anywhere and there is nothing to delete. + prompts/implementer.md already states the create/modify distinction + correctly and prompts/planner.md injects the enforced rule verbatim; the + offending sentence is composed fresh by the planner into each story. This + story does not try to stop it being written - that is plan-time + validation's job. It makes it inert: once reverting decides permission, a + verification requirement restating the stricter rule has nothing to + adjudicate, because the coordinator has already permitted or refused the + edit before the verifier is asked. + +tasks: + - Add a revert parameter to _build_clone that restores the named repository-relative paths from HEAD inside the clone, after the working tree diff is applied and before the commit, and pass it through run_clean_clone. It defaults to reverting nothing, so the clean-clone check's behavior is unchanged. + - Add a revert_check declaration to the implementer stage of workflows/story-workflow.json naming the artifact the check writes, following the verifier's clean_clone declaration - the key is what turns the check on, and no artifact name appears in orchestration code. + - Add the check to the coordinator, immediately after the existing stage output ownership check and inside the same changed_files block, so it runs on the one record whose tests/ edits are known to be the implementer's alone. + - Collect the governed paths from the stage's own record - the modified and deleted entries falling under the stage's may_not_create prefixes after the story's granted prefixes have been subtracted. created is not collected, because the ownership check has already escalated on it. + - Run nothing when no governed path is named. No clone is built, no suite is run, and no artifact is written. + - Run the configured test command once in a clone with every governed path reverted at once, and escalate when it passes - a set of edits none of which the suite needs is authorship, not maintenance. + - Escalate immediately without incrementing retry_count, matching the ownership violation it sits beside, with a reason naming the stage, the prefix and the governed paths. + - Escalate naming the reason when the check cannot run at all, matching how the clean-clone check treats an unresolvable configured interpreter. + - Add schemas/revert-check-result.schema.json and its schemas/manifest.json entry, following clean-clone-result - coordinator-written, in no stage's schemas map, optional fields expressed by absence rather than by null. + - Write the check's record to the run directory under the declared artifact name, carrying the governed paths, whether the edits were permitted, and the evidence behind that decision. + - Append one event when the check permits the edits, so the run can show why an implementer was allowed into a governed prefix rather than only that it was. + - State the granularity limit where the check is defined - in the module docstring and in the schema's own description - saying plainly that the decision is on the whole set and what that does not catch. + - Add one line to the [Workflow facts] section of prompts/planner.md telling a plan not to restate an injected restriction more strictly than the workflow declares it. + +acceptance_criteria: + - An implementer edit under a governed prefix that a legitimate change forces is permitted, and the run records why - demonstrated by a modification without which the suite fails. + - An implementer edit under a governed prefix that adds coverage is escalated, demonstrated by a test function that passes both before and after the implementer's change. + - The check treats a deleted governed path the same way as a modified one - deleting a test the implementer's change broke is permitted, and deleting a test that still passes is escalated. + - A run whose implementer record names no path under a governed prefix performs no extra work: no clone is built, no suite is run, and no revert-check artifact appears in the run directory. + - The governed prefixes come from the implementer stage's may_not_create declaration in the loaded workflow, with the story's granted prefixes subtracted, so a story granting an exception for a prefix is not subject to the revert check on that prefix either. + - No stage name, no prefix and no artifact name introduced by this check appears in orchestration/story_coordinator.py - all three are read off the loaded workflow definition and the story. + - Removing the revert_check declaration from the workflow definition disables the check with no change to orchestration code. + - A clean revert escalates immediately without incrementing retry_count, and the escalation reason names the stage, the prefix and the governed paths in both events.log and escalation-summary.md. + - A check that cannot run escalates naming why, rather than permitting the edits by default. + - The check reverts every governed path in one run and decides on that one result. The record states which paths were reverted, so a reader can tell what the decision covered. + - The granularity limit is stated in the check's own module docstring and in schemas/revert-check-result.schema.json's description - a set containing one forced repair is permitted in full, and a single file mixing a forced repair with added coverage is not caught. + - Constructing the case the granularity misses shows the check reporting what it checked rather than claiming more. + - schemas/revert-check-result.schema.json exists, is listed in schemas/manifest.json, appears in no stage's schemas map, and passes the existing parametrized draft-2020-12, unsupported-keyword and no-additionalProperties checks. + - Nothing in orchestration routes on the revert-check record. It is evidence, like clean-clone-result.json and retry-history.json. + - _build_clone's revert parameter defaults to reverting nothing, and the clean-clone check's observable behavior - the artifact it writes, the events it appends, and the routing it drives - is unchanged. + - The rendered planner prompt tells a plan not to restate an injected restriction more strictly than the workflow declares it, confirmed by rendering prompts/planner.md through orchestration/context_assembler.py rather than by reading the template. + - No story artifact under .harness/stories/ needs the prose rule this story supersedes in order for the suite to hold. + - The full test suite passes. + +technical_plan: + implementation_steps: + - Give _build_clone a keyword-only revert parameter defaulting to an empty sequence. After the git apply of the working tree diff and before git add -A, run git checkout HEAD -- inside the clone for the named paths, so the clone commits those files as HEAD has them while every other change is present. Reverting exactly the paths named is what keeps the record's claim and the clone's contents the same statement. + - Pass the same parameter through run_clean_clone, which stays the single build-a-clone-and-run-the-suite path. Both the clean-clone check and this one go through it. + - Add "revert_check" to the implementer stage in workflows/story-workflow.json naming revert-check-result.json, exactly as the verifier's clean_clone key names clean-clone-result.json. + - In run_story, inside the existing if record_name block and immediately after the ownership escalation, read stage.get("revert_check"). Do nothing when it is absent. The enforced prefix list is already computed there for the ownership check, with the story's granted prefixes subtracted and each grant logged - reuse that same list rather than recomputing it. + - Collect the governed paths with a small helper taking the record and the prefixes and returning the sorted modified and deleted entries falling under any of them. It names no prefix and no stage. + - When the list is empty, skip the check entirely - no clone, no suite, no artifact. The request is explicit that a check which can say nothing should not run. + - Otherwise call a revert_check function shaped like clean_clone_check - scratch directory from tempfile.mkdtemp, run_clean_clone with revert set to the governed paths, shutil.rmtree in a finally whatever the result - and write the record under the declared artifact name. + - Build the record from the CleanCloneResult the shared runner returns plus the governed paths and the permission decision. permitted is true when the suite failed with the edits reverted, which is what makes the edits maintenance. + - When the result did not run, escalate naming its reason. When it ran and the suite passed, escalate: the reason names the stage, the prefix, and the governed paths, and says the edits reverted clean. When it ran and the suite failed, append one event recording that the edits were permitted and fall through to the existing advance. + - Add schemas/revert-check-result.schema.json with ran, command, python and paths required, and python_version, clone_path, exit_code, output_tail, permitted and reason optional - absent rather than null, since the validator subset has no union keyword. The description carries the granularity limit. + - Add the schema's stem to schemas/manifest.json, which is a two-line edit under schemas/ and nothing else since story-013 moved the inventory out of tests/. + - Add the planner line to the [Workflow facts] section of prompts/planner.md beside the injected stage restrictions. It names no stage and no prefix, so the existing assertion that planner.md contains neither continues to hold. + likely_file_changes: + - file: orchestration/story_coordinator.py + stage: implementer + reason: The revert parameter on _build_clone and run_clean_clone, the check itself, the governed-path helper, the record, the event, and the escalations. + - file: workflows/story-workflow.json + stage: implementer + reason: The implementer stage's revert_check declaration, which is what turns the check on. + - file: schemas/revert-check-result.schema.json + stage: implementer + reason: The shape of the check's record, and where the granularity limit is stated for a reader of the artifact. + - file: schemas/manifest.json + stage: implementer + reason: The schemas inventory asserts exact set equality in both directions, so a new schema needs its entry. + - file: prompts/planner.md + stage: implementer + reason: One line telling a plan not to restate an injected restriction more strictly than the workflow declares it. + - file: tests/test_story_017_validation.py + stage: tester + reason: Independent validation of the permitted case, the escalated case, the deleted-path case, the no-governed-path case, the grant interaction, the declaration-driven wiring, and the stated granularity limit. + - file: .harness/docs/ARCHITECTURE.md + stage: documenter + reason: Records the revert check, why the decision rests on reverting rather than on a path prefix, the granularity it decides at, and what it does not catch. + +scope: + modify: + - orchestration/story_coordinator.py + - workflows/story-workflow.json + - schemas/revert-check-result.schema.json + - schemas/manifest.json + - prompts/planner.md + - tests/ + - .harness/docs/ARCHITECTURE.md + do_not_modify: + - rules/ + - orchestration/schema_validator.py + - orchestration/story_parser.py + - orchestration/context_assembler.py + - orchestration/harness_config.py + - orchestration/agent_runner.py + - orchestration/run_status.py + - scripts/ + - prompts/implementer.md + - prompts/tester.md + - prompts/verifier.md + - prompts/documenter.md + - prompts/harness-layer.md + - prompts/assist.md + - schemas/clean-clone-result.schema.json + - schemas/changed-files.schema.json + - schemas/story.schema.json + - .harness/stories/ + - .harness/runs-archive/ + - .harness/config.yaml + - .github/workflows/tests.yml + +verification_requirements: + - Confirm an implementer edit that a legitimate change forces is permitted, by constructing one - a modification without which the suite fails - and showing the check permits it and records why. + - Confirm an implementer edit that adds coverage is escalated, demonstrated by a test function that passes both before and after the implementer's change, so the only thing distinguishing it from a repair is that reverting it costs nothing. + - Confirm a deleted governed path is decided the same way, in both directions - a deleted test the change broke is permitted, a deleted test that still passes is escalated. + - Confirm a run whose implementer record names no governed path performs no extra work, by the absence of the artifact and by the check building no clone. + - Confirm the granularity limit by constructing the case it misses - a set mixing a forced repair with added coverage - and showing the check reports what it checked rather than claiming more. + - Confirm the escalation reason names the stage, the prefix and the governed paths in both events.log and escalation-summary.md, and that retry_count is unchanged by it. + - Confirm the check is driven entirely by the loaded workflow definition and the story, by removing the revert_check declaration and observing the check disappear with no code change, and by grepping orchestration/story_coordinator.py for the stage name, the prefix and the artifact name. + - Confirm the story's granted prefixes are subtracted before the check runs, so a story holding a stage exception for a prefix is not subject to the revert check on it. + - Confirm the clean-clone check is unaffected - same artifact, same events, same routing - with the revert parameter defaulting to reverting nothing. + - Confirm the new schema is listed in schemas/manifest.json, appears in no stage's schemas map, and that no orchestration code routes on the record. + - Confirm the planner guidance reaches a rendered prompt by rendering prompts/planner.md through orchestration/context_assembler.py, not by reading the template, and that planner.md still names no stage and no restricted prefix. + - Confirm no story artifact under .harness/stories/ was edited, and that no test in the suite depends on the prose rule this story supersedes. + - Confirm the full suite passes, in the working tree and in the clean clone. + +constraints: + - This story is not governed by the check it adds. The coordinator loads the workflow definition at run start, so the revert_check declaration written by this story's implementer will not be in the definition its own run already holds - the same gap story-007 hit with may_not_create and story-014 hit with clean_clone. Enforcement begins with the next story. This is expected, not a defect. + - Do not change may_not_create enforcement or any other declaration in the workflow definition. The creation rule is correct and has held throughout; this story adds a decision about modifications beside it, it does not replace it. + - Do not change the clean-clone check's behavior. The added parameter defaults to reverting nothing, and every artifact, event and routing decision that check drives stays exactly as it is. + - Do not write a second clone builder. The check is story-014's existing clone operation with the governed paths restored from HEAD instead of applied. + - Do not edit story-008, story-009, story-011, story-012 or story-013. They are execution records of what was written, not the source of it, and this story does not retroactively correct them. + - Do not add planner prompt guidance as the enforcement. The one line this story adds is a courtesy; the enforcement is the revert check. A written rule in an injected document has already been tried in this repository and failed, which is what tests/test_baseline_honesty.py exists because of. + - Do not close the mechanical hole at plan time. A story whose verification requirements restate a stage's may_not_create prefix more strictly than the workflow declares it is a planning error that .harness/requests/11-plan-time-validation.md proposes catching when the artifact is written. This story makes the requirement harmless; that one stops it being written. + - This story's own verification requirements must not restate the tests/ restriction more strictly than the workflow declares it. That would be the exact error the story exists to defuse. + - The check decides on the whole set of governed paths in one run. Do not add per-file or per-hunk reversion, and do not imply the check discriminates at a granularity it does not - state the limit where the check is defined, the way tests/test_baseline_honesty.py states its own narrowness. + - This does not replace the negative-control guidance. An implementer could weaken an assertion as part of an otherwise-forced repair, and reverting still fails; that is covered by prompts/verifier.md and by the verifier reading the diff, which it has done correctly in every recorded instance. + - The record is evidence, never state. Nothing routes on it, exactly as nothing routes on clean-clone-result.json or retry-history.json. + - Like clean-clone-result.json, the record is not archived on a retry, because it is named in no stage's outputs, changed_files or schemas map. That is a known cost carried deliberately, not an oversight to fix here. + - No test may be weakened or skipped to make the check pass, and any assertion this story adds about an absence needs a negative control demonstrating it can fail. diff --git a/orchestration/story_coordinator.py b/orchestration/story_coordinator.py index 7295902..32a9130 100644 --- a/orchestration/story_coordinator.py +++ b/orchestration/story_coordinator.py @@ -4,6 +4,16 @@ happen: it assembles context, invokes stage agents, saves state, and routes execution from structured artifacts. It never reasons; every decision here is a rule applied to a recorded fact. + +The revert check defined below decides at one granularity, and it is worth +knowing before reading its verdict as more than it is: it reverts the whole +set of governed paths in a single run of the suite and decides on that one +result. So a set containing one forced repair is permitted *in full*, added +coverage in the other files of that set included, and a single file mixing a +forced repair with added coverage is not caught at all. The record names the +paths that were reverted, so a reader can see exactly what the decision +covered. Reading the diff remains the verifier's job; this check bounds a +class of edit, it does not audit one. """ from __future__ import annotations @@ -500,7 +510,9 @@ def _interpreter_version(interpreter: Path) -> str | None: return version if result.returncode == 0 and _VERSION.fullmatch(version) else None -def _build_clone(target_root: Path, clone: Path) -> None: +def _build_clone( + target_root: Path, clone: Path, *, revert: list[str] | tuple[str, ...] = () +) -> None: """Clone the target locally and commit its working tree into the clone. A clone, not a tree copy: the point of the check is that the story is @@ -513,6 +525,12 @@ def _build_clone(target_root: Path, clone: Path) -> None: and untracked-but-not-ignored files — so the clone holds the same set of files _complete's `git add -A` would commit. The target repository is only read: every write happens inside the clone. + + `revert` names repository-relative paths to restore from HEAD *inside the + clone*, after the working tree has been applied and before the commit, so + the clone holds every change the working tree carries except those. It + defaults to reverting nothing, which is the clean-clone check's behavior + and is unchanged by its existence. """ result = subprocess.run( ["git", "clone", "--quiet", "--no-hardlinks", str(target_root), str(clone)], @@ -546,6 +564,14 @@ def _build_clone(target_root: Path, clone: Path) -> None: destination.parent.mkdir(parents=True, exist_ok=True) shutil.copy2(source, destination) + if revert: + reverted = _git(clone, "checkout", "HEAD", "--", *revert) + if reverted.returncode != 0: + raise RuntimeError( + f"Could not revert {', '.join(revert)} from HEAD in {clone}: " + f"{reverted.stderr.strip()}" + ) + _git(clone, "add", "-A") commit = _git( clone, @@ -599,6 +625,7 @@ def run_clean_clone( test_command: str, clean_clone_python: str | None, destination: Path, + revert: list[str] | tuple[str, ...] = (), ) -> CleanCloneResult: """Run the configured test command in a fresh clone with the story committed. @@ -607,6 +634,11 @@ def run_clean_clone( configuration names a `clean_clone_python`, so the check can exercise the oldest supported Python rather than whichever one the developer works in. The caller owns `destination` and removes it whatever the result. + + This is the single build-a-clone-and-run-the-suite path. `revert` is passed + through to the clone builder and defaults to reverting nothing, so the + clean-clone check runs exactly as it did; the revert check is this same + operation with the governed paths restored from HEAD rather than applied. """ argv = shlex.split(test_command) interpreter = clean_clone_python or argv[0] @@ -625,7 +657,7 @@ def run_clean_clone( ) clone = destination / "clone" - _build_clone(target_root, clone) + _build_clone(target_root, clone, revert=revert) _link_interpreter_roots(target_root, clone, [argv[0], interpreter]) result = subprocess.run( @@ -733,6 +765,145 @@ def _clean_clone_failures(output: str) -> str: return "; ".join(failures) +# -------------------------------------------------------------------------- +# The revert check +# +# A stage's may_not_create declaration says what it must not *add*. It says +# nothing about modifying or deleting, deliberately: a legitimate change can +# break an existing test, and the suite has to stay green. What must not +# happen is the stage authoring its own validation. The two acts are separated +# exactly, with no judgement, by reverting: maintenance is by definition the +# edit without which the suite fails. +# +# So an edit under a prefix the stage declared it may not create is permitted +# iff reverting it makes the suite fail. This is the clean-clone operation with +# the governed paths restored from HEAD rather than applied. +# +# Granularity. The check reverts every governed path in one run and decides on +# that one result — see the module docstring, which states plainly what that +# does not catch. +# -------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class GovernedEdits: + """The stage's own modifications and deletions under its governed prefixes. + + `created` is not collected: the ownership check has already escalated on + it, so anything reaching here is an edit to something that already existed. + """ + + paths: tuple[str, ...] + prefixes: tuple[str, ...] + + +def governed_edits( + run_dir: Path, record_name: str, prefixes: list[str] +) -> GovernedEdits: + """Read a stage's record for the edits the revert check decides on. + + Names no stage and no prefix; the caller passes the enforced list it has + already narrowed by the story's grants. Sorted, so the record and the + escalation reason are deterministic. + """ + changed = json.loads((run_dir / record_name).read_text(encoding="utf-8")) + paths, matched = set(), set() + for group in ("modified", "deleted"): + for path in changed.get(group, []): + for prefix in prefixes: + if path.startswith(prefix): + paths.add(path) + matched.add(prefix) + return GovernedEdits(tuple(sorted(paths)), tuple(sorted(matched))) + + +@dataclass(frozen=True) +class RevertCheckResult: + """What the revert check did, as it is recorded in the run directory. + + `permitted` is absent from the record when the check did not run, following + the optional-by-absence convention the other coordinator-written records + use: a check that could not run decided nothing, and null would claim it + decided something. + """ + + result: CleanCloneResult + paths: tuple[str, ...] + permitted: bool | None + + def as_record(self) -> dict: + record = {"ran": self.result.ran, "paths": list(self.paths)} + record.update( + {key: value for key, value in self.result.as_record().items() if key != "ran"} + ) + if self.permitted is not None: + record["permitted"] = self.permitted + return record + + +def revert_check( + run_dir: Path, + target_root: Path, + config: dict, + artifact: str, + paths: tuple[str, ...], +) -> RevertCheckResult: + """Run the suite once with every governed path reverted, and record it. + + Shaped like clean_clone_check: a scratch directory outside the target + repository, the shared runner, and removal in a finally whatever the + result. The decision is the suite's exit status — a non-zero exit means + the edits were needed, which is what makes them maintenance rather than + authorship. + + A clone that cannot be built at all (a governed path with no HEAD version, + say) is reported as a check that did not run, with the reason, rather than + as a permission. + """ + scratch = Path(tempfile.mkdtemp(prefix="l5-revert-check-")) + command = config["test_command"] + try: + result = run_clean_clone( + target_root, + command, + config.get("clean_clone_python"), + scratch, + revert=list(paths), + ) + except (RuntimeError, OSError) as error: + result = CleanCloneResult( + ran=False, + command=command, + python=config.get("clean_clone_python") or shlex.split(command)[0], + reason=f"the clone with the edits reverted could not be built: {error}", + ) + finally: + shutil.rmtree(scratch, ignore_errors=True) + + decided = RevertCheckResult( + result=result, + paths=paths, + permitted=(result.exit_code != 0) if result.ran else None, + ) + (run_dir / artifact).write_text( + json.dumps(decided.as_record(), indent=2) + "\n", encoding="utf-8" + ) + return decided + + +def _revert_check_permitted( + run_dir: Path, stage_name: str, artifact: str, edits: GovernedEdits +) -> None: + append_event( + run_dir, + f"{stage_name} edits under {', '.join(edits.prefixes)} permitted: the " + f"suite fails with {', '.join(edits.paths)} reverted", + kind="revert-check-permitted", + stage=stage_name, + artifacts=[artifact], + ) + + def _refuse(story_path: Path, problems: list[str]) -> int: """The one pre-flight refusal path: exit 1, one message per problem.""" print(f"{story_path} is not a valid story artifact:", file=sys.stderr) @@ -984,6 +1155,42 @@ def elapsed() -> float | None: duration_seconds=elapsed(), ) + # The revert check, on the same record and the same enforced + # prefixes the ownership check just used — the one record whose + # edits under those prefixes are known to be this stage's alone. + # The artifact name comes off the loaded workflow definition, so + # removing that declaration disables the check with no change here. + revert_artifact = stage.get("revert_check") + edits = ( + governed_edits(run_dir, record_name, enforced) + if revert_artifact + else GovernedEdits((), ()) + ) + if edits.paths: + prefixes = ", ".join(edits.prefixes) + listed = ", ".join(edits.paths) + decided = revert_check( + run_dir, target_root, config, revert_artifact, edits.paths + ) + if not decided.result.ran: + return _escalate( + run_dir, + state, + f"the revert check on {name}'s edits under {prefixes} " + f"could not run: {decided.result.reason}", + duration_seconds=elapsed(), + ) + if not decided.permitted: + return _escalate( + run_dir, + state, + f"{name} edited {listed} under {prefixes}, which it " + f"declared it must not create under, and the suite " + f"still passes with those edits reverted", + duration_seconds=elapsed(), + ) + _revert_check_permitted(run_dir, name, revert_artifact, edits) + if name == "verifier": verdict = json.loads((run_dir / "verification-result.json").read_text(encoding="utf-8")) state.verification_iterations += 1 diff --git a/prompts/planner.md b/prompts/planner.md index 846c85e..b5196ea 100644 --- a/prompts/planner.md +++ b/prompts/planner.md @@ -103,6 +103,13 @@ than the stage being validated: {{stage_create_restrictions}} +Restate an injected restriction exactly as the workflow declares it, or not +at all. A task, acceptance criterion or verification requirement that +tightens one — asking a stage to leave a path alone entirely when the +workflow only stops it adding files there — is not a stricter version of an +enforced rule; it is an unenforced rule the harness cannot see broken, and +one a legitimate change can make impossible to satisfy. + A stage_exceptions entry lifts one of those restrictions for one story, which is what a story whose own deliverable is a test suite needs. diff --git a/schemas/manifest.json b/schemas/manifest.json index 0104251..94333da 100644 --- a/schemas/manifest.json +++ b/schemas/manifest.json @@ -5,6 +5,7 @@ "execution-history", "retry-guidance", "retry-history", + "revert-check-result", "story", "test-results", "verification-result" diff --git a/schemas/revert-check-result.schema.json b/schemas/revert-check-result.schema.json new file mode 100644 index 0000000..f76f4f0 --- /dev/null +++ b/schemas/revert-check-result.schema.json @@ -0,0 +1,50 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "title": "revert-check-result", + "description": "The coordinator's record of the revert check: after a stage that declares both a changed-files record and a may_not_create list, its own modifications and deletions under those prefixes are permitted iff reverting them makes the suite fail. A forced repair breaks the suite when reverted; new coverage does not. Coordinator-written rather than stage-written, so it appears in no stage's schemas map and no agent is asked to satisfy it, and nothing in orchestration routes on it — it is evidence, like clean-clone-result. GRANULARITY: the check reverts every governed path at once and decides on that single run of the suite. A set containing one forced repair is therefore permitted in full, including any added coverage sitting in the other files of that set, and a single file mixing a forced repair with added coverage is not caught at all. The paths field states exactly what was reverted, so a reader can tell what the decision covered rather than assuming it discriminated per file. Optional fields are expressed by absence rather than by null, as clean-clone-result does: a check that could not run decided nothing.", + "type": "object", + "required": ["ran", "paths", "command", "python"], + "properties": { + "ran": { + "type": "boolean", + "description": "Whether the suite actually ran in the clone with the edits reverted. False when the check could not run, in which case reason says why and permitted, exit_code, output_tail and python_version are absent." + }, + "paths": { + "type": "array", + "description": "The repository-relative paths that were reverted from HEAD inside the clone, sorted: the stage's own modified and deleted entries falling under a prefix it declared it may not create, with the story's granted prefixes already subtracted. created entries are not collected, because the stage output ownership check has already escalated on them. This is the whole of what the decision below covers.", + "items": { "type": "string" } + }, + "command": { + "type": "string", + "description": "The command executed with the clone as its working directory, taken from the target repository's configured test_command with its interpreter replaced by the one named below." + }, + "python": { + "type": "string", + "description": "The interpreter the run used: .harness/config.yaml's clean_clone_python when that key is set, and test_command's own interpreter otherwise." + }, + "permitted": { + "type": "boolean", + "description": "Whether the edits are permitted. True when the suite failed with every path above reverted, which is what makes the set maintenance the change forced rather than validation the stage authored. False escalates the run immediately, without incrementing retry_count. Absent when ran is false, because a check that could not run permitted nothing and refused nothing." + }, + "python_version": { + "type": "string", + "description": "The version that interpreter reported, so a reader can tell which Python the check exercised. Absent when the interpreter reported no recognizable version, which is what a test command that is not a Python interpreter does." + }, + "clone_path": { + "type": "string", + "description": "Where the clone was built, under a temporary directory outside the target repository. The directory is removed once the run completes, so this identifies the run rather than naming a path to visit. Absent when no clone was built." + }, + "exit_code": { + "type": "integer", + "description": "The suite's exit status in the clone with the paths reverted. Non-zero is the evidence behind permitted being true; zero is the evidence behind it being false. Absent when ran is false." + }, + "output_tail": { + "type": "string", + "description": "The tail of the reverted run's combined stdout and stderr — for a permitted set, the failures the reverted edits were repairing. Absent when ran is false." + }, + "reason": { + "type": "string", + "description": "Why the check did not run, naming what stopped it. Absent when ran is true." + } + } +} diff --git a/tests/test_story_007_validation.py b/tests/test_story_007_validation.py index ef4cd79..e19d814 100644 --- a/tests/test_story_007_validation.py +++ b/tests/test_story_007_validation.py @@ -204,25 +204,47 @@ def test_an_ownership_escalation_does_not_increment_retry_count(target_root, assert state_of(target_root)["retry_count"] == 0 +def ownership_only(tmp_path: Path, harness_root: Path) -> Path: + """The shipped workflow with the implementer's revert_check declaration off. + + story-017 added a second check reading this same record: an edit under a + governed prefix is permitted only if reverting it makes the suite fail. + That is a decision about *modifications*, and these two tests are about the + ownership rule, which reads `created` alone. Removing the declaration takes + the newer check out of the picture — the subject, the record and the + assertions below are exactly what they were — so what they show is that + ownership does not escalate on a modification or a deletion. The revert + check's own behavior on those records is story-017's to demonstrate. + """ + workflow = harness_config.load_workflow(harness_root, "story-workflow") + for stage in workflow["stages"]: + stage.pop("revert_check", None) + return mirror_harness(tmp_path, harness_root, workflow) + + def test_an_implementer_modifying_an_existing_test_does_not_escalate(target_root, - harness_root): + harness_root, + tmp_path): """A changed signature must be allowed to leave the suite compiling.""" runner = Runner(target_root, records={ "implementer": {"modified": ["src/app.py", "tests/test_app.py"], "created": [], "deleted": []}, }) - code = story_coordinator.run_story("story-001", harness_root, target_root, runner) + fake_root = ownership_only(tmp_path, harness_root) + code = story_coordinator.run_story("story-001", fake_root, target_root, runner) assert code == 0 assert state_of(target_root)["status"] == "completed" def test_an_implementer_deleting_under_the_prefix_does_not_escalate(target_root, - harness_root): + harness_root, + tmp_path): runner = Runner(target_root, records={ "implementer": {"modified": [], "created": [], "deleted": ["tests/test_obsolete.py"]}, }) - assert story_coordinator.run_story("story-001", harness_root, target_root, runner) == 0 + fake_root = ownership_only(tmp_path, harness_root) + assert story_coordinator.run_story("story-001", fake_root, target_root, runner) == 0 def test_a_path_merely_containing_the_prefix_is_not_a_violation(target_root, diff --git a/tests/test_story_017_validation.py b/tests/test_story_017_validation.py new file mode 100644 index 0000000..7505e42 --- /dev/null +++ b/tests/test_story_017_validation.py @@ -0,0 +1,965 @@ +"""Independent validation for story-017: deciding an implementer's test +edits by reverting them. + +Written from the story's acceptance criteria. The subject is a decision +about *edits*, so almost nothing here is asserted from source: a target +repository with a real pytest suite is built under tmp_path, a fake +implementer edits its working tree, and the coordinator is run. Whether an +edit is permitted is then whatever the suite does in a clone with that edit +restored from HEAD - the same question the check asks, answered by running +it rather than by reading the code that runs it. + +The two premises the routing rests on are reconstructed first, before any +routing is asserted: + + * the forced repair really is forced - reverting `tests/test_app.py` + alone makes the suite fail, and the same clone with nothing reverted + passes; and + * the added coverage really is coverage - the test function the fake + implementer appends passes against the module both before and after the + implementer's change, so the only thing separating it from a repair is + that reverting it costs nothing. + +Without those two, a check that permitted everything and a check that +permitted nothing would both look green below. + +Every absence asserted here carries a control. "No artifact is written" +sits beside a run that writes one; "no clone is built" is a count against a +run that builds one; "the artifact name appears in no orchestration module" +is paired with the name that does appear; "this story edited no story +artifact" is paired with the file it did edit. + +Nothing here invokes a model: every run goes through a fake agent runner, +and every clone source is a local filesystem path. +""" +import inspect +import json +import re +import shlex +import subprocess +import sys +from pathlib import Path + +import pytest + +from conftest import STORY, story_diff + +import context_assembler +import harness_config +import schema_validator +import story_coordinator +from agent_runner import AgentResult + +REPO_ROOT = Path(__file__).resolve().parents[1] +ORCHESTRATION = REPO_ROOT / "orchestration" +STORIES_DIR = REPO_ROOT / ".harness" / "stories" +TESTS_DIR = REPO_ROOT / "tests" + +WORKFLOW = harness_config.load_workflow(REPO_ROOT, "story-workflow") +IMPLEMENTER_STAGE = next(s for s in WORKFLOW["stages"] if s["name"] == "implementer") +#: The artifact name and the governed prefix are read off the workflow, never +#: spelled here, for the same reason the coordinator may not spell them. +ARTIFACT = IMPLEMENTER_STAGE.get("revert_check") +PREFIX = IMPLEMENTER_STAGE["may_not_create"][0] + +SCHEMA_STEM = "revert-check-result" +SCHEMA_PATH = REPO_ROOT / "schemas" / f"{SCHEMA_STEM}.schema.json" + +PASS = {"status": "passed", "blocking_issues": [], "unverified": [], + "retry_recommended": False} + +TEST_COMMAND = shlex.join([sys.executable, "-m", "pytest", "tests", "-q", + "-p", "no:cacheprovider"]) + +CONFIG = f"""\ +project: suite-target +workflow: story-workflow +branch_prefix: story/ +permission_mode: acceptEdits +stories_dir: .harness/stories +runs_dir: .harness/runs +logs_dir: .harness/logs +standards_dir: .harness/standards +architecture_docs: + - .harness/docs/ARCHITECTURE.md +test_command: {TEST_COMMAND} +""" + +# -------------------------------------------------------------------------- +# The target repository: a real module and a real suite over it. +# +# HEAD holds `greet` and a test calling it. The two implementer changes below +# are the two cases the check must tell apart, reduced to their smallest +# honest form: a rename, which forces the test to change, and an addition, +# which forces nothing. +# -------------------------------------------------------------------------- + +APP_AT_HEAD = '''\ +def greet(name): + return f"hello, {name}" +''' + +APP_RENAMED = '''\ +def salute(name): + return f"hello, {name}" +''' + +APP_ADDITIVE = APP_AT_HEAD + ''' + +def shout(name): + return greet(name).upper() +''' + +TEST_APP_AT_HEAD = '''\ +from app import greet + + +def test_greet(): + assert greet("world") == "hello, world" +''' + +TEST_APP_REPAIRED = '''\ +from app import salute + + +def test_greet(): + assert salute("world") == "hello, world" +''' + +#: A test function that passes against APP_AT_HEAD and against APP_ADDITIVE +#: alike. Appended to an existing file, it is a modification under the +#: governed prefix that no change forced. +ADDED_COVERAGE = ''' + +def test_greet_again(): + assert greet("again") == "hello, again" +''' + +#: Added coverage that depends on nothing in the module at all, for the +#: file that mixes a forced repair with an addition. +INDEPENDENT_COVERAGE = ''' + +def test_addition_is_still_addition(): + assert 1 + 1 == 2 +''' + +TEST_EXTRA_AT_HEAD = '''\ +def test_arithmetic(): + assert 1 + 1 == 2 +''' + +TEST_EXTRA_PLUS_COVERAGE = TEST_EXTRA_AT_HEAD + ''' + +def test_arithmetic_again(): + assert 2 + 2 == 4 +''' + +ROOT_CONFTEST = '''\ +import os +import sys + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "src")) +''' + + +def write(path: Path, text: str) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + + +def write_json(path: Path, payload) -> None: + write(path, json.dumps(payload, indent=2) + "\n") + + +def git(root: Path, *args: str) -> subprocess.CompletedProcess: + return subprocess.run(["git", "-C", str(root), *args], + capture_output=True, text=True, check=True) + + +@pytest.fixture +def target(tmp_path: Path) -> Path: + """A target repository whose configured test command is a real suite.""" + root = tmp_path / "suite-target" + for sub in (".harness/standards", ".harness/stories", ".harness/runs", + ".harness/logs", ".harness/docs"): + (root / sub).mkdir(parents=True) + write(root / ".harness" / "config.yaml", CONFIG) + write(root / ".harness" / "stories" / "story-001.yaml", STORY) + write(root / ".harness" / "standards" / "coding.md", "# Coding\n- simple\n") + write(root / ".harness" / "standards" / "testing.md", "# Testing\n- test it\n") + write(root / ".harness" / "docs" / "ARCHITECTURE.md", "# Architecture\n") + write(root / "conftest.py", ROOT_CONFTEST) + write(root / "src" / "app.py", APP_AT_HEAD) + write(root / "tests" / "test_app.py", TEST_APP_AT_HEAD) + write(root / "tests" / "test_extra.py", TEST_EXTRA_AT_HEAD) + write(root / ".gitignore", ".pytest_cache/\n__pycache__/\n") + subprocess.run(["git", "init", "-q"], cwd=root, check=True) + subprocess.run(["git", "config", "user.email", "t@example.com"], cwd=root, check=True) + subprocess.run(["git", "config", "user.name", "T"], cwd=root, check=True) + subprocess.run(["git", "add", "-A"], cwd=root, check=True) + subprocess.run(["git", "commit", "-q", "-m", "initial"], cwd=root, check=True) + return root + + +@pytest.fixture +def harness_root() -> Path: + return REPO_ROOT + + +# -------------------------------------------------------------------------- +# The implementer's working-tree changes, each paired with the record that +# describes it. The record and the tree always say the same thing: the check +# reads the record and reverts inside a clone of the tree. +# -------------------------------------------------------------------------- + + +def forced_repair(root: Path) -> dict: + """A rename the existing test cannot survive, and the test updated.""" + write(root / "src" / "app.py", APP_RENAMED) + write(root / "tests" / "test_app.py", TEST_APP_REPAIRED) + return {"modified": ["src/app.py", "tests/test_app.py"], "created": [], + "deleted": []} + + +def added_coverage(root: Path) -> dict: + """An addition to the module, and a test the addition did not force.""" + write(root / "src" / "app.py", APP_ADDITIVE) + write(root / "tests" / "test_app.py", TEST_APP_AT_HEAD + ADDED_COVERAGE) + return {"modified": ["src/app.py", "tests/test_app.py"], "created": [], + "deleted": []} + + +def deleted_broken_test(root: Path) -> dict: + """The same rename, with the broken test deleted rather than repaired.""" + write(root / "src" / "app.py", APP_RENAMED) + (root / "tests" / "test_app.py").unlink() + return {"modified": ["src/app.py"], "created": [], + "deleted": ["tests/test_app.py"]} + + +def deleted_passing_test(root: Path) -> dict: + """A deletion nothing forced: the test still passes after the change.""" + write(root / "src" / "app.py", APP_ADDITIVE) + (root / "tests" / "test_extra.py").unlink() + return {"modified": ["src/app.py"], "created": [], + "deleted": ["tests/test_extra.py"]} + + +def mixed_set(root: Path) -> dict: + """One forced repair and one addition, in two different governed files.""" + write(root / "src" / "app.py", APP_RENAMED) + write(root / "tests" / "test_app.py", TEST_APP_REPAIRED) + write(root / "tests" / "test_extra.py", TEST_EXTRA_PLUS_COVERAGE) + return {"modified": ["src/app.py", "tests/test_app.py", "tests/test_extra.py"], + "created": [], "deleted": []} + + +def mixed_file(root: Path) -> dict: + """One forced repair and one addition, inside a single governed file.""" + write(root / "src" / "app.py", APP_RENAMED) + write(root / "tests" / "test_app.py", TEST_APP_REPAIRED + INDEPENDENT_COVERAGE) + return {"modified": ["src/app.py", "tests/test_app.py"], "created": [], + "deleted": []} + + +def nothing_governed(root: Path) -> dict: + """A change that names no path under the governed prefix at all.""" + write(root / "src" / "app.py", APP_ADDITIVE) + return {"modified": ["src/app.py"], "created": [], "deleted": []} + + +def ghost_path(root: Path) -> dict: + """A governed path with no version at HEAD, so the revert cannot happen.""" + write(root / "src" / "app.py", APP_ADDITIVE) + return {"modified": ["src/app.py", "tests/test_ghost.py"], "created": [], + "deleted": []} + + +class Runner: + """A fake agent runner: each stage writes its artifacts, and the stage + holding an edit also makes that edit in the target's working tree.""" + + def __init__(self, target_root: Path, edits: dict, story_id: str = "story-001"): + self.target_root = target_root + self.run_dir = target_root / ".harness" / "runs" / story_id + self.edits = edits + self.records: dict[str, dict] = {} + self.calls: list[str] = [] + + def _record(self, stage: str) -> dict: + edit = self.edits.get(stage) + record = edit(self.target_root) if edit else {"modified": [], "created": [], + "deleted": []} + self.records[stage] = record + return record + + def __call__(self, prompt, *, stage, cwd=None, log_path=None, + permission_mode=None, model=None, allowed_tools=None): + self.calls.append(stage) + if stage == "implementer": + write_json(self.run_dir / "changed-files.json", self._record(stage)) + write(self.run_dir / "implementation-summary.md", "Did it.\n") + elif stage == "tester": + write_json(self.run_dir / "test-results.json", { + "status": "passed", "tests_written": 1, "tests_run": 2, + "tests_passed": 2, "tests_failed": 0, "failures": [], + }) + write_json(self.run_dir / "tester-changed-files.json", self._record(stage)) + elif stage == "verifier": + write_json(self.run_dir / "verification-result.json", PASS) + elif stage == "documenter": + write(self.run_dir / "documentation-report.md", "Nothing.\n") + return AgentResult(ok=True, result_text=f"{stage} done") + + +def run_dir_of(target_root: Path, story_id: str = "story-001") -> Path: + return target_root / ".harness" / "runs" / story_id + + +def state_of(target_root: Path) -> dict: + return json.loads((run_dir_of(target_root) / "state.json").read_text()) + + +def evidence(target_root: Path) -> tuple[str, str]: + """The two places an escalation reason must appear.""" + run_dir = run_dir_of(target_root) + return ((run_dir / "events.log").read_text(), + (run_dir / "escalation-summary.md").read_text()) + + +def record_of(target_root: Path, artifact: str = ARTIFACT) -> dict: + return json.loads((run_dir_of(target_root) / artifact).read_text()) + + +def run(target_root: Path, harness: Path, edits: dict) -> tuple[int, Runner]: + runner = Runner(target_root, edits) + code = story_coordinator.run_story("story-001", harness, target_root, runner) + return code, runner + + +def configure(target_root: Path, **overrides) -> None: + path = target_root / ".harness" / "config.yaml" + lines = path.read_text(encoding="utf-8").splitlines() + for key, value in overrides.items(): + for index, line in enumerate(lines): + if line.startswith(f"{key}:"): + lines[index] = f"{key}: {value}" + break + else: + lines.append(f"{key}: {value}") + path.write_text("\n".join(lines) + "\n", encoding="utf-8") + + +def mirror_harness(tmp_path: Path, workflow: dict) -> Path: + """A harness root identical to the real one but for its workflow file.""" + fake = tmp_path / "harness" + (fake / "workflows").mkdir(parents=True) + for shared in ("prompts", "schemas", "rules"): + (fake / shared).symlink_to(REPO_ROOT / shared) + write_json(fake / "workflows" / "story-workflow.json", workflow) + return fake + + +def loaded_workflow() -> dict: + return harness_config.load_workflow(REPO_ROOT, "story-workflow") + + +def append_to_story(target_root: Path, text: str) -> None: + path = target_root / ".harness" / "stories" / "story-001.yaml" + path.write_text(path.read_text() + text, encoding="utf-8") + + +def executable_source(text: str) -> str: + """Strip docstrings and comment lines; prose may name what code may not.""" + kept, in_docstring = [], False + for line in text.splitlines(): + stripped = line.lstrip() + if stripped.startswith('"""') or stripped.startswith("'''"): + if not (len(stripped) > 3 and stripped.rstrip().endswith('"""') + and stripped.rstrip() != '"""'): + in_docstring = not in_docstring + continue + if in_docstring or stripped.startswith("#"): + continue + kept.append(line) + return "\n".join(kept) + + +@pytest.fixture +def clone_calls(monkeypatch): + """Every call into the shared clone runner, with what it reverted. + + The verifier's clean-clone check goes through the same runner, so the + counts below are read as "a run with something reverted" rather than as + "a run at all" - which is also how the no-governed-path criterion has to + be read, since that run still performs the clean-clone check. + """ + calls: list[tuple[str, ...]] = [] + original = story_coordinator.run_clean_clone + + def spy(*args, **kwargs): + revert = kwargs.get("revert", args[4] if len(args) > 4 else ()) + calls.append(tuple(revert)) + return original(*args, **kwargs) + + monkeypatch.setattr(story_coordinator, "run_clean_clone", spy) + return calls + + +@pytest.fixture +def builds(monkeypatch): + """Every clone the coordinator builds during a run.""" + built: list[tuple[str, ...]] = [] + original = story_coordinator._build_clone + + def spy(target_root, clone, *, revert=()): + built.append(tuple(revert)) + return original(target_root, clone, revert=revert) + + monkeypatch.setattr(story_coordinator, "_build_clone", spy) + return built + + +def suite_in(directory: Path) -> int: + """Run the same suite shape the target configures, in a scratch tree.""" + return subprocess.run( + [sys.executable, "-m", "pytest", "tests", "-q", "-p", "no:cacheprovider"], + cwd=directory, capture_output=True, text=True, + ).returncode + + +# -------------------------------------------------------------------------- +# The premises: the fixtures really are a forced repair and free coverage +# -------------------------------------------------------------------------- + + +def test_reverting_the_repair_fails_the_suite_and_reverting_nothing_passes( + target, tmp_path, +): + """The premise under every permitted case below. Without the second half + of this the check could be failing the suite for some unrelated reason, + and "permitted" would mean nothing.""" + forced_repair(target) + + reverted = story_coordinator.run_clean_clone( + target, TEST_COMMAND, None, tmp_path / "with-revert", + revert=["tests/test_app.py"]) + intact = story_coordinator.run_clean_clone( + target, TEST_COMMAND, None, tmp_path / "no-revert") + + assert reverted.ran is True and reverted.exit_code != 0 + assert intact.ran is True and intact.exit_code == 0 + + +def test_the_added_test_passes_against_the_module_before_and_after(tmp_path): + """The premise under every escalated case: the appended test function is + coverage, not repair - the implementer's change neither forced it nor + breaks it.""" + for name, module in (("before", APP_AT_HEAD), ("after", APP_ADDITIVE)): + scratch = tmp_path / name + write(scratch / "conftest.py", ROOT_CONFTEST) + write(scratch / "src" / "app.py", module) + write(scratch / "tests" / "test_app.py", TEST_APP_AT_HEAD + ADDED_COVERAGE) + assert suite_in(scratch) == 0, name + + +# -------------------------------------------------------------------------- +# A forced edit is permitted; free coverage is escalated +# -------------------------------------------------------------------------- + + +def test_a_forced_edit_under_the_governed_prefix_is_permitted(target, harness_root): + code, runner = run(target, harness_root, {"implementer": forced_repair}) + assert code == 0 + assert state_of(target)["status"] == "completed" + assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + + record = record_of(target) + assert record["ran"] is True + assert record["permitted"] is True + assert record["exit_code"] != 0 + assert record["paths"] == ["tests/test_app.py"] + + +def test_the_run_records_why_the_forced_edit_was_permitted(target, harness_root): + """A reader must be able to see why an implementer was allowed into the + prefix, not only that it was.""" + assert run(target, harness_root, {"implementer": forced_repair})[0] == 0 + events = (run_dir_of(target) / "events.log").read_text() + permitting = [line for line in events.splitlines() if "permitted" in line] + assert len(permitting) == 1 + assert "implementer" in permitting[0] + assert PREFIX in permitting[0] + assert "tests/test_app.py" in permitting[0] + assert record_of(target)["output_tail"] + + +def test_an_edit_that_only_adds_coverage_is_escalated(target, harness_root): + code, runner = run(target, harness_root, {"implementer": added_coverage}) + assert code == 2 + assert state_of(target)["status"] == "escalated" + assert runner.calls == ["implementer"] + + record = record_of(target) + assert record["permitted"] is False + assert record["exit_code"] == 0 + assert record["paths"] == ["tests/test_app.py"] + + +def test_a_deleted_governed_path_the_change_broke_is_permitted(target, harness_root): + code, _ = run(target, harness_root, {"implementer": deleted_broken_test}) + assert code == 0 + record = record_of(target) + assert record["permitted"] is True + assert record["paths"] == ["tests/test_app.py"] + + +def test_deleting_a_governed_path_that_still_passes_is_escalated(target, harness_root): + code, _ = run(target, harness_root, {"implementer": deleted_passing_test}) + assert code == 2 + record = record_of(target) + assert record["permitted"] is False + assert record["paths"] == ["tests/test_extra.py"] + + +def test_the_escalation_names_the_stage_the_prefix_and_the_paths(target, harness_root): + assert run(target, harness_root, {"implementer": added_coverage})[0] == 2 + events, summary = evidence(target) + for text in (events, summary): + assert "implementer" in text + assert PREFIX in text + assert "tests/test_app.py" in text + + +def test_the_escalation_does_not_increment_retry_count(target, harness_root): + """It escalates the way the ownership violation beside it does, not the + way a failed verification does.""" + assert run(target, harness_root, {"implementer": added_coverage})[0] == 2 + state = state_of(target) + assert state["status"] == "escalated" + assert state["retry_count"] == 0 + + +# -------------------------------------------------------------------------- +# A record naming no governed path costs nothing +# -------------------------------------------------------------------------- + + +def test_a_record_naming_no_governed_path_builds_no_clone_and_writes_nothing( + target, harness_root, clone_calls, builds, +): + code, _ = run(target, harness_root, {"implementer": nothing_governed}) + assert code == 0 + assert not (run_dir_of(target) / ARTIFACT).exists() + # The one clone and the one suite run are the verifier's clean-clone + # check, which reverts nothing; the revert check contributed neither. + assert clone_calls == [()] + assert builds == [()] + + +def test_the_same_run_with_a_governed_path_does_build_a_clone_and_write_one( + target, harness_root, clone_calls, builds, +): + """The control for the assertion above: identical machinery, one governed + path added, and the clone, the suite run and the artifact all appear.""" + code, _ = run(target, harness_root, {"implementer": forced_repair}) + assert code == 0 + assert (run_dir_of(target) / ARTIFACT).exists() + assert ("tests/test_app.py",) in clone_calls + assert ("tests/test_app.py",) in builds + + +# -------------------------------------------------------------------------- +# The check is driven by the declaration, not by the code +# -------------------------------------------------------------------------- + + +def test_removing_the_declaration_disables_the_check(target, tmp_path, clone_calls): + """The record that escalates against the shipped workflow completes + against the same workflow with one key removed - no code change.""" + workflow = loaded_workflow() + for stage in workflow["stages"]: + stage.pop("revert_check", None) + fake_root = mirror_harness(tmp_path / "no-declaration", workflow) + + code, _ = run(target, fake_root, {"implementer": added_coverage}) + assert code == 0 + assert not (run_dir_of(target) / ARTIFACT).exists() + assert clone_calls == [()] + + +def test_moving_the_declaration_moves_the_check(target, tmp_path): + """The strongest form of "no stage name and no prefix in the code": a + workflow the coordinator has never seen, governing a different prefix on + a different stage, and the check follows the declaration.""" + workflow = loaded_workflow() + for stage in workflow["stages"]: + stage.pop("may_not_create", None) + stage.pop("revert_check", None) + if stage["name"] == "tester": + stage["may_not_create"] = ["src/"] + stage["revert_check"] = ARTIFACT + fake_root = mirror_harness(tmp_path / "moved", workflow) + + # The implementer's edit under tests/ is now ungoverned; the tester's + # edit under src/ is the one decided, and nothing forced it. + code, runner = run(target, fake_root, {"implementer": added_coverage, + "tester": nothing_governed}) + assert code == 2 + assert runner.calls == ["implementer", "tester"] + record = record_of(target) + assert record["permitted"] is False + assert record["paths"] == ["src/app.py"] + events, summary = evidence(target) + for text in (events, summary): + assert "tester" in text + assert "src/" in text + assert "src/app.py" in text + + +def test_no_stage_name_no_prefix_and_no_artifact_name_is_written_in_the_code(): + """All three are read off the loaded workflow and the story. Docstrings + and comments are stripped first: prose may name what code may not.""" + body = executable_source( + (ORCHESTRATION / "story_coordinator.py").read_text(encoding="utf-8")) + assert PREFIX not in body + assert ARTIFACT not in body + for stage in WORKFLOW["stages"]: + if stage["name"] == "verifier": + continue # the verifier routing branch predates this story + assert stage["name"] not in body, stage["name"] + + +def test_the_governed_path_helper_names_no_stage_and_no_prefix(): + body = executable_source(inspect.getsource(story_coordinator.governed_edits)) + assert "modified" in body and "deleted" in body # stripping kept code + assert PREFIX not in body + for stage in WORKFLOW["stages"]: + assert stage["name"] not in body, stage["name"] + + +# -------------------------------------------------------------------------- +# The story's granted prefixes are subtracted first +# -------------------------------------------------------------------------- + + +def test_a_story_granting_the_prefix_is_not_subject_to_the_check_on_it( + target, harness_root, clone_calls, +): + append_to_story(target, ( + "\nstage_exceptions:\n" + " - stage: implementer\n" + f" create: {PREFIX}\n" + " reason: the deliverable is the suite\n" + )) + code, _ = run(target, harness_root, {"implementer": added_coverage}) + assert code == 0 + assert not (run_dir_of(target) / ARTIFACT).exists() + assert clone_calls == [()] + + +def test_without_the_grant_the_same_record_escalates(target, harness_root): + """The control for the grant: the story is the only difference.""" + assert run(target, harness_root, {"implementer": added_coverage})[0] == 2 + + +# -------------------------------------------------------------------------- +# A check that cannot run refuses rather than permits +# -------------------------------------------------------------------------- + + +def test_a_governed_path_that_cannot_be_reverted_escalates_naming_why( + target, harness_root, +): + """A record naming a governed path with no version at HEAD. The clone + cannot be built, so there is no suite result to read - and the check says + so instead of letting the edits through.""" + code, _ = run(target, harness_root, {"implementer": ghost_path}) + assert code == 2 + record = record_of(target) + assert record["ran"] is False + assert "permitted" not in record + assert record["reason"] + _, summary = evidence(target) + assert "could not run" in summary + assert "tests/test_ghost.py" in summary + + +def test_an_unresolvable_configured_interpreter_escalates_naming_why( + target, harness_root, +): + """The same treatment the clean-clone check gives it.""" + configure(target, clean_clone_python="nowhere/python") + code, _ = run(target, harness_root, {"implementer": forced_repair}) + assert code == 2 + record = record_of(target) + assert record["ran"] is False + assert "permitted" not in record + assert "nowhere/python" in record["reason"] + _, summary = evidence(target) + assert "could not run" in summary + + +# -------------------------------------------------------------------------- +# The granularity the check decides at, and what it does not catch +# -------------------------------------------------------------------------- + + +def test_a_set_containing_one_forced_repair_is_permitted_in_full( + target, harness_root, +): + """The limit, constructed: two governed files, one forced and one not. + The check reverts both at once, the suite fails, and the whole set is + permitted - the addition included.""" + code, _ = run(target, harness_root, {"implementer": mixed_set}) + assert code == 0 + record = record_of(target) + assert record["permitted"] is True + assert record["paths"] == ["tests/test_app.py", "tests/test_extra.py"] + + +def test_the_addition_in_that_set_was_not_forced_by_anything(target, tmp_path): + """What the test above would look like if the check discriminated per + file: reverting the addition alone leaves the suite green. The check + reports the set it reverted rather than claiming it decided per file.""" + mixed_set(target) + alone = story_coordinator.run_clean_clone( + target, TEST_COMMAND, None, tmp_path / "extra-only", + revert=["tests/test_extra.py"]) + assert alone.ran is True + assert alone.exit_code == 0 + + +def test_a_single_file_mixing_a_repair_and_an_addition_is_permitted( + target, harness_root, +): + """The case the granularity misses outright: one file, both acts. The + record names the file it reverted and claims nothing about its hunks.""" + code, _ = run(target, harness_root, {"implementer": mixed_file}) + assert code == 0 + record = record_of(target) + assert record["permitted"] is True + assert record["paths"] == ["tests/test_app.py"] + assert set(record) <= {"ran", "paths", "command", "python", "python_version", + "clone_path", "exit_code", "output_tail", "permitted", + "reason"} + + +def test_the_addition_inside_that_file_needed_no_change_to_pass(tmp_path): + """The control for the case above: the appended function passes against + the module as HEAD has it, so nothing about the implementer's change + forced it into the file.""" + scratch = tmp_path / "independent" + write(scratch / "conftest.py", ROOT_CONFTEST) + write(scratch / "src" / "app.py", APP_AT_HEAD) + write(scratch / "tests" / "test_app.py", TEST_APP_AT_HEAD + INDEPENDENT_COVERAGE) + assert suite_in(scratch) == 0 + + +def test_the_granularity_limit_is_stated_where_the_check_is_defined(): + """In the module docstring and in the schema's own description, so a + reader of either the code or the artifact learns it without inferring + it.""" + schema = json.loads(SCHEMA_PATH.read_text(encoding="utf-8")) + for text in (story_coordinator.__doc__, schema["description"]): + lowered = text.lower() + assert any(phrase in lowered for phrase in + ("whole set", "every governed path at once", "in a single run", + "at once")), lowered + assert "in full" in lowered + assert "not caught" in lowered + assert "mixing" in lowered + + +# -------------------------------------------------------------------------- +# The schema, the inventory, and the record as evidence +# -------------------------------------------------------------------------- + + +def test_the_schema_exists_and_is_listed_in_the_manifest(): + manifest = json.loads( + (REPO_ROOT / "schemas" / "manifest.json").read_text(encoding="utf-8")) + assert SCHEMA_PATH.is_file() + assert SCHEMA_STEM in manifest["schemas"] + assert schema_validator.load_schema(SCHEMA_STEM)["title"] == SCHEMA_STEM + + +def test_the_schema_appears_in_no_stages_schemas_map(): + """No agent is asked to satisfy it: the coordinator writes it. The + control is the record that *is* in a stage's map, so a lookup that had + stopped seeing anything would fail here.""" + mapped = {name for stage in WORKFLOW["stages"] + for name in stage.get("schemas", {}).values()} + assert SCHEMA_STEM not in mapped + assert "changed-files" in mapped + + +def test_the_written_record_satisfies_the_schema(target, harness_root): + assert run(target, harness_root, {"implementer": forced_repair})[0] == 0 + record = record_of(target) + schema = schema_validator.load_schema(SCHEMA_STEM) + assert schema_validator.validate(record, schema) == [] + # The control: the same validator against the same schema rejects a + # record missing what the check must always report. + incomplete = {key: value for key, value in record.items() if key != "paths"} + assert schema_validator.validate(incomplete, schema) != [] + + +def test_nothing_in_orchestration_reads_the_record_back(target, harness_root): + """It is evidence, like clean-clone-result.json and retry-history.json. + The control is clean-clone-result.json, which orchestration *does* name - + so a scan that had stopped matching anything would fail here.""" + named = {module.name: module.read_text(encoding="utf-8") + for module in sorted(ORCHESTRATION.glob("*.py"))} + assert not [name for name, text in named.items() if ARTIFACT in text] + assert [name for name, text in named.items() if "clean-clone-result.json" in text] + + +def test_the_record_is_not_injected_into_any_stage_prompt(target, harness_root): + """Routing and context are the two ways a record could become state.""" + assert run(target, harness_root, {"implementer": forced_repair})[0] == 0 + context = context_assembler.build_context( + story_text=STORY, + story={"acceptance_criteria": []}, + run_dir=run_dir_of(target), + target_root=target, + harness_root=REPO_ROOT, + config=harness_config.load_config(target), + rules=harness_config.load_rules(REPO_ROOT), + retry_count=0, + ) + injected = {key: value for key, value in context.items() + if value and "revert" in str(value) and not key.endswith("_schema")} + assert injected == {} + # The control: the clean-clone record is injected, by the key that names it. + assert context["clean_clone_result"] + + +# -------------------------------------------------------------------------- +# The clean-clone check is unaffected +# -------------------------------------------------------------------------- + + +def test_the_clone_builders_revert_parameter_defaults_to_reverting_nothing(): + for function in (story_coordinator._build_clone, story_coordinator.run_clean_clone): + default = inspect.signature(function).parameters["revert"].default + assert tuple(default) == () + + +def test_a_clone_built_with_the_default_carries_the_edit_and_one_reverted_does_not( + target, tmp_path, +): + """The behavioral half: same builder, same tree, one parameter apart.""" + forced_repair(target) + story_coordinator._build_clone(target, tmp_path / "default") + story_coordinator._build_clone(target, tmp_path / "reverted", + revert=["tests/test_app.py"]) + + assert "salute" in git(tmp_path / "default", "show", + "HEAD:tests/test_app.py").stdout + assert "salute" not in git(tmp_path / "reverted", "show", + "HEAD:tests/test_app.py").stdout + # Everything outside the reverted path is present in both. + for clone in ("default", "reverted"): + assert "salute" in git(tmp_path / clone, "show", "HEAD:src/app.py").stdout + + +def test_the_clean_clone_check_still_writes_its_record_and_event_and_routes( + target, harness_root, +): + verifier = next(s for s in WORKFLOW["stages"] if s["name"] == "verifier") + assert run(target, harness_root, {"implementer": forced_repair})[0] == 0 + run_dir = run_dir_of(target) + clean = json.loads((run_dir / verifier["clean_clone"]).read_text()) + events = (run_dir / "events.log").read_text() + + assert clean["ran"] is True and clean["exit_code"] == 0 + assert "clean-clone" in events + assert state_of(target)["status"] == "completed" + + +# -------------------------------------------------------------------------- +# The planner guidance reaches a rendered prompt +# -------------------------------------------------------------------------- + + +def rendered_planner_prompt() -> str: + context = context_assembler.schema_context(REPO_ROOT) + context.update(context_assembler.workflow_context( + loaded_workflow(), harness_config.load_rules(REPO_ROOT))) + return context_assembler.render( + context_assembler.load_template(REPO_ROOT, "planner.md"), context) + + +def test_the_rendered_planner_prompt_tells_a_plan_not_to_tighten_a_restriction(): + """Rendered through context_assembler rather than read off the template, + because what a planner receives is the rendered prompt.""" + rendered = rendered_planner_prompt() + paragraphs = [p for p in re.split(r"\n\s*\n", rendered) if "restate" in p.lower()] + assert len(paragraphs) == 1 + guidance = paragraphs[0].lower() + assert "restriction" in guidance + assert "workflow" in guidance + assert context_assembler.PLACEHOLDER.search(rendered) is None + + +def test_the_planner_template_still_names_no_stage_and_no_restricted_prefix(): + """The guidance is general, so the story-009 property it could have + broken still holds.""" + template = context_assembler.PLACEHOLDER.sub( + "", context_assembler.load_template(REPO_ROOT, "planner.md")) + assert PREFIX not in template + for stage in WORKFLOW["stages"]: + assert not re.search(rf"\b{stage['name']}\b", template), stage["name"] + + +# -------------------------------------------------------------------------- +# What this story left alone +# -------------------------------------------------------------------------- + + +def test_this_story_edited_no_story_artifact(): + """The control is the file the story did edit: if the diff resolution + had stopped seeing anything, the second assertion would fail too.""" + assert story_diff([".harness/stories/"], validation_file=Path(__file__)) == "" + assert story_diff(["orchestration/story_coordinator.py"], + validation_file=Path(__file__)) != "" + + +def test_no_test_in_the_suite_states_the_prose_rule_this_story_supersedes(): + """The superseded rule is the claim that an implementer's record lists + *nothing* under the governed prefix - stricter than may_not_create, which + governs creation alone. + + This is a source scan for that claim being stated in the suite, and it is + narrow in exactly the way test_baseline_honesty.py is narrow: it catches + the phrasings the story artifacts used, not every way the claim could be + written. The control below constructs one and shows the scan reports it. + + This file is excluded from the scan, and only this file: it is where the + control sentence is written, so a scan including it would report the + control as an offender and could never pass. + """ + assert states_the_prose_rule(SUPERSEDED_RULE_SAMPLE) + offenders = {path.name for path in sorted(TESTS_DIR.glob("test_*.py")) + if path.name != Path(__file__).name + and states_the_prose_rule(path.read_text(encoding="utf-8"))} + assert offenders == set() + + +#: A sentence of the shape the story artifacts used, as the negative control +#: for the scan above. +SUPERSEDED_RULE_SAMPLE = ( + 'assert record["modified"] == [] # the implementer\'s changed-files\n' + "# record lists nothing under tests/\n" +) + +_PROSE_RULE = re.compile( + r"(lists nothing under|touches nothing under|leaves .{0,40}untouched|" + r"must not touch|does not touch)[^\n]{0,60}tests/", + re.IGNORECASE, +) + + +def states_the_prose_rule(text: str) -> bool: + return _PROSE_RULE.search(text) is not None diff --git a/workflows/story-workflow.json b/workflows/story-workflow.json index cab0bf0..e29a750 100644 --- a/workflows/story-workflow.json +++ b/workflows/story-workflow.json @@ -7,6 +7,7 @@ "outputs": ["changed-files.json", "implementation-summary.md"], "changed_files": "changed-files.json", "may_not_create": ["tests/"], + "revert_check": "revert-check-result.json", "schemas": { "changed-files.json": "changed-files" }