feat: let the reviewer see the arguments made against its findings #460
No reviewers
joshtronic
Labels
No labels
Agent
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No assignees
1 participant
Notifications
Total time spent: 16 minutes 50 seconds
Due date
igor
16 minutes 50 seconds
No due date set.
Dependencies
No dependencies set
Reference
joshtronic/igor!460
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/456-feed-dismissals"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Part of #456 — your call on the second question. The rubric question there is answered no: keep it paranoid, since adjudication does the filtering now.
The gap
#455 gave the rework agent standing to dismiss a finding instead of complying, and the harness posts that reasoning to the PR. The reviewer never saw it. Its prompt is title + body + linked issue + CI + diff and nothing else — so it re-raised the same finding the next round, and the argument only ever reached you. The agent was arguing with someone who couldn't hear it.
review_dismissals_sectionpulls the bot's own dismissal comments into the prompt;review_build_prompttakes the bot user as a 9th argument to scope it.Two constraints in the shape of it
Scoped to bot comments carrying
ADJUDICATION_MARKER— not "all PR comments." That would be a new and much wider path into the prompt of the thing gating auto-merge. The marker is an HTML comment rather than the human-facing heading, which carries an emoji and an em dash and would stop matching the first time someone reworded it.Fenced as untrusted despite being bot-authored, because the text is model-generated from a diff that may itself be adversarial. It opens no new channel — the reviewer already reads that diff directly — but laundering hostile text through the agent shouldn't upgrade its trust level.
And the section explicitly tells the reviewer it is not bound by a dismissal: engage with the reason or drop the point, but a wrong argument should still be re-raised with a rebuttal. A dismissal the reviewer had to accept would make it a rubber stamp, which is the opposite of the point.
Marker ownership
lib/adjudication.shwrites it,lib/review.shreads it. Libs here are flat —bin/tick.shdoes all sourcing, and it sources review.sh before adjudication.sh — so rather than introduce lib-to-lib sourcing for one string, review.sh carries a:=fallback and a test asserts the two literals still agree. A drift would leave the reader matching nothing while looking correct.The hole in my first test pass
Worth recording because the mutation run is the only reason it surfaced. My fixture's non-bot comment lacked the marker, so removing the bot-author filter changed nothing and every test still passed:
The marker is a published string anyone can type, so the fixture now gives the human comment the marker too — which is the actual threat model, and which does fail when the filter is dropped:
Four mutants total: marker drift, bot filter, marker filter, and the "you may disagree" language. All caught.
check-sync clean, shellcheck clean.
🤖 Generated with Claude Code
https://claude.ai/code/session_01QfDyMfn2Go73gw21a92wNB
🤖 Review —
COMMENT(automated)CI for
7ad3853a: successFeeds the rework agent's prior dismissal comments back into the reviewer's prompt, scoped to bot-authored comments carrying a new
ADJUDICATION_MARKER. Implementation is clean, in scope (~120 lines), CI is green, and the description's claims all map to real diff hunks. I found no blocking defect, but several things I'd want a human to weigh before this lands, because it changes the input surface of the thing that gates auto-merge.Checklist honesty: all four claimed mutants have a corresponding assertion in
bin/test-review.sh(marker drift →eq "adjudication.sh and review.sh agree on the marker"; bot filter → thecase "$SEC"block; marker filter →eq "an unmarked bot comment is not a dismissal"; disagree language →has ... "NOT bound by them"). The "no bot user → no section" and untrusted-fence claims are also backed. Nothing fabricated.Things to weigh
The production path is entirely mocked (
bin/test-review.sh:205,:222). Every test stubsforgejo_pr_comments, andreview_dismissals_sectionreturns0silently on every failure — non-zero exit, non-array JSON, jq error. If the realforgejo_pr_commentstakes different args, returns a different shape, or doesn't emit.user.login/.body, this feature is a permanent no-op in production and the whole suite still passes green. That is the same class of hole the PR description says the mutation run caught for the bot filter — it's still open one layer down. At minimum, an assertion that the real function exists with the expected arity, or a shape assertion against a captured real payload, would close it.BOT_USERat the call site (bin/tick.sh:3034). I can't see its definition in the diff. Iftick.shruns underset -uandBOT_USERis unset in any path reachingdo_review_tick, this line hard-fails a review tick that previously succeeded. Please confirm it's an unconditionally-set global.Fence escape into the auto-merge gate (
lib/review.sh:246). The untrusted text is interpolated between--- BEGIN/END UNTRUSTED AGENT TEXT ---literals with no escaping. Agent-generated prose derived from an adversarial diff can emit--- END UNTRUSTED AGENT TEXT ---followed by instructions that then read as trusted prompt. The PR argues this opens no new channel since the diff is already in the prompt — fair, but the diff isn't prose specifically arguing "don't raise this finding," and this one is. Worth stripping the delimiter from$textbefore interpolation. (Not blocking on my read, but this is precisely the reservation the human should sign off on rather than me.)Truncation path has no test (
lib/review.sh:239-244).${text: -$REVIEW_DISMISSALS_MAX}and the(TRUNCATED ...)note are untested; a mutant that inverted the slice direction (keeping the head instead of the tail) would pass. Cheap to cover.No staleness scoping. Dismissals from round 1 are fed forward on every subsequent round, including for findings the author later did fix. Combined with "do not re-raise a finding as though it were never answered," this could soften the reviewer on a point that's already moot. Scoping by head sha, or at least noting the round, might be worth a follow-up.
Minor portability:
grep -o ':\s*"${ADJUDICATION_MARKER:=...'(bin/test-review.sh:193) relies on GNU\s; BSD/macOS grep won't match, leavingREV_LITempty and failing the marker-agreement assertion locally rather than skipping.[[:space:]]is portable.Not concerns
:=fallback inlib/review.sh:210plus the cross-file literal assertion is a reasonable answer to the flat-lib constraint; adjudication.sh's unconditional assignment wins at runtime as described.printfpasses$note/$textas arguments, not as format string — no format-injection.${9:-}, so that regression guard is intact.Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.🤖 Review —
COMMENT(automated)CI for
1063f3fe: successFeeds the rework agent's prior dismissal comments back into the reviewer's user turn, scoped to bot-authored comments carrying a new
ADJUDICATION_MARKER, fenced as untrusted. Scope is tight, CI is green, and the test additions are unusually good (marker-drift assertion, non-bot exclusion, tail-truncation, forged-delimiter). I'm not blocking, but this changes the prompt of the agent that gates auto-merge, and there are three things I can't verify from the diff — the human should weigh them.Worth a human decision (the PR itself asks for one)
[delimiter removed]replacement so the strip can't be re-joined into a valid delimiter, and "You are NOT bound by them"). But fencing is mitigation, not proof — this is a persuasion surface that didn't exist before and it's on the last gate before merge.Reviewer system prompt was not updated (
lib/review.sh:286+)## Findings the author already dismissedblock arrives unannounced. It's self-describing inline, so it will probably work — but the system prompt is now factually stale about what the reviewer receives, and it gives no rubric for how a dismissal interacts with the "fail closed / when in doubt don't APPROVE" rule. Suggest adding the section to the input list and one line of policy (e.g. "a dismissal never converts a REQUEST_CHANGES into an APPROVE on its own").Three things I can't confirm from the diff alone
bin/tick.sh:3034passes"$BOT_USER". Iftick.shruns underset -uandBOT_USERis not guaranteed set on this path, that's an unbound-variable abort in the review tick. The unit suite callsreview_build_promptdirectly and never exercisestick.sh, so CI green does not cover this. Please confirmBOT_USERis unconditionally set beforedo_review_tick.logand stdout (lib/review.sh:230,234,238,244). Every failure path logs inside a function whose stdout becomes the prompt section (dismissals=$(review_dismissals_section ...)). If the productionlogwrites to stdout rather than stderr/journal, a fetch failure or contract change would injectwarning: review: could not fetch comments ...into the reviewer prompt as the section body — and[ -n "$dismissals" ]would happily splice it in. The test stubslogto a variable assignment, so the suite cannot catch this. Worth a one-line check.forgejo_pr_comments. The tail-truncation logic assumes the returned array is oldest→newest, and the whole feature assumes the newest dismissal is present. If the underlying call returns only the first page, on a long-running PR the dismissals you most want are the ones missing. The test asserts the function exists and takesrepo/number, but not that it paginates.Nits
review_dismissals_sectionstrips the fence delimiters before truncation; that ordering is correct (truncation can only remove text), but a comment saying so would keep a future refactor from swapping them.${text: -$REVIEW_DISMISSALS_MAX}is correct bash (space present), just fragile to a stray edit — fine as-is.No fabricated checklist items (there is no checklist), no scope creep, no test coverage removed, and the
.forgejo/workflows/tree is untouched.Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.🤖 Review —
COMMENT(automated)CI for
4c128987: successFeeds the rework agent's prior dismissal comments back into the reviewer's prompt (new
review_dismissals_sectioninlib/review.sh, marker written bylib/adjudication.sh, 9th arg threaded throughreview_build_prompt). Scope is tight, CI is green, and the tests are unusually good for this repo. My reservations are things I can't confirm from the diff alone, so I'm handing it to a human rather than approving.What I checked and liked
:=fallback + drift assertion, "not bound by" language, 9th arg) has a corresponding hunk. No fabricated checklist items.lib/review.sh:275-281— stripping the fence delimiters before truncating is the right order, and the comment says why. Truncation keeps the tail, which is the correct end to keep, and the test atbin/test-review.shassertsOLDEST-ROUNDis dropped rather than just assertingTRUNCATEDappears.ADJ_LITvsREV_LIT) is the right guard for a cross-file string constant with no sourcing between the two files..forgejo/workflows/untouched; no secrets, no new auth surface; the new input is bot-authored, marker-scoped, fenced, and length-capped.Reservations for the human
logwriting to stdout.review_dismissals_sectionis called inside$(...)inreview_build_prompt(lib/review.sh:296), and it callslogon four failure paths. Every test that exercises those paths either stubsloginto a variable or discards stdout (2>/dev/null), so if the reallogemits to stdout rather than stderr/journal, the empty-BOT_USERpath would splicewarning: review: no bot user -- skipping the dismissals sectioninto the prompt as the dismissals section and the suite would stay green. Please confirmlogis stderr/file-only — or add one assertion that runs the reallogand checks stdout is empty.BOT_USERavailability atbin/tick.sh:3034. The new arg is"$BOT_USER"unquoted-of-default; iftick.shruns underset -uandBOT_USERis only set on some config paths, this is an unbound-variable abort in the review tick rather than a degraded prompt. The function itself handles empty gracefully, so"${BOT_USER:-}"would make the failure mode match the intent.Minor / non-blocking
bin/test-review.sh:unset -f forgejo_pr_commentsappears twice in a row (once bare, once with2>/dev/null || true); harmless duplication.bin/test-review.sh:unset -f lognear the end removes the reallogfor the remainder of the suite. The later truncation/forged-delimiter cases don't hit a log path today, so it's latent, but restoring rather than unsetting would be safer.bin/test-review-directive.sh: theeq "directive and review.sh agree on the section heading"check passes vacuously if the heading vanished from both files. The precedinggrep -qon the directive covers one side; nothing pins the review.sh side non-empty.Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.🤖 Review —
COMMENT(automated)CI for
608b0d9d: successAdds a
review_dismissals_sectionthat feeds the rework agent's prior dismissal comments back into the reviewer's prompt, scoped to bot-authored comments carrying a newADJUDICATION_MARKER. Scope is tight (~200 lines, one issue), CI is green, and the description matches the diff — no fabricated claims. I'm not blocking, but there are two things I can't verify from the diff and one policy call the author explicitly punts to you.Can't verify from the diff
lib/review.sh:246— the one silent failure path is also the most likely one. Every other bailout logs, but if the jq filter matches nothing,printf '%s' "$text" | grep -q '[^[:space:]]' || return 0returns quietly. "There were no dismissals," "the bot login didn't match," and "the marker isn't in the comment body" are indistinguishable. That is precisely the no-op-in-production mode the function's own comments say the logging exists to prevent.bin/test-review.sh, theREAL_SRCblock) doesn't test what it claims. It greps thatforgejo_pr_commentsstill exists and still takesrepo/number. It does not establish that the endpoint that function reads is the same oneadjudication_comment's output is posted to. If dismissals go out as a PR review body rather than an issue comment, this feature is a permanent no-op and — per the point above — logs nothing. On Forgejo/Gitea PR comments are issue comments, so this is probably fine, but the posting call site isn't in the diff and every other assertion stubsforgejo_pr_comments. Please confirm (or add one assertion tying the writer's post target to the reader's fetch target).bin/tick.sh:3034—"${BOT_USER:-}". The variable isn't set anywhere in this diff. If it's unset or differently named at that point indo_review_tick, every review logsno bot userand the feature is off. Cheap to confirm; worth confirming.Policy call for you, not a defect
This deliberately opens a persuasion channel into the agent that gates auto-merge: model-generated prose whose stated purpose is to argue a finding away, injected into the reviewer's prompt. The mitigations are all prompt-level (
review-directive.md:29-36: "never, on its own, turns what would have been a REQUEST_CHANGES into an APPROVE") plus the untrusted fence. The author says outright "your call on the second question" — so this shouldn't merge on my read alone.Things done well (worth not re-litigating)
:=fallback in the reader plus a test that asserts the two literals inadjudication.shandreview.shstill agree. Drift would otherwise be invisible.--- END UNTRUSTED AGENT TEXT ---test asserts exactly one surviving delimiter.log()-writes-nothing-to-stdout assertion is the right paranoia — a warning splicing itself into the prompt as the dismissals section would have passed every other test in the file.Minor
text="${text: -$REVIEW_DISMISSALS_MAX}") can slice mid-comment, so the newest round may arrive with a headless fragment of the previous one glued in front of it. The(TRUNCATED ...)note covers the reader, but a boundary-aware slice (drop whole comments) would read better.REVIEW_DISMISSALS_MAX=3000is unexplained relative to the diff budget in the same prompt; a one-line rationale would help the next person tuning it.Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.Review round 4. Three findings taken, one declined with measurements. DECLINED -- logging the "no dismissals matched" path. The review calls this the most likely silent failure, but it is also the NORMAL case: every PR without a dismissal reaches it. Measured on the four open igor PRs, comments 7/3/1/4 and dismissals 0/0/0/0. A log there fires on essentially every review and trains the reader to ignore the line, which is the same argument that kept the joshing.you exit threshold off total errors. The distinguishing signal the review wants -- misconfigured bot login, wrong marker -- has no shape that separates it from "nothing was dismissed yet". TRUNCATION now drops WHOLE comments, newest-kept-first, instead of byte-slicing the joined text. A slice left the oldest kept comment headless: a fragment with no attribution glued in front of the round the reviewer actually needs. It also means truncation can no longer cut a fence delimiter in half. A single comment larger than the whole budget still yields a section (keep it, hard-slice the tail, say so) -- selecting whole comments would otherwise pick none and drop the argument silently, the one outcome this function must never produce. WRITER/READER ENDPOINT TIE is now asserted. forgejo_comment POSTs and forgejo_pr_comments GETs the same /repos/{repo}/issues/{number}/comments, and the harness posts dismissals through forgejo_comment at two sites. If those ever diverge the feature is a permanent no-op that logs nothing -- exactly because "no dismissals" is the normal case above. BOT_USER is now pinned by a test rather than re-answered. It has been asked in three consecutive rounds and the answer lives in commit messages the reviewer cannot read; the assertion checks the unconditional assignment AND the `|| BOT_USER=""` fallback. REVIEW_DISMISSALS_MAX gained the requested rationale: sized under the linked-issue budget (4000) because dismissals are argument, not evidence -- the reviewer still has the diff to check them against. Mutation-checked. M11 (reader repointed at a different endpoint) and M12 (BOT_USER fallback removed) caught. M13 (kept comments returned reversed) ESCAPED: the truncation fixture keeps only one comment, so order was unobservable. Added a three-round fixture that asserts chronological order, which does catch it -- a reversed list would have the reviewer reading a superseding argument as though it came first. Also fixed a vacuous assertion of my own: `has ... ""` matches anything and always passed. Fourth of its kind in this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QfDyMfn2Go73gw21a92wNB🤖 Review —
COMMENT(automated)CI for
393ac9cd: successFeeds prior bot-authored dismissal comments (marker-scoped, untrusted-fenced) into the reviewer's prompt so it can engage with arguments instead of re-raising the same finding each round. Well-tested and carefully reasoned; nothing I'd block on, but it opens a new input channel into the thing that gates auto-merge and the author explicitly defers a design question to you — so this wants a human eye rather than a rubber stamp.
Scope / contract
success. No checkbox list to falsify; the prose claims (four mutants: marker drift, bot filter, marker filter, "may disagree" language) each map to a real assertion inbin/test-review.sh/bin/test-review-directive.sh.Findings
lib/review.sh— the jqreduceskips an oversized comment and keeps iterating older ones (else . end), so the kept set can be non-contiguous. If the newest round is oversized but an older one fits, the newest argument — the one the reviewer most needs — is silently dropped while the note still readsTRUNCATED -- N older round(s) dropped. The oversized-single-comment fallback only fires whentextis empty, so it does not cover this case. Either break out of the reduce on first non-fit, or word the note honestly (N round(s) dropped). Minor, but the label is currently wrong in that path.review_dismissals_sectiondepends onforgejo_pr_commentsreturning the whole comment thread. If that helper is a single unpaginated GET (Forgejo defaults to a page limit), a long PR thread would return only the first page and the newest dismissal — the one that matters — would never reach the prompt, silently and with no log line. I can't seelib/forgejo.shin this diff; worth confirming.review_build_promptgrew a 9th parameter. Only thebin/tick.shcall site is updated in this diff. If any other production caller exists (e.g. a standalone review helper), it silently getsbot=""and the feature is a permanent no-op there — with only a stderr warning. Please confirmtick.shis the sole caller.Security posture (not blocking, but the reason a human should sign off): this is model-generated prose derived from a potentially adversarial diff, whose stated purpose is to argue a finding away, injected into the prompt of the auto-merge gate. The mitigations are real and correctly ordered — bot-login filter AND marker filter, strip-then-slice so a hard slice can't bisect a delimiter, both fence delimiters neutralised, and the directive's "a dismissal never, on its own, turns a REQUEST_CHANGES into an APPROVE." The delimiter strip is exact-literal though, so a near-miss variant (
--- END UNTRUSTED AGENT TEXT ---, different dash count) passes through unmodified; it won't match the real fence either, so the blast radius is confusion rather than escape. Acceptable as designed — but it is a policy change to the gate, and the PR body itself says "your call on the second question."Things I checked and found sound
:=fallback inreview.shplus unconditional assignment inadjudication.shis order-independent either way, andtest-review.shpins the two literals against each other.log()writes to stderr, so a fetch failure inside$(...)can't splice a warning string into the prompt as the dismissals section. The test proves this directly rather than assuming it.bot=""→ early return).Follow-up nit
has "the real one takes repo + number" "$REAL_SRC" 'local repo="$1" number="$2"'asserts on another file's exact source formatting; a harmless reorder inlib/forgejo.shbreaks this test for no real reason.Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.🤖 Review —
COMMENT(automated)CI for
aa400602: successFeeds prior rework-agent dismissals (bot-authored comments carrying a new
ADJUDICATION_MARKER) back into the reviewer's user turn, fenced as untrusted. Well-scoped, well-tested, CI green — I found no blocking defect, but there are a few things I can't verify from the diff and one policy decision I'd want you to sign off on rather than have me rubber-stamp.Scope / honesty
review_dismissals_sectionexists,review_build_prompttakes the bot user as a 9th arg (lib/review.sh:346), the marker is an HTML comment written bylib/adjudication.sh:44and read via a:=fallback inlib/review.sh:216, the drift test exists (bin/test-review.shmarker-agreement block), and the "NOT bound by them" language is really in the emitted section. No unchecked-work claims found.Things worth a second look
Policy, not code: this widens what reaches the gate on auto-merge. Agent-generated prose whose explicit purpose is to argue a finding away now lands in the reviewer's prompt. The mitigations are genuinely good (bot-author + marker scoping, delimiter stripping before the hard slice,
lib/review-directive.md's "never, on its own, turns … into an APPROVE"), but the strip only neutralises the two fence delimiters — it does nothing about text that mimics other prompt structure (PR under review:,## Findings…, aVERDICT:line). Low risk given the reviewer already reads the same diff, but it's your gate, so it should be your call.Pagination claim is unverifiable from here (
lib/review.sh, comment above the jq block): "forgejo_pr_comments returns the WHOLE thread, verified 2026-07-30 … no page or limit". If that ever stops holding, comments arrive oldest-first, so a truncated first page silently drops the newest dismissal — exactly the one the reviewer needs — and the code logs nothing, because "no dismissals" is the normal case. Consider a cheap defence (e.g. assert the fetched count or pass an explicitlimit) rather than a dated comment.Only one
review_build_promptcall site is updated (bin/tick.sh:3034).bot="${9:-}"means any other caller silently degrades to "no bot user" — a warning on stderr and no section, indistinguishable from "no dismissals". The suite pinsadjudication_commentcall-site count (POSTS >= 2) but nothing pins that everyreview_build_promptcall site passes a bot user. If there's a second review path, this feature is half-dead. Worth an analogous grep-count assertion.Note is wrong-ish in one corner (
lib/review.sh, oversized-single-comment fallback): when the newest comment blows the budget and older rounds exist,droppedis computed but the note only says(TRUNCATED -- one oversized comment, opening kept). The reviewer isn't told other rounds were dropped too. Cosmetic, but the surrounding comments make a point of the note being accurate about which round was lost.Directive structure (
bin/lib/review-directive.md:23-45): the new### How a dismissal affects your verdictsubsection is inserted between the "What you receive" bullets and the "You do NOT have the working tree … an unreviewable PR is not an approvable one" paragraph. That paragraph now reads as though it belongs to the dismissal subsection rather than to the input list. Moving the###block below it would keep both readable.Test coverage — unusually strong for this repo: marker drift, bot-author filter (with the marker on the human comment, which is the right threat model), unmarked-bot-comment rejection, non-array payload, failed fetch,
log()not polluting stdout inside$(...), chronological ordering, whole-comment truncation with no headless fragment, and the oversized-newest inversion. The stdout/stderr check onlog()is the one that would actually have caught a warning being spliced into the prompt as the dismissals section — good catch to write it.Nothing here blocks on its own; items 2 and 3 are the ones I'd want resolved or consciously accepted before merge.
Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.🤖 Review —
COMMENT(automated)CI for
05e32d70: successFeeds the rework agent's prior dismissal comments back into the reviewer's prompt (
review_dismissals_section), fenced as untrusted and scoped by bot login +ADJUDICATION_MARKER. CI is green and the implementation is careful — but this changes the trust boundary of the thing that gates auto-merge, so I'm handing it to you rather than approving.The thing to actually decide
bin/lib/review-directive.md:34-50saying a dismissal "never, on its own, turns what would have been a REQUEST_CHANGES into an APPROVE"). But it's still an LLM being fed persuasive prose whose stated purpose is to argue a finding away, in an unattended loop.Neither is a defect in the diff; both are the kind of judgement call the rubric says not to rubber-stamp.
Findings
lib/review.sh— sentinel scrubbing is incomplete. You neutralise===BODY===,## Unified diff,PR under review:, and the fence delimiters, but notVERDICT:— the one string the harness actually parses out of the reviewer's output. It's confusion-only rather than an exploit (the scrubbed text lives in the prompt, not the response), but if===BODY===is worth stripping under "defence in depth",VERDICT:is the more obvious member of that set. Suggest adding it, or dropping the whole defence-in-depth block to just the fence.lib/review.sh— page-boundary heuristic will cry wolf. The check fires onn_comments∈ {10,15,20,25,30,50,100} counted over all issue comments, not just bot dismissals. On this repo's own PRs (review verdict + rework comment per round) hitting exactly 10 or 15 comments is routine, and each one emits awarning: ... suspiciously like a page boundaryinto the journal. A warning that fires on ordinary traffic trains you to ignore it. Consider gating it on the filtered dismissal count, or dropping it — the comment above it already documents the verified no-pagination fact.lib/review.sh+172,bin/test-review.sh+256). Nothing was cut to fit, which is the right call, but this is over and could plausibly have been two stacked PRs (marker + section; directive + wiring).bin/test-review.sh— scope creep in the pins. TheBOT_USER=assignment check and theadjudication_comment "$PR_DISMISSED"call-site count assert invariants inbin/tick.shthat this PR doesn't touch. Defensible as guards for the new dependency, but they'll now fail on unrelatedtick.shrefactors.bin/test-review.sh—grep -c ... || echo 0is broken on the zero path.grep -cprints0and exits 1, so the|| echo 0appends a second line:POSTSbecomes"0\n0"and[ "$POSTS" -ge 2 ]errors with "integer expression expected" instead of reporting the intended failure. Same forCALLS/WITH_BOT, where two"0\n0"values would additionally makeeqpass vacuously. Only reachable when the pin is already violated, but that's exactly when you want a clean message.|| trueon the assignment, orgrep -c ... || :, is the fix.What I checked and found clean
reverse | reducewith thedoneflag keeps a contiguous newest-first window,.keep | reverserestores chronological order, and the byte accounting (len + length + 2) matches thejoin("\n\n")— so the normal path can't exceedREVIEW_DISMISSALS_MAXand only the oversized-single-comment fallback reaches the hard slice. The "oversized newest is kept, not skipped" invariant and the two-sided note wording are both correct and tested.review_build_promptcallers degrade tobot=""→ early return → byte-identical prompt, which the pre-438 shape test still covers.:=inlib/review.shwithlib/adjudication.sh's unconditional assignment winning at runtime is correct given the flat sourcing order, and the drift test pins both literals non-empty before comparing (the vacuous-pass trap is handled).logdoesn't write to stdout — which matters because the function runs inside$(...).Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.🤖 Review —
COMMENT(automated)CI for
7a9790fe: successFeeds prior rework-agent dismissal comments (bot-authored, marker-scoped) back into the reviewer prompt, plus directive rules for how to weigh them. The implementation is careful and the tests are unusually thorough — I found no clear blocking defect, but this changes the trust boundary of the thing that gates auto-merge, so I'm leaving it for a human rather than approving.
What I checked and believe is correct
lib/review.shselection jq: newest-first accumulation with an explicitdoneflag (stops at first non-fit rather than skipping it), thenreverseback to chronological.dropped/totalaccounting is right in all four cases I traced (newest oversized, older oversized, single oversized, all-fit).[-1](newest) and keeps the head — matches the stated rationale and the note text.no bot user, fetch failure, non-array payload, jq failure) allreturn 0with a section-less prompt and log to stderr, so nothing splices a warning into the prompt. The test that runs the reallog()and asserts empty stdout is the right test to have written.review_build_prompt9th arg is${9:-}, so the 8-arg call sites in existing tests still work.Things I'd want the human to weigh
lib/review.sh, the${text//...}block).--- END UNTRUSTED AGENT TEXT ----or a variant with different spacing survives, and an LLM will read a near-miss as a closing fence. The comments correctly frame this as defence-in-depth rather than a claimed exploit, so I'm not blocking — but the stripping should not be relied on as the control if the scoping ever loosens.${#text} > MAXbranch is "only reachable via the oversized-single-comment fallback", but the sentinel replacements grow text (VERDICT:8→18,===BODY===10→18). A kept multi-comment set at ~2900 chars containing several such strings can cross 3000 after substitution, and would then be labelled(TRUNCATED -- one oversized comment, opening kept)— a false statement about which rounds survived, in a note the surrounding code makes a point of keeping honest. Cheap fix: compute the note from the selection result (dropped/ oversized-fallback flag) rather than from post-substitution length, or do the length check before substitution.bin/test-review.sh(+245) and nothing was cut to fit, so I don't treat this as a violation — noting it because the budget was exceeded, not evaded.Smaller notes
forgejo_pr_commentsAPI call per review tick, on every PR, including the common "no dismissals" case. Fine now; worth remembering if rate limits ever bite.bin/test-review.shasserts a lot by grepping the source ofbin/tick.shandlib/forgejo.sh(repo="$1",/repos/...comments,BOT_USER=, call-site counts). Those catch real drift, but they'll also fire on harmless reformatting. Acceptable given there's no integration harness here; just expect maintenance.[ "${dropped:-0}" -gt 0 ] && note=...as the last command of athenbranch is the same pattern already used inreview_build_prompt, so it's consistent with the file — flagging only because it'sset -e-fragile if this lib is ever sourced underset -e.CI is green and the description's claims all correspond to real changes in the diff — no fabricated checklist items found.
Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.Review round 8. One finding taken -- a real defect, and one I introduced in round 7's own fix. The rest are dismissed as previously reasoned. THE DEFECT. The sentinel scrubbing added last round GROWS text: VERDICT: is 8 chars and becomes 18, ===BODY=== is 10 and becomes 18. The note was keyed on ${#text} AFTER substitution, so a kept set that fitted the budget could cross it purely from escaping and then be labelled "one oversized comment, opening kept" -- false about both halves. Nothing was oversized and nothing was dropped. Reproduced before fixing: two rounds joining to 2004 chars, both kept, both present in the output, note claiming an oversized comment. Worse than a cosmetic label: the surrounding code makes a point of the note being honest about which rounds survived, and this made it lie in a case reachable by ordinary content -- a dismissal that quotes the reviewer's own output format is not exotic. The note now derives from the SELECTION. Three distinct outcomes, each stated plainly: an oversized single comment (opening kept, plus any older rounds displaced), escaping-induced overflow (named as escaping, not blamed on a round), and ordinary older-round drops. Verified across all four shapes; mutation-checked by reverting the branch condition to ${#text}, which fails the two new assertions. DISMISSED, as in round 7 and for the same reasons: the trust-boundary question (the operator's standing call on #456, correctly raised and correctly not mine to clear), exact-string delimiter matching (already framed in-code as defence-in-depth, not the control), diff size (test code, nothing cut to fit), and the source-grep assertions (the price of pinning a cross-file dependency, cheaper than a silently half-wired feature). Also noted and accepted: one extra forgejo_pr_comments call per review tick. It is one GET on a call that already does several, and the alternative is not fetching the thing the feature exists to read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QfDyMfn2Go73gw21a92wNB🤖 Review —
COMMENT(automated)CI for
59f82cf0: successFeeds the rework agent's prior dismissal comments back into the reviewer's prompt (fenced as untrusted, scoped to bot-authored comments carrying a new
ADJUDICATION_MARKER). Code is careful and the description matches the diff — but this change alters the trust boundary of the thing that gates auto-merge, and the author explicitly defers a design question to you, so it should land on a human's desk rather than be rubber-stamped.Verification of the claims
Every claim in the description is backed by the diff — no fabrication found:
review_dismissals_section(lib/review.sh:216+) filters on.user.login == $bandcontains($m). ✓review_build_prompttakesbotas a 9th arg with${9:-}; the single call site inbin/tick.sh:3034passes${BOT_USER:-}. ✓: "${ADJUDICATION_MARKER:=...}"in review.sh and pinned by a drift test (bin/test-review.sh, ADJ_LIT/REV_LIT compare). The grep patterns are single-quoted, so no accidental shell expansion. ✓bin/lib/review-directive.md. ✓done-flag then re-reversed, so kept comments are contiguous and chronological; the oversized-single-comment fallback takes[-1](newest). Logic reads correct to me.What I'd want you to weigh (not defects)
VERDICT:,===BODY===, the fence delimiters) prevents structural forgery but not a well-argued-but-wrong dismissal. That's the design tradeoff #456 is asking about, and the PR body says "your call on the second question" — so it's yours, not mine.Minor findings
lib/review.shescaping-overflow branch:text="${text:0:$REVIEW_DISMISSALS_MAX}"keeps the head, so the tail of the newest round is what gets cut. Elsewhere the code goes out of its way to argue the newest round is the one the reviewer needs (thedone-flag comment, the oversized fallback). Low impact — only fires when scrubbing inflates the text past budget — but the two branches encode opposite priorities.text=${text//VERDICT:/[sentinel removed]}(and thePR under review:/## Unified diffsubstitutions) will mangle a dismissal that legitimately quotes the reviewer, e.g. "your VERDICT: REQUEST_CHANGES assumed…". Cosmetic, but it degrades exactly the text the feature exists to convey.bin/test-review.shunset -f logthen re-sourceslib/review.shand calls the reallog()twice with a dummy message. Iflog()does anything beyond writing to stderr (appends to a journal/log file), CI runs are now writing "a warning that must not reach the prompt" into it. Worth a glance — I can't seelog()'s definition from this diff.review_build_promptinbin/test-review.shnow pass 8 args →bot=""→ awarning: review: no bot userline on stderr for every prompt-shape assertion. Harmless, just noise.Coverage
Test coverage is unusually thorough for this repo: bot-filter, marker-filter, marker drift, ordering, truncation-keeps-newest, oversized-single-comment, forged fence/sentinel/heading, the
log()-must-not-write-to-stdout invariant, and writer/reader endpoint agreement. The described mutation run (4 mutants, including the fixture fix that made M2 actually fail) is exactly the right instinct and I found no assertion that passes vacuously. CI is green on59f82cf.Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.