16 commits

Author	SHA1	Message	Date
cluade-reviewer-andresmgsl	a35a77f752	fix(forgejo): a read failure names its verb too, and the tests assert the whole diagnostic (#192) ... @codex-reviewer-andresmgsl caught a test that describes evidence it does not collect — mine, and it is the class this PR is about. Two cases were titled "naming the verb, path and status" and asserted only the substring "500". The PUT boundary happened to satisfy the contract because forgejo_write already passes "PUT $endpoint" to forgejo_http_ok. The GET boundary did not: the diagnostic was `HTTP 500 from 'repos/o/r/issues/5'`, with no verb at all — so a caller could not tell a failed READ from a failed WRITE of the same path, and #192's acceptance criterion asks for exactly that distinction. Reads now pass "GET $endpoint" on both non-paginated and paginated paths, and the two tests assert the complete expected diagnostic as one substring rather than a status code that any failure would contain. Reverting the verb reds the GET case. Also, per the same review: the failed GET is asserted to write nothing, and the failed PUT to have attempted exactly one write. forge-backends 117/117 (was 115), test/run.sh 22/22 under jq 1.7 and jq 1.6, shellcheck 0.10.0 and actionlint clean. Refs #192	2026-08-05 13:11:33 +00:00
cluade-reviewer-andresmgsl	062e016a42	fix(labels): all four review gaps — preserved ids, zero-write no-op, every mutation counted, no success token (#192) ... @codex-reviewer-andresmgsl's four gaps, all real, all taken. 1. PRESERVED IDS COME FROM THE ISSUE. The removal path read only .labels[].name and then re-resolved every preserved label through the repository-wide list — so preservation depended on a paginated read with nothing to do with this issue, and an incomplete one would drop a bystander. It now keeps name<TAB>id from the issue payload, subtracts removals by name, and resolves ONLY added names. Fixture: a bystander on the issue with id 14 that is absent from the repo-list fixture entirely must still survive the PUT. 2. AN ABSENT REMOVAL WRITES NOTHING. I had it PUT the unchanged set, arguing the write proved the sweep reached the forge. The GET already proves that, and replacing a set with itself opens ceremony#128's window for no state change — most calls here are exactly this case, since the reconcilers call --remove-label unconditionally. Short-circuits when the wanted set equals the current one. This was the policy-shaped choice flagged for @andres; the reviewer's reasoning is better than mine was. 3. EVERY LABEL MUTATION REACHES THE TALLY. The marker was only on the primary state edit, so clearing `merge-next` and both `stale` edits could fail into the generic per-PR branch and still finish `reconciled.` and exit 0. All four sites go through one `label_write` helper, so a future call site cannot reopen it by forgetting to mark itself. Probe: a failed NON-primary write (unstale on a blocked PR) must fail the sweep. 4. NO SUCCESS TOKEN IN A FAILURE TAIL. "NOT reconciled." still contains "reconciled.", which a log-tail consumer greps for. The line is now "sweep incomplete", and the test asserts the whole output is free of the token rather than only of the success prefix. Also added the two fault boundaries the acceptance plan named and the fixtures never proved: a failed current-label GET and a failed replacement PUT, each non-zero with the backend's verb/path/status diagnostic. Mutation-tested, each gap separately: bypassing the tally reds 3, re-resolving preserved ids reds 7, writing the unchanged set reds 1. forge-backends 115/115 (was 110), labels-reconcile 175/175 (was 172), test/run.sh 22/22 under jq 1.7 and jq 1.6, shellcheck 0.10.0 and actionlint clean. Refs #192	2026-08-05 13:03:25 +00:00
cluade-reviewer-andresmgsl	0f20f4b6ef	fix(labels): a label removal that cannot happen fails the sweep, and removal itself now works (#192) ... Two defects, one cause, and the second is why the first survived a week. THE WRITE. Removal was a per-label `DELETE .../labels/{id}` loop. On this instance that call returns HTTP 500 for every removal under the token the sweep actually holds — measured inside Actions, probe run 701, where the same `PUT .../labels` with the desired full set returns 200 including the empty set for a full clear. A PAT gets 204 on the same DELETE, which is exactly why it went unseen: it fails only for `${{ github.token }}`. Net effect before this: on Forgejo the state machine could only ever ADD labels. Every `state:*` transition needing the previous state cleared and every `blocker:*` that should lift was inert. Both PRs open right now carry stale `blocker:*` labels that are false and that nothing can remove. So the removal path is read-current, compute-wanted, one PUT — the same shape the assignee branch beside it already used. An ADD-ONLY call keeps its additive POST: ceremony#128 lost a `release` label to a read-modify-write that clobbered a concurrent set, and forge_labels_add stays pinned against ever doing that. The window is accepted here and only here, where the caller asked to REMOVE and no additive verb can say that. An unresolvable --add-label refuses before any write, so a replacement PUT can never drop a label nobody asked to remove. THE REPORTING. `labels-reconcile` logged `WARNING: label edit failed`, fell through, and `main` printed `reconciled.` and exited 0 — while `issueflow-reconcile` treated the identical 500 as fatal. One cause, two contradictory policies, and the wrong one hid the write defect. A failed write is fatal now, and the tally reaches main's exit code. That second half is load-bearing: making reconcile_pr fatal alone is not enough, because the loop swallows a per-PR non-zero into a log line and finishes. The per-PR tolerance is right and stays — one bad PR must not blind the board — but it now applies to READS. A sweep that could not write exits non-zero and never prints `reconciled.` The diagnostic says what was attempted and that it did not happen. The old text blamed a missing label and told the operator to bootstrap, when the label was present and the call returned 500 — #101's rule is report, do not diagnose. Mutation-tested, all three ways: restoring the warn-and-continue reds 5 cases, removing the tally reds 2, restoring the DELETE loop reds 7. test/run.sh 22 files 0 failed under jq 1.7 and jq 1.6; shellcheck 0.10.0 and actionlint clean. Refs #192	2026-08-05 12:48:09 +00:00
cluade-reviewer-andresmgsl	ca99182e80	fix(forge): percent-encode asset names, and stop the docs naming a client ... Both findings are @codex's on !193 (#1583), and both are real. The asset name travels as a QUERY VALUE, and the artifact-hook contract permits any file the consumer drops in RELEASE_ASSETS_DIR. Raw interpolation meant `release asset.tgz` made curl reject the URL outright (exit 3), and '&', '#', '+', '%' silently changed the name or the query's shape. `gh release create` handled all of those, so a 1:1 port had to. Encoded through one boundary — jq's @uri, since jq is already a hard dependency of this backend and a hand-rolled sed class is how the next unescaped character gets through. Six backend cases cover it: the encoder on a space and on the delimiters, uploads under both names, the created release id in the path, and the multipart attachment. Mutation-checked: dropping the encoder fails exactly the two name assertions. docs/CONSUMERS.md's artifact-hook recovery still told operators to "run `gh release create` by hand" and described the hook as running "before `gh release create`" — on a Forgejo runner that is precisely the failure this PR fixes. It now names the forge-neutral tag-door recovery first and shows both clients for the manual path, without regressing the GitHub guidance. 1035 assertions, 22 suites, shellcheck-all and actionlint clean. Refs #191 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-04 12:11:06 +00:00
cluade-reviewer-andresmgsl	21c70e06a4	fix(forge): an empty REPO cannot become a fact, and the backend verbs are tested ... Both panel blockers on c63a550. @kimi found the one that mattered: facts.sh got the REPO fix, release.yml's own four call sites did not. A workflow `run:` shell carries no `set -u`, so an unset REPO expands empty and the verb addresses `repos//…` — which 404s, and the 404 is then read as an ANSWER. Reproduced read-only against this instance before fixing: forge_release_exists 0.4.1 -> "no", rc 0 forge_commit_pulls 7fc9afe4 -> "[]", rc 0 (the !189 merge, which HAS a merged PR behind it) The first would have let the nothing-exists assert proceed to CREATE; the second is the drill's original fabricated `labeled=no`, one step after the fix meant to kill it. Fixed once rather than at four call sites, as kimi suggested: forge_select defaults REPO from GITHUB_REPOSITORY, and forgejo_api_base — which every verb reaches the network through — refuses an empty REPO outright. No fifth call site can forget it. @grok and @kimi both blocked on the same AC gap: the backend suite did not cover the five new verbs, so the two measured asymmetries had no offline coverage. test/forge-backends.test.sh now has 15 cases for them — singular /pull wrapped to an array, 404 as an empty array, 500 refusing, release present/absent/unreadable, POST /tags vs /git/refs, the publish body, and the REPO-empty must-fail. Mutation-checked: reading the plural path fails one case, dropping the REPO guard fails the two must-fails. 1029 assertions, 22 suites, shellcheck-all and actionlint clean. Refs #191 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-04 11:53:28 +00:00
cluade-reviewer-andresmgsl	4e929e2083	test(forge): the negative half of the github pass-through pins ... @grok-reviewer-andresmgsl landed the term-5 pins in ff17d1e while I had the same two asks in flight locally; theirs is on the branch and I dropped my duplicate rather than push a competing tip. This adds only what the two suites did not share. Their pins are positive: the github twins DO call the right endpoints, and forge_pr_activity emits all three timestamp sources. Mine had two negatives they did not, and negatives are what catch the drift a 1:1-extraction path actually suffers — a positive pin still passes if the github path GAINS forgejo behaviour, and term 5 is a statement about what must NOT change. - the github timeline is never reshaped. That timeline already IS the shape ruling.sh selects on, so a projection here would be a second, divergent normalizer maintained by nobody. - github activity never derives inline comments from reviews. That derivation exists on the forgejo path only because the flat endpoint 404s there; a github twin quietly adopting the workaround is the "both backends drift together" failure term 5 forbids. Mutation-verified: adding a --jq projection to the github timeline, and swapping the flat PR-comments read for a reviews-derived one, each red their own case. forge-backends 77 -> 79. Refs #188	2026-08-03 15:30:28 +00:00
grok-reviewer-andresmgsl	ff17d1ea3f	fix(forge): term-5 GitHub pins for timeline/activity + keep activity stderr (#188) ... Codex 1566 held APPROVE: only Forgejo stubs covered forge_timeline and forge_pr_activity. Pin the github twins as 1:1 extractions (timeline paginate; issue comments + flat pulls comments + commits). Cluade #4879: drop 2>/dev/null on the labels-reconcile activity call site so a failed read still degrades last_activity but names the failure in the job log (keep || true).	2026-08-03 15:26:08 +00:00
grok-reviewer-andresmgsl	5c8e4f5b84	feat(forge): timeline normalizer, portable PR activity, shellcheck install (#188) ... Panel-unanimous batch that was staged unpushed on 57abe15 (#4853): - forge_timeline: project Forgejo label events into the GitHub shape so the ruling ladder fires on this forge (measured mapping #4849) - forge_pr_activity: stop calling /pulls/{n}/comments (404 here); use reviews with comments_count > 0 for inline comments (#4844) - ci.yml: install shellcheck before lint, mirroring actionlint — the act-22.04 runner image does not ship it Status captured before jq so an unreadable timeline cannot report empty.	2026-08-03 15:13:30 +00:00
cluade-reviewer-andresmgsl	9357f09aea	fix: the four findings from the panel round on 2168e4e ... @codex-reviewer-andresmgsl #4780, concurred by @grok-reviewer-andresmgsl #4785. All four real. 1. Three issueflow call sites still named per_page=100. The backend sanitized it so it worked, but the frozen term and the changelog both say no call site names a page size — and a contract that holds only because something downstream cleans up is not the contract. Endpoints now carry their logical query alone. 2. The suite's summary and `[ "$fail" -eq 0 ]` gate sat in the MIDDLE of test/labels-reconcile.test.sh, and the eight outstanding_requests expects were appended after them. Proven before fixing: a deliberately broken term-4 assertion printed FAIL, was excluded from the totals, and the suite still exited 0. Those assertions were decorative. The gate moves to the true end, with a note that nothing goes below it; the reported count goes 157 -> 164, which is the eight that were never being counted. 3. forge_labels_add and forge_request_reviewer arrived with the port and had no boundary pins. Both backends now have them, and the labels_add cases pin the property ceremony#128 turns on: an additive POST, never a PUT of the whole set, exactly one write so nothing is read-modify-written. Mutation-verified — making it RMW/PUT, or routing github through `issue edit --add-label`, each red their own cases. 4. The historical comment said the old gathers were `forge_api graphql`. My own mechanical port rewrote it; before #188 they were `gh api graphql` and the abstraction did not exist. Refs #188	2026-08-02 19:58:51 +00:00
cluade-reviewer-andresmgsl	2168e4ef9a	test(forge): hermetic cases for the two forgejo edit asymmetries ... The coverage owed with the call-site port (@grok-reviewer-andresmgsl #4741 note 2, #4751 item 2). Live scratch-repo evidence proved these work; these pin the request SHAPE so they keep working. - a removal resolves name -> numeric id, and never sends the name as the path segment (measured: DELETE .../labels/probe:one -> 422, DELETE .../labels/149 -> 204); - a removal of a label the repo does not have writes nothing, matching gh: the reconcilers call --remove-label unconditionally to converge state; - adds take names directly, one request, comma-separated values split as gh splits them; - an assignee removal PATCHes the SURVIVING list, because Forgejo sets assignees rather than adding and removing them — a naive translation would have cleared every other assignee as a side effect of removing one, which is what the mutation test proves is caught. Payloads are now compact JSON. They were pretty-printed, which spread a single write across several lines — harder to read in a log, and it hid the shape from any assertion matching a line. Refs #188	2026-08-02 19:50:37 +00:00
cluade-reviewer-andresmgsl	dce12e0bb5	fix(test): the second curl stub needed the SC2317 disable too ... a968e13 was pushed with shellcheck red. I chained the gates and the push in one command, so a non-zero gate did not stop the push — the gate has to be a condition, not a line of output I read afterwards. Refs #188	2026-08-02 19:24:27 +00:00
cluade-reviewer-andresmgsl	a968e13ca4	fix(forge): parity gaps in the forgejo verbs — upsert, timestamps, typos ... @codex-reviewer-andresmgsl's three findings (#4743), all real. 1. forge_label_create is now an UPSERT, matching gh label create --force. bootstrap_labels creates every declared label on EVERY workflow_dispatch, so a plain POST onto an existing name aborted the bootstrap under set -e from the second dispatch onward. Resolves name -> id and PATCHes when it exists. 2. forge_pr_view carries createdAt/completedAt. checks_state groups repeated contexts and selects the newest by [.startedAt, .createdAt, .completedAt]; mapping only {context,state} left the winner to incidental array order, so a stale re-run could outrank the live verdict. The combined status carries created_at and updated_at — measured. 3. forge_issue_edit refuses unknown flags and missing values. The github backend hands them to gh, which fails; dropping them here turned a mis-typed port site into a mutation that silently did not happen — this issue's own failure class, inside the fix for it. Also settles @grok-reviewer-andresmgsl's note 3 (#4741): Forgejo Actions DO land as commit statuses on this instance, so the rollup is not empty. rig main carries four — "ci / check (push)" and siblings, state success, each with created_at. statusCheckRollup therefore populates, and NONE is not silently substituted for SUCCESS. Each fix mutation-verified: dropping the timestamps, forcing POST-always, and restoring the silent flag skip each red exactly their own cases. The newest-verdict case drives the real checks_state, not a copy. Refs #188	2026-08-02 19:22:28 +00:00
cluade-reviewer-andresmgsl	adf3299192	test(forge): assert the distinguishing text, not a surviving substring ... @codex-reviewer-andresmgsl (#4727) and @grok-reviewer-andresmgsl (#4734): "...and the refusal names both totals" searched only for "4", so it stayed green if the later total vanished from the message. A case named "names BOTH" must fail when one goes. Now asserts "4 then 9". Auditing this file's siblings for the same shape found a second, older instance: "the refusal names the client" searched for "gh", which also occurs in the explanatory prose ("gh speaks GitHub's /api/v3..."), so it would have passed even if the client name never reached the message. Now asserts "the 'gh' client cannot speak it". Both verified by mutation: removing the second total, and removing the interpolated client name, each red exactly their own case. Refs #188	2026-08-02 19:16:48 +00:00
cluade-reviewer-andresmgsl	66e20f12f0	fix(forge): validate the completeness bound itself, on every page ... @codex-reviewer-andresmgsl's three findings (#4712), each a route by which an unprovable read could still be reported as a whole one — the guard leaking the failure class it exists to stop. 1. x-total-count was never validated. `X-Total-Count: not-a-number` returned rc=0 with that string as the bound the walk compared against, reproduced on ab23a3b. Now required to be a canonical non-negative integer. 2. The total was read once. A collection changing size under the walk was invisible: page 1 declaring 4 and page 2 declaring 9 stopped at 4 believing itself whole. Now re-read per page; a moving total means the read was not atomic and is refused. 3. A 200 whose body is not an array counted as zero items, so an error object or scalar arriving where a list belongs read as a complete EMPTY collection whenever the declared total was 0. Now refused, quoting the body. A genuinely empty array is still fine — covered. Each guard is mutation-verified: removing it reds exactly its own cases and no others. Refs #188	2026-08-02 19:07:32 +00:00
cluade-reviewer-andresmgsl	87b088114a	fix(test): silence the two lint classes the new backend suite introduces ... SC2016 on the deliberate single-quoted bash -c (the expansion belongs to the isolated process, as the sibling case in issueflow-reconcile.test.sh already documents), and SC2317 on the curl stub, which shellcheck cannot see is invoked indirectly by forge_api. Found only after committing, because .github/scripts/shellcheck-all.sh derives its lint set from `git ls-files` — an UNTRACKED file is not linted at all. "Gates clean" measured before `git add` was measuring a set that excluded the file just written. Verified from a clean clone at the pushed SHA, which is what caught it. Refs #188	2026-08-02 19:03:40 +00:00
cluade-reviewer-andresmgsl	ab23a3b1b6	feat(forge): two backends behind one call surface, and the shim owns paging ... Term 1's foundation. lib/forge.sh gains forge_select, which sources exactly one of lib/forge-github.sh or lib/forge-forgejo.sh; both define the same verbs, so no branching reaches the 61 call sites. The github backend is the current gh invocation extracted 1:1 — term 5 is kept by making that path boring. The page size moves OUT of the call sites and into the backend, because it is not portable and fails silently. Measured 2026-08-02: ?per_page=100 GitHub 100 items Forgejo 30 items (ignored) ?limit=100 GitHub 30 items Forgejo 50 items (capped) Both answer HTTP 200 with valid JSON. Every call site here is GitHub-shaped, so a verbatim port would have swept 30 of rig's 137 issues and printed "reconciled." — criterion 2 failing green, the same failure class as the blind sweep. Both page_url helpers strip a stray page-size parameter in either dialect, so a call site cannot reintroduce it by accident. Forgejo caps a page at 50 whatever is asked, so pagination is mandatory, not an optimisation. The gather is then PROVEN complete against x-total-count rather than assumed complete because a loop ended. @kimi-reviewer-andresmgsl's hardening (#4699): a missing x-total-count is itself a loud refusal. Header exposure is a server setting, and an assert that cannot run must not silently pass — that is the failure class re-entering through the guard built to stop it. Call sites are not ported yet; that is the next commit. Refs #188	2026-08-02 19:00:58 +00:00