pr-explain: fail closed on provider interruption

allod-agent commented

2026-08-16 21:03:37 +01:00

Member

No description provided.

allod-agent added 15 commits

2026-08-16 21:03:37 +01:00

forge: add stable pull request snapshots 1dd4ce1dbc

forge: bind snapshot URLs to the repository identity they claim d57cc65b18

`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.

allod: keep temp files registered for cleanup df3c1c635a

`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: add 'pr explain', a comprehension-first PR report generator 086cf8131a

`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

pr-explain: extract implementation out of root allod 35af21f29d

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.

pr-explain: scope active-markup/CSS/network heuristics to live structure 32742228bd

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: reject C1 controls, BOM, and bidi/format controls 8a1d0b6559

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.

pr-explain: test every post-run cage postcondition with a mock runner 0e3f15f7f7

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.

pr-explain: sharpen duplicate-id diagnostic and secret scan aa2223401c

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.

pr-explain: resolve forge beside the allod tools root, not PATH 0b9bd0189a

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.

pr-explain: accept an atomically replaced staged report body dc3a396044

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.

pr-explain: repair a rejected report body in one bounded extra pass 83373ab688

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.

pr-explain: add release-review regression tests for issue #138 a80ed270b4

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.

pr-explain: normalize AGit-created PR head refs (issue #138 ) 9e60d411af

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.

pr-explain: fail closed on provider interruption 41319603b7

vnprc commented

2026-08-16 21:04:35 +01:00

Owner

oops

vnprc closed this pull request

2026-08-16 21:04:35 +01:00

Pull request closed

Please reopen this pull request to perform a merge.

Rows
Columns

pr-explain: fail closed on provider interruption #140

Pull request closed