story-017: Decide an implementer's test edits by reverting them

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

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.
 

schemas/manifest.json

@@ -5,6 +5,7 @@
     "execution-history",
     "retry-guidance",
     "retry-history",
+    "revert-check-result",
     "story",
     "test-results",
     "verification-result"

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."
+    }
+  }
+}

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,