Skip to content

fix(qa-gate): terminate no-FAIL diagnosis phase instead of looping (#655 sub-defect B)#657

Open
halindrome wants to merge 5 commits into
swt-labs:mainfrom
halindrome:fix/655-qa-gate-noncode-terminal
Open

fix(qa-gate): terminate no-FAIL diagnosis phase instead of looping (#655 sub-defect B)#657
halindrome wants to merge 5 commits into
swt-labs:mainfrom
halindrome:fix/655-qa-gate-noncode-terminal

Conversation

@halindrome

Copy link
Copy Markdown
Contributor

What

Give the QA remediation gate (scripts/qa-result-gate.sh) a clean terminal state for a phase that legitimately has zero FAIL checks and zero code changes — a read-only diagnosis / documentation-disposition round — so it terminates (PROCEED_TO_UAT) instead of looping forever. Addresses sub-defect B of #655.

Why

source_verification_missing was exempted for input_mode=none only at _gate_stage=done, but a round must clear the verify stage to reach done. A no-FAIL disposition round was therefore flagged qa_gate_source_verification_missing=true at verify and routed REMEDIATION_REQUIRED forever; the only escape observed in the field was manually advancing verify → done. A reproduction confirmed the loop is real and identical under both planning_tracking=manual and ignore.

How

A stage-agnostic exemption that fires only when the round has genuinely nothing to remediate. The naive "drop the done conjunct" approach breaks 7 fail-closed tests, so the predicate is precise:

  • input_mode=none and no source verification path, and
  • zero fail-classifications and zero registry known issues, and
  • the recorded files are all planning metadata (checked on the recorded paths directly — METADATA_ONLY_ROUND is corroboration-dependent and misreads non-metadata edits as metadata-only when no git evidence is available), and
  • the phase's authoritative verification resolves on disk and is a clean PASS with zero FAIL rows and zero pre-existing issues (new helper phase_baseline_is_clean).

Rounds that carry fail-classifications, carry known issues, recorded any non-metadata edit, have no phase verification on disk (a continuation round that lost its source), or whose phase verification carries pre-existing issues do not satisfy the predicate and still fail closed.

Scope: sub-defect B only. Sub-defect A (change-evidence under planning_tracking=manual) does not reproduce on current main — plan-amendment evidence is corroborated under both tracking modes and ROUND_CHANGE_EVIDENCE_UNAVAILABLE only routes REMEDIATION when ROUND_CODE_FIX_COUNT>0. Sub-defect C (deviation_override / accepted-deviation terminal disposition) is a separate concern needing a new verdict. See the issue thread for detail.

One existing test legitimately changed: a no-classification metadata-only round on a clean-baseline phase with a historical phase-root SUMMARY deviation now PROCEEDs — consistent with the gate's existing treatment of phase-root deviations as historical during remediation (they are not counted in the round-scoped deviation override).

Testing

  • New phase_baseline_is_clean helper + the exemption; bash -n + shellcheck -S warning clean.
  • tests/qa-result-gate.bats: the no-FAIL disposition round now PROCEEDs (was the loop); reachable phase_baseline_is_clean fail-closed branches covered (absent phase verification, non-PASS result, pre-existing issues); a non-metadata edit still fails closed. 187 ok / 0 fail.
  • Gate-adjacent suites (qa-remediation-state, two-phase-completion, hard-gates, hard-contracts, typed-protocol, validate-uat-remediation-artifact, uat-remediation-state, prepare-reverification, suggest-next-fix-uat, create-remediation-phase, phase0-bugfix-verify): 495 ok / 0 fail.
  • testing/run-lint.sh: clean.
  • Reproduction PROCEEDs at verify under both planning_tracking modes; all prior fail-closed cases remain REMEDIATION_REQUIRED.

Refs #655.

🤖 Generated with Claude Code

…wt-labs#655 sub-defect B)

The QA remediation gate had no clean terminal state for a phase that
legitimately has zero FAIL checks and zero code changes — a read-only
diagnosis / documentation-disposition round. `source_verification_missing`
was exempted for `input_mode=none` only at `_gate_stage=done`, but a round
must clear the `verify` stage to reach `done`. So such a round was flagged
`qa_gate_source_verification_missing=true` at `verify` and routed
`REMEDIATION_REQUIRED` forever; the only escape observed in the field was
manually advancing `verify -> done`.

Add a stage-agnostic exemption that fires only when the round genuinely has
nothing to remediate, using a precise predicate (not the corroboration-
dependent METADATA_ONLY_ROUND, which reads non-metadata edits as
metadata-only when no git evidence is available):

  - input_mode=none and no source verification path, AND
  - zero fail-classifications and zero registry known issues, AND
  - the recorded files are all planning metadata, AND
  - the phase's authoritative verification resolves on disk and is a clean
    PASS with zero FAIL rows and zero pre-existing/known issues
    (new helper `phase_baseline_is_clean`).

Rounds that carry fail-classifications, carry known issues, recorded any
non-metadata edit, have no phase verification on disk (a continuation round
that lost its source), or whose phase verification carries pre-existing
issues do NOT satisfy the predicate and still fail closed.

Scope: addresses sub-defect B only. Sub-defect A (change-evidence under
planning_tracking=manual) does not reproduce on current main — plan-amendment
evidence is corroborated under both tracking modes and CHANGE_EVIDENCE_
UNAVAILABLE only routes REMEDIATION when ROUND_CODE_FIX_COUNT>0. Sub-defect C
(deviation_override / accepted-deviation disposition) is a separate concern.

Tests:
- qa-result-gate.bats: the no-classification metadata-only round on a clean
  baseline now PROCEEDs (was the loop); a non-metadata edit on a clean
  baseline still fails closed. Existing fail-closed cases (missing phase
  verification, missing prior-round verification, carried known issues,
  classified process-exceptions) remain REMEDIATION_REQUIRED.

Refs swt-labs#655.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
shanemccarron-maker and others added 3 commits June 15, 2026 17:17
Addresses the findings from the round-1 QA review (read-only adversarial
review of the sub-defect B fix):

- phase_baseline_is_clean pre-existing-issues counter miscounted
  GitHub-style colon alignment rows and non-canonical header rows
  (would silently defeat the fix for those formats) — replaced with the
  header-state-machine pattern (skip alignment rows via [[:space:]:|-],
  treat the first non-separator row as the header regardless of titles).
- phase_baseline_is_clean result check was narrower than the gate's
  authoritative RESULT parser — now normalizes with toupper() and falls
  back to the legacy `status:` field.
- gate-level coverage for the reachable phase_baseline_is_clean
  fail-closed branches (absent phase verification, non-PASS result,
  pre-existing issues); removed a misconceived test for the
  count_fail==0 branch, which is unreachable through the gate because a
  phase verification with FAIL rows makes qa-remediation-state derive
  input_mode=verification (documented as defense-in-depth instead).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round 2 QA review came back clean — no blockers or major issues. The
round-1 fixes hold; 187/187 bats pass, shellcheck clean.

One minor, pre-existing, non-regressing observation noted and deferred:
`count_fail_rows_in_verification` keys on a literal "Status" column
header, so a self-contradictory verification (frontmatter result: PASS
but a FAIL row under a differently-named header) is read as 0 FAILs.
This is the established gate-wide parser (not introduced here), real
write-verification.sh output always uses the "Status" header, and the
direction is benign — out of scope for swt-labs#655.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round 3 QA review surfaced two minor, fail-safe findings; both addressed:

- Finding 2 (placeholder pre-existing table): `phase_baseline_is_clean`
  counted a placeholder "None"/"N/A" row in a `## Pre-existing Issues`
  table as a real carried issue, so a clean no-FAIL phase that renders an
  explicit empty table failed closed. The counter now skips placeholder
  rows (None / N/A / "no ... issues"), matching the placeholder-skipping
  convention used elsewhere in the gate. Fail-safe direction either way
  (could only have caused a spurious remediation, never a false PROCEED).

- Finding 1 (round-02+ coverage gap): all prior swt-labs#655 tests used round-01,
  leaving the new exemption's round>1 source-resolution path unverified.
  Added a round-02 no-classification clean-baseline test asserting
  PROCEED_TO_UAT, locking in the deliberate disposition broadening.

189/189 bats pass; shellcheck + bash -n clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@halindrome
halindrome force-pushed the fix/655-qa-gate-noncode-terminal branch from d7d7d39 to b1b935c Compare June 15, 2026 22:34
@halindrome

Copy link
Copy Markdown
Contributor Author

QA Review — Round 1 (against base fix fad36ef9; fixes in 723224af)

- Model used: Claude Opus 4.8 (read-only Code Reviewer agent, analysis-only)

- What was tested:
  • The new phase_baseline_is_clean helper and the new source-missing exemption in
    scripts/qa-result-gate.sh, plus the new "(issue #655)" tests in tests/qa-result-gate.bats.
  • The pre-existing-issues awk counter against varied table shapes (separators, headers).
  • Consistency of the helper's result check vs the gate's authoritative RESULT parser.
  • Reachability/coverage of each phase_baseline_is_clean branch through the gate.
  • set -euo pipefail safety; shellcheck -S warning.

- Expected vs actual:
  1. Pre-existing-issues counter — Expected: counts only real issue rows. Actual: a GitHub-style
     colon alignment row (|:---|:--:|) was counted as a data row, and the header was identified by
     hard-coded "Test"/"File" column titles, so non-canonical headers were counted too. On a clean
     phase using colon alignment, phase_baseline_is_clean returned false → the #655 loop would
     persist for that format (fail-safe, but defeats the fix).
  2. Result check — Expected: same PASS determination as the gate. Actual: the helper accepted only
     PASS|pass|Pass and read only `result:`; the gate's RESULT parser uppercases any casing and
     falls back to legacy `status:`. The same on-disk file could be PASS to the gate but "not clean"
     to the helper.
  3. Branch coverage — Expected: the helper's fail-closed branches are tested through the gate.
     Actual: only the happy-path PROCEED case reached the helper; the cited fail-closed tests
     short-circuit on an earlier guard, so the discriminating branches were untested, and a test
     comment overclaimed that coverage.
  4. 3-digit phase numbers — investigated; non-issue (resolution delegates to
     resolve-verification-path.sh; %02d is a minimum-width spec).

- Severity:
  • Finding 1: major  • Finding 2: minor  • Finding 3: major (coverage)  • Item 4: none

- Confirmed vs hypothetical:
  • Findings 1, 2, 3: CONFIRMED (reproduced live).  • Item 4: confirmed non-issue.

Addressed in 723224af (fix(qa-gate): address QA round 1): replaced the counter with the header-state-machine pattern (colon-aware separators, header-agnostic), normalized the result check (toupper + legacy status:), and added gate-level tests for the reachable fail-closed branches (removing a misconceived test for the unreachable count_fail==0 branch).

@halindrome

Copy link
Copy Markdown
Contributor Author

QA Review — Round 2 (fresh review against 723224af; clean — empty evidence commit 60684e88)

QA REVIEW — PR #657 (fix/655-qa-gate-noncode-terminal), Round 2

- Model used: Claude Opus 4.8 (read-only Code Reviewer agent)

- What was tested:
  • Commit narrative (round-1 fix on top of base fix) and full diff.
  • New helper phase_baseline_is_clean and its dependencies: count_fail_rows_in_verification,
    extract_frontmatter_scalar_value, paths_include_non_metadata, resolve-verification-path.sh.
  • New stage-agnostic source-missing exemption guard.
  • All new "(issue #655)" bats tests + helpers (_seed_clean_guard_round, create_verif,
    create_round_summary_with_files).
  • set -e safety of the &&-chained if; shellcheck -S warning; bash -n; full suite.
  • Mutation test: forced phase_baseline_is_clean to always-return-0 to verify the three
    fail-closed companion tests genuinely isolate the helper's branches.
  • Parser asymmetry between count_fail_rows_in_verification (Status-header-keyed) and the
    helper's header-agnostic pre-existing-issues counter.

- Expected vs actual:
  1. Helper correctness — Expected: fails closed on absent verification, non-PASS result, FAIL
     rows, pre-existing issues. Actual: matches. Mutation test confirms the companion tests flip
     to FAIL when the helper is neutered (so they aren't passing for an unrelated reason). No
     discrepancy.
  2. set -e safety — confirmed safe; full suite green. No discrepancy.
  3. Parser asymmetry — count_fail_rows_in_verification requires a literal "Status" column header;
     a verification with frontmatter result: PASS but a FAIL row under a differently-named header
     is seen as 0 FAILs. Benign: self-contradictory/malformed artifact, the helper reuses the
     established gate-wide parser, real write-verification.sh output always uses "Status". Not a
     regression introduced by this PR.
  4. Conventions (bash, set -euo pipefail, trailing newline, shellcheck) — no discrepancy.

- Severity: minor (finding 3 only); all others: no discrepancy.

- Confirmed vs hypothetical:
  • Finding 3: CONFIRMED reproducible, judged pre-existing/benign, not a defect of this PR.
  • Findings 1, 2, 4: CONFIRMED correct (mutation-tested / executed).

Verdict: Round 2 is CLEAN. No blockers. The only observation is a pre-existing, non-regressing
parser edge that does not warrant blocking.

Clean round → empty evidence commit 60684e88. The pre-existing "Status"-header parser edge is in the shared gate-wide count_fail_rows_in_verification, not introduced here, and is out of scope for #655 — deferred.

@halindrome

Copy link
Copy Markdown
Contributor Author

QA Review — Round 3 (fresh review against 60684e88; two minor fail-safe findings, both fixed in b1b935cb)

- Model used: Claude Opus 4.8 (read-only Code Reviewer agent)

- What was tested:
  • Full diff of the two changed files; no other files touched.
  • phase_baseline_is_clean and its Pre-existing-Issues awk counter against 6 table/prose variants.
  • The stage-agnostic exemption block and how it feeds routing (only sets
    ROUND_SOURCE_VERIFICATION_MISSING=false; PROCEED still requires RESULT=PASS + FAIL_COUNT=0).
  • Whether the exemption can manufacture a false-PROCEED (adversarial round-01 and round-02 cases;
    PR-vs-upstream/main comparison).
  • Whether the new bats tests assert what their names claim and pass for the right reason.
  • Interaction with multi-round (round-02+) remediation.
  • Full bats suite, shellcheck -S warning, bash -n.

- Expected vs actual:
  Finding 1 — Round-02+ no-classification disposition is a confirmed, untested behavioral broadening
  (not a false-PROCEED). A round-02 with input_mode=none, zero fail_classifications, zero known
  issues, all-metadata recorded files, on a clean PASS baseline now routes PROCEED_TO_UAT where
  upstream/main routed REMEDIATION_REQUIRED. Internally consistent with the fix's intent; every
  existing round-02 fail-closed test still passes (they carry fail_classifications). The gap is test
  coverage — all new tests use round-01. Recommend a round-02 no-classification clean-baseline test.

  Finding 2 — A placeholder "None" row in a Pre-existing Issues table (| None | - | - |) is counted
  as a real issue, so phase_baseline_is_clean returns false and the round fails closed. This is the
  safe direction (cannot cause a false-PROCEED) and is the same edge round 2 noted. Could cause a
  narrow spurious-remediation loop only if QA emits a placeholder-row pre-existing table on an
  otherwise clean phase.

  No other discrepancies. The exemption cannot fail open: it only clears the source-missing flag;
  all downstream PASS-path guards remain in force. fail_classifications, known-issues, any
  non-metadata recorded file, and absent/non-PASS/FAIL-bearing/pre-existing-issue phase verifications
  each independently keep the round failing closed (verified live).

- Severity: Finding 1 minor (missing coverage for an intended, fail-safe broadening); Finding 2 minor
  (fail-closed parser edge).

- Confirmed vs hypothetical: both CONFIRMED (reproduced live).

ROUND STATUS: CLEAN. No blocking or major issues. Two minor, non-blocking, fail-safe observations;
neither can produce a false-PROCEED.

Both findings addressed in b1b935cb (fix(qa-gate): address QA round 3): the pre-existing counter now skips placeholder rows (None / N/A / "no … issues"), and a round-02 no-classification clean-baseline test was added to lock in the deliberate disposition broadening. 189/189 bats pass.

@halindrome
halindrome marked this pull request as ready for review June 16, 2026 00:05
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