Conversation
…asking for it
The verdict moved between runs of the same PR — 4 of 5 reported PASS, 3 of 4
reported INCONCLUSIVE — from three stacked causes:
1. The criteria list was re-derived each run with no granularity rule, so the
denominator itself moved.
2. The aggregation rule ("INCONCLUSIVE if nothing is clearly failing but you
couldn't confirm") said nothing about a mix of marks, so the model broke
the tie by feel, differently each time.
3. The grading scale offered "I can't judge this" as an outcome, so criteria
that were never checkable produced a mark the verdict had to absorb.
Across the last nine PRs with a QA Review comment, eight carried at least one
warning, and the warnings were two unrelated things sharing a mark: five were
"can't confirm without running the app" (a permanent property of a workflow
that never boots the app) and two were plainly-visible failures softened to a
warning by the tie-break rule.
The agent now partitions criteria before judging any of them — checkable from
the code, or needs a running app — and grades only the first group, with no
third mark. Runtime criteria are listed unscored rather than dropped, so the
count does not overstate coverage. The verdict is arithmetic on the marks,
computed in the post step: any failed criterion fails the check, otherwise it
passes, with the counts in the heading. INCONCLUSIVE is gone; it described the
workflow's permanent condition, not a per-PR result.
Criteria are also pinned across pushes — a new step reads the existing comment
and the prompt reuses its list verbatim, so a synchronize push re-judges a
fixed list instead of inventing a new one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Acceptance Criteria QA — ✅ PASS — 4/4 code-checkable criteria confirmed, 3 need a running appNo linked issue found — change-only QA.
Not checkable from the code — needs a running app
Suggested Cypress coverageThis PR only changes |
Code Review — ✅ No blockers |
Adversarial Browser QANo reproducible edge-case failures found on the surfaces this PR changes. Also checked and working correctly
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # details block. That separation is what makes both countable. | ||
| PASSED=$(grep -cE '^[0-9]+\. ✅' "$FILE" || true) | ||
| FAILED=$(grep -cE '^[0-9]+\. ❌' "$FILE" || true) | ||
| RUNTIME=$(sed -n '/<summary>Not checkable from the code/,/<\/details>/p' "$FILE" | grep -cE '^- ' || true) |
There was a problem hiding this comment.
🟡 Verdict counts aren't scoped like the RUNTIME count is
PASSED/FAILED are computed with grep -cE '^[0-9]+\. ✅|❌' over the entire file, whereas RUNTIME is deliberately bounded with sed to only the "Not checkable" <details> block. A stray line elsewhere (e.g. in the free-form "Suggested Cypress coverage" prose, where the model naturally cross-references a criterion like 2. ❌ Add a test for the case flagged above) would silently double-count into the verdict — the exact class of formatting-dependent non-determinism this PR sets out to remove. Bound the grep to the region between the heading and the first <details> tag, the same way RUNTIME is bounded.
| PRIOR_REPORT = ${{ github.workspace }}/prior-qa-comment.md (empty = first run) | ||
|
|
||
| 1. Establish the criteria. | ||
| If PRIOR_REPORT is non-empty, this PR has been reviewed before. Read it and REUSE its |
There was a problem hiding this comment.
🟡 No handling for a pre-existing legacy-format prior comment
PRIOR_REPORT is read verbatim from whatever `` comment already exists, but the prompt assumes it already has the new two-group (A/B) shape. Per the PR description, most currently-open PRs already carry an old-style comment (✅/synchronize push on any of those after this merges will feed that old-format body in as "reuse verbatim, same split between the two groups," with no instruction for what to do when the prior report doesn't have that split — likely producing a garbled re-judgment on that one run.
Code Review — ✅ No blockers · 🟡 2 warning(s) — see inline comments |
| 1. Establish the criteria. | ||
| If PRIOR_REPORT is non-empty, this PR has been reviewed before. Read it and REUSE its | ||
| criteria verbatim — same wording, same numbering, same split between the two groups in | ||
| step 2 — then re-judge them against the current diff. Add a criterion only if the issue | ||
| itself gained one. Never re-split, re-word, or renumber an existing one. Holding the | ||
| list fixed is the point: a list that moves between runs makes two runs of the same PR | ||
| incomparable. | ||
| Otherwise, if ISSUE_NUMBER is set, read it (gh issue view <ISSUE_NUMBER> --repo <REPO> | ||
| --json title,body,comments,labels) and derive the criteria. Where the issue states its | ||
| acceptance criteria explicitly, take its bullets one-to-one rather than merging or | ||
| paraphrasing them. If ISSUE_NUMBER is empty, run change-only mode: the criteria are the | ||
| checks you judge worth performing on this change. |
There was a problem hiding this comment.
🟡 Criteria lock-in ignores a newly-linked issue
ISSUE_NUMBER is only consulted when PRIOR_REPORT is empty. PRIOR_REPORT is fetched independently of whether an issue is linked, so a PR that starts with no linked issue (change-only mode, ad-hoc criteria) and later gets a "Closes #123" added to its description (or a branch rename that changes what Resolve linked issue resolves) will never re-derive criteria from that issue — every subsequent run just re-judges the original ad-hoc change-only list forever, silently defeating the acceptance-criteria check. Consider also gating the reuse-vs-derive branch on whether ISSUE_NUMBER differs from what's recorded in PRIOR_REPORT (e.g. the "Validates #<n>" line), not just on whether a prior report exists.
Code Review — ✅ No blockers · 🟡 1 warning(s) — see inline comments |
…g gh into head
Two shapes in this workflow combined `gh api --paginate` with a `--jq` filter
that assumes a single combined result set. gh applies `--jq` once per page, so
`| first // ""` in `Fetch prior QA comment` ran per page: forcing pagination on
this PR's own thread (`?per_page=1`, 10 comments) turns one 25-line body into 30
output lines — the body plus a blank line for every page that did not match,
positioned by comment order. Both steps now select per element and slice the
first match, which is page-independent. base64 keeps a multi-line body on one
line; verified byte-for-byte against the live comment under forced pagination.
Neither step pipes gh into `head` any more. Under `set -o pipefail` that exits
141 as soon as a second match lands on a later page — reproduced directly, and
in `Post or update QA comment` it would have killed the step after the verdict
was computed and before the comment was posted, leaving a red check with
nothing explaining it. `${VAR%%$'\n'*}` slices the capture with no pipe at all.
The whitespace-normalisation line after the fetch is gone: a body that passes
`startswith("<!-- cv-verifier -->")` cannot be blank, and a no-match capture
decodes to zero bytes, so `-s` alone answers the prompt's first-run question.
Never manifested — the default page size is 30 and no PR thread in this repo
has passed it. Found by this workflow's own new gate on this PR (criterion 3 of
run 35025289605); reviewed adversarially, no blockers, four suggestions applied.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Conflict in .github/workflows/claude-change-verifier.yml only, from #4307 (Claude workflow/job rename) landing on dev while this branch rewrote the same report template and gate message. Resolved as dev's names over this branch's logic: - Report heading is `## Acceptance Criteria QA` (dev's name), still bare — this branch removed the agent-written `— <PASS|FAIL|INCONCLUSIVE>` suffix because the post step computes the verdict. - The gate's `::error::` keeps this branch's wording (no "or a clear bug"; the agent no longer judges bugs) and dev's comment name. Two unconflicted lines renamed for consistency, because the post step's parser is coupled to the template it completes: the prompt's "leave the `## …` heading exactly as written" instruction, and the python `startswith(...)` / rewrite pair in `Post or update QA comment`. Leaving those at "QA Review" would have left the heading uncompleted and the counts off every comment. Verification: all five Claude workflows parse under js-yaml; prettier clean. The post step was extracted verbatim from the merged YAML and re-run against the same six fixtures as the original commit — 6✅/1❌ → `❌ FAIL — 1 of 7`; 2✅/1 runtime → `✅ PASS — 2/2 …, 1 needs a running app`; all-runtime → PASS "nothing was checkable"; no criteria → gate red; no marker → gate red; and a disobedient report (own verdict marker, heading suffix, prose ✅) normalised to one marker with the prose ✅ uncounted. Merged tree differs from dev in this one file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3ad0e5d
| 1. Establish the criteria. | ||
| If PRIOR_REPORT is non-empty, this PR has been reviewed before. Read it and REUSE its | ||
| criteria verbatim — same wording, same numbering, same split between the two groups in |
There was a problem hiding this comment.
🟡 Prior-comment reuse breaks for PRs QA'd under the old template
PRIOR_REPORT is fetched from any existing `` comment regardless of which workflow version wrote it. Any PR that already has a QA comment from before this change (single ✅/❌/
Code Review — ✅ No blockers · 🟡 1 warning(s) — see inline comments |
3 participants
Problem
The
QA Reviewverdict is not reproducible. The same PR can reportPASSon 4 of 5 criteria on one run andINCONCLUSIVEon 3 of 4 on the next. No linked issue — raised directly by @agalin920.Three causes, stacked:
Evidence
Pulled the⚠️ (only #4294 is clean), and the ⚠️ lines are two unrelated things sharing a mark:
<!-- cv-verifier -->comment from the last 12 PRs; nine have one. Eight of the nine carry at least onedata-cyabsent from both TitleTextFields), #4286.3 (CLAUDE.md left stale)Change
.github/workflows/claude-change-verifier.ymlonly.2 of 2 confirmedread as full coverage when an AC was never looked at. They have no downstream owner —claude-negative-qa.ymlexplicitly bars AC judgment — so they are the reviewer's manual checklist.✅ PASS — 2/2 code-checkable criteria confirmed, 1 needs a running app). The agent no longer writes a verdict; any marker it writes anyway is stripped.Fetch prior QA commentstep reads the existing comment; the prompt reuses its list verbatim, so asynchronizepush re-judges a fixed list instead of inventing a new one.Merged
dev2026-09-18#4307 renamed this workflow and its report heading while this branch rewrote the same template, so the two conflicted. Resolved as #4307's names over this branch's logic: the heading is
## Acceptance Criteria QA, still bare because the post step completes it. The parser inPost or update QA commentwas renamed to match — it keys on the heading it writes, and git had no reason to flag that coupling. Six fixtures re-run against the post step extracted verbatim from the merged YAML; all five Claude workflows parse; prettier clean.🤖 Generated with Claude Code