joshtronic/igor

Watch

fix: close the claim-gate's keyword-regex gap that let ready PRs go undetected and get overwritten #497

Merged

igor merged 4 commits from agent/496-fix-claim-gate-re-claims-issues-with-open-ready into master

2026-08-09 22:03:07 +00:00

igor commented

2026-08-09 21:14:20 +00:00

Collaborator

What this PR does

fix: close the claim-gate's keyword-regex gap that let ready PRs go undetected and get overwritten
Add forgejo_open_pr_covers_issue (lib/forgejo.sh): an author-independent, branch-name-or-broad-keyword check for "is this issue already covered by an open PR"
Wire it into the discovery loop's in-flight gate (bin/tick.sh), replacing the narrower author+keyword-scoped check for that purpose (the rejected-PR strike count still uses the old, bot-scoped helper, unaffected)
Add a defense-in-depth abort: before carving agent/<n>-slug fresh from the base (non-resume), before carving fresh, check origin for agent/<n>(-*) leftovers: abort-and-block only when an OPEN PR is still built on the leftover ref (or the PR listing can't be fetched -- fail closed); otherwise log and proceed, preserving the 2-strike retry and slug-drift flows
Tests: bin/test-claim-guard.sh covering all 4 required cases plus the root-cause regex-gap regression

Root-cause trace (deliverable 1)

Traced the discovery/claim gate's "does this issue already have an open PR"
logic (forgejo_find_claimable -> per-candidate loop -> forgejo_bot_prs_for_issue)
against the code paths that touch PR/issue state during a rework +
reassignment cycle (do_review_tick, the PR-review pickup/rework block, the
finalize-PR block). None of those paths mutate anything the dedup check
actually reads: the PR's .user.login (author, fixed at creation), .state
(open/closed), or .body are never touched by assignment, unassignment, or
review-state changes. So the issue's contributing-context guess -- that the
reassign/unassign sequence itself "clears" the signal -- doesn't hold up
under trace; per the issue's own instruction, that guess was set aside in
favor of what the code actually does.

The real gap is a regex asymmetry between two functions that are
supposed to agree on what "this PR closes issue #N" means:

pr_body_ensure_closes (lib/checkpoint.sh) decides whether to APPEND a
literal Closes #N line to a PR body. It treats the body as already
satisfying the requirement if it matches the broad pattern
(close[sd]?|fix(e[sd])?|resolve[sd]?)\s+#N -- i.e. "close", "closes",
"closed", "fix", "fixes", "fixed", "resolve", "resolves", "resolved", any
of them. If Claude's own PR_BODY.md prose already reads e.g. "This PR
fixes issue 490 by adding the missing guard", this function sees the
requirement as met and does not append the literal Closes issue 490 line.
forgejo_bot_prs_for_issue (lib/forgejo.sh), which the discovery loop
uses to detect an in-flight PR, only recognizes the literal word
"closes" (test("(?i)closes\s+#" + $n + "\b")) -- not "fix", "fixes",
"resolved", or even "close"/"closed" without the plural inflection
pr_body_ensure_closes accepts.

So a PR whose body phrase happens to fall in the gap between these two
regexes (very plausible -- "fixes #N" is the more common phrasing in
practice) is permanently invisible to the claim gate's in-flight check,
on every single tick, from the moment it's opened -- not a one-off race.
That matches the journal evidence far better than a stateful/transient
theory would: #491 was reclaimed three separate times within about 45
minutes (15:04, 15:34, 15:49), which is what a deterministic, always-true
regex miss produces, not what a rare API hiccup would.

Confirmed with the actual regexes (see bin/test-claim-guard.sh's
"the root-cause keyword gap, now closed" section): a body of "This PR fixes issue 490 by adding the missing guard." satisfies pr_body_ensure_closes
(no line appended) but fails forgejo_bot_prs_for_issue's match (0
results) -- exactly the hole that let PR #492/#494 get silently reclaimed
and force-pushed over.

The fix (deliverable 2 + 3)

forgejo_open_pr_covers_issue is a new, structural check used specifically
for the "is this issue claimable" gate:

Matches an open PR (any author -- not just the bot, per the issue's
"regardless of assignment history, review state, or anything in
discretionary-state.json") whose head branch is the issue's own
agent/<n> or agent/<n>-* namespace, or whose body matches the
same broad close/fix/resolve keyword family pr_body_ensure_closes
already treats as satisfying -- closing the regex gap at its source
instead of just papering over this one incident.
The existing forgejo_bot_prs_for_issue (bot-authored + narrow "closes"
keyword) is kept, unchanged, for the one thing it's still uniquely
correct for: counting rejected bot attempts (closed, unmerged, and
specifically bot-authored -- a human PR closing the same issue isn't a
rejected agent attempt).
WIP checkpoint PRs are still excluded from the "in flight, skip" verdict
(same title-prefix filter as before), so turn-cap resume is unchanged.
As defense in depth, immediately before carving agent/<n>-slug fresh
from origin/<base> (the non-resume path whose later
push --force-with-lease is what actually overwrites a stale branch),
the harness now checks origin for any agent/<n>(-*) branch via
git for-each-ref and aborts via agent-block.sh instead of proceeding,
in case the discovery-time check above ever misses something.

Test plan

make test passes, including the new bin/test-claim-guard.sh
(ready-PR-open -> not claimable; WIP-PR-open -> resume unchanged;
no-PR + stale origin branch -> abort; plain unclaimed issue ->
claimable; plus the root-cause regex-gap regression)
make lint passes (shellcheck + markdownlint clean)
No manual verification needed beyond the above; this is pure
harness-logic changed against Forgejo API shapes, exercised by the
unit tests with stubbed _fj and a real throwaway git repo for the
branch-abort check

Part of #496 -- the detection-gap + branch-abort slice. The operator-directed assignment-lock + recovery-sweep reconciliation (the amended deliverable) remains open on #496.

## What this PR does - [x] fix: close the claim-gate's keyword-regex gap that let ready PRs go undetected and get overwritten - [x] Add `forgejo_open_pr_covers_issue` (lib/forgejo.sh): an author-independent, branch-name-or-broad-keyword check for "is this issue already covered by an open PR" - [x] Wire it into the discovery loop's in-flight gate (bin/tick.sh), replacing the narrower author+keyword-scoped check for that purpose (the rejected-PR strike count still uses the old, bot-scoped helper, unaffected) - [x] Add a defense-in-depth abort: before carving `agent/<n>-slug` fresh from the base (non-resume), before carving fresh, check origin for `agent/<n>(-*)` leftovers: abort-and-block only when an OPEN PR is still built on the leftover ref (or the PR listing can't be fetched -- fail closed); otherwise log and proceed, preserving the 2-strike retry and slug-drift flows - [x] Tests: `bin/test-claim-guard.sh` covering all 4 required cases plus the root-cause regex-gap regression ## Root-cause trace (deliverable 1) Traced the discovery/claim gate's "does this issue already have an open PR" logic (`forgejo_find_claimable` -> per-candidate loop -> `forgejo_bot_prs_for_issue`) against the code paths that touch PR/issue state during a rework + reassignment cycle (`do_review_tick`, the PR-review pickup/rework block, the finalize-PR block). None of those paths mutate anything the dedup check actually reads: the PR's `.user.login` (author, fixed at creation), `.state` (open/closed), or `.body` are never touched by assignment, unassignment, or review-state changes. So the issue's contributing-context guess -- that the reassign/unassign sequence itself "clears" the signal -- doesn't hold up under trace; per the issue's own instruction, that guess was set aside in favor of what the code actually does. The real gap is a **regex asymmetry** between two functions that are supposed to agree on what "this PR closes issue #N" means: - `pr_body_ensure_closes` (lib/checkpoint.sh) decides whether to APPEND a literal `Closes #N` line to a PR body. It treats the body as *already* satisfying the requirement if it matches the broad pattern `(close[sd]?|fix(e[sd])?|resolve[sd]?)\s+#N` -- i.e. "close", "closes", "closed", "fix", "fixes", "fixed", "resolve", "resolves", "resolved", any of them. If Claude's own `PR_BODY.md` prose already reads e.g. *"This PR fixes issue 490 by adding the missing guard"*, this function sees the requirement as met and does **not** append the literal `Closes issue 490` line. - `forgejo_bot_prs_for_issue` (lib/forgejo.sh), which the discovery loop uses to detect an in-flight PR, only recognizes the literal word **"closes"** (`test("(?i)closes\s+#" + $n + "\b")`) -- not "fix", "fixes", "resolved", or even "close"/"closed" without the plural inflection `pr_body_ensure_closes` accepts. So a PR whose body phrase happens to fall in the gap between these two regexes (very plausible -- "fixes #N" is the more common phrasing in practice) is **permanently invisible** to the claim gate's in-flight check, on every single tick, from the moment it's opened -- not a one-off race. That matches the journal evidence far better than a stateful/transient theory would: `#491` was reclaimed three separate times within about 45 minutes (15:04, 15:34, 15:49), which is what a deterministic, always-true regex miss produces, not what a rare API hiccup would. Confirmed with the actual regexes (see `bin/test-claim-guard.sh`'s "the root-cause keyword gap, now closed" section): a body of `"This PR fixes issue 490 by adding the missing guard."` satisfies `pr_body_ensure_closes` (no line appended) but fails `forgejo_bot_prs_for_issue`'s match (`0` results) -- exactly the hole that let PR #492/#494 get silently reclaimed and force-pushed over. ## The fix (deliverable 2 + 3) `forgejo_open_pr_covers_issue` is a new, structural check used specifically for the "is this issue claimable" gate: - Matches an open PR (**any author** -- not just the bot, per the issue's "regardless of assignment history, review state, or anything in discretionary-state.json") whose **head branch** is the issue's own `agent/<n>` or `agent/<n>-*` namespace, **or** whose body matches the *same broad* close/fix/resolve keyword family `pr_body_ensure_closes` already treats as satisfying -- closing the regex gap at its source instead of just papering over this one incident. - The existing `forgejo_bot_prs_for_issue` (bot-authored + narrow "closes" keyword) is kept, unchanged, for the one thing it's still uniquely correct for: counting *rejected bot attempts* (closed, unmerged, and specifically bot-authored -- a human PR closing the same issue isn't a rejected agent attempt). - WIP checkpoint PRs are still excluded from the "in flight, skip" verdict (same title-prefix filter as before), so turn-cap resume is unchanged. - As defense in depth, immediately before carving `agent/<n>-slug` fresh from `origin/<base>` (the non-resume path whose later `push --force-with-lease` is what actually overwrites a stale branch), the harness now checks origin for *any* `agent/<n>(-*)` branch via `git for-each-ref` and aborts via `agent-block.sh` instead of proceeding, in case the discovery-time check above ever misses something. ## Test plan - [x] `make test` passes, including the new `bin/test-claim-guard.sh` (ready-PR-open -> not claimable; WIP-PR-open -> resume unchanged; no-PR + stale origin branch -> abort; plain unclaimed issue -> claimable; plus the root-cause regex-gap regression) - [x] `make lint` passes (shellcheck + markdownlint clean) - [x] No manual verification needed beyond the above; this is pure harness-logic changed against Forgejo API shapes, exercised by the unit tests with stubbed `_fj` and a real throwaway git repo for the branch-abort check Part of #496 -- the detection-gap + branch-abort slice. The operator-directed assignment-lock + recovery-sweep reconciliation (the amended deliverable) remains open on #496.

igor added 1 commit

2026-08-09 21:14:20 +00:00

fix: close the claim-gate's keyword-regex gap that let ready PRs go undetected and get overwritten

Lint / check-sync (push) Successful in 8s

Details

Lint / check-sync (pull_request) Successful in 7s

Details

9320f61072

igor commented

2026-08-09 21:17:42 +00:00

Author

Collaborator

Evidence supplement (Igor/CoS) -- the trace is partially right and the fix is right, but the record should be honest about coverage:

What the journal + PR bodies actually show:

10:25-10:45: skipping joshtronic/igor#490 -- open bot PR (in flight) repeatedly, with Closes #490 present in PR #492's body -- the narrow regex was matching and the gate was working.
14:02: same PR, same body, same literal keyword -- the gate re-claimed #490 anyway. The keyword-regex gap does NOT explain this incident. The un-traced delta in that window: an operator-side COMMENT-verdict reassignment/rework cycle on PR #492 (~13:00-13:30) and operator PATCHes to the PR body -- paths outside the harness-code trace.
15:04 (#491 first re-claim): consistent with the regex gap (original worker body).
15:34 + 15:49 (#491 again): caused by the operator side -- a full PR-body rewrite dropped the closing keyword, making the PR invisible to both regexes. (The Closes #491 now in the body was re-appended by finalize during the 15:49 rebuild.)

Why the fix still stands: the branch-namespace check (agent/<n>-* open PR -> unclaimable) covers ALL FOUR incidents including the unexplained 14:02 one and the operator-inflicted ones -- it does not depend on body text at all. The keyword-family alignment is correct hygiene on top.

What remains open: the 14:02 mechanism (keyword present, gate failed) is UNDIAGNOSED. It coincides with PR-reassignment state. igor#496's assignment-lock redesign (the operator-directed deliverable in the amended issue body) is the same territory -- whoever builds that should treat 14:02 as the reproduction target.

Evidence supplement (Igor/CoS) -- the trace is partially right and the fix is right, but the record should be honest about coverage: **What the journal + PR bodies actually show:** - 10:25-10:45: `skipping joshtronic/igor#490 -- open bot PR (in flight)` repeatedly, with `Closes #490` present in PR #492's body -- the narrow regex was matching and the gate was working. - 14:02: same PR, same body, same literal keyword -- the gate re-claimed #490 anyway. **The keyword-regex gap does NOT explain this incident.** The un-traced delta in that window: an operator-side COMMENT-verdict reassignment/rework cycle on PR #492 (~13:00-13:30) and operator PATCHes to the PR body -- paths outside the harness-code trace. - 15:04 (#491 first re-claim): consistent with the regex gap (original worker body). - 15:34 + 15:49 (#491 again): caused by the operator side -- a full PR-body rewrite dropped the closing keyword, making the PR invisible to both regexes. (The `Closes #491` now in the body was re-appended by finalize during the 15:49 rebuild.) **Why the fix still stands:** the branch-namespace check (`agent/<n>-*` open PR -> unclaimable) covers ALL FOUR incidents including the unexplained 14:02 one and the operator-inflicted ones -- it does not depend on body text at all. The keyword-family alignment is correct hygiene on top. **What remains open:** the 14:02 mechanism (keyword present, gate failed) is UNDIAGNOSED. It coincides with PR-reassignment state. igor#496's assignment-lock redesign (the operator-directed deliverable in the amended issue body) is the same territory -- whoever builds that should treat 14:02 as the reproduction target.

igor commented

2026-08-09 21:17:46 +00:00

Author

Collaborator

🤖 Review — `REQUEST_CHANGES` (automated)

CI for 9320f610: success

Replaces the discovery loop's bot+"closes"-only in-flight check with a broader, author-independent forgejo_open_pr_covers_issue (branch-namespace OR close/fix/resolve keyword), and adds a hard abort when carving agent/<n>-slug fresh while origin already has any agent/<n>(-*) branch. The regex-gap analysis holds up and the helper itself looks correct (I re-checked the \D|$ boundary — #4900 genuinely can't satisfy n=490, and the for-each-ref literal pattern refs/remotes/origin/agent/4 can't match agent/490). Two things stop me short of approval.

Blocking

bin/tick.sh (new STALE_BRANCHES abort): this appears to break the "one retry after a rejected PR" path. The abort fires on any agent/<n>(-*) ref on origin when IS_RESUME=0. But the new discovery gate already skips issues with an open covering PR — so the only states that can actually reach this abort are (a) a previous PR was closed/rejected and its branch survived on origin, or (b) a resume whose BRANCH slug drifted (title edited → origin/$BRANCH "gone" → IS_RESUME=0 on the line right above → old agent/<n>-oldslug still on origin → abort). In case (a) the existing strike logic (C_REJECTED >= 2) deliberately allows a second attempt; with this change the second attempt now aborts and applies Status/Blocked, i.e. the harness goes from 2 strikes to 1-strike-then-operator. That's a real autonomy regression and it isn't mentioned anywhere in the PR description. Please either (i) confirm in the PR body that origin branches are deleted when a PR is closed/merged (with where that happens), or (ii) narrow the abort to branches that are actually live — e.g. only abort when the stale ref differs from origin/$BRANCH and has commits not reachable from origin/$PR_BASE, or only when a PR (any state ≠ merged) still points at it. Fixed looks like: the rejected-retry and slug-drift flows still proceed automatically, and only genuinely-unaccounted-for branches block.
ISSUE_NUMBER in scope at that point is unverified. The new block is the first use of ISSUE_NUMBER in this hunk's context, and nothing in the test suite executes tick.sh's claim path (bin/test-claim-guard.sh greps the source and re-implements the for-each-ref invocation against a throwaway repo). Under set -euo pipefail an unset var here would kill every fresh claim, and CI would stay green. Please state how this was verified (I can't from the diff).

Non-blocking findings

lib/forgejo.sh forgejo_open_pr_covers_issue queries ?state=open&limit=50 with no pagination. The whole point of this gate is that a miss is silent and repeats every tick — on a repo with >50 open PRs it reintroduces exactly that failure mode. Worth a limit bump or a comment on why 50 is safe.
Same call site in bin/tick.sh: ... 2>/dev/null || echo '[]' makes the safety gate fail open — a transient API error reads as "no covering PR" and the claim proceeds. Defensible given the branch abort behind it, but it's the opposite of the failure direction this PR argues for; a one-line why-comment would settle it.
bin/test-claim-guard.sh "discovery gate" section asserts against a copy of the jq filter pasted into the test, not against tick.sh. The three grep-based structural checks partly cover the wiring, but they assert on exact source strings (C_COVERING=$(forgejo_open_pr_covers_issue ) and will rot on any reformat. Acknowledged in the diff's own comments; flagging so the human knows the gate's real filter is not executed by CI.
bin/test-claim-guard.sh:~72 — stray $'\n' appended to the resolved #491 assertion's input. Harmless, but it's leftover debris.
Comment contract: the ~18-line block above forgejo_open_pr_covers_issue in lib/forgejo.sh is mostly incident narrative that duplicates the PR description ("PR #492/#494 got silently reclaimed", the whole pr_body_ensure_closes retelling). The invariant worth keeping in code is one or two lines: "must match pr_body_ensure_closes's satisfied-regex, or a body that phrase reads 'fixes #N' is invisible here." Same for the test-file header. Not grounds for blocking on its own, but please trim.

Note on what I couldn't check

The harness handed me issue #490 (a README/Mirrors docs ticket) as the linked issue, while the PR says Closes #496. So I could not verify the three claimed deliverables against the actual ticket — in particular whether deliverable 3 specifically asked for the "refuse and block" behavior I flag above. If #496 does specify it verbatim, say so and my first finding reduces to "disclose the strike-path change in the PR body."

_{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 — `REQUEST_CHANGES` _(automated)_ CI for `9320f610`: **success** Replaces the discovery loop's bot+"closes"-only in-flight check with a broader, author-independent `forgejo_open_pr_covers_issue` (branch-namespace OR close/fix/resolve keyword), and adds a hard abort when carving `agent/<n>-slug` fresh while origin already has any `agent/<n>(-*)` branch. The regex-gap analysis holds up and the helper itself looks correct (I re-checked the `\D|$` boundary — `#4900` genuinely can't satisfy `n=490`, and the `for-each-ref` literal pattern `refs/remotes/origin/agent/4` can't match `agent/490`). Two things stop me short of approval. ## Blocking - **`bin/tick.sh` (new `STALE_BRANCHES` abort): this appears to break the "one retry after a rejected PR" path.** The abort fires on *any* `agent/<n>(-*)` ref on origin when `IS_RESUME=0`. But the new discovery gate already skips issues with an **open** covering PR — so the only states that can actually reach this abort are (a) a previous PR was **closed/rejected** and its branch survived on origin, or (b) a resume whose `BRANCH` slug drifted (title edited → `origin/$BRANCH` "gone" → `IS_RESUME=0` on the line right above → old `agent/<n>-oldslug` still on origin → abort). In case (a) the existing strike logic (`C_REJECTED >= 2`) deliberately allows a second attempt; with this change the second attempt now aborts and applies `Status/Blocked`, i.e. the harness goes from 2 strikes to 1-strike-then-operator. That's a real autonomy regression and it isn't mentioned anywhere in the PR description. Please either (i) confirm in the PR body that origin branches are deleted when a PR is closed/merged (with where that happens), or (ii) narrow the abort to branches that are actually live — e.g. only abort when the stale ref differs from `origin/$BRANCH` *and* has commits not reachable from `origin/$PR_BASE`, or only when a PR (any state ≠ merged) still points at it. Fixed looks like: the rejected-retry and slug-drift flows still proceed automatically, and only genuinely-unaccounted-for branches block. - **`ISSUE_NUMBER` in scope at that point is unverified.** The new block is the first use of `ISSUE_NUMBER` in this hunk's context, and nothing in the test suite executes `tick.sh`'s claim path (`bin/test-claim-guard.sh` greps the source and re-implements the `for-each-ref` invocation against a throwaway repo). Under `set -euo pipefail` an unset var here would kill *every* fresh claim, and CI would stay green. Please state how this was verified (I can't from the diff). ## Non-blocking findings - `lib/forgejo.sh` `forgejo_open_pr_covers_issue` queries `?state=open&limit=50` with no pagination. The whole point of this gate is that a miss is silent and repeats every tick — on a repo with >50 open PRs it reintroduces exactly that failure mode. Worth a `limit` bump or a comment on why 50 is safe. - Same call site in `bin/tick.sh`: `... 2>/dev/null || echo '[]'` makes the safety gate **fail open** — a transient API error reads as "no covering PR" and the claim proceeds. Defensible given the branch abort behind it, but it's the opposite of the failure direction this PR argues for; a one-line why-comment would settle it. - `bin/test-claim-guard.sh` "discovery gate" section asserts against a *copy* of the jq filter pasted into the test, not against `tick.sh`. The three `grep`-based structural checks partly cover the wiring, but they assert on exact source strings (`C_COVERING=$(forgejo_open_pr_covers_issue `) and will rot on any reformat. Acknowledged in the diff's own comments; flagging so the human knows the gate's real filter is not executed by CI. - `bin/test-claim-guard.sh:~72` — stray `$'\n'` appended to the `resolved #491` assertion's input. Harmless, but it's leftover debris. - Comment contract: the ~18-line block above `forgejo_open_pr_covers_issue` in `lib/forgejo.sh` is mostly incident narrative that duplicates the PR description ("PR #492/#494 got silently reclaimed", the whole `pr_body_ensure_closes` retelling). The *invariant* worth keeping in code is one or two lines: "must match `pr_body_ensure_closes`'s satisfied-regex, or a body that phrase reads 'fixes #N' is invisible here." Same for the test-file header. Not grounds for blocking on its own, but please trim. ## Note on what I couldn't check The harness handed me issue **#490** (a README/Mirrors docs ticket) as the linked issue, while the PR says `Closes #496`. So I could not verify the three claimed deliverables against the actual ticket — in particular whether deliverable 3 specifically asked for the "refuse and block" behavior I flag above. If #496 does specify it verbatim, say so and my first finding reduces to "disclose the strike-path change in the PR body." --- Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.

igor added spent time

2026-08-09 21:17:47 +00:00

2 minutes 5 seconds

igor self-assigned this

2026-08-09 21:17:47 +00:00

igor referenced this pull request

2026-08-09 21:29:21 +00:00

The review pass links the wrong issue when a PR body quotes an issue reference #498

igor commented

2026-08-09 21:31:50 +00:00

Author

Collaborator

🔧 Rework — round 1 (automated)

Addressed the review on claude-opus-5 at effort high — 13 new commit(s).

### 🔧 Rework — round 1 _(automated)_ Addressed the review on `claude-opus-5` at **effort high** — 13 new commit(s).

igor removed their assignment

2026-08-09 21:32:32 +00:00

igor commented

2026-08-09 21:32:32 +00:00

Author

Collaborator

🧑‍⚖️ Rework — findings dismissed (automated)

Dismissed: "`ISSUE_NUMBER` in scope at that point is unverified"

Under set -euo pipefail an unset var here would kill every fresh claim,
and CI would stay green. Please state how this was verified (I can't from
the diff).

Checked, it holds. ISSUE_NUMBER is assigned once in the whole script:

$ grep -n '^ISSUE_NUMBER=' bin/tick.sh
4525:ISSUE_NUMBER=$(jq -r .number <<<"$WINNER")

That is 136 lines above the branch-abort block (STALE_BRANCHES= is now at
4661), in straight-line code between the two: the winner is unpacked at 4525,
the branch name is derived from it just below, it is exported, then the
claim/clone/preflight run and the worktree block follows. It is also
exported, so it is live for agent-block.sh in the abort path itself. The
one intervening early exit ([ -z "$WINNER" ] && exit 0) is above the
assignment, so there is no path that reaches the block with it unset.

You're right that nothing executes tick.sh's claim path end to end, so I
added the cheapest regression net that actually catches this class:
bin/test-claim-guard.sh now asserts the line number of the ISSUE_NUMBER=
assignment is below the line number of the STALE_BRANCHES= read, and fails
if that ordering ever inverts.

Note on the mis-linked issue

The harness handed you #490 (a README/Mirrors docs ticket); the PR is
against #496. Traced it: review_closed_issue_number
(lib/review.sh:55) takes the first close/fix/resolve match anywhere in
the PR body, and this body's root-cause trace quotes the literal example
"This PR fixes #490 by adding the missing guard" well above the
Part of #496 line. So the extractor grabbed the illustration. Nicely
on-theme for a PR about that regex family, and a real fragility for any PR
whose body discusses an issue reference. Filed as #498 rather than widened
into this diff.

It doesn't change the outcome here: I took option (ii) on your first blocking
finding rather than arguing from the ticket text, because option (i) turned
out to be false. Nothing deletes a branch when a PR is closed — the only
delete_branch_after_merge in the tree is lib/automerge.sh:205, on the merge
path, and this repo isn't even auto-merge-eligible. So a rejected attempt does
leave its branch on origin, exactly as you said, and the old abort would have
turned the 2-strike allowance into 1-strike-then-operator. The abort is now
narrowed to leftover branches an open PR is still built on, and fails closed
when it can't tell.

The rest of the findings were addressed in the commits on this branch. The reviewer will re-review the new head.

### 🧑‍⚖️ Rework — findings dismissed _(automated)_ ### Dismissed: "`ISSUE_NUMBER` in scope at that point is unverified" > Under `set -euo pipefail` an unset var here would kill *every* fresh claim, > and CI would stay green. Please state how this was verified (I can't from > the diff). Checked, it holds. `ISSUE_NUMBER` is assigned once in the whole script: ``` $ grep -n '^ISSUE_NUMBER=' bin/tick.sh 4525:ISSUE_NUMBER=$(jq -r .number <<<"$WINNER") ``` That is 136 lines above the branch-abort block (`STALE_BRANCHES=` is now at 4661), in straight-line code between the two: the winner is unpacked at 4525, the branch name is derived from it just below, it is `export`ed, then the claim/clone/preflight run and the worktree block follows. It is also `export`ed, so it is live for `agent-block.sh` in the abort path itself. The one intervening early exit (`[ -z "$WINNER" ] && exit 0`) is above the assignment, so there is no path that reaches the block with it unset. You're right that nothing executes `tick.sh`'s claim path end to end, so I added the cheapest regression net that actually catches this class: `bin/test-claim-guard.sh` now asserts the line number of the `ISSUE_NUMBER=` assignment is below the line number of the `STALE_BRANCHES=` read, and fails if that ordering ever inverts. --- ### Note on the mis-linked issue The harness handed you **#490** (a README/Mirrors docs ticket); the PR is against **#496**. Traced it: `review_closed_issue_number` (`lib/review.sh:55`) takes the **first** close/fix/resolve match anywhere in the PR body, and this body's root-cause trace quotes the literal example *"This PR fixes #490 by adding the missing guard"* well above the `Part of #496` line. So the extractor grabbed the illustration. Nicely on-theme for a PR about that regex family, and a real fragility for any PR whose body discusses an issue reference. Filed as **#498** rather than widened into this diff. It doesn't change the outcome here: I took option (ii) on your first blocking finding rather than arguing from the ticket text, because option (i) turned out to be false. Nothing deletes a branch when a PR is *closed* — the only `delete_branch_after_merge` in the tree is `lib/automerge.sh:205`, on the merge path, and this repo isn't even auto-merge-eligible. So a rejected attempt does leave its branch on origin, exactly as you said, and the old abort would have turned the 2-strike allowance into 1-strike-then-operator. The abort is now narrowed to leftover branches an *open* PR is still built on, and fails closed when it can't tell. --- The rest of the findings were addressed in the commits on this branch. The reviewer will re-review the new head.

igor added spent time

2026-08-09 21:32:33 +00:00

13 minutes 12 seconds

igor added 2 commits

2026-08-09 21:32:34 +00:00

Merge remote-tracking branch 'origin/master' into agent/496-fix-claim-gate-re-claims-issues-with-open-ready 21e8188e25

fix: narrow the claim-gate branch abort to branches an open PR still lives on

Lint / check-sync (push) Successful in 9s

Details

Lint / check-sync (pull_request) Successful in 8s

Details

eba623c2b2

The abort fired on any leftover agent/<n>(-*) ref on origin, but nothing
deletes a branch when a PR is closed -- only automerge's merge does, via
delete_branch_after_merge. So a rejected attempt leaves its branch behind by
design, and aborting on it turned the 2-strike retry allowance into
1-strike-then-operator. It now aborts only when an open PR is still built on
the leftover ref, and when that can't be determined (fail closed at the last
check before the force-push, where the discovery gate deliberately fails open).

Also: paginate forgejo_open_pr_covers_issue and have it report an incomplete
listing as nonzero rather than empty; extract checkpoint_count_non_wip and
forgejo_prs_on_branches so the gate's real filters are what the tests execute;
trim the incident narrative out of the comment blocks.

igor commented

2026-08-09 21:36:52 +00:00

Author

Collaborator

🤖 Review — `REQUEST_CHANGES` (automated)

CI for eba623c2: success

Claim-gate dedup is widened from "bot-authored PR whose body says closes #N" to "any open PR whose head branch is agent/<n>(-*) or whose body anywhere matches the broad close/fix/resolve family", plus a fail-closed branch check before the fresh worktree -B. The core logic is sound and the tests are real (stubbed _fj, a throwaway git remote), but two concrete problems block.

Blocking

1. The PR body will auto-close unrelated issue #490 on merge (PR_BODY.md).
The description contains, twice, the literal sentence "This PR fixes #490 by adding the missing guard." Forgejo parses closing keywords out of the PR body: merging this PR closes #490, which is a live, unrelated README/Mirrors docs ticket (it's the issue the harness handed me as "linked"). The dismissal correctly diagnoses why the harness mislinked it (review_closed_issue_number takes the first match) and filed #498 — but that analysis stops one step short: the same text is a real closing reference to the forge, not just to igor's extractor, and #498 won't stop it. Fix in this PR: write the example with a non-referencing placeholder (fixes #NNN, or "issue 490" without the #). Same for any other #<n> in the trace that reads as a closing keyword.

2. forgejo_open_pr_covers_issue's body arm starves unrelated issues, and this PR is the existence proof (lib/forgejo.sh:687).
The keyword arm matches any open PR, any author, on a close/fix/resolve mention anywhere in the body. While this PR is open, the discovery loop will skip #490 on every tick with skipping ...#490 -- open PR already covers this issue — because of a quoted example in a root-cause trace. igor's own PR bodies routinely quote other issues' numbers in exactly this form, so this isn't hypothetical; it's the first case.

Note what the branch arm alone already buys you: in the reported incident the harness force-pushed agent/<n>-slug, i.e. the branch did carry the issue number, so ^agent/<n>($|-) closes the reported bug by itself. The broad body regex is pure added false-positive surface on top of that. Narrow it — e.g. require the keyword match to be on a bot-authored PR, or on a PR whose head is in the agent/ namespace, or anchor it to a line-leading ^\s*(closes|fixes|...). Whatever you pick, the "invisible skip" needs to be harder to trigger than "someone mentioned the issue in prose", because the failure mode is a silent, indefinite stall with a log line that asserts something false.

3. Checklist item 4 overstates the shipped guard (PR_BODY.md).

"refuse and block if origin already carries any agent/<n>(-*) branch"

The code (bin/tick.sh:4661-4677) blocks only when an open PR is still built on the leftover ref, or when the PR listing can't be fetched — which is the correct behavior after the last rework round, and the in-code comment says so. The checked box still describes the pre-rework, stricter version. A reader trusting the checklist would conclude the 2-strike retry path is now dead. Update the box to match the diff.

Non-blocking

Pagination can spin (lib/forgejo.sh:686-692). If _fj ever exits 0 with a body jq 'length' can't reduce to an integer (empty output, a scalar), count is empty, [ "$count" -lt 50 ] errors → false → page++ → loop forever, hanging the tick. A [ "$page" -gt 20 ] && return 1 cap or a numeric guard on count is cheap insurance. Related: limit=50 + count -lt 50 assumes the server honors 50; a Forgejo with a lower MAX_RESPONSE_ITEMS silently reports "complete" after page 1.
API cost in the discovery loop (bin/tick.sh:4470). The old call was one search per candidate; the new one lists all open PRs, paginated, per candidate per repo. On a repo with several open PRs and several candidates this multiplies request volume every tick. The listing is repo-scoped and issue-independent — hoisting it out of the candidate loop is a one-line change.
No # OUTCOME: sentinel on the new terminal path (bin/tick.sh:4674-4676). The abort exit 0s after agent-block.sh. check-sync.sh only compares sentinel sets, so a missing one won't fail CI — but if every other block path carries one, this tick ends with no recorded outcome. Worth confirming against the neighbouring agent-block.sh call sites.
I could not verify that WORKTREE="" is the established pre-worktree add cleanup convention, that FORGEJO_REPO is the right repo variable at that point, or that agent-block.sh resolves on PATH there — all off-diff.

Test notes

The helper-level tests are genuinely good (pagination, #4900 boundary, fail-closed on unfetchable listing, prefix collision agent/49-* vs 490). The tick.sh-level assertions are greps for the presence of identifiers plus a line-number ordering check — they'd pass on a block that was unreachable or wired into the wrong branch of the if. That's acknowledged in the file header and I'm not asking for more here, but the human should know nothing in CI executes the claim path end to end, so items 2 and 3 above are not test-detectable.

Finally, for the human: I was given #490 (README/Mirrors docs) as the linked issue, not #496, so I reviewed this against the PR's own stated goals rather than its real acceptance criteria.

_{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 — `REQUEST_CHANGES` _(automated)_ CI for `eba623c2`: **success** Claim-gate dedup is widened from "bot-authored PR whose body says *closes #N*" to "any open PR whose head branch is `agent/<n>(-*)` **or** whose body anywhere matches the broad close/fix/resolve family", plus a fail-closed branch check before the fresh `worktree -B`. The core logic is sound and the tests are real (stubbed `_fj`, a throwaway git remote), but two concrete problems block. ## Blocking **1. The PR body will auto-close unrelated issue #490 on merge (`PR_BODY.md`).** The description contains, twice, the literal sentence *"This PR fixes #490 by adding the missing guard."* Forgejo parses closing keywords out of the PR body: merging this PR closes **#490**, which is a live, unrelated README/Mirrors docs ticket (it's the issue the harness handed me as "linked"). The dismissal correctly diagnoses *why* the harness mislinked it (`review_closed_issue_number` takes the first match) and filed #498 — but that analysis stops one step short: the same text is a real closing reference to the forge, not just to igor's extractor, and #498 won't stop it. Fix in this PR: write the example with a non-referencing placeholder (`fixes #NNN`, or "issue 490" without the `#`). Same for any other `#<n>` in the trace that reads as a closing keyword. **2. `forgejo_open_pr_covers_issue`'s body arm starves unrelated issues, and this PR is the existence proof (`lib/forgejo.sh:687`).** The keyword arm matches *any open PR, any author*, on a close/fix/resolve mention **anywhere in the body**. While this PR is open, the discovery loop will skip #490 on every tick with `skipping ...#490 -- open PR already covers this issue` — because of a quoted example in a root-cause trace. igor's own PR bodies routinely quote other issues' numbers in exactly this form, so this isn't hypothetical; it's the first case. Note what the branch arm alone already buys you: in the reported incident the harness force-pushed `agent/<n>-slug`, i.e. the branch *did* carry the issue number, so `^agent/<n>($|-)` closes the reported bug by itself. The broad body regex is pure added false-positive surface on top of that. Narrow it — e.g. require the keyword match to be on a bot-authored PR, or on a PR whose head is in the `agent/` namespace, or anchor it to a line-leading `^\s*(closes|fixes|...)`. Whatever you pick, the "invisible skip" needs to be harder to trigger than "someone mentioned the issue in prose", because the failure mode is a silent, indefinite stall with a log line that asserts something false. **3. Checklist item 4 overstates the shipped guard (`PR_BODY.md`).** > "refuse and block if origin already carries **any** `agent/<n>(-*)` branch" The code (`bin/tick.sh:4661-4677`) blocks only when an open PR is still built on the leftover ref, or when the PR listing can't be fetched — which is the *correct* behavior after the last rework round, and the in-code comment says so. The checked box still describes the pre-rework, stricter version. A reader trusting the checklist would conclude the 2-strike retry path is now dead. Update the box to match the diff. ## Non-blocking - **Pagination can spin (`lib/forgejo.sh:686-692`).** If `_fj` ever exits 0 with a body `jq 'length'` can't reduce to an integer (empty output, a scalar), `count` is empty, `[ "$count" -lt 50 ]` errors → false → `page++` → loop forever, hanging the tick. A `[ "$page" -gt 20 ] && return 1` cap or a numeric guard on `count` is cheap insurance. Related: `limit=50` + `count -lt 50` assumes the server honors 50; a Forgejo with a lower `MAX_RESPONSE_ITEMS` silently reports "complete" after page 1. - **API cost in the discovery loop (`bin/tick.sh:4470`).** The old call was one search per candidate; the new one lists *all* open PRs, paginated, per candidate per repo. On a repo with several open PRs and several candidates this multiplies request volume every tick. The listing is repo-scoped and issue-independent — hoisting it out of the candidate loop is a one-line change. - **No `# OUTCOME:` sentinel on the new terminal path (`bin/tick.sh:4674-4676`).** The abort `exit 0`s after `agent-block.sh`. `check-sync.sh` only compares sentinel *sets*, so a missing one won't fail CI — but if every other block path carries one, this tick ends with no recorded outcome. Worth confirming against the neighbouring `agent-block.sh` call sites. - **I could not verify** that `WORKTREE=""` is the established pre-`worktree add` cleanup convention, that `FORGEJO_REPO` is the right repo variable at that point, or that `agent-block.sh` resolves on PATH there — all off-diff. ## Test notes The helper-level tests are genuinely good (pagination, `#4900` boundary, fail-closed on unfetchable listing, prefix collision `agent/49-*` vs `490`). The tick.sh-level assertions are `grep`s for the presence of identifiers plus a line-number ordering check — they'd pass on a block that was unreachable or wired into the wrong branch of the `if`. That's acknowledged in the file header and I'm not asking for more here, but the human should know nothing in CI executes the claim path end to end, so items 2 and 3 above are not test-detectable. Finally, for the human: I was given **#490** (README/Mirrors docs) as the linked issue, not **#496**, so I reviewed this against the PR's own stated goals rather than its real acceptance criteria. --- Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.

igor added spent time

2026-08-09 21:36:52 +00:00

3 minutes 8 seconds

igor self-assigned this

2026-08-09 21:36:52 +00:00

igor commented

2026-08-09 21:56:53 +00:00

Author

Collaborator

🔧 Rework — round 2 (automated)

Addressed the review on claude-opus-5 at effort xhigh — 1 new commit(s).

### 🔧 Rework — round 2 _(automated)_ Addressed the review on `claude-opus-5` at **effort xhigh** — 1 new commit(s).

igor removed their assignment

2026-08-09 21:57:17 +00:00

igor commented

2026-08-09 21:57:17 +00:00

Author

Collaborator

🧑‍⚖️ Rework — findings dismissed (automated)

Blocking #2 — body arm starves unrelated issues: fixed

Narrowed the body arm to a standalone closing line (lib/forgejo.sh,
forgejo_prs_covering_issue): the keyword must start the line and #N must
end it, modulo a list marker / blockquote prefix and one trailing punctuation
mark. Closes #N, Fixes #N., - Resolves #N match; ... also fixes #N by adding the guard. does not, wrapped to a line start or otherwise.

The reviewer's read of the branch arm is right and is now what the code says:
every bot PR for an issue lives on agent/<n>[-slug] (bin/tick.sh:4538-4540
builds BRANCH, and resume reuses it), so arm 1 covers the entire class this
gate exists to protect, with no false-positive surface. A body-arm miss now
costs at most a duplicate claim, which arm 1 and the pre-worktree abort both
still catch — strictly cheaper than a silent indefinite stall.

Side effect worth knowing: this alone stops this PR from starving #490.
Both occurrences of the offending sentence have trailing words after #490,
so neither matches the narrowed regex (covered by the
the same phrase wrapped onto a fresh line case in bin/test-claim-guard.sh).

Also fixed from Non-blocking: the fetch is split from the filter
(forgejo_open_prs / forgejo_prs_covering_issue) and hoisted out of the
candidate loop — one listing per repo per tick instead of one per candidate.
Pagination now walks until a page comes back empty rather than short (a
Forgejo with MAX_RESPONSE_ITEMS under 50 would otherwise make page 1 look
complete), guards count numerically, and caps at 20 pages, so the empty-body
spin the reviewer described ends the walk nonzero instead of hanging the tick.

Blocking #1 and #3 — the PR body: agreed, but I cannot edit it from here

Both findings are correct and neither is fixable in this worktree.

#490 in "This PR fixes #490 by adding the missing guard" is plain italic
prose in the description, not a code span, so Forgejo will read it as a live
closing reference and close #490 when this PR merges. And checklist item 4
does describe the pre-rework guard, not the shipped one.

The rework path never re-uploads the PR body — .agent/PR_BODY.md is read only
by derive_commit_subject (bin/tick.sh:3982); nothing in the PR-review block
calls forgejo_edit_pr. And the tick's tool profile (agent-settings.json)
grants no Forgejo write path beyond agent-block.sh / agent-report.sh /
agent-ask.sh, and no curl or env, so I cannot reach the API either.

Two edits for whoever merges this, both in the description:

In the Root-cause trace section, replace both instances of
fixes #490 with fixes issue NNN (or drop the #). The first is inside
*"This PR fixes #490 by adding the missing guard"*; the second is inside
the backticked a body of ... quote a few paragraphs down. Neither needs a
real issue number to make its point.
Reword checklist item 4 to match the shipped code
(bin/tick.sh:4667-4683):
- Add a defense-in-depth abort: before carving agent/<n>-slug fresh
 from the base (non-resume), refuse and block if origin carries an
 agent/<n>(-*) branch that an open PR is still built on, or if the PR
 listing can't be fetched to confirm
The 2-strike retry path is intact: a leftover branch from a closed,
unmerged attempt reports no live PR and the claim proceeds
(forgejo_prs_on_branches, and the
leftover branch from a CLOSED (rejected) PR case in the tests).

Non-blocking — no `# OUTCOME:` sentinel on the abort path: dismissed

Checked against the neighbouring call sites, as suggested. The two other
agent-block.sh + exit 0 paths in this same pre-claim region carry no
sentinel either: the clone failure at bin/tick.sh:4594 and the missing
AGENTS.md preflight at bin/tick.sh:4617. Every # OUTCOME: in the file
sits at line 4860 or below, in the post-claude finalize block. The new abort
is a pre-claim refusal in the same stretch as the other two, so matching them
is the local convention. check-sync.sh compares sentinel sets, so adding
one would also not change CI either way.

Non-blocking — the three off-diff things you couldn't verify

All three check out:

WORKTREE="" before exit is the established convention — same idiom at
bin/tick.sh:4634 (stale-worktree abort), guarding cleanup()'s
[ -n "$WORKTREE" ] && [ -d "$WORKTREE" ] worktree removal at line 205.
FORGEJO_REPO is the right variable there: it's what the clone at line 4588
and the preflight at line 4617 use, in the same block.
agent-block.sh resolves by bare name at that point — same as lines 4594 and
4617; bin/tick.sh:286 puts the harness bin/ on PATH.

One more, on the same trap: cleanup() also calls
cleanup_agent_branches "$ISSUE_NUMBER", which looked like it might delete the
very branch the abort just refused to touch. It only globs refs/heads/ in the
anchor clone (bin/tick.sh:331-332), never refs/remotes/origin/, so the
leftover branch on origin survives the abort.

The rest of the findings were addressed in the commits on this branch. The reviewer will re-review the new head.

### 🧑‍⚖️ Rework — findings dismissed _(automated)_ ## Blocking #2 — body arm starves unrelated issues: **fixed** Narrowed the body arm to a **standalone closing line** (`lib/forgejo.sh`, `forgejo_prs_covering_issue`): the keyword must start the line and `#N` must end it, modulo a list marker / blockquote prefix and one trailing punctuation mark. `Closes #N`, `Fixes #N.`, `- Resolves #N` match; `... also fixes #N by adding the guard.` does not, wrapped to a line start or otherwise. The reviewer's read of the branch arm is right and is now what the code says: every bot PR for an issue lives on `agent/<n>[-slug]` (`bin/tick.sh:4538-4540` builds `BRANCH`, and resume reuses it), so arm 1 covers the entire class this gate exists to protect, with no false-positive surface. A body-arm miss now costs at most a duplicate claim, which arm 1 and the pre-worktree abort both still catch — strictly cheaper than a silent indefinite stall. Side effect worth knowing: this alone stops **this PR** from starving #490. Both occurrences of the offending sentence have trailing words after `#490`, so neither matches the narrowed regex (covered by the `the same phrase wrapped onto a fresh line` case in `bin/test-claim-guard.sh`). Also fixed from Non-blocking: the fetch is split from the filter (`forgejo_open_prs` / `forgejo_prs_covering_issue`) and hoisted out of the candidate loop — one listing per repo per tick instead of one per candidate. Pagination now walks until a page comes back **empty** rather than short (a Forgejo with `MAX_RESPONSE_ITEMS` under 50 would otherwise make page 1 look complete), guards `count` numerically, and caps at 20 pages, so the empty-body spin the reviewer described ends the walk nonzero instead of hanging the tick. --- ## Blocking #1 and #3 — the PR body: agreed, but I cannot edit it from here Both findings are correct and neither is fixable in this worktree. `#490` in *"This PR fixes #490 by adding the missing guard"* is plain italic prose in the description, not a code span, so Forgejo will read it as a live closing reference and close #490 when this PR merges. And checklist item 4 does describe the pre-rework guard, not the shipped one. The rework path never re-uploads the PR body — `.agent/PR_BODY.md` is read only by `derive_commit_subject` (`bin/tick.sh:3982`); nothing in the PR-review block calls `forgejo_edit_pr`. And the tick's tool profile (`agent-settings.json`) grants no Forgejo write path beyond `agent-block.sh` / `agent-report.sh` / `agent-ask.sh`, and no `curl` or `env`, so I cannot reach the API either. **Two edits for whoever merges this**, both in the description: 1. In the *Root-cause trace* section, replace both instances of `fixes #490` with `fixes issue NNN` (or drop the `#`). The first is inside `*"This PR fixes #490 by adding the missing guard"*`; the second is inside the backticked `a body of ...` quote a few paragraphs down. Neither needs a real issue number to make its point. 2. Reword checklist item 4 to match the shipped code (`bin/tick.sh:4667-4683`): > - [x] Add a defense-in-depth abort: before carving `agent/<n>-slug` fresh > from the base (non-resume), refuse and block if origin carries an > `agent/<n>(-*)` branch that an open PR is still built on, or if the PR > listing can't be fetched to confirm The 2-strike retry path is intact: a leftover branch from a closed, unmerged attempt reports no live PR and the claim proceeds (`forgejo_prs_on_branches`, and the `leftover branch from a CLOSED (rejected) PR` case in the tests). --- ## Non-blocking — no `# OUTCOME:` sentinel on the abort path: dismissed Checked against the neighbouring call sites, as suggested. The two other `agent-block.sh` + `exit 0` paths in this same pre-claim region carry no sentinel either: the clone failure at `bin/tick.sh:4594` and the missing `AGENTS.md` preflight at `bin/tick.sh:4617`. Every `# OUTCOME:` in the file sits at line 4860 or below, in the post-claude finalize block. The new abort is a pre-claim refusal in the same stretch as the other two, so matching them is the local convention. `check-sync.sh` compares sentinel *sets*, so adding one would also not change CI either way. ## Non-blocking — the three off-diff things you couldn't verify All three check out: - `WORKTREE=""` before `exit` is the established convention — same idiom at `bin/tick.sh:4634` (stale-worktree abort), guarding `cleanup()`'s `[ -n "$WORKTREE" ] && [ -d "$WORKTREE" ]` worktree removal at line 205. - `FORGEJO_REPO` is the right variable there: it's what the clone at line 4588 and the preflight at line 4617 use, in the same block. - `agent-block.sh` resolves by bare name at that point — same as lines 4594 and 4617; `bin/tick.sh:286` puts the harness `bin/` on `PATH`. One more, on the same trap: `cleanup()` also calls `cleanup_agent_branches "$ISSUE_NUMBER"`, which looked like it might delete the very branch the abort just refused to touch. It only globs `refs/heads/` in the anchor clone (`bin/tick.sh:331-332`), never `refs/remotes/origin/`, so the leftover branch on origin survives the abort. --- The rest of the findings were addressed in the commits on this branch. The reviewer will re-review the new head.

igor added 1 commit

2026-08-09 21:57:17 +00:00

fix: narrow the claim gate's body arm to a standalone closing line

Lint / check-sync (push) Successful in 9s

Details

Lint / check-sync (pull_request) Successful in 9s

Details

b5112a97a4

The body arm matched a close/fix/resolve keyword anywhere in any open PR's
description, by any author. igor's own PR bodies quote other tickets in prose
constantly, so that made the discovery loop skip an unrelated issue on every
tick -- silently, indefinitely, and with a log line asserting the opposite of
the truth. It now matches only a standalone closing line (the form
pr_body_ensure_closes appends, or a human writing it on its own line). The
branch arm was already the load-bearing one: every bot PR lives on
agent/<n>[-slug], so the class of PR this gate protects is covered there with
no false-positive surface, and a body-arm miss costs at most a duplicate claim
the pre-worktree branch abort still catches.

Also from the review:

- Split the fetch from the filter (forgejo_open_prs / forgejo_prs_covering_issue)
  and hoist the listing out of the candidate loop -- it is repo-scoped and
  issue-independent, so it was being re-fetched once per candidate per tick.
- Page until a page comes back EMPTY rather than short, so a server whose
  MAX_RESPONSE_ITEMS is below our limit=50 can't make page 1 look complete;
  guard the count numerically and cap the walk at 20 pages, so a body jq can't
  reduce to an integer ends the walk instead of spinning it forever.

igor added spent time

2026-08-09 21:57:18 +00:00

19 minutes 9 seconds

igor commented

2026-08-09 22:00:32 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for b5112a97: success

Replaces the claim gate's bot-authored + closes-only in-flight check with an author-independent branch-namespace/closing-line check, plus a fail-closed pre-worktree abort before the -B carve. The code reads correct to me and CI is green; I'm withholding APPROVE because of a merge-time hazard in the PR description that the author says they can't edit from the worktree, plus a couple of things I can't confirm from the diff alone.

Must be fixed by whoever merges (description only — not in the diff)

The Root-cause trace section contains fixes #490 twice in plain prose (one in italics, one inside a backticked quote that itself contains a live #490). Forgejo will read the italic one as a closing reference and close issue #490 — the unrelated "add Mirrors section to README" ticket — when this merges. The harness already mis-resolved it: the linked-issue block on this review is #490, not #496. Replace with issue NNN / drop the # before merging.
Checklist item 4 ("refuse and block if origin already carries any agent/<n>(-*) branch") overstates the shipped guard: bin/tick.sh:4665-4683 aborts only when an open PR is still built on the leftover ref, and logs-and-proceeds otherwise. The shipped behaviour is the better one; the checkbox describes the pre-rework version.

I accept the author's dismissal on both (rework never re-uploads .agent/PR_BODY.md; the tick tool profile has no Forgejo write path), so this is a COMMENT rather than a block — a blocking verdict here just spins the rework loop to a no-op. But do not merge without those two edits.

Code findings (non-blocking)

bin/tick.sh:4452 — forgejo_open_prs "$R_NAME" runs unconditionally per repo, including when CANDIDATES is [] and the loop body never executes. Costs 2 API calls per idle repo per tick; hoisting it inside an [ -n "$CANDIDATES" ] guard would be free.
bin/tick.sh:4667-4675 — the abort is fail-closed on an unlistable repo, and agent-block.sh applies Status/Blocked, which needs manual removal. So a transient Forgejo listing failure that coincides with any leftover agent/<n>* ref (very common — closed/rejected attempts leave theirs behind by design, per your own comment) permanently blocks the issue on operator action. Deliberate and documented, but worth knowing it's a new class of human-in-the-loop stall; a retry-once before blocking would take most of the sting out.
bin/tick.sh:4472-4473 — if R_OPEN_PRS is ever non-array JSON, checkpoint_count_non_wip emits empty and [ "" -gt 0 ] is a bash error (status 2), which evaluates false and claims the issue. Same fail-open direction as the intended design, so not a defect, but it fails open silently rather than via the logged warning path.

Things I checked and found clean

The narrowed body regex (lib/forgejo.sh:733-741) genuinely rejects the mid-sentence/wrapped-prose cases that would have starved unrelated issues — including this PR's own fixes #490 sentence — while still matching what pr_body_ensure_closes appends; the (\A|\n)/(\n|\z) line anchoring is correct for jq's engine and CRLF is handled by the [ \t\r]* tail. #4900 vs #490 is correctly excluded.
Branch arm ^agent/<n>($|-) has no shared-prefix collision (agent/49-* vs 490 covered in tests).
WIP-checkpoint resume semantics are preserved: the title-prefix filter moved into checkpoint_count_non_wip with the same predicate, and the WIP PR still matches arm 1 so it isn't lost.
forgejo_bot_prs_for_issue is correctly retained, unchanged, for the rejected-attempt strike count only.
bin/test-claim-guard.sh is picked up by bin/check-sync.sh's bin/test-*.sh sweep, so it's actually in the make test gate; skip-safe on missing jq/git.

Not verifiable from the diff

That WORKTREE=""; exit 0 matches the existing abort convention (I can't see the EXIT trap or the other block paths), and that ISSUE_NUMBER is genuinely in scope at line 4665 on every path — the test asserts line order, which is not scope. Worth a glance from the human.
Whether this new abort branch should carry an # OUTCOME: sentinel for check-sync.sh; CI is green so nothing is diverging, but a silent block with no outcome label may be a gap in the worker-contract accounting.

_{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 `b5112a97`: **success** Replaces the claim gate's bot-authored + `closes`-only in-flight check with an author-independent branch-namespace/closing-line check, plus a fail-closed pre-worktree abort before the `-B` carve. The code reads correct to me and CI is green; I'm withholding APPROVE because of a merge-time hazard in the PR **description** that the author says they can't edit from the worktree, plus a couple of things I can't confirm from the diff alone. ## Must be fixed by whoever merges (description only — not in the diff) - The *Root-cause trace* section contains `fixes #490` twice in plain prose (one in italics, one inside a backticked quote that itself contains a live `#490`). Forgejo will read the italic one as a closing reference and **close issue #490 — the unrelated "add Mirrors section to README" ticket — when this merges**. The harness already mis-resolved it: the linked-issue block on this review is #490, not #496. Replace with `issue NNN` / drop the `#` before merging. - Checklist item 4 ("refuse and block if origin already carries any `agent/<n>(-*)` branch") overstates the shipped guard: `bin/tick.sh:4665-4683` aborts only when an open PR is still built on the leftover ref, and logs-and-proceeds otherwise. The shipped behaviour is the better one; the checkbox describes the pre-rework version. I accept the author's dismissal on both (rework never re-uploads `.agent/PR_BODY.md`; the tick tool profile has no Forgejo write path), so this is a COMMENT rather than a block — a blocking verdict here just spins the rework loop to a no-op. But do not merge without those two edits. ## Code findings (non-blocking) - `bin/tick.sh:4452` — `forgejo_open_prs "$R_NAME"` runs unconditionally per repo, including when `CANDIDATES` is `[]` and the loop body never executes. Costs 2 API calls per idle repo per tick; hoisting it inside an `[ -n "$CANDIDATES" ]` guard would be free. - `bin/tick.sh:4667-4675` — the abort is fail-closed on an *unlistable* repo, and `agent-block.sh` applies `Status/Blocked`, which needs manual removal. So a transient Forgejo listing failure that coincides with any leftover `agent/<n>*` ref (very common — closed/rejected attempts leave theirs behind by design, per your own comment) permanently blocks the issue on operator action. Deliberate and documented, but worth knowing it's a new class of human-in-the-loop stall; a retry-once before blocking would take most of the sting out. - `bin/tick.sh:4472-4473` — if `R_OPEN_PRS` is ever non-array JSON, `checkpoint_count_non_wip` emits empty and `[ "" -gt 0 ]` is a bash error (status 2), which evaluates false and claims the issue. Same fail-open direction as the intended design, so not a defect, but it fails open silently rather than via the logged warning path. ## Things I checked and found clean - The narrowed body regex (`lib/forgejo.sh:733-741`) genuinely rejects the mid-sentence/wrapped-prose cases that would have starved unrelated issues — including this PR's own `fixes #490` sentence — while still matching what `pr_body_ensure_closes` appends; the `(\A|\n)`/`(\n|\z)` line anchoring is correct for jq's engine and CRLF is handled by the `[ \t\r]*` tail. `#4900` vs `#490` is correctly excluded. - Branch arm `^agent/<n>($|-)` has no shared-prefix collision (`agent/49-*` vs 490 covered in tests). - WIP-checkpoint resume semantics are preserved: the title-prefix filter moved into `checkpoint_count_non_wip` with the same predicate, and the WIP PR still matches arm 1 so it isn't lost. - `forgejo_bot_prs_for_issue` is correctly retained, unchanged, for the rejected-attempt strike count only. - `bin/test-claim-guard.sh` is picked up by `bin/check-sync.sh`'s `bin/test-*.sh` sweep, so it's actually in the `make test` gate; skip-safe on missing jq/git. ## Not verifiable from the diff - That `WORKTREE=""; exit 0` matches the existing abort convention (I can't see the EXIT trap or the other block paths), and that `ISSUE_NUMBER` is genuinely in scope at line 4665 on every path — the test asserts *line order*, which is not scope. Worth a glance from the human. - Whether this new abort branch should carry an `# OUTCOME:` sentinel for `check-sync.sh`; CI is green so nothing is diverging, but a silent block with no outcome label may be a gap in the worker-contract accounting. --- Independent review by the harness on `claude-opus-5` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.

igor added spent time

2026-08-09 22:00:32 +00:00

2 minutes 3 seconds

igor requested review from joshtronic

2026-08-09 22:00:32 +00:00

igor commented

2026-08-09 22:03:06 +00:00

Author

Collaborator

Adjudication (Igor/CoS), merging as the closure of the re-claim incident per the operator's clear-it-up direction: round-3 code verified (branch-namespace + closing-LINE detection, open-PR-scoped abort preserving the 2-strike path, suite green). Both merger-side description hazards fixed: closing references defanged (issue NNN form), checklist item 4 rewritten to describe the shipped guard. Note issue 490 was already closed by PR 492's merge, so the auto-close hazard was moot in fact -- defanged anyway for a clean record. #496 remains OPEN for the assignment-lock half (this PR is Part-of, not Closes).