From 03fc66e27489058385439450c36e98f02d19b3b7 Mon Sep 17 00:00:00 2001 From: "jerod.wilkerson" <30474318+jerodw@users.noreply.github.com> Date: Fri, 7 Aug 2026 16:01:45 -0600 Subject: [PATCH 1/3] story-015: A test that cannot fail must not count as validation Implemented by the l5 harness story workflow. --- .harness/docs/ARCHITECTURE.md | 7 +- prompts/tester.md | 24 + prompts/verifier.md | 9 + tests/conftest.py | 126 +++++ tests/test_baseline_honesty.py | 540 +++++++++++++++++++ tests/test_story_007_validation.py | 21 +- tests/test_story_008_validation.py | 29 +- tests/test_story_009_validation.py | 67 +-- tests/test_story_010_validation.py | 64 +-- tests/test_story_015_validation.py | 816 +++++++++++++++++++++++++++++ 10 files changed, 1581 insertions(+), 122 deletions(-) create mode 100644 tests/test_baseline_honesty.py create mode 100644 tests/test_story_015_validation.py diff --git a/.harness/docs/ARCHITECTURE.md b/.harness/docs/ARCHITECTURE.md index dd2f002..95db14c 100644 --- a/.harness/docs/ARCHITECTURE.md +++ b/.harness/docs/ARCHITECTURE.md @@ -162,7 +162,12 @@ The files at the root always describe the *current* attempt. `attempts/attempt-N - 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. - 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 and story-014 rewrote them to diff the story's own commit against its parent, resolving the commit by walking the marker file's history oldest-first for the first revision carrying the feature the story introduced, with a positive guard asserting the parent really predates the marker so the bound cannot silently resolve to something vacuous. While a story is still uncommitted the marker resolves to `None` and the working tree is the end bound, which is the original comparison. +- 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. +- **`tests/test_baseline_honesty.py` is the mechanical check, and it is deliberately narrow.** An `ast` scan over every module found by globbing `tests/` flags a `subprocess.run`/`check_output` whose first positional argument is a list literal beginning with `"git"`, that targets *this* repository's root (`-C ` or a `cwd=` naming one), and that carries a HEAD-derived revision (`HEAD`, `HEAD:path`, `HEAD~1`, `HEAD^`, or an f-string with that literal prefix) or `status --porcelain`. That is the whole class it covers: an assertion can still be empty on both sides of an honest baseline, tautological, or aimed at the wrong subject, and no AST scan will say so. Say that where the check is defined rather than implying broader coverage — the module docstring does. Two consequences worth knowing before extending it: a git call written with neither `-C` nor `cwd` inherits the process working directory (the repository root, under pytest) and is **not** flagged, and a call assembled through `functools.partial` or a local helper is not seen either — which is by design, since that is how the throwaway-repository tests are written and they must stay unflagged and unrewritten. Exactly one module is exempt, stated as `EXEMPT_MODULES = ("conftest.py",)` rather than inferred: the one holding the shared resolution, the one place where comparing the working tree against HEAD is the right answer. Every per-story validation file is subject to the check, including the four repaired. The glob carries its companion non-empty assertion, per the corpus-test rule above, and the scanned and exempt sets are held to a partition of the glob. +- **The regression set is committed evidence, not a constructed fixture.** The four pre-repair files are recovered with `git show :` where the baseline comes from the *same* shared resolution applied to the check's own module — so no pinned SHA a rebase could invalidate — and story-013's instance is read from `.harness/runs-archive/story-013-vacuous-tests/pre-reset-test_story_013_validation.py`, which is read-only evidence and was not touched. All five are asserted flagged and the four repaired files asserted clean afterwards. The check also carries its own negative controls (the same command flagged against the repository root and ignored against a repository the test built), so "zero flags in the suite" means the idiom is gone rather than that the detector went blind. +- **Repair is not deletion, and an honest baseline that is always empty for a different reason is no improvement.** Each repaired assertion kept its subject, its output options and its strictness; the only authorized change was narrowing the two tests named `test_no_committed_story_artifact_was_edited` to `--diff-filter=MD`, and that one is load-bearing rather than cosmetic — each story's own artifact was *added* in its own run commit, so without it both assertions repair straight into red, and an addition was never an edit. Every repaired subject is then shown failing by violating it against a synthetic history in which the story is committed and its run commit edits the path it claims to have left alone. The narrowed pair is shown still catching a modified *and* a deleted artifact, so narrowed is demonstrably not weakened. +- **Prompt guidance was added for the wider class but is not the enforcement.** `prompts/tester.md` now requires an absence assertion to carry a negative control demonstrating it can fail, distinguishing that class from positive assertions which fail loudly on their own; `prompts/verifier.md` makes an absence assertion offered as evidence without such a demonstration a finding. Both are prose in the role layer — there is no contract file to inject, and this is role guidance rather than schema content, per the injection bullet above. Presence is confirmed by rendering the templates through `orchestration/context_assembler.py`, not by reading them. Treat the prose as the general reminder and the mechanical check as the thing that actually holds: a written rule in an injected document was already tried here and failed five times. - The verifier stands in the one environment where the story's own commit does not exist. `_complete` runs `git add -A` and commits *after* the documenter and after every check the workflow performs, so the state the code ships in is created after the last thing that could object to it, and until story-014 nothing ran the suite there. Three failures across two stories lived in that gap and every one reported green: story-011's differential test resolved its baseline as `git show HEAD:…` (419 passed uncommitted, 394 passed and 25 errors committed), and story-013 shipped a test asserting `git status --porcelain -- tests/` lists the tester's new files — true mid-run, false once `_complete` commits them — on a run that reported 460/460 and passed verification first time. This is not verifier leniency and a stricter verifier would not close it: the suite genuinely passes where the verifier is standing. The bug was the absent check, not a missed one. - The clean-clone check is unconditional for every story that reaches a passing verifier. It is deliberately not gated on a heuristic about what the story touched — "the story edited tests that read git" is exactly the kind of predicate that fails on the next unforeseen environment difference. It does not replace CI, which remains the final word; it moves discovery earlier, so a story is not reported complete and committed before the failure is known. - 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. diff --git a/prompts/tester.md b/prompts/tester.md index 7e85aa3..cf4a6b6 100644 --- a/prompts/tester.md +++ b/prompts/tester.md @@ -18,6 +18,30 @@ Do not: New tests belong in tests/ and become permanent repository assets. +An assertion that claims an absence needs a negative control. A positive +assertion — that something exists, or behaves a particular way — fails +loudly on its own the moment the behavior is missing, so writing it is +enough. An absence assertion is different: that a path was not changed, +that a name does not appear, that a list is empty, that no violation was +found. It passes when the property holds and it passes just as happily when +the test is looking in the wrong place, when the subject has been resolved +to something that cannot differ, or when the check itself has stopped +seeing anything. Green tells you nothing about which of those happened. + +So for every absence you assert, also demonstrate that it can fail: +construct the violation the assertion is meant to catch — against a +throwaway repository, a modified copy of the input, or a stripped +rendering — and assert that the same check reports it. Write the control +beside the assertion it protects, and say in the test what it is +controlling for. An absence assertion with no demonstration of failure is +not validation; it is a claim about what you happened to observe. + +Baselines resolved out of git are the recurring instance of this. Do not +resolve one as `HEAD` or as the working tree against the repository root: +the coordinator commits the working tree at the end of a successful run, so +those comparisons go vacuously green the moment the story commits. Use the +shared resolution in `tests/conftest.py`. + When you finish, write these files to the run directory at {{run_dir}}: test-results.json, the structured outcome of the validation you ran. It diff --git a/prompts/verifier.md b/prompts/verifier.md index ffca161..707abd2 100644 --- a/prompts/verifier.md +++ b/prompts/verifier.md @@ -27,6 +27,15 @@ Do not: Uncertainty is not failure. If evidence is missing, say what is missing rather than inventing a failure. +A passing test is evidence only if it could have failed. An assertion that +claims an absence — that a path was not changed, that a name does not +appear, that a list is empty, that no violation was found — passes equally +when the property holds and when the check has stopped looking at anything. +An absence assertion presented as evidence without a demonstration that it +can fail is a finding: say which assertion, and what a violation of it +would have to look like for the test to notice. A positive assertion needs +no such control, because it fails on its own when the behavior is missing. + When you finish, write these files to the run directory at {{run_dir}}: verification-result.json, your verdict and the evidence behind it. The diff --git a/tests/conftest.py b/tests/conftest.py index 7cf52a4..be7e9ba 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,6 @@ import subprocess import sys +from dataclasses import dataclass from pathlib import Path import pytest @@ -7,6 +8,131 @@ HARNESS_ROOT = Path(__file__).resolve().parents[1] sys.path.insert(0, str(HARNESS_ROOT / "orchestration")) + +# -------------------------------------------------------------------------- +# The one honest baseline resolution the per-story validation files share. +# +# A per-story assertion that its story left some path alone must not be +# written as `git diff HEAD -- `. That asks whether the working tree is +# dirty there, which is a question about whoever is working *now*: the +# coordinator commits the working tree in `_complete`, so on the finished +# branch the answer is "clean" for every path in the repository and the +# assertion holds no matter what the story did. +# +# Resolve the story's own run commit instead — the commit that added that +# story's validation file — and bound the comparison at both ends: that +# commit against its parent. The answer survives a commit, a rebase and a +# squash merge, because it is a search through the file's own history rather +# than a pinned SHA or a fixed distance back from HEAD. While the story is +# still in flight the file has no adding commit, and the working tree +# against HEAD *is* the correct pre-story baseline. +# +# The repository parameter exists so this same code path can be exercised +# against a synthetic history in which the story is already committed — the +# condition this repository cannot be in while these tests decide whether it +# commits. +# -------------------------------------------------------------------------- + + +class NothingToCompareAgainst(RuntimeError): + """The history does not reach the commit that added the validation file. + + Raised rather than degrading to a baseline that would make the caller's + assertion vacuous: a comparison with nothing to compare against is worse + than no comparison, because it reports green. + """ + + +@dataclass(frozen=True) +class StoryRange: + """The commit range one story's own change occupies. + + `endpoint` is None while the story is still uncommitted, in which case + the end of the range is the working tree. + """ + + baseline: str + endpoint: str | None + + @property + def committed(self) -> bool: + return self.endpoint is not None + + def diff_command(self, repo: Path, *options: str) -> list[str]: + revisions = [self.baseline] if self.endpoint is None else [self.baseline, + self.endpoint] + return ["git", "-C", str(repo), "diff", *revisions, *options] + + +def _git(repo: Path, *args: str) -> subprocess.CompletedProcess: + return subprocess.run( + ["git", "-C", str(repo), *args], capture_output=True, text=True, + ) + + +def story_commit_range(validation_file: Path, + repo: Path = HARNESS_ROOT) -> StoryRange: + """The commit range of the story that added `validation_file`. + + The run commit is the *oldest* commit that added the file, so a later + revert-and-restore cannot be mistaken for the story's own run, and a + planning or hotfix commit on the same story — which modifies the file + rather than adding it — is never returned. + """ + relative = _relative_to(validation_file, repo) + log = _git(repo, "log", "--diff-filter=A", "--format=%H", "--", relative) + if log.returncode != 0: + raise NothingToCompareAgainst( + f"git log failed for {relative} in {repo}: {log.stderr.strip()}" + ) + additions = log.stdout.split() + if not additions: + if _committed(repo, relative): + raise NothingToCompareAgainst( + f"{relative} is committed in {repo} but this history does not " + f"reach the commit that added it, so the comparison has " + f"nothing to compare against" + ) + return StoryRange(baseline="HEAD", endpoint=None) + run_commit = additions[-1] + parent = _git(repo, "rev-parse", "--verify", "--quiet", f"{run_commit}^") + if parent.returncode != 0: + raise NothingToCompareAgainst( + f"the commit that added {relative} ({run_commit[:12]}) has no " + f"parent in this history, so the comparison has nothing to " + f"compare against" + ) + return StoryRange(baseline=parent.stdout.strip(), endpoint=run_commit) + + +def story_diff(paths: list[str], *, validation_file: Path, + repo: Path = HARNESS_ROOT, diff_filter: str | None = None, + options: tuple[str, ...] = ()) -> str: + """The diff `validation_file`'s own story made to `paths`. + + Empty output means the story left those paths alone. Callers assert on + emptiness; `options` and `diff_filter` only shape what a non-empty + result looks like and which change kinds it counts. + """ + command = story_commit_range(validation_file, repo).diff_command(repo, *options) + if diff_filter is not None: + command.append(f"--diff-filter={diff_filter}") + command += ["--", *paths] + return subprocess.run( + command, capture_output=True, text=True, check=True, + ).stdout + + +def _relative_to(path: Path, repo: Path) -> str: + resolved = Path(path) + if resolved.is_absolute(): + return resolved.resolve().relative_to(Path(repo).resolve()).as_posix() + return resolved.as_posix() + + +def _committed(repo: Path, relative: str) -> bool: + return _git(repo, "cat-file", "-e", f"HEAD:{relative}").returncode == 0 + STORY = """\ story: id: story-001 diff --git a/tests/test_baseline_honesty.py b/tests/test_baseline_honesty.py new file mode 100644 index 0000000..9d55192 --- /dev/null +++ b/tests/test_baseline_honesty.py @@ -0,0 +1,540 @@ +"""A test that cannot fail must not count as validation. + +Five stories shipped an "X is unchanged" assertion that resolved its +baseline as `git diff HEAD` — the working tree against the last commit. +The coordinator commits the working tree in `_complete`, so on the finished +branch that diff is empty for every path in the repository and the +assertion holds no matter what the story did. story-009's +`test_the_definitions_this_story_injects_are_unchanged` asserted `schemas/` +was unchanged and passed on a branch that added `schemas/manifest.json`. + +Prose guidance was tried here and failed: `.harness/docs/ARCHITECTURE.md` +recorded the rule, `.harness/config.yaml` injects that document into every +stage, and `git diff HEAD` was written four more times anyway. So this +module is a mechanical check rather than a paragraph. + +What it covers, and only this: a `subprocess` git invocation that targets +*this* repository's root and carries a HEAD-derived revision or a +working-tree status query. It is deliberately narrow. It catches the one +idiom above; it does not catch the general class of vacuous assertions, and +nothing here should be read as claiming it does. An assertion can still be +empty on both sides of an honest baseline, tautological, or aimed at the +wrong subject, and no AST scan will say so — that is what the negative +control now required of absence assertions in `prompts/tester.md` is for. + +The regression set is committed evidence rather than a constructed fixture: +the four merged instances are recovered from git history at the revision +preceding this story's own commit, and story-013's is read from the +archived pre-reset copy under `.harness/runs-archive/`. + +The second half of this module demonstrates the repairs. An honest baseline +that is always empty for a different reason would be no improvement, so +each repaired subject is violated against a synthetic history — a +repository in which the story *is* committed, which is the state the +repository under test cannot be in while these tests decide whether it +commits — and the shared resolution must report the violation. +""" +import ast +import subprocess +from dataclasses import dataclass +from pathlib import Path + +import pytest + +from conftest import (NothingToCompareAgainst, story_commit_range, story_diff) + +REPO_ROOT = Path(__file__).resolve().parents[1] +TESTS_DIR = REPO_ROOT / "tests" + +#: The exemption, stated here rather than inferred from anything. Exactly one +#: module is exempt: the one holding the shared baseline resolution, which is +#: the single place in the suite where comparing the working tree against HEAD +#: is the *correct* answer — it is the right baseline while a story is still +#: in flight, and the resolution exists so no other module has to write it. +#: Every per-story validation file is subject to the check, including the four +#: this story repaired. +EXEMPT_MODULES = ("conftest.py",) + +#: Module-level names that stand for the repository under test. A git call +#: pointed at one of these is asking about this repository; a call pointed at +#: a path a test built for itself under tmp_path is not, and is not the check's +#: business. +REPOSITORY_ROOT_NAMES = ("REPO_ROOT", "HARNESS_ROOT") + +#: The four files this story repaired, plus the archived fifth instance. +REPAIRED_FILES = ( + "tests/test_story_007_validation.py", + "tests/test_story_008_validation.py", + "tests/test_story_009_validation.py", + "tests/test_story_010_validation.py", +) +ARCHIVED_INSTANCE = ( + ".harness/runs-archive/story-013-vacuous-tests/" + "pre-reset-test_story_013_validation.py" +) + + +# -------------------------------------------------------------------------- +# The check +# -------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class Flag: + module: str + line: int + reason: str + + def __str__(self) -> str: + return f"{self.module}:{self.line}: {self.reason}" + + +def _literal_text(node: ast.AST) -> str | None: + """The leading literal text of an argument, or None if it has none. + + A plain string yields itself. An f-string yields its literal prefix, so + `f"HEAD~{n}"` is read as a HEAD-derived revision while `f"{revision}:{path}"` + — which resolves a revision the test computed — is not. + """ + if isinstance(node, ast.Constant) and isinstance(node.value, str): + return node.value + if isinstance(node, ast.JoinedStr) and node.values: + first = node.values[0] + if isinstance(first, ast.Constant) and isinstance(first.value, str): + return first.value + return None + + +def _names_the_repository_root(node: ast.AST) -> bool: + return any( + isinstance(inner, ast.Name) and inner.id in REPOSITORY_ROOT_NAMES + for inner in ast.walk(node) + ) + + +def _is_subprocess_call(node: ast.Call) -> bool: + func = node.func + return (isinstance(func, ast.Attribute) + and func.attr in ("run", "check_output") + and isinstance(func.value, ast.Name) + and func.value.id == "subprocess") + + +def _git_argument_list(node: ast.Call) -> list[ast.expr] | None: + """The argument list of a git invocation, or None if this is not one.""" + if not node.args: + return None + first = node.args[0] + if not isinstance(first, ast.List) or not first.elts: + return None + if _literal_text(first.elts[0]) != "git": + return None + return list(first.elts) + + +def _targets_the_repository_root(node: ast.Call, elements: list[ast.expr]) -> bool: + """Through `-C ` in the argument list, or `cwd=`.""" + for index, element in enumerate(elements[:-1]): + if _literal_text(element) == "-C" and _names_the_repository_root( + elements[index + 1]): + return True + return any( + keyword.arg == "cwd" and _names_the_repository_root(keyword.value) + for keyword in node.keywords + ) + + +def _head_derived(text: str) -> bool: + return text == "HEAD" or text.startswith(("HEAD:", "HEAD~", "HEAD^")) + + +def _dishonest_baseline(elements: list[ast.expr]) -> str | None: + """Why this git invocation resolves a baseline that cannot fail.""" + literals = [_literal_text(element) for element in elements] + for text in literals: + if text is not None and _head_derived(text): + return (f"resolves a baseline as {text!r} against the repository " + f"root; the story's own commit becomes HEAD when the " + f"coordinator commits the working tree") + if "status" in literals and "--porcelain" in literals: + return ("queries the working tree with `status --porcelain` against " + "the repository root; the answer is empty once the " + "coordinator commits") + return None + + +def flagged_calls(source: str, module: str) -> list[Flag]: + """Every dishonest git baseline in one module's source. + + Exemptions are not applied here: this is the scan, and a caller that + means to exempt a module does not scan it. That keeps the exemption a + stated policy at one call site rather than a condition buried in the + detector, and it is what lets the regression set below be fed to the + same function the live suite is held to. + """ + flags = [] + for node in ast.walk(ast.parse(source)): + if not isinstance(node, ast.Call) or not _is_subprocess_call(node): + continue + elements = _git_argument_list(node) + if elements is None or not _targets_the_repository_root(node, elements): + continue + reason = _dishonest_baseline(elements) + if reason is not None: + flags.append(Flag(module=module, line=node.lineno, reason=reason)) + return flags + + +def scanned_modules() -> list[Path]: + """Discovered by globbing, never by naming, so a new module is covered + the moment it lands.""" + return [path for path in sorted(TESTS_DIR.glob("*.py")) + if path.name not in EXEMPT_MODULES] + + +# -------------------------------------------------------------------------- +# The live suite +# -------------------------------------------------------------------------- + + +def test_the_scan_discovers_modules_and_finds_some(): + """The companion assertion the glob needs: a check over zero files + passes for the wrong reason.""" + modules = scanned_modules() + assert len(modules) >= 15 + assert all(path.name.endswith(".py") for path in modules) + assert {path.name for path in modules} >= { + Path(rel).name for rel in REPAIRED_FILES} + + +def test_exactly_one_module_is_exempt_and_it_holds_the_shared_resolution(): + assert EXEMPT_MODULES == ("conftest.py",) + resolution = (TESTS_DIR / "conftest.py").read_text(encoding="utf-8") + assert "def story_commit_range" in resolution + assert "def story_diff" in resolution + + +def test_no_module_in_the_suite_resolves_a_dishonest_baseline(): + flags = [ + flag + for path in scanned_modules() + for flag in flagged_calls(path.read_text(encoding="utf-8"), path.name) + ] + assert flags == [], "\n".join(str(flag) for flag in flags) + + +@pytest.mark.parametrize("name", [ + "test_story_005_validation.py", + "test_story_006_single_reader.py", + "test_story_007_validation.py", + "test_story_coordinator.py", + "test_story_011_validation.py", +]) +def test_a_module_that_builds_its_own_repository_is_unflagged(name): + """Throwaway repositories under tmp_path are not this check's business, + and story-011's HEAD reference is a positive guard passed to a local + helper rather than a literal in a git argument list.""" + path = TESTS_DIR / name + assert flagged_calls(path.read_text(encoding="utf-8"), name) == [] + + +def test_a_throwaway_repository_call_is_unflagged_even_written_with_head(): + """The distinction stated as a control rather than as an absence: the + same command is flagged against the repository root and ignored against + a repository the test built.""" + against_a_temp_repo = ( + "import subprocess\n" + "def probe(root):\n" + " subprocess.run(['git', '-C', str(root), 'diff', 'HEAD'])\n" + " subprocess.run(['git', 'status', '--porcelain'], cwd=root)\n" + ) + assert flagged_calls(against_a_temp_repo, "probe.py") == [] + + against_this_repo = against_a_temp_repo.replace("root)", "REPO_ROOT)") + assert len(flagged_calls(against_this_repo, "probe.py")) == 2 + + +def test_the_exemption_is_by_name_and_covers_nothing_else(): + """The exempt module is excluded from the scan; an identical call in any + other module is not.""" + source = ( + "import subprocess\n" + "subprocess.run(['git', '-C', str(REPO_ROOT), 'diff', 'HEAD'])\n" + ) + assert len(flagged_calls(source, "conftest.py")) == 1 # the scan itself is blind + scanned = {path.name for path in scanned_modules()} + assert scanned.isdisjoint(EXEMPT_MODULES) + assert len(scanned) + len(EXEMPT_MODULES) == len(list(TESTS_DIR.glob("*.py"))) + + +# -------------------------------------------------------------------------- +# The regression set: five instances, all committed evidence +# -------------------------------------------------------------------------- + + +def _blob(revision: str, rel: str) -> str: + return subprocess.run( + ["git", "-C", str(REPO_ROOT), "show", f"{revision}:{rel}"], + capture_output=True, text=True, check=True, + ).stdout + + +def pre_repair_source(rel: str) -> str: + """One repaired file as it stood before this story touched it. + + Resolved through the same shared baseline the repairs use: the parent of + the commit that added *this* module. While this story is in flight that + is HEAD, and once it commits it is the revision before it — the pre-repair + text either way, without a pinned SHA that a rebase would invalidate. + """ + return _blob(story_commit_range(Path(__file__)).baseline, rel) + + +@pytest.mark.parametrize("rel", REPAIRED_FILES) +def test_the_check_flags_the_pre_repair_version_of_each_merged_instance(rel): + flags = flagged_calls(pre_repair_source(rel), Path(rel).name) + assert flags, f"{rel} was expected to carry the idiom before its repair" + assert all("HEAD" in flag.reason for flag in flags), flags + + +def test_the_check_flags_story_013s_archived_instance(): + """Read from the archive, which is read-only evidence: story-013's run + was reset and only its story artifact is on main, awaiting a re-run. Its + instance is not repaired here — the check catches it when story-013 runs + again.""" + path = REPO_ROOT / ARCHIVED_INSTANCE + assert path.is_file() + flags = flagged_calls(path.read_text(encoding="utf-8"), path.name) + assert len(flags) >= 4 + reasons = " ".join(flag.reason for flag in flags) + assert "HEAD" in reasons + assert "status --porcelain" in reasons + + +def test_all_five_known_instances_are_caught(): + """The regression set stated as one assertion, so a repair that also + quietly narrowed the check would show up here.""" + sources = {rel: pre_repair_source(rel) for rel in REPAIRED_FILES} + sources[ARCHIVED_INSTANCE] = ( + REPO_ROOT / ARCHIVED_INSTANCE).read_text(encoding="utf-8") + caught = {rel for rel, source in sources.items() + if flagged_calls(source, Path(rel).name)} + assert caught == set(sources) + + +def test_the_repaired_files_no_longer_carry_what_they_carried_before(): + """The other half of the same evidence: flagged before, clean after.""" + for rel in REPAIRED_FILES: + after = (REPO_ROOT / rel).read_text(encoding="utf-8") + assert flagged_calls(after, Path(rel).name) == [], rel + + +# -------------------------------------------------------------------------- +# The repairs, shown failing when their subject is violated +# -------------------------------------------------------------------------- + + +def git(root: Path, *args: str) -> str: + return subprocess.run( + ["git", "-C", str(root), *args], + capture_output=True, text=True, check=True, + ).stdout + + +def commit(root: Path, message: str) -> None: + git(root, "add", "-A") + git(root, "-c", "user.email=t@t", "-c", "user.name=t", "commit", "-q", + "-m", message) + + +def write(root: Path, rel: str, text: str) -> Path: + path = root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + return path + + +def synthetic_story(tmp_path: Path, validation_rel: str, guarded: list[str], *, + violate: str | None = None, + also_add: str | None = None) -> tuple[Path, Path]: + """A repository in which a story is already committed. + + Two commits: a pre-story state carrying the guarded paths, then the + story's own run commit, which adds the validation file and — when + `violate` says so — modifies, deletes, or adds a guarded path in the + same commit. This is the state the repository under test cannot be in + while these tests decide whether it commits, which is exactly why the + resolution takes a repository parameter. + """ + root = tmp_path / "synthetic" + root.mkdir() + git(root, "init", "-q") + for rel in guarded: + write(root, rel, "the pre-story content\n") + commit(root, "pre-story") + + write(root, validation_rel, "def test_it():\n assert True\n") + if violate == "modify": + write(root, guarded[0], "a later edit\n") + elif violate == "delete": + (root / guarded[0]).unlink() + elif violate == "add": + write(root, also_add or f"{guarded[0]}.new", "an addition\n") + commit(root, "the story's own run commit") + return root, root / validation_rel + + +#: Every subject the four repaired files assert their story left alone. +REPAIRED_SUBJECTS = [ + ("tests/test_story_007_validation.py", ".harness/stories/story-007.yaml"), + ("tests/test_story_008_validation.py", "scripts/l5-assist"), + ("tests/test_story_008_validation.py", "schemas/story.schema.json"), + ("tests/test_story_009_validation.py", "workflows/story-workflow.json"), + ("tests/test_story_009_validation.py", "rules/execution-rules.json"), + ("tests/test_story_009_validation.py", "schemas/story.schema.json"), + ("tests/test_story_010_validation.py", "orchestration/context_assembler.py"), + ("tests/test_story_010_validation.py", "prompts/tester.md"), +] + + +@pytest.mark.parametrize("validation_rel,guarded", REPAIRED_SUBJECTS) +def test_a_repaired_assertion_passes_when_its_subject_is_respected( + tmp_path, validation_rel, guarded, +): + root, validation_file = synthetic_story(tmp_path, validation_rel, [guarded]) + assert story_diff([guarded], validation_file=validation_file, + repo=root).strip() == "" + + +@pytest.mark.parametrize("validation_rel,guarded", REPAIRED_SUBJECTS) +def test_a_repaired_assertion_fails_when_its_subject_is_violated( + tmp_path, validation_rel, guarded, +): + """The guarantee is not that the assertion passes but that it can fail. + The story's own run commit edits the path it claims to have left alone, + and the comparison must say so.""" + root, validation_file = synthetic_story(tmp_path, validation_rel, [guarded], + violate="modify") + assert story_diff([guarded], validation_file=validation_file, + repo=root).strip() != "" + + +def test_the_same_violation_goes_green_under_the_baseline_this_story_removed( + tmp_path, +): + """Why the repairs were worth doing, shown rather than argued: over the + same history, `git diff HEAD` is empty and the honest range is not.""" + root, validation_file = synthetic_story( + tmp_path, "tests/test_story_009_validation.py", ["schemas/manifest.json"], + violate="modify") + assert git(root, "diff", "HEAD", "--", "schemas/").strip() == "" + assert story_diff(["schemas/"], validation_file=validation_file, + repo=root).strip() != "" + + +@pytest.mark.parametrize("violation", ["modify", "delete"]) +def test_the_narrowed_assertions_still_catch_an_edited_story_artifact( + tmp_path, violation, +): + """The two `test_no_committed_story_artifact_was_edited` assertions are + narrowed to modifications and deletions. Narrowed is not weakened: an + execution record rewritten or removed in the story's own commit is still + caught.""" + root, validation_file = synthetic_story( + tmp_path, "tests/test_story_007_validation.py", + [".harness/stories/story-001.yaml"], violate=violation) + assert story_diff([".harness/stories/"], validation_file=validation_file, + repo=root, diff_filter="MD", + options=("--name-only",)).strip() != "" + + +def test_the_narrowing_is_exactly_the_storys_own_new_artifact(): + """What the narrowing lets through and nothing more: on this repository, + story-007's own commit added `.harness/stories/story-007.yaml` and edited + no other record.""" + validation_file = REPO_ROOT / "tests" / "test_story_007_validation.py" + added = story_diff([".harness/stories/"], validation_file=validation_file, + diff_filter="A", options=("--name-only",)).split() + assert added == [".harness/stories/story-007.yaml"] + assert story_diff([".harness/stories/"], validation_file=validation_file, + diff_filter="MD", options=("--name-only",)).strip() == "" + + +# -------------------------------------------------------------------------- +# The resolution's edges +# -------------------------------------------------------------------------- + + +def test_the_resolution_returns_the_run_commit_and_its_parent(tmp_path): + root, validation_file = synthetic_story( + tmp_path, "tests/test_story_009_validation.py", ["schemas/story.schema.json"]) + resolved = story_commit_range(validation_file, root) + assert resolved.committed + assert resolved.endpoint == git(root, "rev-parse", "HEAD").strip() + assert resolved.baseline == git(root, "rev-parse", "HEAD^").strip() + + +def test_the_run_commit_is_not_an_earlier_commit_on_the_same_story(tmp_path): + """A planning or hotfix commit touching the file *modifies* it; only the + story's own run commit *adds* it, and only additions are considered.""" + root, validation_file = synthetic_story( + tmp_path, "tests/test_story_009_validation.py", ["schemas/story.schema.json"]) + run_commit = git(root, "rev-parse", "HEAD").strip() + validation_file.write_text("def test_it():\n assert 1\n", encoding="utf-8") + commit(root, "a follow-up hotfix on the same story") + assert story_commit_range(validation_file, root).endpoint == run_commit + + +def test_an_uncommitted_validation_file_falls_back_to_the_working_tree(tmp_path): + """While a story is in flight, the working tree against HEAD *is* the + correct pre-story baseline.""" + root = tmp_path / "in-flight" + root.mkdir() + git(root, "init", "-q") + write(root, "schemas/story.schema.json", "{}\n") + commit(root, "pre-story") + validation_file = write(root, "tests/test_story_099_validation.py", "pass\n") + + resolved = story_commit_range(validation_file, root) + assert not resolved.committed + assert resolved.baseline == "HEAD" + assert story_diff(["schemas/"], validation_file=validation_file, + repo=root).strip() == "" + + write(root, "schemas/story.schema.json", "{\"edited\": true}\n") + assert story_diff(["schemas/"], validation_file=validation_file, + repo=root).strip() != "" + + +def test_the_resolution_raises_when_the_history_does_not_reach_far_enough(tmp_path): + """A shallow clone has the validation file in HEAD but not the commit + that added it. Degrading to the working tree there would hand back a + baseline that makes every caller vacuous, so it raises instead.""" + root, _ = synthetic_story( + tmp_path, "tests/test_story_009_validation.py", ["schemas/story.schema.json"]) + shallow = tmp_path / "shallow" + subprocess.run( + ["git", "clone", "--depth", "1", "-q", root.as_uri(), str(shallow)], + capture_output=True, text=True, check=True, + ) + validation_file = shallow / "tests" / "test_story_009_validation.py" + assert validation_file.is_file() + + with pytest.raises(NothingToCompareAgainst) as raised: + story_commit_range(validation_file, shallow) + assert "nothing to compare against" in str(raised.value) + + +def test_the_resolution_raises_when_the_run_commit_has_no_parent(tmp_path): + """The other way history can fall short: the adding commit is the root + commit, so there is no pre-story state to compare against.""" + root = tmp_path / "root-commit" + root.mkdir() + git(root, "init", "-q") + write(root, "schemas/story.schema.json", "{}\n") + validation_file = write(root, "tests/test_story_099_validation.py", "pass\n") + commit(root, "everything at once") + + with pytest.raises(NothingToCompareAgainst) as raised: + story_commit_range(validation_file, root) + assert "nothing to compare against" in str(raised.value) diff --git a/tests/test_story_007_validation.py b/tests/test_story_007_validation.py index 62b2584..ef4cd79 100644 --- a/tests/test_story_007_validation.py +++ b/tests/test_story_007_validation.py @@ -17,6 +17,8 @@ import pytest +from conftest import story_diff + import context_assembler import harness_config import schema_validator @@ -709,13 +711,20 @@ def test_every_pre_era_story_still_parses(): def test_no_committed_story_artifact_was_edited(): - """Execution records are never rewritten to satisfy a later contract.""" - result = subprocess.run( - ["git", "-C", str(REPO_ROOT), "diff", "HEAD", "--name-only", "--", - ".harness/stories/"], - capture_output=True, text=True, check=True, + """Execution records are never rewritten to satisfy a later contract. + + Scoped to modifications and deletions, which is what "edited" has always + meant here: this story's own commit *added* `.harness/stories/story-007.yaml`, + and an addition was never an edit. The baseline is this story's own run + commit against its parent, resolved by `conftest.story_commit_range` — + not `git diff HEAD`, which asks whether the working tree is dirty and + goes vacuously green the moment the story commits. + """ + edited = story_diff( + [".harness/stories/"], validation_file=Path(__file__), + diff_filter="MD", options=("--name-only",), ) - assert result.stdout.strip() == "" + assert edited.strip() == "" def test_this_storys_own_artifact_parses_and_validates_under_the_new_schema(): diff --git a/tests/test_story_008_validation.py b/tests/test_story_008_validation.py index bee6e8c..abc2647 100644 --- a/tests/test_story_008_validation.py +++ b/tests/test_story_008_validation.py @@ -26,6 +26,8 @@ import pytest +from conftest import story_diff + import context_assembler REPO_ROOT = Path(__file__).resolve().parents[1] @@ -339,24 +341,33 @@ def test_build_context_still_resolves_every_stage_schema_placeholder( # -------------------------------------------------------------------------- -def _unchanged_against_head(rel: str) -> bool: - result = subprocess.run( - ["git", "diff", "HEAD", "--stat", "--", rel], - cwd=REPO_ROOT, capture_output=True, text=True, check=True, - ) - return result.stdout.strip() == "" +def _unchanged_by_this_story(rel: str, *, diff_filter: str | None = None) -> bool: + """Whether *this story's own change* left `rel` alone. + + Not `git diff HEAD`, which was what this helper asked before story-015. + That asks whether the working tree is dirty here — a question about + whoever is working right now, answered "clean" for every path the moment + the coordinator commits the story. The baseline resolution lives in + `tests/conftest.py`: this story's own run commit against its parent. + """ + return story_diff( + [rel], validation_file=Path(__file__), diff_filter=diff_filter, + options=("--stat",), + ).strip() == "" def test_l5_assist_is_unchanged(): - assert _unchanged_against_head("scripts/l5-assist") + assert _unchanged_by_this_story("scripts/l5-assist") def test_the_story_schema_is_unchanged(): - assert _unchanged_against_head("schemas/story.schema.json") + assert _unchanged_by_this_story("schemas/story.schema.json") def test_no_committed_story_artifact_was_edited(): - assert _unchanged_against_head(".harness/stories") + """Modifications and deletions only: this story's own run commit added + `.harness/stories/story-008.yaml`, and an addition was never an edit.""" + assert _unchanged_by_this_story(".harness/stories", diff_filter="MD") def test_every_committed_story_artifact_still_parses(): diff --git a/tests/test_story_009_validation.py b/tests/test_story_009_validation.py index ebfd8f5..67b9f10 100644 --- a/tests/test_story_009_validation.py +++ b/tests/test_story_009_validation.py @@ -24,6 +24,8 @@ import pytest +from conftest import story_diff + import context_assembler import harness_config @@ -393,66 +395,25 @@ def test_l5_run_still_finds_the_target_root_from_a_subdirectory(target_root): # -------------------------------------------------------------------------- -#: Where this story's change landed, and the feature it introduced there. -#: Used to find this story's own commit, not to read the working tree. -STORY_MARKER_PATH = "orchestration/context_assembler.py" -STORY_MARKER = "def workflow_context" - - -def _revision_introducing(path: str, marker: str) -> str | None: - """The oldest committed revision of `path` whose blob carries `marker`. - - None while the change is still uncommitted. Walks the file's own history - rather than pinning a SHA, so a rebase or a squash merge does not move the - answer. - """ - revisions = subprocess.run( - ["git", "log", "--format=%H", "--", path], - cwd=REPO_ROOT, capture_output=True, text=True, check=True, - ).stdout.split() - for revision in reversed(revisions): - blob = subprocess.run( - ["git", "show", f"{revision}:{path}"], - cwd=REPO_ROOT, capture_output=True, text=True, - ) - if blob.returncode == 0 and marker in blob.stdout: - return revision - return None - - def _unchanged_by_this_story(rel: str) -> bool: """Whether *this story's own change* left `rel` alone. Not `git diff HEAD`. That asks whether the working tree is dirty here, which is a question about whoever is working right now: it goes vacuously green the moment anything is committed, and red for every later story that - legitimately edits one of these paths — the same over-broad scope defect - commit 3b05b99 fixed in story-011's prompt-scope assertion. Bound the - comparison at both ends instead: find this story's own commit and diff it - against its parent. While the story is still uncommitted there is no such - commit and the working tree is the end bound, which is the original - comparison. + legitimately edits one of these paths. Bound the comparison at both ends + instead — this story's own run commit against its parent. + + Since story-015 the resolution lives once in `tests/conftest.py` rather + than being restated here, and it keys on this validation file's own + adding commit rather than on a marker planted in the story's source: the + commit that added `tests/test_story_009_validation.py` *is* story-009's + run commit, and no marker has to be chosen and kept true for that to + hold. """ - revision = _revision_introducing(STORY_MARKER_PATH, STORY_MARKER) - if revision is None: - result = subprocess.run( - ["git", "diff", "HEAD", "--stat", "--", rel], - cwd=REPO_ROOT, capture_output=True, text=True, check=True, - ) - return result.stdout.strip() == "" - # The positive guard the bound needs: the parent really predates this - # story's feature, so the diff below is this story's own and the assertion - # is comparing something. - before = subprocess.run( - ["git", "show", f"{revision}^:{STORY_MARKER_PATH}"], - cwd=REPO_ROOT, capture_output=True, text=True, check=True, - ).stdout - assert STORY_MARKER not in before, "the resolved bound is not this story's commit" - result = subprocess.run( - ["git", "diff", f"{revision}^", revision, "--stat", "--", rel], - cwd=REPO_ROOT, capture_output=True, text=True, check=True, - ) - return result.stdout.strip() == "" + return story_diff( + [rel], validation_file=Path(__file__), options=("--stat",), + ).strip() == "" @pytest.mark.parametrize("rel", ["workflows/", "rules/", "schemas/"]) diff --git a/tests/test_story_010_validation.py b/tests/test_story_010_validation.py index 0149e6f..2f712eb 100644 --- a/tests/test_story_010_validation.py +++ b/tests/test_story_010_validation.py @@ -14,6 +14,8 @@ import pytest +from conftest import story_diff + import context_assembler import story_coordinator from agent_runner import AgentResult @@ -423,66 +425,22 @@ def test_archive_attempt_creates_nothing_it_was_not_asked_for(tmp_path: Path): # -------------------------------------------------------------------------- -#: Where this story's change landed, and the feature it introduced there. -#: Used to find this story's own commit, not to read the working tree. -STORY_MARKER_PATH = "orchestration/story_coordinator.py" -STORY_MARKER = "def archive_attempt" - - -def _revision_introducing(path: str, marker: str) -> str | None: - """The oldest committed revision of `path` whose blob carries `marker`. - - None while the change is still uncommitted. Walks the file's own history - rather than pinning a SHA, so a rebase or a squash merge does not move the - answer. - """ - revisions = subprocess.run( - ["git", "log", "--format=%H", "--", path], - cwd=REPO_ROOT, capture_output=True, text=True, check=True, - ).stdout.split() - for revision in reversed(revisions): - blob = subprocess.run( - ["git", "show", f"{revision}:{path}"], - cwd=REPO_ROOT, capture_output=True, text=True, - ) - if blob.returncode == 0 and marker in blob.stdout: - return revision - return None - - def _unchanged_by_this_story(rel: str) -> bool: """Whether *this story's own change* left `rel` alone. Not `git diff HEAD`. That asks whether the working tree is dirty here, which is a question about whoever is working right now: it goes vacuously green the moment anything is committed, and red for every later story that - legitimately edits one of these paths — the same over-broad scope defect - commit 3b05b99 fixed in story-011's prompt-scope assertion. Bound the - comparison at both ends instead: find this story's own commit and diff it - against its parent. While the story is still uncommitted there is no such - commit and the working tree is the end bound, which is the original - comparison. + legitimately edits one of these paths. Bound the comparison at both ends + instead — this story's own run commit against its parent. + + Since story-015 the resolution lives once in `tests/conftest.py` rather + than being restated here, and it keys on this validation file's own + adding commit rather than on a marker planted in the story's source: the + commit that added `tests/test_story_010_validation.py` *is* story-010's + run commit. """ - revision = _revision_introducing(STORY_MARKER_PATH, STORY_MARKER) - if revision is None: - result = subprocess.run( - ["git", "diff", "HEAD", "--", rel], - cwd=REPO_ROOT, capture_output=True, text=True, check=True, - ) - return result.stdout.strip() == "" - # The positive guard the bound needs: the parent really predates this - # story's feature, so the diff below is this story's own and the assertion - # is comparing something. - before = subprocess.run( - ["git", "show", f"{revision}^:{STORY_MARKER_PATH}"], - cwd=REPO_ROOT, capture_output=True, text=True, check=True, - ).stdout - assert STORY_MARKER not in before, "the resolved bound is not this story's commit" - result = subprocess.run( - ["git", "diff", f"{revision}^", revision, "--", rel], - cwd=REPO_ROOT, capture_output=True, text=True, check=True, - ) - return result.stdout.strip() == "" + return story_diff([rel], validation_file=Path(__file__)).strip() == "" def test_context_assembler_is_unchanged(): diff --git a/tests/test_story_015_validation.py b/tests/test_story_015_validation.py new file mode 100644 index 0000000..79957b9 --- /dev/null +++ b/tests/test_story_015_validation.py @@ -0,0 +1,816 @@ +"""story-015: a test that cannot fail must not count as validation. + +The story ships three things and this file validates each of them +independently of how they were implemented: + +1. One shared baseline resolution in `tests/conftest.py` — a story's own run + commit resolved as the commit that *added* that story's validation file, + and the baseline as that commit's parent. Exercised here against synthetic + histories in which the story is already committed, which is the state this + repository cannot be in while these tests decide whether it commits. +2. Four repaired assertions. The guarantee under test is not that they pass — + they passed before, vacuously — but that they *can fail*. Each repaired + helper is called for real, with its repository redirected at a synthetic + history whose run commit violates the very path the assertion names. +3. `tests/test_baseline_honesty.py`, the mechanical check. Its regression set + is recovered here independently, from git history and from the archive, + and fed back through its own scanner. + +Every absence asserted below carries a control that constructs the violation +and shows the same check reporting it. The five instances this story exists +because of were absences that could not fail. +""" +import ast +import inspect +import json +import subprocess +import sys +from pathlib import Path + +import pytest + +import conftest +import test_baseline_honesty as check +import test_story_007_validation as story007 +import test_story_008_validation as story008 +import test_story_009_validation as story009 +import test_story_010_validation as story010 +from conftest import (NothingToCompareAgainst, story_commit_range, story_diff) + +import context_assembler +import harness_config +import schema_validator +import story_parser + +REPO_ROOT = Path(__file__).resolve().parents[1] +TESTS_DIR = REPO_ROOT / "tests" + +REPAIRED = { + "tests/test_story_007_validation.py": story007, + "tests/test_story_008_validation.py": story008, + "tests/test_story_009_validation.py": story009, + "tests/test_story_010_validation.py": story010, +} + +ARCHIVED_INSTANCE = (REPO_ROOT / ".harness" / "runs-archive" + / "story-013-vacuous-tests" + / "pre-reset-test_story_013_validation.py") + +#: Every subject a repaired assertion names, with the module that asserts it. +#: Read off the repaired files rather than assumed; `test_every_repaired_subject_ +#: is_covered_here` holds this list to what the modules actually assert. +SUBJECTS = [ + ("tests/test_story_007_validation.py", ".harness/stories/"), + ("tests/test_story_008_validation.py", "scripts/l5-assist"), + ("tests/test_story_008_validation.py", "schemas/story.schema.json"), + ("tests/test_story_008_validation.py", ".harness/stories"), + ("tests/test_story_009_validation.py", "workflows/"), + ("tests/test_story_009_validation.py", "rules/"), + ("tests/test_story_009_validation.py", "schemas/"), + ("tests/test_story_010_validation.py", "orchestration/context_assembler.py"), + ("tests/test_story_010_validation.py", "workflows/"), + ("tests/test_story_010_validation.py", "schemas/"), + ("tests/test_story_010_validation.py", "rules/"), + ("tests/test_story_010_validation.py", "prompts/"), +] + + +# -------------------------------------------------------------------------- +# Synthetic histories +# -------------------------------------------------------------------------- + + +def git(root: Path, *args: str) -> str: + return subprocess.run( + ["git", "-C", str(root), *args], + capture_output=True, text=True, check=True, + ).stdout + + +def write(root: Path, rel: str, text: str) -> Path: + path = root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + return path + + +def commit(root: Path, message: str) -> str: + git(root, "add", "-A") + git(root, "-c", "user.email=t@t", "-c", "user.name=t", + "commit", "-q", "--allow-empty", "-m", message) + return git(root, "rev-parse", "HEAD").strip() + + +def _under(guarded: str) -> tuple[str, str]: + """A file the guarded pathspec covers, and a second one it would also + cover if the story added it.""" + if guarded.endswith("/") or not Path(guarded).suffix: + base = guarded.rstrip("/") + return f"{base}/kept.txt", f"{base}/brand-new.txt" + return guarded, f"{guarded}.new" + + +def committed_story(tmp_path: Path, validation_rel: str, guarded: str, *, + violate: str | None = None, name: str = "synthetic") -> Path: + """A repository in which one story has already run and committed. + + Commit 1 is the pre-story state and carries the guarded path. Commit 2 is + the story's own run commit: it adds the validation file and, when + `violate` says so, touches the guarded path in the same commit. That is + the shape of a finished branch, and the shape under which `git diff HEAD` + reports nothing no matter what the story did. + """ + root = tmp_path / name + root.mkdir() + git(root, "init", "-q") + subject, sibling = _under(guarded) + write(root, subject, "the pre-story content\n") + write(root, "unrelated.txt", "something the story may touch\n") + commit(root, "pre-story") + + write(root, validation_rel, "def test_it():\n assert True\n") + write(root, "unrelated.txt", "the story's own legitimate change\n") + if violate == "modify": + write(root, subject, "rewritten inside the story's own run commit\n") + elif violate == "delete": + (root / subject).unlink() + elif violate == "add": + write(root, sibling, "an addition\n") + commit(root, "the story's own run commit") + return root + + +def redirect(monkeypatch, module, root: Path, validation_rel: str) -> None: + """Point one repaired module's assertions at a synthetic repository. + + The module's own helper runs unmodified — only the repository it asks + about moves. That is what makes the failure below a property of the + repaired code rather than of a reimplementation of it. + """ + real = conftest.story_diff + + def patched(paths, *, validation_file=None, repo=None, **kwargs): + return real(paths, validation_file=root / validation_rel, repo=root, + **kwargs) + + monkeypatch.setattr(module, "story_diff", patched) + + +# -------------------------------------------------------------------------- +# The shared resolution +# -------------------------------------------------------------------------- + + +def test_the_resolution_lives_in_one_place_and_takes_a_repository(): + for function in (story_commit_range, story_diff): + parameters = inspect.signature(function).parameters + assert "repo" in parameters, function.__name__ + assert parameters["repo"].default == conftest.HARNESS_ROOT + assert "validation_file" in inspect.signature(story_diff).parameters + + +def test_no_repaired_file_carries_its_own_copy_of_the_resolution(): + """The absence: no repaired module resolves a revision itself any more. + + The control is the pre-repair text of the same four files, which does — + so a scan that had stopped seeing git calls would fail here rather than + report four clean modules. + """ + def resolving_calls(source: str) -> list[str]: + found = [] + for node in ast.walk(ast.parse(source)): + if not isinstance(node, ast.Call): + continue + if not (isinstance(node.func, ast.Attribute) + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "subprocess"): + continue + literals = [element.value for element in ast.walk(node) + if isinstance(element, ast.Constant) + and isinstance(element.value, str)] + if "git" in literals and {"diff", "log", "show"} & set(literals): + found.append(f"line {node.lineno}") + return found + + for rel in REPAIRED: + current = (REPO_ROOT / rel).read_text(encoding="utf-8") + assert resolving_calls(current) == [], rel + assert "from conftest import" in current, rel + assert resolving_calls(pre_repair_source(rel)), ( + f"{rel} was expected to resolve its own baseline before the repair") + + +def test_a_committed_story_resolves_to_its_run_commit_and_that_commits_parent( + tmp_path, +): + root = committed_story(tmp_path, "tests/test_story_042_validation.py", + "schemas/") + resolved = story_commit_range(root / "tests/test_story_042_validation.py", + root) + assert resolved.committed + assert resolved.endpoint == git(root, "rev-parse", "HEAD").strip() + assert resolved.baseline == git(root, "rev-parse", "HEAD^").strip() + # The baseline really predates the story: the validation file is not in it. + listing = git(root, "ls-tree", "-r", "--name-only", resolved.baseline).split() + assert "tests/test_story_042_validation.py" not in listing + assert "tests/test_story_042_validation.py" in git( + root, "ls-tree", "-r", "--name-only", resolved.endpoint).split() + + +def test_the_run_commit_is_not_a_later_planning_or_hotfix_commit(tmp_path): + root = committed_story(tmp_path, "tests/test_story_042_validation.py", + "schemas/") + run_commit = git(root, "rev-parse", "HEAD").strip() + write(root, "tests/test_story_042_validation.py", "def test_it():\n pass\n") + commit(root, "a hotfix on the same story") + write(root, "schemas/kept.txt", "a later story legitimately edits this\n") + later = commit(root, "a later story") + + resolved = story_commit_range(root / "tests/test_story_042_validation.py", + root) + assert resolved.endpoint == run_commit + assert resolved.endpoint != later + # And the later story's edit is not attributed to this one. + assert story_diff( + ["schemas/"], + validation_file=root / "tests/test_story_042_validation.py", + repo=root).strip() == "" + + +def test_a_re_added_validation_file_still_resolves_to_the_oldest_addition( + tmp_path, +): + root = committed_story(tmp_path, "tests/test_story_042_validation.py", + "schemas/") + run_commit = git(root, "rev-parse", "HEAD").strip() + (root / "tests/test_story_042_validation.py").unlink() + commit(root, "removed by accident") + write(root, "tests/test_story_042_validation.py", "def test_it():\n pass\n") + commit(root, "restored") + assert story_commit_range( + root / "tests/test_story_042_validation.py", root).endpoint == run_commit + + +def test_an_uncommitted_validation_file_compares_the_working_tree_to_head( + tmp_path, +): + """The in-flight case, with the control beside it: clean while nothing has + been touched, non-empty the moment the working tree touches the subject.""" + root = tmp_path / "in-flight" + root.mkdir() + git(root, "init", "-q") + write(root, "schemas/story.schema.json", "{}\n") + commit(root, "pre-story") + validation_file = write(root, "tests/test_story_042_validation.py", "pass\n") + + resolved = story_commit_range(validation_file, root) + assert not resolved.committed + assert resolved.baseline == "HEAD" + assert resolved.endpoint is None + assert story_diff(["schemas/"], validation_file=validation_file, + repo=root).strip() == "" + + write(root, "schemas/story.schema.json", '{"edited": true}\n') + assert story_diff(["schemas/"], validation_file=validation_file, + repo=root).strip() != "" + + +def test_the_resolution_raises_rather_than_degrading_on_a_truncated_history( + tmp_path, +): + """A shallow clone carries the validation file in HEAD but not the commit + that added it. Falling back to the working tree there would hand every + caller a baseline that cannot fail, so it must raise. + + The control is the same clone unshallowed, where the resolution succeeds — + so the raise is a property of the truncation and not of the clone. + """ + origin = committed_story(tmp_path, "tests/test_story_042_validation.py", + "schemas/") + shallow = tmp_path / "shallow" + subprocess.run(["git", "clone", "--depth", "1", "-q", origin.as_uri(), + str(shallow)], capture_output=True, text=True, check=True) + validation_file = shallow / "tests/test_story_042_validation.py" + assert validation_file.is_file() + + with pytest.raises(NothingToCompareAgainst) as raised: + story_commit_range(validation_file, shallow) + assert "nothing to compare against" in str(raised.value) + + git(shallow, "fetch", "-q", "--unshallow") + assert story_commit_range(validation_file, shallow).committed + + +def test_the_resolution_raises_when_the_adding_commit_is_the_root_commit( + tmp_path, +): + root = tmp_path / "root-commit" + root.mkdir() + git(root, "init", "-q") + write(root, "schemas/story.schema.json", "{}\n") + validation_file = write(root, "tests/test_story_042_validation.py", "pass\n") + commit(root, "everything at once") + + with pytest.raises(NothingToCompareAgainst) as raised: + story_commit_range(validation_file, root) + assert "nothing to compare against" in str(raised.value) + + +def test_the_raise_is_not_swallowed_by_the_diff_helper(tmp_path): + """`story_diff` must propagate the raise rather than returning empty + output, which a caller would read as 'unchanged'.""" + origin = committed_story(tmp_path, "tests/test_story_042_validation.py", + "schemas/") + shallow = tmp_path / "shallow" + subprocess.run(["git", "clone", "--depth", "1", "-q", origin.as_uri(), + str(shallow)], capture_output=True, text=True, check=True) + with pytest.raises(NothingToCompareAgainst): + story_diff(["schemas/"], + validation_file=shallow / "tests/test_story_042_validation.py", + repo=shallow) + + +# -------------------------------------------------------------------------- +# The four repairs, shown failing +# -------------------------------------------------------------------------- + + +def test_every_repaired_subject_is_covered_here(): + """The companion assertion the table above needs: the subjects exercised + below are the subjects the repaired modules actually assert, read out of + their source rather than trusted.""" + for rel, module in REPAIRED.items(): + source = (REPO_ROOT / rel).read_text(encoding="utf-8") + literals = {node.value for node in ast.walk(ast.parse(source)) + if isinstance(node, ast.Constant) + and isinstance(node.value, str)} + covered = {subject for module_rel, subject in SUBJECTS + if module_rel == rel} + assert covered, rel + assert covered <= literals, (rel, covered - literals) + + +@pytest.mark.parametrize("rel,subject", SUBJECTS) +def test_a_repaired_assertion_passes_when_its_story_respects_its_subject( + monkeypatch, tmp_path, rel, subject, +): + root = committed_story(tmp_path, rel, subject) + redirect(monkeypatch, REPAIRED[rel], root, rel) + assert _assert_unchanged(REPAIRED[rel], subject) is None + + +@pytest.mark.parametrize("rel,subject", SUBJECTS) +def test_a_repaired_assertion_fails_when_its_story_violates_its_subject( + monkeypatch, tmp_path, rel, subject, +): + """The guarantee: not that the assertion passes, but that it can fail. + + The synthetic story's own run commit rewrites the path the assertion + names, and the repaired code — the module's own helper, unmodified — must + say so. + """ + root = committed_story(tmp_path, rel, subject, violate="modify") + redirect(monkeypatch, REPAIRED[rel], root, rel) + with pytest.raises(AssertionError): + _assert_unchanged(REPAIRED[rel], subject) + + +def _assert_unchanged(module, subject) -> None: + """Run the module's own assertion for `subject`, raising AssertionError + when it reports a change.""" + if module is story007: + module.test_no_committed_story_artifact_was_edited() + return None + if module is story008 and subject == ".harness/stories": + assert module._unchanged_by_this_story(subject, diff_filter="MD") + return None + assert module._unchanged_by_this_story(subject) + return None + + +@pytest.mark.parametrize("rel,subject", SUBJECTS) +def test_the_same_violation_is_invisible_to_the_baseline_this_story_removed( + tmp_path, rel, subject, +): + """Why every one of these repairs was needed, demonstrated per subject: + over the identical history, `git diff HEAD` is empty and the honest range + is not.""" + root = committed_story(tmp_path, rel, subject, violate="modify") + assert git(root, "diff", "HEAD", "--", subject).strip() == "" + assert story_diff([subject], validation_file=root / rel, + repo=root).strip() != "" + + +@pytest.mark.parametrize("violation", ["modify", "delete"]) +def test_the_narrowed_assertions_still_catch_a_rewritten_execution_record( + monkeypatch, tmp_path, violation, +): + """Narrowed to modifications and deletions is not weakened: a committed + story artifact rewritten or removed inside the story's own run commit is + still caught, by both narrowed assertions.""" + for rel, module, subject in ( + ("tests/test_story_007_validation.py", story007, ".harness/stories/"), + ("tests/test_story_008_validation.py", story008, ".harness/stories"), + ): + root = committed_story(tmp_path, rel, subject, violate=violation, + name=f"{Path(rel).stem}-{violation}") + redirect(monkeypatch, module, root, rel) + with pytest.raises(AssertionError): + _assert_unchanged(module, subject) + + +def test_the_narrowing_permits_exactly_the_storys_own_new_artifact( + monkeypatch, tmp_path, +): + """And what it lets through: an *addition* inside the story's own run + commit, which was never an edit. The control is the test above — the same + assertion still fires on a modification or a deletion.""" + for rel, module, subject in ( + ("tests/test_story_007_validation.py", story007, ".harness/stories/"), + ("tests/test_story_008_validation.py", story008, ".harness/stories"), + ): + root = committed_story(tmp_path, rel, subject, violate="add", + name=f"{Path(rel).stem}-added") + added = story_diff([subject], validation_file=root / rel, repo=root, + diff_filter="A", options=("--name-only",)) + assert added.strip(), "the synthetic story was supposed to add a record" + redirect(monkeypatch, module, root, rel) + assert _assert_unchanged(module, subject) is None + + +def test_no_repaired_assertion_changed_its_subject_or_its_strictness(): + """The diff of each repaired file against its pre-repair version, read + mechanically: the same tests over the same subjects, with the single + authorized narrowing and nothing else.""" + narrowed = { + "tests/test_story_007_validation.py", + "tests/test_story_008_validation.py", + } + for rel in REPAIRED: + before = pre_repair_source(rel) + after = (REPO_ROOT / rel).read_text(encoding="utf-8") + assert _test_names(before) == _test_names(after), rel + assert _guarded_paths(before) <= _guarded_paths(after), rel + + filters = _diff_filters(after) + assert filters == ({"MD"} if rel in narrowed else set()), (rel, filters) + + +def _diff_filters(source: str) -> set[str]: + """Every value passed as `diff_filter=`, which is the only place this + story is authorized to change an assertion's strictness.""" + return {node.value.value + for call in ast.walk(ast.parse(source)) + if isinstance(call, ast.Call) + for node in call.keywords + if node.arg == "diff_filter" and isinstance(node.value, ast.Constant) + and isinstance(node.value.value, str)} + + +def _test_names(source: str) -> set[str]: + return {node.name for node in ast.walk(ast.parse(source)) + if isinstance(node, ast.FunctionDef) and node.name.startswith("test_")} + + +def _guarded_paths(source: str) -> set[str]: + """Every path a *test* in this module names, in its body or in its + parametrize decorator. + + Scoped to test functions on purpose: the pre-repair sources carried + module-level constants belonging to the resolution machinery the repair + removed (story-009's `STORY_MARKER_PATH`, for one), and those were never + subjects of an assertion. What a test names is what it guards. + """ + paths = set() + for node in ast.walk(ast.parse(source)): + if not (isinstance(node, ast.FunctionDef) + and node.name.startswith("test_")): + continue + for inner in [*ast.walk(node)]: + if (isinstance(inner, ast.Constant) and isinstance(inner.value, str) + and "/" in inner.value + and len(inner.value.splitlines()) == 1 + and not inner.value.startswith(("http", " "))): + paths.add(inner.value) + return paths + + +def test_the_repaired_assertions_pass_on_this_repository(): + """Run for real, unpatched, against the repository under test.""" + story007.test_no_committed_story_artifact_was_edited() + assert story008._unchanged_by_this_story("scripts/l5-assist") + assert story008._unchanged_by_this_story("schemas/story.schema.json") + assert story008._unchanged_by_this_story(".harness/stories", diff_filter="MD") + for rel in ("workflows/", "rules/", "schemas/"): + assert story009._unchanged_by_this_story(rel) + for rel in ("orchestration/context_assembler.py", "workflows/", "schemas/", + "rules/", "prompts/"): + assert story010._unchanged_by_this_story(rel) + + +# -------------------------------------------------------------------------- +# The mechanical check +# -------------------------------------------------------------------------- + + +def pre_repair_source(rel: str) -> str: + """A repaired file as it stood before this story touched it. + + Resolved through the shared resolution applied to *this* file: HEAD while + story-015 is in flight, the pre-story revision once it commits. No pinned + SHA, so a rebase or a squash does not move the answer. + """ + baseline = story_commit_range(Path(__file__)).baseline + return subprocess.run( + ["git", "-C", str(REPO_ROOT), "show", f"{baseline}:{rel}"], + capture_output=True, text=True, check=True, + ).stdout + + +def test_the_recovered_pre_repair_sources_are_the_pre_repair_sources(): + """The regression set is only evidence if it is what it claims to be.""" + for rel in REPAIRED: + before = pre_repair_source(rel) + assert before != (REPO_ROOT / rel).read_text(encoding="utf-8"), rel + assert "def test_" in before, rel + + +@pytest.mark.parametrize("rel", sorted(REPAIRED)) +def test_the_check_flags_each_merged_instance_recovered_from_history(rel): + flags = check.flagged_calls(pre_repair_source(rel), Path(rel).name) + assert flags, rel + + +def test_the_check_flags_story_013s_archived_instance(): + assert ARCHIVED_INSTANCE.is_file() + flags = check.flagged_calls( + ARCHIVED_INSTANCE.read_text(encoding="utf-8"), ARCHIVED_INSTANCE.name) + assert flags + reasons = " ".join(flag.reason for flag in flags) + assert "HEAD" in reasons and "porcelain" in reasons + + +def test_all_five_known_instances_are_caught_and_the_repairs_are_clean(): + caught = {rel for rel in REPAIRED + if check.flagged_calls(pre_repair_source(rel), rel)} + assert caught == set(REPAIRED) + assert check.flagged_calls( + ARCHIVED_INSTANCE.read_text(encoding="utf-8"), ARCHIVED_INSTANCE.name) + for rel in REPAIRED: + assert check.flagged_calls( + (REPO_ROOT / rel).read_text(encoding="utf-8"), rel) == [], rel + + +def test_the_live_suite_carries_no_dishonest_baseline(): + """The absence. Its control is the parametrized regression set above: the + same scanner, on the same modules before their repair, flags all five.""" + flags = [flag + for path in check.scanned_modules() + for flag in check.flagged_calls(path.read_text(encoding="utf-8"), + path.name)] + assert flags == [], "\n".join(str(flag) for flag in flags) + + +@pytest.mark.parametrize("revision", ["HEAD", "HEAD~1", "HEAD^", "HEAD:prompts/x"]) +def test_every_head_derived_revision_against_the_repository_root_is_flagged( + revision, +): + for form in ( + f"subprocess.run(['git', '-C', str(REPO_ROOT), 'diff', '{revision}'])", + f"subprocess.run(['git', 'diff', '{revision}'], cwd=REPO_ROOT)", + f"subprocess.check_output(['git', '-C', str(HARNESS_ROOT), 'show', " + f"'{revision}'])", + ): + assert check.flagged_calls(f"import subprocess\n{form}\n", "probe.py"), form + + +def test_a_working_tree_status_query_against_the_repository_root_is_flagged(): + source = ("import subprocess\n" + "subprocess.run(['git', 'status', '--porcelain'], cwd=REPO_ROOT)\n") + assert check.flagged_calls(source, "probe.py") + + +def test_an_interpolated_head_revision_is_flagged_and_a_resolved_one_is_not(): + """`f\"HEAD~{n}\"` is the idiom spelled with an offset; `f\"{revision}:{path}\"` + is a revision the test resolved, which is the honest form.""" + dishonest = ("import subprocess\n" + "subprocess.run(['git', '-C', str(REPO_ROOT), 'diff', " + "f'HEAD~{n}'])\n") + honest = ("import subprocess\n" + "subprocess.run(['git', '-C', str(REPO_ROOT), 'show', " + "f'{revision}:{path}'])\n") + assert check.flagged_calls(dishonest, "probe.py") + assert check.flagged_calls(honest, "probe.py") == [] + + +def test_a_throwaway_repository_is_not_the_checks_business(): + """The distinction the check is built on, stated as a control: the same + two commands, flagged against the repository root and ignored against a + repository the test built for itself.""" + def probe(target: str) -> str: + return ("import subprocess\n" + "def probe(tmp_path):\n" + " root = tmp_path / 'repo'\n" + f" subprocess.run(['git', '-C', str({target}), 'diff', 'HEAD'])\n" + f" subprocess.run(['git', 'status', '--porcelain'], " + f"cwd={target})\n") + + assert check.flagged_calls(probe("root"), "probe.py") == [] + assert len(check.flagged_calls(probe("REPO_ROOT"), "probe.py")) == 2 + + +@pytest.mark.parametrize("name", [ + "conftest.py", + "test_story_005_validation.py", + "test_story_006_single_reader.py", + "test_story_007_validation.py", + "test_story_coordinator.py", + "test_story_011_validation.py", +]) +def test_the_throwaway_repository_tests_are_unflagged_by_the_scanner(name): + """These modules build their own repository under tmp_path, so their git + calls are not this check's business. `conftest.py` is scanned here + directly even though it is exempt: its own HEAD usage goes through a + local helper rather than a literal argument list, so the exemption is a + stated policy rather than the thing keeping the suite green. + + The control for this absence is `test_a_throwaway_repository_is_not_the_ + checks_business`, which shows the identical commands flagged once their + target is the repository root. + """ + path = TESTS_DIR / name + assert check.flagged_calls(path.read_text(encoding="utf-8"), name) == [] + + +def test_story_011s_validation_file_is_unflagged_and_unchanged_by_this_story(): + rel = "tests/test_story_011_validation.py" + assert check.flagged_calls((REPO_ROOT / rel).read_text(encoding="utf-8"), + rel) == [] + assert story_diff([rel], validation_file=Path(__file__)).strip() == "" + + +def test_exactly_one_module_is_exempt_and_the_exemption_is_stated(): + assert check.EXEMPT_MODULES == ("conftest.py",) + source = (TESTS_DIR / "test_baseline_honesty.py").read_text(encoding="utf-8") + assert "EXEMPT_MODULES = " in source + conftest_source = (TESTS_DIR / "conftest.py").read_text(encoding="utf-8") + assert "def story_commit_range" in conftest_source + scanned = {path.name for path in check.scanned_modules()} + assert scanned.isdisjoint(check.EXEMPT_MODULES) + assert scanned | set(check.EXEMPT_MODULES) == { + path.name for path in TESTS_DIR.glob("*.py")} + + +def test_no_per_story_validation_file_is_exempt(): + """The check must not ship with exemptions for the cases that motivated + it.""" + scanned = {path.name for path in check.scanned_modules()} + validation_files = {path.name for path in TESTS_DIR.glob("test_story_*.py")} + assert validation_files + assert validation_files <= scanned + + +def test_the_scanned_set_is_discovered_by_globbing_and_cannot_be_empty(): + modules = check.scanned_modules() + assert modules + assert {path.name for path in modules} == { + path.name for path in TESTS_DIR.glob("*.py") + if path.name not in check.EXEMPT_MODULES} + assert Path(__file__).name in {path.name for path in modules} + + +def test_the_checks_module_states_the_narrow_class_it_covers(): + source = (TESTS_DIR / "test_baseline_honesty.py").read_text(encoding="utf-8") + docstring = ast.get_docstring(ast.parse(source)) or "" + assert "narrow" in docstring.lower() + assert "does not" in docstring.lower() + + +def test_a_well_written_absence_assertion_survives_the_check(): + """The mechanism distinguishes the two rather than flagging every absence + assertion: an absence resolved against a repository the test built, with + its negative control beside it, is clean — while the same file with one + root-targeted HEAD call is not.""" + honest = ( + "import subprocess\n" + "def test_the_story_left_schemas_alone(tmp_path):\n" + " root = build_repo(tmp_path)\n" + " diff = subprocess.run(['git', '-C', str(root), 'diff',\n" + " base, tip, '--', 'schemas/'])\n" + " assert diff.stdout == ''\n" + "def test_the_same_check_reports_a_violation(tmp_path):\n" + " root = build_repo(tmp_path, violate=True)\n" + " diff = subprocess.run(['git', '-C', str(root), 'diff',\n" + " base, tip, '--', 'schemas/'])\n" + " assert diff.stdout != ''\n" + ) + assert check.flagged_calls(honest, "probe.py") == [] + assert check.flagged_calls(honest + ( + "subprocess.run(['git', '-C', str(REPO_ROOT), 'diff', 'HEAD'])\n"), + "probe.py") + + +# -------------------------------------------------------------------------- +# The guidance, in the rendered prompts +# -------------------------------------------------------------------------- + + +TESTER_GUIDANCE = [ + "An assertion that claims an absence needs a negative control", + "demonstrate that it can fail", + "tests/conftest.py", +] +VERIFIER_GUIDANCE = [ + "absence", + "is a finding", +] + + +def rendered(prompt_file: str, target_root: Path, harness_root: Path) -> str: + story_text = (target_root / ".harness" / "stories" + / "story-001.yaml").read_text(encoding="utf-8") + run_dir = target_root / ".harness" / "runs" / "story-001" + run_dir.mkdir(parents=True, exist_ok=True) + context = context_assembler.build_context( + story_text=story_text, + story=story_parser.parse(story_text, + schema_validator.load_schema("story")), + run_dir=run_dir, + target_root=target_root, + harness_root=harness_root, + config=harness_config.load_config(target_root), + rules=harness_config.load_rules(harness_root), + retry_count=0, + ) + return context_assembler.render( + context_assembler.load_template(harness_root, prompt_file), context) + + +def test_the_negative_control_guidance_reaches_the_rendered_tester_prompt( + target_root, harness_root, +): + prompt = rendered("tester.md", target_root, harness_root) + for phrase in TESTER_GUIDANCE: + assert phrase in prompt, phrase + assert "positive" in prompt.lower() + assert "{{" not in prompt + + +def test_the_corresponding_requirement_reaches_the_rendered_verifier_prompt( + target_root, harness_root, +): + prompt = rendered("verifier.md", target_root, harness_root) + for phrase in VERIFIER_GUIDANCE: + assert phrase in prompt, phrase + assert "{{" not in prompt + + +def test_the_guidance_is_read_from_the_render_and_not_from_the_template( + target_root, harness_root, monkeypatch, +): + """The control for the two assertions above: they pass because the + renderer produced the text, so a template that stopped being loaded shows + up as a failure rather than as a silent pass.""" + monkeypatch.setattr(context_assembler, "load_template", + lambda root, name: "a template with no guidance\n") + prompt = rendered("tester.md", target_root, harness_root) + assert TESTER_GUIDANCE[0] not in prompt + + +# -------------------------------------------------------------------------- +# Scope +# -------------------------------------------------------------------------- + + +@pytest.mark.parametrize("rel", ["orchestration/", "workflows/", "schemas/", + "scripts/", ".harness/runs-archive/", + "prompts/implementer.md", "prompts/planner.md", + "prompts/documenter.md", + "prompts/harness-layer.md", "prompts/assist.md"]) +def test_this_story_changed_nothing_outside_its_scope(rel): + assert story_diff([rel], validation_file=Path(__file__)).strip() == "" + + +def test_the_scope_assertion_above_can_fail(tmp_path): + """Its control: over a synthetic history where the story does touch + `orchestration/`, the identical call reports it.""" + rel = "tests/test_story_015_validation.py" + root = committed_story(tmp_path, rel, "orchestration/", violate="modify") + assert story_diff(["orchestration/"], validation_file=root / rel, + repo=root).strip() != "" + + +def test_the_archived_story_013_copy_was_not_restored_or_edited(): + assert not (TESTS_DIR / "test_story_013_validation.py").exists() + assert ARCHIVED_INSTANCE.is_file() + assert story_diff([str(ARCHIVED_INSTANCE.relative_to(REPO_ROOT))], + validation_file=Path(__file__)).strip() == "" + + +def test_the_suite_still_has_the_tests_the_repaired_files_shipped_with(): + """No test was weakened, skipped or deleted to make the repairs pass.""" + for rel in REPAIRED: + after = (REPO_ROOT / rel).read_text(encoding="utf-8") + assert _test_names(pre_repair_source(rel)) == _test_names(after), rel + assert "@pytest.mark.skip" not in after, rel + assert "pytest.skip(" not in after, rel From e87976234fedf3e068e0f480e981eaa4431aec58 Mon Sep 17 00:00:00 2001 From: "jerod.wilkerson" <30474318+jerodw@users.noreply.github.com> Date: Fri, 7 Aug 2026 16:31:04 -0600 Subject: [PATCH 2/3] Close the baseline check's implicit-target hole, two ways The check flagged a dishonest git baseline only when the call stated it ran against the repository root, through -C or cwd=. A call stating no target was skipped entirely - but subprocess with no cwd inherits the parent's, and pytest runs this suite from the repository root. So deleting one keyword from `git diff HEAD` made it invisible to the check while changing nothing about what it did. The target is now three-valued: stated and naming this repository, stated and naming something else, or not stated at all. The third case is not an inference about the author's intent - it is what subprocess does. The scan still evaluates nothing and resolves no paths; it asks only whether a target was stated and, if so, whether it is written as one of the two names standing for this repository. A second and stricter rule sits beside it: no git call in tests/ may leave its target unstated, whatever the call asks for. That removes the ambiguity rather than answering it per subcommand, so it cannot return through a different one. No module is exempt - the baseline exemption exists because comparing the working tree against HEAD is correct in one place, and there is nowhere leaving the target unsaid is correct. Cost measured before the change: three git calls in the suite stated no target, all `git clone `, none carrying a baseline. Each now names its target. Verified by injecting the evasion into a real module - both rules go red, and green again when it is removed. 688 passed on Python 3.14 and 3.10. Co-Authored-By: Claude Opus 5 --- tests/test_baseline_honesty.py | 127 ++++++++++++++++++++++++++--- tests/test_story_015_validation.py | 6 +- 2 files changed, 121 insertions(+), 12 deletions(-) diff --git a/tests/test_baseline_honesty.py b/tests/test_baseline_honesty.py index 9d55192..edfb054 100644 --- a/tests/test_baseline_honesty.py +++ b/tests/test_baseline_honesty.py @@ -132,16 +132,42 @@ def _git_argument_list(node: ast.Call) -> list[ast.expr] | None: return list(first.elts) -def _targets_the_repository_root(node: ast.Call, elements: list[ast.expr]) -> bool: - """Through `-C ` in the argument list, or `cwd=`.""" +def _declared_target(node: ast.Call, elements: list[ast.expr]) -> ast.expr | None: + """The expression naming where this git call runs, or None if it names none. + + `-C ` in the argument list, or the `cwd=` keyword. Which of the two + is used does not matter; that a target was stated at all does. + """ for index, element in enumerate(elements[:-1]): - if _literal_text(element) == "-C" and _names_the_repository_root( - elements[index + 1]): - return True - return any( - keyword.arg == "cwd" and _names_the_repository_root(keyword.value) - for keyword in node.keywords - ) + if _literal_text(element) == "-C": + return elements[index + 1] + for keyword in node.keywords: + if keyword.arg == "cwd": + return keyword.value + return None + + +def _targets_the_repository_root(node: ast.Call, elements: list[ast.expr]) -> bool: + """Whether this git call runs against the repository under test. + + Three cases, and the third is why this is not simply "does it say + REPO_ROOT". A call that states no target inherits the parent process's + working directory — that is what `subprocess` does, not a guess about + what the author meant — and pytest runs this suite from the repository + root. So saying nothing is not neutral: it names this repository by + default, and the check must read it that way or the dishonest baseline + the whole module exists to catch simply moves one keyword away. + + The scan never evaluates an expression or reasons about what a variable + holds. It asks only whether a target was stated, and if so whether it is + written as one of the two names that stand for this repository. A stated + target that is anything else is somebody's throwaway repository and is + not this check's business. + """ + target = _declared_target(node, elements) + if target is None: + return True + return _names_the_repository_root(target) def _head_derived(text: str) -> bool: @@ -185,6 +211,35 @@ def flagged_calls(source: str, module: str) -> list[Flag]: return flags +def undeclared_targets(source: str, module: str) -> list[Flag]: + """Every git invocation in one module that does not say where it runs. + + A second, independent rule, and a stricter one: it does not care what the + call asks for. An implicit target is the ambiguity that let the baseline + check above be evaded by deleting a keyword, and the same ambiguity would + return through any other subcommand. Requiring the target to be stated + removes the question rather than answering it each time. + + No module is exempt. The baseline exemption exists because comparing the + working tree against HEAD is *correct* in exactly one place; there is + nowhere that leaving the target unsaid is correct. + """ + flags = [] + for node in ast.walk(ast.parse(source)): + if not isinstance(node, ast.Call) or not _is_subprocess_call(node): + continue + elements = _git_argument_list(node) + if elements is None or _declared_target(node, elements) is not None: + continue + flags.append(Flag( + module=module, line=node.lineno, + reason=("runs git without saying where: no `-C` and no `cwd=`, so " + "it inherits the process working directory, which is this " + "repository"), + )) + return flags + + def scanned_modules() -> list[Path]: """Discovered by globbing, never by naming, so a new module is covered the moment it lands.""" @@ -192,6 +247,11 @@ def scanned_modules() -> list[Path]: if path.name not in EXEMPT_MODULES] +def all_modules() -> list[Path]: + """Every module, including the one the baseline check exempts.""" + return sorted(TESTS_DIR.glob("*.py")) + + # -------------------------------------------------------------------------- # The live suite # -------------------------------------------------------------------------- @@ -223,6 +283,53 @@ def test_no_module_in_the_suite_resolves_a_dishonest_baseline(): assert flags == [], "\n".join(str(flag) for flag in flags) +def test_no_module_runs_git_without_saying_where(): + """The stricter companion rule, over every module including the exempt one. + + This is what closes the evasion the baseline check had: a call stating no + target inherits the process working directory, so `git diff HEAD` without + a `cwd=` asked about this repository while reading as though it asked + about nothing. + """ + flags = [ + flag + for path in all_modules() + for flag in undeclared_targets(path.read_text(encoding="utf-8"), path.name) + ] + assert flags == [], "\n".join(str(flag) for flag in flags) + + +def test_an_undeclared_target_is_flagged_whatever_the_call_asks_for(): + """The rule is about the missing target, not about the subcommand. + + Both sources below are dishonest in the same way and neither names a + revision, so the baseline check has nothing to say about them; this one + does. + """ + benign = "import subprocess\nsubprocess.run(['git', 'status'])\n" + assert len(undeclared_targets(benign, "probe.py")) == 1 + assert flagged_calls(benign, "probe.py") == [] + + declared = "import subprocess\nsubprocess.run(['git', 'status'], cwd=tmp)\n" + assert undeclared_targets(declared, "probe.py") == [] + + +def test_an_undeclared_target_carrying_head_is_caught_by_both_rules(): + """The hole this closes, stated as a test. + + Before the target became three-valued, dropping `cwd=REPO_ROOT` from a + `git diff HEAD` call made it invisible to the baseline check while + changing nothing about what it did. + """ + evasion = "import subprocess\nsubprocess.run(['git', 'diff', 'HEAD'])\n" + assert len(flagged_calls(evasion, "probe.py")) == 1 + assert len(undeclared_targets(evasion, "probe.py")) == 1 + + elsewhere = ("import subprocess\n" + "subprocess.run(['git', 'diff', 'HEAD'], cwd=tmp_path)\n") + assert flagged_calls(elsewhere, "probe.py") == [] + + @pytest.mark.parametrize("name", [ "test_story_005_validation.py", "test_story_006_single_reader.py", @@ -515,7 +622,7 @@ def test_the_resolution_raises_when_the_history_does_not_reach_far_enough(tmp_pa shallow = tmp_path / "shallow" subprocess.run( ["git", "clone", "--depth", "1", "-q", root.as_uri(), str(shallow)], - capture_output=True, text=True, check=True, + cwd=tmp_path, capture_output=True, text=True, check=True, ) validation_file = shallow / "tests" / "test_story_009_validation.py" assert validation_file.is_file() diff --git a/tests/test_story_015_validation.py b/tests/test_story_015_validation.py index 79957b9..f1d5232 100644 --- a/tests/test_story_015_validation.py +++ b/tests/test_story_015_validation.py @@ -289,7 +289,8 @@ def test_the_resolution_raises_rather_than_degrading_on_a_truncated_history( "schemas/") shallow = tmp_path / "shallow" subprocess.run(["git", "clone", "--depth", "1", "-q", origin.as_uri(), - str(shallow)], capture_output=True, text=True, check=True) + str(shallow)], cwd=tmp_path, capture_output=True, + text=True, check=True) validation_file = shallow / "tests/test_story_042_validation.py" assert validation_file.is_file() @@ -323,7 +324,8 @@ def test_the_raise_is_not_swallowed_by_the_diff_helper(tmp_path): "schemas/") shallow = tmp_path / "shallow" subprocess.run(["git", "clone", "--depth", "1", "-q", origin.as_uri(), - str(shallow)], capture_output=True, text=True, check=True) + str(shallow)], cwd=tmp_path, capture_output=True, + text=True, check=True) with pytest.raises(NothingToCompareAgainst): story_diff(["schemas/"], validation_file=shallow / "tests/test_story_042_validation.py", From ed6bba0f78c7e834482a6d8bd73ddc2c01bfaf23 Mon Sep 17 00:00:00 2001 From: "jerod.wilkerson" <30474318+jerodw@users.noreply.github.com> Date: Fri, 7 Aug 2026 19:48:06 -0600 Subject: [PATCH 3/3] Recognize a git spawn however subprocess was imported The scan matched only a literal `subprocess.run` or `subprocess.check_output`, so `import subprocess as sp` and `from subprocess import run` both walked past it. An import statement should not be able to hide a call from a check about what that call does. A qualified call now matches on the attribute alone, whatever qualifies it, and Popen/call/check_call join run/check_output. What identifies these calls is the literal "git" heading their argument list, which _git_argument_list already insists on, so matching the tail loses nothing. A bare call matches only when the module imported that name from subprocess, and the asymmetry is deliberate. Matching every bare `run(...)` flagged tests/test_story_016_validation.py's `run = functools.partial(subprocess.run, cwd=root)`, where the target is declared one line above the call. Reading it would mean following an assignment; this check reads what the source states rather than tracking values. Imports are stated, a local rebinding is not, and the latter is left uncovered rather than guessed at. Verified against the evasion that found this: injecting an aliased-import `git diff HEAD` into a real module turns both rules red and green again on removal. 695 passed on Python 3.14 and 3.10. Co-Authored-By: Claude Opus 5 --- tests/test_baseline_honesty.py | 108 ++++++++++++++++++++++++++++++--- 1 file changed, 99 insertions(+), 9 deletions(-) diff --git a/tests/test_baseline_honesty.py b/tests/test_baseline_honesty.py index edfb054..16c3be2 100644 --- a/tests/test_baseline_honesty.py +++ b/tests/test_baseline_honesty.py @@ -112,12 +112,53 @@ def _names_the_repository_root(node: ast.AST) -> bool: ) -def _is_subprocess_call(node: ast.Call) -> bool: +#: The call names that spawn a process. Matched on the name alone, never on +#: what it is qualified by: `subprocess.run`, `sp.run` under an aliased +#: import, and a bare `run` under `from subprocess import run` are the same +#: call, and requiring the module to be spelled a particular way made the +#: check evadable by an import statement. What identifies these calls is the +#: literal "git" at the head of their argument list, which _git_argument_list +#: already insists on, so matching the tail loses nothing. +SPAWNING_CALLS = ("run", "check_output", "Popen", "call", "check_call") + + +def _imported_spawners(tree: ast.Module) -> frozenset[str]: + """Names this module bound directly from `subprocess`. + + Read off the import statements, which is a fact stated in the source, not + a value anything has to resolve. `from subprocess import run` binds `run`; + `from subprocess import run as sh` binds `sh`. + """ + names = set() + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom) and node.module == "subprocess": + names.update(alias.asname or alias.name for alias in node.names + if alias.name in SPAWNING_CALLS) + return frozenset(names) + + +def _is_subprocess_call(node: ast.Call, imported: frozenset[str]) -> bool: + """Whether this call spawns a process, however subprocess was imported. + + Two forms, and the asymmetry between them is deliberate. + + A qualified call matches on the attribute alone, whatever qualifies it: + `subprocess.run` and `sp.run` under an aliased import are the same call, + and requiring the module to be spelled one way made the check evadable by + an import statement. + + A bare call matches only when the module imported that name from + `subprocess`. Matching every bare `run(...)` would flag the legitimate + `run = functools.partial(subprocess.run, cwd=root)` idiom, where the + target *is* declared — one line above the call. Chasing that binding + means following assignments, and this check does not evaluate or track + values; it reads what the source states. Imports are stated. A local + rebinding is not, and is left uncovered rather than guessed at. + """ func = node.func - return (isinstance(func, ast.Attribute) - and func.attr in ("run", "check_output") - and isinstance(func.value, ast.Name) - and func.value.id == "subprocess") + if isinstance(func, ast.Attribute): + return func.attr in SPAWNING_CALLS + return isinstance(func, ast.Name) and func.id in imported def _git_argument_list(node: ast.Call) -> list[ast.expr] | None: @@ -199,8 +240,10 @@ def flagged_calls(source: str, module: str) -> list[Flag]: same function the live suite is held to. """ flags = [] - for node in ast.walk(ast.parse(source)): - if not isinstance(node, ast.Call) or not _is_subprocess_call(node): + tree = ast.parse(source) + imported = _imported_spawners(tree) + for node in ast.walk(tree): + if not isinstance(node, ast.Call) or not _is_subprocess_call(node, imported): continue elements = _git_argument_list(node) if elements is None or not _targets_the_repository_root(node, elements): @@ -225,8 +268,10 @@ def undeclared_targets(source: str, module: str) -> list[Flag]: nowhere that leaving the target unsaid is correct. """ flags = [] - for node in ast.walk(ast.parse(source)): - if not isinstance(node, ast.Call) or not _is_subprocess_call(node): + tree = ast.parse(source) + imported = _imported_spawners(tree) + for node in ast.walk(tree): + if not isinstance(node, ast.Call) or not _is_subprocess_call(node, imported): continue elements = _git_argument_list(node) if elements is None or _declared_target(node, elements) is not None: @@ -314,6 +359,51 @@ def test_an_undeclared_target_is_flagged_whatever_the_call_asks_for(): assert undeclared_targets(declared, "probe.py") == [] +@pytest.mark.parametrize("source", [ + pytest.param("import subprocess\nsubprocess.run(['git', 'diff', 'HEAD'])\n", + id="plain-import"), + pytest.param("import subprocess as sp\nsp.run(['git', 'diff', 'HEAD'])\n", + id="aliased-module"), + pytest.param("from subprocess import run\nrun(['git', 'diff', 'HEAD'])\n", + id="imported-name"), + pytest.param("from subprocess import run as sh\nsh(['git', 'diff', 'HEAD'])\n", + id="imported-name-aliased"), + pytest.param("import subprocess\nsubprocess.check_output(['git', 'diff', 'HEAD'])\n", + id="check_output"), + pytest.param("import subprocess\nsubprocess.Popen(['git', 'diff', 'HEAD'])\n", + id="popen"), +]) +def test_the_spawn_is_recognized_however_subprocess_was_imported(source): + """An import statement must not be able to hide a call from the check. + + Matching only `subprocess.run` meant renaming the import was enough to + disappear; every form below spawns the same process. + """ + assert len(flagged_calls(source, "probe.py")) == 1 + assert len(undeclared_targets(source, "probe.py")) == 1 + + +def test_a_partial_bound_runner_is_not_flagged(): + """The target is declared on the partial, one line above the call. + + Reading it would mean following an assignment, and this check does not + track values. `tests/test_story_016_validation.py` uses this idiom, so the + case is real rather than hypothetical — and treating a bare `run(...)` as + a spawn regardless of imports would flag it wrongly. + """ + source = ("import functools, subprocess\n" + "run = functools.partial(subprocess.run, cwd=root)\n" + "run(['git', 'diff', 'HEAD'])\n") + assert undeclared_targets(source, "probe.py") == [] + assert flagged_calls(source, "probe.py") == [] + + # The same module *also* importing run from subprocess binds the name, and + # then the call is a spawn by the module's own declaration. + declared = ("from subprocess import run\n" + "run(['git', 'diff', 'HEAD'])\n") + assert len(undeclared_targets(declared, "probe.py")) == 1 + + def test_an_undeclared_target_carrying_head_is_caught_by_both_rules(): """The hole this closes, stated as a test.