Risky changes need a person, loopstra retro, and acceptance tests from Done when

docs/architecture.md

@@ -22,6 +22,7 @@ About 165 modules under `src/`, in eight layers. A module imports only from its
    - `heartbeat.ts` holds the loop's lock and pause.
    - `requests.ts`, `inbox.ts`, `conversation.ts` and `questions.ts` are what people leave for the loop.
    - `stamp.ts` holds what an upgrade knows.
+   - `risk.ts` scores a change's diff against `gates.merge.risky` (`scoreRisk`), for the merge gate.
    - `agents.ts` resolves a step's agent setup from the config (`resolveSetup`) and holds its rules: which steps are read-only, which git a step may never get, and which steps run as the build's agent.
 4. **Records.**
    - `record-draft.ts` holds `RecordDraft`.

docs/decisions.md

@@ -909,3 +909,47 @@ A new `agents` block in `loopstra/config.yaml` gives each step an agent to play,
 - **New files the resolve session leaves are removed** before the resolution is saved (`git clean -fd`, not `-x`), as after the test loop's runs: the change's own new files are already in the index, and the prompt says not to add files. Commits are allowed empty: a resolution may keep main's side of everything.
 - **The waiting check is cached by commit.** `conflictsWhileWaiting` keeps its last answer per branch with main's and the branch's commits, so ticks and watcher looks that find neither moved run no `merge-tree`.
 - **A resolution in the local merge checks holds the merge lock.** Without a remote, the merge checks run under it, so other changes' merges wait while a resolution and its tests run. Fix sessions in the merge checks already did this; moving the catch-up before the lock would leave a window where main moves again. The catch-up rebase inside `mergeIntoMain` still blocks on a conflict: under the lock, main can only have gained records there.
+
+## loopstra retro (2026-10-27)
+
+- **A read-only report over the trace, in plain text like `loopstra judges`.** `src/retro.ts` is pure: it builds the report from rows the trace gives (`statusChangesSince`, `gatesSince`, `phasesSince`, `eventsOfTypeSince`), the config and the judges' report, and `src/commands/retro.ts` reads them. No schema change and no new storage; it writes nothing.
+- **`--days` is a whole number from 1 to 3650; 7 without it.** Anything else is the usage line, like a bad port. One day reads "the last day". The first day of the window is shown in local time ("since Oct 20").
+- **Every status move is read, the rest only the window's.** A change merged this week may have been accepted long before, so the median from accepted to merged needs its whole history. Its start is the move to accepted, or, for a change written as accepted, its first move.
+- **A move to the same status is not a move.** It only rewrote the note, so it neither counts as a block (or a merge) nor ends a wait.
+- **Blocks are told apart by the constants the runtime writes them from.** The notes that were local are exported (`TESTS_KEPT_FAILING_NOTE`, `REVIEW_STILL_FOUND_NOTE`, `UNMET_NOTE`, `SESSION_LIMIT_NOTE` and the rest), and a note's kind is the first whose constant it contains, so a reworded note cannot drift from its kind. A failed session's note (timed out, crashed, the spending limit, too many questions) is looked for first, since a stage's note may lead with its own words ("The finished change could not be checked. The assistant took too long..."). A project command that kept running past its limit after every fix counts with tests that kept failing (the fix loops ran out either way), and reviews used up after late fixes with the reviewer that kept finding problems. Any other note is listed by its first sentence, without the retry advice.
+- **Gates leave out what a person did.** `done/human` and `merge/pr-approved` record a person, not a check, and `merge/risk` says whether a person decides (its `waiting` is no failure), so none of them is a gate line. A change passed a check the first time when its first row in the window passed. Lines follow the stages' order.
+- **Spending counts ended phases only.** A running phase's cost is a budget hold, not spending. "A change" is a change with a phase that ended in the window; kinds of step that cost nothing (the code phases) are not listed. Judging is every spec-check, plan-challenge and review session, the cheaper judges' and the early review's included.
+- **A wait at merge-review is a person's unless its note says it waits for GitHub's checks.** The other review statuses are entered only when a person decides, so every wait there is a person's. This reads what happened, whatever the config says now. A wait starts in the window and ends at the change's next move to another status; one still open counts until now and is reported as still waiting, since the longest waits are the ones that matter. A question's wait ends at an applied answer or at the change's next status move.
+- **Suggestion details.** Timeouts are counted from blocks, not from timeouts retried away. The longer timeout is half as long again, rounded up to 5 minutes. A person may come off a gate only when the config has one on it now, the median wait there is over 24 hours, and every automatic check of that gate in the window passed, at least 5 times in all. Whether a cheaper judge has earned going first is `judgeReport`'s new `earned` field, not its words.
+- **Two rules point at the other two features.** With nobody on the merge gate and nothing under `gates.merge.risky`, one merge in the window is enough to suggest risky paths: the setting costs nothing until a risky change comes. With `stages.build.acceptance_tests` off, one change blocked on an unmet Done when criterion suggests turning it on: that is the failure the tests exist to catch before the merge.
+- **An empty window is said in one line.** Nothing moved, no gate ran and no change's phase or question started in the window (chat alone does not count): "Nothing happened in the last 7 days.", then the suggestions, which need data and so say there are none.
+- **Lists of changes stop at five**, then "and N more".
+
+## Risky changes need a person (2026-10-27)
+
+- **Risk is facts about the diff, never an agent's judgment.** A change is risky when a file it changes matches a pattern in `gates.merge.risky.paths`, or when its lines added plus removed are more than `gates.merge.risky.lines`. Both leave out `intent/`, and a binary file adds no lines. It is predictable and testable, and costs no session. Nothing is risky until `paths` or `lines` is set, the default, so an upgraded loop behaves as before.
+- **Patterns are Bun.Glob on repository paths with `/`.** A bare name (`package.json`) matches only at the top; `**/package.json` matches anywhere. A blank pattern is a config problem.
+- **The diff is the branch against main from where they parted** (`git diff --numstat -z -M main...HEAD` in the change's worktree): only the change's own work counts, not what main gained since. `-z` keeps names with spaces and other alphabets as they are. A rename is one record, and it counts under its new name or its old one, whichever a pattern matches: moving a workflow out of `.github/` changes CI as much as moving one in. A diff git cannot make fails the step, so nothing merges unscored.
+- **The decision is made at one place, `passMergeGate`, after the merge checks pass.** Every change Loopstra makes to the branch (build, fix, revise, the merge checks' own fixes) goes through another review and then through this point, so the decision is always about the code that will merge. Commits a person pushes to the pull request on GitHub are not scored again: a person is already involved.
+- **`merge_gate` records who decides, in the same record as the move to `merge-review`.** The pull request watcher reads the change's frontmatter fresh on every look, so a status without the field could merge an unapproved risky change. `setStatus` takes frontmatter fields to set or remove in its one record for this. The field is written only when it changes who decides, that is with `gates.merge.human: none`; a change that is not risky loses one an earlier pass wrote, in that record or in the record of its merge. It is runtime-managed, and left out when blank, like `hold`.
+- **One helper, `changeGates`, gives a change's own gates.** The config's, with `merge_gate` on the merge gate when the config has nobody there; it never takes a person off. `waitsForPerson`, `isRunnable` and `offers` apply it themselves, so the scheduler, `queue.md`, `status`, the console and chat need no change of their own; apply's approve and send-back check, the attention note, plan-ahead's "may merge first" and the merge stage (through `mergeHuman`) use it too. A person who deletes the line takes the change back to the config's gate: a person's edits win, as everywhere.
+- **Without a remote, `pr` acts as `status`.** There is no pull request to approve, so a risky change waits in `merge-review` for `merge-approved` rather than blocking. The field still says `pr`, what the config asked for, so the change waits for an approval on GitHub if a remote is added before it merges.
+- **With a person on the config's merge gate, risk is not scored.** It would change nothing, so no diff is read and no risk row is written (a `waiting` row that waits for nothing would read as a check that keeps failing); a stale `merge_gate` is removed and the note is the gate's usual one. The review of the branch found the first version wrote the row anyway.
+- **The note names the files.** Whoever approves a merge needs to know what to look at, so the owner's note names up to three of them ("and N more" after that), and says how to let the change in. A change risky only by size says how many lines changed against the limit. The trace's `merge`/`risk` row names up to ten, with the size when that tripped it: `pass` with "nothing risky", or `waiting`. No row is written when nothing is set to be risky.
+- **Setup asks for paths only when nobody is on the merge gate,** in the GitHub section, where the merge gate is asked. The file's own list is the suggestion; without one, `.github/**`, `**/migrations/**`, and each common dependency or build file found at the top of the repository. `-` empties the list. `--defaults` never adds one: nothing is risky until a person says so. `lines` and `human` are left to the file, where the template's comment shows them.
+- **Known limit: a change is scored when it passes the merge gate.** Risky settings added while a change already waits in `merge-review` with nobody on the gate do not apply to it until its next pass (a fix, a send-back or a rebuild).
+
+## Acceptance tests (2026-10-27)
+
+- **Off unless set; new repos get it.** `stages.build.acceptance_tests` defaults to `false` in the schema, so an upgraded repo runs exactly as before. The config `loopstra init` writes sets it to `true`, and `loopstra setup gates` asks "Write tests from Done when before each build?", suggesting the file's own value, or yes when the file does not say. So `setup --defaults` on an older config turns it on: running setup is a person choosing. It costs one more session per build.
+- **A step of the build stage, with its own model and agent setup.** `acceptance` is a step key under build (its model follows the build stage's), listed before `build`, so `stages.build.steps.acceptance` and `agents.steps.acceptance` work like the others. It is a writing step, never read-only, and not a continuation of the build's session: it runs in a fresh session as its own agent, since it must not know how the build will go about it.
+- **Once per build, before the build session.** It runs after the worktree is ready and the build's `before` commands ran. The run folder's `acceptance` marker (the test files, as JSON) says it ran for this build. A new build (the status was not `building`) clears it, so its step runs again and may keep or update what an earlier build's step wrote; a build resumed after a stop keeps it and does not run the step twice. What an earlier build left unsaved in the worktree is committed first (`chore: keep unfinished changes`), so the acceptance commit holds only the tests.
+- **The protected files come from git, and from the tests it names.** The marker lists the files the acceptance commit added or changed (deletions left out), plus each file its answer names as a test that is in the worktree: a step that keeps an earlier build's tests unchanged writes nothing, and they must stay protected. A named path is taken relative to the worktree with forward slashes; one outside it is ignored.
+- **The commit is the runtime's.** `loopstra(<slug>): acceptance tests`, with the session's `commit_message`, when it gives one, as the body.
+- **Failing softly.** A session that fails (an answer of `fail`, a crash or a timeout after its retry, the spending limit) does not block the change: an `error` event says why, what it wrote is dropped from the worktree, `acceptance.md` says no tests were written, the marker is written empty so a resumed build does not try again, and the build goes on as without the setting. A stop, a question limit or an unavailable assistant still ends the step as everywhere, and the step runs again when it resumes. Only a commit that fails blocks, as the build's own commit would.
+- **`acceptance.md` is for the owner.** "Covered by tests" lists each test as its criterion in the owner's words (without its last period), then its file as code and its name in quotes, as in the spec's example; "Not covered by a test" lists each criterion it could not test with why. A test whose file is not in the marker is left out. With no test, it says "No acceptance tests were written for this change." and why. It is in the console's document list.
+- **Every session gets the files; the hook decides who may edit them.** `LOOPSTRA_ACCEPTANCE` is set for every session from the marker (`[]` when there is none), rather than for a chosen few, so the merge checks' fixes, which run in another stage, are covered too. The hook refuses an edit to one of them in build, fix, reconcile and revise, logs it with the other refused tests, and says "this is an acceptance test written from Done when; fix the code, not the test". In the `acceptance` phase it refuses any file that is not a test by its existing rules (test folders and names; fixtures under a test folder count). A list that cannot be read holds nothing, so a broken marker never stops a build. Paths are compared with forward slashes, and without case on Windows and macOS, as the fix rule does.
+- **The block note names an acceptance test apart from other protected tests.** "It was not allowed to edit the acceptance test `tests/count.test.ts`, written from Done when before the build. If the test is wrong, an engineer can fix it on the branch intent/count-words." When a fix was refused both kinds, both sentences come before the one about the branch.
+- **The drift check leaves them out.** Acceptance test files are not the plan's to list, so they never start a reconcile. They are not added to the files a change holds against other changes (see `sameFileHolds`): two changes that write the same test file are rare, and the merge brings any clash up as it does today.
+- **The done-check reads the list.** `{{acceptance}}` is acceptance.md's "Covered by tests" list, or "No acceptance tests were written for this change.", and the prompt says a criterion covered by an acceptance test that passes in its copy of main is met. A repository's older `done-check.md` without the variable gets the list appended, but only when there is one, so a change without acceptance tests sees that prompt unchanged.
+- **A work session that never asks.** `acceptance` can be messaged, redirected and stopped like the build, and reads `conversation.md`. It has no `question` in its answer: the Done when list is the owner's answer already.