review: an author acknowledgment stops reading as unaddressed - #367
review: an author acknowledgment stops reading as unaddressed#367jwbron wants to merge 2 commits into
Conversation
… as unaddressed The webapp#41290 pathology: the author replied will-fix/TODO on bot threads without resolving them, and every later recap counted those threads unaddressed while the pipeline re-derived the defect afresh. The thread-reconciler already had the concession rule (keep, never re-raise) but no output channel for it. The reconciler now reports conceded-but-unfixed keeps as acknowledged; code verifies each id against the staged reply chain (keep membership required, the PR author must actually have replied, bot replies never count, no staged author verifies nothing) and the recap renders those threads as acknowledged (fix pending) instead of unaddressed. An acknowledged blocking thread still renders visibly and still counts toward keptBlockingCount, so the flip gate is unchanged. Verified ids land in rereview.json and the dispatch artifact as the membership feed the family-suppression matcher (design/family-suppression.md) is gated on; no suppression pass consumes them yet.
🦋 Changeset detectedLatest commit: 804e35a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested — see inline comments.
Note: correctness-reviewer not assessed this run (correctness-reviewer output unavailable).
review details
review-v1.17.1 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation| @@ -0,0 +1,4 @@ | |||
| --- | |||
| --- | |||
There was a problem hiding this comment.
issue (blocking, best-practice): Versioning: a workflow change must ship a changeset that bumps the review package. workflows/review/README.md (the review-workflow-contract skill), Versioning, states the rule: "Published as git tags via the repo's changeset → utils/run-publish.ts release flow. A change to this workflow lands with a changeset bumping the review package; on release a review-v<major>.<minor>.<patch> tag (and a moving review-v<major> tag) is cut at the real commit tree".
The added changeset carries no package bump — its whole frontmatter is two consecutive delimiters:
---
---
That is an empty (no-op) changeset. I confirmed on the checkout that this is a divergence, not a house convention: workflows/review/package.json is at "version": "1.17.1", .changeset/config.json has an empty ignore: [] (so review is not exempt), and workflows/review/CHANGELOG.md shows every prior comparable change shipping a real bump — 1.17.0 for the 👎-adjudication behavior change and 1.16.0 for the linguist-generated negation change, both of which are the same class of reviewer-behavior change as this one. This PR is squarely a behavior change to the shipped workflow: it adds an acknowledged output to the thread-reconciler prompt (workflows/review/review.md), a new verifiedAcknowledgedIds verification path plus new recap wording (workflows/review/lib/rereview.ts), and new rereview.json artifact fields.
Fix: give the changeset a package bump line, matching the minor-bump precedent set by 1.16.0/1.17.0 for reviewer-behavior changes.
A sketch, not a committable replacement:
---
"review": minor
---
review details
found by skill-auditor | also flagged by completeness| --- | ||
| --- | ||
|
|
||
| An author acknowledgment stops reading as "unaddressed": the thread-reconciler now reports kept threads whose reply chain shows the author conceded the finding (will fix, TODO stands in) as `acknowledged`, code verifies each id against the staged reply chain (keep membership required, the PR author must actually have replied, bot replies never count, no staged author verifies nothing), and the re-review recap counts those threads as "acknowledged (fix pending)" instead of unaddressed. An acknowledged blocking thread still renders visibly and still counts toward `keptBlockingCount`, so the flip gate is unchanged. Verified ids land in `rereview.json` and the dispatch artifact as a membership feed for the adjudicated-corpus suppression (dedup-adjudicated.ts); no suppression pass consumes them yet. |
There was a problem hiding this comment.
suggestion (non-blocking, documentation): Changeset says "verified ids land in ... the dispatch artifact"; only the raw reconciler list does. The changeset states "Verified ids land in rereview.json and the dispatch artifact as a membership feed for the adjudicated-corpus suppression (dedup-adjudicated.ts)", but dispatch.ts stores the unverified parse — reconciliation = parseReconciliation(parsed); (dispatch.ts:686) into ...(reconciliation !== undefined ? {reconciliation} : {}) (dispatch.ts:929) — and this PR's own dispatch.test.ts comment says "the recap CLI verifies each id separately"; verification runs only in rereview.ts and only rereview.json carries the verified subset. Rewrite as: verified ids land in rereview.json; the dispatch artifact carries the reconciler's unverified list.
review details
found by documentation| : [], | ||
| skipLines: parsed["skipLines"] ?? [], | ||
| }; | ||
| reconciliation = parseReconciliation(parsed); |
There was a problem hiding this comment.
note (non-blocking): The dispatch artifact carries the reconciler's raw acknowledged, not the "verified ids" the description and changeset claim. verifiedAcknowledgedIds runs only in rereview.ts, and its output lands in rereview.json alone; README line 59 says precisely that, while the PR description and changeset both say "Verified ids land in rereview.json and the dispatch artifact" (the dispatch test comment confirms the artifact copy is unverified). Nothing consumes it yet, so the cost today is a misleading claim about the membership feed's guarantee — worth narrowing the wording or recording the verified set alongside it.
review details
found by completeness| // must not invalidate the whole reconciliation the way a malformed | ||
| // resolve/keep does, or a bad ack array would erase the accountability | ||
| // section entirely. | ||
| const acknowledged = ids(raw["acknowledged"]); |
There was a problem hiding this comment.
suggestion (non-blocking): The same new field gets two different admission rules in the two parsers this PR touches. This PR extracts parseReconciliation into dispatch-contracts.ts as the documented shape authority for the reconciler's JSON, then hand-extends the separate parseReconciler in rereview.ts with the opposite leniency for the same field — the two new tests encode the divergence rather than reconcile it (dispatch.test.ts:544 expects ["t1"] from ["t1", 42]; rereview.test.ts:951 expects none from [42]). rereview.ts already imports sibling modules (./threads, ./render-comment), so sharing one admission function is available.
A sketch, not a committable replacement:
Give `acknowledged` one rule in one place — e.g. have rereview.ts's local `ids` filter non-strings for this field instead of rejecting the array, matching `parseReconciliation`:
const acknowledged = Array.isArray(raw["acknowledged"])
? raw["acknowledged"].filter((v): v is string => typeof v === "string")
: undefined;
(or import `parseReconciliation` and drop the second parser).
review details
found by holistic| // REQUEST_CHANGES until a full-depth review re-judges it, which is | ||
| // noise, not a wrongly-permitted approval. | ||
| blocking: label === "unknown" || isBlockingLabel(label), | ||
| acknowledged: acknowledgedIds.has(threadId), |
There was a problem hiding this comment.
suggestion (non-blocking): No test asserts an unacknowledged kept thread rendered beside an acknowledged one lacks the marker. The one mixed-render test (rereview.test.ts:810, threads ack + plain) asserts the header count and the ack line, but never asserts anything about the plain line; every other new render test has a single thread or zero verified acks (grep for acknowledged in rereview.test.ts shows no negative per-line assertion). One extra expect on the plain line pins the per-entry semantics.
| acknowledged: acknowledgedIds.has(threadId), | |
| // in "counts verified acks as addressed-pending in the header and marks their lines" | |
| expect(result.section).toContain( | |
| "- **note (non-blocking)** " + | |
| "[`a/a.go:5`](https://github.com/o/r/pull/1#discussion_plain): " + | |
| "Side effects escape the gate.", | |
| ); |
review details
found by test-adequacy| return surviving; | ||
| }; | ||
|
|
||
| /** The reconciler's decision, as {@link parseReconciliation} admits it. */ |
There was a problem hiding this comment.
nitpick (non-blocking): New contract block appended without the file's section banner. Every distinct concern in this file opens with a banner, e.g. /* -------------------------------------------------------------------------- */ /* Defect clustering (the claim-clusterer contract) */ (dispatch-contracts.ts:336-338), and likewise at lines 46, 389, 439, and 492. The new reconciler contract starts at /** The reconciler's decision, as {@link parseReconciliation} admits it. */ (line 707), appended straight onto the tail of the "Claims and verification" section with no banner of its own.
review details
found by conventions| * "nothing acknowledged", and `skipLines` passes through raw (its consumer | ||
| * shape-checks it). | ||
| */ | ||
| export const parseReconciliation = ( |
There was a problem hiding this comment.
question (non-blocking): Does the recap-label win justify the cross-layer plumbing, given no consumer and a re-derivation problem the open-thread corpus already covers? The rereview CLI parses thread-reconciler.json itself (parseReconciler, rereview.ts:566) and never reads DispatchResult.reconciliation; submission.ts reads only resolve/skipLines, so parseReconciliation's acknowledged is write-only artifact data by the PR's own admission ("No suppression pass consumes them yet"). The description's other stated symptom — "the pipeline re-derived the defect afresh" — is about a kept, unresolved thread, which threads.json already stages into the open-thread suppression corpus (dedup-adjudicated.ts's header describes the resolved-thread case as the gap), so it is worth stating what re-derivation remains that this feed would fix rather than shipping the carrier ahead of it.
review details
found by first-principles| }; | ||
|
|
||
| /** The reconciler's decision, as {@link parseReconciliation} admits it. */ | ||
| export type Reconciliation = { |
There was a problem hiding this comment.
suggestion (non-blocking): New Reconciliation re-declares rereview.ts's ReconcilerResult rather than extending it. The lib's pattern is one declaration imported everywhere — Claim/ProposedCluster are declared in dispatch-contracts.ts and imported by dedup.ts, dedup-cluster.ts, dedup-adjudicated.ts, submission.ts, and where a module adds fields it intersects the imported type: export type FetchedThread = StagedThread & { (threads.ts:259, importing StagedThread from rereview.ts). The new export type Reconciliation = {resolve: string[]; keep: string[]; acknowledged?: string[]; skipLines: unknown}; (dispatch-contracts.ts:708-722) instead restates export type ReconcilerResult (rereview.ts:48-61) field for field, including the new acknowledged doc block.
| export type Reconciliation = { | |
| import type {ReconcilerResult} from "./rereview"; | |
| /** The reconciler's decision, as {`@link` parseReconciliation} admits it. */ | |
| export type Reconciliation = ReconcilerResult & {skipLines: unknown}; |
review details
found by conventions| acknowledged: ["t1"], | ||
| }, | ||
| // No prAuthor staged: verification fails closed, and the | ||
| // section is byte-identical to the pre-PRA-47 render. |
There was a problem hiding this comment.
suggestion (non-blocking, documentation): Test comment cites an undefined codename, "pre-PRA-47". The comment reads "the section is byte-identical to the pre-PRA-47 render" (rereview.test.ts:845), but nothing defines "PRA-47": its only other traces in the repo are this PR's own branch name (agent/pra47-ack-rule) and changeset filename (.changeset/pra47-acknowledged-threads.md), and the PR itself cites KORE-2514. The test's own name already has the plain term — "keeps the exact pre-acknowledgment wording when nothing verifies" (rereview.test.ts:836).
| // section is byte-identical to the pre-PRA-47 render. | |
| // section is byte-identical to the pre-acknowledgment render. |
review details
found by documentation| prAuthor === undefined || | ||
| prAuthor === "" || | ||
| isReviewBotAuthor(prAuthor) || | ||
| /\[bot\]$/i.test(prAuthor) |
There was a problem hiding this comment.
suggestion (non-blocking): Bot-suffix knowledge is spelled inline here instead of coming from threads.ts. threads.ts is documented as owning login identity — "This module also owns the answer to 'is this login our review bot' ... #302 was exactly that disagreement" — and keeps the suffix rule in one private helper: return lowered.endsWith("[bot]") ? lowered.slice(0, -"[bot]".length) : lowered; (threads.ts:88-92, baseLogin), exposed only through sameLogin/isReviewBotAuthor. The new code re-spells that rule locally twice: /\[bot\]$/i.test(prAuthor) (rereview.ts:96) and !/\[bot\]$/i.test(comment.author) (rereview.ts:112) — the only inline bot-suffix tests in workflows/review/lib. Worth noting this is a request to add the missing predicate rather than to use an existing one: threads.ts exports no generic "is this any bot login" check today, which is presumably why the rule got re-spelled here. Cheap to fix now, and it keeps the suffix knowledge in the module that documents itself as owning it.
A sketch, not a committable replacement:
// threads.ts, beside sameLogin/isReviewBotAuthor:
export const isBotLogin = (login: string): boolean =>
login.toLowerCase().endsWith("[bot]");
// rereview.ts:
import {isBotLogin, isReviewBotAuthor, sameLogin} from "./threads";
// ...
isReviewBotAuthor(prAuthor) ||
isBotLogin(prAuthor)
review details
found by conventions
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus 0441b7c6a581 (10 cases).
Adversarial hard gate: PASSED on the candidate arm. Raw output of each failed agentcandidate / incident-money-rounding / claim-validatorAgent failures
Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
The changeset shipped with empty frontmatter, a no-op that would have published nothing; it now bumps review as minor, matching the 1.16.0/1.17.0 precedent for reviewer-behavior changes. The dispatch-artifact carriage of acknowledged is dropped rather than documented better (dispatch.ts, dispatch-contracts.ts, dispatch.test.ts reverted to their pre-PR state): nothing read reconciliation.acknowledged from dispatch-result.json, the raw reconciler JSON already lands in the run artifact at out/thread-reconciler.json, and the future adjudicated-corpus consumer needs the verified set, which lives in rereview.json. That leaves one admission rule for the field, and rereview's parseReconciler now filters non-string entries instead of dropping the whole array: one junk entry must not erase real acknowledgments, and every survivor still passes verifiedAcknowledgedIds. The verification docs stop claiming the thread-resolution pattern: code verifies that a real non-bot PR-author reply exists on a kept thread (necessary for a concession, not sufficient), and concession-vs-pushback stays the reconciler's judgment, with a wrong call costing one mislabeled recap line rendered under the reply's own author. The inline [bot]-suffix tests move to threads.ts as isBotLogin, the module that owns login identity. Tests: the mixed render asserts the unmarked plain line, the resolvedCount === 0 branch asserts its header form, and the undefined pre-PRA-47 codename in a test comment is gone.
|
Addressed in 804e35a; per-thread rundown since I can't reply inline from this session.
|
|
Holding for human review — the automated review could not complete safely this run.
|
2 participants
On webapp#41290 the author replied will-fix/TODO on bot threads without resolving them, and every later recap counted those threads "unaddressed" while the pipeline re-derived the defect afresh. The reconciler prompt already had the concession rule (keep the thread, never re-raise) but no output channel for it; this gives it one.
acknowledged: kept threads whose reply chain shows the author conceded the finding (agreed, will fix, TODO stands in) with the code unchanged. Prompt-side rules: only the author's own reply counts, never a bot reply, never pushback, never a TODO inferred from code alone; when in doubt leave it out.keptBlockingCount. The code change is what resolves a blocking thread, never the promise of one.rereview.jsononly; nothing consumes them yet. The dispatch artifact doesn't carry the field: the raw reconciler JSON is already in the run artifact atout/thread-reconciler.json, and the adjudicated-corpus suppression's cross-file widening (the eventual consumer, which wants the verified set) ships separately.KORE-2514