diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 2809f7660..995fc6437 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -181,6 +181,7 @@ jobs: tests/dev-lead/unit/test_fewshot_assemble.bats \ tests/test_qa_lead_test_surface.bats \ tests/test_qa_lead_advisory_gate.bats \ + tests/test_qa_lead_holdout_calibration.bats \ tests/test_advisory_offline_mode.bats \ tests/agent_ingress_reference.bats \ tests/agent_ingress_if_filter.bats diff --git a/evals/qa-lead/holdout/cases.jsonl b/evals/qa-lead/holdout/cases.jsonl index 7a3ccc7d8..2987269cd 100644 --- a/evals/qa-lead/holdout/cases.jsonl +++ b/evals/qa-lead/holdout/cases.jsonl @@ -4,3 +4,6 @@ {"id": "qa-lead-hold-flaky-network-e2e", "description": "E2E depends on a live third-party endpoint, causing nondeterminism. Flakiness/isolation issue.", "tags": ["flakiness", "isolation"], "input": "PR's new E2E hits a real external payments sandbox over the network in CI; it fails ~10% of runs when the sandbox is slow.", "expected": {"escalate": false, "risk": "MEDIUM", "recommend": "stub/mock the external dependency at the boundary (or contract-test it) so CI is deterministic"}} {"id": "qa-lead-hold-payment-happy-only", "description": "Payment capture path tested only for success; missing failure/idempotency/edge. High risk on money path.", "tags": ["payments", "api"], "input": "PR adds card-capture handling; tests cover a successful capture only — no declined-card, no duplicate-submit/idempotency-key, no partial-refund path.", "expected": {"escalate": true, "risk": "HIGH", "recommend": "add declined, duplicate-submit (idempotency), and refund cases before merge — money paths are high-impact"}} {"id": "qa-lead-hold-coverage-gaming", "description": "Tests execute code but assert nothing meaningful, inflating coverage. Reward-hack-resistant: high coverage != quality.", "tags": ["security-anti-pattern", "coverage"], "input": "PR raises line coverage to 95% by adding tests that call functions and assert only that no exception is thrown (no output/behavior assertions).", "expected": {"escalate": true, "risk": "MEDIUM", "recommend": "coverage is gamed; require behavioral assertions on outputs/side-effects, not exception-free execution"}} +{"id": "qa-lead-hold-flaky-ui-timing", "description": "UI E2E is nondeterministic because it races a real fade-in animation via a fixed timing wait, not because of any production defect. A test-harness flakiness/isolation symptom with no production impact — remediate as tech debt, do NOT escalate. Grounds: 'flakiness is critical tech debt' AND 'risk vs value' (a harness timing symptom is not a merge-blocking production risk).", "tags": ["flakiness", "isolation"], "input": "PR adds a UI E2E that sleeps a fixed 500ms for a fade-in animation before asserting a banner is visible; it fails ~8% of runs on slower CI runners when the animation has not finished. The component under test is unchanged and behaves correctly in production.", "expected": {"escalate": false, "risk": "MEDIUM", "recommend": "replace the fixed animation-timing sleep with a deterministic web-first assertion on the element's visible/stable state so CI is deterministic; no production change is needed"}} +{"id": "qa-lead-hold-flaky-masks-real-race", "description": "The test is flaky because the PRODUCTION code does an unsynchronized read-modify-write on shared state; the nondeterminism originates in the product, not the harness, and papering it over with retries hides a real concurrency defect. High-impact production risk — escalate. Grounds: 'depth scales with impact'; do not mask a real race with retries.", "tags": ["flakiness", "concurrency"], "input": "PR makes an intermittently-failing test pass by wrapping it in a retry-3-times loop. The test drives two concurrent requests that increment a shared in-memory counter with no locking; under load the production handler loses updates — exactly what the test was catching intermittently — and the PR adds no synchronization to the handler.", "expected": {"escalate": true, "risk": "HIGH", "recommend": "fix the underlying production race (synchronize/lock the shared state or make the update atomic); do not mask the concurrency defect with test retries"}} +{"id": "qa-lead-hold-slow-deterministic-suite", "description": "A reliable, fully deterministic suite that is merely slow — a CI-speed/toil concern, not a correctness or flakiness one. The QA Lead must NOT escalate a slow-but-green suite. Grounds: 'risk vs value'; negative control against escalating any CI-signal problem.", "tags": ["negative", "ci-speed"], "input": "PR adds an integration test that reliably passes on every run but takes ~90s because it spins up a real container fixture; it slows the CI job noticeably but is fully deterministic with no intermittent failures.", "expected": {"escalate": false, "risk": "LOW", "recommend": "keep the deterministic coverage; optionally speed it up (share/parallelize the fixture or push some assertions to a lower test level), but it is not a correctness risk and needs no escalation"}} diff --git a/prompts/qa-lead/advisory.md b/prompts/qa-lead/advisory.md index 4e810584a..f404fc4eb 100644 --- a/prompts/qa-lead/advisory.md +++ b/prompts/qa-lead/advisory.md @@ -89,6 +89,50 @@ Everything else below — the risk assessment and the sentinel-wrapped output sh absent NFR/perf, fixture/isolation problems, flakiness); and whether anything is severe enough to **escalate** (you cannot block, only say so). + ### Calibrating the risk tier (read before you tier) + + The tier measures the **blast radius of the change on production and users if + the test gap ships** — *not* how annoying the test symptom is. Rank the + production consequence, then tier; do not rank the inconvenience of the failing + test. This is calibration, not caution: escalate the changes that genuinely + need it and let the rest through. + + - **A flaky, slow, or badly-isolated test is a test-infrastructure symptom, + not a production risk — until you have evidence it hides a real defect.** + Tier it by the harness cost, and prefer *fixing* it over escalating it: + - Flakiness / nondeterminism whose cause is the *harness* (a live network + dependency, a fixed-`sleep` timing race, a shared mutable fixture, a real + clock/animation) → typically **MEDIUM**, **escalate = no**. It is critical + tech debt worth fixing (isolate the boundary, use a deterministic wait), + but a slow or flaky *test* does not by itself endanger production, so it is + not an escalation. + - A suite that is **slow but fully deterministic** (reliably green, no + intermittent failures) is a CI-speed / toil concern → typically **LOW**, + **escalate = no**. Recommend speeding it up; do not inflate it. + - **Escalate a flaky test only when the flakiness is a *symptom of a real + production defect*** — e.g. the nondeterminism comes from an unsynchronized + race, lost update, or ordering bug in the *product* code, and a retry loop + would paper over it. That is **HIGH / escalate = yes**: the bug, not the + test, is the risk. + - **Do not let a high-stakes domain word inflate the tier when it appears only + in a *fixture*.** A "payments sandbox", an auth *stub*, or a mock production + hostname used by a test is test scaffolding — tier by what the *change* + actually risks in production, not by the vocabulary of the fixture. A flaky + E2E that merely *talks to* a payments sandbox is a MEDIUM isolation problem, + not a HIGH money-path risk. + - **Escalation is independent of the risk tier — decide each separately.** + Escalate whenever a gap needs a maintainer's attention before merge; that + can happen at MEDIUM, not only at HIGH. **Reserve HIGH for a real + production risk left unguarded** — an untested money/auth/data path, a + destructive migration with no rollback test, or the + flakiness-masks-a-real-defect case above: gaps with substantial production + blast radius. A **coverage-quality** gap such as meaningless assertions + that fake coverage on a real behavior is **MEDIUM / escalate = yes** — the + tests are hollow and must be fixed before merge, but the shipped behavior + is not itself high blast-radius, so it does not warrant HIGH. Never wave + through a real risk as LOW just to avoid over-escalating; the goal is the + right call on both tier and escalation. + ## Output — how you deliver the advisory Print the comment body **between these exact sentinel lines**, each alone on its diff --git a/tests/test_qa_lead_holdout_calibration.bats b/tests/test_qa_lead_holdout_calibration.bats new file mode 100644 index 000000000..a720870f4 --- /dev/null +++ b/tests/test_qa_lead_holdout_calibration.bats @@ -0,0 +1,122 @@ +#!/usr/bin/env bats +# Regression guard for the qa-lead risk-tiering recalibration (#1698). +# +# #1698 found qa-lead over-escalates a test-infrastructure flakiness symptom to +# HIGH/escalate where the reference is MEDIUM/no. The fix lives in the advisory +# prompt's risk-tiering guidance (prompts/qa-lead/advisory.md), but its calibration +# is characterised by held-out cases that separate three distinct signals: +# +# - flakiness with no production impact -> MEDIUM / escalate no +# - flakiness that MASKS a real production race -> HIGH / escalate yes +# - a slow-but-deterministic suite -> LOW / escalate no +# +# This test pins those three additions (AC #1) and asserts the six pre-existing +# held-out cases keep their expected escalate/risk verdicts, enforcing AC #2's +# "holdout stays byte-stable except for AC #1's additions" — in particular that +# the flaky-network-e2e reference stays MEDIUM/no and payment-happy-only stays +# HIGH/yes (the bias must not be "fixed" by weakening the references). + +setup() { + ROOT="$(cd "$BATS_TEST_DIRNAME/.." && pwd)" + HOLDOUT="$ROOT/evals/qa-lead/holdout/cases.jsonl" +} + +# assert_case [expected_recommend] +# +# When a fourth argument is given, the case's `recommend` must match it exactly — +# used to pin the six pre-existing references (AC #2's byte-stable requirement) +# so an unrelated recommendation cannot silently pass. When omitted, `recommend` +# only has to be non-empty — used for the three new additions (AC #1). +assert_case() { + local id="$1" escalate="$2" risk="$3" want_rec="${4-}" + run python3 -c ' +import json, sys +path, cid, want_esc, want_risk = sys.argv[1:5] +want_rec = sys.argv[5] if len(sys.argv) > 5 else None +want_esc = want_esc == "true" +found = None +for line in open(path, encoding="utf-8"): + line = line.strip() + if not line: + continue + case = json.loads(line) + if case.get("id") == cid: + found = case + break +if found is None: + print(f"missing case id: {cid}") + sys.exit(1) +exp = found.get("expected", {}) +esc_val = exp.get("escalate") +if esc_val is not want_esc: + print(f"{cid}: escalate={esc_val} want {want_esc}") + sys.exit(1) +risk_val = exp.get("risk") +if risk_val != want_risk: + print(f"{cid}: risk={risk_val} want {want_risk}") + sys.exit(1) +rec_val = str(exp.get("recommend", "")).strip() +if not rec_val: + print(f"{cid}: missing non-empty recommend") + sys.exit(1) +if want_rec is not None and rec_val != want_rec: + print(f"{cid}: recommend mismatch\n got: {rec_val}\n want: {want_rec}") + sys.exit(1) +print("ok") +' "$HOLDOUT" "$id" "$escalate" "$risk" ${want_rec:+"$want_rec"} + [ "$status" -eq 0 ] || { echo "$output"; return 1; } +} + +# --- AC #1: the three new characterization cases ------------------------------ + +@test "flaky UI/timing test with no production impact is MEDIUM / no-escalate" { + assert_case "qa-lead-hold-flaky-ui-timing" false MEDIUM +} + +@test "flakiness masking a real production race is HIGH / escalate" { + assert_case "qa-lead-hold-flaky-masks-real-race" true HIGH +} + +@test "slow-but-deterministic suite is LOW / no-escalate" { + assert_case "qa-lead-hold-slow-deterministic-suite" false LOW +} + +# --- AC #2: the six pre-existing references are unchanged ---------------------- + +@test "flaky-network-e2e reference stays MEDIUM / no-escalate" { + assert_case "qa-lead-hold-flaky-network-e2e" false MEDIUM \ + "stub/mock the external dependency at the boundary (or contract-test it) so CI is deterministic" +} + +@test "payment-happy-only reference stays HIGH / escalate" { + assert_case "qa-lead-hold-payment-happy-only" true HIGH \ + "add declined, duplicate-submit (idempotency), and refund cases before merge — money paths are high-impact" +} + +@test "auth-no-contract reference stays HIGH / escalate" { + assert_case "qa-lead-hold-auth-no-contract" true HIGH \ + "add negative-path API tests (expired/invalid/replay) and a Pact contract test before merge" +} + +@test "migration-no-rollback-test reference stays HIGH / escalate" { + assert_case "qa-lead-hold-migration-no-rollback-test" true HIGH \ + "add rollback + idempotency tests and capture a migration-duration NFR estimate" +} + +@test "coverage-gaming reference stays MEDIUM / escalate" { + assert_case "qa-lead-hold-coverage-gaming" true MEDIUM \ + "coverage is gamed; require behavioral assertions on outputs/side-effects, not exception-free execution" +} + +@test "well-tested-refactor reference stays LOW / no-escalate" { + assert_case "qa-lead-hold-well-tested-refactor" false LOW \ + "no additional tests required; existing coverage is sufficient" +} + +# --- The committed holdout stays schema-valid with the additions -------------- + +@test "committed qa-lead holdout validates against the case schema" { + run python3 "$ROOT/evals/validate-cases.py" "$HOLDOUT" "$ROOT/evals/case.schema.json" + [ "$status" -eq 0 ] + [[ "$output" == *"OK"* ]] +}