j4k-oss/review-wrapper
Watch
2
0
Fork
You've already forked review-wrapper
 0
Code Issues 12 Pull requests Releases Packages Activity Actions

feat: attach to an earlier review of the same diff on events #29

Merged
jercik merged 2 commits from feat/same-diff-attach into main 2026-10-05 09:24:03 +00:00
Conversation 8 Commits 2 Files changed 13 +830 -114
jercik commented 2026-10-04 19:23:19 +00:00
Owner
Copy link

Events now send attach_same_diff: true, so a rebase that leaves the PR's diff unchanged reuses the service's earlier review instead of starting a new one. A manual workflow_dispatch sends false and still starts fresh.

Reuse lets an earlier review return after a later one marked its threads superseded. The reconcilers counted those threads as posted, leaving the returning review's findings with no live thread; they now repost them.

A 422 diff-unreadable from the service now ends the run with its own outcome and a re-run remedy.

Merge after j4k/review#142. Enrolled repos see no change until the align pin moves. The local attach_same_diff type in src/review/api.ts breaks the build once a published @j4k/review declares the field, so it goes with that bump.

A run cancelled between labelling an old thread superseded and resolving it can leave that thread open beside the fresh one.

🤖 Generated with Claude Code

Events now send `attach_same_diff: true`, so a rebase that leaves the PR's diff unchanged reuses the service's earlier review instead of starting a new one. A manual `workflow_dispatch` sends `false` and still starts fresh. Reuse lets an earlier review return after a later one marked its threads superseded. The reconcilers counted those threads as posted, leaving the returning review's findings with no live thread; they now repost them. A 422 `diff-unreadable` from the service now ends the run with its own outcome and a re-run remedy. Merge after [j4k/review#142](https://code.j4k.dev/j4k/review/pulls/142). Enrolled repos see no change until the align pin moves. The local `attach_same_diff` type in `src/review/api.ts` breaks the build once a published `@j4k/review` declares the field, so it goes with that bump. A run cancelled between labelling an old thread superseded and resolving it can leave that thread open beside the fresh one. 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
An event run now sends attach_same_diff: true with its create request, so the
service may return a review it already ran for the same code diff instead of
starting a new one. A workflow_dispatch run sends false: it is the explicit
rerun and must run on the current tree.

When the returned review was created for another commit, the job log names the
review and that commit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fix: repost a returning review's findings instead of trusting superseded threads
All checks were successful
commit-msg / commitlint (pull_request) Successful in 24s
Details
Checks / quality-checks (pull_request) Successful in 47s
Details
Review / Review (pull_request_target) Successful in 6m37s
Details
49f287c316
 
A thread labelled superseded is resolved with no way back. When review A
returned after review B (A, B, A), the inline reconciler still counted A's
superseded threads as posted, so A's findings never reappeared as live
threads, and the disposition projector projected onto the dead ones.

The inline reconciler now counts a claim as posted only when a conversation
without the superseded label carries its marker, so a returning review posts
fresh threads. The disposition projector skips superseded conversations of
the current review; the fresh thread carries its dispositions.

Tests drive both reconcilers through A, B, A sequences against an in-memory
forge: a rebased head, seven alternations, a run cancelled mid-reconcile, a
dismissed claim, two claims on one line, and a first run cancelled before
posting. Six of the seven scenarios fail against the previous reconcilers, as
do the two unit tests; the scenario of a first run cancelled before posting
passes against them and guards a regression.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
forgejo-actions commented 2026-10-04 19:29:55 +00:00
Copy link

Review 01M445XX16DSK1ZPJXFRG8AK47 — head 49f287c3161debaa4069d03fbcdaa0fdcaea5abc

Review — j4k-oss/review-wrapper @ 47c7ab2f66

Scope: diff against base tree e69943819640
Status: dispatched — coverage complete (5/5 slots terminal)
Facts: current review-wide projection

Computed under:

{
  "abandonment": "abandonment-v1",
  "anchor_recipe": 1,
  "batch_policy": "batch-v1",
  "coverage": "coverage-v3",
  "dispatch_policy": "dispatch-v2",
  "grounder_version": 1,
  "grounding_read_rule": "grounding-read-v1",
  "promotion_policy": "promotion-v1",
  "report": "report-v4",
  "tally": "tally-v1",
  "triage_settle": "triage-settle-v2"
}

Findings (5)

medium — README outcomes table copies every member of OUTCOMES and its exit code from OUTCOME_EXIT

  • claim: 01M4460WHPR42RVC894XYHHR2Q
  • anchor: README.md (snippet)
| 13  | `diff-unreadable`     | No comment — the service could not read the PR's diff from the forge. Names the re-run remedy.                                                                | 1    |
  • lens: restated-sets · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M4464JEE7KQN5XV4FYGXWQYE · valid: Grounded row 13 confirms that the README table restates outcomes with exit codes. The body names the sources (OUTCOMES and OUTCOME_EXIT in src/contract/types.ts) and lists every member and exit code. The table covers the same twelve outcomes plus row 11, which the table itself labels as a flag, not an outcome, so no outcome and no exit code is mismatched. Severity is medium. No exception applies. The README is not generated or read by code, the outcome set is this repository's own, the table is not a dated record, and it is not a table of contents for choosing what to open. Its reader can open the source, so the sandboxed-reader exception does not apply. The correction follows the guideline: point to the sources, move per-outcome detail into each OUTCOMES entry's comment, and keep in prose only the row-11 and head-moved detail that no member definition can hold.
  • disposition: none

The Outcomes and exit codes table is a second, hand-maintained copy of the wrapper's outcome set and exit mapping, so every new outcome needs a README row too; this change had to add row 13 for diff-unreadable by hand, and an outcome or exit code changed only in code leaves the table wrong with nothing to flag it.

The table lists each outcome with its exit code: fork-skip 0, service-unreachable 1, auth-failed 1, forge-host-mismatch 1, ask-conflict 1, stuck 1, partial-coverage 1, grounding-pending 1, clean-zero-findings 0, clean-findings 0, row 11 superseded —, head-moved 0, diff-unreadable 1. What the wrapper reads is OUTCOMES and OUTCOME_EXIT in src/contract/types.ts, whose entries already carry per-row behaviour comments such as "diff-unreadable", // row 13: red, NO comment, name the re-run remedy; the remedies live in FAILURE_REMEDIES in src/wrapper/orchestrator.ts.

Source members (OUTCOMES): the twelve outcomes above other than row 11. Copy members: the same twelve, plus row 11, which the table itself labels "A flag, not an outcome". Every exit code in the table matches OUTCOME_EXIT. No outcome appears in only one list, so the copy still agrees. No exception applies: the README is not read or generated from by the code; the set is chosen by this repository, not a third party; the table is not a table of contents or a dated record; and a reader configuring the action can open src/contract/types.ts in the same repository.

Correction: replace the table with a pointer, e.g. "The outcomes and their exit codes are OUTCOMES and OUTCOME_EXIT in src/contract/types.ts; failure remedies are FAILURE_REMEDIES in src/wrapper/orchestrator.ts." Move the per-outcome behaviour the array cannot show (the job-log prefixes for stuck and partial-coverage, "no /v1 call precedes it" for fork-skip) into the comment on that member's OUTCOMES entry, and keep in prose only the detail with no member definition to hold it: that supersession is a flag on WaitResult that never changes the exit code, and the head-moved timing paragraph.

Examined: the README section, OUTCOMES/OUTCOME_EXIT/WaitResult in src/contract/types.ts, FAILURE_REMEDIES in src/wrapper/orchestrator.ts, and the diff hunk adding row 13.

medium — README paragraph enumerates the HTTP 422 problem types toFailure classifies, framed as the complete set

  • claim: 01M44617NH6GQ6P74MFWDZT4RV
  • anchor: README.md (snippet)
HTTP 422 with problem type `/problems/forge-host-mismatch` receives the forge-host
remedy, and `/problems/diff-unreadable` (the service could not read the PR's diff from
the forge) receives a re-run remedy; the wrapper retries neither. Unclassified service
  • lens: restated-sets · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M4464JEE7KQN5XV4FYGXWQYE · valid: The grounded excerpt names both 422 problem types and says 'the wrapper retries neither', followed by 'Unclassified service rejections...'. That framing presents the two types as the complete classified set. The body names the source (the status === 422 && problem.type branches of toFailure in src/review/session.ts) and lists its two members. They match the copy, so this is medium. The third-party exception does not apply. The service defines the problem-type vocabulary, but the README describes which types the wrapper chooses to classify, and that is a selection the subject makes. The correction points to toFailure and lists no members, so it does not restate the set.
  • disposition: none

The paragraph presents its two problem types as everything the wrapper classifies at HTTP 422 ("the wrapper retries neither. Unclassified service rejections fail with exit 1 ..."), so the next /problems/... branch added to toFailure leaves the README telling readers that type is unclassified and logged raw. This change already had to rewrite the sentence (from "Only HTTP 422 with problem type /problems/forge-host-mismatch receives the forge-host remedy") to add the second member.

The set is defined by the status === 422 && error.problem?.type === ... branches of toFailure in src/review/session.ts. Source members: /problems/forge-host-mismatch → forge-host-mismatch, /problems/diff-unreadable → diff-unreadable. Copy members: the same two, with "neither" asserting the count. No member appears in only one list, so the copy still agrees. No exception applies: the README is not read by the code, the problem types the wrapper maps are this repository's selection rather than a third party's set, and the "Unclassified" contrast plus "neither" make the list read as complete rather than as examples. (The including unknown-tree, unknown-commit, and HTTP 413 tree-too-large list later in the same paragraph is framed as examples and is not part of this claim.)

Correction: point to the classifier instead of listing it, e.g. "HTTP 422 problem types that toFailure in src/review/session.ts maps to an outcome fail with that outcome's remedy, without retry; any other service rejection fails with exit 1 and retains the HTTP status, problem type, title, and detail in the job log ...". Each mapped type's remedy already lives in FAILURE_REMEDIES in src/wrapper/orchestrator.ts, and the outcomes table rows 4 and 13 already describe both outcomes.

Examined: toFailure in src/review/session.ts (all status branches: 401/403, the two 422 problem types, 409, 404), FAILURE_REMEDIES in src/wrapper/orchestrator.ts, and the diff hunk rewriting this paragraph.

medium — Outcomes header comment re-lists the table rows that OUTCOMES defines directly below it

  • claim: 01M4460EPAG60AS8PW0MQ03K9H
  • anchor: src/contract/types.ts (snippet)
// ---- Outcomes: the §7 failure/exit table (rows 1–10, 12, 13; row 11 is the
  • lens: restated-sets · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M4464JEE7KQN5XV4FYGXWQYE · valid: Grounded exact anchor shows the header enumerating 'rows 1–10, 12, 13'. The body names the source (OUTCOMES in src/contract/types.ts, directly below) and lists its members by row comment (1–10, 12, 13), compares them with the copy, and finds no mismatch. So the copy agrees, which makes this medium, not high. No exception applies: a code comment is not read by the system, not a dated record, and the list claims to be the whole set. Because the comment describes the adjacent array, the guideline's correction is to delete the enumeration. The proposed fix does that and keeps only what the array cannot show, the note about the excluded row 11. Medium is correct.
  • duplicates: 01M4460KEGVT32PYF5ZKJ9MDKW (writing-quality)
  • disposition: none

The section header enumerates which failure-table rows the outcome list holds, so every new outcome must also edit this comment; the diff had to change it from rows 1–10, 12 to rows 1–10, 12, 13 when it added diff-unreadable, and a future row added only to the array leaves the header silently wrong.

The header reads // ---- Outcomes: the §7 failure/exit table (rows 1–10, 12, 13; row 11 is the superseded flag on WaitResult — it never changes the exit code, which is whatever the pinned-pass outcome maps to) ----. The OUTCOMES array immediately below defines the set, and each entry already carries its own // row N: comment.

Members of OUTCOMES (by row comment): 1 fork-skip, 2 service-unreachable, 3 auth-failed, 4 forge-host-mismatch, 5 ask-conflict, 6 stuck, 7 partial-coverage, 8 grounding-pending, 9 clean-zero-findings, 10 clean-findings, 12 head-moved, 13 diff-unreadable. The copy covers rows 1–10, 12, 13. No member appears in only one list, so the copy still agrees. No exception applies: the comment is not read by the system, not a dated record, and the row list claims to be the whole set rather than examples.

Because the comment only describes the array next to it, delete the row enumeration and keep only the detail the array cannot show, about the excluded row 11: // ---- Outcomes: the §7 failure/exit table; row 11 is WaitResult.superseded, which never changes the exit code ----. The reason supersession cannot change the exit is already documented on WaitResult.superseded in the same file (Supersession alone cannot change the exit: ...).

Examined: src/contract/types.ts header, OUTCOMES, OUTCOME_EXIT, and WaitResult; the diff hunk that edited the header.

low — README restates the diff-unreadable meaning that table row 13 directly above already gives

  • claim: 01M4461DCY4ADTWA9YKT5XCVSC
  • anchor: README.md (snippet)
remedy, and `/problems/diff-unreadable` (the service could not read the PR's diff from
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M4464JEE7KQN5XV4FYGXWQYE · valid: Both sides are grounded. Row 13 (anchor of 01M4460WHPR42RVC894XYHHR2Q) reads 'the service could not read the PR's diff from the forge', and the paragraph's parenthetical repeats that clause word for word a few lines later. Under the writing skill's 'One Idea, One Place', the duplicate should go. Deleting it loses nothing, because row 13 keeps the meaning, and the problem-type mapping and the no-retry rule remain. The paragraph also leaves forge-host-mismatch unexplained in the same way, so the result reads consistently. Low is appropriate.
  • disposition: none

In the paragraph below the outcomes table, the parenthetical gives the meaning of diff-unreadable again. Row 13 of the table, a few lines up, already defines it: | 13 | \diff-unreadable` | No comment — the service could not read the PR's diff from the forge. Names the re-run remedy. | 1 |`. The parenthetical "(the service could not read the PR's diff from the forge)" repeats that clause word for word. Readers read it twice, and if the outcome's description ever changes, both places have to be edited.

The writing skill's "One Idea, One Place" section says not to restate what an earlier sentence already says. The paragraph's new information is how the problem type maps to the outcome and that the wrapper does not retry it. It treats forge-host-mismatch the same way and does not explain that one either.

Correction: delete the parenthetical, giving: "HTTP 422 with problem type /problems/forge-host-mismatch receives the forge-host remedy, and /problems/diff-unreadable receives a re-run remedy; the wrapper retries neither." This keeps the problem-type mapping and the no-retry rule, and the meaning stays in row 13.

I read the README's "Outcomes and exit codes" section in full at the reviewed revision.

low — Comment calls a superseded thread "closed", but the check is the label alone and a labelled thread can stay open

  • claim: 01M4461DPCHEBNQQMHCR63NNKR
  • anchor: src/reconcile/inline.ts (snippet)
// A superseded thread was closed for a later Review, with no way back, so it no longer
  • lens: writing-quality · arm: default
  • verdicts: 1 valid / 0 invalid / 0 uncertain
    • pass 01M4464JEE7KQN5XV4FYGXWQYE · valid: The grounded comment says a superseded thread 'was closed'. The reviewer's trace is specific and coherent. postedClaimIds skips a conversation when isSuperseded is true. isSuperseded only checks for a '<!-- review:superseded:' marker comment. The scenario-7 test expects a labelled but open thread ('headA claim+superseded open') followed by a fresh post. In review-thread vocabulary, 'closed' plausibly reads as resolved, which contradicts the label-only rule and the open state the test pins. A maintainer could then switch the check to resolution state and break that case. The correction describes the label and keeps the A, B, A reasoning, so it loses nothing. Low is appropriate for a misleading comment.
  • disposition: none

The new comment says a superseded thread "was closed". The code it explains decides on the label alone, and a labelled thread can still be open. A maintainer who trusts the comment might change postedClaimIds to test resolution state (resolver !== null). That change would count a labelled but open thread as live, and the returning claim would not be reposted. The new test scenario 7: a thread left labelled and open by a cancelled run stays, and a live one is posted covers exactly this state. It expects "headA claim+superseded open" followed by a fresh "headA claim open".

postedClaimIds skips a conversation when isSuperseded(conversation) is true. In conversation-markers.ts, isSuperseded only checks for a comment that starts with <!-- review:superseded:. The README uses the same rule: "A thread labelled superseded is final, so it does not count as a posted finding."

Correction: describe the label, not the resolution. For example: "A thread labelled superseded is final, even if a cancelled run left it open, so it no longer shows its claim: a Review that returns to the head (A, B, A) must post the claim afresh." This keeps the reason for reposting and the A, B, A example, and states the open-thread case the test pins.

I read postedClaimIds in src/reconcile/inline.ts, isSuperseded and SUPERSEDED_MARKER_PATTERN in src/reconcile/conversation-markers.ts, and the scenario 7 test in src/reconcile/returning-review.test.ts. This is a static trace; I did not run the tests.

Other claims

  • grounding-pending (0)
  • ungrounded (0)
  • rejected (0)
  • duplicate-of (1)
    • 01M4460KEGVT32PYF5ZKJ9MDKW medium — Outcomes header comment restates the row set that the OUTCOMES array directly below already defines → 01M4460EPAG60AS8PW0MQ03K9H
  • unadjudicated (1)
    • 01M4460ZCR7EHPVD63SMPTH5SS low — Test names carry "scenario N" numbers (1, 2, 4, 7, 10–12) from a list nothing in the repository defines

Coverage

Coverage pass: 01M445XX2WEGF4M429DDMPW1B3
Accounting: complete
Slot health: healthy

lens part arm unit status runs loss
general-bug whole default no-claims 1 no
writing-quality whole default claims-emitted 1 no
test-trimming whole default no-claims 1 no
restated-sets whole default claims-emitted 1 no
project-docs whole default no-claims 1 no
<!-- review:summary --> **Review** `01M445XX16DSK1ZPJXFRG8AK47` — head `49f287c3161debaa4069d03fbcdaa0fdcaea5abc` # Review — j4k-oss/review-wrapper @ 47c7ab2f663e Scope: diff against base tree `e69943819640` Status: dispatched — coverage complete (5/5 slots terminal) Facts: current review-wide projection Computed under: ```json { "abandonment": "abandonment-v1", "anchor_recipe": 1, "batch_policy": "batch-v1", "coverage": "coverage-v3", "dispatch_policy": "dispatch-v2", "grounder_version": 1, "grounding_read_rule": "grounding-read-v1", "promotion_policy": "promotion-v1", "report": "report-v4", "tally": "tally-v1", "triage_settle": "triage-settle-v2" } ``` ## Findings (5) ### medium — README outcomes table copies every member of `OUTCOMES` and its exit code from `OUTCOME_EXIT` - claim: `01M4460WHPR42RVC894XYHHR2Q` - anchor: `README.md` (snippet) ``` | 13 | `diff-unreadable` | No comment — the service could not read the PR's diff from the forge. Names the re-run remedy. | 1 | ``` - lens: restated-sets · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M4464JEE7KQN5XV4FYGXWQYE` · valid: Grounded row 13 confirms that the README table restates outcomes with exit codes. The body names the sources (OUTCOMES and OUTCOME_EXIT in src/contract/types.ts) and lists every member and exit code. The table covers the same twelve outcomes plus row 11, which the table itself labels as a flag, not an outcome, so no outcome and no exit code is mismatched. Severity is medium. No exception applies. The README is not generated or read by code, the outcome set is this repository's own, the table is not a dated record, and it is not a table of contents for choosing what to open. Its reader can open the source, so the sandboxed-reader exception does not apply. The correction follows the guideline: point to the sources, move per-outcome detail into each OUTCOMES entry's comment, and keep in prose only the row-11 and head-moved detail that no member definition can hold. - disposition: none > The `Outcomes and exit codes` table is a second, hand-maintained copy of the wrapper's outcome set and exit mapping, so every new outcome needs a README row too; this change had to add row 13 for `diff-unreadable` by hand, and an outcome or exit code changed only in code leaves the table wrong with nothing to flag it. > > The table lists each outcome with its exit code: `fork-skip` 0, `service-unreachable` 1, `auth-failed` 1, `forge-host-mismatch` 1, `ask-conflict` 1, `stuck` 1, `partial-coverage` 1, `grounding-pending` 1, `clean-zero-findings` 0, `clean-findings` 0, row 11 superseded `—`, `head-moved` 0, `diff-unreadable` 1. What the wrapper reads is `OUTCOMES` and `OUTCOME_EXIT` in `src/contract/types.ts`, whose entries already carry per-row behaviour comments such as `"diff-unreadable", // row 13: red, NO comment, name the re-run remedy`; the remedies live in `FAILURE_REMEDIES` in `src/wrapper/orchestrator.ts`. > > Source members (`OUTCOMES`): the twelve outcomes above other than row 11. Copy members: the same twelve, plus row 11, which the table itself labels "A flag, not an outcome". Every exit code in the table matches `OUTCOME_EXIT`. No outcome appears in only one list, so the copy still agrees. No exception applies: the README is not read or generated from by the code; the set is chosen by this repository, not a third party; the table is not a table of contents or a dated record; and a reader configuring the action can open `src/contract/types.ts` in the same repository. > > Correction: replace the table with a pointer, e.g. "The outcomes and their exit codes are `OUTCOMES` and `OUTCOME_EXIT` in `src/contract/types.ts`; failure remedies are `FAILURE_REMEDIES` in `src/wrapper/orchestrator.ts`." Move the per-outcome behaviour the array cannot show (the job-log prefixes for `stuck` and `partial-coverage`, "no `/v1` call precedes it" for `fork-skip`) into the comment on that member's `OUTCOMES` entry, and keep in prose only the detail with no member definition to hold it: that supersession is a flag on `WaitResult` that never changes the exit code, and the `head-moved` timing paragraph. > > Examined: the README section, `OUTCOMES`/`OUTCOME_EXIT`/`WaitResult` in `src/contract/types.ts`, `FAILURE_REMEDIES` in `src/wrapper/orchestrator.ts`, and the diff hunk adding row 13. ### medium — README paragraph enumerates the HTTP 422 problem types `toFailure` classifies, framed as the complete set - claim: `01M44617NH6GQ6P74MFWDZT4RV` - anchor: `README.md` (snippet) ``` HTTP 422 with problem type `/problems/forge-host-mismatch` receives the forge-host remedy, and `/problems/diff-unreadable` (the service could not read the PR's diff from the forge) receives a re-run remedy; the wrapper retries neither. Unclassified service ``` - lens: restated-sets · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M4464JEE7KQN5XV4FYGXWQYE` · valid: The grounded excerpt names both 422 problem types and says 'the wrapper retries neither', followed by 'Unclassified service rejections...'. That framing presents the two types as the complete classified set. The body names the source (the status === 422 && problem.type branches of toFailure in src/review/session.ts) and lists its two members. They match the copy, so this is medium. The third-party exception does not apply. The service defines the problem-type vocabulary, but the README describes which types the wrapper chooses to classify, and that is a selection the subject makes. The correction points to toFailure and lists no members, so it does not restate the set. - disposition: none > The paragraph presents its two problem types as everything the wrapper classifies at HTTP 422 ("the wrapper retries neither. Unclassified service rejections fail with exit 1 ..."), so the next `/problems/...` branch added to `toFailure` leaves the README telling readers that type is unclassified and logged raw. This change already had to rewrite the sentence (from "Only HTTP 422 with problem type `/problems/forge-host-mismatch` receives the forge-host remedy") to add the second member. > > The set is defined by the `status === 422 && error.problem?.type === ...` branches of `toFailure` in `src/review/session.ts`. Source members: `/problems/forge-host-mismatch` → `forge-host-mismatch`, `/problems/diff-unreadable` → `diff-unreadable`. Copy members: the same two, with "neither" asserting the count. No member appears in only one list, so the copy still agrees. No exception applies: the README is not read by the code, the problem types the wrapper maps are this repository's selection rather than a third party's set, and the "Unclassified" contrast plus "neither" make the list read as complete rather than as examples. (The `including unknown-tree, unknown-commit, and HTTP 413 tree-too-large` list later in the same paragraph is framed as examples and is not part of this claim.) > > Correction: point to the classifier instead of listing it, e.g. "HTTP 422 problem types that `toFailure` in `src/review/session.ts` maps to an outcome fail with that outcome's remedy, without retry; any other service rejection fails with exit 1 and retains the HTTP status, problem type, title, and detail in the job log ...". Each mapped type's remedy already lives in `FAILURE_REMEDIES` in `src/wrapper/orchestrator.ts`, and the outcomes table rows 4 and 13 already describe both outcomes. > > Examined: `toFailure` in `src/review/session.ts` (all status branches: 401/403, the two 422 problem types, 409, 404), `FAILURE_REMEDIES` in `src/wrapper/orchestrator.ts`, and the diff hunk rewriting this paragraph. ### medium — Outcomes header comment re-lists the table rows that `OUTCOMES` defines directly below it - claim: `01M4460EPAG60AS8PW0MQ03K9H` - anchor: `src/contract/types.ts` (snippet) ``` // ---- Outcomes: the §7 failure/exit table (rows 1–10, 12, 13; row 11 is the ``` - lens: restated-sets · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M4464JEE7KQN5XV4FYGXWQYE` · valid: Grounded exact anchor shows the header enumerating 'rows 1–10, 12, 13'. The body names the source (OUTCOMES in src/contract/types.ts, directly below) and lists its members by row comment (1–10, 12, 13), compares them with the copy, and finds no mismatch. So the copy agrees, which makes this medium, not high. No exception applies: a code comment is not read by the system, not a dated record, and the list claims to be the whole set. Because the comment describes the adjacent array, the guideline's correction is to delete the enumeration. The proposed fix does that and keeps only what the array cannot show, the note about the excluded row 11. Medium is correct. - duplicates: `01M4460KEGVT32PYF5ZKJ9MDKW` (writing-quality) - disposition: none > The section header enumerates which failure-table rows the outcome list holds, so every new outcome must also edit this comment; the diff had to change it from `rows 1–10, 12` to `rows 1–10, 12, 13` when it added `diff-unreadable`, and a future row added only to the array leaves the header silently wrong. > > The header reads `// ---- Outcomes: the §7 failure/exit table (rows 1–10, 12, 13; row 11 is the superseded flag on WaitResult — it never changes the exit code, which is whatever the pinned-pass outcome maps to) ----`. The `OUTCOMES` array immediately below defines the set, and each entry already carries its own `// row N:` comment. > > Members of `OUTCOMES` (by row comment): 1 `fork-skip`, 2 `service-unreachable`, 3 `auth-failed`, 4 `forge-host-mismatch`, 5 `ask-conflict`, 6 `stuck`, 7 `partial-coverage`, 8 `grounding-pending`, 9 `clean-zero-findings`, 10 `clean-findings`, 12 `head-moved`, 13 `diff-unreadable`. The copy covers rows 1–10, 12, 13. No member appears in only one list, so the copy still agrees. No exception applies: the comment is not read by the system, not a dated record, and the row list claims to be the whole set rather than examples. > > Because the comment only describes the array next to it, delete the row enumeration and keep only the detail the array cannot show, about the excluded row 11: `// ---- Outcomes: the §7 failure/exit table; row 11 is WaitResult.superseded, which never changes the exit code ----`. The reason supersession cannot change the exit is already documented on `WaitResult.superseded` in the same file (`Supersession alone cannot change the exit: ...`). > > Examined: `src/contract/types.ts` header, `OUTCOMES`, `OUTCOME_EXIT`, and `WaitResult`; the diff hunk that edited the header. ### low — README restates the `diff-unreadable` meaning that table row 13 directly above already gives - claim: `01M4461DCY4ADTWA9YKT5XCVSC` - anchor: `README.md` (snippet) ``` remedy, and `/problems/diff-unreadable` (the service could not read the PR's diff from ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M4464JEE7KQN5XV4FYGXWQYE` · valid: Both sides are grounded. Row 13 (anchor of 01M4460WHPR42RVC894XYHHR2Q) reads 'the service could not read the PR's diff from the forge', and the paragraph's parenthetical repeats that clause word for word a few lines later. Under the writing skill's 'One Idea, One Place', the duplicate should go. Deleting it loses nothing, because row 13 keeps the meaning, and the problem-type mapping and the no-retry rule remain. The paragraph also leaves forge-host-mismatch unexplained in the same way, so the result reads consistently. Low is appropriate. - disposition: none > In the paragraph below the outcomes table, the parenthetical gives the meaning of `diff-unreadable` again. Row 13 of the table, a few lines up, already defines it: `| 13 | \`diff-unreadable\` | No comment — the service could not read the PR's diff from the forge. Names the re-run remedy. | 1 |`. The parenthetical "(the service could not read the PR's diff from the forge)" repeats that clause word for word. Readers read it twice, and if the outcome's description ever changes, both places have to be edited. > > The writing skill's "One Idea, One Place" section says not to restate what an earlier sentence already says. The paragraph's new information is how the problem type maps to the outcome and that the wrapper does not retry it. It treats `forge-host-mismatch` the same way and does not explain that one either. > > Correction: delete the parenthetical, giving: "HTTP 422 with problem type `/problems/forge-host-mismatch` receives the forge-host remedy, and `/problems/diff-unreadable` receives a re-run remedy; the wrapper retries neither." This keeps the problem-type mapping and the no-retry rule, and the meaning stays in row 13. > > I read the README's "Outcomes and exit codes" section in full at the reviewed revision. ### low — Comment calls a superseded thread "closed", but the check is the label alone and a labelled thread can stay open - claim: `01M4461DPCHEBNQQMHCR63NNKR` - anchor: `src/reconcile/inline.ts` (snippet) ``` // A superseded thread was closed for a later Review, with no way back, so it no longer ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M4464JEE7KQN5XV4FYGXWQYE` · valid: The grounded comment says a superseded thread 'was closed'. The reviewer's trace is specific and coherent. postedClaimIds skips a conversation when isSuperseded is true. isSuperseded only checks for a '&lt;!-- review:superseded:' marker comment. The scenario-7 test expects a labelled but open thread ('headA claim+superseded open') followed by a fresh post. In review-thread vocabulary, 'closed' plausibly reads as resolved, which contradicts the label-only rule and the open state the test pins. A maintainer could then switch the check to resolution state and break that case. The correction describes the label and keeps the A, B, A reasoning, so it loses nothing. Low is appropriate for a misleading comment. - disposition: none > The new comment says a superseded thread "was closed". The code it explains decides on the label alone, and a labelled thread can still be open. A maintainer who trusts the comment might change `postedClaimIds` to test resolution state (`resolver !== null`). That change would count a labelled but open thread as live, and the returning claim would not be reposted. The new test `scenario 7: a thread left labelled and open by a cancelled run stays, and a live one is posted` covers exactly this state. It expects `"headA claim+superseded open"` followed by a fresh `"headA claim open"`. > > `postedClaimIds` skips a conversation when `isSuperseded(conversation)` is true. In `conversation-markers.ts`, `isSuperseded` only checks for a comment that starts with <code>&lt;&#33;-- review:superseded:</code>. The README uses the same rule: "A thread labelled superseded is final, so it does not count as a posted finding." > > Correction: describe the label, not the resolution. For example: "A thread labelled superseded is final, even if a cancelled run left it open, so it no longer shows its claim: a Review that returns to the head (A, B, A) must post the claim afresh." This keeps the reason for reposting and the A, B, A example, and states the open-thread case the test pins. > > I read `postedClaimIds` in `src/reconcile/inline.ts`, `isSuperseded` and `SUPERSEDED_MARKER_PATTERN` in `src/reconcile/conversation-markers.ts`, and the scenario 7 test in `src/reconcile/returning-review.test.ts`. This is a static trace; I did not run the tests. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (1) - `01M4460KEGVT32PYF5ZKJ9MDKW` medium — Outcomes header comment restates the row set that the `OUTCOMES` array directly below already defines → `01M4460EPAG60AS8PW0MQ03K9H` - unadjudicated (1) - `01M4460ZCR7EHPVD63SMPTH5SS` low — Test names carry "scenario N" numbers (1, 2, 4, 7, 10–12) from a list nothing in the repository defines ## Coverage Coverage pass: 01M445XX2WEGF4M429DDMPW1B3 Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | no-claims | 1 | no | | writing-quality | whole | default | claims-emitted | 1 | no | | test-trimming | whole | default | no-claims | 1 | no | | restated-sets | whole | default | claims-emitted | 1 | no | | project-docs | whole | default | no-claims | 1 | no |
🚀 1
forgejo-actions reviewed 2026-10-04 19:29:55 +00:00
README.md
@ -84,16 +84,19 @@ the secret.
| 10 | `clean-findings` | Full reconcile. | 0 |
| 11 | superseded | A flag, not an outcome; it does not itself change the exit code. Coverage and report accounting stay pinned; triage, claims, and findings remain review-wide. | — |
| 12 | `head-moved` | No PR comment or conversation-resolution writes; the newer run owns the comment surface. See the timing below. | 0 |
| 13 | `diff-unreadable` | No comment — the service could not read the PR's diff from the forge. Names the re-run remedy. | 1 |
forgejo-actions commented 2026-10-04 19:29:55 +00:00
Copy link

medium — README outcomes table copies every member of OUTCOMES and its exit code from OUTCOME_EXIT

The Outcomes and exit codes table is a second, hand-maintained copy of the wrapper's outcome set and exit mapping, so every new outcome needs a README row too; this change had to add row 13 for diff-unreadable by hand, and an outcome or exit code changed only in code leaves the table wrong with nothing to flag it.

The table lists each outcome with its exit code: fork-skip 0, service-unreachable 1, auth-failed 1, forge-host-mismatch 1, ask-conflict 1, stuck 1, partial-coverage 1, grounding-pending 1, clean-zero-findings 0, clean-findings 0, row 11 superseded —, head-moved 0, diff-unreadable 1. What the wrapper reads is OUTCOMES and OUTCOME_EXIT in src/contract/types.ts, whose entries already carry per-row behaviour comments such as "diff-unreadable", // row 13: red, NO comment, name the re-run remedy; the remedies live in FAILURE_REMEDIES in src/wrapper/orchestrator.ts.

Source members (OUTCOMES): the twelve outcomes above other than row 11. Copy members: the same twelve, plus row 11, which the table itself labels "A flag, not an outcome". Every exit code in the table matches OUTCOME_EXIT. No outcome appears in only one list, so the copy still agrees. No exception applies: the README is not read or generated from by the code; the set is chosen by this repository, not a third party; the table is not a table of contents or a dated record; and a reader configuring the action can open src/contract/types.ts in the same repository.

Correction: replace the table with a pointer, e.g. "The outcomes and their exit codes are OUTCOMES and OUTCOME_EXIT in src/contract/types.ts; failure remedies are FAILURE_REMEDIES in src/wrapper/orchestrator.ts." Move the per-outcome behaviour the array cannot show (the job-log prefixes for stuck and partial-coverage, "no /v1 call precedes it" for fork-skip) into the comment on that member's OUTCOMES entry, and keep in prose only the detail with no member definition to hold it: that supersession is a flag on WaitResult that never changes the exit code, and the head-moved timing paragraph.

Examined: the README section, OUTCOMES/OUTCOME_EXIT/WaitResult in src/contract/types.ts, FAILURE_REMEDIES in src/wrapper/orchestrator.ts, and the diff hunk adding row 13.

lens restated-sets · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M4460WHPR42RVC894XYHHR2Q of review 01M445XX16DSK1ZPJXFRG8AK47

<!-- review:claim:01M4460WHPR42RVC894XYHHR2Q --> **medium** — README outcomes table copies every member of `OUTCOMES` and its exit code from `OUTCOME_EXIT` > The `Outcomes and exit codes` table is a second, hand-maintained copy of the wrapper's outcome set and exit mapping, so every new outcome needs a README row too; this change had to add row 13 for `diff-unreadable` by hand, and an outcome or exit code changed only in code leaves the table wrong with nothing to flag it. > > The table lists each outcome with its exit code: `fork-skip` 0, `service-unreachable` 1, `auth-failed` 1, `forge-host-mismatch` 1, `ask-conflict` 1, `stuck` 1, `partial-coverage` 1, `grounding-pending` 1, `clean-zero-findings` 0, `clean-findings` 0, row 11 superseded `—`, `head-moved` 0, `diff-unreadable` 1. What the wrapper reads is `OUTCOMES` and `OUTCOME_EXIT` in `src/contract/types.ts`, whose entries already carry per-row behaviour comments such as `"diff-unreadable", // row 13: red, NO comment, name the re-run remedy`; the remedies live in `FAILURE_REMEDIES` in `src/wrapper/orchestrator.ts`. > > Source members (`OUTCOMES`): the twelve outcomes above other than row 11. Copy members: the same twelve, plus row 11, which the table itself labels "A flag, not an outcome". Every exit code in the table matches `OUTCOME_EXIT`. No outcome appears in only one list, so the copy still agrees. No exception applies: the README is not read or generated from by the code; the set is chosen by this repository, not a third party; the table is not a table of contents or a dated record; and a reader configuring the action can open `src/contract/types.ts` in the same repository. > > Correction: replace the table with a pointer, e.g. "The outcomes and their exit codes are `OUTCOMES` and `OUTCOME_EXIT` in `src/contract/types.ts`; failure remedies are `FAILURE_REMEDIES` in `src/wrapper/orchestrator.ts`." Move the per-outcome behaviour the array cannot show (the job-log prefixes for `stuck` and `partial-coverage`, "no `/v1` call precedes it" for `fork-skip`) into the comment on that member's `OUTCOMES` entry, and keep in prose only the detail with no member definition to hold it: that supersession is a flag on `WaitResult` that never changes the exit code, and the `head-moved` timing paragraph. > > Examined: the README section, `OUTCOMES`/`OUTCOME_EXIT`/`WaitResult` in `src/contract/types.ts`, `FAILURE_REMEDIES` in `src/wrapper/orchestrator.ts`, and the diff hunk adding row 13. lens `restated-sets` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M4460WHPR42RVC894XYHHR2Q` of review `01M445XX16DSK1ZPJXFRG8AK47`
🚀 1
jercik marked this conversation as resolved
README.md
@ -95,2 +93,2 @@
`unknown-commit`, and HTTP 413 `tree-too-large`. Unparseable error responses retain
the HTTP status and raw body. A rejection before reconciliation writes no comments.
HTTP 422 with problem type `/problems/forge-host-mismatch` receives the forge-host
remedy, and `/problems/diff-unreadable` (the service could not read the PR's diff from
forgejo-actions commented 2026-10-04 19:29:55 +00:00
Copy link

low — README restates the diff-unreadable meaning that table row 13 directly above already gives

In the paragraph below the outcomes table, the parenthetical gives the meaning of diff-unreadable again. Row 13 of the table, a few lines up, already defines it: | 13 | \diff-unreadable` | No comment — the service could not read the PR's diff from the forge. Names the re-run remedy. | 1 |`. The parenthetical "(the service could not read the PR's diff from the forge)" repeats that clause word for word. Readers read it twice, and if the outcome's description ever changes, both places have to be edited.

The writing skill's "One Idea, One Place" section says not to restate what an earlier sentence already says. The paragraph's new information is how the problem type maps to the outcome and that the wrapper does not retry it. It treats forge-host-mismatch the same way and does not explain that one either.

Correction: delete the parenthetical, giving: "HTTP 422 with problem type /problems/forge-host-mismatch receives the forge-host remedy, and /problems/diff-unreadable receives a re-run remedy; the wrapper retries neither." This keeps the problem-type mapping and the no-retry rule, and the meaning stays in row 13.

I read the README's "Outcomes and exit codes" section in full at the reviewed revision.

lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M4461DCY4ADTWA9YKT5XCVSC of review 01M445XX16DSK1ZPJXFRG8AK47

<!-- review:claim:01M4461DCY4ADTWA9YKT5XCVSC --> **low** — README restates the `diff-unreadable` meaning that table row 13 directly above already gives > In the paragraph below the outcomes table, the parenthetical gives the meaning of `diff-unreadable` again. Row 13 of the table, a few lines up, already defines it: `| 13 | \`diff-unreadable\` | No comment — the service could not read the PR's diff from the forge. Names the re-run remedy. | 1 |`. The parenthetical "(the service could not read the PR's diff from the forge)" repeats that clause word for word. Readers read it twice, and if the outcome's description ever changes, both places have to be edited. > > The writing skill's "One Idea, One Place" section says not to restate what an earlier sentence already says. The paragraph's new information is how the problem type maps to the outcome and that the wrapper does not retry it. It treats `forge-host-mismatch` the same way and does not explain that one either. > > Correction: delete the parenthetical, giving: "HTTP 422 with problem type `/problems/forge-host-mismatch` receives the forge-host remedy, and `/problems/diff-unreadable` receives a re-run remedy; the wrapper retries neither." This keeps the problem-type mapping and the no-retry rule, and the meaning stays in row 13. > > I read the README's "Outcomes and exit codes" section in full at the reviewed revision. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M4461DCY4ADTWA9YKT5XCVSC` of review `01M445XX16DSK1ZPJXFRG8AK47`
🚀 1
jercik marked this conversation as resolved
README.md
Lines 93-95
@ -94,3 +93,3 @@
HTTP status, problem type, title, and detail in the job log, including `unknown-tree`,
`unknown-commit`, and HTTP 413 `tree-too-large`. Unparseable error responses retain
the HTTP status and raw body. A rejection before reconciliation writes no comments.
HTTP 422 with problem type `/problems/forge-host-mismatch` receives the forge-host
remedy, and `/problems/diff-unreadable` (the service could not read the PR's diff from
the forge) receives a re-run remedy; the wrapper retries neither. Unclassified service
forgejo-actions commented 2026-10-04 19:29:55 +00:00
Copy link

medium — README paragraph enumerates the HTTP 422 problem types toFailure classifies, framed as the complete set

The paragraph presents its two problem types as everything the wrapper classifies at HTTP 422 ("the wrapper retries neither. Unclassified service rejections fail with exit 1 ..."), so the next /problems/... branch added to toFailure leaves the README telling readers that type is unclassified and logged raw. This change already had to rewrite the sentence (from "Only HTTP 422 with problem type /problems/forge-host-mismatch receives the forge-host remedy") to add the second member.

The set is defined by the status === 422 && error.problem?.type === ... branches of toFailure in src/review/session.ts. Source members: /problems/forge-host-mismatch → forge-host-mismatch, /problems/diff-unreadable → diff-unreadable. Copy members: the same two, with "neither" asserting the count. No member appears in only one list, so the copy still agrees. No exception applies: the README is not read by the code, the problem types the wrapper maps are this repository's selection rather than a third party's set, and the "Unclassified" contrast plus "neither" make the list read as complete rather than as examples. (The including unknown-tree, unknown-commit, and HTTP 413 tree-too-large list later in the same paragraph is framed as examples and is not part of this claim.)

Correction: point to the classifier instead of listing it, e.g. "HTTP 422 problem types that toFailure in src/review/session.ts maps to an outcome fail with that outcome's remedy, without retry; any other service rejection fails with exit 1 and retains the HTTP status, problem type, title, and detail in the job log ...". Each mapped type's remedy already lives in FAILURE_REMEDIES in src/wrapper/orchestrator.ts, and the outcomes table rows 4 and 13 already describe both outcomes.

Examined: toFailure in src/review/session.ts (all status branches: 401/403, the two 422 problem types, 409, 404), FAILURE_REMEDIES in src/wrapper/orchestrator.ts, and the diff hunk rewriting this paragraph.

lens restated-sets · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M44617NH6GQ6P74MFWDZT4RV of review 01M445XX16DSK1ZPJXFRG8AK47

<!-- review:claim:01M44617NH6GQ6P74MFWDZT4RV --> **medium** — README paragraph enumerates the HTTP 422 problem types `toFailure` classifies, framed as the complete set > The paragraph presents its two problem types as everything the wrapper classifies at HTTP 422 ("the wrapper retries neither. Unclassified service rejections fail with exit 1 ..."), so the next `/problems/...` branch added to `toFailure` leaves the README telling readers that type is unclassified and logged raw. This change already had to rewrite the sentence (from "Only HTTP 422 with problem type `/problems/forge-host-mismatch` receives the forge-host remedy") to add the second member. > > The set is defined by the `status === 422 && error.problem?.type === ...` branches of `toFailure` in `src/review/session.ts`. Source members: `/problems/forge-host-mismatch` → `forge-host-mismatch`, `/problems/diff-unreadable` → `diff-unreadable`. Copy members: the same two, with "neither" asserting the count. No member appears in only one list, so the copy still agrees. No exception applies: the README is not read by the code, the problem types the wrapper maps are this repository's selection rather than a third party's set, and the "Unclassified" contrast plus "neither" make the list read as complete rather than as examples. (The `including unknown-tree, unknown-commit, and HTTP 413 tree-too-large` list later in the same paragraph is framed as examples and is not part of this claim.) > > Correction: point to the classifier instead of listing it, e.g. "HTTP 422 problem types that `toFailure` in `src/review/session.ts` maps to an outcome fail with that outcome's remedy, without retry; any other service rejection fails with exit 1 and retains the HTTP status, problem type, title, and detail in the job log ...". Each mapped type's remedy already lives in `FAILURE_REMEDIES` in `src/wrapper/orchestrator.ts`, and the outcomes table rows 4 and 13 already describe both outcomes. > > Examined: `toFailure` in `src/review/session.ts` (all status branches: 401/403, the two 422 problem types, 409, 404), `FAILURE_REMEDIES` in `src/wrapper/orchestrator.ts`, and the diff hunk rewriting this paragraph. lens `restated-sets` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44617NH6GQ6P74MFWDZT4RV` of review `01M445XX16DSK1ZPJXFRG8AK47`
🚀 1
jercik marked this conversation as resolved
src/contract/types.ts
@ -36,3 +36,3 @@
}
// ---- Outcomes: the §7 failure/exit table (rows 1–10, 12; row 11 is the
// ---- Outcomes: the §7 failure/exit table (rows 1–10, 12, 13; row 11 is the
forgejo-actions commented 2026-10-04 19:29:55 +00:00
Copy link

medium — Outcomes header comment re-lists the table rows that OUTCOMES defines directly below it

The section header enumerates which failure-table rows the outcome list holds, so every new outcome must also edit this comment; the diff had to change it from rows 1–10, 12 to rows 1–10, 12, 13 when it added diff-unreadable, and a future row added only to the array leaves the header silently wrong.

The header reads // ---- Outcomes: the §7 failure/exit table (rows 1–10, 12, 13; row 11 is the superseded flag on WaitResult — it never changes the exit code, which is whatever the pinned-pass outcome maps to) ----. The OUTCOMES array immediately below defines the set, and each entry already carries its own // row N: comment.

Members of OUTCOMES (by row comment): 1 fork-skip, 2 service-unreachable, 3 auth-failed, 4 forge-host-mismatch, 5 ask-conflict, 6 stuck, 7 partial-coverage, 8 grounding-pending, 9 clean-zero-findings, 10 clean-findings, 12 head-moved, 13 diff-unreadable. The copy covers rows 1–10, 12, 13. No member appears in only one list, so the copy still agrees. No exception applies: the comment is not read by the system, not a dated record, and the row list claims to be the whole set rather than examples.

Because the comment only describes the array next to it, delete the row enumeration and keep only the detail the array cannot show, about the excluded row 11: // ---- Outcomes: the §7 failure/exit table; row 11 is WaitResult.superseded, which never changes the exit code ----. The reason supersession cannot change the exit is already documented on WaitResult.superseded in the same file (Supersession alone cannot change the exit: ...).

Examined: src/contract/types.ts header, OUTCOMES, OUTCOME_EXIT, and WaitResult; the diff hunk that edited the header.

lens restated-sets · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M4460EPAG60AS8PW0MQ03K9H of review 01M445XX16DSK1ZPJXFRG8AK47

<!-- review:claim:01M4460EPAG60AS8PW0MQ03K9H --> **medium** — Outcomes header comment re-lists the table rows that `OUTCOMES` defines directly below it > The section header enumerates which failure-table rows the outcome list holds, so every new outcome must also edit this comment; the diff had to change it from `rows 1–10, 12` to `rows 1–10, 12, 13` when it added `diff-unreadable`, and a future row added only to the array leaves the header silently wrong. > > The header reads `// ---- Outcomes: the §7 failure/exit table (rows 1–10, 12, 13; row 11 is the superseded flag on WaitResult — it never changes the exit code, which is whatever the pinned-pass outcome maps to) ----`. The `OUTCOMES` array immediately below defines the set, and each entry already carries its own `// row N:` comment. > > Members of `OUTCOMES` (by row comment): 1 `fork-skip`, 2 `service-unreachable`, 3 `auth-failed`, 4 `forge-host-mismatch`, 5 `ask-conflict`, 6 `stuck`, 7 `partial-coverage`, 8 `grounding-pending`, 9 `clean-zero-findings`, 10 `clean-findings`, 12 `head-moved`, 13 `diff-unreadable`. The copy covers rows 1–10, 12, 13. No member appears in only one list, so the copy still agrees. No exception applies: the comment is not read by the system, not a dated record, and the row list claims to be the whole set rather than examples. > > Because the comment only describes the array next to it, delete the row enumeration and keep only the detail the array cannot show, about the excluded row 11: `// ---- Outcomes: the §7 failure/exit table; row 11 is WaitResult.superseded, which never changes the exit code ----`. The reason supersession cannot change the exit is already documented on `WaitResult.superseded` in the same file (`Supersession alone cannot change the exit: ...`). > > Examined: `src/contract/types.ts` header, `OUTCOMES`, `OUTCOME_EXIT`, and `WaitResult`; the diff hunk that edited the header. lens `restated-sets` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M4460EPAG60AS8PW0MQ03K9H` of review `01M445XX16DSK1ZPJXFRG8AK47`
🚀 1
jercik marked this conversation as resolved
src/reconcile/inline.ts
@ -25,3 +27,3 @@
}
async function existingClaimMarkers(forge: ForgeClient, prNumber: number): Promise<string[]> {
// A superseded thread was closed for a later Review, with no way back, so it no longer
forgejo-actions commented 2026-10-04 19:29:55 +00:00
Copy link

low — Comment calls a superseded thread "closed", but the check is the label alone and a labelled thread can stay open

The new comment says a superseded thread "was closed". The code it explains decides on the label alone, and a labelled thread can still be open. A maintainer who trusts the comment might change postedClaimIds to test resolution state (resolver !== null). That change would count a labelled but open thread as live, and the returning claim would not be reposted. The new test scenario 7: a thread left labelled and open by a cancelled run stays, and a live one is posted covers exactly this state. It expects "headA claim+superseded open" followed by a fresh "headA claim open".

postedClaimIds skips a conversation when isSuperseded(conversation) is true. In conversation-markers.ts, isSuperseded only checks for a comment that starts with <!-- review:superseded:. The README uses the same rule: "A thread labelled superseded is final, so it does not count as a posted finding."

Correction: describe the label, not the resolution. For example: "A thread labelled superseded is final, even if a cancelled run left it open, so it no longer shows its claim: a Review that returns to the head (A, B, A) must post the claim afresh." This keeps the reason for reposting and the A, B, A example, and states the open-thread case the test pins.

I read postedClaimIds in src/reconcile/inline.ts, isSuperseded and SUPERSEDED_MARKER_PATTERN in src/reconcile/conversation-markers.ts, and the scenario 7 test in src/reconcile/returning-review.test.ts. This is a static trace; I did not run the tests.

lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M4461DPCHEBNQQMHCR63NNKR of review 01M445XX16DSK1ZPJXFRG8AK47

<!-- review:claim:01M4461DPCHEBNQQMHCR63NNKR --> **low** — Comment calls a superseded thread "closed", but the check is the label alone and a labelled thread can stay open > The new comment says a superseded thread "was closed". The code it explains decides on the label alone, and a labelled thread can still be open. A maintainer who trusts the comment might change `postedClaimIds` to test resolution state (`resolver !== null`). That change would count a labelled but open thread as live, and the returning claim would not be reposted. The new test `scenario 7: a thread left labelled and open by a cancelled run stays, and a live one is posted` covers exactly this state. It expects `"headA claim+superseded open"` followed by a fresh `"headA claim open"`. > > `postedClaimIds` skips a conversation when `isSuperseded(conversation)` is true. In `conversation-markers.ts`, `isSuperseded` only checks for a comment that starts with <code>&lt;&#33;-- review:superseded:</code>. The README uses the same rule: "A thread labelled superseded is final, so it does not count as a posted finding." > > Correction: describe the label, not the resolution. For example: "A thread labelled superseded is final, even if a cancelled run left it open, so it no longer shows its claim: a Review that returns to the head (A, B, A) must post the claim afresh." This keeps the reason for reposting and the A, B, A example, and states the open-thread case the test pins. > > I read `postedClaimIds` in `src/reconcile/inline.ts`, `isSuperseded` and `SUPERSEDED_MARKER_PATTERN` in `src/reconcile/conversation-markers.ts`, and the scenario 7 test in `src/reconcile/returning-review.test.ts`. This is a static trace; I did not run the tests. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M4461DPCHEBNQQMHCR63NNKR` of review `01M445XX16DSK1ZPJXFRG8AK47`
🚀 1
jercik marked this conversation as resolved
jercik commented 2026-10-04 21:02:55 +00:00
Author
Owner
Copy link

Replying to review comment #119844

Tracked in #30, which replaces the copied outcome rows, the 422 problem-type list and the row list in the types.ts header with pointers to OUTCOMES, OUTCOME_EXIT, toFailure, FAILURE_REMEDIES and prefixFor. It is stacked on this PR.

> Replying to review comment #119844 Tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/30, which replaces the copied outcome rows, the 422 problem-type list and the row list in the `types.ts` header with pointers to `OUTCOMES`, `OUTCOME_EXIT`, `toFailure`, `FAILURE_REMEDIES` and `prefixFor`. It is stacked on this PR.
jercik commented 2026-10-04 21:02:58 +00:00
Author
Owner
Copy link

Replying to review comment #119845

Tracked in #30, which replaces the copied outcome rows, the 422 problem-type list and the row list in the types.ts header with pointers to OUTCOMES, OUTCOME_EXIT, toFailure, FAILURE_REMEDIES and prefixFor. It is stacked on this PR.

> Replying to review comment #119845 Tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/30, which replaces the copied outcome rows, the 422 problem-type list and the row list in the `types.ts` header with pointers to `OUTCOMES`, `OUTCOME_EXIT`, `toFailure`, `FAILURE_REMEDIES` and `prefixFor`. It is stacked on this PR.
jercik commented 2026-10-04 21:02:59 +00:00
Author
Owner
Copy link

Replying to review comment #119846

Tracked in #30, which replaces the copied outcome rows, the 422 problem-type list and the row list in the types.ts header with pointers to OUTCOMES, OUTCOME_EXIT, toFailure, FAILURE_REMEDIES and prefixFor. It is stacked on this PR.

> Replying to review comment #119846 Tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/30, which replaces the copied outcome rows, the 422 problem-type list and the row list in the `types.ts` header with pointers to `OUTCOMES`, `OUTCOME_EXIT`, `toFailure`, `FAILURE_REMEDIES` and `prefixFor`. It is stacked on this PR.
jercik commented 2026-10-04 21:02:59 +00:00
Author
Owner
Copy link

Replying to review comment #119847

Tracked in #30, which replaces the copied outcome rows, the 422 problem-type list and the row list in the types.ts header with pointers to OUTCOMES, OUTCOME_EXIT, toFailure, FAILURE_REMEDIES and prefixFor. It is stacked on this PR.

> Replying to review comment #119847 Tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/30, which replaces the copied outcome rows, the 422 problem-type list and the row list in the `types.ts` header with pointers to `OUTCOMES`, `OUTCOME_EXIT`, `toFailure`, `FAILURE_REMEDIES` and `prefixFor`. It is stacked on this PR.
jercik commented 2026-10-04 21:02:59 +00:00
Author
Owner
Copy link

Replying to review comment #119848

Tracked in #31, which rewords the comment to say a thread labelled superseded is final even when a cancelled run left it open, matching the label-only check in isSuperseded. It is stacked on this PR.

> Replying to review comment #119848 Tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/31, which rewords the comment to say a thread labelled superseded is final even when a cancelled run left it open, matching the label-only check in `isSuperseded`. It is stacked on this PR.
jercik commented 2026-10-04 21:04:57 +00:00
Author
Owner
Copy link

Replying to comment #119843

The unadjudicated claim 01M4460ZCR7EHPVD63SMPTH5SS is valid: the test names in src/reconcile/returning-review.test.ts carried "scenario N" numbers that no file in this repository defines. Tracked in #31, which renames those tests to say what each one sets up and asserts. It is stacked on this PR.

> Replying to comment #119843 The unadjudicated claim `01M4460ZCR7EHPVD63SMPTH5SS` is valid: the test names in `src/reconcile/returning-review.test.ts` carried "scenario N" numbers that no file in this repository defines. Tracked in https://code.j4k.dev/j4k-oss/review-wrapper/pulls/31, which renames those tests to say what each one sets up and asserts. It is stacked on this PR.
jercik merged commit 196d88aa1b into main 2026-10-05 09:24:03 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
j4k-oss/review-wrapper!29
No description provided.