allod/tools
Watch
2
0
Fork
You've already forked tools
 0
Code Issues 27 Pull requests Projects Releases Packages Wiki Activity Actions

Generate comprehension-first PR explanation reports #141

Merged
vnprc merged 19 commits from agent/pr-explain-report-tool into master 2026-08-19 17:20:47 +01:00
Conversation 7 Commits 19 Files changed 24 +10849 -6
allod-agent commented 2026-08-16 21:24:16 +01:00
Member
Copy link

Implements the reusable PR explanation report generator tracked in #138.

Replaces #139, which was opened unintentionally by an AGit push and has no head branch. This PR carries the same 14 commits on a normal agent/* branch, plus one additional commit fixing the interruption defect raised in review on #139.

What this adds

  • allod pr explain <number> --codex|--claude --output <report.html> with explicit provider consent, immutable PR SHA verification, detached analysis checkout, provider-environment sanitization, atomic publication, overwrite protection, dry-run, and one bounded validation-repair pass.
  • forge pr snapshot <number> as a stable read-only JSON interface for same-repository and fork PRs.
  • A hash-pinned, self-contained report component vocabulary and gallery: operator summary, progressive disclosure, responsive flow/branch/sequence diagrams, code walks, comparisons, timelines, callouts, quizzes, and provenance.
  • A strict standalone validator for active/network-capable markup, secrets, provenance, anchors, semantic component contracts, no-JS completeness, accessibility, reduced motion, exact code whitespace, and connector geometry.
  • Documentation for report generation and the iterative human-review workflow.

The narrow-screen alignment bug is fixed structurally: flow connectors are pseudo-elements owned by their destination nodes. The authored glyph changes from ↓ to →/← when the shared container has room; no full-width connector box is rotated independently of its node.

Signal handling fix

pr-explain/explain previously installed only an EXIT trap and derived its preserve/report decision from $?. A SIGINT while waiting on the provider, or an external SIGTERM, could leave the EXIT trap observing status 0 — so an interrupted run could leave the private .allod-pr-explain.* job directory behind without announcing it, and in some paths continue toward publication. That directory holds the complete diff and review context.

Explicit INT and TERM handlers now record a non-zero interrupted state, terminate the provider child, and force the EXIT cleanup and the pass runner to honour that state. The provider now runs as a background child that the script waits on, so the signal reaches the shell rather than being absorbed by a foreground child.

Human workflow

  1. Generate a report from a reviewed PR snapshot.
  2. Read the self-contained HTML locally.
  3. Give concrete comprehension/usability feedback.
  4. Revise this tool/template on its own PR, then regenerate.

The command never commits, pushes, edits the PR, or chooses a private publication path.

Validation

  • Complete repository test suite passes on this branch head: 16 suites, 0 failures.
  • PR-explain suites: 476 command assertions, 81 component assertions, 72 validator assertions.
  • Forge snapshot suite: 19 assertions, plus the complete Forge help/read/mutation/validation/auth/token/api-error/pr-close/issue-close suites.
  • The signal fix adds 8 mocked slow-runner assertions covering SIGINT and SIGTERM: each verifies non-zero exit, no published report, an announced diagnosis, and a preserved private diagnostics directory.
  • Real attended Claude smoke against allod/tools#136 succeeded and produced a 67 KB validated report. This ran against the original 14 commits, not the signal fix.
  • Independent Claude Opus review found no release blocker in the original 14 commits. The signal-fix commit has not had that review pass.

The 446 command assertions and 14 snapshot assertions quoted on #139 were already stale when that body was written; later commits on the same branch added tests. The numbers above are measured on this branch head.

Codex runner behavior is fully mocked and tested; no Codex tokens were used after the account quota was exhausted.

Risk

The signal handling change moves both provider invocations from foreground execution to a backgrounded child plus wait. The claude path also changes from a subshell cd to pushd/popd, so the working-directory change is no longer confined to a subshell — the popd restores it on the success path, and an interrupted run exits rather than continuing. Signal behaviour is covered by mocked runners only; no attended interrupt of a real provider was performed.

Implements the reusable PR explanation report generator tracked in #138. Replaces #139, which was opened unintentionally by an AGit push and has no head branch. This PR carries the same 14 commits on a normal `agent/*` branch, plus one additional commit fixing the interruption defect raised in review on #139. ## What this adds - `allod pr explain <number> --codex|--claude --output <report.html>` with explicit provider consent, immutable PR SHA verification, detached analysis checkout, provider-environment sanitization, atomic publication, overwrite protection, dry-run, and one bounded validation-repair pass. - `forge pr snapshot <number>` as a stable read-only JSON interface for same-repository and fork PRs. - A hash-pinned, self-contained report component vocabulary and gallery: operator summary, progressive disclosure, responsive flow/branch/sequence diagrams, code walks, comparisons, timelines, callouts, quizzes, and provenance. - A strict standalone validator for active/network-capable markup, secrets, provenance, anchors, semantic component contracts, no-JS completeness, accessibility, reduced motion, exact code whitespace, and connector geometry. - Documentation for report generation and the iterative human-review workflow. The narrow-screen alignment bug is fixed structurally: flow connectors are pseudo-elements owned by their destination nodes. The authored glyph changes from `↓` to `→`/`←` when the shared container has room; no full-width connector box is rotated independently of its node. ## Signal handling fix `pr-explain/explain` previously installed only an `EXIT` trap and derived its preserve/report decision from `$?`. A SIGINT while waiting on the provider, or an external SIGTERM, could leave the EXIT trap observing status 0 — so an interrupted run could leave the private `.allod-pr-explain.*` job directory behind without announcing it, and in some paths continue toward publication. That directory holds the complete diff and review context. Explicit `INT` and `TERM` handlers now record a non-zero interrupted state, terminate the provider child, and force the EXIT cleanup and the pass runner to honour that state. The provider now runs as a background child that the script `wait`s on, so the signal reaches the shell rather than being absorbed by a foreground child. ## Human workflow 1. Generate a report from a reviewed PR snapshot. 2. Read the self-contained HTML locally. 3. Give concrete comprehension/usability feedback. 4. Revise this tool/template on its own PR, then regenerate. The command never commits, pushes, edits the PR, or chooses a private publication path. ## Validation - Complete repository test suite passes on this branch head: 16 suites, 0 failures. - PR-explain suites: 476 command assertions, 81 component assertions, 72 validator assertions. - Forge snapshot suite: 19 assertions, plus the complete Forge help/read/mutation/validation/auth/token/api-error/pr-close/issue-close suites. - The signal fix adds 8 mocked slow-runner assertions covering SIGINT and SIGTERM: each verifies non-zero exit, no published report, an announced diagnosis, and a preserved private diagnostics directory. - Real attended Claude smoke against `allod/tools#136` succeeded and produced a 67 KB validated report. This ran against the original 14 commits, not the signal fix. - Independent Claude Opus review found no release blocker in the original 14 commits. The signal-fix commit has not had that review pass. The 446 command assertions and 14 snapshot assertions quoted on #139 were already stale when that body was written; later commits on the same branch added tests. The numbers above are measured on this branch head. Codex runner behavior is fully mocked and tested; no Codex tokens were used after the account quota was exhausted. ## Risk The signal handling change moves both provider invocations from foreground execution to a backgrounded child plus `wait`. The `claude` path also changes from a subshell `cd` to `pushd`/`popd`, so the working-directory change is no longer confined to a subshell — the `popd` restores it on the success path, and an interrupted run exits rather than continuing. Signal behaviour is covered by mocked runners only; no attended interrupt of a real provider was performed.
`pr snapshot` feeds a machine-readable contract to other tools, so a
malformed or hostile field reaches a prompt, a terminal, and a `git fetch`
argument. Accepting any `https://` string was too weak for that job.

Clone and web URLs must now carry no userinfo, query, or fragment, and
their host and path must agree with the owner and repository name the same
object declares. A fork whose clone URL points somewhere else is refused
rather than fetched. Terminal-facing text rejects control characters, which
kept a crafted title from forging extra lines in a consent block. Both
sides must use one Git object format, since a 40-character base with a
64-character head cannot describe a real pull request.

Five snapshot fixtures cover the new refusals.
`make_temp_file` appended each path to `TEMP_FILES` and then printed it, so
every caller read it through command substitution. That runs the function in
a subshell, so the append landed in a child shell and vanished. `TEMP_FILES`
was always empty, and the EXIT trap that walks it deleted nothing; both
commit-message and PR-body temp files leaked one file per invocation.

The function now takes the name of a destination variable and assigns it
with `printf -v`, so it runs in the calling shell and the registration
survives.
`allod pr explain <number> --codex|--claude --output <file>` turns a pull
request into one self-contained HTML report built to be understood, not
skimmed. It resolves an immutable snapshot, runs the selected installed
subscription CLI against a detached checkout, validates what comes back, and
only then writes the report.

The provider flag is the disclosure boundary. Before any source leaves the
machine the command prints the repository, pull request, both immutable
commits, and the runner, then strips API-key and provider-override variables
from the runner's environment so the run cannot silently fall back to a
metered API. It never comments, commits, pushes, or edits the pull request.

Reports reuse one hashed component vocabulary instead of hand-built CSS. The
report agent authors semantic HTML only; the tool supplies the stylesheet and
script byte-for-byte and the validator refuses anything else. That is the
fix for the narrow-screen drift in earlier hand-built reports: a connector is
a pseudo-element owned by the node it follows, so a node and its arrow cannot
disagree about alignment, and direction changes by swapping an authored glyph
rather than rotating a box. JavaScript is pure enhancement, so the report is
complete in Forge's script-free iframe.

Validation is fail-closed and runs against a staged copy. It refuses external
or network-capable markup, modified template bodies, secret-looking content,
provenance that disagrees with the snapshot, inaccessible quiz or figure
structures, and unresolvable internal links. The previous report and the job
directory survive every failure, and publication is an atomic rename.

Assets live under pr-explain/ and resolve the way pm/ already does, through
ALLOD_TOOLS_DIR, the script's own directory, then the workspace checkout.

Tests use mocked forge and runners and spend no provider tokens: 249 command
cases covering same-repo and fork heads, SHA movement, wrong and dirty
checkouts, empty diffs, runner failure and silence, overwrite refusal, dry
run, environment sanitization, atomic preservation, and both runner command
shapes; 68 component cases; and 54 validator cases driven by sabotage
fixtures mutated from a report the real generator produced.

Refs allod/tools#138
Move the PR-explanation generator, template/asset embedding, and
report validator out of the root allod dispatcher into pr-explain/
(explain, validate-report, lib.sh), following the pm/pm resolver and
packaging convention: allod resolves the tool directory via
ALLOD_TOOLS_DIR, its own script dir, or a WORK_DIR checkout, then
execs into it. Root allod now contains only dispatch/resolution glue
for the pr namespace. pr-explain/validate-report is also a documented
standalone entry point; allod pr _validate-report delegates to it.

Behavior is unchanged: all 371 existing pr-explain tests pass without
modification.
The active-markup, network-URL, inline-style, and CSS-behavior
heuristics scanned the whole report file, so a code walk that
faithfully quoted style=, onclick=, url(...), @import, or
transform: rotate(...) from a frontend diff — legitimately escaped
text inside <pre>/<code> — tripped them as false positives.

Add pr_explain_strip_quoted_code, which drops text content inside
<pre>/<code> (keeping tags themselves, so a real element smuggled in
unescaped is still visible) before those five checks run. The
canonical template <style>/<script> blocks are skipped verbatim since
they're already byte-compared separately, so real CSS/JS punctuation
in them can't desync the tag scan.

Add tests proving: an escaped code walk quoting all five patterns is
accepted; the same pattern as plain prose outside any code element is
still rejected; and a live attribute on a real element nested inside
a code block is still rejected.
pr_explain_is_safe_utf8 checked byte-sequence structural validity and
ASCII control bytes, but let several dangerous-but-valid codepoints
through: C1 controls (U+0080-U+009F), a byte-order mark (U+FEFF),
U+2028/U+2029 line/paragraph separators, and the zero-width and
bidi/format controls (U+200B-U+200F, U+202A-U+202E, U+2066-U+2069)
that can reorder or hide rendered text without any invalid byte
sequence.

Track the lead and second bytes of each multi-byte sequence and check
completed sequences against these codepoint ranges once decoded.
Add one sabotage test per class and a normal-Unicode acceptance case
(accents, CJK, em dash, emoji) proving the check isn't just rejecting
all non-ASCII text.
Every fail-closed postcondition check after the provider runs (body
symlink/non-regular, body inode replaced in place, snapshot tamper,
detached-checkout HEAD moved, detached-checkout left dirty) previously
had no test forcing a mock runner to actually violate it. Extend the
mock runner with body-symlink, body-replace, tamper-snapshot,
move-head, and dirty-worktree modes, each sabotaging exactly one
postcondition after a nominally successful run, and assert the
command fails closed, does not publish, and preserves diagnostics.

Add moved-base and moved-fork-head fixtures/scenarios: dedicated
git branches and forge-mock cases proving the base ref and a fork's
head ref are independently verified against the snapshot, not just
the same-repo head ref the existing "moved" test already covered.

Add a publication-race mode where the mock runner writes directly to
the real output path mid-run (simulating a file appearing after the
pre-run existence check). Prove no-clobber mode refuses to overwrite
it and leaves it byte-for-byte intact, while --replace overwrites it —
matching --replace's documented "authorizes replacement of that local
artifact" consent even under a race.

command.sh: 249 -> 287 tests; all pr-explain suites remain green.
Report both line numbers on a duplicate id instead of just the
colliding value, so a report author doesn't have to grep for the
second occurrence themselves.

Strengthen the secret scan: accept ':' as well as '=' as the
key/value delimiter (colon-delimited JSON/YAML-shaped credentials),
widen the key-name vocabulary (api_key, access_key, auth_token,
client_secret, private_key, pwd), and add two realistic shorter/
common shapes with low false-positive risk: Stripe-style prefixed
keys (sk_live_/rk_live_/pk_live_) and JWT-shaped bearer tokens
(three dot-separated base64url segments starting with eyJ).

Add negative fixtures for each new shape and a positive fixture
proving ordinary prose that merely names password/token/api_key
fields (not glued to a contiguous value) still passes. Document the
scan as defense-in-depth, not a completeness guarantee.
A stale forge installed on PATH shadowed a source checkout's own
compatible forge, since pr-explain always did a plain `command -v
forge`. Add pr_explain_resolve_forge: it prefers the forge shipped
beside the resolved allod tools root (the same directory
resolve_pr_explain_dir picked pr-explain from — ALLOD_TOOLS_DIR, a
source checkout, or a $WORK_DIR/allod/tools checkout), falls back to
PATH for installs where forge ships as its own package, and honors an
explicit ALLOD_PR_EXPLAIN_FORGE override for tests/development. Both
the snapshot and discussion calls now use the same resolved binary.

Tests previously relied on prepending a mock forge onto PATH, which
masked this exact bug; they now inject the mock explicitly via
ALLOD_PR_EXPLAIN_FORGE, plus new regression coverage for the
tools-root-beats-stale-PATH case and malformed/missing overrides.
Supported runners like Claude Code's Write tool atomically replace a
file (write a temp file, then rename it over the target) rather than
writing into it in place, so the inode-identity postcondition rejected
every real Claude run with "replaced the staged report instead of
populating it in place" even though the result was a legitimate
regular file at the exact staging path.

Drop the inode-identity comparison and instead require a regular,
non-symlink body at the canonical job-dir path (rejecting
symlink/FIFO/device/directory/missing bodies and path escapes via
realpath). To avoid a TOCTOU between that check and later
assembly/validation, capture the body through an opened file
descriptor whose fstat is cross-checked against the preceding lstat,
then read only from that descriptor into a new tool-owned snapshot
file in the private job directory; assembly and validation operate
solely on those immutable captured bytes. Update prompt.md to describe
atomic replacement as acceptable instead of instructing populate-in-place.

Flip the body-replace mock test from a rejected case into a passing
atomic-replace case, and keep the symlink mock (whose target lives
outside the job directory) as the retained outside-path/symlink
failure case.
Two attended runs produced bodies the validator correctly rejected — a
provenance runner label used where the raw id belongs, skipped heading
levels, and a diagram outside its figure. The contracts are right; asking
for one-shot perfection against a long component gallery is not ergonomic
for an on-demand command.

When the assembled report fails validation, hand the validator's exact
diagnostics back to the same explicitly consented provider for exactly one
repair pass, then validate the result identically. A run now costs at most
two provider calls, and the disclosure block says so before any source is
sent; --no-repair keeps a run to one call for strict cost control.

The repair prompt carries only the canonical body path, the diagnostics as
delimited data, and the immutable authoring rules. Diagnostics are
sanitized and reach the runner on stdin only, never as an argument, because
some of them quote body-derived text. Both passes go through one runner
invocation helper, so the hardened argv and sanitized environment cannot
drift apart, and every cage postcondition — safe body capture, snapshot
digest, detached HEAD, clean worktree — is re-checked after the repair.
Each pass keeps its own prompt, runner logs, captured body, and
diagnostics in the preserved job directory.

The validator is unchanged. The prompt's final check now leads with the
three contracts real runs actually broke.
Covers four review items with tests only, no production change needed:

- A pre-placed --output symlink: the default path refuses without
  touching the symlink's target, and --replace (already using mv -fT,
  a same-directory rename that never follows the link) replaces the
  symlink itself and leaves the target byte-for-byte untouched, per
  its documented "replaces that local artifact only" semantics.
- Repair-prompt diagnostic neutralization: a diagnostic crafted with a
  backtick, a control byte, and an attempted "END VALIDATOR
  DIAGNOSTICS" spoof reaches the repair pass neutralized, bounded to
  300 characters, never in argv, and unable to forge an early close of
  the diagnostics envelope. A second case proves the 60-diagnostic cap
  and its "further diagnostics omitted" notice.
- E19 secret-looking-content: an explicit case proves a credential-
  shaped value quoted inside <code> is still blocked, since the scan
  is defense-in-depth over raw bytes, not exempting any element.

Skipped: a deterministic test for the lstat/open/fstat capture race
in pr_explain_capture_run_result. That gap is strictly after the
runner process has exited, with the tool itself as the only writer, so
closing it in a test requires a production-only seam (an env-gated
hook to swap the file mid-check) that would carve a new code path
through the exact cage the test is meant to verify. Given the
instruction to avoid sleeps/flakiness and to skip rather than
complicate production for this case, it's left uncovered here.
Forgejo reports an AGit-created PR's head as refs/pull/<n>/head, its own
pull namespace, instead of a pushed branch. The fetch logic always
prepended refs/heads/, so `allod pr explain` failed with "couldn't find
remote ref refs/heads/refs/pull/<n>/head" against a real repository.

Normalize every snapshot ref into an exact remote ref before any git
invocation: ordinary names become refs/heads/<name>, and the one explicit
full ref accepted is refs/pull/<n>/head for head, only when <n> matches
the pull request. Base stays an ordinary branch. Everything else —
arbitrary refs/*, a leading dash, whitespace/control bytes, :^~*,
traversal — is rejected up front, both in the new bash-side normalizer
and in the forge/pr-explain snapshot schema validation, which previously
just required a non-empty control-free string.
allod-agent commented 2026-08-16 21:24:40 +01:00
Author
Member
Copy link

Copied from #139 — originally posted by vnprc-agent on 2026-08-15.

Owner review artifact generated by this PR itself: https://forge.anarch.diy/vnprc/notes/raw/branch/master/allod/reports/pr-explain-report-tool-explanation.html

The report is self-contained and validated. Please review both its explanation and the visual/interaction language; concrete comprehension feedback can drive the next template iteration.

_Copied from #139 — originally posted by vnprc-agent on 2026-08-15._ Owner review artifact generated by this PR itself: https://forge.anarch.diy/vnprc/notes/raw/branch/master/allod/reports/pr-explain-report-tool-explanation.html The report is self-contained and validated. Please review both its explanation and the visual/interaction language; concrete comprehension feedback can drive the next template iteration.
allod-agent commented 2026-08-16 21:24:41 +01:00
Author
Member
Copy link

Copied from #139 — originally posted by vnprc-agent on 2026-08-16.

Review finding

High — interrupted runs are reported as success and leak the private job directory. pr-explain/explain:17-33 only installs an EXIT trap and decides whether to preserve/report the job from $?. In Bash, an interactive SIGINT received while waiting for a foreground child does not make the shell exit non-zero by itself; after codex/claude is interrupted, execution can continue (or a later command can become the final status), and an external SIGTERM can terminate the shell with the EXIT trap observing status 0. That means Ctrl-C or service shutdown can leave .allod-pr-explain.* behind without printing diagnostics preserved, and can in some paths continue toward publication. This is especially problematic because the directory contains the complete diff and review context. Install explicit INT/TERM handlers that record a non-zero interrupted state (and terminate the provider/process group as needed), make the EXIT cleanup/report path use that state, and add mocked slow-runner tests for both signals verifying non-zero exit, no publication, and an announced preserved directory.

Addressed on this PR by df54ab0 ("pr-explain: fail closed on provider interruption"), which is the one commit this PR carries beyond #139. See the Signal handling fix and Validation sections of the description.

_Copied from #139 — originally posted by vnprc-agent on 2026-08-16._ ## Review finding **High — interrupted runs are reported as success and leak the private job directory.** `pr-explain/explain:17-33` only installs an `EXIT` trap and decides whether to preserve/report the job from `$?`. In Bash, an interactive SIGINT received while waiting for a foreground child does not make the shell exit non-zero by itself; after `codex`/`claude` is interrupted, execution can continue (or a later command can become the final status), and an external SIGTERM can terminate the shell with the EXIT trap observing status 0. That means Ctrl-C or service shutdown can leave `.allod-pr-explain.*` behind without printing `diagnostics preserved`, and can in some paths continue toward publication. This is especially problematic because the directory contains the complete diff and review context. Install explicit `INT`/`TERM` handlers that record a non-zero interrupted state (and terminate the provider/process group as needed), make the EXIT cleanup/report path use that state, and add mocked slow-runner tests for both signals verifying non-zero exit, no publication, and an announced preserved directory. **Addressed on this PR** by `df54ab0` ("pr-explain: fail closed on provider interruption"), which is the one commit this PR carries beyond #139. See the Signal handling fix and Validation sections of the description.
vnprc-agent commented 2026-08-18 06:18:33 +01:00
Contributor
Copy link

Owner-requested direction change: comprehension redesign lands on this PR

The owner reviewed a research pass on how this tool teaches (learning-science survey: cognitive load theory, Mayer's multimedia principles, expertise reversal, ICAP, retrieval practice, backward design, program-comprehension research) and chose to extend this PR rather than replace it, so the tool keeps being dogfooded on its own PR. The snapshot interface, provider cage, invariant validator, and component vocabulary stay; the single-shot authoring prompt is replaced by a staged pipeline. Requested changes, to land as commit series here:

  1. Triage stage before generation: a cheap structured assessment emitting a concept inventory, prerequisite gaps, developer-question list, complexity tier (T0-T3), and a reading-time budget. T0 declines to generate (PR body suffices) unless --force-tier overrides.
  2. Backward design: a learning-objectives stage; every section, figure, and quiz item downstream carries an objective tag the validator checks for coverage; untagged content is a defect.
  3. Layered structure: decision -> concept -> mechanism -> receipts, each an independently coherent stopping point with a declared time cost; prediction-before-reveal rhythm at the concept and mechanism layers; retrieval interleaved at section boundaries instead of one end quiz.
  4. A separate deletion-only slop pass: a second model call that may only cut - untagged sentences, repeated claim structures, hedging - backed by a mechanical slop linter in the validator.
  5. Vocabulary extensions: inline SVG under a strict validator grammar (no scripts, no external refs), prediction widgets, scrubbable state steppers, and a declarative toy-model pattern - all no-JS complete, self-containment invariants unchanged.
  6. Retention layer: quiz items carry machine-readable concept tags and front/back text; first-try answer capture plus a local results-export file feed a reader-knowledge model and a plain-text spaced-repetition deck (SM-2, ~40 lines of shell) reviewed through the same HTML quiz components. No external tools, no network, opt-in only.
  7. Concept primers: cached, PR-independent explainers of one subsystem each, linked by reports instead of inlining background; private-first with a sanitizing promotion path to a public docs/primers/.

Witness for each slice: regenerate this PR's own self-explanation report and judge the comprehension gain directly; merge when the owner's comprehension gate passes.

## Owner-requested direction change: comprehension redesign lands on this PR The owner reviewed a research pass on how this tool teaches (learning-science survey: cognitive load theory, Mayer's multimedia principles, expertise reversal, ICAP, retrieval practice, backward design, program-comprehension research) and chose to extend this PR rather than replace it, so the tool keeps being dogfooded on its own PR. The snapshot interface, provider cage, invariant validator, and component vocabulary stay; the single-shot authoring prompt is replaced by a staged pipeline. Requested changes, to land as commit series here: 1. **Triage stage before generation**: a cheap structured assessment emitting a concept inventory, prerequisite gaps, developer-question list, complexity tier (T0-T3), and a reading-time budget. T0 declines to generate (PR body suffices) unless `--force-tier` overrides. 2. **Backward design**: a learning-objectives stage; every section, figure, and quiz item downstream carries an objective tag the validator checks for coverage; untagged content is a defect. 3. **Layered structure**: decision -> concept -> mechanism -> receipts, each an independently coherent stopping point with a declared time cost; prediction-before-reveal rhythm at the concept and mechanism layers; retrieval interleaved at section boundaries instead of one end quiz. 4. **A separate deletion-only slop pass**: a second model call that may only cut - untagged sentences, repeated claim structures, hedging - backed by a mechanical slop linter in the validator. 5. **Vocabulary extensions**: inline SVG under a strict validator grammar (no scripts, no external refs), prediction widgets, scrubbable state steppers, and a declarative toy-model pattern - all no-JS complete, self-containment invariants unchanged. 6. **Retention layer**: quiz items carry machine-readable concept tags and front/back text; first-try answer capture plus a local results-export file feed a reader-knowledge model and a plain-text spaced-repetition deck (SM-2, ~40 lines of shell) reviewed through the same HTML quiz components. No external tools, no network, opt-in only. 7. **Concept primers**: cached, PR-independent explainers of one subsystem each, linked by reports instead of inlining background; private-first with a sanitizing promotion path to a public `docs/primers/`. Witness for each slice: regenerate this PR's own self-explanation report and judge the comprehension gain directly; merge when the owner's comprehension gate passes.
vnprc-agent commented 2026-08-18 13:18:50 +01:00
Contributor
Copy link

The redesign's first three slices are implemented and committed on this branch, awaiting relay-push (two commits: pr-explain: teach through a staged comprehension pipeline, pr-explain: read identifiers from markup tags, not raw bytes).

What landed

  • Triage pass (triage-prompt.md): investigates the checkout and writes a schema-gated triage.json — tier T0–T3, decision risk, reading budget, concept inventory, developer questions, and 1–7 verb-first learning objectives. A T0 verdict declines to generate; --force-tier <T1|T2|T3> overrides in either direction and is recorded in the judgment. The pass is caged like the others: snapshot immutable, checkout untouched, body still empty.
  • Report grammar v2: masthead prices its stopping points (p.rx-cost); main opens with an objectives block mirroring triage ids; every main section carries data-layer in non-decreasing concept → mechanism → receipts order; sections, figures, and quiz items carry data-objective, and the validator proves every objective is both taught and tested. Quiz items (3–7, interleavable at section ends) carry data-concept; no answer letter may be correct more than twice.
  • Slop pass (slop-prompt.md, skippable with --no-slop): deletion-only tightening, enforced by the shell — the output must be byte-smaller than the author draft. A mechanical linter backs it: banned-vocabulary errors, repeated sentence-opening trigram errors, hedge-density warnings.
  • Cost ceiling: at most 4 provider calls (triage, author, slop, one repair), disclosed in the consent block.
  • Tests: mock runner now serves per-pass behaviors; 737 tests across the three suites cover the T0 decline, forced tiers, triage cage deaths, the slop size guard, per-scenario call accounting, and every new validator rule.

Dogfood result

The first attended v2 run explained this PR itself. Triage judged T3 ("a layered trust model … it rests on Bash signal-disposition and AGit ref-namespace prerequisites the reviewer likely lacks"). The authored report then got rejected by the tool's own validator — and the rejection was a genuine validator bug: the report quoted PR_EXPLAIN_PROVIDER_PID="" in a codewalk, and the E3 identifier census read raw bytes, so PID="" counted as an id attribute. Identifier and reference accounting now runs on a markup tag stream, with regression probes in both directions. The report validates cleanly under the fixed validator; it is stored in the owner's notes for the comprehension merge gate.

Remaining slices from the recorded plan: SVG + interactive vocabulary, primer library, reader model + spaced-repetition cards.

The redesign's first three slices are implemented and committed on this branch, awaiting relay-push (two commits: `pr-explain: teach through a staged comprehension pipeline`, `pr-explain: read identifiers from markup tags, not raw bytes`). **What landed** - **Triage pass** (`triage-prompt.md`): investigates the checkout and writes a schema-gated `triage.json` — tier T0–T3, decision risk, reading budget, concept inventory, developer questions, and 1–7 verb-first learning objectives. A T0 verdict declines to generate; `--force-tier <T1|T2|T3>` overrides in either direction and is recorded in the judgment. The pass is caged like the others: snapshot immutable, checkout untouched, body still empty. - **Report grammar v2**: masthead prices its stopping points (`p.rx-cost`); `main` opens with an objectives block mirroring triage ids; every main section carries `data-layer` in non-decreasing concept → mechanism → receipts order; sections, figures, and quiz items carry `data-objective`, and the validator proves every objective is both taught and tested. Quiz items (3–7, interleavable at section ends) carry `data-concept`; no answer letter may be correct more than twice. - **Slop pass** (`slop-prompt.md`, skippable with `--no-slop`): deletion-only tightening, enforced by the shell — the output must be byte-smaller than the author draft. A mechanical linter backs it: banned-vocabulary errors, repeated sentence-opening trigram errors, hedge-density warnings. - **Cost ceiling**: at most 4 provider calls (triage, author, slop, one repair), disclosed in the consent block. - **Tests**: mock runner now serves per-pass behaviors; 737 tests across the three suites cover the T0 decline, forced tiers, triage cage deaths, the slop size guard, per-scenario call accounting, and every new validator rule. **Dogfood result** The first attended v2 run explained this PR itself. Triage judged **T3** ("a layered trust model … it rests on Bash signal-disposition and AGit ref-namespace prerequisites the reviewer likely lacks"). The authored report then got rejected by the tool's own validator — and the rejection was a genuine validator bug: the report quoted `PR_EXPLAIN_PROVIDER_PID=""` in a codewalk, and the E3 identifier census read raw bytes, so `PID=""` counted as an id attribute. Identifier and reference accounting now runs on a markup tag stream, with regression probes in both directions. The report validates cleanly under the fixed validator; it is stored in the owner's notes for the comprehension merge gate. Remaining slices from the recorded plan: SVG + interactive vocabulary, primer library, reader model + spaced-repetition cards.
vnprc-agent commented 2026-08-18 15:06:33 +01:00
Contributor
Copy link

Live-tested the interruption fix (pr-explain: fail closed on provider interruption) against real runs of this PR, since the suite only ever proved it against a mock.

Live results (interrupting attended claude runs mid-triage and mid-author):

  • SIGTERM: exit 143 within ~1s, interrupted by SIGTERM, the real claude CLI process died from the forwarded TERM, no report published, diagnostics preserved.
  • SIGINT: exit 130 within ~1s, interrupted by SIGINT, same fail-closed behavior — but only once the signal was deliverable at all (see below).

One suite defect found and fixed (pr-explain: make the interruption tests non-vacuous): the INT test case launched the command as a backgrounded job, so explain inherited SIGINT ignored at entry per the POSIX background rule — and bash cannot trap a signal ignored at entry. The trap under test could never fire. The case passed anyway because the harness then hand-killed the provider, producing every asserted symptom (non-zero exit, no report, preserved diagnostics) with zero interruption handling involved. The case now launches under env --default-signal=SIGINT,SIGQUIT, never touches the provider, and asserts what the live probe checked: the provider dies from the trap's forwarded TERM, the exit status is exactly 130/143, and the diagnosis names the signal.

The product code needed no changes — the fix was correct; its strongest test was fiction. Three commits now await relay: the pipeline, the tag-stream validator fix, and this.

Live-tested the interruption fix (`pr-explain: fail closed on provider interruption`) against real runs of this PR, since the suite only ever proved it against a mock. **Live results** (interrupting attended claude runs mid-triage and mid-author): - SIGTERM: exit 143 within ~1s, `interrupted by SIGTERM`, the real claude CLI process died from the forwarded TERM, no report published, diagnostics preserved. - SIGINT: exit 130 within ~1s, `interrupted by SIGINT`, same fail-closed behavior — but only once the signal was deliverable at all (see below). **One suite defect found and fixed** (`pr-explain: make the interruption tests non-vacuous`): the INT test case launched the command as a backgrounded job, so explain inherited SIGINT *ignored at entry* per the POSIX background rule — and bash cannot trap a signal ignored at entry. The trap under test could never fire. The case passed anyway because the harness then hand-killed the provider, producing every asserted symptom (non-zero exit, no report, preserved diagnostics) with zero interruption handling involved. The case now launches under `env --default-signal=SIGINT,SIGQUIT`, never touches the provider, and asserts what the live probe checked: the provider dies from the trap's forwarded TERM, the exit status is exactly 130/143, and the diagnosis names the signal. The product code needed no changes — the fix was correct; its strongest test was fiction. Three commits now await relay: the pipeline, the tag-stream validator fix, and this.
vnprc-agent commented 2026-08-19 15:57:44 +01:00
Contributor
Copy link

Requested change 8 (recorded before implementing, per convention): remove the AGit acceptance carve-out from allod pr explain.

With the forge set to stop accepting AGit submissions (per the open governance issue on the forge configuration), the carve-out in pr_explain_normalize_ref and the pull_head_ref clause of the snapshot schema serve only a flow the stack is outlawing. Removing them:

  • tightens the invariant to one sentence — both refs must be ordinary branch names — which deletes a prerequisite concept from every future review of this code (triage flagged AGit ref-namespace background as a reviewer gap; after this it has no reason to);
  • makes the tool a policy enforcement point: an AGit-shaped pull request fails closed at snapshot validation, before any fetch or provider disclosure, with a diagnosis that names AGit so the refusal explains itself — correct behavior both before and after the forge-side door closes;
  • keeps every rejection: the explicit-ref refusals, check-ref-format, and the dangerous-syntax checks all stand. Only the acceptance goes.

One deliberate asymmetry: forge pr snapshot keeps describing AGit-created PRs faithfully. It is a read command with no side effects, and historical AGit PRs exist on the forge; a reader that refuses to describe reality hurts archaeology without adding safety. Reads describe reality; actions enforce policy. The two snapshot schemas (forge-side and explain-side) become intentionally different at exactly this clause, and the docs will say so.

Test changes: the two AGit acceptance cases flip to rejection cases asserting the named diagnosis and that no provider is invoked; the existing rejection cases (mismatched number, arbitrary explicit ref, dangerous syntax, AGit-shaped base) stay as they are.

This also refines my earlier note on the governance issue: the warning there — don't delete the validation half when closing the gap — stands. What goes here is the acceptance half, which that warning was never meant to protect.

Requested change 8 (recorded before implementing, per convention): **remove the AGit acceptance carve-out from `allod pr explain`.** With the forge set to stop accepting AGit submissions (per the open governance issue on the forge configuration), the carve-out in `pr_explain_normalize_ref` and the `pull_head_ref` clause of the snapshot schema serve only a flow the stack is outlawing. Removing them: - tightens the invariant to one sentence — *both refs must be ordinary branch names* — which deletes a prerequisite concept from every future review of this code (triage flagged AGit ref-namespace background as a reviewer gap; after this it has no reason to); - makes the tool a policy enforcement point: an AGit-shaped pull request fails closed at snapshot validation, before any fetch or provider disclosure, with a diagnosis that names AGit so the refusal explains itself — correct behavior both before and after the forge-side door closes; - keeps every rejection: the explicit-ref refusals, `check-ref-format`, and the dangerous-syntax checks all stand. Only the acceptance goes. One deliberate asymmetry: `forge pr snapshot` keeps describing AGit-created PRs faithfully. It is a read command with no side effects, and historical AGit PRs exist on the forge; a reader that refuses to describe reality hurts archaeology without adding safety. Reads describe reality; actions enforce policy. The two snapshot schemas (forge-side and explain-side) become intentionally different at exactly this clause, and the docs will say so. Test changes: the two AGit acceptance cases flip to rejection cases asserting the named diagnosis and that no provider is invoked; the existing rejection cases (mismatched number, arbitrary explicit ref, dangerous syntax, AGit-shaped base) stay as they are. This also refines my earlier note on the governance issue: the warning there — don't delete the *validation* half when closing the gap — stands. What goes here is the acceptance half, which that warning was never meant to protect.
vnprc-agent commented 2026-08-19 16:31:55 +01:00
Contributor
Copy link

Owner completed the comprehension gate on the v2 self-explanation report. Recording the reader-measurement results verbatim in substance, because they are the first real datapoint the measurement layer has produced:

  1. Reading-time estimate badly wrong. The rx-cost budget assumes the reader holds the prerequisites; a reader missing them doesn't run 20% over, they run multiples over.
  2. Register too technical for a reader who does not know bash. Concept-layer text was written at receipts-layer density. The layered design licenses jargon in exactly one place — the receipts layer; the concept layer is contractually the plain-language mental model. This is a prompt defect, not a tradeoff.
  3. Prerequisite gaps named but never closed. Triage correctly identified the reviewer gaps (signal dispositions, ref namespaces) and the report had no primers to link — the exact failure the primer-library slice exists to fix.

Consequences proposed (pending owner confirmation):

  • Reorder the remaining slices by measured pain: (A) prompt-level register fixes — plain voice at the concept layer, terms defined on first use, triage defaults to a novice-in-this-language reader, cost estimates padded for missing prerequisites; (B) primer library, promoted; (C) reader model + quiz capture + spaced repetition; (D) SVG + interactive vocabulary, demoted from first to last. Each ships as its own PR after this one merges.
  • This PR merges as-is once the four pending commits land. The gate question was whether the owner built enough of a model to judge this PR; the findings above are themselves the evidence that the reading produced a working model — the report's weaknesses were identified in the design's own vocabulary.

This is the feedback loop the plan called "measurement"; the first cycle ran on a human, as intended.

Owner completed the comprehension gate on the v2 self-explanation report. Recording the reader-measurement results verbatim in substance, because they are the first real datapoint the measurement layer has produced: 1. **Reading-time estimate badly wrong.** The rx-cost budget assumes the reader holds the prerequisites; a reader missing them doesn't run 20% over, they run multiples over. 2. **Register too technical for a reader who does not know bash.** Concept-layer text was written at receipts-layer density. The layered design licenses jargon in exactly one place — the receipts layer; the concept layer is contractually the plain-language mental model. This is a prompt defect, not a tradeoff. 3. **Prerequisite gaps named but never closed.** Triage correctly identified the reviewer gaps (signal dispositions, ref namespaces) and the report had no primers to link — the exact failure the primer-library slice exists to fix. Consequences proposed (pending owner confirmation): - **Reorder the remaining slices by measured pain**: (A) prompt-level register fixes — plain voice at the concept layer, terms defined on first use, triage defaults to a novice-in-this-language reader, cost estimates padded for missing prerequisites; (B) primer library, promoted; (C) reader model + quiz capture + spaced repetition; (D) SVG + interactive vocabulary, demoted from first to last. Each ships as its own PR after this one merges. - **This PR merges as-is** once the four pending commits land. The gate question was whether the owner built enough of a model to judge this PR; the findings above are themselves the evidence that the reading produced a working model — the report's weaknesses were identified in the design's own vocabulary. This is the feedback loop the plan called "measurement"; the first cycle ran on a human, as intended.
vnprc force-pushed agent/pr-explain-report-tool from df54ab0ec6 to 4e6a22cff8 2026-08-19 16:53:16 +01:00 Compare 
Replace the single-shot authoring prompt with a staged pipeline built on
learning science (backward design, cognitive load theory, retrieval
practice), per the redesign recorded on PR #141.

Pipeline: a triage pass now precedes authoring and writes a schema-gated
judgment (tier T0-T3, decision risk, reading budget, concept inventory,
developer questions, learning objectives) to triage.json. A T0 verdict
declines to generate: the PR body suffices, and --force-tier <T1|T2|T3>
is the operator's override in either direction, recorded in the judgment
itself. After the author pass, a deletion-only slop pass tightens prose
(--no-slop skips it); the shell enforces that it can only cut. The
bounded repair pass is unchanged. A full run costs at most four provider
calls, disclosed up front, and the triage pass is caged like every other
pass: snapshot immutable, checkout untouched, body still empty.

Report grammar v2: the masthead prices its stopping points (p.rx-cost);
main opens with an objectives block whose obj-N ids mirror triage; every
main section carries data-layer in non-decreasing concept, mechanism,
receipts order so each layer is a safe stopping point; sections, figures,
and quiz items carry data-objective, and the validator proves every
objective is both taught and tested; quiz items (now 3-7, interleavable
at section ends) carry data-concept for the future retention layer, with
no answer letter correct more than twice. A mechanical slop linter
rejects banned AI-slop vocabulary and repeated sentence-opening trigrams
and warns on hedge density, so the deletion pass has teeth.

The prompts are rewritten around the same contract: triage-prompt.md
demands investigation before judgment, prompt.md authors backward from
the triage objectives under a four-layer structure with prediction-
before-reveal rhythm and a plain-declarative voice spec, and
slop-prompt.md may only delete. Gallery and docs describe the v2
vocabulary; the gallery still passes the production validator.

Tests: the mock runner now serves per-pass behaviors (triage judgment,
author body, slop rewrite, repair), fixtures speak the v2 grammar, and
the suites cover the T0 decline, forced tiers, triage cage deaths, the
slop size guard, per-scenario call accounting, and every new validator
rule. 733 tests pass across the three suites.

Co-authored via parallel subscription-CLI subagents working to a shared
spec; all self-containment, no-network, no-hidden-content, and
provenance invariants are unchanged.
The first attended v2 run explained this tool's own pull request and was
rejected by its own validator: the report faithfully quoted the line
PR_EXPLAIN_PROVIDER_PID="" inside a codewalk, the E3 identifier census
grepped the raw report bytes case-insensitively, and PID="" counted as an
id attribute the canonical-syntax collector refused, so the counts
disagreed. Any report quoting attribute-shaped code — which a report
about an HTML-generating tool cannot avoid — would fail the same way.
The body was valid; the validator was wrong.

Identifier and reference accounting (id census, duplicate detection,
single-quote rejection, fragment-link and accessibility-reference
resolution, anchor counting) now runs on a tag stream extracted under
the one-tag-per-line grammar, excluding all text content and the
canonical template style/script payloads. Text in quoted code or prose
can no longer impersonate markup, while a malformed id carried by a
real tag still fails closed.

Regression probes cover both directions: a spliced section quoting
PID="", pid="worker-7", and href="#local" inside code must validate,
and a real tag carrying id="9lives" must still be rejected.
An attended live probe of the interruption handling found the fix works
and its INT test never tested it. The suite launched the command as a
backgrounded job, so the explain process inherited SIGINT ignored at
entry (the POSIX background rule), and bash cannot trap a signal that
was ignored at entry: the trap under test could not fire. The case
passed anyway because the harness then killed the provider itself,
which produced every asserted symptom — non-zero exit, no report,
preserved diagnostics — without any interruption handling running.

Live, with default dispositions restored, both signals behave as the
fix promises: SIGINT and SIGTERM each exit within about a second with
status 130 or 143, print "interrupted by SIG...", kill the real claude
subscription CLI through the forwarded TERM, publish nothing, and
preserve diagnostics. The probes interrupted real runs mid-triage and
mid-author on this tool's own pull request.

The suite now launches the case under env --default-signal so the
signal is deliverable, never touches the provider, and asserts what
the live probe checked: the provider process dies from the trap's
forwarded TERM, the exit status is exactly 130 or 143, and the
diagnosis names the signal.
The snapshot schema and ref normalizer accepted one explicit ref shape,
the Forgejo AGit pull namespace refs/pull/<n>/head, so that an
AGit-created pull request could be explained. The stack has since
decided AGit submissions are not part of the allod workflow at all:
they exist only because the AGit namespace bypasses the public
repository push denial, and that door is being closed on the forge.

Accepting the shape therefore served only a flow the stack outlaws,
and it cost every future reviewer of this code a prerequisite concept
— the triage pass itself judged AGit ref-namespace background a
reviewer gap. Drop the acceptance: both refs must now be ordinary
branch names, full stop. An AGit-shaped head fails closed at snapshot
validation, before any fetch or provider disclosure, with a diagnosis
that names AGit as the cause instead of a generic schema failure. The
normalizer keeps a matching refusal as defense in depth, and every
existing rejection — explicit refs, dangerous ref syntax, the
AGit-shaped base — stands unchanged.

Deliberate asymmetry: forge pr snapshot still describes AGit-created
pull requests faithfully. It is a read with no side effects, and such
pull requests exist on forges; a reader that refuses to describe
reality hurts archaeology without adding safety. Reads describe
reality; actions enforce policy. The two snapshot schemas now differ
at exactly this clause, and both docs say so.

Requested on the pull request as change 8 before implementing.
vnprc merged commit 58fb26c459 into master 2026-08-19 17:20:47 +01:00
vnprc deleted branch agent/pr-explain-report-tool 2026-08-19 17:20:47 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug
Something is not working

  
duplicate
This issue or pull request already exists

  
enhancement
New feature

  
help wanted
Need some help

  
invalid
Something is wrong

  
question
More information is needed

  
wontfix
This won't be fixed

  
bug
Something is not working

  
duplicate
This issue or pull request already exists

  
enhancement
New feature

  
help wanted
Need some help

  
invalid
Something is wrong

  
question
More information is needed

  
wontfix
This won't be fixed

No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
allod/tools!141
No description provided.