From fbf1e874f6bdc23c8fbcc91b9927c6d855cc4e60 Mon Sep 17 00:00:00 2001 From: "jerod.wilkerson" <30474318+jerodw@users.noreply.github.com> Date: Tue, 15 Sep 2026 12:12:01 -0600 Subject: [PATCH 1/3] Record the mandate conferred for story-150 --- .harness/history/mandate-log.jsonl | 1 + 1 file changed, 1 insertion(+) diff --git a/.harness/history/mandate-log.jsonl b/.harness/history/mandate-log.jsonl index 9341203..1842700 100644 --- a/.harness/history/mandate-log.jsonl +++ b/.harness/history/mandate-log.jsonl @@ -55,3 +55,4 @@ {"story_id": "story-147", "timestamp": "2026-09-14 17:45:08", "conferred_by": "jerod.wilkerson <30474318+jerodw@users.noreply.github.com>", "source_kind": "human", "recorded_by": "l5-plan", "discarded_session_block": false} {"story_id": "story-148", "timestamp": "2026-09-14 20:41:55", "conferred_by": "jerod.wilkerson <30474318+jerodw@users.noreply.github.com>", "source_kind": "human", "recorded_by": "l5-plan", "discarded_session_block": false} {"story_id": "story-149", "timestamp": "2026-09-15 08:36:13", "conferred_by": "jerod.wilkerson <30474318+jerodw@users.noreply.github.com>", "source_kind": "human", "recorded_by": "l5-plan", "discarded_session_block": false} +{"story_id": "story-150", "timestamp": "2026-09-15 12:12:00", "conferred_by": "jerod.wilkerson <30474318+jerodw@users.noreply.github.com>", "source_kind": "human", "recorded_by": "l5-plan", "discarded_session_block": false} From 8ce6c8e4501df76fdbe873d9d3103bb85d6cda51 Mon Sep 17 00:00:00 2001 From: "jerod.wilkerson" <30474318+jerodw@users.noreply.github.com> Date: Tue, 15 Sep 2026 12:12:01 -0600 Subject: [PATCH 2/3] Plan story-150: An interrupted planning session is judged on what it committed, whatever order its prompts were answered in --- .harness/stories/story-150.yaml | 137 ++++++++++++++++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 .harness/stories/story-150.yaml diff --git a/.harness/stories/story-150.yaml b/.harness/stories/story-150.yaml new file mode 100644 index 0000000..fa49aba --- /dev/null +++ b/.harness/stories/story-150.yaml @@ -0,0 +1,137 @@ +story: + id: story-150 + title: An interrupted planning session is judged on what it committed, whatever order its prompts were answered in + description: | + Planned from the brief filed under 298. + + `tests/test_plan_commit.py::test_an_interrupt_still_commits_what_was_written_and_exits_130` + fails under load and passes on a quiet machine. It failed in a full + `-n auto` run of the suite at `tests/test_plan_commit.py:996` -- + `assert status == 130` -- and the same suite passes that case when it is + not competing for a machine. It is 31.05 s, the fourth most expensive test + in the suite. This is the third timing-dependent failure this week, after + the escaped-descendant case story-149 repaired and the family that waits + out a killed child; each one costs a retry when it lands in a clean-clone + check, which is where the first of them cost story-148 an attempt and about + thirty minutes. + + What the case exists to establish is in its title and is not in doubt: an + interrupted session still commits what it had written. That half is + asserted at `:997` and is not what failed. + + The case drives a sequence and then trusts the order it happened in. It + starts a planning session on a pty with a stub told to sleep thirty + seconds, waits for the artifact to appear, sends `SIGINT` to the process + group at `:985`, and only then writes two answers into the pty behind it at + `:992` -- the approval, and an `n` declining the run offer. Nothing makes + those two writes land before the session needs them, so the status the case + asserts rests on an interleaving the scheduler chooses. + + Two facts sharpen where the repair has to go. First, the answers cannot be + what produces the 130: `scripts/l5-plan` ends with + `sys.exit(status or committing)` and an interrupted session sets + `status = 130`, so the approval and the declined offer decide whether the + artifact is committed and whether the process finishes at all, never the + status. Second, the brief's other route -- establish that the session + reached the approval prompt before the signal was sent -- is not available + here, because that prompt is only reached once the interrupt has ended the + session. It is downstream of the signal, not upstream of it. What is left + is to give the session every answer it needs before the signal exists as a + possibility, which is what `tests/conftest.py`'s `a_terminal_for_stdin` + already does for every other module that drives this script through a + terminal. + + The mechanism is established before it is repaired rather than assumed. The + brief's confidence is medium: the failing run captured the assertion and + not the pty transcript, so which write lost the race is inferred. Reading + the case raises a second doubt beside it -- the stub sleeps thirty seconds + and the case costs 31.05 s, which says the sleep runs close to completion + even when the case passes, and that is not what the comment at `:986` + describes. So the first work is to watch it: the pty transcript, the stub's + own fate, the status `l5-plan` exits with and where the wall-clock goes, + on a quiet machine and under a loaded one. + + The developer has asked that whatever that observation finds be repaired in + one pass. If the non-determinism is in the test's ordering, the ordering is + what changes. If it is in `scripts/l5-plan` -- an interrupt landing + somewhere the script does not handle, so the status a developer's own + terminal would see genuinely varies -- then the script is repaired too, and + the case is left resting on a determinism that is now real rather than + arranged around one that is not. The behaviour the case asserts is correct + either way and is not what changes: an interrupted session commits what it + wrote and exits 130. + + workflow: story-workflow + +tasks: + - Establish what actually happens when the process group is interrupted - capture the pty transcript, the stub's own exit, the status l5-plan exits with, and where the case's wall-clock goes - on an unloaded machine and with the machine loaded, so the interleaving is observed rather than inferred from the assertion that failed. + - Give run_plan_on_a_pty an optional reply that is written into the master before the child is started, on the reasoning tests/conftest.py's a_terminal_for_stdin already records, leaving the default behaviour of the helper exactly as it is. + - Rewrite test_an_interrupt_still_commits_what_was_written_and_exits_130 so every answer the session needs is in the pty before the interrupt is sent and nothing is written after it, keeping both of its assertions - the 130 and the commit holding exactly the artifact. + - Rewrite that case's comments to say what the investigation observed rather than what the previous comment assumed, including where the case's cost goes, so the next reader has the transcript's account and does not reach for the stub's sleep. + - If and only if the investigation shows the varying status comes from scripts/l5-plan rather than from the order the test wrote in, repair it there so an interrupt reaching the script during any part of its post-session work ends in the same status, and cover that repair with a case that drives the newly handled point. + +acceptance_criteria: + - test_an_interrupt_still_commits_what_was_written_and_exits_130 writes nothing into the pty after the signal is sent; every input the session and the script need is in the terminal before the process is started. + - That case still asserts both halves - the exit status is 130, and the plan commit holds exactly the one artifact the session wrote - and neither assertion is weakened, made conditional, or replaced by a looser one. + - That case's comments state what was observed of the interrupt - the stub's fate, the status the script exits with, and where the case's wall-clock goes - rather than restating the account the previous comment gave. + - run_plan_on_a_pty called as its existing callers call it behaves exactly as it does today; the case at tests/test_plan_commit.py:965 and the caller in tests/test_the_approval_is_observed.py, which writes its answers late on purpose because it is measuring a hold, are unchanged in behaviour. + - No assertion in the rewritten case passes only when one event beats another on a busy machine, and no precondition the case cannot control is reported as a failure - an unmet one reports through machine_load.inconclusive, which is the only route tests/test_no_test_bounds_the_machine.py permits. + - tests/test_no_test_bounds_the_machine.py still classifies test_plan_commit.py as CARRIES_PRECONDITIONS and the module still satisfies that classification. + - The stub's thirty-second sleep is unchanged, and no assertion in the case depends on the session being interrupted within any wall-clock window. + - If scripts/l5-plan is changed, an interrupt arriving at the point the investigation found is covered by a test that drives that point, and the status such an interrupt ends in is the same 130 the case asserts. + - If scripts/l5-plan is not changed, the story says in the case's comments what the investigation established about the script, so the reader can see the question was asked and answered rather than passed over. + - The behaviour under test is unchanged - an interrupted planning session still commits what it had written, still pushes it, and still exits 130 - and no test asserting that is deleted or relaxed. + +technical_plan: + implementation_steps: + - Read the case at tests/test_plan_commit.py:972-997, the helper at :819, drain at :840, and scripts/l5-plan:310-345 where the session's status is decided, so what the answers actually decide is taken from the source rather than from the case's comment. + - Run the case with the pty transcript kept and with the stub's exit observable - what the stub did with the SIGINT, whether subprocess.run's own interrupt handling waited out the sleep, and what l5-plan printed before exiting - and repeat it with the machine loaded. Write down the status seen in each case. This is what decides whether the following steps touch the script. + - Add an optional pre-loaded reply to run_plan_on_a_pty, written to the master between pty.openpty and Popen, defaulting to writing nothing so existing callers are untouched, with a docstring saying why a reply that must be waiting cannot be written behind the child. + - Rewrite the interrupt case to pass the approval and the declined offer through that parameter, drop the os.write at :992, and leave the killpg where it is. Rewrite the comments around it to the investigation's account. + - If the investigation put the varying status in scripts/l5-plan, repair it there - the try that catches KeyboardInterrupt covers only the session subprocess, so an interrupt arriving while the script is committing, stamping, validating or prompting is not handled by it - and add a case covering the point that was found, in the module that already covers this script's interrupt behaviour. + - Re-run the case repeatedly alongside a loaded machine to confirm the status no longer varies, and record in the module what was run to confirm it. + likely_file_changes: + - file: tests/test_plan_commit.py + stage: tester + reason: Holds the interrupt case being rewritten, the pty helper gaining the optional pre-loaded reply, and the comments recording what the investigation observed. + - file: scripts/l5-plan + stage: implementer + reason: Changed only if the investigation shows the varying exit status comes from the script's own handling of an interrupt rather than from the order the test wrote its answers in. Left untouched otherwise. + +scope: + modify: + - tests/test_plan_commit.py + - scripts/l5-plan + do_not_modify: + - tests/test_the_approval_is_observed.py + - tests/test_no_test_bounds_the_machine.py + - orchestration/plan_mandate.py + - orchestration/plan_commit.py + - prompts/ + - schemas/ + - workflows/ + +verification_requirements: + - Confirm no input the session or the script needs is written into the pty after the signal is sent, so the case's result does not depend on a write winning a race. + - Confirm both assertions the case exists for are still present and unweakened - the 130 and the commit holding exactly the one artifact. + - Confirm the case's comments record what was observed of the interrupt rather than what was assumed of it, and that they account for the case's cost so a later reader does not reach for the stub's sleep. + - Confirm existing callers of run_plan_on_a_pty are unaffected, including the one in tests/test_the_approval_is_observed.py that writes its answers late deliberately. + - Confirm the module is still classified CARRIES_PRECONDITIONS in tests/test_no_test_bounds_the_machine.py and still satisfies that classification, with any unmet precondition reported through machine_load.inconclusive. + - Confirm that where scripts/l5-plan was changed there is a test driving the point that was repaired, and that where it was not changed the case's comments say what the investigation established about it. + - Confirm the full suite passes under the configured test command. + +constraints: + - The behaviour under test is correct and is not in question - an interrupted session commits what it wrote and exits 130 - so no change may alter it. + - The stub's thirty-second sleep stays as it is; it is what keeps the session alive to be interrupted, and shortening it would tighten the race rather than remove it. + - No assertion may be deleted, skipped or weakened to make the case pass, and no bound may be placed on how fast the machine has to be. + - A precondition the test cannot control is reported through machine_load.inconclusive and by no other route. + - Any change to scripts/l5-plan is confined to how an interrupt is handled; nothing about what the script commits, stamps, validates or pushes changes. + +mandate: + source: + kind: human + conferred_at: 2026-09-15 12:12:00 + conferred_by: jerod.wilkerson <30474318+jerodw@users.noreply.github.com> + recorded_by: l5-plan + +brief_key: 298 From ad2effca02fa50ee4c645293c52195562290943a Mon Sep 17 00:00:00 2001 From: "jerod.wilkerson" <30474318+jerodw@users.noreply.github.com> Date: Tue, 15 Sep 2026 12:47:40 -0600 Subject: [PATCH 3/3] story-150: An interrupted planning session is judged on what it committed, whatever order its prompts were answered in Implemented by the l5 harness story workflow. --- .harness/docs/ARCHITECTURE.md | 10 +- .harness/history/execution-log.jsonl | 1 + .harness/history/inspection-log.jsonl | 1 + scripts/l5-plan | 78 ++++++++---- tests/test_plan_commit.py | 170 ++++++++++++++++++++++++-- 5 files changed, 224 insertions(+), 36 deletions(-) diff --git a/.harness/docs/ARCHITECTURE.md b/.harness/docs/ARCHITECTURE.md index 365eb35..4480fb4 100644 --- a/.harness/docs/ARCHITECTURE.md +++ b/.harness/docs/ARCHITECTURE.md @@ -211,7 +211,7 @@ Thin entry points only; no orchestration logic. `l5-init`, `l5-plan`, `l5-run`, `l5-plan` is the exception to "the coordinator renders prompts": the planner is not a workflow stage, so the script loads `prompts/planner.md` with `context_assembler.load_template`, renders it with `context_assembler.render`, and passes the rendered text to `--append-system-prompt`. The render context is `schema_context(HARNESS_ROOT)` merged with `workflow_context(workflow, rules)`: since story-009, `l5-plan` locates the target repository like every other run script (`find_target_root`, same no-config refusal, no session started), loads the target's config, loads a workflow and the execution rules from HARNESS_ROOT, and injects the stage list, per-stage create restrictions, and blocked paths alongside the story schema. Since story-069 *which* workflow is `[--workflow ]`, parsed ahead of the request text the way `--base` is; the script also sets `workflow_name` in the render context from the name it passed to `load_workflow`, so the prompt cannot name a workflow other than the one it was rendered against, and a name with no definition refuses before the session starts. See "A run executes the workflow its work item names" below. The flag used to default to the workflow the config names; since story-072 it does not, and **the script reads the configured `workflow` key on no path** — without the flag the name comes from a proposal a developer confirmed, and without a terminal to confirm at, the invocation is refused. See "The planner proposes the workflow it plans for" below. Since story-096 the same loop also parses `--brief `, which names a brief to plan *instead of* giving the request as text and is refused in combination with it; where it is given, the request the session receives is what `brief_fetch.render` made of the fetched brief and the workflow decision has a proposal already — see "l5-plan plans from a brief" below. Requiring a target was not a loss — a planner that cannot see the project cannot list `.harness/stories/` to assign the next story number either. All lookup, loading, and rendering stays in orchestration; the script wires them together. Since story-104 `l5-assist` renders `prompts/assist.md` the same way, through the same `load_template`/`render` pair, so the shared prose partial reaches it too — see "A second shared partial joined it" above. Since story-099 `l5-assist` also passes `--plugin-dir` naming the shipped `plugin/` directory and exports `L5_HARNESS_ROOT` before the exec, so the session it starts has the harness's skills and can name an entry point without a path relative to wherever the developer was standing. It keeps its `os.execvp`: it has nothing to do after the session, which is what `l5-plan` gave the exec up for. -Since story-023 the script no longer ends in `os.execvp`. It runs the session as a **child process with stdin, stdout and stderr inherited** — the developer's terminal is the session's terminal — so the script outlives the session and can commit what the session wrote. The argument list handed to `claude` is byte for byte what the exec passed, and everything before the session (target-root discovery, config loading, workflow and rules loading, the render context, the rendered planner prompt) is unchanged; the exec was replaced, not the script around it. The session's exit status becomes the script's, a child reaped with a negative return code is translated to the shell's `128 + signal`, and a `KeyboardInterrupt` in the parent is 130 — so both routes to an interrupt exit 130 and both still commit an artifact already on disk. The only new logic in the script is a `report()` function that turns `plan_commit`'s return values into stdout lines and a status; the snapshot, the commit, the push and the remote resolution live in `orchestration/plan_commit.py`, per "thin entry points only". `l5-run`, `l5-status`, `l5-assist` and `l5-init` keep the process model they had. +Since story-023 the script no longer ends in `os.execvp`. It runs the session as a **child process with stdin, stdout and stderr inherited** — the developer's terminal is the session's terminal — so the script outlives the session and can commit what the session wrote. The argument list handed to `claude` is byte for byte what the exec passed, and everything before the session (target-root discovery, config loading, workflow and rules loading, the render context, the rendered planner prompt) is unchanged; the exec was replaced, not the script around it. The session's exit status becomes the script's, a child reaped with a negative return code is translated to the shell's `128 + signal`, and a `KeyboardInterrupt` in the parent is 130 — so both routes to an interrupt exit 130 and both still commit an artifact already on disk. **A third landing was missed until story-150, and it is worth reading before adding another act to this script.** The `try` that caught `KeyboardInterrupt` covered the session subprocess alone, so an interrupt arriving in everything the script does *afterwards* — committing, stamping, validating, publishing, or waiting at either of the script's own prompts — left `main` through an uncaught `KeyboardInterrupt`, CPython restored the default handler and re-raised, and the process **died by the signal rather than exiting**. A shell shows 130 for both ends, which is why it stood from story-023 to story-150 unnoticed; anything that reads the process's return code instead — a test, a script, the coordinator — sees `-SIGINT` for one and `130` for the other. The status normalisation and the `report(...)` call are now inside a second `try` whose `except KeyboardInterrupt` prints one line and exits `INTERRUPTED_EXIT_CODE`, which is where 130 is named once for both sites. Nothing on that path is undone, retried or completed: whatever the acts above the interrupt had already committed, stamped, pushed or published stands as they left it, and the acts below it did not happen — which is what dying by the signal already meant. The guard is at the call site in `main` rather than inside `report`, so the scan that holds `report` to writing and removing nothing (below) still reads the same function. **The general point for a later act: an act added inside `report` inherits this handling, and one added after `sys.exit` would not exist to inherit it — but an act added *outside* both `try` blocks would reopen exactly this hole.** The only new logic in the script is a `report()` function that turns `plan_commit`'s return values into stdout lines and a status; the snapshot, the commit, the push and the remote resolution live in `orchestration/plan_commit.py`, per "thin entry points only". `l5-run`, `l5-status`, `l5-assist` and `l5-init` keep the process model they had. Outliving the session is what made **plan-time validation** possible, and story-025 put it between the two acts the script already performed. `report()` takes the loaded workflow's `stages` (which `main` already had) and calls `plan_validation.artifact_problems` after `plan_commit.new_artifacts` and before `plan_commit.commit_artifacts`, so an artifact that pre-flight would refuse is never committed. On problems it prints each artifact's problems through `story_coordinator.refuse_bad_story`, says it committed nothing and that the artifacts remain in the working tree, and returns 1 — `sys.exit(status or committing)` is unchanged, so the session's own status still wins and an interrupt still exits 130. It **refuses rather than warns**, for all three classes: a warning at plan time is a warning nobody sees, because the session that could have repaired it has ended. Nothing is committed, pushed, deleted or rewritten on that path, and no automatic repair is attempted — the developer must see the problems and decide, and the artifact stays exactly where the session wrote it, which is the state to fix and re-run from. When one of several new artifacts fails, **none** is committed: the commit is one commit for the session's artifacts and a partial one would invent a split the session did not have. The no-new-artifact early return above it is untouched — a session that wrote nothing still says it committed nothing and exits on the session's own status. The script gained no decision and no composition; both live in `plan_validation`. **The check holding that guarantee reads `report` rather than the whole script, since story-076.** It scans for `.unlink(`, `.write_text(`, `shutil.` and `os.remove`, and scanning the whole file was a sound proxy for "a refused artifact is left as the session wrote it" only while the script wrote and removed nothing *anywhere* — which ended when phase one's answer and transcript moved into the script's own hands. The subject is now the function that validates and refuses, resolved through `conftest.function_source`; the pattern set, the strictness and the non-vacuity control are unchanged. A later story adding a write elsewhere in `l5-plan` should scope the check to the function whose guarantee it is rather than widen the pattern set, for the same reason: a whole-file scan over a file that legitimately writes reports the wrong thing in both directions. story-030 put a second check in the same gap, immediately below validation and still above the commit: the base check, which asks whether the branch about to receive the plan artifact is the base at all. Same shape, same refusal helper, same "committed nothing, the artifacts remain in the working tree" line — see "The base a story branches from" below. There is deliberately **no `l5-validate`** and no separate command a developer or the planner agent must remember to run: forgetting it returns the harness to the behaviour the story removed, which is also why `planner.md` gained no step telling the planner to validate its own output — the prompt paragraph is a courtesy and the script's check is the enforcement. @@ -1426,6 +1426,14 @@ The section above left the suite parallel and said that a test failing under loa **That rewrite needs a control, because the assertion it replaced was doing two jobs.** An outcome carrying an exit status is consistent with a helper that waits on the fetch's *output* too, unless something shows the two waits coming apart. So `test_that_same_subject_waited_on_through_its_output_does_time_out` drives the identical subject — written once by `escapes_into_its_own_session` rather than reproduced, so "identical" is a fact rather than a resemblance — through `Popen` and `communicate(timeout=...)` under the short bound, and observes the expiry. Its soundness is load-independent for the same reason the kill halves are: the escaped descendant holds the inherited pipes far past that bound, and a loaded machine holds them longer. The `CLASSIFICATION` entry for the module stays `BOUNDS_ONLY_SLOWNESS` and its comment now argues this pair as well as the kill cases, because that comment is where a later reader weighs the classification. +**A fourth route is to remove the race rather than to decide around it, and story-150 is the pattern.** `tests/test_plan_commit.py::test_an_interrupt_still_commits_what_was_written_and_exits_130` drove a sequence and then trusted the order it happened in: it signalled the process group and only *then* wrote the two answers the interrupted invocation needs into the pty behind it, so the result rested on an interleaving the scheduler chose. Neither a bound nor an artifact was the subject here — the answers were simply written too late to be certain of arriving, and an answer already sitting in a terminal is there whenever the child looks, with nothing about when it looks able to change what it finds. `run_plan_on_a_pty` therefore takes an optional `reply` written into the master **between `pty.openpty` and `Popen`**, defaulting to writing nothing so every existing caller starts a child with an empty terminal exactly as before — including the one in `tests/test_the_approval_is_observed.py`, which writes its answers behind the child on purpose because the gap that opens is what it measures. This is the arrangement `tests/conftest.py`'s `a_terminal_for_stdin` already makes for every other module that drives `l5-plan` through a terminal, and the reason is the same one. **The general rule: an input the subject will need is put where it is waiting before the subject exists, not written after the subject is running.** + +**Pre-loading removes a race and does not by itself make a status deterministic, which is why that case still carries a precondition.** With the answers waiting, a machine that starves the invocation past the stub's sleep and past the script's own post-session work leaves nothing for the signal to reach — and a signal that reaches nothing is a run that never conducted the case rather than an answer to it. The case reads that off the invocation rather than off a clock: `process.poll()` before the `killpg`, and a `ProcessLookupError` out of it, each report through `machine_load.inconclusive`. The module's `CLASSIFICATION` entry stays `CARRIES_PRECONDITIONS`. Both of that case's assertions are exactly as they were — the status is 130 and the commit holds exactly the one artifact the session wrote — and the stub's thirty-second sleep is unchanged, because shortening it would tighten the race rather than remove it. + +**Reaching a point *inside* a subject is done by what the subject printed, not by waiting a chosen number of seconds.** The new case covering the script's post-session landing has to interrupt `l5-plan` after the session has ended, and `wait_for_the_pty_to_say` gets it there: a line the script prints on its way into a question is the script saying it is at that question, so a slow machine changes when that line arrives rather than whether it does. The helper tells two endings apart, and the distinction is the same one the paragraph on preconditions draws. Not reaching the line inside the deadline is `machine_load.inconclusive` — the question went unasked. Reaching **end of output** without the line is an `AssertionError`, because no amount of load makes a script skip a question it is written to ask. + +**Where the case's cost actually goes is recorded in the case, because the previous comment invited the wrong conclusion.** The comment read as though the case were expensive from waiting the stub's thirty-second sleep out. The signal goes to the process **group**, so it reaches the stub as well, and the stub dies of it at once. Essentially all of the run is spent *before* the signal, getting the session to a point where there is something to interrupt at all — the base refresh, the id reservation pushed to the remote, the worktree, the build-state link, the workflow load, the prompt render, and the stub's own start. story-150's measurements of that split are in a run directory and reach no clone; what the repository holds is the account in the case's docstring in `tests/test_plan_commit.py`, which is where the next reader of that case will be standing. + **The ripple landed where a nested suite is assembled by copying files.** `tests/conftest.py` now imports from `tests/machine_load.py`, so every module that builds a throwaway tests directory has to copy the helper beside the conftest or the copy cannot import at all — `test_contract_assertions_bite.py`, `test_schema_inventory_location.py` and the two copied-tests tuples in `test_config_keys_are_obeyed.py` each gained the name. That is the recurring shape of giving `tests/conftest.py` a new import: the modules that will redden are the ones enumerating which test files a constructed harness gets, and a plan touching the conftest should expect them rather than discover them. `tests/conftest.py` also inserts its own directory on `sys.path` explicitly rather than relying on pytest having done it, because a ceiling that silently failed to install is exactly the quiet disappearance it exists to prevent. ## The revert check asks one test first diff --git a/.harness/history/execution-log.jsonl b/.harness/history/execution-log.jsonl index cd7f5c1..ad8e97c 100644 --- a/.harness/history/execution-log.jsonl +++ b/.harness/history/execution-log.jsonl @@ -81,3 +81,4 @@ {"story_id": "story-147", "status": "completed", "retry_count": 1, "timestamp": "2026-09-14 18:43:39"} {"story_id": "story-148", "status": "completed", "retry_count": 1, "timestamp": "2026-09-14 21:49:01"} {"story_id": "story-149", "status": "completed", "retry_count": 0, "timestamp": "2026-09-15 09:31:50"} +{"story_id": "story-150", "status": "completed", "retry_count": 0, "timestamp": "2026-09-15 12:47:39"} diff --git a/.harness/history/inspection-log.jsonl b/.harness/history/inspection-log.jsonl index b9ccb84..2b6dc67 100644 --- a/.harness/history/inspection-log.jsonl +++ b/.harness/history/inspection-log.jsonl @@ -42,3 +42,4 @@ {"story_id": "story-148", "mode": "narrow", "cost_usd": 2.7776145, "scope_files": 60, "invocations": 1, "timestamp": "2026-09-14 21:04:24", "findings": 2, "filed": 0, "dropped": 0, "dedupe_ran": true} {"story_id": "story-148", "mode": "narrow", "cost_usd": 3.7401380000000004, "scope_files": 60, "invocations": 1, "timestamp": "2026-09-14 21:37:12", "findings": 4, "filed": 0, "dropped": 0, "dedupe_ran": true} {"story_id": "story-149", "mode": "narrow", "cost_usd": 1.749655, "scope_files": 60, "invocations": 1, "timestamp": "2026-09-15 09:20:14", "findings": 1, "filed": 0, "dropped": 0, "dedupe_ran": true} +{"story_id": "story-150", "mode": "narrow", "cost_usd": 2.4072125, "scope_files": 60, "invocations": 1, "timestamp": "2026-09-15 12:37:54", "findings": 1, "filed": 0, "dropped": 0, "dedupe_ran": true} diff --git a/scripts/l5-plan b/scripts/l5-plan index f03b53a..dc39a8f 100755 --- a/scripts/l5-plan +++ b/scripts/l5-plan @@ -112,6 +112,18 @@ import story_ids # noqa: E402 import workflow_selection # noqa: E402 import worktrees # noqa: E402 +#: What an invocation interrupted in the session, or in the post-session work +#: below it, exits with. Everything `main` does above the session — the base +#: refresh, the id reservation, the worktree, the build-state link, and the +#: script's own workflow-confirmation prompt — is outside both guards and still +#: dies by the signal. +#: A shell reports a process killed by SIGINT as 128 + the signal, so this is +#: the status a developer's own terminal shows either way; what naming it once +#: buys is that the script *exits* with it rather than dying by the signal, so +#: a caller reading the return code of the process — a test, a script, anything +#: that is not a shell — is told the same thing the shell is told. +INTERRUPTED_EXIT_CODE = 130 + def main() -> None: argv = sys.argv[1:] @@ -323,29 +335,49 @@ def main() -> None: except KeyboardInterrupt: # An interrupted session ends like any other end: whatever it already # wrote is committed, and the exit status is the shell's own 130. - status = 130 - if status < 0: - # A session killed by a signal reports it as a negative return code; - # a shell reports the same end as 128 + the signal, and an interrupt - # is 130 either way. - status = 128 - status - # Committing happens whatever the session's status was, so it is done - # before the status is consulted at all. The session's own status then - # wins; the commit's is what is left to say when the session ended - # cleanly and the push did not. - committing = report( - worktree, - stories_dir, - before, - workflow["stages"], - config, - base, - workflow_name, - reservation=reservation, - invoked_root=target_root, - remote=remote, - brief_key=brief_key, - ) + status = INTERRUPTED_EXIT_CODE + try: + if status < 0: + # A session killed by a signal reports it as a negative return + # code; a shell reports the same end as 128 + the signal, and an + # interrupt is 130 either way. + status = 128 - status + # Committing happens whatever the session's status was, so it is done + # before the status is consulted at all. The session's own status then + # wins; the commit's is what is left to say when the session ended + # cleanly and the push did not. + committing = report( + worktree, + stories_dir, + before, + workflow["stages"], + config, + base, + workflow_name, + reservation=reservation, + invoked_root=target_root, + remote=remote, + brief_key=brief_key, + ) + except KeyboardInterrupt: + # An interrupt that lands *after* the session is the same end reached + # at a different moment, and until this guard it was the one moment the + # script did not handle: the try above covers the session alone, so an + # interrupt arriving while this script was committing, stamping, + # validating, publishing or waiting at one of its own prompts left main + # through an uncaught KeyboardInterrupt and the process died by the + # signal rather than exiting. A shell shows 130 for both, which is why + # it went unnoticed; anything reading the process's return code sees + # -SIGINT for one and 130 for the other. + # + # Nothing is undone, retried or completed here. Whatever the steps + # above had already committed, stamped, pushed or published stands + # exactly as they left it, and the steps below the interrupt did not + # happen — which is what the process dying by the signal already meant. + # All that changes is that the script says so in its exit status. + print("l5-plan: interrupted. Whatever had already been committed and " + "pushed stands; nothing after the interrupt was done.") + sys.exit(INTERRUPTED_EXIT_CODE) sys.exit(status or committing) diff --git a/tests/test_plan_commit.py b/tests/test_plan_commit.py index c9b336e..7ed82b5 100644 --- a/tests/test_plan_commit.py +++ b/tests/test_plan_commit.py @@ -816,9 +816,27 @@ def test_more_than_one_new_artifact_is_one_commit_naming_each(planning: Planning # -------------------------------------------------------------------------- -def run_plan_on_a_pty(planning: Planning, **stub): - """Run l5-plan with a pty for stdin, stdout and stderr.""" +def run_plan_on_a_pty(planning: Planning, *, reply: str = "", **stub): + """Run l5-plan with a pty for stdin, stdout and stderr. + + `reply` is written into the master *before* the child is started, which is + the arrangement `tests/conftest.py`'s `a_terminal_for_stdin` already makes + for every module that drives this script through a terminal, and for its + reason: an answer written behind a running child arrives when the + scheduler lets it, so a test whose outcome depends on the child having the + answer depends on winning a race it does not run. An answer already in the + terminal is there whenever the child looks, and nothing about when it + looks can change what it finds. + + It defaults to writing nothing, so a caller that says nothing about it + starts a child with an empty terminal exactly as before -- including the + caller in `tests/test_the_approval_is_observed.py`, which writes its + answers behind the child on purpose because the gap it opens by doing so + is what that test measures. + """ master, slave = pty.openpty() + if reply: + os.write(master, reply.encode()) process = subprocess.Popen( [sys.executable, str(L5_PLAN), "--workflow", PLANNED_WORKFLOW, "add a thing"], @@ -961,6 +979,49 @@ def wait_for_the_planning_session_to_write( time.sleep(0.05) +def wait_for_the_pty_to_say(master: int, said: str, + deadline: float = SESSION_WRITE_DEADLINE) -> str: + """Read the pty until the child has printed `said`, and return all of it. + + Waiting on what the script printed is how a caller reaches a point inside + the script's own post-session work without timing its way there: a line + the script prints on its way into a question is the script saying it is at + that question, and a machine that ran it slowly changes when that line + arrives rather than whether it does. + + A machine that did not let the child get there inside `deadline` left the + caller's question unasked, which is `machine_load.inconclusive`'s case. A + child that reached end of output without ever saying it is a different + thing -- it ended instead of asking -- and that is reported as a failure, + because no amount of load makes a script skip a question. + """ + heard, selector = "", selectors.DefaultSelector() + selector.register(master, selectors.EVENT_READ) + expires = time.monotonic() + deadline + try: + while said not in heard: + remaining = expires - time.monotonic() + if remaining <= 0: + machine_load.inconclusive( + f"the child under the pty never printed {said!r} within " + f"{deadline:g}s, so it never reached the point this test " + f"interrupts it at; it had said: {heard!r}") + if not selector.select(timeout=remaining): + continue + try: + chunk = os.read(master, 4096) + except OSError: # EIO: the child closed its side + chunk = b"" + if not chunk: + raise AssertionError( + f"the child under the pty ended without printing {said!r}; " + f"it said: {heard!r}") + heard += chunk.decode(errors="replace") + finally: + selector.close() + return heard + + def test_the_developers_terminal_is_the_sessions_terminal(planning: Planning): process, master = run_plan_on_a_pty(planning, L5_STUB_EXIT=7) status, _ = drain(process, master) @@ -971,8 +1032,49 @@ def test_the_developers_terminal_is_the_sessions_terminal(planning: Planning): def test_an_interrupt_still_commits_what_was_written_and_exits_130( planning: Planning): + """An interrupted session commits what it wrote, and the script exits 130. + + Both answers are in the terminal before the child is started, and nothing + is written into it after the signal. What story-150 observed of this case, + in the transcript rather than from the assertion that failed: + + * The stub does not sleep its thirty seconds out. The signal goes to the + process group, so it reaches the stub too, and the stub dies of it + immediately -- its own `KeyboardInterrupt` out of `time.sleep` is in + the transcript. The sleep is here to keep the session alive long + enough to be interrupted, and that is all it does. + * Essentially the whole of this case's cost is spent *before* the signal, + getting the session to the point where there is something to interrupt: + the base refresh, the id reservation pushed to the remote, the + worktree, the build-state link, the workflow load, the prompt render + and the stub's own start. Measured at the signal, everything after it + was a fraction of a second on every machine story-150 ran it on, quiet + and loaded. A reader wondering where thirty seconds went should look + above this line rather than at the sleep. + * The answers never decide the status. `l5-plan` ends with + `sys.exit(status or committing)`, and an interrupt sets that status to + 130 whatever the answers did. What they decide is whether there is a + commit to assert about and whether the process finishes at all: with + nothing in the terminal this invocation blocks at the approval question + and never ends, which is a hang rather than a failure. + * The status this asserts was once the script's alone to get wrong, and + story-150 found it doing so: an interrupt arriving after the session + had ended left `main` uncaught and the process died *by* the signal, + which a shell also shows as 130 but which anything reading the return + code sees as -SIGINT. That is repaired in the script, and the case + below this one drives the repaired point. + """ process, master = run_plan_on_a_pty( planning, + # The interrupted session's artifact is still committed and pushed, and + # since story-059 a successful push on a terminal ends by offering to + # run what was committed. This pty is a terminal, so both questions are + # asked and both have to be answered: since story-088 the plan is + # approved first, which is what lets an interrupted session's artifact + # be committed at all, and then the offer is declined so no run starts. + # They wait in the terminal from before the child exists, so no answer + # this invocation needs is racing the signal that follows. + reply=conftest.APPROVES + conftest.DECLINES, L5_STUB_WRITE=writes((".harness/stories/story-900.yaml", artifact())), L5_STUB_SLEEP=30, ) @@ -982,21 +1084,65 @@ def test_an_interrupt_still_commits_what_was_written_and_exits_130( # before the session had written anything to commit. wait_for_the_planning_session_to_write( planning, ".harness/stories/story-900.yaml") - os.killpg(os.getpgid(process.pid), signal.SIGINT) - # The interrupted session's artifact is still committed and pushed, and - # since story-059 a successful push on a terminal ends by offering to run - # what was committed. This pty is a terminal, so both questions are asked - # and both have to be answered: since story-088 the plan is approved first, - # which is what lets an interrupted session's artifact be committed at all, - # and then the offer is declined so the interrupt's own status wins. - os.write(master, conftest.APPROVES.encode() + b"n\n") + # The one precondition this case cannot control: the interrupt has to reach + # an invocation that is still running. It is asked of the invocation rather + # than of a clock -- a machine that starved this process past the stub's + # own sleep and past everything the script does afterwards leaves nothing + # to interrupt, and that is a run that never reached this question rather + # than an answer to it. + if process.poll() is not None: + machine_load.inconclusive( + "the invocation had already finished by the time the signal was " + "to be sent, so the interrupt this case is about never happened") + try: + os.killpg(os.getpgid(process.pid), signal.SIGINT) + except ProcessLookupError: + machine_load.inconclusive( + "the invocation's process group was gone by the time the signal " + "was sent, so the interrupt this case is about never happened") - status, _ = drain(process, master) + status, output = drain(process, master) - assert status == 130 + assert status == 130, output assert planning.planned_paths() == [".harness/stories/story-900.yaml"] +def test_an_interrupt_after_the_session_ends_exits_130_rather_than_dying_of_it( + planning: Planning): + """The same end reached at a different moment, and the same status. + + The case above interrupts a live session. This one interrupts the script's + own post-session work, which story-150 found to be the moment the script + did not handle: the `try` around the session covered the session alone, so + an interrupt arriving while `l5-plan` was committing, stamping, validating, + publishing or waiting at one of its own questions left `main` through an + uncaught `KeyboardInterrupt`, and the process died by the signal instead of + exiting. `Popen.wait` reports that as -SIGINT, not as 130. A shell shows + 130 for both ends, which is why it stood so long. + + The point is reached by what the script says rather than by waiting a + chosen number of seconds: the stub writes and exits at once, nothing is + put in the terminal, and the script therefore stops at the approval + question and stays there. The question being printed is the script saying + it is past the session and inside the work this case is about, so the + signal lands where it is meant to on a machine of any speed. + """ + process, master = run_plan_on_a_pty( + planning, + L5_STUB_WRITE=writes((".harness/stories/story-900.yaml", artifact())), + ) + heard = wait_for_the_pty_to_say(master, "approve this plan?") + os.killpg(os.getpgid(process.pid), signal.SIGINT) + + status, output = drain(process, master) + + assert status == 130, heard + output + # The script exited rather than died, and said so: the line is what tells + # a developer that what had already been committed and pushed stands and + # that nothing below the interrupt was done. + assert "l5-plan: interrupted." in heard + output + + # -------------------------------------------------------------------------- # Everything before the session is unchanged. # --------------------------------------------------------------------------