Skip to content

CI - Make the QA Review verdict deterministic - #4309

Open
agalin920 wants to merge 2 commits into
devfrom
fix/cv-verifier-deterministic-verdict
Open

CI - Make the QA Review verdict deterministic#4309
agalin920 wants to merge 2 commits into
devfrom
fix/cv-verifier-deterministic-verdict

Conversation

@agalin920

@agalin920 agalin920 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Problem

The QA Review verdict is not reproducible. The same PR can report PASS on 4 of 5 criteria on one run and INCONCLUSIVE on 3 of 4 on the next. No linked issue — raised directly by @agalin920.

Three causes, stacked:

  1. The denominator is model-generated. The prompt asked the agent to derive the criteria list every run with no rule on granularity, so the same issue yielded 5 criteria one run and 4 the next.
  2. The aggregation rule was ambiguous, and the model applied it. "INCONCLUSIVE if nothing is clearly failing but you couldn't confirm" says nothing about a mix — 4 ✅ + 1 ⚠️ is both "nothing failing" and "couldn't confirm one thing". Bash only grepped the marker the agent had already written.
  3. The scale offered "I can't judge this" as an outcome. The agent was asked to grade criteria it had no way to check, so it manufactured a mark and the verdict had to absorb it. The ✅/⚠️ boundary — the one that actually moves — had no tie-break at all.

Evidence

Pulled the <!-- cv-verifier --> comment from the last 12 PRs; nine have one. Eight of the nine carry at least one ⚠️ (only #4294 is clean), and the ⚠️ lines are two unrelated things sharing a mark:

kind examples
Needs a running app — permanent, not a PR property #4301.3, #4297.3, #4296.4, #4293.3, #4290.3
Plainly visible in the diff, softened to ⚠️ by the tie-break #4295.7 (data-cy absent from both Title TextFields), #4286.3 (CLAUDE.md left stale)
Genuinely uncertain #4291.1

Change

.github/workflows/claude-change-verifier.yml only.

  • Partition before grading. The agent splits criteria into checkable from the code and needs a running app, then grades only the first group as ✅ or ❌. No third mark, so the boundary that was drifting no longer exists.
  • Runtime criteria are listed, unscored, in a collapsed block. Dropping them silently would let 2 of 2 confirmed read as full coverage when an AC was never looked at. They have no downstream owner — claude-negative-qa.yml explicitly bars AC judgment — so they are the reviewer's manual checklist.
  • The verdict is computed in the post step from the marks: any ❌ → FAIL, otherwise PASS, with the counts in the heading (✅ 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.
  • INCONCLUSIVE is removed. It described this workflow's permanent condition — it never boots the app — not a per-PR result.
  • Criteria are pinned across pushes. A new Fetch prior QA comment step reads the existing comment; the prompt reuses its list verbatim, so a synchronize push re-judges a fixed list instead of inventing a new one.

…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>
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

QA Review — ❌ FAIL — 1 of 4 code-checkable criteria not met, 3 need a running app

No linked issue found — change-only QA.

  1. ✅ Verdict is computed deterministically from the agent's ✅/❌ marks in the post-processing step rather than declared by the agent
  2. ✅ Agent grading is restricted to two marks (✅ Satisfied / ❌ Not met) for checkable criteria, with non-checkable criteria listed separately and unscored
  3. ❌ New "Fetch prior QA comment" step reliably retrieves the single existing QA comment so criteria can be reused verbatim on reruns — it combines --paginate with a --jq filter wrapped in first // "" (.github/workflows/claude-change-verifier.yml:75-76), and gh's own pagination caveat is that --jq/--template filters run once per page rather than over the combined result set, so whole-set operations like first can misbehave once a PR's comment thread spans more than one page. The CID lookup a few lines below (:263-264) avoids exactly this by using a flat select(...) | .id piped to head -n1, which degrades correctly across pages; the new step doesn't reuse that pattern.
  4. ✅ QA gate fails only on a ❌ criterion or a missing/malformed report, with no special-casing left for the removed INCONCLUSIVE state
Not checkable from the code — needs a running app
  • Whether the agent actually follows the "reuse criteria verbatim, never renumber" instruction on a real second run against a live prior comment — needs the workflow triggered twice on an actual PR (e.g. opened then synchronize) and the resulting comment diffed.
  • Whether the completed comment (heading, collapsed <details> blocks, verdict marker) renders as intended in GitHub's markdown viewer — needs the posted comment opened in the GitHub UI.
  • Whether the --paginate/--jq pagination issue in criterion 3 actually manifests — needs a PR whose issue-comment thread exceeds one page, observed against the real GitHub API.
Suggested Cypress coverage

This PR only changes .github/workflows/claude-change-verifier.yml, a CI workflow with no corresponding application surface under cypress/e2e/; there is no manager-ui feature here for a Cypress spec to exercise, and none of the existing specs (e.g. cypress/e2e/settings/workflows.spec.js, which covers a different "workflow" — content publish-blocking labels) are relevant. No Cypress coverage applies to this change.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Code Review — ✅ No blockers

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Negative QA

No reproducible edge-case failures found on the surfaces this PR changes.

Also checked and working correctly
  • Confirmed the diff is scoped entirely to .github/workflows/claude-change-verifier.yml (a CI workflow), with no changes to app source, routes, or sub-apps — nothing in the running app to probe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants