feat: detect what is waiting on the operator #449
No reviewers
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
2 participants
Notifications
Total time spent: 55 minutes 18 seconds
Due date
igor
55 minutes 18 seconds
No due date set.
Dependencies
No dependencies set
Reference
joshtronic/igor!449
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/439-needsyou-detection"
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?
What this PR does
Detection half of #439. Delivery (email) is a separate change — the first attempt at this issue tried both at once and blocked at 792 lines against the 400 cap.
lib/needsyou.sh— the waiting-on-you set, its event semantics, and a human-readable descriptionbin/tick.shthat builds the live set and logs what is NEWbin/test-needsyou.sh— 46 checksReview round 2 took the branch from ~380 to 458 net lines, past the 400 cap. The rework path does not run the scope gate, so this is a note rather than a block: the 73 added lines are the scan-blindness fix below, its regression tests, and the three defensiveness changes the review asked for. There is nothing in here I can cut without dropping coverage or the comments that explain the verdict table. Say the word and I will split the
needsyou_pr_numbersfix into its own PR ahead of this one.Event-driven, not periodic
A recurring digest that usually says "nothing needs you" trains the reader to stop opening it, and then it fails exactly when it matters. So only additions are announceable — removals are silent, and an unchanged set says nothing at all.
The dry-run found the scan blind
The first version of this description claimed a dry-run showed
debtmom.com#1reading as needing the human. The reviewer caught that this was run against an earlier! automerge_will_takepredicate and never re-run after the redesign. Re-running it against the shipped code found zero — and not because the fleet was quiet.forgejo_list_open_bot_prsanswers with a JSON array. The scan read that text line by line, soneedsyou_pr_whywas called with[,{and"number": 449,and never once with a PR number. The feature detected nothing at all, on every repo, and the only reason that read as plausible is that "zero things need you" is a legitimate answer.needsyou_pr_numbersnow pulls.numberout the waydo_automerge_tickdoes, and dropsWIP:checkpoint PRs — that is Igor mid-task, not your turn, and such a PR can still carry the verdict it had from before it checkpointed.Honest dry-run against the fixed scan, today: 2 open bot PRs fleet-wide, 0 needing you.
debtmom.com#1igor#449(this PR)REQUEST_CHANGES, round 2 of 3Plus 2 open issues fleet-wide, neither carrying a
Status/*label, so 0 parked on you. Zero items today, and every zero is checkable against reality.The decision worth reviewing
Which PR states are your turn is a hand-enumerated verdict table, not a derived one. The first cut asked
! automerge_will_take, which is much broader than "you are the blocker": it is also true of a PR nobody has reviewed yet (the shadow reviewer's turn) and of one inside the rework loop (Igor's turn, for up to 3 rounds). Both flip in and out of the set on their own, so announcing them is exactly the noise this feature exists to avoid.So
needsyou_pr_whyenumerates instead, mirroringdo_review_tick's own routing — unreviewed goes to the reviewer,APPROVEto nobody unless the repo pins itself to a human,COMMENTto you,REQUEST_CHANGESto Igor for rounds 1–3 and then to you.The tradeoff to scrutinise is drift. That table is a second reading of the merge policy, and if the policy moves the table has to move with it. It lives in one function, is unit-tested state by state, and carries a comment saying why — but nothing enforces the correspondence. If that is the wrong call, deriving it from
automerge_will_takeand then subtracting the reviewer's-turn and rework-loop cases is the alternative.Scanned, not hooked. The cheaper design hooks where the loop already decides —
review_request_human()andagent-block.sh— at zero API cost. I did not, because items must also leave the set when you deal with them. A hook can only ever add; without removal an item stays "already known" forever and a genuine recurrence is never announced again. A scan is idempotent, self-correcting, and picks up anything already queued before this ships.Throttled to every 20th tick. It costs a PR-list and an issue-list call for every repo in the analysis set — 2N, exactly the fleet-sweep cost #441 was about — plus one
agent.jsonfetch per repo that actually has an open bot PR. Gating the issue call the way theagent.jsonfetch is gated would not work: an issue can be parked on you in a repo with no bot PR at all.needsyou_mergepreserves each item'ssinceacross scans. Without that every scan resets the clock and nothing can ever be reported as having waited days, which is most of the value.Test plan
bash bin/test-needsyou.sh— 46 checks: additions vs removals, an unchanged set announcing nothing, first-ever scan,sincepreserved across scans, an item that left and returned resetting its clock, wait-time formatting (m/h/d), which verdicts are whose turn,WIP:drafts dropped, and malformed input degrading to empty rather than crashingmake testandmake lintpass;bash -nclean ontick.shneedsyou_pr_why— the table aboveneedsyou_scan_set+needsyou_passverbatim out oftick.sh, ran them against live fleet data with a throwaway state file. Five passes: live state announces nothing, an unchanged rescan is silent, forcingigor#449toCOMMENTannounces it once, rescanning is silent again, withdrawing the verdict drops it from the set with no output. Manual — not in CI, see below.Not in this PR
Delivery. The next change wires the additions into the existing ship-report mail path so the arrival of the mail is the signal.
Also not covered by CI:
needsyou_scan_setandneedsyou_passthemselves. Every unit test targets the pure helpers inlib/needsyou.sh; the state-file write, themktemp/mv, and the API glue are verified only by the manual run above — and that glue is exactly where this round's bug lived. The delivery PR should move the scan intolib/needsyou.shbehind stubbable seams and cover it, the waytest-automerge.shstubs_fj.Part of #439
🤖 Review —
REQUEST_CHANGES(automated)CI for
afb2bb3c: successDetection-only half of the "what's waiting on the operator" feature: a new pure-JSON lib (
lib/needsyou.sh), a throttled scan inbin/tick.sh, and 21 unit assertions. Scope (~190 lines), CI green, and the checklist is honest — I recounted the assertions inbin/test-needsyou.shand got exactly 21, and every checked item corresponds to real code in the diff. The pure functions are well-factored and thesince-preservation semantics are correct.What blocks it is the classification predicate itself, which the description explicitly asks to be reviewed.
Blocking
1.
bin/tick.sh:~4100— "auto-merge will not take it" is much broader than "the human is the blocker".!automerge_will_takeis true for at least three distinct states, only one of which is the operator:verdict = none(not yet reviewed) → waiting on the reviewer, not the human. Every freshly opened bot PR sits in this state; with a 20-tick scan a good fraction will be sampled before review lands and announced as "needs you", then silently leave.verdict = REQUEST_CHANGES→ waiting on Igor's own rework loop (up to 3 rounds before it escalates). The human is explicitly not the blocker during those rounds, yet each rework round that flips the item out and back in re-announces it, becauseneedsyou_mergedeliberately resetssinceon return.verdict = COMMENT, orautomerge_require_human→ genuinely the human. ✔This contradicts the PR's own stated design goal ("a digest that usually says nothing needs you trains the reader to stop opening it"). The live dry-run cited in the description is a single snapshot of a fleet whose PRs happened to already be reviewed; it doesn't exercise the transient states above. Fix looks like: enumerate the verdicts that actually mean "human", or gate on an escalation/age threshold rather than on the negation of the merge predicate. If the intent really is "anything auto-merge won't take, because you're the fallback", say so in the
whystring and the docs — but then the noise argument needs revisiting.2. No test covers the only new decision logic in this PR.
needsyou_scan_set()andneedsyou_pass()live inline intick.shand are untestable as written; the 21 assertions all exercise the pure set algebra inlib/needsyou.sh, which is the part least likely to be wrong. The two things the description calls "worth reviewing" — the PR predicate and theStatus/Blocked/Status/Need More Infoissue filter — have zero automated coverage. Lifting the per-item classification into a pure function inlib/needsyou.sh(taking repo/verdict/require_human → why-string or empty) would make finding #1 a one-line test.3.
bin/tick.sh—repois not guarded against the literal stringnull.If a line parses as JSON but lacks
.full_name,jq -remitsnull, which passes[ -n ], and the loop then issuesforgejo_list_open_bot_prs nullandGET /repos/null/issues. The PR loop three lines below already checks[ "$pr" = "null" ]— apply the same check here.Non-blocking notes
lib/needsyou.sh:needsyou_merge—local prev="${1:-\{\}}"is dead/wrong: the file's own_needsyou_objcomment explains that\{\}reaches jq verbatim as an invalid document. It's harmless only because_needsyou_objsanitises it afterwards. Drop the default to${1:-}for consistency withneedsyou_added/needsyou_removed.needsyou_scan_setdeclareskeyinlocalbut never assigns it.tick.sh:~4088since it's the justification forNEEDSYOU_SCAN_EVERY=20.needsyou_describewill render a negative age (-1m) ifsince > now(clock skew / hand-edited state). Cheap to clamp at 0.discretionary-state.jsonis ever unparseable, thejq ... > "$tmp"write is skipped silently andprevreads as{}forever, so every scan re-announces the whole set. Consider logging the write failure.CASCADE_TICK/set -unote is accurate; the new block sits between two existing comment sections without orphaning anything.Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.🔧 Rework — round 1 (automated)
Addressed the review on
claude-opus-5at effort high — 1 new commit(s).The scan classified a PR as waiting on the operator whenever auto-merge would not take it. That is three states, and only one of them is his: an unreviewed PR is the shadow reviewer's turn, and a REQUEST_CHANGES inside the rework loop is Igor's for up to three rounds. Both flip in and out of the set on their own, so both would be announced and then silently leave -- the every-scan noise this feature exists to avoid. The verdicts that mean HUMAN are now enumerated in needsyou_pr_why, mirroring do_review_tick's own routing (COMMENT, an APPROVE on a human-pinned repo, an escalated or CI-unverifiable REQUEST_CHANGES). The issue filter moves to needsyou_issue_lines. Both are pure and unit-tested, so the only decision logic in the change is now covered: 17 new assertions, 38 total. Also from review: guard the repo name against a literal "null" the way the PR loop already does, clamp a negative age at 0, drop the dead `\{\}` default and the unassigned local, log a state-write failure rather than silently re-announcing the whole set forever, and correct the throttle rationale -- the sweep costs 2N calls, not N. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>🤖 Review —
REQUEST_CHANGES(automated)CI for
3a6091c2: successDetection-only half of igor#439: a new
lib/needsyou.sh(pure JSON helpers), a throttled scan/announce pass inbin/tick.sh, and a self-contained unit test. The library code itself is careful and well tested; what blocks is that the PR description makes verification claims the shipped code contradicts, plus one ordering risk intick.shI can't verify from the diff.Blocking
1. The checked "Predicate dry-run against live fleet data" claim contradicts the shipped predicate.
The description states: "across every open PR in the fleet, only
debtmom.com#1reads as needing the human (verdictnone, so auto-merge will not take it)". Butlib/needsyou.sh:needsyou_pr_whyhas nononebranch — an unrecognised/noneverdict falls through thecaseand returns empty — andbin/test-needsyou.shasserts exactly that:So the one item the dry-run says would be announced would not be announced by this code. The dry-run was evidently run against the earlier
! automerge_will_takepredicate and never re-run after the redesign. That matters beyond bookkeeping: the honest reading is that the shipped predicate currently matches nothing in the fleet, and the only evidence offered that it does anything is stale. Please re-run the dry-run againstneedsyou_pr_whyas written and report the real result (even if it's "zero items today"), or drop the checkbox.2. "Two decisions worth reviewing" describes a design that isn't in the diff.
The description says the predicate is "expressed as the negation of
automerge_will_takerather than a second copy of the rule… That predicate already encodes…". The code says the opposite, at length (lib/needsyou.sh, comment aboveneedsyou_pr_why): "The first cut of this asked! automerge_will_take, which is much broader… So the verdicts that mean HUMAN are enumerated instead." The human reads the description to decide what to scrutinise; here it points at the wrong design and the wrong risk (a hand-enumerated verdict table that can drift from the merge policy — which is the tradeoff actually worth reviewing). Update the description to match what shipped.3. Unverifiable: is
ANALYSIS_REPOS_JSONassigned at the call site?needsyou_scan_setends withdone <<<"$ANALYSIS_REPOS_JSON"(bin/tick.sh, new block), andneedsyou_passis invoked immediately afterCASCADE_TICK=…, before the cascade stages run. Underset -u— which the PR itself notes is in force, and which already produced one of the two self-caught bugs — an unassignedANALYSIS_REPOS_JSONat that point aborts the entire tick, and because the pass only fires on every 20th tick that failure would be rare and confusing. I can't see where that variable is set from the diff. Either confirm it's assigned above line ~4239, or make it defensive (<<<"${ANALYSIS_REPOS_JSON:-}") so a missing/late assignment degrades to an empty scan rather than killing the tick.Non-blocking
_fj GET /repos/…/issuesper repo, the issue call being paid even for repos with no bot PRs. The throttle rationale (igor#441) is argued from the smaller number. Consider gating the issue list the same wayautomerge_require_human/maintenance_repo_validatedare gated, or at least fix the number.eq/haschecks inbin/test-needsyou.sh. Undercount, harmless, but the checklist should be accurate.needsyou_scan_set/needsyou_passhave no test coverage. All 38 assertions target the pure helpers; the parts that touch the state file,mktemp/mv, and the API glue — i.e. where the two self-caught bugs lived — are exercised only in production. Not a blocker for a detection-only PR, but worth a note in the follow-up.needsyou_removedis defined and tested but never called. Documented as "available for a caller"; fine, just flagging the dead path.needsyou_itemfailure mode: a non-numericsincemakesjqfail and the function prints{}, which the scan then folds into the set under a valid key —needsyou_describewould rendernull#null. Only reachable via a caller bug (the scan always passes0), so low priority.What I did check and found clean
_fjoutput is guarded by_needsyou_arr, state reads by_needsyou_obj.needsyou_mergesince-preservation semantics are correct ($p[.key].since // $now, new keys take$now, returning items reset — all directly asserted).automerge_require_human … && require_human=trueis safe underset -e(failing left side of an AND-list doesn't trigger exit).Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.🔧 Rework — round 2 (automated)
Addressed the review on
claude-opus-5at effort xhigh — 1 new commit(s).The dry-run the PR description offered as evidence was run against the earlier `! automerge_will_take` predicate, not the one that shipped. Re-run against the shipped code it found nothing -- and not because the fleet was quiet. `forgejo_list_open_bot_prs` answers with a JSON ARRAY, and the scan read that text line by line, so `needsyou_pr_why` was called with "[", " {" and '"number": 449,' and never once with a PR number. `needsyou_pr_numbers` pulls `.number` out the way do_automerge_tick does, and drops `WIP:` checkpoint PRs for the same reason the rework loop is dropped: that is Igor mid-task, not the operator's turn, and such a PR can still carry the verdict it had before it checkpointed. Fixing it also makes the emptiness test below it mean something. "[]" is a non-empty string, so gating on the raw payload paid for the agent.json fetch on every repo in the fleet rather than only where a PR exists. Also from review: - `<<<"${ANALYSIS_REPOS_JSON:-}"`. The validation sweep assigns it well above the call site, but this is the one fleet loop that runs from the cascade prelude, so it is the likeliest to end up above that assignment later. Under `set -u` that aborts the tick, and only every 20th tick -- a rare, confusing failure. - A non-numeric `since` no longer voids `needsyou_item` into a bare {}, which the scan would fold in under a valid key and describe as "null#null". - The cost comment now says what it actually costs: 2N, plus one agent.json fetch per repo that has an open bot PR. Dry-run re-run against the fixed scan: 2 open bot PRs fleet-wide, 0 needing the human. debtmom.com#1 has no shadow verdict (the reviewer's turn) and igor#449 is REQUEST_CHANGES at round 2 (Igor's turn). Both correctly silent, and both checkable against reality.🤖 Review —
REQUEST_CHANGES(automated)CI for
864e6f54: successDetection half of igor#439: a new
lib/needsyou.shwith pure set/verdict helpers, a throttled fleet scan wired intobin/tick.sh's cascade prelude, and 46 unit checks. The pure helpers are genuinely good — theneedsyou_pr_whyenumeration is well-reasoned and the description is unusually honest (I recounted the test file: 46 checks, matches the claim; the scope overrun is self-declared rather than hidden). What blocks me is that the layer where last round's bug lived is still the layer with zero coverage, and it still contains the same unverified assumption.Blocking
1.
needsyou_scan_setiteratesANALYSIS_REPOS_JSONline-by-line — the exact pattern that made the last version blind (bin/tick.sh,done <<<"${ANALYSIS_REPOS_JSON:-}"withjq -r '.full_name // empty' <<<"$repo_line"). This only works if that variable is newline-delimited compact JSON objects. If it is a JSON array — pretty-printed or not — everyrepo_lineis[,{,"full_name": ...and the outer loop yields zero repos, i.e. the feature is silently dead fleet-wide again, and "0 items need you" is once more a plausible-looking answer. I cannot verify the format from this diff (the assignment isn't in it). The manual end-to-end run does not cover this: the description says the functions were lifted verbatim out oftick.shand run with a hand-built environment, so the harness supplied whatever format the author assumed rather than what tick.sh actually assigns. Fixed looks like: point to the assignment site and confirm the shape, or normalise defensively (jq -c '.[]?' <<<"$ANALYSIS_REPOS_JSON"style, matching whateverneedsyou_pr_numbersnow does for pulls).2. The glue is uncovered, and the author says so. Every one of the 46 checks targets pure helpers.
needsyou_scan_setandneedsyou_pass— the state-file write, themktemp/mv, the API fan-out, theANALYSIS_REPOS_JSONloop above — have no test at all. Deferring that to the delivery PR is not acceptable in a round that exists because an untested glue bug shipped and read as a legitimate zero. The author already names the pattern (test-automerge.shstubs_fj); at minimum stub_fj/forgejo_list_open_bot_prs/automerge_require_human/review_rework_roundsand assert thatneedsyou_scan_setproduces the right keys for a two-repo fixture. That single test would have caught both this round's bug and finding #1.3. Scope: 458 net lines against the ~400 cap. Declared honestly, but declared is not waived. The author's own offer is the right fix: land
needsyou_pr_numbers+ its regression tests as a small stacked PR first, then the rest under budget.Non-blocking, but I'd want these addressed
forgejo_list_open_bot_prs ... 2>/dev/null || trueand_fj GET ... || trueboth degrade to empty, andneedsyou_passthen persists amergedset that has dropped those items — losing theirsince. One transient API blip silently removes an item the operator hasn't touched, and the next successful scan re-announces it as new. That is precisely the "notification the reader learns to ignore" failure the design section argues against. Consider skipping the state write entirely when the scan produced no repos / no successful list calls, rather than persisting an empty set.set -e.prev=$(jq -c '.needsyou // {}' "$sf" 2>/dev/null || echo '{}')is guarded;verdict=$(jq -r ... "$sf" 2>/dev/null)andout=$(jq -c ... <<<"$out")inneedsyou_scan_setare not. Iftick.shruns underset -e, a malformeddiscretionary-state.jsonaborts the whole tick from inside the scan. Add the same|| echo ...fallbacks.(m/h/d); the tests assertwaiting 1d,waiting 30m,waiting 0m— there is no hours case, and the 60/1440 boundaries are untested. Small, but it's a checklist item that doesn't fully match the diff.needsyou_describe'sif $i == null then emptyis dead code —($s[$k] // empty) as $ialready emits nothing for a missing key. Harmless, but the comment/branch implies a guard that isn't doing anything._needsyou_objaccepts any valid JSON, not just objects:_needsyou_obj '[]'returns[], andneedsyou_addedthen reports array indices as keys. Tightening it tojq -ce 'type == "object"'(mirroring_needsyou_arr) closes that.What's good
needsyou_pr_whyas a hand-enumerated table rather than! automerge_will_takeis the right call and is argued for honestly, including the drift risk. The state-by-state tests make the table's intent legible.WIP:checkpoints, and the reasoning that such a PR can carry a stale verdict, is a sharp catch.sinceacross scans via merge rather than replace, with a test that an item which left and returned resets its clock.jqwith--arg/--argjson, never eval'd). CI is green.Independent review by the harness on
claude-opus-5(effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.🔧 Rework — round 3 (automated)
Addressed the review on
claude-opus-5at effort max — 2 new commit(s).🤖 Review —
COMMENT(automated)CI for
365bb32c: successDetection-only half of #439: a new
lib/needsyou.sh(pure predicates + a fleet scan), a 271-line unit suite, and a throttledneedsyou_passcall in the cascade prelude. CI is green and I found no defect I can prove from the diff — but I can't approve it: the diff is ~625 added lines against the 400 cap, the description is materially stale relative to what shipped, and every integration seam is stubbed by the same author who wrote the code, in exactly the layer that shipped blind last round.What I can't verify from the diff (why this isn't an APPROVE)
needsyou_scan_setreaches five things defined outside this diff, and the new suite stubs all five. A signature or shape mismatch reproduces last round's bug class — the feature reads as "nothing needs you" while the tests stay green:_fj GET "/repos/${repo}/issues?state=open&type=issues&limit=50"— the stub matches on"$2", so it encodes_fj <METHOD> <PATH>. Please confirm against the real_fj.forgejo_list_open_bot_prs "$repo" "${BOT_USER:-}"— called with two args; the stub only reads$1. Harmless if the real arity is 1, wrong if arg 2 means something else.review_rework_rounds "${repo}#${pr}"— stub returns0. This is the silent one:needsyou_pr_whycoerces a non-numeric round to 0, so a wrong key/arity means escalatedREQUEST_CHANGESPRs are never announced, with no error anywhere..review["repo#pr"].verdictin the discretionary state file — also silent on mismatch (verdict reads""→ "reviewer's turn" → nothing announced, forever).automerge_require_human/maintenance_repo_validatedpolarity (rc 0 = "pinned" / "validated").Also unverifiable: the call site's comment claims
ANALYSIS_REPOS_JSONis "assigned well above by the validation sweep" (bin/tick.sh, new block afterCASCADE_TICK=). The${ANALYSIS_REPOS_JSON:-}guard plus the incomplete-scan return means a wrong ordering fails loudly (needs-you: scan incompleteevery 20th tick) — good design — but I can't confirm the ordering itself.Description vs. diff
The checkboxes all correspond to real work; the prose is stale in the understating direction, which makes it an unreliable map rather than a fabrication:
bin/test-needsyou.sh.needsyou_scan_setandneedsyou_passthemselves … verified only by the manual run above" — flatly contradicted by the-- the glue --section of the same diff, which stubs the seams and covers both functions, the state write, theset -euo pipefailpath, and the incomplete-scan bail. That paragraph should be deleted, not left telling the human a gap exists that was closed.needsyou_scan_set+needsyou_passverbatim out oftick.sh" — those functions live inlib/needsyou.shin the shipped diff.git diffhere is 625 insertions, 0 deletions across three files. If the repo's scope gate counts non-comment/non-blank lines, 458 is plausible as that metric, but the human should know the raw number.Scope
625 added lines against a ~400 cap, self-acknowledged. I'm explicitly not asking for it to be trimmed — the overage is tests and the comments that justify the verdict table, which the contract says never to cut. But the split decision is the human's, and the author has already offered one. Worth noting the natural seam is lib+tests vs. the
tick.shwiring, not theneedsyou_pr_numbersfix.Design / coverage observations (non-blocking)
needsyou_pr_whyreturns empty forAPPROVEunless the repo is human-pinned. A PR approved days ago that auto-merge can't take (red CI, merge conflict, protected branch) is nobody's turn under this table, forever. That looks like a real "you are the blocker" case the enumeration drops. The description defends the table against derivation, but doesn't address this hole.check-sync.shenforcing exactly that kind of contract for OUTCOME sentinels and cascade stages — a similar gate is the obvious follow-up.check-sync.shenforces for stages. The stated reason (CASCADE_TICKisn't assigned yet) is fair; flagging it as an architecture call for the human.limit=50on the issue list — a repo with >50 open issues can hide aStatus/Blockedone with no signal.NEEDSYOU_SCAN_EVERY=0in the environment makes the throttle a division by zero ($(( CASCADE_TICK % NEEDSYOU_SCAN_EVERY ))). Exempt from errexit inside theif, so it degrades to noise, but a-gt 0guard is one line.needsyou_removedis defined and tested but unused in production.${CHECKPOINT_WIP_PREFIX:-WIP: }— if the repo's real constant has another name/value, the WIP filter silently stops matching. Failure mode is visible (a checkpoint PR gets announced), so low risk.What's good
The
_needsyou_obj/_needsyou_arrtype checks (not just parse checks) close thekeys-on-an-array hole;_needsyou_listedcorrectly distinguishes "list call didn't answer" from "no bot PRs" given_fj | jqreturns jq's status;needsyou_passthrowing away a partial scan rather than persisting it is the right call and is tested; theerrexit_rchelper with its comment aboutif ( set -e; … )suppressing errexit is a genuinely subtle trap correctly avoided; jq is used with--arg/--argjsonthroughout, no injection surface. No security concerns.Independent review by the harness on
claude-opus-5(effort: max). The human reviewer is requested once Igor has reviewed; a human still merges.