diff --git a/.harness/docs/ARCHITECTURE.md b/.harness/docs/ARCHITECTURE.md index 9b922cb..afaa677 100644 --- a/.harness/docs/ARCHITECTURE.md +++ b/.harness/docs/ARCHITECTURE.md @@ -20,15 +20,15 @@ l5 is a level 3 agentic harness: a story execution system. The workflow defines ### Workflow definitions (`workflows/`) -`story-workflow.json` defines the execution structure: stage list (implementer, tester, verifier, documenter), the prompt and expected artifacts for each stage, the retry routing table (which category of defect returns execution to which stage), and the escalation rule (retries exhausted → escalate). It carries no retry ceiling: since story-028 `max_retries` lives only in `rules/execution-rules.json` — see "Where a retry goes" below. +`story-workflow.json` defines the execution structure: stage list (implementer, tester, documenter, verifier — the documenter moved ahead of the verifier in story-045, see "Where a retry goes" below), the prompt and expected artifacts for each stage, the retry routing table (which category of defect returns execution to which stage), and the escalation rule (retries exhausted → escalate). It carries no retry ceiling: since story-028 `max_retries` lives only in `rules/execution-rules.json` — see "Where a retry goes" below. -A stage that writes to the repository declares an optional `changed_files` key naming its changed-files record: the implementer declares `changed-files.json`, the tester declares `tester-changed-files.json`, and since story-044 the documenter declares `documenter-changed-files.json`. After any stage with this declaration completes, the coordinator checks that record against `blocked_paths` and escalates on violation — enforcement is driven by the workflow definition, with no stage names hard-coded in the coordinator. Adding the documenter proved that: the diff that enabled the check touched no file under `orchestration/`, only the stage's `outputs`, `changed_files` and `schemas` entries in the workflow and the prompt paragraph asking for the record. The cost landed instead in the fixtures — the record became a required artifact, so every fake runner in `tests/` that drives the shipped workflow through the documenter had to start writing it. The declaration turns on the required-artifact check and the schema check along with the blocked-path check; it does not bring `may_not_create`, a revert check or a stage baseline, which remain separate declarations the documenter does not carry. All three records share one schema definition (`modified`/`created`/`deleted` arrays), not three copies. The documenter's record is written outward and read by nothing: no stage runs after it, so unlike the implementer's record — injected into later templates through `{{changed_files}}` — it is checked and then only committed. +A stage that writes to the repository declares an optional `changed_files` key naming its changed-files record: the implementer declares `changed-files.json`, the tester declares `tester-changed-files.json`, and since story-044 the documenter declares `documenter-changed-files.json`. After any stage with this declaration completes, the coordinator checks that record against `blocked_paths` and escalates on violation — enforcement is driven by the workflow definition, with no stage names hard-coded in the coordinator. Adding the documenter proved that: the diff that enabled the check touched no file under `orchestration/`, only the stage's `outputs`, `changed_files` and `schemas` entries in the workflow and the prompt paragraph asking for the record. The cost landed instead in the fixtures — the record became a required artifact, so every fake runner in `tests/` that drives the shipped workflow through the documenter had to start writing it. The declaration turns on the required-artifact check and the schema check along with the blocked-path check; it does not bring `may_not_create`, a revert check or a stage baseline, which remain separate declarations the documenter does not carry. All three records share one schema definition (`modified`/`created`/`deleted` arrays), not three copies. The documenter's record was written outward and read by nothing while the documenter ran last; since story-045 the verifier runs after it and receives it through `{{documenter_changed_files}}`, exactly as it receives the implementer's through `{{changed_files}}`, so all three records are now both checked and read. A stage may also declare an optional `may_not_create` key, a list of repository-relative path prefixes it is not allowed to add files under. The implementer declares `["tests/"]`; no other stage does. After a stage that declares both `changed_files` and `may_not_create`, the coordinator reads that stage's own record and escalates when any entry in its **`created`** array falls under a declared prefix. `modified` and `deleted` are not examined by *this* check — the rule is about independence, not about directories: an implementer must be able to update an existing test whose call site its own signature change broke, but validation it authors itself checks what it built rather than what was asked. Since story-017 those two arrays are decided by the revert check below rather than left unexamined. As with blocked paths, no stage name and no prefix appears in orchestration code; both are read off the stage dict. A stage may also declare an optional `revert_check` key, an object naming both artifacts that check needs: `result`, the record it writes, and `baseline`, the run-directory directory its pre-stage content is captured into. The implementer declares `{"result": "revert-check-result.json", "baseline": "stage-baseline"}`. One key still turns the whole thing on, exactly as `clean_clone` does on the verifier: the coordinator reads `stage.get("revert_check")` and does nothing when it is absent, so removing the declaration disables the capture *and* the check with no change to orchestration code and neither name appears there. It was a bare string until story-019 needed a second name and widened it rather than adding a second key — one declaration, so the capture and the check cannot be switched on independently and disagree. The check runs immediately after the ownership check, inside the same `changed_files` block, on the same record, the **same enforced prefix list** and the **same exemption**. Since story-032 that list is the stage's `may_not_create` declarations *unshortened*, and the story's grants are carried beside it as a set of exempt paths rather than subtracted from it; both checks skip a path `grant_covers` covers. A story holding a `stage_exception` is therefore not subject to the revert check on the granted path either, from reuse rather than from a second subtraction — the property is unchanged, only its mechanism moved. See "Work a stage cannot own" below. -The verifier stage declares an optional `clean_clone` key, an object naming both things that check needs: `result`, the artifact it writes (`clean-clone-result.json`), and `retry_stage`, the stage a clean-clone failure routes to. The key is what turns the check on: the coordinator reads `stage.get("clean_clone")` and does nothing when it is absent, so removing the declaration disables the check with no change to orchestration code, and neither the artifact name nor the destination appears there. It was a bare string until story-028 needed a second name and widened it rather than adding a second key, exactly as `revert_check` was widened in story-019 — one declaration, so the check and its route cannot be switched on independently and disagree. Its route is not one of the verifier's retry categories: nothing *chooses* it, so it carries no `when` and is never rendered into a prompt, but it is held to the same pre-flight as the categories are. It sits on the verifier because the check runs on the verifier's passing verdict, in that stage's branch, before routing. +The verifier stage declares an optional `clean_clone` key, an object naming both things that check needs: `result`, the artifact it writes (`clean-clone-result.json`), and `retry_stage`, the stage a clean-clone failure routes to. The key is what turns the check on: the coordinator reads `stage.get("clean_clone")` and does nothing when it is absent, so removing the declaration disables the check with no change to orchestration code, and neither the artifact name nor the destination appears there. It was a bare string until story-028 needed a second name and widened it rather than adding a second key, exactly as `revert_check` was widened in story-019 — one declaration, so the check and its route cannot be switched on independently and disagree. Its route is not one of the verifier's retry categories: nothing *chooses* it, so it carries no `when` and is never rendered into a prompt, but it is held to the same pre-flight as the categories are. It sits on the verifier because the check runs on the verifier's passing verdict, in that stage's branch, before routing. Since story-045 the verifier is the *last* stage, so the tree the check clones already holds the documenter's edits — the declaration, the mechanics and the recorded result are unchanged, and only what is in the tree when it runs changed, as a consequence of the reorder rather than of an edit to the check. A stage may also declare an optional `max_self_routes` key, a non-negative integer bounding how many times *in a row* that stage may run again in place after failing mechanically. The implementer declares `1`; every other stage declares none, and a stage that declares none escalates on a mechanical failure exactly as it did before the key existed. The budget comes off the loaded stage dict, exactly as `may_not_create` and `clean_clone` do — no stage name and no budget value appears in orchestration code — and it is held to a pre-flight check (`self_route_problems`) like the routing table is. See "A stage that failed mechanically runs again" below for what it bounds and why it is a second budget rather than a second reading of `max_retries`. @@ -84,6 +84,8 @@ A prompt states a boundary; it does not hold it. `implementer.md`'s do-not list `verifier.md` takes the same treatment for the routing categories it must choose between: `{{retry_routes}}` injects one line per declared category — the category, its destination stage and its `when` description — beside the verification-result schema, and the surrounding prose says only that a recommended retry must name one of them in `retry_target` and that the coordinator escalates on a missing or unknown one. The template **names no category**, so adding one to `workflows/story-workflow.json` changes the rendered prompt with no edit to any prompt file. `tester.md` gained `{{retry_guidance}}` and `{{retry_state}}` in the same story: the tester can now be a retry destination, and until then it had neither, because it could only ever be arrived at going forward. +story-045 is the first real test of both halves of that. `documenter.md` gained `{{retry_guidance}}` for exactly the reason `tester.md` did — becoming a retry destination brought it under the standing rule that every stage a route can name must be able to say it is on a retry — and the third category reached the verifier's prompt with **no edit to `verifier.md`'s routing prose**, which is the property story-028 landed and nothing had exercised until a third route existed. What `verifier.md` did gain is the new subject rather than the new category: two runtime-state placeholders, `{{documenter_changed_files}}` and `{{documentation_report}}`, and one responsibility line saying the documenter's output is part of what the verifier evaluates. The template still names no category, no destination and no `when`. + Templates carry no inline JSON artifact bodies. A stage that must produce a structured artifact injects its schema — `implementer.md` uses `{{changed_files_schema}}`; `tester.md` uses `{{test_results_schema}}` and `{{changed_files_schema}}`; `verifier.md` uses `{{verification_result_schema}}` and `{{retry_guidance_schema}}` — keeping only the surrounding sentence that names the file and says what it is for. Adding a field to an artifact is therefore a one-file edit in `schemas/`. `planner.md` follows the same rule for the artifact it *asks for* rather than produces: it injects `{{story_schema}}` and states no required section and no required field of its own. What survives around the injection is deliberate and of two kinds. The skeleton stays, labeled an illustration rather than the contract, because the planner writes the story dialect and a shape teaches indentation, block scalars, and dash-prefixed items in a way a schema cannot; it names no field absent from `schemas/story.schema.json`. The `stage_exceptions` ask-first instruction also stays — "do not add one without asking the developer first" is planner role guidance, not schema content, and removing it would be an over-application of the injection rule. story-007 is why this matters: it changed the story contract twice, `planner.md` was in that story's `do_not_modify` list, and between the merge and the follow-up patches the planner wrote stories `l5-run` refused at pre-flight. @@ -95,7 +97,7 @@ The drift source that paragraph used to name is closed: `planner.md` no longer s - `story_coordinator.py` — the Story Coordinator. Loads the workflow definition, story artifact, and rules; creates the story branch and run directory *or resumes an existing one*; loops: determine stage → assemble context → render prompt → invoke agent → save artifacts → update state → route (advance, retry, or escalate). `run_story` takes an optional keyword-only `start_stage` overriding where execution enters — the recorded stage on a resume, `stage_names[0]` on a fresh run. It is named `start_stage` rather than `stage` because `stage` is the loop's name for the stage being executed. A `start_stage` the loaded workflow does not define is refused above everything else, in the same shape as the other pre-flight refusals: exit 1, one message naming the stages the workflow does define, nothing created and no agent invoked. See "Resuming a run" below for the resume branch, the escalation commit and the guard. Post-stage checks run in a fixed order: required artifacts present → required artifacts written by *this* attempt → declared artifacts match their schemas → changed-files record clear of blocked paths → stage output ownership → the revert check. The freshness check sits immediately after the presence check and never before it, so an artifact that is genuinely absent keeps its own missing-artifacts reason and the two stay distinguishable; see "An artifact's presence is not evidence of its authorship" below. Those first two, together with a failed agent process above them, are the three **mechanical** failures, and since story-036 each is routed through `self_route` rather than escalating outright: the stage runs again in place while it has unspent budget, and escalates with the same reason it always used when it has none. The checks after them — schema violations, blocked paths, ownership, the revert check — are unchanged and still escalate. See "A stage that failed mechanically runs again" below. Schema validation sits in the middle deliberately, so a malformed `changed-files.json` escalates with a validation error naming the field rather than raising out of the blocked-paths check that reads the same file. Ownership runs last, on the same record, after that record is known to be well-formed and clear of blocked paths. `_ownership_violation` returns a frozen `OwnershipViolation(path, prefix)`, and the escalation reason names stage, path, and prefix in both `events.log` and `escalation-summary.md`. `granted_paths(story, stage_name)` reads the story's grants for that stage and `grant_covers(granted, path)` decides whether any of them covers a path; the enforced list is passed whole and the grants are handed alongside it as an exemption, so `_ownership_violation(run_dir, record_name, prefixes, granted)` skips a covered path rather than the prefix being removed. Each grant is appended to `events.log` as `stage exception applied: may create `, one per grant whatever its granularity, so routing stays reconstructable from the log alone. The revert check sits immediately after, in the same block, reusing that same `enforced` list *and* that same exemption rather than recomputing either: `governed_edits(run_dir, record_name, prefixes, granted)` returns a frozen `GovernedEdits(paths, prefixes)` holding the sorted `modified` and `deleted` entries under any of the prefixes and covered by no grant, plus the prefixes that matched, and it names no stage, no prefix and no artifact. When `paths` is empty nothing at all happens — no clone, no suite, no artifact — because a check that can say nothing should not run. Otherwise `revert_check(run_dir, target_root, config, artifact, paths, baseline)` is shaped exactly like `clean_clone_check`: `tempfile.mkdtemp` scratch, the shared `run_clean_clone` with `revert` set to the governed paths, `shutil.rmtree` in a `finally` whatever the result, and `RevertCheckResult.as_record()` written under the declared artifact name. `permitted` is `result.exit_code != 0`. Two conditions stop it from running and neither permits: a stage that declares the check with no baseline captured (decided *before* any clone is attempted, so the reason names the missing directory rather than surfacing as a generic clone failure) and a clone that cannot be built. A check that did not run escalates naming the reason; `permitted` false escalates naming the stage, the prefixes and the paths; `permitted` true appends `_revert_check_permitted` and falls through to the existing advance. Both escalations go through `_escalate`, which does not touch `retry_count`. Since story-020 `_escalate` takes `target_root` and `harness_root` as required keywords — it commits and it records the harness revision — so every escalation site forwards both; a new escalation that forgets them is a `TypeError` rather than a run that quietly leaves its work uncommitted. `_build_clone` and `run_clean_clone` gained a `revert` parameter for this and, in story-019, the `baseline` the reverted content is restored *from*; both default to reverting nothing, so the clean-clone check is untouched. When `revert` is non-empty the restore runs inside the clone *after* the working-tree diff is applied and the untracked files are copied and *before* `git add -A`, so the clone commits those paths as the stage found them while every other change is present: the baseline's copy is copied over the clone's for each governed path it holds, and each governed path it does **not** hold is deleted in the clone, because a path absent from the baseline did not exist when the stage started. Deleting rather than skipping is the point — skipping decides nothing and would report a permission the check never established, which is the assertion-that-cannot-fail failure mode `tests/test_baseline_honesty.py` exists to prevent. Naming paths with no baseline raises `RuntimeError` naming them. No code path in the check reverts to HEAD any longer. Pre-flight story reading is one function, `read_story(story_text)`: load `story.schema.json`, parse with it, validate against it, and return a frozen `StoryReading` carrying both the `parsed` story (`None` when parsing failed) and the `problems` list. It runs above the run-directory creation and the branch checkout, so a rejection is an exit-1 refusal leaving no run directory, no `state.json`, no log, and no new branch — and no agent invoked. It is called exactly once per run, and the parse it returns is the run's only reading of the artifact: `reading.parsed` is threaded into every `build_context` call and into `_complete`, which takes the completion-report title and commit-message subject from `story["story"]["title"]` rather than scanning lines. A missing title is a loud `KeyError`; the schema marks it required and the run cannot reach `_complete` without having validated. `read_story` stays schema conformance only. Whether a story's `stage_exceptions` mean anything against the workflow *this run loaded* is a separate question the schema cannot answer, so it is a separate function — `stage_exception_problems(story, stages)`, called from `run_story` beside `read_story` and above the run-directory creation. It refuses an exception naming a stage the workflow does not define, and one granting a value beneath no prefix that stage was restricted on: an exception that grants nothing is a planning error, not a harmless one. Matching was exact equality against the declared prefixes until story-032 widened it to *containment* — a grant may name the whole prefix, a directory beneath it, or a single file beneath it — while both refusal messages read as they always did, so `create: tests` against a declared `tests/` still refuses rather than silently granting part of it. The stage → restricted-prefix mapping it needs is derived by `stage_restrictions(stages) -> list[tuple[str, str]]`, sitting immediately above it, which returns the workflow's (stage, prefix) create-restriction pairs in declared order; `stage_exception_problems` builds its per-stage mapping from that helper (taking the stage-name set from `stages` directly, so a stage declaring no `may_not_create` is still known to exist and the "names a stage the workflow does not define" branch is unaffected). The derivation exists once because story-025's plan-time strictness check needs the same pairs, and a second copy would be a second answer to "what does the workflow restrict". A third pre-flight joins them since story-021, the clean-tree check: `dirty_paths(target_root)` runs one `git status --porcelain` and returns the sorted, deduplicated paths, stripping the status codes, reducing a rename's `old -> new` to the new path, and unquoting a quoted path. It only *reads* the target — no commit, no branch, no stash, no index change — and a root that is not a git repository, or a git that fails for any other reason, reports nothing dirty, the same one-directional bias `unchanged_since_escalation` takes. Untracked files count, because a file no stage produced is exactly what `git add -A` would absorb; ignored files do not, which is why a gitignored `.harness/runs/` is not what the check is about. See "The tree a run starts from" below for which runs it applies to and why. A fourth joins them since story-027, the finished-branch check: `story_branch(config, story_id)` derives the branch name from the configured prefix in one place, and `completion_commits(target_root, branch, story_id)` returns one `" "` line per commit reachable from that branch that a finished run of this story made. It sits **above** the clean-tree check, so a developer whose tree is also dirty meets the refusal that makes the run pointless before the one that makes it unaccountable, rather than in two round trips. See "The branch a run starts from" below for the evidence it uses and why. A fifth joins them since story-030, the base check: `resolve_base(target_root, config, base)` settles what a story branch is cut from, and `base_problems(target_root, base, declared)` returns what refuses a run that would cut it from somewhere else. It sits between the finished-branch check and the clean-tree check, gated on the story branch not already existing, and `branch_behind(target_root, branch, base)` feeds the note an *existing* branch gets instead of a refusal. Both entry points read those two functions — see "The base a story branches from" below. A sixth joins them since story-028, and it is the only one that asks about the *definition* rather than about the target repository: `retry_routing_problems(stages)` sits beside `stage_exception_problems` and refuses a declared route whose destination the workflow does not define, and one whose destination does not sit strictly before the stage declaring it — see "Where a retry goes" below. A seventh joins them since story-036 and asks the same kind of question about the definition: `self_route_problems(stages)` sits beside `retry_routing_problems` and refuses a stage whose declared `max_self_routes` is not a non-negative integer — a budget that is not a count cannot be spent, and every run under that definition would meet it. All seven refusals print through one extracted `refuse(header, problems, guidance)`, so the refusal shape (exit 1, one message per problem, nothing created) is a single code path rather than a copy per reason; what differs between them is the sentence above the list and the sentence below it, which is what the six thin callers — `refuse_bad_story`, `_refuse_dirty_tree`, `_refuse_finished_branch`, `_refuse_base`, `_refuse_bad_routing` and `_refuse_bad_self_routes` — supply. `refuse` and `refuse_bad_story` are **public** since story-025, because plan time and pre-flight must print a given defect identically: `l5-plan` reports a failing artifact through `refuse_bad_story`, so the same defect in the same artifact produces the same text whether it is caught when the artifact is written or when it is run, from one function rather than from two that agree today. `_refuse_dirty_tree` stays private — it is a run-time refusal with no plan-time counterpart — and a helper is promoted when a second caller outside the coordinator actually needs it, not on principle. `_refuse_base` is the one place that rule is currently bent and it is recorded rather than glossed: it *does* have a caller outside the coordinator — `l5-plan` prints its base refusal through it, which is what makes the two entry points unable to word the same defect differently — and it kept its underscore. The decision functions the script reads, `resolve_base` and `base_problems`, are public; promote `_refuse_base` to match them in the story that has another reason to touch it. `load_state(run_dir)` moved above the run-directory creation for this, since the clean-tree check decides from the state the run is starting from and `load_state` reads a file rather than requiring a directory; nothing else about the resume decision moved with it. The retry branch archives before it increments: `archive_attempt(run_dir, archivable_artifacts(stages), state.retry_count + 1)` copies the superseded attempt's artifacts under `attempts/attempt-N/` — see the archive decisions below. `append_event(run_dir, message, *, kind, stage, artifacts, duration_seconds, verifier_outcome, retry_decision, retry_reason, retry_category, retry_stage)` is the run's single event write path: the prose message stays positional and is what the `events.log` line is built from, and the *same call* appends one structured entry to `execution-history.json`. `load_history(run_dir)` is the read side, called only by `append_event` for the next sequence number. `run_story` captures `stage_started_at = time.monotonic()` at the stage-started event and reads it through a local `elapsed()` at every event that ends a stage, so a completed stage's entry carries a duration the log only made derivable; `_escalate` forwards whatever structured fields an escalation has and tags its entry `escalated`. The clean-clone check is the last thing the verifier branch does on a passing verdict: `clean_clone_check(run_dir, target_root, config, artifact)` builds a scratch clone with `tempfile.mkdtemp`, runs `run_clean_clone`, removes the scratch directory in a `finally` whatever the result, and writes the returned `CleanCloneResult.as_record()` to the run directory under the declared artifact name. `_build_clone` does `git clone --no-local` from the target's filesystem path — over git's normal transport, see "The clone is built over the normal transport" below — applies the target's tracked edits as `git diff --binary HEAD` piped to `git apply`, copies the untracked-but-not-ignored files from `git ls-files --others --exclude-standard`, then commits inside the clone; the target repository is only read. `_link_interpreter_roots` links the top-level directory of each configured interpreter path into the clone and appends those names to the clone's `.git/info/exclude`, because a virtualenv is gitignored and therefore absent from a fresh clone, and a `.gitignore` entry for a directory does not cover a symlink standing in its place. Zero exit appends `_clean_clone_passed` and falls through to the existing advance; non-zero takes the retry path the verification-failed branch already takes — `archive_attempt` above the increment, then increment, save, `_clean_clone_failed`, and `index = stage_names.index(destination)` — or the existing escalation path at the ceiling, with `_clean_clone_failures` collapsing the output's `FAILED` lines into the one-line reason. Since story-028 the destination on that path is `clean_clone["retry_stage"]`, read off the widened declaration, rather than borrowed from the verifier's own table. Both events are module-level helpers rather than inline calls, for a reason worth keeping: `tests/test_execution_history.py` proves its own non-vacuity by deleting the first `retry_decision="retry",` line at the verification-failed branch's indentation, and an inline clean-clone branch nests deeper and sits earlier in the file, so its line *contains* that indented text and the mutation lands there instead of where it was aimed. - `story_parser.py` — lexer plus schema-directed interpreter for the story artifact. **The story dialect is not YAML**; see the module docstring before reaching for `yaml.safe_load`, which reads committed artifacts differently and wrongly. The lexer produces line/indent/content records, drops blank lines and full-line comments, consumes a `key: |` block scalar body whole (so blank and `#`-shaped lines *inside* it survive), and rejects tab indentation. The interpreter dispatches on the schema node's `type`, consulting structure only where the schema is silent. Under `items.type == "string"` a `- ` item is the verbatim remainder of its line, colons included; under `items.type == "object"` the same syntax parses into key/value pairs. Scalars are never coerced — every value is a `str`. A single `StoryParseError` carries line, expectation, and finding, rendering as `line 12: expected …, found …`. - `schema_validator.py` — `schemas_dir`, `load_schema`, `shipped_schemas`, `unsupported_keywords`, and `validate(instance, schema) -> list[str]`. `shipped_schemas(harness_root=None) -> tuple[str, ...]` reads `schemas/manifest.json` through `schemas_dir`, so the override behaves identically to `load_schema`'s, and raises `ValueError` on anything short of a well-formed non-empty list of strings. A deliberately small JSON Schema subset — `type`, `required`, `properties`, `items`, `enum` — because the harness is standard library only. `validate` walks the whole schema first and raises `ValueError` if any keyword outside that subset appears anywhere in it, so a schema can never claim a constraint the validator silently drops. Errors carry a tracked JSON path, the expectation, and the found value: `$.blocking_issues[0].severity: expected one of ["high", "medium", "low"], found string ("critical")`. -- `context_assembler.py` — builds each stage's runtime context from the story artifact, prior stage artifacts, retry state, and architecture documents, and renders it into the prompt template. `build_context` takes the raw `story_text`, the required keyword-only `story` (the parsed artifact from `read_story`), and — since story-028 — the required keyword-only `workflow`, whose `workflow_context` it merges into the assembled context; it never reads either artifact itself. The workflow was the one thing every stage's context was built without, which is why the verifier could not be told what the coordinator routes on. Making it required rather than optional was deliberate: a default would let a call site silently render a verifier prompt with no categories in it. `{{story}}` is `story_text` verbatim, and `{{acceptance_criteria}}` comes from the parsed list via `_dashed_lines`, which renders one `- `-prefixed criterion per line and returns `None` for an absent or empty list. `{{stage_exceptions}}` follows the same convention through `_exception_lines`: one dash-prefixed line per grant naming the stage, the granted path, and the reason, `None` when the story declares none. `render()` is single-pass: `re.sub` does not re-scan substituted text, so a placeholder injected by one substitution is not itself resolved. `build_context` therefore resolves the shared `prompts/harness-layer.md` partial as a **two-pass render** — it renders that partial (including the partial's own `{{blocked_paths}}` placeholder) against the assembled context first, then stores the already-resolved text as the `harness_layer` context value for injection into stage templates. When the partial is absent, `harness_layer` is left unset and renders as `None`. The schema placeholders come from `schema_context(harness_root) -> dict[str, str]`, a public function of the same module: it globs `harness_root/schemas/*.schema.json` and exposes each file's text under the stem with hyphens replaced by underscores plus `_schema` (`verification-result.schema.json` → `{{verification_result_schema}}`). `build_context` merges it with `update` at the point the inline loop used to run, before the two-pass render, so the values are available to any template. A new schema file becomes an injectable placeholder with no code change. The glob appears exactly once in the module because it has two callers: `build_context` for workflow stages, and `l5-plan` for the planner template, which no coordinator renders. `workflow_context(workflow, rules) -> dict[str, str | None]` sits beside `schema_context` for the same reason: it maps the loaded workflow's stage names to `{{workflow_stages}}`, each stage's `may_not_create` declarations to `{{stage_create_restrictions}}` (`" may not create files under "`, one line per pair), the rules' `blocked_paths` to `{{blocked_paths}}`, and the workflow's declared retry routes to `{{retry_routes}}` (`" -> : "`, one line per declared category), all through the shared `_dashed_lines` helper — `build_context`'s own `blocked_paths` rendering goes through the same helper, so the harness-layer partial and the planner render the list identically. `_dashed_lines` returns `None` for an empty list, and `render()` maps `None` to the literal `None`, so the empty-list edge changes no rendered prompt. `retry_routes(stages) -> list[RetryRoute]` sits above it as the single derivation of the workflow's `(declared_by, category, stage, when)` triples, in the same spirit as `stage_restrictions`. It lives in this module rather than in the coordinator because the coordinator imports this module and not the reverse, and its two readers — the coordinator's pre-flight check on the table and the rendering just above — therefore read one answer to "what does this workflow route". The rendering exists once: there is no second function turning workflow routes into prompt text. `config_context(config) -> dict[str, str | None]` joins the pair since story-035, mapping the target config's `allowed_tools` to `{{allowed_tools}}` through the same `_dashed_lines` helper, and `build_context` merges it beside `workflow_context` from a new **optional** keyword-only `allowed_tools` argument — optional where `workflow` is required, deliberately, because a call that omits it must render exactly what it rendered before the argument existed. The coordinator passes `config.get("allowed_tools")` at its one call site. See "Tool allowlist" above for why the grants are injected rather than restated in prose. `self_route_result` joins it since story-036 on identical terms — optional, keyword-only, defaulting to `None`, exposed under a `self_route_result` key and rendering as `None` when nothing applies — so a call that omits it renders exactly what it rendered before the argument existed. The coordinator reads the value back off the evidence artifact it has just written rather than passing it along in memory, so what the prompt says and what the run directory records are one thing. +- `context_assembler.py` — builds each stage's runtime context from the story artifact, prior stage artifacts, retry state, and architecture documents, and renders it into the prompt template. `build_context` takes the raw `story_text`, the required keyword-only `story` (the parsed artifact from `read_story`), and — since story-028 — the required keyword-only `workflow`, whose `workflow_context` it merges into the assembled context; it never reads either artifact itself. The workflow was the one thing every stage's context was built without, which is why the verifier could not be told what the coordinator routes on. Making it required rather than optional was deliberate: a default would let a call site silently render a verifier prompt with no categories in it. `{{story}}` is `story_text` verbatim, and `{{acceptance_criteria}}` comes from the parsed list via `_dashed_lines`, which renders one `- `-prefixed criterion per line and returns `None` for an absent or empty list. `{{stage_exceptions}}` follows the same convention through `_exception_lines`: one dash-prefixed line per grant naming the stage, the granted path, and the reason, `None` when the story declares none. `render()` is single-pass: `re.sub` does not re-scan substituted text, so a placeholder injected by one substitution is not itself resolved. `build_context` therefore resolves the shared `prompts/harness-layer.md` partial as a **two-pass render** — it renders that partial (including the partial's own `{{blocked_paths}}` placeholder) against the assembled context first, then stores the already-resolved text as the `harness_layer` context value for injection into stage templates. When the partial is absent, `harness_layer` is left unset and renders as `None`. The schema placeholders come from `schema_context(harness_root) -> dict[str, str]`, a public function of the same module: it globs `harness_root/schemas/*.schema.json` and exposes each file's text under the stem with hyphens replaced by underscores plus `_schema` (`verification-result.schema.json` → `{{verification_result_schema}}`). `build_context` merges it with `update` at the point the inline loop used to run, before the two-pass render, so the values are available to any template. A new schema file becomes an injectable placeholder with no code change. The glob appears exactly once in the module because it has two callers: `build_context` for workflow stages, and `l5-plan` for the planner template, which no coordinator renders. `workflow_context(workflow, rules) -> dict[str, str | None]` sits beside `schema_context` for the same reason: it maps the loaded workflow's stage names to `{{workflow_stages}}`, each stage's `may_not_create` declarations to `{{stage_create_restrictions}}` (`" may not create files under "`, one line per pair), the rules' `blocked_paths` to `{{blocked_paths}}`, and the workflow's declared retry routes to `{{retry_routes}}` (`" -> : "`, one line per declared category), all through the shared `_dashed_lines` helper — `build_context`'s own `blocked_paths` rendering goes through the same helper, so the harness-layer partial and the planner render the list identically. `_dashed_lines` returns `None` for an empty list, and `render()` maps `None` to the literal `None`, so the empty-list edge changes no rendered prompt. `retry_routes(stages) -> list[RetryRoute]` sits above it as the single derivation of the workflow's `(declared_by, category, stage, when)` triples, in the same spirit as `stage_restrictions`. It lives in this module rather than in the coordinator because the coordinator imports this module and not the reverse, and its two readers — the coordinator's pre-flight check on the table and the rendering just above — therefore read one answer to "what does this workflow route". The rendering exists once: there is no second function turning workflow routes into prompt text. `config_context(config) -> dict[str, str | None]` joins the pair since story-035, mapping the target config's `allowed_tools` to `{{allowed_tools}}` through the same `_dashed_lines` helper, and `build_context` merges it beside `workflow_context` from a new **optional** keyword-only `allowed_tools` argument — optional where `workflow` is required, deliberately, because a call that omits it must render exactly what it rendered before the argument existed. The coordinator passes `config.get("allowed_tools")` at its one call site. See "Tool allowlist" above for why the grants are injected rather than restated in prose. Since story-045 the context dict also carries `documentation_report` (from `documentation-report.md`) and `documenter_changed_files` (from `documenter-changed-files.json`), read through the same `_read` helper as the artifacts beside them, so an absent artifact renders as `None` rather than failing — which is what a stage running before the documenter, and this repository's own story-045 run, get. `self_route_result` joins it since story-036 on identical terms — optional, keyword-only, defaulting to `None`, exposed under a `self_route_result` key and rendering as `None` when nothing applies — so a call that omits it renders exactly what it rendered before the argument existed. The coordinator reads the value back off the evidence artifact it has just written rather than passing it along in memory, so what the prompt says and what the run directory records are one thing. - `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. @@ -155,13 +157,11 @@ Outliving the session is what made **plan-time validation** possible, and story- ↓ l5-run → Story Coordinator ↓ - implement → test → verify ──fail──→ retry the stage the reported - ↓ pass ↑ category routes to (bounded) - │ │ ↓ retries exhausted, or a - clean-clone check ──fail─────────┘ verdict that cannot be routed - ↓ pass escalated (escalation-summary.md) - ↓ pass - document + implement → test → document → verify ──fail──→ retry the stage the reported + ↓ pass ↑ category routes to (bounded) + │ │ ↓ retries exhausted, or a + clean-clone check ──fail────────────────┘ verdict that cannot be routed + ↓ pass escalated (escalation-summary.md) ↓ completed (completion-report.md) @@ -342,7 +342,9 @@ Until story-028 every failed verification returned to the implementer, whatever **story-011 is the observed cost, and it was paid twice.** Its verification failed on a defect wholly inside a test file the *tester* had created, and the verifier scoped the retry to exactly that one file. The coordinator routed to the implementer anyway: the implementer was given the wrong prompt for the job, and the stage that owned the artifact never got to repair its own output. story-011 is cited as evidence here and was neither re-run nor re-judged. -**The verifier names a category; the workflow says where a category goes.** The verifier's `on_failure` is now a `retry_routing` table, each entry carrying a destination `stage` and a `when` description of the defect it covers. The shipped table defines two: `implementation` → implementer, for a defect in the code under test, and `validation` → tester, for a defect in the tests themselves — a wrong, missing or fragile assertion, one that cannot fail, or coverage that does not exercise what it claims. The coordinator reads `verdict["retry_target"]` against that table and routes there. Adding a third category is a workflow edit and nothing else: the categories are injected into the verifier's prompt from the same table the coordinator routes on, so the prompt restates nothing and a category cannot exist in one and not the other. +**The verifier names a category; the workflow says where a category goes.** The verifier's `on_failure` is now a `retry_routing` table, each entry carrying a destination `stage` and a `when` description of the defect it covers. The shipped table defines three since story-045: `implementation` → implementer, for a defect in the code under test; `validation` → tester, for a defect in the tests themselves — a wrong, missing or fragile assertion, one that cannot fail, or coverage that does not exercise what it claims; and `documentation` → documenter, for a defect in the documentation itself — a document describing behaviour the code does not have, naming a file, module or symbol that does not exist, contradicting what the run's own artifacts record, or omitting a change the story required be written down. The coordinator reads `verdict["retry_target"]` against that table and routes there. Adding a category is a workflow edit and nothing else: the categories are injected into the verifier's prompt from the same table the coordinator routes on, so the prompt restates nothing and a category cannot exist in one and not the other — and the third category is the first time that claim was tested rather than asserted. + +**The `documentation` category's `when` carries a boundary the other two do not need**, and it is the line a verifier will actually stand on: judge the document against the code as it now stands, so a document that *accurately* describes wrong behaviour is an **implementation** defect and belongs to the category that owns the code. Without it the two categories overlap on every story where the documentation is the first place a reader meets the defect, and a route chosen by the wrong half sends the repair to a stage that cannot make it. **No default route survives, and that is the whole point.** `retry_stage` was deleted rather than kept as a fallback. A recommended retry carrying no `retry_target`, and one naming a category the workflow does not define, each **escalate** — a silent fallback is precisely the drift the table exists to remove, and absorbing a malformed verdict into "send it to the implementer" would reproduce the old behaviour under a new name while looking like it had been fixed. Both escalations go through `_escalate`, so `retry_count` is untouched, and both sit **above** `archive_attempt`, so no `attempts/attempt-N/` directory is written: nothing is being superseded, and the artifacts at the run-directory root already describe the attempt that failed. Both reasons name the offending value *and* list the categories the workflow does define, in `events.log` and in `escalation-summary.md`, so the escalation is actionable without opening the workflow definition. @@ -367,6 +369,28 @@ A table naming a stage that does not exist is a defect in the definition, and a **This harness now diverges from Appendix A's excerpts.** The appendix builds this system from an empty directory and prints `workflows/story-workflow.json` and the coordinator's verification branch as they stood at the `appendix-a` tag: a workflow whose `on_failure` is `{"retry_stage": "implementer", "max_retries": 2}`, a `clean_clone` declared as a bare artifact name, and a coordinator that indexes `stage_names` on that constant. All three are gone. A reader following the appendix and then reading this repository will find the routing table, the widened `clean_clone` object, the single ceiling in `rules/execution-rules.json`, and the two escalations in place of them; the appendix's build is still a correct level 3 harness, and this is the divergence rather than an erratum. The book documents the design this story implements in Chapters 14 and 17 and states it as a contract in `outputs/artifact-naming-changes-for-harness-repo.md`. **That manuscript is not in this checkout**, so the story was planned against the request's summary of the contract, and the conformance check against Chapter 17's retry-management section is an outstanding item carried forward rather than one performed here. +**story-045 widens that divergence rather than opening a new one.** The appendix's excerpts describe the four stages in the previous order — implementer, tester, verifier, documenter, with the documenter printed last and the coordinator's verification branch printed against that arrangement — and the shipped workflow now runs the documenter third. A reader following the appendix and then reading this repository will find the stage list reordered and a third routing category beside the two the appendix has none of; the appendix's build remains a correct level 3 harness, and this is the same kind of divergence as the routing table itself, not an erratum in either. + +## The documenter runs before the verifier + +Until story-045 the order was implementer → tester → verifier → documenter, and the documenter's position bought two gaps that were the same gap seen from two sides. + +**Nothing judged the documentation.** The verifier ran before the stage that wrote it, so seven consecutive runs — story-034, 037, 039, 040, 041, 042 and 043 — carry a verifier note saying the documenter's output could not be judged. A stage whose work no stage evaluates is a stage whose acceptance criteria are unfalsifiable. + +**And the clean-clone check ran on a tree the documenter had not finished writing.** The check is declared on the verifier and runs on its passing verdict, so under the old order the documenter edited the tree *after* the check cloned it and ran the suite in it. story-043 is where that cost something and it is worth stating concretely: its documenter wrote a sentence into this document naming a `tests/` module the same story had deleted, story-038's `test_every_tests_path_the_architecture_document_names_exists` failed on it, all three CI jobs went red, and the run's own execution history shows `clean-clone-passed` minutes *before* the stage that broke it. A completed run could ship a red suite, and the check that exists to prevent exactly that had already reported. + +One reorder closes both. The verifier is now last, so it judges the documenter's output through `{{documentation_report}}` and `{{documenter_changed_files}}`, and the clean-clone check clones a tree that already holds the documenter's edits. On a passing run the execution history now shows `clean-clone-passed` after the documenter's `stage-completed` entry and before `story-completed`, which is the ordering story-043 needed and did not have. + +**The reorder and the third route are inseparable, in both directions**, which is why they landed as one story rather than one and a follow-on. Without the route, story-028's rule that a missing or unrecognised category escalates rather than falling back means every documentation finding the verifier could now make becomes an escalation, because no category owns it. And the route cannot land first: `retry_routing_problems` refuses a destination that does not sit strictly before the declaring stage, so `documentation` → documenter with the documenter *after* the verifier makes every run refuse at pre-flight. Either half alone is a broken definition. + +**The cost is recorded as a decision rather than discovered in a bill: documentation is redone on every retry.** A failing verdict re-enters at the implementer or the tester, and the documenter now sits on the way back, so each retry costs one extra documenter invocation — roughly two to four dollars on this repository's runs. That cost is precisely why the stage was placed last originally. It was accepted because the alternative is documentation nothing judges, and because it is paid only when a story retries; a run that passes first time invokes the documenter exactly once, which is asserted rather than assumed. A redone documentation stage spends no retry: the ceiling in `rules/execution-rules.json` and the way retries are counted are untouched. + +**A fifth stage was considered and rejected**: a documentation verifier with its own prompt, routing and retry budget. The reorder gets the same judgement from the verifier that already exists and gets the clean-clone guarantee for free, where a fifth stage gets the judgement, costs a stage, and leaves the clean-clone ordering exactly where it was. Settled with the developer on 2026-08-15. + +**One thing the documenter is told changed as a side effect, and is left as it is.** Its `{{verification_result}}` renders as `None` on a first attempt, because it no longer runs after a verdict; on a retry it still receives the previous attempt's verdict from the run-directory root. Nothing routes on it and no behaviour depends on it. Changing what the documenter is told was outside story-045; a story that wants the documenter to see a verdict on the first pass has to decide what verdict that would be. + +**story-045 could not verify itself**, for the reason story-028 and story-030 already lived through: the coordinator loads the workflow at run start, so the run that lands the reorder executes under the old order, with its own documenter running last and unjudged. This section was written by that unjudged documenter. The first run *governed* by the new order is the next story. + ## A stage that failed mechanically runs again Three failures used to end a run that a second attempt would plausibly fix: the agent process died, a required output was never written, or the required output at the run root is a previous attempt's. None is a judgement about the work — the stage simply did not produce what it declared, and no verifier ever saw it. story-028's implementer is the observed case: it died on a dropped connection 127 turns and 34 minutes in, and the coordinator escalated with `retry_count` untouched, leaving $14.12 of substantial work needing a manual resume. @@ -453,6 +477,11 @@ The harness runs against any repository. Its only tie to a target's language, to changed-files.json implementer's record (modified/created/deleted) tester-changed-files.json tester's record, same schema definition; required tester output test-results.json + documentation-report.md documenter's account of what it wrote and why + documenter-changed-files.json + documenter's record, same schema definition; + required documenter output, and since story-045 + read by the verifier that runs after it verification/iteration-1.json retry-guidance.json written by the verifier on failure clean-clone-result.json the clean-clone check's record, coordinator-written @@ -470,7 +499,7 @@ The harness runs against any repository. Its only tie to a target's language, to retry-history.json one entry per retry taken; absent when none was completion-report.md or escalation-summary.md -The files at the root always describe the *current* attempt. `attempts/attempt-N/` appears only once a retry has occurred: before each retry begins, the coordinator copies the superseded attempt's stage artifacts there under their canonical filenames (`changed-files.json`, `implementation-summary.md`, `test-results.json`, `tester-changed-files.json`, `verification-result.json`, `retry-guidance.json`). A run that never retries has no `attempts/` directory at all, so its absence is itself evidence. N is the same attempt number the rendered prompts use, so `prompt-implementer-attempt-1.md` and `attempts/attempt-1/` describe one attempt. **A self-route leaves that correspondence alone**: it writes no `attempts/` directory, does not move N, and adds a `-try-M` suffix to its own prompt only, so `prompt-implementer-attempt-1.md` remains the prompt the first invocation of attempt 1 was given and `prompt-implementer-attempt-1-try-1.md` is the re-run's. Reading a stage's self-routes off the directory is therefore a matter of the `self-route-*` records and the try-suffixed prompts beside them, and their absence is evidence in the same way `attempts/`'s is. +The files at the root always describe the *current* attempt. `attempts/attempt-N/` appears only once a retry has occurred: before each retry begins, the coordinator copies the superseded attempt's stage artifacts there under their canonical filenames (`changed-files.json`, `implementation-summary.md`, `test-results.json`, `tester-changed-files.json`, `documentation-report.md`, `documenter-changed-files.json`, `verification-result.json`, `retry-guidance.json`). The documenter's two are new to the archive since story-045 and were added by nothing: `archivable_artifacts` derives the list from the loaded workflow, and the reorder simply means the documenter has run by the time an attempt is superseded, so its artifacts are there to copy. A run that never retries has no `attempts/` directory at all, so its absence is itself evidence. N is the same attempt number the rendered prompts use, so `prompt-implementer-attempt-1.md` and `attempts/attempt-1/` describe one attempt. **A self-route leaves that correspondence alone**: it writes no `attempts/` directory, does not move N, and adds a `-try-M` suffix to its own prompt only, so `prompt-implementer-attempt-1.md` remains the prompt the first invocation of attempt 1 was given and `prompt-implementer-attempt-1-try-1.md` is the re-run's. Reading a stage's self-routes off the directory is therefore a matter of the `self-route-*` records and the try-suffixed prompts beside them, and their absence is evidence in the same way `attempts/`'s is. `execution-history.json` is deliberately *not* among the archived artifacts: it is not a stage output, so `archivable_artifacts` never names it and a retry neither copies nor overwrites it. It stays one continuous stream across every attempt of the run, which is what lets a retried or escalated run be reconstructed from it end to end. @@ -498,7 +527,7 @@ It is narrower than Chapter 18's **checkpoints**, and the difference is the reas - An ownership violation escalates immediately without incrementing `retry_count`, matching a blocked-path violation. The stage did not fail at its work; it produced an output that is not its to produce, and a retry of the same instructions would produce it again. No new retry axis and no new `RunState` field. - A `stage_exception` is the pressure valve, and it is deliberately narrow: a path at or beneath a prefix the stage is actually restricted on, required `reason`, cross-checked against the loaded workflow at pre-flight, and recorded in `events.log` when applied. A story whose deliverable is the regression suite lifts the filesystem rule; it does not lift independence, because the tester still validates what the implementer wrote. story-032 made the *granularity* finer without making the valve wider: what a grant may name grew from one prefix to any path under it, and what a grant exempts shrank to exactly the path named — so the narrowest grant that does the job is now expressible, and is what to prefer. - **A grant is now something the plan is checked against, not only something the run applies.** The pressure valve and the refusal are two halves of one design: the plan-time check names declaring a grant as one of its two resolutions, so the grant has to be decidable at plan time, which is why `grant_covers` is shared between the check and the two run-time consumers rather than reimplemented. Three readers, one function; the alternative is a grant that means one thing when a plan is written and another when it runs. See "Work a stage cannot own" above. -- Every writing stage keeps its own changed-files record, and the verifier receives them injected separately: the implementer's `{{changed_files}}` is held to the approved story scope, while `{{tester_changed_files}}` lists test files that are expected additions of a later stage, not scope violations (`None` when absent, e.g. before the tester has run). Requiring the record in the stage's `outputs` list makes the existing required-artifacts check escalate when it is missing — no separate code path. +- Every writing stage keeps its own changed-files record, and the verifier receives them injected separately: the implementer's `{{changed_files}}` is held to the approved story scope, while `{{tester_changed_files}}` lists test files that are expected additions of a later stage, not scope violations (`None` when absent, e.g. before the tester has run). Since story-045 `{{documenter_changed_files}}` joins them on the same terms, beside `{{documentation_report}}`. Requiring the record in the stage's `outputs` list makes the existing required-artifacts check escalate when it is missing — no separate code path. - The coordinator loads the workflow definition at run start, so changes to the workflow (new outputs, new `changed_files` declarations) take effect for runs started after they merge, not for the run that made them. story-007 added `may_not_create` and so could not be governed by it: the declaration written by its implementer was not in the definition the coordinator had already loaded. Enforcement begins with story-008, the first run the rule actually governed, and it held: the implementer's `changed-files.json` listed three files, none under `tests/`, with an empty `created` array, and `tests/test_planner_injection.py` appears only in `tester-changed-files.json`. A story that adds an enforcement rule must expect to be the last story that rule does not cover, and say so in its constraints rather than treating the gap as a defect. - The same staleness applies to orchestration code, and it is sharper when the harness modifies itself. The coordinator process imports `context_assembler` once at start; a story that edits that module leaves later stages of its own run rendering *new* templates from disk against the *old* context builder. In story-004 that surfaced as `{{..._schema}}` placeholders rendering as `None` in the tester and verifier prompts stored under `.harness/runs/story-004/`. Not a defect and not something the run can fix — when reviewing a self-modifying story, judge the rendered prompts in that run directory as stale and confirm behavior from a fresh process instead. story-006 hit it again: `.harness/runs/story-006/prompt-verifier-attempt-1.md` carries the old indented-YAML criteria slice because the coordinator imported `context_assembler` before the implementer rewrote it. Expect this on any story touching `context_assembler`. - A schema mismatch escalates immediately — no retry, no change to `retry_count`. This keeps routing to a single new branch with no second retry axis and no new `RunState` field, and matches the repository's "fail loudly" standard. The cost of that strictness is bought down by the escalation reason (in both `events.log` and `escalation-summary.md`) naming the artifact, the failing path, what was expected, and what was found. Whether bounded regeneration is worth adding is an open question this design generates data for rather than answers. @@ -620,5 +649,9 @@ It is narrower than Chapter 18's **checkpoints**, and the difference is the reas - **A reference naming a path at a pinned historical revision keeps its historical spelling.** The rename sweep rewrote only references naming a file *as it exists now* — in `tests/`, `orchestration/`, `scripts/`, `prompts/` and this document. A reference that reads a path's text at a pinned revision was left spelled as it was there, because that is the name the object has at that revision and rewriting it breaks the read; `tests/test_baseline_honesty.py`'s regression set and `STORY_ORIGINS` itself are full of these. Nothing under `.harness/stories/` or `.harness/runs-archive/` was touched: both record what was true when they were written, and history describing the past accurately is not drift. - **A suite count recorded in a story is a measurement of the tree it was taken from, and the tree moves.** story-038's acceptance criterion recorded 2002 tests "before the rename", and reconciling the finished suite against it took two attempts. The number was correct when taken — the plan commit `abce051` collects exactly 2002 — but `426fe1f` ("Grant the verifier a self-route budget of 1") landed between the plan and the work, moving one declaration out of `BUDGETLESS` into `BUDGETED` and shrinking a parametrization by 3, so the branch base collects 1999. Every item of the difference to the finished count is accounted for by module: +3 for the naming scan and its two controls, +109 for this story's own validation. **The reconciliation that settles it is per-module collection counts between the two trees, not a total**, since a total can absorb a lost module against an added one; here 32 of the 34 renamed modules collect an identical count and the two that differ are the two named. Five test *names* disappeared and all five are merge disambiguations rather than deletions — two collisions in the story-008/story-009 merge renamed, three in the story-014/story-033 merge suffixed `_by_story_033`. When a story records a count, record the revision it was measured at with it; when a later stage cannot reproduce it, compare per module before concluding anything was lost. - **story-040 forced the standing-assertion repair pattern twice more, and the collision was unavoidable rather than incidental.** A module whose deliverable is to declare `tests/` and `pytest` cannot do it without spelling them, so `tests/test_stage_output_ownership.py::test_no_path_prefix_is_named_in_orchestration_code` and `tests/test_clean_clone_check.py::test_no_test_command_string_appears_in_orchestration_code` both went red on `orchestration/harness_source.py` the moment it existed. Each was repaired the way the bullet above prescribes — a by-name exemption held shut from both sides: the exempt module must exist *and* must actually contain the literal, or the exemption is stale and the test goes red; every other module is held to the original assertion unchanged; and neither exemption widens past the one literal it is about, so `harness_source.py` is still held to `may_not_create` in the first and to `-m pytest` and `unittest` in the second. Both repairs were folded into the existing test functions rather than added as new ones, because `tests/test_shared_baseline_resolution.py` pins `test_stage_output_ownership.py`'s test-name set exactly. No assertion was weakened, skipped or deleted and no test function was added or removed — the pre-story total of 2196 was unchanged by the repairs, and the finished suite is 2248 with story-040's own 52 cases. **The general shape worth carrying forward:** a story that adds a *declaration* of forbidden literals will collide with every standing scan that forbids them, and the collision is the declaration working rather than a defect in either. +- **Moving the last stage changed what `_complete` could rely on, and the finding was worth more than the reorder's own diff.** With the documenter last, `_complete`'s `git add -A && git commit` all but always found a dirty tree. With the verifier last, a run that enters at the verifier — a resume, for instance — changes no repository file, so `git commit` with nothing staged fails and the run completes leaving **no completion commit on the branch**. That is not cosmetic: `completion_commits` is how story-027's pre-flight recognises a finished story, so the branch would have been re-runnable. The commit now carries `--allow-empty`, for the reason both escalation commits already do; the message, the report and the ordering are untouched, and `tests/test_rerun_refusal.py::test_a_resume_of_an_escalated_run_is_unaffected` is what caught it and what holds it. **The general shape: a stage order is load-bearing for anything downstream that assumed what the last stage leaves behind.** Everything else the plan asked to be confirmed — `archivable_artifacts`, the attempt archive, the completion report's evidence list, the resume path — behaved correctly with no change. +- **story-045 modified a file in its own `do_not_modify` list, and the conflict was with a standing rule rather than with the story.** `prompts/documenter.md` gained `{{retry_guidance}}`, because story-028's rule — every stage a route can name must be able to say it is on a retry, asserted by `tests/test_retry_routing.py::test_every_stage_that_can_receive_a_retry_declares_the_placeholders`, which reads the destinations off the workflow — became true of the documenter the moment it became a destination. The diff is one placeholder with a two-word label, mirroring what `tester.md` got in story-028 for exactly this reason; nothing about what the documenter writes, nor the content or schema of `documentation-report.md`, changed, and reverting the edit re-breaks the suite. It was recorded as a stated deviation for the verifier to adjudicate rather than made silently, in the shape story-012's took, and the verifier accepted it. **A story that makes a stage a retry destination must expect that stage's prompt to be in scope**, whatever its `do_not_modify` list says. +- **Three assertions whose subject was "the documenter never ran" were restated rather than deleted.** A run that fails the clean-clone check, a refused clean-clone check, and an escalation at the retry ceiling all used the documenter's absence as the observable; the reorder makes the documenter genuinely run in all three, so each now asserts the guarantee that actually survives — the run did not *complete*: no completion report, status escalated. Repointing rather than deleting is the standing pattern; what is new is the reminder that **an assertion phrased against a stage's position expires when the position moves**, so prefer the outcome the check is about. 23 existing modules were repaired for the reorder in total, with no assertion weakened, skipped or deleted, and where a list could be derived off the loaded workflow instead of restated it now is (`stages_from` in `tests/test_escalation_resume.py`, `THROUGH_DOCUMENTER` in `tests/test_changed_files_records.py`), so the next reorder does not need them edited again. +- story-045's own run was not governed by the reorder it lands, for the same stale-workflow reason story-007, story-028 and story-030 record: the coordinator loads the workflow at run start, so its documenter ran last and unjudged and its verifier could not evaluate this document. Expected, stated in the story's own constraints, and not a defect. - Verification rules never change between retries; retries narrow scope, they do not restart the workflow. - Capacity exhaustion (rate limits) is a reason to wait, not to fail; budget ceilings are a reason to stop. diff --git a/orchestration/context_assembler.py b/orchestration/context_assembler.py index 795db02..8d24ee8 100644 --- a/orchestration/context_assembler.py +++ b/orchestration/context_assembler.py @@ -223,6 +223,10 @@ def build_context( "run_dir": str(run_dir), "changed_files": _read(run_dir / "changed-files.json"), "tester_changed_files": _read(run_dir / "tester-changed-files.json"), + "documenter_changed_files": _read( + run_dir / "documenter-changed-files.json" + ), + "documentation_report": _read(run_dir / "documentation-report.md"), "implementation_summary": _read(run_dir / "implementation-summary.md"), "test_results": _read(run_dir / "test-results.json"), "verification_result": _read(run_dir / "verification-result.json"), diff --git a/orchestration/story_coordinator.py b/orchestration/story_coordinator.py index 9e599e6..b9de586 100644 --- a/orchestration/story_coordinator.py +++ b/orchestration/story_coordinator.py @@ -934,7 +934,8 @@ def append_retry_record( # commits the tree after every check the workflow performs. Everything below # runs the same suite once more where the code actually ships — a fresh clone # of the repository with the story committed into it — after the verifier -# passes and before the documenter runs. +# passes. Since story-045 the verifier is the workflow's last stage, so the +# tree it clones already holds the documenter's edits. # -------------------------------------------------------------------------- #: How much of the run's combined output the record keeps. Enough to identify @@ -2501,7 +2502,16 @@ def _complete(run_dir: Path, state: RunState, story: dict, target_root: Path) -> ) (run_dir / "completion-report.md").write_text(report, encoding="utf-8") _git(target_root, "add", "-A") - _git(target_root, "commit", "-m", completion_commit_message(state, title)) + # `--allow-empty`, for the reason the escalation commits carry it: the + # commit is how a finished run is recognised — completion_commits reads it, + # and the pre-flight that refuses a re-run onto a finished branch reads + # that — so a run whose last stage changed no repository file must still + # leave one. Before story-045 the documenter ran last and all but + # guaranteed a dirty tree here; with the verifier last, a run that entered + # at it writes only run-directory artifacts, which a repository ignoring + # its run directory has nothing to commit from. + _git(target_root, "commit", "--allow-empty", "-m", + completion_commit_message(state, title)) append_event( run_dir, f"story completed on branch {state.branch}", diff --git a/prompts/documenter.md b/prompts/documenter.md index 8871951..8eb0b3f 100644 --- a/prompts/documenter.md +++ b/prompts/documenter.md @@ -60,6 +60,9 @@ after failing mechanically. The coordinator wrote it, not an agent: no verifier has judged this work, and it says what was missing or stale: {{self_route_result}} +Retry guidance: +{{retry_guidance}} + Retry lessons (retry history for this run): {{retry_state}} diff --git a/prompts/verifier.md b/prompts/verifier.md index 65aef84..786c9e7 100644 --- a/prompts/verifier.md +++ b/prompts/verifier.md @@ -12,6 +12,10 @@ You are a verification agent. Your responsibilities are to: - evaluate implementation behavior against the acceptance criteria, +- evaluate the documentation written for this story — the documentation + report and the documenter's changed-files record below are part of what + you judge, and a claim a document makes is held to the same evidence + standard as any other claim, - identify incomplete execution, - identify violations of the repository standards, and - produce evidence-backed findings. @@ -93,6 +97,13 @@ the tester stage; treat them as expected additions of a later stage, not implementation scope violations): {{tester_changed_files}} +Documenter changed files (documenter's record — documentation files created +or modified by the documenter stage): +{{documenter_changed_files}} + +Documentation report (the documenter's account of what it wrote and why): +{{documentation_report}} + Implementation summary: {{implementation_summary}} diff --git a/tests/test_artifact_schemas.py b/tests/test_artifact_schemas.py index aea82df..a71610a 100644 --- a/tests/test_artifact_schemas.py +++ b/tests/test_artifact_schemas.py @@ -305,7 +305,7 @@ def test_verdict_missing_a_routed_field_escalates_instead_of_routing( ) code = story_coordinator.run_story("story-001", harness_root, target_root, runner) assert code == 2 - assert runner.calls == ["implementer", "tester", "verifier"] + assert runner.calls == ["implementer", "tester", "documenter", "verifier"] state = json.loads((runner.run_dir / "state.json").read_text()) assert state["status"] == "escalated" @@ -375,7 +375,7 @@ def test_extra_keys_on_every_artifact_do_not_stop_a_run(target_root, harness_roo ) code = story_coordinator.run_story("story-001", harness_root, target_root, runner) assert code == 0 - assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + assert runner.calls == ["implementer", "tester", "documenter", "verifier"] assert not (runner.run_dir / "escalation-summary.md").exists() diff --git a/tests/test_attempt_archiving.py b/tests/test_attempt_archiving.py index 4ae25df..b187a2b 100644 --- a/tests/test_attempt_archiving.py +++ b/tests/test_attempt_archiving.py @@ -128,8 +128,14 @@ def read_state(run_dir: Path) -> dict: return json.loads((run_dir / "state.json").read_text(encoding="utf-8")) +#: What a superseded attempt leaves behind, sorted as the archive lists it. +#: The documenter's two artifacts joined it in story-045: the documenter now +#: runs before the verifier, so an attempt that fails verification has already +#: produced them. ATTEMPT_1_ARTIFACTS = [ "changed-files.json", + "documentation-report.md", + "documenter-changed-files.json", "implementation-summary.md", "retry-guidance.json", "test-results.json", @@ -151,7 +157,7 @@ def retry_then_pass(target_root, harness_root): return runner, run_dir_of(target_root) -def test_attempt_1_archive_holds_the_six_artifacts_under_canonical_names( +def test_attempt_1_archive_holds_every_stage_artifact_under_canonical_names( retry_then_pass, ): _, run_dir = retry_then_pass @@ -201,15 +207,26 @@ def test_the_archive_copies_rather_than_moves(retry_then_pass): assert (run_dir / name).is_file(), name -def test_an_artifact_the_attempt_did_not_write_is_skipped(retry_then_pass): - """The documenter never runs before a retry, so its report is absent from - the attempt-1 archive - skipped, not an archive failure.""" +def test_the_documenters_artifacts_are_archived_with_the_attempt(retry_then_pass): + """Since story-045 the documenter runs before the verifier, so a failed + attempt has already written its report and its record: both are archived + rather than skipped, and both are still at the run-directory root. + + What this case used to assert - an archivable artifact the attempt did not + write is skipped rather than failing the archive - is held directly on + archive_attempt by test_archive_attempt_skips_absent_artifacts_and_reports_ + what_it_copied below, which is where it can be exercised without depending + on which stage happens to run last. + """ _, run_dir = retry_then_pass - assert not (run_dir / "attempts" / "attempt-1" / "documentation-report.md").exists() - assert "documentation-report.md" in story_coordinator.archivable_artifacts( + archive = run_dir / "attempts" / "attempt-1" + archived = story_coordinator.archivable_artifacts( json.loads((REPO_ROOT / "workflows" / "story-workflow.json").read_text())["stages"] ) - assert (run_dir / "documentation-report.md").is_file() + for name in ("documentation-report.md", "documenter-changed-files.json"): + assert name in archived, name + assert (archive / name).is_file(), name + assert (run_dir / name).is_file(), name def test_the_archive_happens_before_the_retry_begins(retry_then_pass): @@ -276,8 +293,8 @@ def test_routing_is_unchanged_by_the_archive(retry_then_pass): assert state["status"] == "completed" assert state["retry_count"] == 1 assert runner.calls == [ - "implementer", "tester", "verifier", - "implementer", "tester", "verifier", "documenter", + "implementer", "tester", "documenter", + "verifier", "implementer", "tester", "documenter", "verifier", ] assert "retry 1 of 2" in (run_dir / "events.log").read_text() diff --git a/tests/test_branch_base.py b/tests/test_branch_base.py index 659f87e..5beb1dd 100644 --- a/tests/test_branch_base.py +++ b/tests/test_branch_base.py @@ -676,8 +676,8 @@ def test_the_note_does_not_change_where_execution_goes(stale, make_based): git(fresh, "branch", STORY_BRANCH) assert run(fresh)[0] == 0 - assert stale_runner.calls == ["implementer", "tester", "verifier", - "documenter"] + assert stale_runner.calls == ["implementer", "tester", "documenter", + "verifier"] assert notes(fresh) == [] diff --git a/tests/test_changed_files_records.py b/tests/test_changed_files_records.py index 934573e..9dc89bf 100644 --- a/tests/test_changed_files_records.py +++ b/tests/test_changed_files_records.py @@ -153,7 +153,7 @@ def test_enforcement_follows_declaration_not_stage_name(target_root, harness_roo ) code = story_coordinator.run_story("story-001", harness_copy, target_root, runner) assert code == 0 - assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + assert runner.calls == ["implementer", "tester", "documenter", "verifier"] # -------------------------------------------------------------------------- @@ -168,7 +168,13 @@ def test_enforcement_follows_declaration_not_stage_name(target_root, harness_roo # -------------------------------------------------------------------------- -ALL_STAGES = ["implementer", "tester", "verifier", "documenter"] +ALL_STAGES = ["implementer", "tester", "documenter", "verifier"] + +#: Where a run ends when the documenter's own record is what escalates it. +#: Since story-045 the documenter runs before the verifier, so an escalation +#: at that stage leaves the stage after it uninvoked. Derived from the list +#: above rather than written out, so the two cannot disagree. +THROUGH_DOCUMENTER = ALL_STAGES[:ALL_STAGES.index("documenter") + 1] #: A record naming a path the rules block, used for both the tester's and the #: documenter's blocked-path case so the two messages are comparable. @@ -291,21 +297,39 @@ def _sources_naming(directory: Path, name: str) -> list[str]: if name in p.read_text(encoding="utf-8")) +#: The one module allowed to spell the documenter's record, and it is the +#: injection side rather than the enforcement side: since story-045 the +#: verifier is handed the documenter's record through a placeholder, and +#: context_assembler already spells the implementer's and the tester's records +#: the same way. The exemption is held shut from both directions below — the +#: exempt module must actually contain the name, or it is stale — and the +#: subject is unchanged: what the *coordinator* enforces still reaches it only +#: off the loaded workflow. +NAMES_THE_RECORD_FOR_INJECTION = "context_assembler.py" + + def test_no_orchestration_source_names_the_documenters_record(harness_root, tmp_path): """The record name reaches the coordinator only off the loaded workflow. - The absence is that no module under orchestration/ spells it; the control - is a copy of orchestration/ with the name planted in one module, which the - same scan reports. + The absence is that no module under orchestration/ spells it, save the one + exempt module named above; the control is a copy of orchestration/ with the + name planted in one module, which the same scan reports. """ orchestration = harness_root / "orchestration" - assert _sources_naming(orchestration, "documenter-changed-files.json") == [] + naming = _sources_naming(orchestration, "documenter-changed-files.json") + assert naming == [NAMES_THE_RECORD_FOR_INJECTION] + # Held shut from the other side: the coordinator, which is what enforces + # the record, still spells neither the record nor the stage's own name for + # it, so the exemption cannot quietly widen into the routing code. + assert "documenter-changed-files.json" not in ( + orchestration / "story_coordinator.py").read_text(encoding="utf-8") planted = tmp_path / "orchestration-with-the-name" shutil.copytree(orchestration, planted) (planted / "planted.py").write_text( 'RECORD = "documenter-changed-files.json"\n', encoding="utf-8") - assert _sources_naming(planted, "documenter-changed-files.json") == ["planted.py"] + assert _sources_naming(planted, "documenter-changed-files.json") == [ + NAMES_THE_RECORD_FOR_INJECTION, "planted.py"] def test_documenter_writing_a_clean_record_completes_the_run(target_root, harness_root): @@ -329,7 +353,7 @@ def test_documenter_without_a_record_escalates_as_a_missing_artifact( runner = StageRunner(target_root, write_documenter_record=False) code = story_coordinator.run_story("story-001", harness_root, target_root, runner) assert code == 2 - assert runner.calls == ALL_STAGES + assert runner.calls == THROUGH_DOCUMENTER reason = story_coordinator.escalation_reason(runner.run_dir) assert reason == ("documenter did not produce required artifacts: " "documenter-changed-files.json") @@ -340,7 +364,7 @@ def test_documenter_naming_a_blocked_path_escalates(target_root, harness_root): runner = StageRunner(target_root, documenter_record=BLOCKED_RECORD) code = story_coordinator.run_story("story-001", harness_root, target_root, runner) assert code == 2 - assert runner.calls == ALL_STAGES + assert runner.calls == THROUGH_DOCUMENTER reason = story_coordinator.escalation_reason(runner.run_dir) assert reason == "documenter modified blocked path: rules/execution-rules.json" @@ -376,7 +400,7 @@ def test_documenter_record_failing_the_schema_escalates_as_invalid( ) code = story_coordinator.run_story("story-001", harness_root, target_root, runner) assert code == 2 - assert runner.calls == ALL_STAGES + assert runner.calls == THROUGH_DOCUMENTER reason = story_coordinator.escalation_reason(runner.run_dir) assert reason.startswith( "documenter wrote an invalid artifact: documenter-changed-files.json " diff --git a/tests/test_clean_clone_check.py b/tests/test_clean_clone_check.py index a8eeb0d..794f28d 100644 --- a/tests/test_clean_clone_check.py +++ b/tests/test_clean_clone_check.py @@ -795,19 +795,27 @@ def committed_failure_run(story_target, harness_root): return code, runner, run_dir_of(story_target) -def test_a_story_that_fails_only_once_committed_never_reaches_the_documenter( +def test_a_story_that_fails_only_once_committed_never_completes( committed_failure_run, ): + """What the check buys, restated where story-045 moved it. + + It used to be that such a run never reached the documenter, the last + stage. The documenter now runs before the verifier, so the guarantee is + the one that was always the point: a story whose suite fails where the + code ships does not finish - no completion report, and the run ends + escalated with its retries spent. + """ code, runner, run_dir = committed_failure_run assert code == 2 - assert "documenter" not in runner.calls - assert not (run_dir / "documentation-report.md").exists() + assert not (run_dir / "completion-report.md").exists() + assert read_state(run_dir)["status"] == "escalated" def test_the_same_story_with_its_baseline_corrected_advances(green_run): code, runner, run_dir = green_run assert code == 0 - assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + assert runner.calls == ["implementer", "tester", "documenter", "verifier"] assert read_state(run_dir)["status"] == "completed" @@ -834,16 +842,22 @@ def test_a_failing_check_records_its_evidence_too(committed_failure_run): assert schema_validator.validate(record, SCHEMA) == [] -def test_the_check_runs_before_the_documenter_stage_starts(green_run): - """Ordering asserted on the event stream, not on the call list alone.""" +def test_the_check_runs_after_the_documenter_stage_completes(green_run): + """Ordering asserted on the event stream, not on the call list alone. + + story-045 moved the documenter ahead of the verifier, so the check - which + runs on the verifier's passing verdict - now clones a tree that already + holds the documenter's edits. The ordering is what says so. + """ _, _, run_dir = green_run events = [e["event"] for e in history_of(run_dir)] stages = [e.get("stage") for e in history_of(run_dir)] passed = events.index("clean-clone-passed") documenter = next( index for index, entry in enumerate(history_of(run_dir)) - if entry["event"] == "stage-started" and entry["stage"] == "documenter") - assert events.index("verification-passed") < passed < documenter + if entry["event"] == "stage-completed" and entry["stage"] == "documenter") + assert documenter < events.index("verification-passed") < passed + assert passed < events.index("story-completed") assert stages[passed] == "verifier" @@ -870,11 +884,11 @@ def test_a_clean_clone_failure_reroutes_to_the_workflows_declared_retry_stage( _, runner, _ = committed_failure_run retry_stage = VERIFIER_STAGE["clean_clone"]["retry_stage"] assert runner.calls == [ - "implementer", "tester", "verifier", - "implementer", "tester", "verifier", - "implementer", "tester", "verifier", + "implementer", "tester", "documenter", "verifier", + "implementer", "tester", "documenter", "verifier", + "implementer", "tester", "documenter", "verifier", ] - assert runner.calls[3] == retry_stage + assert runner.calls[4] == retry_stage def test_each_clean_clone_failure_increments_the_retry_count_exactly_once( @@ -935,7 +949,7 @@ def test_a_refused_check_escalates_naming_the_missing_interpreter( entry = history_of(run_dir)[-1] assert entry["event"] == "escalated" assert ".venv999/bin/python" in entry["message"] - assert "documenter" not in runner.calls + assert not (run_dir / "completion-report.md").exists() assert record_of(run_dir)["ran"] is False @@ -975,8 +989,8 @@ def test_a_failed_verification_still_retries_exactly_as_before( run_dir = run_dir_of(story_target) assert runner.calls == [ - "implementer", "tester", "verifier", - "implementer", "tester", "verifier", "documenter", + "implementer", "tester", "documenter", + "verifier", "implementer", "tester", "documenter", "verifier", ] assert read_state(run_dir)["retry_count"] == 1 entry = next(e for e in history_of(run_dir) if e["event"] == "verification-failed") diff --git a/tests/test_config_keys_are_obeyed.py b/tests/test_config_keys_are_obeyed.py index 350f82e..2740883 100644 --- a/tests/test_config_keys_are_obeyed.py +++ b/tests/test_config_keys_are_obeyed.py @@ -932,7 +932,7 @@ def test_workflow_names_the_definition_the_run_actually_executes(tmp_path): # distinguishes "the named definition was loaded" from "a definition with # the same stages as the shipped one was loaded". assert AUDIT_STAGE in run.stages - assert run.stages == ["implementer", "tester", "verifier", "documenter", + assert run.stages == ["implementer", "tester", "documenter", "verifier", AUDIT_STAGE] assert (run.run_dir / AUDIT_ARTIFACT).is_file() assert AUDIT_STAGE not in [ diff --git a/tests/test_contract_assertions_bite.py b/tests/test_contract_assertions_bite.py index 8b524c4..b745795 100644 --- a/tests/test_contract_assertions_bite.py +++ b/tests/test_contract_assertions_bite.py @@ -598,9 +598,17 @@ def test_the_functions_that_did_change_are_only_those_that_took_the_baseline(): def test_the_named_survivors_are_present_and_unchanged(): - """The three the acceptance criteria name outright.""" + """The three the acceptance criteria name outright. + + Bounded at *this story's* endpoint, like both siblings above and for the + same reason: read against today's working tree it asks what the file looks + like now, so a later story that legitimately edits one of the three turns + it red for something story-011 has nothing to say about — which is what + story-045 did to it by reordering the stage list those assertions name. + The subject and the strictness are unchanged; only the upper bound moves. + """ before = functions_of(story_011_before_this_story()) - after = functions_of(STORY_011_FILE.read_text(encoding="utf-8")) + after = functions_of(story_011_at_this_storys_endpoint()) for name in ("test_every_log_line_has_one_history_entry_in_the_same_order", "test_the_retried_run_records_both_attempts_in_one_stream", "test_the_history_a_run_produced_validates_against_the_schema"): diff --git a/tests/test_documenter_before_verification.py b/tests/test_documenter_before_verification.py new file mode 100644 index 0000000..cb6e337 --- /dev/null +++ b/tests/test_documenter_before_verification.py @@ -0,0 +1,930 @@ +"""Independent validation for story-045: the documenter runs before the +verifier, and documentation is a retry category. + +One reorder, three consequences, and this module holds all three: + + * **the order.** The workflow reads implementer -> tester -> documenter -> + verifier, and a run invokes the stages in that order. + * **the third route.** The verifier's routing table declares + documentation -> documenter beside the two it already declared; the + category reaches the verifier's prompt through the injection story-028 + landed rather than through any prose in `prompts/verifier.md`; and a + failing verdict naming it re-enters at the documenter. + * **what the reorder buys.** The verifier is handed the documenter's + output, and the clean-clone check — which runs on the verifier's passing + verdict — clones a tree that already holds the documenter's edits. That + second one is story-043 reduced to a fixture: a documenter wrote a + sentence naming a `tests/` module the same story deleted, the suite + rejected it, and the run completed anyway because the check had already + passed minutes before. + +Almost nothing here is asserted from source. A target repository is built +under tmp_path, fake stage agents drive it into each shape, and what the +coordinator actually wrote — the execution history, the rendered prompts, +the clean clone's own committed tree, the run directory — is read back. + +Every absence asserted here carries a demonstration that it can fail, and +for this story the demonstration has one natural shape: *the previous +behaviour*. A harness root carrying the old stage order is built beside the +shipped one and the same fixture is run against both, so each ordering +claim is shown red under the order this story replaced: + + * "the clean clone holds the documenter's edits" sits beside the same run + under the old order, where the same clone does not hold them; + * "a documented claim the suite rejects ends the run" sits beside the same + documenter under the old order, where the run completes with a red + suite — which is what story-043 shipped; + * "the reordered workflow has no routing problems" sits beside the same + check over the old order with the route left in place, which reports the + documentation route by name; + * "`prompts/verifier.md` restates no category, destination or `when`" sits + beside the prompt rendered from that same template, which carries all + three; + * "the verifier's prompt carries the documenter's artifacts" sits beside + the same template rendered against a run directory holding neither, + where both placeholders resolve to the optional-placeholder None. + +This story cannot verify itself: the coordinator loads the workflow at the +start of a run, so the run that lands the reorder executes under the old +order. Everything below is about the *definition on disk* and about runs +driven from it explicitly, neither of which depends on how this run itself +was sequenced. + +Nothing here invokes a model: every run goes through a fake agent runner, +and `no_model` below turns the single subprocess call that would reach one +into a failure. +""" +import json +import shutil +import subprocess +from pathlib import Path + +import pytest + +import agent_runner +import context_assembler +import harness_config +import story_coordinator +from agent_runner import AgentResult +from conftest import BASELINE, ENDPOINT, repository_file_at + +REPO_ROOT = Path(story_coordinator.__file__).resolve().parents[1] + +WORKFLOW = harness_config.load_workflow(REPO_ROOT, "story-workflow") +STAGE_NAMES = [stage["name"] for stage in WORKFLOW["stages"]] + +#: The stage that declares the routing table, found by the declaration +#: rather than by name. +VERIFIER_STAGE = next(s for s in WORKFLOW["stages"] if "on_failure" in s) +VERIFIER_NAME = VERIFIER_STAGE["name"] +ROUTES = VERIFIER_STAGE["on_failure"]["retry_routing"] + +#: This module names the four stages and the three categories outright, +#: where `tests/test_retry_routing.py` deliberately does not. The difference +#: is the subject: that module validates *routing whatever the workflow +#: declares*, and a name written into it would let a coordinator that routes +#: to a constant pass. This module validates the story's own acceptance +#: criteria, which name the order and the categories, so a workflow that +#: quietly declares something else is exactly what it must report. +EXPECTED_ORDER = ["implementer", "tester", "documenter", "verifier"] +EXPECTED_CATEGORIES = ["documentation", "implementation", "validation"] +DOCUMENTATION = "documentation" +DOCUMENTER = "documenter" + +#: The order this story replaced, derived from the new one so it stays the +#: previous order rather than a second list to maintain. +PREVIOUS_ORDER = ["implementer", "tester", "verifier", "documenter"] + +VERIFIER_TEMPLATE_PATH = REPO_ROOT / "prompts" / f"{VERIFIER_NAME}.md" +VERIFIER_TEMPLATE = VERIFIER_TEMPLATE_PATH.read_text(encoding="utf-8") + +RULES = harness_config.load_rules(REPO_ROOT) +MAX_RETRIES = RULES["max_retries"] + +STORY_ID = "story-001" +DEFAULT_BRANCH = "main" +ARCHITECTURE_DOC = ".harness/docs/ARCHITECTURE.md" + +#: The documenter's marker in the repository tree, and its marker in the +#: report it writes into the run directory. Two markers because the two +#: reach the verifier by different routes — one through the architecture +#: document the tree carries, one through {{documentation_report}} — and a +#: single marker could not tell them apart. +DOC_MARKER = "DOCUMENTER_WROTE_THIS" +#: Deliberately not a phrase the verifier's template itself uses, so its +#: presence in a rendered prompt is content that was injected rather than +#: the label the template prints above the placeholder. +REPORT_MARKER = "REPORTED_BY_THE_DOCUMENTER" + +#: story-043's case, reduced to one sentence: a documenter naming a tests/ +#: module the same story deleted. The name is one no module in this +#: repository has, so a suite that rejects it is rejecting this sentence. +DELETED_MODULE = "tests/test_a_module_this_story_deleted.py" + +PASS = {"status": "passed", "blocking_issues": [], "unverified": [], + "retry_recommended": False} + + +def failing(category: str) -> dict: + return { + "status": "failed", + "blocking_issues": [{ + "severity": "high", + "issue": "the sample behavior is not implemented", + "location": "src/app.py:1", + "required_behavior": "the sample behavior exists", + }], + "unverified": [], + "retry_recommended": True, + "retry_target": category, + } + + +STORY = f"""\ +story: + id: {STORY_ID} + title: Sample story for coordinator tests + description: | + A stand-in story used to exercise the workflow deterministically. + +tasks: + - do the sample work + +acceptance_criteria: + - the sample behavior exists + - existing behavior is preserved + +scope: + modify: + - src/ + do_not_modify: + - rules/ + +verification_requirements: + - confirm the sample behavior + +constraints: + - preserve existing behavior +""" + +CONFIG = """\ +workflow: {workflow} +branch_prefix: story/ +permission_mode: acceptEdits +stories_dir: .harness/stories +runs_dir: .harness/runs +logs_dir: .harness/logs +standards_dir: .harness/standards +architecture_docs: + - {doc} +test_command: {test_command} +""" + + +# -------------------------------------------------------------------------- +# No model, for every test in this file +# -------------------------------------------------------------------------- + + +@pytest.fixture(autouse=True) +def no_model(monkeypatch): + """Turn the one call that would reach a model into a failure. + + Every run below passes a fake runner explicitly, so this should never + fire; it exists so "no model was invoked" is enforced rather than + assumed, and the test beneath it shows the guard can fire. + """ + real = agent_runner.subprocess.Popen + + def guarded(command, *args, **kwargs): + first = command[0] if isinstance(command, (list, tuple)) else command + if str(first).endswith("claude"): + raise AssertionError("a model was invoked") + return real(command, *args, **kwargs) + + monkeypatch.setattr(agent_runner.subprocess, "Popen", guarded) + + +def test_the_no_model_guard_fires_when_a_model_is_invoked(tmp_path): + """The control for the guard every other test in this file runs under.""" + with pytest.raises(AssertionError, match="a model was invoked"): + agent_runner.run_agent("prompt", stage=EXPECTED_ORDER[0], cwd=tmp_path, + log_path=tmp_path / "agent.log") + + +# -------------------------------------------------------------------------- +# A target repository, harness roots, and a fake runner +# -------------------------------------------------------------------------- + + +def write(path: Path, text: str) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + + +def write_json(path: Path, payload) -> None: + write(path, json.dumps(payload, indent=2) + "\n") + + +def git(root: Path, *args: str, check: bool = True) -> subprocess.CompletedProcess: + return subprocess.run(["git", "-C", str(root), *args], + capture_output=True, text=True, check=check) + + +def build_target(root: Path, *, workflow: str = "story-workflow", + test_command: str = "echo tests-ok") -> Path: + """A target repository with a story, standards and an architecture doc. + + The run directory and the log directory are ignored, as they are in a + real target: the clean-clone check copies untracked-but-not-ignored + files into the clone, and a run's own artifacts are not part of the tree + the suite is meant to be run against. + """ + for sub in (".harness/standards", ".harness/stories", ".harness/runs", + ".harness/logs", ".harness/docs"): + (root / sub).mkdir(parents=True) + write(root / ".gitignore", ".harness/runs/\n.harness/logs/\n") + write(root / ".harness" / "config.yaml", + CONFIG.format(workflow=workflow, doc=ARCHITECTURE_DOC, + test_command=test_command)) + write(root / ".harness" / "stories" / f"{STORY_ID}.yaml", STORY) + write(root / ".harness" / "standards" / "coding.md", "# Coding\n- simple\n") + write(root / ".harness" / "standards" / "testing.md", "# Testing\n- test it\n") + write(root / ARCHITECTURE_DOC, "# Architecture\n\nThe harness runs stages.\n") + write(root / "src" / "app.py", "print('hello')\n") + git(root, "init", "-q") + git(root, "config", "user.email", "t@example.com") + git(root, "config", "user.name", "T") + git(root, "add", "-A") + git(root, "commit", "-q", "-m", "initial") + git(root, "branch", "-M", DEFAULT_BRANCH) + return root + + +@pytest.fixture +def target(tmp_path: Path) -> Path: + return build_target(tmp_path / "target") + + +@pytest.fixture +def harness_root() -> Path: + return REPO_ROOT + + +def probe_harness(tmp_path: Path, name: str, mutate) -> Path: + """A harness root carrying a workflow this repository does not ship. + + Everything but the workflow is the shipped harness — the same prompts, + the same schemas, the same rules — so a run against it differs from a + run against this repository in exactly the definition under test. This + is how the *previous* stage order is exercised: it is not a workflow + anything ships any more, and only a run driven from it can show what the + reorder changed. + """ + root = tmp_path / name + root.mkdir() + for directory in ("prompts", "rules", "schemas"): + shutil.copytree(REPO_ROOT / directory, root / directory) + workflow = json.loads(json.dumps(WORKFLOW)) + workflow["name"] = name + mutate(workflow) + (root / "workflows").mkdir() + write_json(root / "workflows" / f"{name}.json", workflow) + return root + + +def reorder(workflow: dict, order: list[str]) -> None: + """Put the workflow's stages in `order`, changing no declaration.""" + by_name = {stage["name"]: stage for stage in workflow["stages"]} + workflow["stages"] = [by_name[name] for name in order] + + +def drop_documentation_route(workflow: dict) -> None: + verifier = next(s for s in workflow["stages"] if s["name"] == VERIFIER_NAME) + verifier["on_failure"]["retry_routing"].pop(DOCUMENTATION) + + +def previous_order(workflow: dict) -> None: + """The workflow as it stood before this story: old order, two routes. + + Both halves, because they are inseparable — `retry_routing_problems` + refuses the documentation route the moment the documenter sits after the + verifier, so a workflow in the previous order that kept the route is not + a workflow any run reaches. + """ + reorder(workflow, PREVIOUS_ORDER) + drop_documentation_route(workflow) + + +@pytest.fixture +def old_order_harness(tmp_path: Path) -> Path: + return probe_harness(tmp_path, "previous-order", previous_order) + + +class Runner: + """A fake agent runner: each stage writes the artifacts it declares. + + The implementer edits the repository tree and the documenter edits the + architecture document, which is what gives the clean-clone check two + distinguishable things to carry into its clone. `documented` is the + sentence the documenter writes, so a test chooses whether the document + makes a claim the suite accepts. + + The verdicts are consumed one per verifier call, the last repeating, so + a run is driven into a shape by listing what the verifier says. + """ + + def __init__(self, target_root: Path, verdicts: list | None = None, *, + documented: str = DOC_MARKER): + self.target_root = target_root + self.run_dir = target_root / ".harness" / "runs" / STORY_ID + self.verdicts = list(verdicts or [PASS]) + self.documented = documented + self.calls: list[str] = [] + + def __call__(self, prompt, *, stage, cwd=None, log_path=None, + permission_mode=None, model=None, allowed_tools=None): + self.calls.append(stage) + if stage == "implementer": + write(self.target_root / "src" / "app.py", + "print('hello')\n# the story's change\n") + write_json(self.run_dir / "changed-files.json", + {"modified": ["src/app.py"], "created": [], "deleted": []}) + write(self.run_dir / "implementation-summary.md", "Did the work.\n") + elif stage == "tester": + write_json(self.run_dir / "test-results.json", { + "status": "passed", "tests_written": 1, "tests_run": 1, + "tests_passed": 1, "tests_failed": 0, "failures": [], + }) + write_json(self.run_dir / "tester-changed-files.json", { + "modified": [], "created": ["tests/test_app.py"], "deleted": [], + }) + elif stage == "documenter": + write(self.target_root / ARCHITECTURE_DOC, + f"# Architecture\n\nThe harness runs stages.\n" + f"{self.documented}\n") + write(self.run_dir / "documentation-report.md", + f"# Documentation report\n\n{REPORT_MARKER}: " + f"{self.documented}\n") + write_json(self.run_dir / "documenter-changed-files.json", { + "modified": [ARCHITECTURE_DOC], "created": [], "deleted": [], + }) + elif stage == VERIFIER_NAME: + verdict = self.verdicts.pop(0) if len(self.verdicts) > 1 \ + else self.verdicts[0] + write_json(self.run_dir / "verification-result.json", verdict) + if verdict["status"] == "failed": + write_json(self.run_dir / "retry-guidance.json", { + "current_focus": ["fix what the verdict named"], + "preserve_behavior": ["existing behavior"], + "retry_scope": ["src/app.py"], + }) + return AgentResult(ok=True, result_text=f"{stage} done") + + +def run_dir_of(target_root: Path) -> Path: + return target_root / ".harness" / "runs" / STORY_ID + + +def history_of(target_root: Path) -> list[dict]: + return json.loads((run_dir_of(target_root) / "execution-history.json") + .read_text(encoding="utf-8")) + + +def prompt_of(target_root: Path, stage: str, attempt: int) -> str: + return (run_dir_of(target_root) / + story_coordinator.prompt_file(stage, attempt)).read_text( + encoding="utf-8") + + +def state_of(target_root: Path) -> dict: + return json.loads((run_dir_of(target_root) / "state.json").read_text( + encoding="utf-8")) + + +# -------------------------------------------------------------------------- +# The order +# -------------------------------------------------------------------------- + + +def test_the_workflow_lists_the_stages_in_the_new_order(): + assert STAGE_NAMES == EXPECTED_ORDER + + +def test_a_run_invokes_the_stages_in_that_order(target, harness_root): + runner = Runner(target) + assert story_coordinator.run_story( + STORY_ID, harness_root, target, runner) == 0 + assert runner.calls == EXPECTED_ORDER + + +def test_a_run_under_the_previous_order_invokes_them_in_the_previous_order( + tmp_path, old_order_harness, +): + """The control for the two assertions above. + + The order is something the definition decides and the loop follows, so + the same coordinator and the same fake runner produce the previous order + when handed the previous definition. Without this, "the calls came in + this order" would hold equally against a loop that ignored the + definition and ran a sequence written into it. + """ + root = build_target(tmp_path / "old-order-target", workflow="previous-order") + runner = Runner(root) + assert story_coordinator.run_story( + STORY_ID, old_order_harness, root, runner) == 0 + assert runner.calls == PREVIOUS_ORDER + assert runner.calls != EXPECTED_ORDER + + +def test_the_move_changed_no_stage_declaration(harness_root): + """A reorder and nothing else: every stage's own declaration is what it + was, apart from the one route the story adds. + + Read at both ends of this story's own commit range through the shared + resolution, so it is a comparison of the definition before this story + against the definition after it — not of the working tree against + whatever HEAD happens to be. + """ + before = json.loads(repository_file_at( + "workflows/story-workflow.json", validation_file=Path(__file__), + bound=BASELINE)) + after = json.loads(repository_file_at( + "workflows/story-workflow.json", validation_file=Path(__file__), + bound=ENDPOINT)) + + old = {stage["name"]: stage for stage in before["stages"]} + new = {stage["name"]: stage for stage in after["stages"]} + assert sorted(old) == sorted(new) + + # The comparison is live: the two readings really are of different + # orders, so the equality below is a statement about declarations rather + # than about one file read twice. + assert [s["name"] for s in before["stages"]] == PREVIOUS_ORDER + assert [s["name"] for s in after["stages"]] == EXPECTED_ORDER + + for name in old: + if name == VERIFIER_NAME: + continue + assert new[name] == old[name], name + + old_routes = old[VERIFIER_NAME]["on_failure"]["retry_routing"] + new_routes = new[VERIFIER_NAME]["on_failure"]["retry_routing"] + assert set(new_routes) - set(old_routes) == {DOCUMENTATION} + for category, route in old_routes.items(): + assert new_routes[category] == route, category + # Everything else the verifier declares — its prompt, its outputs, its + # schemas, its clean-clone declaration, its self-route budget — is + # untouched by the move. + assert {key: value for key, value in new[VERIFIER_NAME].items() + if key != "on_failure"} == \ + {key: value for key, value in old[VERIFIER_NAME].items() + if key != "on_failure"} + + +# -------------------------------------------------------------------------- +# The third route +# -------------------------------------------------------------------------- + + +def test_the_verifier_declares_exactly_the_three_categories(): + assert sorted(ROUTES) == EXPECTED_CATEGORIES + assert ROUTES[DOCUMENTATION]["stage"] == DOCUMENTER + assert ROUTES[DOCUMENTATION]["when"].strip() + + +def test_the_documentation_when_tells_the_document_from_the_code_it_describes(): + """The `when` is what the verifier chooses by, so what it distinguishes + is a property of this story rather than of any code path.""" + when = ROUTES[DOCUMENTATION]["when"].lower() + # A defect in the document itself is the subject. + assert "in the documentation itself" in when + # And the case that belongs to the other category is stated outright, + # naming that category. + assert "accurately describes wrong behaviour" in when + assert "implementation defect" in when + assert "not a documentation one" in when + + +def test_the_two_existing_routes_are_preserved(): + assert ROUTES["implementation"]["stage"] == "implementer" + assert ROUTES["validation"]["stage"] == "tester" + + +def test_the_reordered_workflow_declares_no_routing_problem(): + assert story_coordinator.retry_routing_problems(WORKFLOW["stages"]) == [] + + +def test_restoring_the_previous_order_makes_the_documentation_route_a_problem(): + """The control for the absence above, and the reason the reorder and the + route are one story: the route is legal only after the move.""" + workflow = json.loads(json.dumps(WORKFLOW)) + reorder(workflow, PREVIOUS_ORDER) + + problems = story_coordinator.retry_routing_problems(workflow["stages"]) + + assert len(problems) == 1, problems + assert DOCUMENTATION in problems[0] + assert DOCUMENTER in problems[0] + assert VERIFIER_NAME in problems[0] + + +def test_the_previous_order_with_the_route_is_refused_at_pre_flight(tmp_path): + """End to end: no run directory, no branch, no agent invoked.""" + harness = probe_harness(tmp_path, "old-order-with-route", + lambda workflow: reorder(workflow, PREVIOUS_ORDER)) + root = build_target(tmp_path / "refused-target", + workflow="old-order-with-route") + runner = Runner(root) + + assert story_coordinator.run_story(STORY_ID, harness, root, runner) == 1 + + assert runner.calls == [] + assert not run_dir_of(root).exists() + assert git(root, "branch", "--list", f"story/{STORY_ID}").stdout.strip() == "" + + +def test_the_same_workflow_without_the_route_is_not_refused( + tmp_path, old_order_harness, +): + """The control for the refusal above: what is refused is the route, not + the probe harness or the old order.""" + root = build_target(tmp_path / "accepted-target", workflow="previous-order") + runner = Runner(root) + + assert story_coordinator.run_story( + STORY_ID, old_order_harness, root, runner) == 0 + assert runner.calls == PREVIOUS_ORDER + + +# -------------------------------------------------------------------------- +# A documentation verdict re-enters at the documenter +# -------------------------------------------------------------------------- + + +def test_a_documentation_verdict_re_runs_the_documenter_and_not_the_implementer( + target, harness_root, +): + runner = Runner(target, [failing(DOCUMENTATION), PASS]) + assert story_coordinator.run_story( + STORY_ID, harness_root, target, runner) == 0 + + assert runner.calls == EXPECTED_ORDER + [DOCUMENTER, VERIFIER_NAME] + # The stages before the destination are not re-invoked on the way back. + assert runner.calls.count("implementer") == 1 + assert runner.calls.count("tester") == 1 + assert runner.calls.count(DOCUMENTER) == 2 + # The retried documenter is told it is on a retry, and by which category. + retried = prompt_of(target, DOCUMENTER, 2) + assert DOCUMENTATION in retried + assert context_assembler.PLACEHOLDER.search(retried) is None + + +@pytest.mark.parametrize("category", EXPECTED_CATEGORIES) +def test_the_history_records_every_category_the_same_way( + target, harness_root, category, +): + """The documentation category is recorded exactly as the other two are: + same event, same fields, same shape — only the values differ.""" + destination = ROUTES[category]["stage"] + runner = Runner(target, [failing(category), PASS]) + assert story_coordinator.run_story( + STORY_ID, harness_root, target, runner) == 0 + + entry = next(e for e in history_of(target) + if e["event"] == "verification-failed") + assert entry["retry_category"] == category + assert entry["retry_stage"] == destination + assert entry["retry_decision"] == "retry" + assert entry["stage"] == VERIFIER_NAME + assert entry["verifier_outcome"] == "failed" + assert destination in entry["message"] and category in entry["message"] + + +def test_a_run_that_passes_first_time_invokes_the_documenter_once( + target, harness_root, +): + """The cost the story accepted is paid only on a retry. A reorder that + quietly ran the documenter twice on the common case would be a + different, more expensive trade.""" + runner = Runner(target) + assert story_coordinator.run_story( + STORY_ID, harness_root, target, runner) == 0 + + assert runner.calls.count(DOCUMENTER) == 1 + started = [e for e in history_of(target) + if e["event"] == "stage-started" and e["stage"] == DOCUMENTER] + assert len(started) == 1 + + +# -------------------------------------------------------------------------- +# The verifier's new subject +# -------------------------------------------------------------------------- + + +def test_the_verifiers_prompt_carries_the_documenters_report_and_record( + target, harness_root, +): + runner = Runner(target) + assert story_coordinator.run_story( + STORY_ID, harness_root, target, runner) == 0 + + run_dir = run_dir_of(target) + prompt = prompt_of(target, VERIFIER_NAME, 1) + + report = (run_dir / "documentation-report.md").read_text(encoding="utf-8") + record = (run_dir / "documenter-changed-files.json").read_text( + encoding="utf-8") + assert report.strip() in prompt + assert record.strip() in prompt + assert REPORT_MARKER in prompt + assert ARCHITECTURE_DOC in prompt + assert context_assembler.PLACEHOLDER.search(prompt) is None + + +def test_the_prompt_says_none_when_the_documenter_wrote_nothing(target, tmp_path): + """The control for the assertion above. + + The same template rendered against a run directory holding neither + artifact resolves both placeholders to the optional-placeholder None, so + what the run's prompt carried is content that was injected rather than + prose the template always had. + """ + empty = tmp_path / "empty-run-dir" + (empty / "verification").mkdir(parents=True) + + context = context_assembler.build_context( + story_text=STORY, + story=story_coordinator.read_story(STORY).parsed, + run_dir=empty, + target_root=target, + harness_root=REPO_ROOT, + config=harness_config.load_config(target), + rules=RULES, + workflow=WORKFLOW, + retry_count=0, + ) + rendered = context_assembler.render(VERIFIER_TEMPLATE, context) + + assert context["documentation_report"] is None + assert context["documenter_changed_files"] is None + assert REPORT_MARKER not in rendered + assert context_assembler.PLACEHOLDER.search(rendered) is None + + +def test_the_template_declares_both_placeholders(): + assert "{{documentation_report}}" in VERIFIER_TEMPLATE + assert "{{documenter_changed_files}}" in VERIFIER_TEMPLATE + + +def test_the_role_layer_says_the_documenters_output_is_part_of_the_subject(): + """Positive: the template must say it, so it fails on its own if the + sentence is dropped.""" + role = VERIFIER_TEMPLATE.split("[Role Layer]", 1)[1].split("[Workflow Layer]", 1)[0] + # Whitespace-collapsed, so a sentence the template hard-wraps reads as + # one sentence rather than as whatever the wrap happened to split. + responsibilities = " ".join(role.split("Do not:", 1)[0].lower().split()) + assert "documentation report" in responsibilities + assert "changed-files record" in responsibilities + assert "documenter" in responsibilities + + +def test_the_prompt_names_all_three_routes_with_destination_and_when( + target, harness_root, +): + runner = Runner(target) + assert story_coordinator.run_story( + STORY_ID, harness_root, target, runner) == 0 + prompt = prompt_of(target, VERIFIER_NAME, 1) + + for category, route in ROUTES.items(): + line = next((line for line in prompt.splitlines() + if category in line and route["stage"] in line), None) + assert line is not None, category + assert route["when"] in line + + +def test_the_template_restates_no_category_destination_or_when( + target, harness_root, +): + """The routes reach the verifier by injection, not by prose. + + The absence is asserted against the *rendered* prompt as its control: + every pairing and every `when` this test says the template lacks is + shown present once the same template has been rendered, so a check + looking at the wrong text would report the pairing missing from both. + """ + runner = Runner(target) + assert story_coordinator.run_story( + STORY_ID, harness_root, target, runner) == 0 + prompt = prompt_of(target, VERIFIER_NAME, 1) + + for category, route in ROUTES.items(): + pairing = f"{category} -> {route['stage']}" + assert pairing not in VERIFIER_TEMPLATE, category + assert pairing in prompt, category + assert route["when"] not in VERIFIER_TEMPLATE, category + assert route["when"] in prompt, category + + +def test_a_fourth_category_reaches_the_prompt_with_no_edit_to_the_template( + tmp_path, target, harness_root, +): + """The property story-028 landed, of which this story's third category is + the first real test — asserted here against a fourth, so the injection is + shown to be general rather than to have been widened by hand to three.""" + added = "packaging" + assert added not in ROUTES, "pick a category the shipped workflow lacks" + when = "the defect is in how this story's work is packaged" + + def mutate(workflow: dict) -> None: + verifier = next(s for s in workflow["stages"] + if s["name"] == VERIFIER_NAME) + verifier["on_failure"]["retry_routing"][added] = { + "stage": EXPECTED_ORDER[0], "when": when, + } + + harness = probe_harness(tmp_path, "four-categories", mutate) + root = build_target(tmp_path / "target-four", workflow="four-categories") + + assert story_coordinator.run_story( + STORY_ID, harness, root, Runner(root)) == 0 + assert story_coordinator.run_story( + STORY_ID, harness_root, target, Runner(target)) == 0 + + assert (harness / "prompts" / f"{VERIFIER_NAME}.md").read_bytes() == \ + VERIFIER_TEMPLATE_PATH.read_bytes() + + with_four = prompt_of(root, VERIFIER_NAME, 1) + with_three = prompt_of(target, VERIFIER_NAME, 1) + assert any(added in line and EXPECTED_ORDER[0] in line and when in line + for line in with_four.splitlines()) + assert added not in with_three + + +# -------------------------------------------------------------------------- +# The clean-clone check now runs over a tree the documenter has edited +# -------------------------------------------------------------------------- + + +def clone_evidence(tmp_path: Path, name: str) -> tuple[str, Path, Path]: + """A test command that reports what the clean clone's own commit holds. + + Asserting on the check's exit code alone would say only that *a* suite + passed somewhere. This command runs inside the clone, reads the + architecture document out of the clone's `HEAD` commit — the state a + test resolving a baseline out of git history actually sees — and records + the clone's root beside it, so the evidence can be tied to the scratch + clone the run's own record names. + """ + doc = tmp_path / f"{name}-doc.txt" + where = tmp_path / f"{name}-where.txt" + command = ( + f"sh -c 'git show HEAD:{ARCHITECTURE_DOC} > {doc}; " + f"git rev-parse --show-toplevel > {where}'" + ) + return command, doc, where + + +def test_the_clean_clone_holds_the_documenters_edits(tmp_path, harness_root): + command, doc, where = clone_evidence(tmp_path, "new-order") + root = build_target(tmp_path / "clone-target", test_command=command) + + assert story_coordinator.run_story( + STORY_ID, harness_root, root, Runner(root)) == 0 + + record = json.loads((run_dir_of(root) / "clean-clone-result.json") + .read_text(encoding="utf-8")) + assert record["ran"] is True and record["exit_code"] == 0 + # The evidence came from the scratch clone this run's own record names, + # not from some other checkout that happened to be lying around. + assert Path(where.read_text(encoding="utf-8").strip()).resolve() == \ + Path(record["clone_path"]).resolve() + # And that clone's commit holds what the documenter wrote. + assert DOC_MARKER in doc.read_text(encoding="utf-8") + + +def test_under_the_previous_order_the_clean_clone_lacked_them( + tmp_path, old_order_harness, +): + """The control, and the gap this story closed: the same check over the + same fixture under the previous order clones a tree the documenter has + not touched yet.""" + command, doc, where = clone_evidence(tmp_path, "old-order") + root = build_target(tmp_path / "old-clone-target", workflow="previous-order", + test_command=command) + + assert story_coordinator.run_story( + STORY_ID, old_order_harness, root, Runner(root)) == 0 + + assert where.is_file(), "the check did not run at all" + assert DOC_MARKER not in doc.read_text(encoding="utf-8") + # The documenter did run — after the check, which is the whole point. + assert DOC_MARKER in (root / ARCHITECTURE_DOC).read_text(encoding="utf-8") + + +def test_the_check_runs_after_the_documenter_and_before_the_run_completes( + target, harness_root, +): + runner = Runner(target) + assert story_coordinator.run_story( + STORY_ID, harness_root, target, runner) == 0 + + events = [(e["event"], e.get("stage")) for e in history_of(target)] + documented = events.index(("stage-completed", DOCUMENTER)) + passed = events.index(("verification-passed", VERIFIER_NAME)) + clean = events.index(("clean-clone-passed", VERIFIER_NAME)) + completed = events.index(("story-completed", None)) + assert documented < passed < clean < completed + + +def test_under_the_previous_order_the_check_ran_before_the_documenter( + tmp_path, old_order_harness, +): + """The control for the ordering above, on the same event stream.""" + root = build_target(tmp_path / "old-events-target", workflow="previous-order") + assert story_coordinator.run_story( + STORY_ID, old_order_harness, root, Runner(root)) == 0 + + events = [(e["event"], e.get("stage")) for e in history_of(root)] + assert events.index(("clean-clone-passed", VERIFIER_NAME)) < \ + events.index(("stage-completed", DOCUMENTER)) + + +# -------------------------------------------------------------------------- +# story-043, reconstructed +# -------------------------------------------------------------------------- + + +def rejecting_command() -> str: + """A suite that rejects one claim: a document naming a deleted module.""" + return ( + f"sh -c 'if grep -q {DELETED_MODULE} {ARCHITECTURE_DOC}; " + f"then exit 1; fi'" + ) + + +def test_a_documented_claim_the_suite_rejects_now_ends_the_run( + tmp_path, harness_root, +): + """story-043's case: the documenter writes a sentence naming a tests/ + module the same story deleted. The clean-clone check now sees it, and + the run escalates rather than completing.""" + root = build_target(tmp_path / "story-043-target", + test_command=rejecting_command()) + sentence = f"Validation for this lives in {DELETED_MODULE}." + runner = Runner(root, documented=sentence) + + assert story_coordinator.run_story( + STORY_ID, harness_root, root, runner) == 2 + + run_dir = run_dir_of(root) + assert not (run_dir / "completion-report.md").exists() + assert state_of(root)["status"] == "escalated" + record = json.loads((run_dir / "clean-clone-result.json").read_text( + encoding="utf-8")) + assert record["ran"] is True and record["exit_code"] != 0 + # It ended on the check rather than on some other refusal: the run spent + # its retries on the clean-clone route before escalating. + assert state_of(root)["retry_count"] == MAX_RETRIES + assert runner.calls.count(DOCUMENTER) == MAX_RETRIES + 1 + + +def test_a_documented_claim_the_suite_accepts_completes_the_run( + tmp_path, harness_root, +): + """The control for the run above: the same fixture and the same suite, + with the one sentence the suite rejects replaced by one it does not. A + check that failed every run would be no check.""" + root = build_target(tmp_path / "accepted-doc-target", + test_command=rejecting_command()) + runner = Runner(root, documented="Validation for this lives in the suite.") + + assert story_coordinator.run_story( + STORY_ID, harness_root, root, runner) == 0 + assert (run_dir_of(root) / "completion-report.md").is_file() + + +def test_under_the_previous_order_the_same_claim_completed_the_run( + tmp_path, old_order_harness, +): + """What story-043 shipped, reconstructed: the check passes minutes before + the stage that breaks the suite, and the run completes red.""" + root = build_target(tmp_path / "story-043-old-target", + workflow="previous-order", + test_command=rejecting_command()) + sentence = f"Validation for this lives in {DELETED_MODULE}." + runner = Runner(root, documented=sentence) + + assert story_coordinator.run_story( + STORY_ID, old_order_harness, root, runner) == 0 + + run_dir = run_dir_of(root) + assert (run_dir / "completion-report.md").is_file() + record = json.loads((run_dir / "clean-clone-result.json").read_text( + encoding="utf-8")) + assert record["exit_code"] == 0 + # And the tree the completed run left behind is one that suite rejects. + assert subprocess.run( + ["sh", "-c", f"grep -q {DELETED_MODULE} {ARCHITECTURE_DOC}"], + cwd=root).returncode == 0 diff --git a/tests/test_escalation_resume.py b/tests/test_escalation_resume.py index 705fda2..f43c574 100644 --- a/tests/test_escalation_resume.py +++ b/tests/test_escalation_resume.py @@ -77,6 +77,17 @@ if "revert_check" in s) BASELINE = IMPLEMENTER_STAGE["revert_check"]["baseline"] + +def stages_from(name: str) -> list[str]: + """The stages a run entering at `name` invokes, in workflow order. + + Read off the loaded definition rather than written out, so a reorder of + the stage list - story-045 moved the documenter ahead of the verifier - + changes what a resume is expected to run without these cases stating an + order of their own. + """ + return STAGE_NAMES[STAGE_NAMES.index(name):] + STORY_ID = "story-001" STORY_TITLE = "Sample story for coordinator tests" DEFAULT_BRANCH = "main" @@ -832,7 +843,7 @@ def test_an_escalated_run_resumes_at_the_recorded_stage(target, harness_root): code, resumed = run(target, harness_root, verdicts=[PASS]) assert code == 0 - assert resumed.calls == [VERIFIER_STAGE["name"], "documenter"] + assert resumed.calls == stages_from(VERIFIER_STAGE["name"]) assert state_of(target)["status"] == "completed" @@ -1003,7 +1014,7 @@ def test_a_resumed_run_carries_the_counters_and_preserves_the_attempt( # writes over the copy at the run root, which is why the archive exists. assert (story_coordinator.attempt_dir(run_dir, 2) / "prompt-verifier-attempt-2.md").read_text() == prompt - assert resumed.calls == [VERIFIER_STAGE["name"], "documenter"] + assert resumed.calls == stages_from(VERIFIER_STAGE["name"]) def test_resetting_the_counters_would_have_overwritten_that_evidence( @@ -1135,8 +1146,7 @@ def test_a_stage_argument_overrides_the_recorded_stage(target, harness_root): assert code == 0 assert overridden.calls[0] == RETRY_STAGE - assert overridden.calls == [RETRY_STAGE, "tester", VERIFIER_STAGE["name"], - "documenter"] + assert overridden.calls == stages_from(RETRY_STAGE) elsewhere = build_target(target.parent / "no-override") escalate(elsewhere, harness_root) @@ -1180,7 +1190,7 @@ def test_a_fresh_run_started_at_a_later_stage_records_that_stage( code, started = run(target, harness_root, verdicts=[PASS], start_stage=VERIFIER_STAGE["name"]) assert code == 0 - assert started.calls == [VERIFIER_STAGE["name"], "documenter"] + assert started.calls == stages_from(VERIFIER_STAGE["name"]) elsewhere = build_target(target.parent / "fresh-default") assert run(elsewhere, harness_root, verdicts=[PASS])[1].calls[0] \ @@ -1487,7 +1497,7 @@ def test_the_resumed_stage_comes_from_state_json(target, harness_root): code, resumed = run(target, harness_root) assert code == 0 - assert resumed.calls == ["documenter"] + assert resumed.calls == stages_from("documenter") def test_nothing_routes_on_the_summary_the_archive_or_the_baseline( @@ -1510,7 +1520,7 @@ def test_nothing_routes_on_the_summary_the_archive_or_the_baseline( code, stripped = run(target, harness_root, verdicts=[PASS]) assert code == 0 - assert stripped.calls == [VERIFIER_STAGE["name"], "documenter"] + assert stripped.calls == stages_from(VERIFIER_STAGE["name"]) intact = build_target(target.parent / "intact") escalate(intact, harness_root) diff --git a/tests/test_escalation_summary.py b/tests/test_escalation_summary.py index b4b019b..fe3910f 100644 --- a/tests/test_escalation_summary.py +++ b/tests/test_escalation_summary.py @@ -870,13 +870,17 @@ def test_escalation_reason_returns_the_string_it_returned_before( #: The files an escalation at the retry ceiling leaves at the run-directory #: root. Every one of them predates this story: each section the story added #: renders an artifact that already existed, so the story added no file to -#: this list and none of these names is one it introduced. +#: this list and none of these names is one it introduced. The documenter's +#: two artifacts joined the set in story-045, which introduced neither: the +#: documenter now runs before the verifier, so a run escalating at the retry +#: ceiling has already written them. ESCALATION_RUN_DIRECTORY = { "changed-files.json", "escalation-summary.md", "events.log", "execution-history.json", "implementation-summary.md", "retry-history.json", "state.json", "test-results.json", "tester-changed-files.json", "verification-result.json", "retry-guidance.json", + "documentation-report.md", "documenter-changed-files.json", } #: The rendered prompts, one per stage and attempt. Named by shape rather than diff --git a/tests/test_execution_history.py b/tests/test_execution_history.py index d938481..8b5fe1b 100644 --- a/tests/test_execution_history.py +++ b/tests/test_execution_history.py @@ -214,8 +214,8 @@ def test_the_retried_run_records_both_attempts_in_one_stream(retry_then_pass): started = [e["stage"] for e in history if e["event"] == "stage-started"] assert started == runner.calls assert started == [ - "implementer", "tester", "verifier", - "implementer", "tester", "verifier", "documenter", + "implementer", "tester", "documenter", + "verifier", "implementer", "tester", "documenter", "verifier", ] assert [e["sequence"] for e in history] == list(range(1, len(history) + 1)) # Not a stage output, so the retry archive neither copies nor overwrites it. @@ -270,7 +270,10 @@ def test_a_completed_stage_carries_an_elapsed_duration(retry_then_pass): states it.""" _, run_dir = retry_then_pass completions = [e for e in history_of(run_dir) if e["event"] == "stage-completed"] - assert len(completions) == 5 # implementer and tester twice, documenter + # implementer, tester and documenter twice: since story-045 the + # documenter runs before the verifier, so a failed attempt has + # already completed it. + assert len(completions) == 6 for entry in completions: assert isinstance(entry["duration_seconds"], (int, float)) assert not isinstance(entry["duration_seconds"], bool) @@ -708,7 +711,11 @@ def test_the_retry_ceiling_and_its_counters_are_untouched(escalated): assert state["retry_count"] == 2 assert state["verification_iterations"] == 3 assert runner.calls.count("implementer") == 3 - assert "documenter" not in runner.calls + # The run did not finish. Stated as the completion report's + # absence rather than as the documenter never running: since + # story-045 the documenter runs before the verifier, so an + # escalating run has invoked it. + assert not (run_dir / "completion-report.md").is_file() assert (run_dir / "verification" / "iteration-3.json").is_file() diff --git a/tests/test_foreign_work_refusal.py b/tests/test_foreign_work_refusal.py index e3e57ed..d9ca992 100644 --- a/tests/test_foreign_work_refusal.py +++ b/tests/test_foreign_work_refusal.py @@ -546,7 +546,7 @@ def test_a_gitignored_path_is_not_what_the_check_is_about( #: ends on. Every entry is something the pre-story coordinator produced and #: this one still produces; the point of the check this story adds is that it #: changes none of them when the tree is clean. -CLEAN_RUN_STAGES = ["implementer", "tester", "verifier", "documenter"] +CLEAN_RUN_STAGES = ["implementer", "tester", "documenter", "verifier"] CLEAN_RUN_ARTIFACTS = [ "changed-files.json", "clean-clone-result.json", "escalation-summary.md", "events.log", "execution-history.json", "implementation-summary.md", @@ -557,9 +557,9 @@ def test_a_gitignored_path_is_not_what_the_check_is_about( "workflow started for story-001", "implementer stage started", "implementer stage completed", "tester stage started", "tester stage completed", + "documenter stage started", "documenter stage completed", "verifier stage started", "verification passed", "clean-clone suite passed with the story committed", - "documenter stage started", "documenter stage completed", "story completed on branch story/story-001", ] @@ -971,7 +971,14 @@ def test_a_resumed_escalated_runs_commit_carries_nothing_that_predated_it( assert code == 0 assert runner.calls != [] assert STRAY not in files_in(target) - assert files_in(target) != [] # it did commit something + # It did end on a commit of its own. Stated as the completion commit's + # subject rather than as "the commit holds files": since story-045 the + # verifier is the last stage, so a resume entering at it changes no + # repository file and its completion commit is legitimately empty. The + # non-vacuity this guard is for is unchanged - the reading below still + # finds the stray in the developer's commit. + assert subject_of(target) == story_coordinator.completion_commit_subject( + STORY_ID, STORY_TITLE) assert STRAY in files_in(target, developers) # the control diff --git a/tests/test_planner_injection.py b/tests/test_planner_injection.py index 69bcb32..b9862a4 100644 --- a/tests/test_planner_injection.py +++ b/tests/test_planner_injection.py @@ -514,7 +514,7 @@ def run_script(name: str, *args: str, cwd: Path, env: dict | None = None def test_the_workflow_defines_the_four_expected_stages(): """Anchor the data-driven assertions below to the stages the acceptance criteria name, so an accidentally emptied workflow cannot vacuously pass.""" - assert stage_names() == ["implementer", "tester", "verifier", "documenter"] + assert stage_names() == ["implementer", "tester", "documenter", "verifier"] assert declared_restrictions() == [("implementer", "tests/")] assert rules()["blocked_paths"] == [".git/", ".harness/runs/", "rules/"] diff --git a/tests/test_rerun_refusal.py b/tests/test_rerun_refusal.py index 465caf3..f602454 100644 --- a/tests/test_rerun_refusal.py +++ b/tests/test_rerun_refusal.py @@ -605,7 +605,7 @@ def test_a_branch_that_exists_but_never_finished_still_runs( #: The artifacts are required rather than exhaustive, per the standing rule #: that a run directory is not asserted as an exact set: every story that adds #: an artifact would otherwise fail an assertion about something else. -FIRST_RUN_STAGES = ["implementer", "tester", "verifier", "documenter"] +FIRST_RUN_STAGES = ["implementer", "tester", "documenter", "verifier"] FIRST_RUN_ARTIFACTS = [ "changed-files.json", "clean-clone-result.json", "completion-report.md", "events.log", "execution-history.json", "implementation-summary.md", @@ -616,9 +616,9 @@ def test_a_branch_that_exists_but_never_finished_still_runs( f"workflow started for {STORY_ID}", "implementer stage started", "implementer stage completed", "tester stage started", "tester stage completed", + "documenter stage started", "documenter stage completed", "verifier stage started", "verification passed", "clean-clone suite passed with the story committed", - "documenter stage started", "documenter stage completed", f"story completed on branch {STORY_BRANCH}", ] diff --git a/tests/test_retry_history.py b/tests/test_retry_history.py index 476e68e..9b347c0 100644 --- a/tests/test_retry_history.py +++ b/tests/test_retry_history.py @@ -345,9 +345,10 @@ def test_the_file_appears_at_the_first_retry_and_not_before(retry_then_pass): did.""" runner, _ = retry_then_pass assert runner.history_seen == [ - ("implementer", False), ("tester", False), ("verifier", False), - ("implementer", True), ("tester", True), ("verifier", True), - ("documenter", True), + ("implementer", False), ("tester", False), + ("documenter", False), ("verifier", False), + ("implementer", True), ("tester", True), + ("documenter", True), ("verifier", True), ] @@ -694,7 +695,10 @@ def test_the_retry_ceiling_and_its_counters_are_unchanged(retries_exhausted): assert state["retry_count"] == MAX_RETRIES == 2 assert state["verification_iterations"] == MAX_RETRIES + 1 assert runner.calls.count(RETRY_STAGE) == MAX_RETRIES + 1 - assert "documenter" not in runner.calls + # The run did not finish. Stated as the completion report's + # absence rather than as the documenter never running: since + # story-045 the documenter runs before the verifier. + assert not (run_dir / "completion-report.md").is_file() assert (run_dir / "escalation-summary.md").is_file() diff --git a/tests/test_retry_routing.py b/tests/test_retry_routing.py index 77f066d..84ff0d1 100644 --- a/tests/test_retry_routing.py +++ b/tests/test_retry_routing.py @@ -921,17 +921,20 @@ def test_the_placeholder_check_sees_a_placeholder_when_there_is_one(): assert "{{retry_routes}}" in template -def test_a_third_category_changes_the_prompt_with_no_edit_to_the_template( +def test_a_further_category_changes_the_prompt_with_no_edit_to_the_template( tmp_path, harness_root, target, ): """The routes are injected, not restated. - A workflow with a third category renders a verifier prompt naming it, - while `prompts/verifier.md` is byte-identical in both harness roots. + A workflow with one more category than the shipped one renders a verifier + prompt naming it, while `prompts/verifier.md` is byte-identical in both + harness roots. The added name is whatever the shipped workflow lacks: + it was `documentation` until story-045 shipped that category, and the + subject is the injection rather than any particular category. """ - added = "documentation" + added = "tooling" assert added not in ROUTES, "pick a category the shipped workflow lacks" - when = "the defect is in the documentation this story was to leave behind" + when = "the defect is in the harness tooling this story was to leave behind" def mutate(workflow: dict) -> None: verifier_stage_of(workflow)["on_failure"]["retry_routing"][added] = { diff --git a/tests/test_revert_baseline.py b/tests/test_revert_baseline.py index ea65660..dd8e6bc 100644 --- a/tests/test_revert_baseline.py +++ b/tests/test_revert_baseline.py @@ -615,8 +615,8 @@ def test_a_forced_edit_to_a_file_created_earlier_in_the_run_is_permitted( code, runner = run(target, harness_root, RETRY_SHAPE, [FAIL, PASS]) assert code == 0 assert state_of(target)["status"] == "completed" - assert runner.calls == ["implementer", "tester", "verifier", - "implementer", "tester", "verifier", "documenter"] + assert runner.calls == ["implementer", "tester", "documenter", + "verifier", "implementer", "tester", "documenter", "verifier"] record = record_of(target) assert record["ran"] is True @@ -653,7 +653,8 @@ def test_an_additive_edit_to_a_file_created_earlier_in_the_run_is_escalated( ) assert code == 2 assert state_of(target)["status"] == "escalated" - assert runner.calls == ["implementer", "tester", "verifier", "implementer"] + assert runner.calls == ["implementer", "tester", "documenter", + "verifier", "implementer"] record = record_of(target) assert record["ran"] is True diff --git a/tests/test_revert_check.py b/tests/test_revert_check.py index 3c83036..d0a3b38 100644 --- a/tests/test_revert_check.py +++ b/tests/test_revert_check.py @@ -499,7 +499,7 @@ def test_a_forced_edit_under_the_governed_prefix_is_permitted(target, harness_ro code, runner = run(target, harness_root, {"implementer": forced_repair}) assert code == 0 assert state_of(target)["status"] == "completed" - assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + assert runner.calls == ["implementer", "tester", "documenter", "verifier"] record = record_of(target) assert record["ran"] is True diff --git a/tests/test_shared_baseline_resolution.py b/tests/test_shared_baseline_resolution.py index d9cfc33..7b72746 100644 --- a/tests/test_shared_baseline_resolution.py +++ b/tests/test_shared_baseline_resolution.py @@ -70,11 +70,16 @@ "tests/test_story_010_validation.py": "tests/test_attempt_archiving.py", } -#: The two test names story-038's merge had to change, because story-008 and -#: story-009 each shipped a test of that name and the merged module can hold -#: only one of each. Both survive, under story-009's renamed spelling, and -#: the name-set comparisons below map through this rather than dropping -#: either side of the collision. +#: The test names a later story had to change, mapped from the spelling the +#: origin shipped to the one the module carries now. Two are story-038's +#: merge, because story-008 and story-009 each shipped a test of that name and +#: the merged module can hold only one of each. Two are story-045's, which +#: moved the documenter ahead of the verifier: an attempt-1 archive now holds +#: eight artifacts rather than six, and the documenter's report is no longer +#: the artifact a failed attempt did not write, so each of those two cases was +#: renamed for what it now checks rather than left describing what it used to. +#: Every one of them survives under its new spelling, and the name-set +#: comparisons below map through this rather than dropping either side. MERGE_RENAMES = { "tests/test_story_009_validation.py": { "test_the_rendered_prompt_has_no_leftover_placeholder": @@ -82,6 +87,12 @@ "test_the_coverage_comes_from_the_injection_and_not_from_leftover_prose": "test_the_workflow_fact_coverage_comes_from_the_injection_not_leftover_prose", }, + "tests/test_story_010_validation.py": { + "test_attempt_1_archive_holds_the_six_artifacts_under_canonical_names": + "test_attempt_1_archive_holds_every_stage_artifact_under_canonical_names", + "test_an_artifact_the_attempt_did_not_write_is_skipped": + "test_the_documenters_artifacts_are_archived_with_the_attempt", + }, } diff --git a/tests/test_single_story_reader.py b/tests/test_single_story_reader.py index 48f9da6..32fe68e 100644 --- a/tests/test_single_story_reader.py +++ b/tests/test_single_story_reader.py @@ -151,7 +151,7 @@ def complete_run(target_root: Path, harness_root: Path) -> Path: assert story_coordinator.run_story( "story-001", harness_root, target_root, runner ) == 0 - assert runner.stages == ["implementer", "tester", "verifier", "documenter"] + assert runner.stages == ["implementer", "tester", "documenter", "verifier"] return run_dir @@ -450,7 +450,7 @@ def test_the_prompt_carries_the_story_file_byte_for_byte(target_root, harness_ro story_path = install(target_root, AWKWARD_STORY) run_dir = complete_run(target_root, harness_root) on_disk = story_path.read_text(encoding="utf-8") - for name in ("implementer", "tester", "verifier", "documenter"): + for name in ("implementer", "tester", "documenter", "verifier"): prompt = (run_dir / f"prompt-{name}-attempt-1.md").read_text(encoding="utf-8") assert on_disk in prompt, name diff --git a/tests/test_stage_baseline.py b/tests/test_stage_baseline.py index f98e39a..7e2b09f 100644 --- a/tests/test_stage_baseline.py +++ b/tests/test_stage_baseline.py @@ -605,7 +605,8 @@ def test_the_pre_story_code_escalates_on_the_same_run(target, harness_root, tmp_ assert code == 2 assert state_of(target)["status"] == "escalated" - assert runner.calls == ["implementer", "tester", "verifier", "implementer"] + assert runner.calls == ["implementer", "tester", "documenter", + "verifier", "implementer"] record = record_of(target) assert record["ran"] is True @@ -642,8 +643,8 @@ def test_the_second_attempts_edit_to_a_file_it_also_edited_is_permitted( assert code == 0 assert state_of(target)["status"] == "completed" - assert runner.calls == ["implementer", "tester", "verifier", - "implementer", "tester", "verifier", "documenter"] + assert runner.calls == ["implementer", "tester", "documenter", + "verifier", "implementer", "tester", "documenter", "verifier"] record = record_of(target) assert record["ran"] is True @@ -690,7 +691,8 @@ def test_a_retry_editing_a_path_it_did_not_touch_before_is_still_escalated( assert code == 2 assert state_of(target)["status"] == "escalated" - assert runner.calls == ["implementer", "tester", "verifier", "implementer"] + assert runner.calls == ["implementer", "tester", "documenter", + "verifier", "implementer"] record = record_of(target) assert record["ran"] is True diff --git a/tests/test_stage_output_ownership.py b/tests/test_stage_output_ownership.py index 9d486c9..da760b2 100644 --- a/tests/test_stage_output_ownership.py +++ b/tests/test_stage_output_ownership.py @@ -281,7 +281,7 @@ def test_a_stage_that_declares_nothing_may_create_under_the_prefix(target_root, }) code = story_coordinator.run_story("story-001", harness_root, target_root, runner) assert code == 0 - assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + assert runner.calls == ["implementer", "tester", "documenter", "verifier"] # -------------------------------------------------------------------------- diff --git a/tests/test_story_coordinator.py b/tests/test_story_coordinator.py index 8c51dc2..13a67c9 100644 --- a/tests/test_story_coordinator.py +++ b/tests/test_story_coordinator.py @@ -88,7 +88,7 @@ def test_happy_path_completes(target_root, harness_root): state = read_state(target_root) assert state["status"] == "completed" assert state["retry_count"] == 0 - assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + assert runner.calls == ["implementer", "tester", "documenter", "verifier"] run_dir = target_root / ".harness" / "runs" / "story-001" assert (run_dir / "completion-report.md").is_file() assert (run_dir / "verification" / "iteration-1.json").is_file() @@ -104,8 +104,8 @@ def test_verification_failure_retries_then_completes(target_root, harness_root): assert state["status"] == "completed" assert state["retry_count"] == 1 assert runner.calls == [ - "implementer", "tester", "verifier", - "implementer", "tester", "verifier", "documenter", + "implementer", "tester", "documenter", + "verifier", "implementer", "tester", "documenter", "verifier", ] run_dir = target_root / ".harness" / "runs" / "story-001" assert (run_dir / "verification" / "iteration-2.json").is_file() @@ -390,7 +390,7 @@ def test_invalid_verifier_artifact_escalates_without_a_retry(target_root, harnes code = story_coordinator.run_story("story-001", harness_root, target_root, runner) assert code == 2 assert read_state(target_root)["retry_count"] == 0 - assert runner.calls == ["implementer", "tester", "verifier"] + assert runner.calls == ["implementer", "tester", "documenter", "verifier"] summary = (target_root / ".harness" / "runs" / "story-001" / "escalation-summary.md").read_text() assert "retry-guidance.json" in summary assert "$.retry_scope" in summary diff --git a/tests/test_undeclared_config_keys.py b/tests/test_undeclared_config_keys.py index 685b72a..d73f37b 100644 --- a/tests/test_undeclared_config_keys.py +++ b/tests/test_undeclared_config_keys.py @@ -371,7 +371,7 @@ def test_the_same_fixture_without_the_key_creates_all_five( encoding="utf-8"))["status"] == "completed" assert (sound_target / ".harness" / "logs" / f"{STORY_ID}.log").is_file() assert branches(sound_target) - before == {f"story/{STORY_ID}"} - assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + assert runner.calls == ["implementer", "tester", "documenter", "verifier"] def test_removing_the_offending_key_from_a_refused_target_lets_it_run( @@ -388,7 +388,7 @@ def test_removing_the_offending_key_from_a_refused_target_lets_it_run( code, runner, _ = run(sound_target, harness_root) assert code == 0, runner.calls - assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + assert runner.calls == ["implementer", "tester", "documenter", "verifier"] def test_several_undeclared_keys_are_all_named_in_one_refusal( @@ -429,7 +429,7 @@ def test_a_comment_naming_a_retired_or_unknown_key_is_not_refused( code, runner, _ = run(sound_target, harness_root) assert code == 0, runner.calls - assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + assert runner.calls == ["implementer", "tester", "documenter", "verifier"] def test_the_same_line_without_its_comment_marker_is_refused( @@ -649,7 +649,7 @@ def test_a_freshly_initialised_target_can_run_a_story( code, runner, _ = run(target_root, harness_root) assert code == 0, (runner.calls, result.stdout) - assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + assert runner.calls == ["implementer", "tester", "documenter", "verifier"] #: The declared set, read once, for deciding which runs of lines in a test diff --git a/workflows/story-workflow.json b/workflows/story-workflow.json index b289f46..fa3ef32 100644 --- a/workflows/story-workflow.json +++ b/workflows/story-workflow.json @@ -26,6 +26,15 @@ "tester-changed-files.json": "changed-files" } }, + { + "name": "documenter", + "prompt": "documenter.md", + "outputs": ["documentation-report.md", "documenter-changed-files.json"], + "changed_files": "documenter-changed-files.json", + "schemas": { + "documenter-changed-files.json": "changed-files" + } + }, { "name": "verifier", "prompt": "verifier.md", @@ -48,18 +57,13 @@ "validation": { "stage": "tester", "when": "the defect is in the tests themselves: a wrong, missing or fragile assertion, an assertion that cannot fail, or coverage that does not exercise what it claims" + }, + "documentation": { + "stage": "documenter", + "when": "the defect is in the documentation itself: it describes behaviour the code does not have, names a file, module or symbol that does not exist, contradicts what the run's own artifacts record, or omits a change the story required be written down. Judge the document against the code as it now stands: a document that accurately describes wrong behaviour is an implementation defect, not a documentation one, and belongs to the category that owns the code" } } } - }, - { - "name": "documenter", - "prompt": "documenter.md", - "outputs": ["documentation-report.md", "documenter-changed-files.json"], - "changed_files": "documenter-changed-files.json", - "schemas": { - "documenter-changed-files.json": "changed-files" - } } ], "escalation_rules": {