docs: describe the superseded label and drop scenario numbers from test names #31

jercik · 2026-10-04T20:58:11Z

jercik commented

2026-10-04 20:58:11 +00:00

Follow-up to #29 for its review comment 119848 and for claim 01M4460ZCR7EHPVD63SMPTH5SS in the summary comment 119843.

Stacked on #29; merge after it.

🤖 Generated with Claude Code

Follow-up to #29 for its review comment [119848](https://code.j4k.dev/j4k-oss/review-wrapper/pulls/29#issuecomment-119848) and for claim `01M4460ZCR7EHPVD63SMPTH5SS` in the summary comment [119843](https://code.j4k.dev/j4k-oss/review-wrapper/pulls/29#issuecomment-119843). Stacked on #29; merge after it. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

jercik added 1 commit

2026-10-04 20:58:11 +00:00

docs: describe the superseded label and drop scenario numbers from test names

commit-msg / commitlint (pull_request) Successful in 27s

Details

Checks / quality-checks (pull_request) Successful in 1m1s

Details

Review / Review (pull_request_target) Successful in 6m21s

Details

a8d36d891b

The comment above `postedClaimIds` said a superseded thread "was closed",
but the check is the label alone and a labelled thread can stay open. The
test names carried "scenario N" numbers that no file in this repository
defines; they now describe what each test sets up and asserts.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

jercik referenced this pull request

2026-10-04 21:02:59 +00:00

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

forgejo-actions commented

2026-10-04 21:04:31 +00:00

Review 01M45PATCH387QEBE2FVHAQQ9F — head d9b0744fc4dd51b650ff1929dd8551f902662a3b

Review — j4k-oss/review-wrapper @ `211444be4c`

Scope: diff against base tree 61c3b02b7258
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"
}

Triage stalled: sandbox infrastructure failed.

Findings (0)

No findings survived.

Reviewed:

general-bug (whole/default): no-claims
writing-quality (whole/default): claims-emitted
test-trimming (whole/default): no-claims
restated-sets (whole/default): no-claims
project-docs (whole/default): no-claims

Other claims

grounding-pending (0)
ungrounded (0)
rejected (0)
duplicate-of (0)
unadjudicated (2)
- 01M45PGY7FGS5RJ7999KNVY12V low — Cancellation docstrings identify the wrong write as the failing one
- 01M45PHYB0T2FBNX7MXVHHRD6R medium — The test-file summary duplicates the reconciler selection in play()

Coverage

Coverage pass: 01M45PATG28MKZ8368859K12AB
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	no-claims	1	no
project-docs	whole	default	no-claims	1	no

**Review** `01M45PATCH387QEBE2FVHAQQ9F` — head `d9b0744fc4dd51b650ff1929dd8551f902662a3b` # Review — j4k-oss/review-wrapper @ 211444be4c6d Scope: diff against base tree `61c3b02b7258` 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" } ``` Triage stalled: sandbox infrastructure failed. ## Findings (0) No findings survived. Reviewed: - general-bug (whole/default): no-claims - writing-quality (whole/default): claims-emitted - test-trimming (whole/default): no-claims - restated-sets (whole/default): no-claims - project-docs (whole/default): no-claims ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (2) - `01M45PGY7FGS5RJ7999KNVY12V` low — Cancellation docstrings identify the wrong write as the failing one - `01M45PHYB0T2FBNX7MXVHHRD6R` medium — The test-file summary duplicates the reconciler selection in play() ## Coverage Coverage pass: 01M45PATG28MKZ8368859K12AB 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 | no-claims | 1 | no | | project-docs | whole | default | no-claims | 1 | no |

🚀 1

forgejo-actions reviewed

2026-10-04 21:04:31 +00:00

src/reconcile/inline.ts Outdated

Lines 29-30

					
				@ -28,3 +28,3 @@

				// A superseded thread was closed for a later Review, with no way back, so it no longer

				// shows its claim: a Review that returns to the head (A, B, A) must post the claim afresh.

				// A thread labelled superseded is final, even when 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 it afresh.

low — Superseded thread comment says the claim disappears

The comment tells maintainers that a superseded thread “no longer shows its claim,” although the claim comment remains in that thread, including when cancellation leaves it open. This makes the label sound like removal or hiding rather than a rule for whether the reconciler should repost the claim.

The function below the comment skips conversations with isSuperseded when building posted, while projectSuperseded in src/reconcile/dispositions.ts appends a label and may resolve the thread; it does not remove the original claim comment. returning-review.test.ts asserts that a cancelled run can leave headA claim+superseded open beside a new headA claim open. The README describes the intended rule as “does not count as a posted finding.”

Replace “so it no longer shows its claim” with “so it does not count as a posted claim.” This preserves the finality and reposting rule while describing the actual check. I traced postedClaimIds, projectSuperseded, and the cancellation test; I did not inspect the forge UI. Forge rendering that hides claim content for superseded threads would refute the visibility concern, but the code and test establish that the claim comment remains in the conversation.

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

**low** — Superseded thread comment says the claim disappears > The comment tells maintainers that a superseded thread “no longer shows its claim,” although the claim comment remains in that thread, including when cancellation leaves it open. This makes the label sound like removal or hiding rather than a rule for whether the reconciler should repost the claim. > > The function below the comment skips conversations with `isSuperseded` when building `posted`, while `projectSuperseded` in `src/reconcile/dispositions.ts` appends a label and may resolve the thread; it does not remove the original claim comment. `returning-review.test.ts` asserts that a cancelled run can leave `headA claim+superseded open` beside a new `headA claim open`. The README describes the intended rule as “does not count as a posted finding.” > > Replace “so it no longer shows its claim” with “so it does not count as a posted claim.” This preserves the finality and reposting rule while describing the actual check. I traced `postedClaimIds`, `projectSuperseded`, and the cancellation test; I did not inspect the forge UI. Forge rendering that hides claim content for superseded threads would refute the visibility concern, but the code and test establish that the claim comment remains in the conversation. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44BGP3185Z8B7F25CES7R7S` of review `01M44BBJW88A8XVAQ8T0GSSRF1`

👍 1

Fixed in 2a35ecb. The comment now says a superseded thread "does not count as a posted claim", which is what postedClaimIds checks; the claim comment itself stays in the thread.

Fixed in 2a35ecb. The comment now says a superseded thread "does not count as a posted claim", which is what `postedClaimIds` checks; the claim comment itself stays in the thread.

jercik marked this conversation as resolved

src/reconcile/returning-review.test.ts Outdated

					
				@ -320,3 +320,3 @@

				  });

				  it("scenario 2: A, B, then A on a rebased head posts on the rebased head", async () => {

				  it("returning to A on a rebased head after B posts A's findings on the rebased head", async () => {

low — Returning review test name attributes A's repost to B

The test name reads as though B posts A's findings on the rebased head. A reader scanning test results could infer the wrong owner and head for the posting step, obscuring what the test actually protects.

In this case a owns claims on lines 2 and 5, b owns a claim on line 8, and play runs A on headA, B on headA, then A on headA2. The final assertions find A's new open threads on headA2. The preceding renamed test has the same “after B posts A's findings” phrasing, though there the returning run uses the original head.

Name the cases from the actor and transition: “A returns after B and reposts its findings on the rebased head” here, and “A returns after B and reposts its findings in live threads” for the preceding case. This preserves the tested outcomes and makes the posting actor clear. I traced the fixtures and play calls in returning-review.test.ts; I did not run the suite. The claim would be refuted if B's ServiceReview or a forge call in play actually posted A's claims, but the fixture and reconciler calls show B receives only B's claims.

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

**low** — Returning review test name attributes A's repost to B > The test name reads as though B posts A's findings on the rebased head. A reader scanning test results could infer the wrong owner and head for the posting step, obscuring what the test actually protects. > > In this case `a` owns claims on lines 2 and 5, `b` owns a claim on line 8, and `play` runs A on `headA`, B on `headA`, then A on `headA2`. The final assertions find A's new open threads on `headA2`. The preceding renamed test has the same “after B posts A's findings” phrasing, though there the returning run uses the original head. > > Name the cases from the actor and transition: “A returns after B and reposts its findings on the rebased head” here, and “A returns after B and reposts its findings in live threads” for the preceding case. This preserves the tested outcomes and makes the posting actor clear. I traced the fixtures and `play` calls in `returning-review.test.ts`; I did not run the suite. The claim would be refuted if B's `ServiceReview` or a forge call in `play` actually posted A's claims, but the fixture and reconciler calls show B receives only B's claims. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M44BHD09DNEYTD6D6YQNANKT` of review `01M44BBJW88A8XVAQ8T0GSSRF1`

👍 1

Fixed in 2a35ecb. This test is now "after B, review A reposts its findings on the rebased head", and the one before it "after B, review A reposts its findings with one live thread each". The names start lowercase because the vitest lint rule requires it.

Fixed in 2a35ecb. This test is now "after B, review A reposts its findings on the rebased head", and the one before it "after B, review A reposts its findings with one live thread each". The names start lowercase because the vitest lint rule requires it.

jercik marked this conversation as resolved

jercik referenced this pull request

2026-10-04 21:04:57 +00:00

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

jercik added 1 commit

2026-10-04 21:06:53 +00:00

docs: say a superseded thread no longer counts as posted, and name the reposting review

commit-msg / commitlint (pull_request) Successful in 16s

Details

Checks / quality-checks (pull_request) Successful in 33s

Details

Review / Review (pull_request_target) Has been cancelled

Details

2a35ecb269

The comment above `postedClaimIds` said a superseded thread "no longer shows
its claim", but the claim comment stays in that thread; the reconciler only
stops counting it as posted. Two test names read as if B posted A's findings;
they now name review A as the one that reposts them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

jercik changed target branch from feat/same-diff-attach to main

2026-10-05 09:29:10 +00:00

jercik force-pushed docs/returning-review-comments from 2a35ecb269

commit-msg / commitlint (pull_request) Successful in 16s

Details

Checks / quality-checks (pull_request) Successful in 33s

Details

Review / Review (pull_request_target) Has been cancelled

Details

to d9b0744fc4

commit-msg / commitlint (pull_request) Successful in 36s

Details

Checks / quality-checks (pull_request) Successful in 47s

Details

Review / Review (pull_request_target) Failing after 6m53s

Details

2026-10-05 09:29:12 +00:00

Compare

jercik merged commit 345d5bb064 into main

2026-10-05 09:30:14 +00:00

jercik referenced this pull request from a commit

2026-10-05 09:30:14 +00:00

docs: describe the superseded label and drop scenario numbers from test names (#31)

Sign in to join this conversation.

No reviewers

No labels

No milestone

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!31

Rows
Columns