From 9fc6bbf883eceb0f1fd3cac986407f0b6b06b54a Mon Sep 17 00:00:00 2001 From: "jerod.wilkerson" <30474318+jerodw@users.noreply.github.com> Date: Sat, 15 Aug 2026 14:44:49 -0600 Subject: [PATCH] story-042: A plan may assign an existing file to the implementer Implemented by the l5 harness story workflow. --- .harness/docs/ARCHITECTURE.md | 16 +- orchestration/plan_validation.py | 94 +++- prompts/planner.md | 12 +- scripts/l5-plan | 6 +- tests/test_plan_assignment_refusal.py | 617 +++++++++++++++++++++++-- tests/test_plan_time_validation.py | 10 +- tests/test_validation_module_naming.py | 10 +- 7 files changed, 697 insertions(+), 68 deletions(-) diff --git a/.harness/docs/ARCHITECTURE.md b/.harness/docs/ARCHITECTURE.md index 7cecadf..b8112ef 100644 --- a/.harness/docs/ARCHITECTURE.md +++ b/.harness/docs/ARCHITECTURE.md @@ -93,7 +93,7 @@ The drift source that paragraph used to name is closed: `planner.md` no longer s - `agent_runner.py` — invokes `claude -p` headlessly (`--permission-mode acceptEdits --output-format stream-json --verbose`, prompt on stdin), streams raw output to the run's log, and returns the agent's final result text. Since story-035 it also passes `--settings` on every stage invocation, registering the shipped deny-only Bash guard: `hooks_dir(harness_root=None)` resolves `hooks/` relative to this module the way `schema_validator` resolves schemas, and `guard_settings` reads `hooks/settings.json` and substitutes the guard's absolute path for the declaration's `{guard_path}`. An absent or unreadable declaration, or a missing guard file, returns `None` and the stage runs without the hook — the guard is the net and the allowlist is the gate, so failing to register it must not stop a run. The settings are resolved *here* rather than passed in, so `run_agent`'s signature is unchanged and the fake runners the suite injects need not know the hook exists. See "Tool allowlist" above for what the guard decides. - `harness_config.py` — loads `.harness/config.yaml` (a deliberately small YAML subset parsed directly, keeping the harness dependency-free), workflow definitions, and execution rules. Also owns `find_target_root(start) -> Path`: the walk-up from a starting directory to the nearest ancestor containing `.harness/config.yaml`, exiting 1 with `No .harness/config.yaml found here or above. Run l5-init first.` when none exists. That loop appears exactly once in the repository — `l5-run`, `l5-plan`, and `l5-status` all call it (story-009 extracted it from `l5-run`, which `l5-status` had copied byte-for-byte). `l5-init`'s config check is a different thing: a non-walking existence probe on a directory it was explicitly given. - `plan_commit.py` — the decision behind `l5-plan`'s post-session commit, kept out of the script so it is testable without spawning `claude`. Every function returns what happened rather than printing it. `snapshot(stories_dir)` is every file under the configured stories directory right now (a directory that does not exist yet snapshots as empty, so a repository's first story appears like any other); `new_artifacts(stories_dir, before)` is what appeared since. **Appearance is the whole test** — nothing here reads the session's exit status, and a session that only edited an existing artifact yields nothing. `commit_artifacts` runs `git add -- ` then `git commit -m -- `: the pathspec on the *commit* as well as the add is what keeps unrelated dirty work — and whatever the developer had already staged — out of it. There is no `git add -A` anywhere in this module. `commit_subject` is `Plan story-NNN: ` for one readable artifact, `Plan story-NNN` on any parse failure, and `Plan story-NNN, story-MMM` when one session added more than one, committed together because splitting them would invent an order the session did not have. The title is read through `story_coordinator.read_story` — the run's one reading of a story artifact, per that rule below — not through a second `story_parser.parse` call, so the commit message describes the artifact the way the run that executes it will. `current_branch`, `resolve_remote` and `push_commit` resolve the remote from `branch.<name>.remote`, fall back to `origin` when the branch tracks nothing, and report rather than attempt when neither exists; the push is `git push <remote> HEAD`, so a branch with no upstream is pushed under its own name without writing tracking configuration into the developer's repository as a side effect of planning. Nothing in the module rolls back, amends or resets: a failed push leaves the commit exactly where it is. It chooses, creates and switches no branch — the commit lands on whatever branch the developer was on, as the planner's own commit did. -- `plan_validation.py` — the decision behind `l5-plan`'s plan-time validation, added by story-025 and kept out of the script for the same reason `plan_commit` is. It **returns** the problems it found; it never prints them, never repairs an artifact and never deletes one. `artifact_problems(artifacts, stages) -> dict[Path, list[str]]` is the composition function: one `story_coordinator.read_story` call per artifact, its parse handed to `story_coordinator.stage_exception_problems` and to `strictness_problems`, keyed by path and holding only the artifacts with problems, so an empty mapping is the whole of "these may be committed". An artifact `read_story` has something to say about yields that and nothing further — `run_story`'s own pre-flight order rather than a second one, because a story that failed to parse has no parse for the later checks and one that failed its *schema* has one whose shape they may not assume (`stage_exception_problems` indexes `exception["stage"]`; `strictness_problems` splits each entry as a string). +- `plan_validation.py` — the decision behind `l5-plan`'s plan-time validation, added by story-025 and kept out of the script for the same reason `plan_commit` is. It **returns** the problems it found; it never prints them, never repairs an artifact and never deletes one. `artifact_problems(artifacts, stages, root) -> dict[Path, list[str]]` is the composition function (`root` is the target root, required and defaulted nowhere, forwarded to the one check below that reads the filesystem): one `story_coordinator.read_story` call per artifact, its parse handed to `story_coordinator.stage_exception_problems` and to `strictness_problems`, keyed by path and holding only the artifacts with problems, so an empty mapping is the whole of "these may be committed". An artifact `read_story` has something to say about yields that and nothing further — `run_story`'s own pre-flight order rather than a second one, because a story that failed to parse has no parse for the later checks and one that failed its *schema* has one whose shape they may not assume (`stage_exception_problems` indexes `exception["stage"]`; `strictness_problems` splits each entry as a string). Five classes of problem since story-038, and the module invents the third, the fourth and the fifth. **Schema conformance** is `read_story`. **Agreement of a story's `stage_exceptions` with the loaded workflow** is `stage_exception_problems`. Neither parsing, schema validation nor the exception cross-check is reimplemented here — a plan-time check with its own reader is the divergence story-005 existed to remove — so `story_parser.parse` and `schema_validator.validate` gained no new caller. Those two are exactly what `l5-run` already refuses; moving them earlier changes nothing about pre-flight, which still refuses the same artifacts with the same status. @@ -101,7 +101,7 @@ The drift source that paragraph used to name is closed: `planner.md` no longer s **What the third check does not catch is stated in the module docstring, where a reader meets it**, because the general problem — is this English sentence stronger than that declared rule — is not decidable and this is the narrow, stated version rather than an attempt at it. Two classes are outside it by construction. A phrasing that names *neither* the stage nor the prefix ("the stage that writes the code leaves the regression suite untouched") restricts exactly what the reported sentences restrict and matches on nothing, because both halves of the match are literals read off the workflow; any paraphrase of either half passes. And a strictness the clause split does not isolate: the split is what makes the check work at all — the historical entries confirm a file *was created* in their first clause and over-restrict a second stage in their second, so an entry-level creation-word test would let every one of them through — at the cost of the mirror case, a single clause that both restricts creation and restricts more than it ("`<stage>` neither creates nor modifies files under `<prefix>`"), which reads as scoped to creation and is not reported. It errs the other way deliberately: a clause that merely *describes* the restriction is reported if it names both halves without a creation word, because refusing a well-meant sentence costs one rephrasing at plan time, which is where a human is present to do it. - The fourth, **`assignment_problems(story, stages)`**, is plan-time-only for exactly the same reason and is otherwise unlike its neighbour — which is why the module docstring now distinguishes the two rather than blending them. It reports one problem per `technical_plan.likely_file_changes` entry whose `file` falls under a prefix the entry's own `stage` is restricted from creating under, unless a grant on that stage covers the file. It is called from `artifact_problems` beside `strictness_problems`, below the `read_story` gate, so the existing order is unchanged. Both halves come off `stage_restrictions` and the grant is decided by `grant_covers`, so the module still names no stage and no prefix; a story with no `technical_plan`, and an entry missing `file` or `stage`, yield nothing rather than raising. It is **structural** — two literals compared, one in the artifact and one in the workflow definition — and inherits none of the third check's hedging: no clause split, no vocabulary of scoping words, no paraphrase that evades it, nothing it declines to decide. See "Work a stage cannot own" below for why the subject is `likely_file_changes` and what the check costs. + The fourth, **`assignment_problems(story, stages, root)`**, is plan-time-only for exactly the same reason and is otherwise unlike its neighbour — which is why the module docstring now distinguishes the two rather than blending them. It reports one problem per `technical_plan.likely_file_changes` entry whose `file` falls under a prefix the entry's own `stage` is restricted from creating under, unless a grant on that stage covers the file — and, since story-042, **only when no such file exists beneath `root`**. It is called from `artifact_problems` beside `strictness_problems`, below the `read_story` gate, so the existing order is unchanged. Both halves come off `stage_restrictions` and the grant is decided by `grant_covers`, so the module still names no stage and no prefix; a story with no `technical_plan`, and an entry missing `file` or `stage`, yield nothing rather than raising. It is **structural** — two literals compared, one in the artifact and one in the workflow definition, plus one question put to the filesystem — and inherits none of the third check's hedging: no clause split, no vocabulary of scoping words, no paraphrase that evades it, nothing it declines to decide. See "Work a stage cannot own" below for why the subject is `likely_file_changes`, what the check costs, and what the existence conjunct is for. The fifth, **`naming_problems(story)`**, is plan-time-only for exactly the reason the third and the fourth are — committed artifacts that have already run name their modules for story numbers, and refusing them at pre-flight would make those stories unrunnable. It reports one problem per `technical_plan.likely_file_changes` entry whose `file` has a basename matching `STORY_NUMBERED_MODULE` (`^test_story_\d+`), and the message says what to name the module instead rather than only that the name is wrong. Its subject is `likely_file_changes` for the reason `assignment_problems` takes it: that is where a plan states which files it expects written, so a name is decidable there, before anything exists under it. It is **structural** like its neighbour — one literal in the artifact against one pattern — and inherits none of the third check's hedging. It is the only one of the five that reads no workflow, so it takes `story` alone; a story with no `technical_plan`, and an entry with no `file`, yield nothing rather than raising. See "A test module is named for what it checks" below for the convention it holds and the two other mechanisms holding it. - `harness_source.py` — a **declaration the suite asserts against**, added by story-040, and the one module here that no run-time path reads. It declares where the harness's own source lives (`HARNESS_SOURCE_DIRS`: orchestration, prompts, workflows, schemas, rules, hooks, scripts, templates), which of those a target reads or describes (`TARGET_FACING_DIRS`: prompts and workflows), the tokens naming a language or toolchain (`STACK_TOKENS`), the directory shapes a target's test layout would use (`TARGET_LAYOUT_PATHS`), the one file exempt from its own scan (`EXEMPT_FILES`), and the directory names that are not source (`SKIP_DIR_NAMES`). `scan(root=None)` walks the declared directories of a repository root — this repository by default, resolved relative to the module the way `schema_validator` resolves `schemas/`, and a throwaway one built by a test otherwise, so both go through one code path — and returns sorted frozen `Finding(path, line_number, line, token, rule)` records. A directory the root does not have is skipped rather than raising, so a throwaway root need only carry what a test is about. Two rules: `STACK_RULE` reads all eight directories, `LAYOUT_RULE` reads `TARGET_FACING_DIRS` only. The coordinator does not call it, nothing routes on it, and no run is refused because of what it reports — see "No target-stack literal in harness source" below for what it is for and what it does not catch. @@ -399,7 +399,17 @@ story-031 escalated seventeen minutes into its implementer on a conflict that wa **Both halves of the check are read off the loaded workflow**, through `stage_restrictions`, per the standing rule: `orchestration/plan_validation.py` names no stage and no path prefix, docstring included, exactly as the strictness check beside it already promised. -**The check is plan-time only and is deliberately absent from `l5-run`'s pre-flight**, the same reasoning `strictness_problems` carries: pre-flight refusal would make committed artifacts that have already run unrunnable. That is not hypothetical — run over the committed corpus under `.harness/stories/`, the new check reports story-029's artifact (fifteen entries assigning files under the restricted prefix to the restricted stage) along with story-007, story-013, story-014, story-019, story-020 and story-021. **story-029 being refused by the new check is the intended outcome, not a regression**: its plan really did assign that work to a stage that could not own it, and it passed on a property of its own diff rather than on the assignment being sound. Pre-flight is untouched, so none of those artifacts becomes unrunnable. +**The check is plan-time only and is deliberately absent from `l5-run`'s pre-flight**, the same reasoning `strictness_problems` carries: pre-flight refusal would make committed artifacts that have already run unrunnable. That is not hypothetical — run over the committed corpus under `.harness/stories/`, the new check reports story-029's artifact (fifteen entries assigning files under the restricted prefix to the restricted stage) along with story-007, story-013, story-014, story-019, story-020 and story-021. (After story-042's narrowing the set is story-029 — thirteen entries survive, all naming modules story-038 renamed away — with story-007, story-013, story-014, story-019 and story-021; story-041's is reported by nothing.) **story-029 being refused by the new check is the intended outcome, not a regression**: its plan really did assign that work to a stage that could not own it, and it passed on a property of its own diff rather than on the assignment being sound. Pre-flight is untouched, so none of those artifacts becomes unrunnable. + +**story-042 narrowed the check to the rule it enforces.** As written the condition guessed: an entry says only which stage is expected to *write* a file, never whether writing it means creating it, and the restriction is about creation alone. So the check refused both cases, including the one the restriction does not cover. Modifying a file that already exists is legal and governed by a different instrument entirely — the revert check, which restores the stage's edits under governed prefixes, re-runs the suite, and decides whether they were forced. The plan-time check was applying the creation rule to a case the revert check owns. The evidence was story-041's planning session, refused for four entries naming files that all exist here; not one could have been created, and the story required editing every one. The three stories before it had their implementers edit files under the same prefix, all three were permitted by the revert check, and none of the three plans predicted those edits — so the check refused a plan that forecast its forced edits and passed a plan that stayed quiet. **A check that rewards a vaguer plan is the defect**, not the refusals themselves. + +The added conjunct is one question: `(root / entry_path).exists()` is false. `exists()` rather than `is_file()`, so a path present as a directory counts as existing exactly as a regular file does. The grant check still short-circuits above it, so `grant_covers` keeps deciding grants and the existence question decides nothing about them. + +**Existence is resolved against the target root**, the third parameter both `assignment_problems` and `artifact_problems` now require. Not the harness root and not the process working directory: the three coincide in this repository and will not in general, and no parameter carries a default, so a two-argument call raises rather than silently answering against whatever directory the process is standing in. `scripts/l5-plan` passes the root `main` already resolves through `harness_config.find_target_root` — one argument through `report()`, which prints and returns exactly what it did before. The fact is derived from the repository rather than declared by a new `likely_file_changes` field: a field would be the planner's claim about something the filesystem already knows, the check would have to verify it against the repository anyway to be trustworthy, and the forty-odd committed artifacts do not carry it. + +**The narrowed check is still a prediction and does not claim otherwise.** A file present when the plan is written may be gone by the time the story runs. The run-time ownership check and the revert check remain the authority on what a stage actually did; this one only refuses a plan that could not have worked out. + +**No run is affected.** `assignment_problems` is called only from `artifact_problems`, which is called only from `scripts/l5-plan`. `run_story` calls neither, at pre-flight or anywhere else, before this story or after it — and a test asserts that rather than leaving it to inspection. A change to a check usually does move run behaviour; this one does not, which is why it is written down. After the narrowing the corpus is not silently empty either: story-029 is still reported, because story-038 renamed away every module its thirteen entries named, so none of them exists — the corpus keeps a true positive. **This story's own plan was not governed by the check it adds**, for the eighth time in this pattern's history, and the cause is a new one worth naming: the artifact was written and committed by an `l5-plan` session that ran before `assignment_problems` existed. It happens to be clean — it assigns its test module to the stage that owns validation, which is the convention this story writes down — but it was not checked. Enforcement begins with the next plan, which is one step earlier than the stale-workflow and stale-import cases, since the artifact rather than the run is what escapes. diff --git a/orchestration/plan_validation.py b/orchestration/plan_validation.py index f0b3c2f..4850510 100644 --- a/orchestration/plan_validation.py +++ b/orchestration/plan_validation.py @@ -80,23 +80,49 @@ What the fourth check is ------------------------ -A plan must not assign work to a stage that cannot own it. A +Two rules govern a stage's relationship with a governed prefix and this check +used to conflate them. The workflow's restriction is about *creating* files +there, and it is the one this check enforces. *Modifying* a file that already +exists is legal and is governed by the revert check instead, which restores +the stage's edits under the governed prefixes and re-runs the suite, +escalating when they were not forced and recording them as permitted when +they were. That is a question about what a run did, so it is a run's to +answer; nothing here duplicates it or anticipates it. + +So a plan must not assign work to a stage that cannot own it, and what it +cannot own is a file it would have to *create*. A technical_plan.likely_file_changes entry naming a file beneath a prefix its own stage is restricted from creating under, with no grant covering that -file, describes a run that can only end one way: the stage does exactly what -the plan named, and the coordinator refuses the result. Both halves of the -conflict are literals — one in the artifact, one in the workflow definition — -so it is fully decidable before the run starts, and the decision belongs -where a developer is present to repair it in one exchange. +file, and with no such file beneath the root, describes a run that can only +end one way: the stage does exactly what the plan named, and the coordinator +refuses the result. All of that is decidable before the run starts — two +literals, one in the artifact and one in the workflow definition, and one +question put to the filesystem — and the decision belongs where a developer +is present to repair it in one exchange. + +Existence is resolved against the **target root**, the repository the story +will run in, which artifact_problems requires of its caller and passes down. +It is neither the harness root nor the process working directory: the three +coincide when the harness is its own target and will not in general, and no +default hides which one was consulted. The fact is derived from the +repository rather than declared by a likely_file_changes field, because a +field would be the planner's claim about something the filesystem already +knows and the check would have to verify it against the repository anyway. That is why it is structural rather than a scan of English. It compares an -entry's declared file against an entry's declared stage; it does not read -prose, does not guess at intent, and does not err in either direction. -Nothing about it is hedged, and the limits recorded above for the third check -are not its limits. likely_file_changes is its subject because it is the only -field carrying a file and a stage together — scope.modify names paths with no -stage and cannot state this conflict at all. A story with no technical_plan, -or an entry missing either field, yields nothing rather than raising. +entry's declared file against an entry's declared stage and asks the +repository whether the file is there; it does not read prose, does not guess +at intent, and does not err in either direction. Nothing about it is hedged, +and the limits recorded above for the third check are not its limits. +likely_file_changes is its subject because it is the only field carrying a +file and a stage together — scope.modify names paths with no stage and cannot +state this conflict at all. A story with no technical_plan, or an entry +missing either field, yields nothing rather than raising. + +What it stays is a **prediction**. A file present when the plan is written +may be gone by the time the story runs, and this check makes no promise +about that: the run-time ownership check and the revert check remain the +authority on what a stage was actually allowed to do. What the fifth check is ----------------------- @@ -185,7 +211,7 @@ def strictness_problems(story: dict, stages: list[dict]) -> list[str]: return problems -def assignment_problems(story: dict, stages: list[dict]) -> list[str]: +def assignment_problems(story: dict, stages: list[dict], root: Path) -> list[str]: """Report plan entries assigning a file to a stage that cannot own it. The subject is technical_plan.likely_file_changes and it is the only place @@ -195,11 +221,24 @@ def assignment_problems(story: dict, stages: list[dict]) -> list[str]: restriction. An entry offends when its file falls under a prefix the entry's own stage - is restricted from creating under and no grant on that stage covers the - file. Both halves come off story_coordinator.stage_restrictions and the - grant is decided by story_coordinator.grant_covers, so no stage name and - no prefix is written here — the same promise the check beside this one - makes. + is restricted from creating under, no grant on that stage covers the file, + and no such file exists beneath `root`. The last is what keeps the check to + the rule it enforces: the workflow restricts *creating* files under a + prefix, and a file that is already there is not one the stage can create. + An entry naming an existing file predicts a modification, which the revert + check owns at run time and this check says nothing about. + + `root` is the repository existence is resolved against — the target root, + not the harness root and not the process working directory. It is required + and carries no default, so no caller can silently be given whatever + directory the process happens to be standing in. Existence is decided with + exists() on the root joined with the entry's path, so a path present as a + directory counts exactly as a regular file does. + + Both halves of the restriction come off story_coordinator.stage_restrictions + and the grant is decided by story_coordinator.grant_covers, so no stage name + and no prefix is written here — the same promise the check beside this one + makes; the path comes off the entry. A story carrying no technical_plan, and an entry missing either field, yield no problem rather than an error: this reports a conflict it can see @@ -219,7 +258,11 @@ def assignment_problems(story: dict, stages: list[dict]) -> list[str]: if story_coordinator.grant_covers(granted, path): continue for stage, prefix in restrictions: - if stage == name and path.startswith(prefix): + if ( + stage == name + and path.startswith(prefix) + and not (Path(root) / path).exists() + ): problems.append( f"$.technical_plan.likely_file_changes[{index}]: assigns " f"'{path}' to stage '{name}', which the workflow declares: " @@ -280,7 +323,7 @@ def naming_problems(story: dict) -> list[str]: def artifact_problems( - artifacts: Iterable[Path], stages: list[dict] + artifacts: Iterable[Path], stages: list[dict], root: Path ) -> dict[Path, list[str]]: """Validate each artifact a planning session added; report what is wrong. @@ -299,6 +342,13 @@ def artifact_problems( omits one: the story schema ships with the harness code and schema_validator resolves it relative to its own module, so plan time and pre-flight load the one file. + + `root` is the target repository the stories will run in, required for the + same reason assignment_problems requires it: it is the only check here that + asks the filesystem anything, and a defaulted root would let a caller + resolve existence against the process working directory without saying so. + The two checks that read no filesystem — strictness_problems and + naming_problems — keep their signatures. """ problems: dict[Path, list[str]] = {} for artifact in artifacts: @@ -309,7 +359,7 @@ def artifact_problems( if not found: found += story_coordinator.stage_exception_problems(reading.parsed, stages) found += strictness_problems(reading.parsed, stages) - found += assignment_problems(reading.parsed, stages) + found += assignment_problems(reading.parsed, stages, root) found += naming_problems(reading.parsed) if found: problems[Path(artifact)] = found diff --git a/prompts/planner.md b/prompts/planner.md index 9bff50f..58d949e 100644 --- a/prompts/planner.md +++ b/prompts/planner.md @@ -129,10 +129,14 @@ below. A likely_file_changes entry naming a file beneath a restricted path, assigned to the very stage restricted from creating there, is refused when -the session ends: the run it describes could only ever end in the harness -refusing the result. The problem names the file, the stage, the prefix, and -the two ways out — reassign the file to a stage that may own it, or declare -a stage_exceptions grant naming it. +the session ends — but only when no such file exists in the target +repository, because then the entry describes a creation and the run it +describes could only ever end in the harness refusing the result. An entry +naming a file that is already there predicts a modification, not a creation, +and is not refused: predict those edits rather than leaving them out. The +problem names the file, the stage, the prefix, and the two ways out — +reassign the file to a stage that may own it, or declare a stage_exceptions +grant naming it. A stage_exceptions entry lifts one of those restrictions for one story, which is what a story whose own deliverable is a test suite needs. diff --git a/scripts/l5-plan b/scripts/l5-plan index 659c486..1c43cfc 100755 --- a/scripts/l5-plan +++ b/scripts/l5-plan @@ -109,7 +109,11 @@ def report(target_root: Path, stories_dir: Path, before, stages, config=None, ba # and re-runs from. When one of several artifacts fails, none is # committed; the commit is one commit for the session's artifacts, and a # partial one would invent a split the session did not have. - problems = plan_validation.artifact_problems(artifacts, stages) + # The target root goes with the artifacts: the assignment check asks the + # repository the story will run in whether a file it names is already + # there, and that repository is the one find_target_root resolved, never + # the harness root and never wherever this process was started from. + problems = plan_validation.artifact_problems(artifacts, stages, target_root) if problems: for path, found in problems.items(): # The coordinator's own refusal helper, so the text a developer diff --git a/tests/test_plan_assignment_refusal.py b/tests/test_plan_assignment_refusal.py index b2db7a2..6a82b98 100644 --- a/tests/test_plan_assignment_refusal.py +++ b/tests/test_plan_assignment_refusal.py @@ -51,16 +51,41 @@ Nothing here invokes a model: every coordinator run goes through a fake agent runner, and every planning run goes through the stub session. + +Since story-042 the same check is narrowed: an entry is reported only when the +file it names does *not* exist beneath the target root, because a file that is +already there is one the stage would modify rather than create, and modifying +is the revert check's question at run time. The last section validates that +narrowing, and every assertion in it is a matched pair or carries its own +control: + + * "the present file is not reported" sits beside the very same story and the + very same entry checked against a root that does not hold the file, which + is reported; + * "the root decides" is not reasoned about: the process working directory is + moved to a root that disagrees with the one passed in, in both directions, + and the answer follows the argument; + * "a grant still short-circuits" sits beside the same story with the grant + removed, and beside the same story with `grant_covers` replaced, so the + grant is shown to be what decided it; + * "story-041's committed artifact is reported by nothing" is read off the + file on disk through the reader a run uses, beside the same artifact + checked against a root holding none of its files, which is reported, and + beside story-029, which this repository still reports; + * "no run reads the check" is driven rather than inspected: both functions + are spied on across a real coordinator run of a story carrying the + conflict, and the spies are shown to fire when the check is called. """ import inspect import json import re +import subprocess import sys from pathlib import Path import pytest -from conftest import load_mutant +from conftest import load_mutant, load_script from test_revert_check import ( # noqa: F401 - fixtures used by name APP_ADDITIVE, @@ -92,6 +117,7 @@ sys.path.insert(0, str(HARNESS_ROOT / "orchestration")) import harness_config # noqa: E402 +import plan_commit # noqa: E402 import plan_validation # noqa: E402 import story_coordinator # noqa: E402 @@ -131,6 +157,16 @@ #: second governed path is needed. OUTSIDE_EVERY_PREFIX = "orchestration/story_coordinator.py" +#: The root the check resolves existence against, for the assertions here whose +#: subject is the assignment itself. Since story-042 an entry is reported only +#: when the file it names does not exist beneath the given root — a file that +#: is already there is one the stage would modify, not create — and every path +#: this section names is absent beneath this one, so each assertion below still +#: turns on the assignment rather than on what this repository happens to hold. +#: The corpus assertions pass HARNESS_ROOT instead, because the corpus is read +#: from this repository and is about it. +ABSENT_ROOT = HARNESS_ROOT / "a-directory-this-repository-does-not-have" + def test_the_workflow_this_file_reads_still_has_something_to_say(): """Every derivation above is load-bearing; an empty one would go green.""" @@ -138,6 +174,7 @@ def test_the_workflow_this_file_reads_still_has_something_to_say(): assert UNRESTRICTED_STAGE in STAGE_NAMES assert UNDEFINED_STAGE not in STAGE_NAMES assert STORY_031_FILE.startswith(RESTRICTED_PREFIX) + assert not ABSENT_ROOT.exists() assert not OUTSIDE_EVERY_PREFIX.startswith(RESTRICTED_PREFIX) for _, prefix in RESTRICTIONS: assert not OUTSIDE_EVERY_PREFIX.startswith(prefix) @@ -181,7 +218,7 @@ def with_grant(story: dict, create: str, stage: str = RESTRICTED_STAGE) -> dict: def test_the_conflict_story_031_carried_is_reported(): - problems = plan_validation.assignment_problems(CONFLICT, STAGES) + problems = plan_validation.assignment_problems(CONFLICT, STAGES, ABSENT_ROOT) assert len(problems) == 1, problems @@ -192,7 +229,7 @@ def test_the_reported_problem_names_the_file_the_stage_the_prefix_and_both_ways_ without going to the workflow definition, which is the point of stating the restriction in the workflow's own words. """ - (problem,) = plan_validation.assignment_problems(CONFLICT, STAGES) + (problem,) = plan_validation.assignment_problems(CONFLICT, STAGES, ABSENT_ROOT) assert STORY_031_FILE in problem assert STORY_031_STAGE in problem @@ -209,35 +246,35 @@ def test_the_reported_problem_names_the_file_the_stage_the_prefix_and_both_ways_ def test_the_same_plan_naming_a_stage_that_may_own_the_file_yields_nothing(): """The first clean resolution.""" resolved = plan(entry(STORY_031_FILE, UNRESTRICTED_STAGE)) - assert plan_validation.assignment_problems(resolved, STAGES) == [] + assert plan_validation.assignment_problems(resolved, STAGES, ABSENT_ROOT) == [] def test_the_same_plan_with_a_grant_naming_that_exact_file_yields_nothing(): """The second clean resolution, at the granularity of one file.""" resolved = with_grant(CONFLICT, STORY_031_FILE) - assert plan_validation.assignment_problems(resolved, STAGES) == [] + assert plan_validation.assignment_problems(resolved, STAGES, ABSENT_ROOT) == [] def test_a_grant_naming_the_whole_prefix_also_yields_nothing(): resolved = with_grant(CONFLICT, RESTRICTED_PREFIX) - assert plan_validation.assignment_problems(resolved, STAGES) == [] + assert plan_validation.assignment_problems(resolved, STAGES, ABSENT_ROOT) == [] def test_a_grant_naming_a_different_file_beneath_the_prefix_does_not_suppress_it(): """The grant is not a prefix match here either.""" other = with_grant(CONFLICT, f"{RESTRICTED_PREFIX}test_something_else.py") - assert len(plan_validation.assignment_problems(other, STAGES)) == 1 + assert len(plan_validation.assignment_problems(other, STAGES, ABSENT_ROOT)) == 1 def test_a_grant_on_another_stage_does_not_suppress_it(): other = with_grant(CONFLICT, STORY_031_FILE, stage=UNRESTRICTED_STAGE) - assert len(plan_validation.assignment_problems(other, STAGES)) == 1 + assert len(plan_validation.assignment_problems(other, STAGES, ABSENT_ROOT)) == 1 @pytest.mark.parametrize("stage", STAGE_NAMES) def test_a_file_outside_every_declared_prefix_yields_nothing_whatever_the_stage(stage): outside = plan(entry(OUTSIDE_EVERY_PREFIX, stage)) - assert plan_validation.assignment_problems(outside, STAGES) == [] + assert plan_validation.assignment_problems(outside, STAGES, ABSENT_ROOT) == [] @pytest.mark.parametrize( @@ -246,7 +283,7 @@ def test_a_file_outside_every_declared_prefix_yields_nothing_whatever_the_stage( def test_a_file_beneath_a_prefix_assigned_to_an_unrestricted_stage_yields_nothing( stage): allowed = plan(entry(STORY_031_FILE, stage)) - assert plan_validation.assignment_problems(allowed, STAGES) == [] + assert plan_validation.assignment_problems(allowed, STAGES, ABSENT_ROOT) == [] # -------------------------------------------------------------------------- @@ -270,11 +307,11 @@ def test_a_half_it_cannot_see_yields_no_problem_and_raises_nothing(story): Non-vacuous because the same call over the complete entry — the control directly below — does report it. """ - assert plan_validation.assignment_problems(story, STAGES) == [] + assert plan_validation.assignment_problems(story, STAGES, ABSENT_ROOT) == [] def test_the_control_for_every_incomplete_entry_above_is_the_complete_one(): - assert plan_validation.assignment_problems(CONFLICT, STAGES) != [] + assert plan_validation.assignment_problems(CONFLICT, STAGES, ABSENT_ROOT) != [] # -------------------------------------------------------------------------- @@ -297,7 +334,7 @@ def test_both_halves_of_the_match_come_off_the_loaded_workflow(): entry(STORY_031_FILE, STORY_031_STAGE), ) - problems = plan_validation.assignment_problems(story, synthetic_stages()) + problems = plan_validation.assignment_problems(story, synthetic_stages(), ABSENT_ROOT) assert len(problems) == 1, problems assert "cartography/atlas.py" in problems[0] @@ -305,15 +342,15 @@ def test_both_halves_of_the_match_come_off_the_loaded_workflow(): # The real pair is silent here, and the synthetic pair is silent against # the real workflow: neither is written into the module. assert STORY_031_FILE not in problems[0] - assert plan_validation.assignment_problems(story, STAGES) != [] + assert plan_validation.assignment_problems(story, STAGES, ABSENT_ROOT) != [] assert "cartography" not in " ".join( - plan_validation.assignment_problems(story, STAGES)) + plan_validation.assignment_problems(story, STAGES, ABSENT_ROOT)) def test_a_workflow_that_restricts_nothing_reports_nothing(): unrestricted = [{"name": name} for name in STAGE_NAMES] assert story_coordinator.stage_restrictions(unrestricted) == [] - assert plan_validation.assignment_problems(CONFLICT, unrestricted) == [] + assert plan_validation.assignment_problems(CONFLICT, unrestricted, ABSENT_ROOT) == [] def literals_named(text: str) -> list[str]: @@ -391,15 +428,15 @@ def test_every_artifact_this_file_uses_is_what_it_claims_to_be(): reading.parsed, STAGES) == [], name assert plan_validation.strictness_problems(reading.parsed, STAGES) == [], name assert plan_validation.assignment_problems( - story_coordinator.read_story(CONFLICTING_ARTIFACT).parsed, STAGES) != [] + story_coordinator.read_story(CONFLICTING_ARTIFACT).parsed, STAGES, ABSENT_ROOT) != [] for clean in (REASSIGNED_ARTIFACT, GRANTED_ARTIFACT): assert plan_validation.assignment_problems( - story_coordinator.read_story(clean).parsed, STAGES) == [] + story_coordinator.read_story(clean).parsed, STAGES, ABSENT_ROOT) == [] def test_artifact_problems_reports_the_new_class(tmp_path: Path): path = write_artifact(tmp_path, CONFLICTING_ARTIFACT) - found = plan_validation.artifact_problems([path], STAGES) + found = plan_validation.artifact_problems([path], STAGES, ABSENT_ROOT) assert list(found) == [path] assert any(STORY_031_FILE in problem for problem in found[path]) @@ -407,7 +444,7 @@ def test_artifact_problems_reports_the_new_class(tmp_path: Path): def test_artifact_problems_holds_the_clean_resolutions_back(tmp_path: Path): for index, text in enumerate((REASSIGNED_ARTIFACT, GRANTED_ARTIFACT)): path = write_artifact(tmp_path, text, f"story-90{index}.yaml") - assert plan_validation.artifact_problems([path], STAGES) == {} + assert plan_validation.artifact_problems([path], STAGES, ABSENT_ROOT) == {} def test_a_story_that_fails_the_gate_yields_that_and_nothing_further(tmp_path: Path): @@ -417,7 +454,7 @@ def test_a_story_that_fails_the_gate_yields_that_and_nothing_further(tmp_path: P check — the second half below. """ unparseable = write_artifact(tmp_path, "this: is: not: a story\n\t- ?\n") - found = plan_validation.artifact_problems([unparseable], STAGES) + found = plan_validation.artifact_problems([unparseable], STAGES, ABSENT_ROOT) assert found[unparseable] assert not any(STORY_031_FILE in problem for problem in found[unparseable]) @@ -426,14 +463,14 @@ def test_a_story_that_fails_the_gate_yields_that_and_nothing_further(tmp_path: P tmp_path, CONFLICTING_ARTIFACT.replace("tasks:\n - do the sample work\n", ""), "story-901.yaml") - found = plan_validation.artifact_problems([invalid], STAGES) + found = plan_validation.artifact_problems([invalid], STAGES, ABSENT_ROOT) assert found[invalid] assert not any(STORY_031_FILE in problem for problem in found[invalid]) reached = write_artifact(tmp_path, CONFLICTING_ARTIFACT, "story-902.yaml") assert any(STORY_031_FILE in problem for problem in plan_validation.artifact_problems( - [reached], STAGES)[reached]) + [reached], STAGES, ABSENT_ROOT)[reached]) def test_the_strictness_check_still_reports_beside_the_new_one(tmp_path: Path): @@ -445,7 +482,7 @@ def test_the_strictness_check_still_reports_beside_the_new_one(tmp_path: Path): f"entirely\n" + plan_block((STORY_031_FILE, STORY_031_STAGE))) path = write_artifact(tmp_path, both) - problems = plan_validation.artifact_problems([path], STAGES)[path] + problems = plan_validation.artifact_problems([path], STAGES, ABSENT_ROOT)[path] assert any("states a restriction the workflow does not" in p for p in problems) assert any("a stage that may own it" in p for p in problems) @@ -473,7 +510,8 @@ def test_the_committed_artifact_story_029_shipped_is_reported_by_the_new_check() stories = corpus() assert stories, "no committed story artifact parsed" reported = {name for name, story in stories.items() - if plan_validation.assignment_problems(story, STAGES)} + if plan_validation.assignment_problems(story, STAGES, + HARNESS_ROOT)} assert "story-029" in reported # Not everything is reported, so "reported" is a property of the artifact @@ -490,7 +528,8 @@ def test_no_committed_artifact_becomes_unrunnable(tmp_path: Path): """ stories = corpus() reported = [story for name, story in stories.items() - if plan_validation.assignment_problems(story, STAGES)] + if plan_validation.assignment_problems(story, STAGES, + HARNESS_ROOT)] assert reported for story in reported: assert story_coordinator.stage_exception_problems(story, STAGES) == [] @@ -695,7 +734,7 @@ def test_the_three_readers_all_go_through_the_one_matcher(monkeypatch, granted_story = with_grant(plan(entry(FILE_GRANT, RESTRICTED_STAGE)), FILE_GRANT) # With the real matcher, all three say "covered". - assert plan_validation.assignment_problems(granted_story, STAGES) == [] + assert plan_validation.assignment_problems(granted_story, STAGES, ABSENT_ROOT) == [] assert story_coordinator._ownership_violation( run_dir, "changed-files.json", prefixes, granted) is None assert story_coordinator.governed_edits( @@ -704,7 +743,7 @@ def test_the_three_readers_all_go_through_the_one_matcher(monkeypatch, monkeypatch.setattr(story_coordinator, "grant_covers", lambda granted, path: False) - assert plan_validation.assignment_problems(granted_story, STAGES) != [] + assert plan_validation.assignment_problems(granted_story, STAGES, ABSENT_ROOT) != [] assert story_coordinator._ownership_violation( run_dir, "changed-files.json", prefixes, granted) is not None assert story_coordinator.governed_edits( @@ -1100,3 +1139,525 @@ def test_the_clause_level_scan_is_untouched_by_this_story(tmp_path: Path): [("creat(?:e|es|ed|ing|ion)", "nothing(?:-at-all)")], name="plan_validation_without_the_creation_word", tmp_path=tmp_path) assert mutant.strictness_problems(scoped, STAGES) != [] + + +# ========================================================================== +# story-042: an entry naming a file that already exists is a modification +# +# The check enforces the workflow's *creation* restriction. A file already +# beneath the target root is not one the stage can create, so an entry naming +# it predicts a modification — which the revert check owns at run time — and +# is not reported. Every assertion below is one half of a matched pair: the +# same story, the same entry, two roots that differ only in whether the named +# file is there. +# ========================================================================== + + +#: The two paths this section names, both beneath the restricted prefix and +#: both assigned to the restricted stage, so the only thing that can differ +#: between an accepted and a refused answer is whether the file exists. +PRESENT_FILE = f"{RESTRICTED_PREFIX}test_already_on_disk.py" +ABSENT_FILE = f"{RESTRICTED_PREFIX}test_not_written_yet.py" +PRESENT_DIRECTORY = f"{RESTRICTED_PREFIX}a_directory_that_is_there" + +PRESENT = plan(entry(PRESENT_FILE, RESTRICTED_STAGE)) +ABSENT = plan(entry(ABSENT_FILE, RESTRICTED_STAGE)) + + +def roots(tmp_path: Path) -> tuple[Path, Path]: + """A root holding this section's paths, and one holding none of them. + + Both are real directories, so "absent" is a root that exists and does not + hold the file rather than a root that is not there at all — the weaker of + the two conditions, and the one a plan is actually written against. + """ + holding, empty = tmp_path / "holding", tmp_path / "empty" + (holding / PRESENT_FILE).parent.mkdir(parents=True, exist_ok=True) + (holding / PRESENT_FILE).write_text("# already here\n", encoding="utf-8") + (holding / PRESENT_DIRECTORY).mkdir(parents=True, exist_ok=True) + (empty / RESTRICTED_PREFIX).mkdir(parents=True, exist_ok=True) + return holding, empty + + +def test_the_two_roots_this_section_uses_are_what_it_assumes(tmp_path: Path): + holding, empty = roots(tmp_path) + for root in (holding, empty): + assert root.is_dir() + assert (holding / PRESENT_FILE).is_file() + assert (holding / PRESENT_DIRECTORY).is_dir() + for path in (PRESENT_FILE, ABSENT_FILE, PRESENT_DIRECTORY): + assert path.startswith(RESTRICTED_PREFIX) + assert not (empty / path).exists() + assert not (holding / ABSENT_FILE).exists() + + +def test_an_entry_naming_a_file_that_exists_beneath_the_root_is_not_reported( + tmp_path: Path): + """The accepting half. Its control is the refusing half directly below. + + Same story, same entry, same workflow: the roots are the only difference, + so the two answers together are what show the existence question is asked + at all. + """ + holding, empty = roots(tmp_path) + + assert plan_validation.assignment_problems(PRESENT, STAGES, holding) == [] + assert len(plan_validation.assignment_problems(PRESENT, STAGES, empty)) == 1 + + +def test_an_entry_naming_a_file_that_does_not_exist_is_still_reported( + tmp_path: Path): + """The refusing half: that entry does describe a creation. + + A narrowing that had disabled the check outright would pass the test above + and fail this one. + """ + holding, _ = roots(tmp_path) + + (problem,) = plan_validation.assignment_problems(ABSENT, STAGES, holding) + assert ABSENT_FILE in problem + + +def test_the_message_for_the_reported_case_is_word_for_word_todays( + tmp_path: Path): + """Read off the produced text, not off the source that builds it. + + The whole message, not four substrings of it: the case that survives the + narrowing is the case the message was written for, so any rewording of it + is a change this story was not allowed to make. + """ + _, empty = roots(tmp_path) + + (problem,) = plan_validation.assignment_problems(PRESENT, STAGES, empty) + + assert problem == ( + f"$.technical_plan.likely_file_changes[0]: assigns " + f"'{PRESENT_FILE}' to stage '{RESTRICTED_STAGE}', which the workflow " + f"declares: {RESTRICTED_STAGE} may not create files under " + f"{RESTRICTED_PREFIX}. Either assign '{PRESENT_FILE}' to a stage that " + f"may own it, or declare a stage_exceptions grant naming " + f"'{PRESENT_FILE}' for {RESTRICTED_STAGE}." + ) + + +def test_a_path_present_as_a_directory_counts_as_existing(tmp_path: Path): + """exists(), not is_file(): a directory is there and cannot be created. + + Beside the same entry against the root that does not hold it, which is + reported, so this is not a directory the check simply never looked at. + """ + holding, empty = roots(tmp_path) + directory = plan(entry(PRESENT_DIRECTORY, RESTRICTED_STAGE)) + + assert plan_validation.assignment_problems(directory, STAGES, holding) == [] + assert len(plan_validation.assignment_problems(directory, STAGES, empty)) == 1 + # And it is the same answer a regular file gets, rather than a second rule. + assert plan_validation.assignment_problems(PRESENT, STAGES, holding) == [] + + +def test_the_root_argument_decides_and_not_the_process_working_directory( + tmp_path: Path, monkeypatch): + """Driven from a working directory that disagrees with the root, both ways. + + The two coincide when the harness is its own target, so a check run from + the repository root would prove nothing here. Standing in the root that + holds the file while passing the one that does not must still report, and + standing in the root that does not while passing the one that does must + still stay silent; a check reading `Path.cwd()` fails both. + """ + holding, empty = roots(tmp_path) + + monkeypatch.chdir(holding) + assert len(plan_validation.assignment_problems(PRESENT, STAGES, empty)) == 1 + + monkeypatch.chdir(empty) + assert plan_validation.assignment_problems(PRESENT, STAGES, holding) == [] + + +def test_a_relative_root_is_still_the_root_it_was_given(tmp_path: Path, + monkeypatch): + """The control above, with the root written relative to somewhere else. + + A relative root is resolved by the process working directory, so this is + the one case where the two are allowed to interact — and the interaction + is the caller's, not the check's. + """ + holding, empty = roots(tmp_path) + + monkeypatch.chdir(tmp_path) + assert plan_validation.assignment_problems(PRESENT, STAGES, + Path("holding")) == [] + assert len(plan_validation.assignment_problems(PRESENT, STAGES, + Path("empty"))) == 1 + + +def test_neither_function_hides_the_root_behind_a_default(): + """A two-argument call raises rather than falling back to the cwd.""" + for function in (plan_validation.assignment_problems, + plan_validation.artifact_problems): + parameters = list(inspect.signature(function).parameters.values()) + assert len(parameters) == 3, function.__name__ + assert parameters[-1].default is inspect.Parameter.empty, function.__name__ + + with pytest.raises(TypeError): + plan_validation.assignment_problems(PRESENT, STAGES) + with pytest.raises(TypeError): + plan_validation.artifact_problems([], STAGES) + # Control: the three-argument calls those two are missing an argument for + # do not raise. + assert plan_validation.assignment_problems(PRESENT, STAGES, ABSENT_ROOT) != [] + assert plan_validation.artifact_problems([], STAGES, ABSENT_ROOT) == {} + + +def test_the_two_checks_that_read_no_filesystem_keep_their_signatures(): + """strictness_problems and naming_problems were left alone.""" + assert list(inspect.signature( + plan_validation.strictness_problems).parameters) == ["story", "stages"] + assert list(inspect.signature( + plan_validation.naming_problems).parameters) == ["story"] + + +def test_a_grant_still_short_circuits_before_existence_is_asked(tmp_path: Path): + """grant_covers keeps deciding grants; the new condition decides none of it. + + Two controls: the same story with the grant removed is reported, so the + grant is what silenced it; and the same story with `grant_covers` replaced + by a matcher that covers nothing is reported too, so the grant reached the + answer through that function rather than through the file being anywhere. + """ + _, empty = roots(tmp_path) + granted = with_grant(PRESENT, PRESENT_FILE) + + assert plan_validation.assignment_problems(granted, STAGES, empty) == [] + assert len(plan_validation.assignment_problems(PRESENT, STAGES, empty)) == 1 + + +def test_the_grant_is_what_silences_it_and_not_the_existence_question( + tmp_path: Path, monkeypatch): + _, empty = roots(tmp_path) + granted = with_grant(PRESENT, PRESENT_FILE) + + monkeypatch.setattr(story_coordinator, "grant_covers", + lambda granted, path: False) + + assert len(plan_validation.assignment_problems(granted, STAGES, empty)) == 1 + + +def test_artifact_problems_resolves_existence_against_the_root_it_is_given( + tmp_path: Path): + """The same pair one level up, through the function l5-plan calls.""" + holding, empty = roots(tmp_path) + text = artifact("story-900") + plan_block((PRESENT_FILE, RESTRICTED_STAGE)) + path = write_artifact(tmp_path, text, "story-903.yaml") + + assert plan_validation.artifact_problems([path], STAGES, holding) == {} + assert any(PRESENT_FILE in problem + for problem in plan_validation.artifact_problems( + [path], STAGES, empty)[path]) + + +# -------------------------------------------------------------------------- +# The committed corpus after the narrowing +# -------------------------------------------------------------------------- + + +def story_on_disk(story_id: str) -> tuple[Path, dict]: + """One committed artifact and its parse, through the reader a run uses.""" + path = STORIES_DIR / f"{story_id}.yaml" + reading = story_coordinator.read_story(path.read_text(encoding="utf-8")) + assert reading.problems == [], (story_id, reading.problems) + return path, reading.parsed + + +def test_story_041s_committed_artifact_is_reported_by_nothing(tmp_path: Path): + """The observed case that motivated this story, read from disk. + + Its four entries assign existing files to the restricted stage, and every + one of them was refused before the narrowing. Two controls, so this is not + an artifact the check merely never looked at: the same artifact checked + against a root holding none of its files is reported, and the entries are + required to be the conflicting kind — beneath the restricted prefix, on + the restricted stage — rather than merely uninteresting. + """ + path, story = story_on_disk("story-041") + + assert plan_validation.artifact_problems([path], STAGES, HARNESS_ROOT) == {} + + conflicting = [e for e in story["technical_plan"]["likely_file_changes"] + if e["stage"] == RESTRICTED_STAGE + and e["file"].startswith(RESTRICTED_PREFIX)] + assert conflicting, "story-041 no longer carries the entries this is about" + for named in conflicting: + assert (HARNESS_ROOT / named["file"]).exists(), named["file"] + empty = tmp_path / "holds-none-of-them" + empty.mkdir() + assert len(plan_validation.assignment_problems(story, STAGES, empty)) == \ + len(conflicting) + + +def test_the_corpus_still_holds_a_true_positive_after_the_narrowing(): + """story-029 is still reported, and reported for files that are not there. + + story-038 renamed away every module it named for the implementer, so none + of them can exist and none of them could have been created — which is what + keeps the corpus evidence for the refusing half from being silently empty. + """ + path, story = story_on_disk("story-029") + + reported = plan_validation.assignment_problems(story, STAGES, HARNESS_ROOT) + assert reported + assert plan_validation.artifact_problems([path], STAGES, HARNESS_ROOT)[path] + for named in story["technical_plan"]["likely_file_changes"]: + if any(named["file"] in problem for problem in reported): + assert not (HARNESS_ROOT / named["file"]).exists(), named["file"] + + +def test_the_corpus_after_the_narrowing_is_neither_all_reported_nor_none(): + """Both halves are exercised by artifacts this repository actually holds.""" + stories = corpus() + reported = {name for name, story in stories.items() + if plan_validation.assignment_problems(story, STAGES, + HARNESS_ROOT)} + assert reported + assert reported != set(stories) + assert "story-041" not in reported + assert "story-029" in reported + + +# -------------------------------------------------------------------------- +# The module still names neither half of the restriction, docstring included +# -------------------------------------------------------------------------- + + +def test_the_module_names_no_stage_and_no_prefix_in_its_prose_either(): + """The raw text, not the stripped text: the docstring was rewritten here. + + `literals_named` strips docstrings and comments, which is right for the + promise about *code* and would miss a stage name written into the + rewritten prose. Beside the same scan over the same text with each literal + planted in it. + """ + module = (HARNESS_ROOT / "orchestration" / "plan_validation.py").read_text( + encoding="utf-8") + + for name in STAGE_NAMES: + assert not re.search(rf"\b{re.escape(name)}\b", module), name + for _, prefix in RESTRICTIONS: + assert prefix not in module, prefix + + for planted in (RESTRICTED_STAGE, UNRESTRICTED_STAGE): + assert re.search(rf"\b{re.escape(planted)}\b", module + f"\n{planted}\n") + assert RESTRICTED_PREFIX in module + f"\n{RESTRICTED_PREFIX}\n" + + +def test_the_docstring_states_the_two_rules_and_what_existence_is_relative_to(): + doc = flowed(plan_validation.__doc__) + assert re.search(r"(?i)revert check", doc) + assert re.search(r"(?i)creat", doc) + assert re.search(r"(?i)target root", doc) + assert re.search(r"(?i)neither the harness root nor the process working " + r"directory|not the harness root and not the process " + r"working directory", doc) + assert re.search(r"(?i)prediction", doc) + + +# -------------------------------------------------------------------------- +# scripts/l5-plan: the root it passes, and the refusing path unchanged +# -------------------------------------------------------------------------- + + +L5_PLAN_SCRIPT = load_script("l5-plan", name="l5_plan_for_story_042") + + +def test_report_passes_the_target_root_it_was_given_to_the_check( + tmp_path: Path, monkeypatch): + """Not read off the source: the check records the root it was called with.""" + stories_dir = tmp_path / "stories" + stories_dir.mkdir() + before = plan_commit.snapshot(stories_dir) + (stories_dir / "story-900.yaml").write_text( + artifact("story-900") + plan_block((PRESENT_FILE, RESTRICTED_STAGE)), + encoding="utf-8") + _, empty = roots(tmp_path) + seen: list[Path] = [] + + original = plan_validation.artifact_problems + + def spy(artifacts, stages, root): + seen.append(root) + return original(artifacts, stages, root) + + monkeypatch.setattr(plan_validation, "artifact_problems", spy) + + L5_PLAN_SCRIPT.report(empty, stories_dir, before, STAGES) + + assert seen == [empty] + + +def test_report_prints_and_returns_on_the_refusing_path_exactly_as_today( + tmp_path: Path, capsys): + """Byte for byte: the header, the problem, the guidance, the summary line. + + The status too, which is what `main` exits with when the session itself + succeeded. Every assertion here is a positive one over produced text, so + each fails on its own the moment a character of the refusing path moves; + that the accepting root gets past validation instead is the pair above, + driven through `artifact_problems` and end to end through `l5-plan`. + """ + stories_dir = tmp_path / "stories" + stories_dir.mkdir() + before = plan_commit.snapshot(stories_dir) + path = stories_dir / "story-900.yaml" + path.write_text(artifact("story-900") + plan_block( + (PRESENT_FILE, RESTRICTED_STAGE)), encoding="utf-8") + _, empty = roots(tmp_path) + + status = L5_PLAN_SCRIPT.report(empty, stories_dir, before, STAGES) + printed = capsys.readouterr() + + (problem,) = plan_validation.assignment_problems(PRESENT, STAGES, empty) + assert status == 1 + assert printed.err == ( + f"{path} is not a valid story artifact:\n" + f" - {problem}\n" + "Fix the artifact or re-run planning before executing the story.\n" + ) + assert printed.out == ( + f"l5-plan: committed nothing; {path} remain in the working tree.\n" + ) + + +@pytest.fixture +def planning_holding(tmp_path: Path) -> Planning: + """The `planning` fixture's repository, already holding the planned file. + + The file is committed *before* the bare origin is made, so the two are + level and the base check has nothing to say — the difference between this + fixture and `planning` is the file and nothing else. + """ + made = make_planning(tmp_path) + path = made.root / PRESENT_FILE + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("# already here\n", encoding="utf-8") + made.git("add", "-A") + made.git("commit", "-q", "-m", "the file the plan names") + made.remote = bare_remote(tmp_path, made, upstream=True) + return made + + +PRESENT_ARTIFACT = artifact("story-900") + plan_block( + (PRESENT_FILE, RESTRICTED_STAGE)) + + +def test_l5_plan_commits_a_plan_naming_a_file_the_target_already_holds( + planning_holding: Planning): + """End to end, and the pair is the same artifact against two repositories. + + The next test is the control: this very artifact, this very stub, in a + target that does not hold the file — refused and uncommitted. + """ + before = planning_holding.head() + refs_before = remote_refs(planning_holding.remote) + + result = run_plan(planning_holding, L5_STUB_WRITE=writes( + (ARTIFACT_PATH, PRESENT_ARTIFACT))) + + assert result.returncode == 0, result.stdout + result.stderr + assert planning_holding.head() != before + assert remote_refs(planning_holding.remote) != refs_before + assert planning_holding.status() == "" + + +def test_l5_plan_refuses_the_same_plan_where_the_target_lacks_the_file( + planning: Planning): + """The control for the acceptance above: only the repository differs.""" + before = planning.head() + + result = run_plan(planning, L5_STUB_WRITE=writes( + (ARTIFACT_PATH, PRESENT_ARTIFACT))) + + assert result.returncode != 0 + assert planning.head() == before + assert PRESENT_FILE in result.stdout + result.stderr + assert ARTIFACT_PATH in planning.status() + + +def test_l5_plan_resolves_existence_against_the_target_root_not_its_cwd( + planning_holding: Planning): + """Run from a subdirectory, where the two answers differ. + + From `work/`, the plan's path resolved against the working directory is + not there and resolved against the target root is; the run is accepted, so + the root `find_target_root` walked up to is the one that decided. The + control is the second run in the same repository with that file removed, + which is refused — so acceptance is a property of the file being there. + """ + work = planning_holding.root / "work" + work.mkdir() + assert not (work / PRESENT_FILE).exists() + + accepted = subprocess.run( + [sys.executable, str(HARNESS_ROOT / "scripts" / "l5-plan"), "add a thing"], + cwd=work, + env=planning_holding.env(L5_STUB_WRITE=writes( + (f"../{ARTIFACT_PATH}", PRESENT_ARTIFACT))), + capture_output=True, text=True, + ) + + assert accepted.returncode == 0, accepted.stdout + accepted.stderr + + (planning_holding.root / PRESENT_FILE).unlink() + refused = subprocess.run( + [sys.executable, str(HARNESS_ROOT / "scripts" / "l5-plan"), "add a thing"], + cwd=work, + env=planning_holding.env(L5_STUB_WRITE=writes( + (f"../{ARTIFACT_PATH.replace('900', '901')}", + PRESENT_ARTIFACT.replace("story-900", "story-901")))), + capture_output=True, text=True, + ) + + assert refused.returncode != 0 + assert PRESENT_FILE in refused.stdout + refused.stderr + + +# -------------------------------------------------------------------------- +# No run reads this check +# -------------------------------------------------------------------------- + + +def test_no_run_calls_either_plan_time_function(target: Path, harness_root: Path, + monkeypatch): + """Driven, not inspected: both functions are spied on across a real run. + + The story the run executes carries exactly the entry the check reports — + an absent file beneath the restricted prefix on the restricted stage — so + a coordinator that consulted the check at pre-flight or anywhere else + would both fire a spy and refuse the run. It does neither, and the run + completes through all four stages as it does today. + """ + calls: list[str] = [] + for name in ("assignment_problems", "artifact_problems"): + original = getattr(plan_validation, name) + + def spy(*args, _name=name, _original=original, **kwargs): + calls.append(_name) + return _original(*args, **kwargs) + + monkeypatch.setattr(plan_validation, name, spy) + + append_to_story(target, plan_block((ABSENT_FILE, RESTRICTED_STAGE))) + story = story_coordinator.read_story( + (target / ".harness" / "stories" / "story-001.yaml").read_text( + encoding="utf-8")) + assert story.problems == [] + + code, runner = run(target, harness_root, {}) + + assert code == 0 + assert runner.calls == STAGE_NAMES + assert calls == [] + # Control one: the artifact this run carried is one the check does report, + # so the silence above is the coordinator's and not the artifact's. + assert plan_validation.assignment_problems(story.parsed, STAGES, target) != [] + # Control two: that call went through the spy, so the spies were wired. + assert calls == ["assignment_problems"] diff --git a/tests/test_plan_time_validation.py b/tests/test_plan_time_validation.py index 004fd83..0ac398c 100644 --- a/tests/test_plan_time_validation.py +++ b/tests/test_plan_time_validation.py @@ -274,7 +274,7 @@ def recording(text, harness_root=None): path = tmp_path / "story-900.yaml" path.write_text(DEFECTS["schema"], encoding="utf-8") - problems = plan_validation.artifact_problems([path], STAGES) + problems = plan_validation.artifact_problems([path], STAGES, HARNESS_ROOT) assert calls == [DEFECTS["schema"]] assert problems[path] == real(DEFECTS["schema"]).problems @@ -292,13 +292,13 @@ def test_an_artifact_read_story_rejects_is_not_carried_into_the_later_checks( path = tmp_path / "story-900.yaml" path.write_text(DEFECTS["unparseable"], encoding="utf-8") - assert plan_validation.artifact_problems([path], STAGES)[path] + assert plan_validation.artifact_problems([path], STAGES, HARNESS_ROOT)[path] assert seen == [] # Control: a well-formed artifact does reach both, so the emptiness above # is the guard rather than the recorder never being wired up. path.write_text(artifact(), encoding="utf-8") - assert plan_validation.artifact_problems([path], STAGES) == {} + assert plan_validation.artifact_problems([path], STAGES, HARNESS_ROOT) == {} assert seen == ["exceptions", "strictness"] @@ -307,10 +307,10 @@ def test_artifact_problems_holds_only_the_artifacts_with_problems(tmp_path: Path good.write_text(artifact("story-900"), encoding="utf-8") bad.write_text(DEFECTS["schema"], encoding="utf-8") - problems = plan_validation.artifact_problems([good, bad], STAGES) + problems = plan_validation.artifact_problems([good, bad], STAGES, HARNESS_ROOT) assert list(problems) == [bad] - assert plan_validation.artifact_problems([good], STAGES) == {} + assert plan_validation.artifact_problems([good], STAGES, HARNESS_ROOT) == {} #: A second reader: any route to a parse or a schema validation that does not diff --git a/tests/test_validation_module_naming.py b/tests/test_validation_module_naming.py index 116d71c..8e463fe 100644 --- a/tests/test_validation_module_naming.py +++ b/tests/test_validation_module_naming.py @@ -589,7 +589,7 @@ def test_both_artifacts_this_file_uses_are_what_they_claim_to_be(): reading = story_coordinator.read_story(text) assert reading.problems == [], (name, reading.problems) assert plan_validation.strictness_problems(reading.parsed, STAGES) == [], name - assert plan_validation.assignment_problems(reading.parsed, STAGES) == [], name + assert plan_validation.assignment_problems(reading.parsed, STAGES, REPO_ROOT) == [], name def test_the_check_reports_the_offending_entry_and_only_it(): @@ -633,7 +633,7 @@ def test_the_control_for_every_incomplete_story_above_is_the_complete_one(): def test_artifact_problems_reports_the_new_class(tmp_path: Path): path = tmp_path / "story-900.yaml" path.write_text(OFFENDING_ARTIFACT, encoding="utf-8") - found = plan_validation.artifact_problems([path], STAGES) + found = plan_validation.artifact_problems([path], STAGES, REPO_ROOT) assert list(found) == [path] assert any(OFFENDING in problem for problem in found[path]) @@ -641,7 +641,7 @@ def test_artifact_problems_reports_the_new_class(tmp_path: Path): def test_artifact_problems_holds_the_well_named_artifact_back(tmp_path: Path): path = tmp_path / "story-901.yaml" path.write_text(WELL_NAMED_ARTIFACT, encoding="utf-8") - assert plan_validation.artifact_problems([path], STAGES) == {} + assert plan_validation.artifact_problems([path], STAGES, REPO_ROOT) == {} def test_a_story_that_fails_the_gate_yields_that_and_nothing_further( @@ -653,7 +653,7 @@ def test_a_story_that_fails_the_gate_yields_that_and_nothing_further( """ unparseable = tmp_path / "story-902.yaml" unparseable.write_text("this: is: not: a story\n\t- ?\n", encoding="utf-8") - found = plan_validation.artifact_problems([unparseable], STAGES) + found = plan_validation.artifact_problems([unparseable], STAGES, REPO_ROOT) assert found[unparseable] assert not any(OFFENDING in problem for problem in found[unparseable]) @@ -661,7 +661,7 @@ def test_a_story_that_fails_the_gate_yields_that_and_nothing_further( reached.write_text(OFFENDING_ARTIFACT, encoding="utf-8") assert any(OFFENDING in problem for problem in plan_validation.artifact_problems( - [reached], STAGES)[reached]) + [reached], STAGES, REPO_ROOT)[reached]) def corpus() -> dict[str, dict]: