pr-explain: author each section in its own provider call #152

Merged

vnprc merged 5 commits from agent/pr-explain-sectioned-authoring into master

2026-08-21 02:39:54 +01:00

allod-agent commented

2026-08-20 20:39:35 +01:00

Member

When you run allod pr explain, the report is now written one piece per provider call: an outline pass plans the sections and writes the front matter, each section gets its own call that re-reads the full reader contract and the report so far, and a final call writes the quiz and provenance; the tool stitches the pieces together. This exists because three straight reads showed report quality decaying in the back half of single-pass generations — every call now starts with the contract a short distance from the text it governs, plus the master-teacher characterization paragraph you asked for. A full run can spend up to 17 provider calls instead of 4 (typically sections + 4; the consent text states the ceiling before anything is sent). One deliberate narrowing: every quiz item now lives in the final quiz section — master could also place an item right after the teaching it tests — so the global quiz rules bind one call that sees the whole report; your next read can judge that trade. Everything else is identical: same consent boundary, same cage and per-call postconditions, same validator with the same blocking errors, same slop and single-repair passes, same flags. The section plan and every fragment are mechanically checked before the next call spends anything, and a bad plan or misplaced fragment fails the run closed with diagnostics preserved. All 896 mocked-runner tests pass, including new sabotage fixtures for every new mechanical guard. If it is wrong, revert this PR — no state, interface, or other tool is touched, and already-generated reports are unaffected. The real witness is your next read of a live report generated from this branch or after merge.

Risk

R2 per the dev plan (allod/strategy dev-plans/pr-explain-sectioned-authoring.md). The change is localized to report generation in one CLI; the pipeline glue is deterministic and covered by mocked-runner fixtures; rollback is a straight revert. What local tests cannot witness is the goal itself — register quality held through the last section — which needs a live provider run and an owner read. Worth human eyes: the consent/ceiling text, the assembly path (the validator must see one complete body), and the outline schema gate's forced-T0 tolerance (a triage forced past T0 has no objective vocabulary, so id membership checks stand down and ids are held to shape only).

Validation

bash tests/pr-explain/command.sh      # All 706 allod pr explain command tests passed.
bash tests/pr-explain/validation.sh   # All 104 PR explanation report validator tests passed.
bash tests/pr-explain/components.sh   # All 86 PR explanation components tests passed.

New coverage: a two-section sequential run (each later pass shown receiving the accepted prose verbatim), five outline schema-gate sabotages (non-JSON, duplicate ids, backward layers, unclaimed objective, reserved id), a plan-contradicting table of contents, a mislabeled section fragment, a section fragment missing data-layer, a script-bearing fragment, a fragment pass writing the assembled body, a quiz fragment that never closes main, an outline pass that writes no plan, a mid-section interruption that fails closed, and a valid post-TOC anchor that must pass the TOC guard (the regression witness for the review's finding 3 — the guard now bounds its href scan to the nav element instead of scanning to end-of-fragment). Existing repair, slop, cage-postcondition, consent, and interruption tests were renumbered to the new pass sequence and all still pass. No live provider was run; the mocked-runner rig drives every pass.

The review pass (comment below) found no hard defects; its findings 1–4 are addressed by this body revision and the follow-up commit bounding the TOC guard.

Deviations from the plan, with reasons: the heading-naming rule lives in the shared contract rather than the outline prompt, because it also binds the h3/h4 headings, disclosure summaries, and figure captions that section passes write; the front-matter/section/quiz fragments carry the main open and close lines themselves (front opens it, quiz closes it) so assembly stays a pure concatenation; and quiz items are confined to the final quiz section as stated in the summary.

Refs allod/tools#138

When you run `allod pr explain`, the report is now written one piece per provider call: an outline pass plans the sections and writes the front matter, each section gets its own call that re-reads the full reader contract and the report so far, and a final call writes the quiz and provenance; the tool stitches the pieces together. This exists because three straight reads showed report quality decaying in the back half of single-pass generations — every call now starts with the contract a short distance from the text it governs, plus the master-teacher characterization paragraph you asked for. A full run can spend up to 17 provider calls instead of 4 (typically sections + 4; the consent text states the ceiling before anything is sent). One deliberate narrowing: every quiz item now lives in the final quiz section — master could also place an item right after the teaching it tests — so the global quiz rules bind one call that sees the whole report; your next read can judge that trade. Everything else is identical: same consent boundary, same cage and per-call postconditions, same validator with the same blocking errors, same slop and single-repair passes, same flags. The section plan and every fragment are mechanically checked before the next call spends anything, and a bad plan or misplaced fragment fails the run closed with diagnostics preserved. All 896 mocked-runner tests pass, including new sabotage fixtures for every new mechanical guard. If it is wrong, revert this PR — no state, interface, or other tool is touched, and already-generated reports are unaffected. The real witness is your next read of a live report generated from this branch or after merge. ## Risk R2 per the dev plan (`allod/strategy` `dev-plans/pr-explain-sectioned-authoring.md`). The change is localized to report generation in one CLI; the pipeline glue is deterministic and covered by mocked-runner fixtures; rollback is a straight revert. What local tests cannot witness is the goal itself — register quality held through the last section — which needs a live provider run and an owner read. Worth human eyes: the consent/ceiling text, the assembly path (the validator must see one complete body), and the outline schema gate's forced-T0 tolerance (a triage forced past T0 has no objective vocabulary, so id membership checks stand down and ids are held to shape only). ## Validation ``` bash tests/pr-explain/command.sh # All 706 allod pr explain command tests passed. bash tests/pr-explain/validation.sh # All 104 PR explanation report validator tests passed. bash tests/pr-explain/components.sh # All 86 PR explanation components tests passed. ``` New coverage: a two-section sequential run (each later pass shown receiving the accepted prose verbatim), five outline schema-gate sabotages (non-JSON, duplicate ids, backward layers, unclaimed objective, reserved id), a plan-contradicting table of contents, a mislabeled section fragment, a section fragment missing `data-layer`, a script-bearing fragment, a fragment pass writing the assembled body, a quiz fragment that never closes `main`, an outline pass that writes no plan, a mid-section interruption that fails closed, and a valid post-TOC anchor that must pass the TOC guard (the regression witness for the review's finding 3 — the guard now bounds its href scan to the nav element instead of scanning to end-of-fragment). Existing repair, slop, cage-postcondition, consent, and interruption tests were renumbered to the new pass sequence and all still pass. No live provider was run; the mocked-runner rig drives every pass. The review pass (comment below) found no hard defects; its findings 1–4 are addressed by this body revision and the follow-up commit bounding the TOC guard. Deviations from the plan, with reasons: the heading-naming rule lives in the shared contract rather than the outline prompt, because it also binds the `h3`/`h4` headings, disclosure summaries, and figure captions that section passes write; the front-matter/section/quiz fragments carry the `main` open and close lines themselves (front opens it, quiz closes it) so assembly stays a pure concatenation; and quiz items are confined to the final quiz section as stated in the summary. Refs allod/tools#138

allod-agent added 4 commits

2026-08-20 20:39:35 +01:00

pr-explain: split the authoring prompt into a shared contract and per-pass prompts 73d8e4d8f3

pr-explain: author each section in its own provider call b841f96870

pr-explain: exercise the sectioned pipeline in the mocked-runner suites ac2259694f

pr-explain: document the sectioned pipeline and its call ceiling b894446d97

allod-agent commented

2026-08-20 21:06:03 +01:00

Author

Member

Read-only review pass by a fresh agent (adversarial brief; independent re-run of all three suites from the branch worktree). No hard defects. All nine plan interface contracts verified against the code; the cage argv/env path is shared by all six provider-call sites with ALLOD_PR_EXPLAIN_OUTLINE the only added variable; validator, report.css, report.js, and gallery are byte-untouched; suites reproduce 699/104/86.

Findings, most material first:

PR body overstates the typical cost by one call. "typically sections + 5" counts the repair pass, which a typical first-validate run does not spend; the change's own table says N + 4 (docs/pr-explain.md:296). The consent ceiling (17/16/15) is exact and unaffected. Fix: reword to sections + 4. The wording traces to the dev plan; the docs table is the correct one.
A deliberate narrowing is missing from the body's "everything else is identical" framing. Master allowed a quiz item at the end of the section it tests ("active retrieval near the teaching"); the sectioned pipeline forces every item into the final quiz section, because the global quiz rules need whole-report view. Valid under the unchanged validator and chosen by the plan — but it is a pedagogy change and the owner should see it stated. Fix: one disclosure line in the PR body; behavior stands, and the next reader measurement can judge it.
Latent robustness wart in the front-matter TOC guard (pr-explain/lib.sh:176). The sed range /class="rx-toc"/,/<\/nav>/ never matches its closing address when the nav opens and closes on one line, so the range runs to end-of-fragment and the href scan scoops any anchor after the TOC. Today the objectives block carries no hrefs, so the guard only ever fails closed — but a future front-matter a.rx-termref would spuriously reject valid reports. Fix: bound the extraction to the nav element, with a valid-case fixture as the witness.
The plan's acceptance list names a missing data-layer fragment sabotage that has no dedicated fixture. It is functionally covered — the mechanical check is an exact full-opening-tag match, the same assertion section-wrong-id exercises — which satisfies the one-sabotage-per-validator testing policy; adding the small fixture makes the plan's enumeration literally true. Will add.
Observation, no action: a later authoring pass could overwrite an earlier captured fragment inside the job dir before assembly re-reads it. The assembled-body validator backstops this exactly as it backstopped master's single author pass, and fragment paths stay symlink/realpath-confined to the job dir; recorded so the posture is explicit rather than implicit.

Verified in passing: the mid-section interruption fixture genuinely blocks inside pass 3 and asserts exit 130, no output, diagnostics preserved; the five outline sabotages each isolate exactly one schema clause; deviation 3 (forced-T0 membership relaxation) opens no hole — the untouched validator still rejects an empty objectives block, and it never cross-checked triage ids on master either.

Fixes for findings 1–4 follow in a commit and PR-body edit on this branch.

Read-only review pass by a fresh agent (adversarial brief; independent re-run of all three suites from the branch worktree). **No hard defects.** All nine plan interface contracts verified against the code; the cage argv/env path is shared by all six provider-call sites with `ALLOD_PR_EXPLAIN_OUTLINE` the only added variable; validator, `report.css`, `report.js`, and gallery are byte-untouched; suites reproduce 699/104/86. Findings, most material first: 1. **PR body overstates the typical cost by one call.** "typically sections + 5" counts the repair pass, which a typical first-validate run does not spend; the change's own table says N + 4 (`docs/pr-explain.md:296`). The consent ceiling (17/16/15) is exact and unaffected. Fix: reword to sections + 4. The wording traces to the dev plan; the docs table is the correct one. 2. **A deliberate narrowing is missing from the body's "everything else is identical" framing.** Master allowed a quiz item at the end of the section it tests ("active retrieval near the teaching"); the sectioned pipeline forces every item into the final quiz section, because the global quiz rules need whole-report view. Valid under the unchanged validator and chosen by the plan — but it is a pedagogy change and the owner should see it stated. Fix: one disclosure line in the PR body; behavior stands, and the next reader measurement can judge it. 3. **Latent robustness wart in the front-matter TOC guard** (`pr-explain/lib.sh:176`). The sed range `/class="rx-toc"/,/<\/nav>/` never matches its closing address when the nav opens and closes on one line, so the range runs to end-of-fragment and the href scan scoops any anchor after the TOC. Today the objectives block carries no hrefs, so the guard only ever fails closed — but a future front-matter `a.rx-termref` would spuriously reject valid reports. Fix: bound the extraction to the nav element, with a valid-case fixture as the witness. 4. The plan's acceptance list names a `missing data-layer` fragment sabotage that has no dedicated fixture. It is functionally covered — the mechanical check is an exact full-opening-tag match, the same assertion `section-wrong-id` exercises — which satisfies the one-sabotage-per-validator testing policy; adding the small fixture makes the plan's enumeration literally true. Will add. 5. Observation, no action: a later authoring pass could overwrite an earlier captured fragment inside the job dir before assembly re-reads it. The assembled-body validator backstops this exactly as it backstopped master's single author pass, and fragment paths stay symlink/realpath-confined to the job dir; recorded so the posture is explicit rather than implicit. Verified in passing: the mid-section interruption fixture genuinely blocks inside pass 3 and asserts exit 130, no output, diagnostics preserved; the five outline sabotages each isolate exactly one schema clause; deviation 3 (forced-T0 membership relaxation) opens no hole — the untouched validator still rejects an empty objectives block, and it never cross-checked triage ids on master either. Fixes for findings 1–4 follow in a commit and PR-body edit on this branch.

allod-agent added 1 commit

2026-08-21 01:21:42 +01:00

pr-explain: bound the TOC guard to the nav element and add review fixtures 41abaf8160

allod-agent commented

2026-08-21 01:22:16 +01:00

Author

Member

Review findings addressed: commit 41abaf8 bounds the TOC guard's href scan to the nav element (finding 3, with a valid post-TOC-anchor witness that would have failed under the old sed range) and adds the missing-data-layer fragment sabotage (finding 4). The PR body now says sections + 4 for the typical run (finding 1) and discloses the quiz-placement narrowing (finding 2). Finding 5 stands as a recorded observation, no action. Suites after the fix: 706 / 104 / 86, all passing. One note for a future cleanup, out of scope here and pre-existing on master: the repair-runner-failure case asserts $(cat …/runner.stderr) equals empty, which passes identically whether the file is empty or missing — a vacuous-assert candidate per memory/shell.md.

Review findings addressed: commit 41abaf8 bounds the TOC guard's href scan to the nav element (finding 3, with a valid post-TOC-anchor witness that would have failed under the old sed range) and adds the missing-data-layer fragment sabotage (finding 4). The PR body now says sections + 4 for the typical run (finding 1) and discloses the quiz-placement narrowing (finding 2). Finding 5 stands as a recorded observation, no action. Suites after the fix: 706 / 104 / 86, all passing. One note for a future cleanup, out of scope here and pre-existing on master: the repair-runner-failure case asserts `$(cat …/runner.stderr)` equals empty, which passes identically whether the file is empty or missing — a vacuous-assert candidate per memory/shell.md.