Skip to content

fix(qa-gate): resolve per-plan VERIFICATION.md for single-plan phases#656

Open
halindrome wants to merge 6 commits into
swt-labs:mainfrom
halindrome:fix/653-per-plan-verification-gate
Open

fix(qa-gate): resolve per-plan VERIFICATION.md for single-plan phases#656
halindrome wants to merge 6 commits into
swt-labs:mainfrom
halindrome:fix/653-per-plan-verification-gate

Conversation

@halindrome

Copy link
Copy Markdown
Contributor

What

Teach resolve-verification-path.sh to resolve a per-plan verification artifact ({NN}-{MM}-VERIFICATION.md) as a last-resort fallback when a phase has exactly one such file and no phase-level ({NN}-VERIFICATION.md), plain (VERIFICATION.md), or per-wave ({NN}-VERIFICATION-wave*.md) artifact exists.

Why

Closes #653. On a passing single-plan phase, the verification writer emits the per-plan name {NN}-{MM}-VERIFICATION.md, but the QA gate's resolver only knew about the phase-level, plain, and wave names. The gate therefore read qa_gate_writer=missing / qa_gate_result=missing and forced a spurious QA_RERUN_REQUIRED on a phase that had already passed — an extra QA cycle (cost + time) recoverable only by manually renaming the artifact.

Fixing the shared resolver fixes every consumer (phase / current / authoritative) and the QA gate in one place — a root-cause fix rather than patching the gate alone.

The issue's secondary git mv -k bug has no locus in the committed tree (there is no git mv anywhere in scripts/, hooks/, commands/, references/, or templates/) — it was a one-off manual-recovery action during the live incident, not committed workflow code. Verified via repo-wide grep.

How

phase_level_path() adopts a single per-plan {NN}-{MM}-VERIFICATION.md only when no higher-precedence artifact is present. Multiple per-plan files are ambiguous (the integration artifact is authoritative) and fall through to the canonical phase-level name, so multi-plan phases are unchanged.

Testing

  • tests/resolve-verification-path.bats — single per-plan adopted; multiple per-plan → phase-level (ambiguous); on-disk phase-level wins; wave files win. EXIT=0, 0 failures.
  • tests/qa-result-gate.bats — single-plan phase with only a per-plan VERIFICATION.md routes PROCEED_TO_UAT. EXIT=0, 0 failures.
  • bash -n + shellcheck -S warning on scripts/resolve-verification-path.sh — clean.
  • Full testing/run-all.sh: zero not ok bats and zero FAIL: contract/lint across the suite. (The agent-shutdown-integration.bats integration test passes cleanly in isolation — EXIT=0, 26 ok — but can hang under heavy local parallelism due to its sleep-based crashed-lock-holder simulation; unrelated to this change.)

🤖 Generated with Claude Code

shanemccarron-maker and others added 4 commits June 13, 2026 17:13
A passing single-plan phase whose QA artifact was written with the
per-plan name `{NN}-{MM}-VERIFICATION.md` was read by the gate as
missing: `resolve-verification-path.sh` only resolved the phase-level
`{NN}-VERIFICATION.md`, the brownfield plain `VERIFICATION.md`, or
per-wave `{NN}-VERIFICATION-wave*.md` — never the per-plan name. The
gate then reported `qa_gate_writer=missing` / `qa_gate_result=missing`
and forced a spurious `QA_RERUN_REQUIRED` on a phase that already
passed.

Teach `phase_level_path()` to adopt a per-plan verification as a
last-resort fallback when exactly one `{NN}-{MM}-VERIFICATION.md`
exists and no phase-level, plain, or wave-level artifact is present.
Multiple per-plan files are ambiguous (the integration artifact is
authoritative) and fall through to the canonical phase-level name, so
multi-plan phases are unchanged. Fixing the shared resolver fixes every
consumer (`phase`/`current`/`authoritative`) and the QA gate in one
place.

Tests:
- resolve-verification-path.bats: single per-plan adopted; multiple
  per-plan ambiguous -> phase-level; on-disk phase-level wins; wave
  files win.
- qa-result-gate.bats: single-plan phase with only a per-plan
  VERIFICATION.md routes PROCEED_TO_UAT.

The secondary `git mv -k` no-op on untracked artifacts noted in the
issue has no locus in the current tree (no `git mv` anywhere); it was a
live manual-recovery action, not committed code.

Closes swt-labs#653.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses the findings from the round-1 QA review (read-only adversarial
review of the per-plan VERIFICATION.md fix):

- The per-plan fallback glob `[0-9][0-9]` missed 3-digit plan numbers
  (`{NN}-100-VERIFICATION.md` and beyond); the writer formats plan
  numbers with `printf %02d`, so the 100th+ plan yields 3 digits and the
  artifact was read as missing — the same swt-labs#653 symptom at a different
  plan-number range. Widened to `[0-9][0-9]*` and replaced the
  `ls -1 ... | grep -c .` parse with a `nullglob` array (exact entry
  count, no `ls` parsing, no newline-in-filename ambiguity).
- Added coverage in resolve-verification-path.bats: the zero-artifact
  fallthrough regression guard and a 3-digit per-plan adoption test.
- Added the missing trailing newline to resolve-verification-path.bats.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round 2 QA review flagged the round-1 per-plan glob `[0-9][0-9]*` as
greedy: the trailing `*` also matched non-canonical names like
`{NN}-01a-VERIFICATION.md` and `{NN}-01-extra-VERIFICATION.md`, which
would then be adopted as the sole per-plan artifact. No writer in the
codebase emits such names (MM is always `printf %02d`, no suffix), so it
was latent rather than reachable — but loose.

Tighten it: keep the nullglob array as a cheap prefilter, then re-filter
with a strict regex `^{prefix}-[0-9][0-9]+-VERIFICATION\.md$` (>=2-digit,
all-numeric MM) so over-matches are rejected before the single-match
decision. 2- and 3-digit plan numbers are still adopted; non-numeric or
extra-segment names fall through to the canonical phase-level name.

Added a negative test asserting non-canonical per-plan names are not
adopted.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round 3 QA review came back fully clean — no findings at any severity.
Verified: the round-2 regex re-filter closes the greedy-glob over-match;
`phase_prefix` is provably digit-only (both derivation paths go through
`printf %02d` / a `^[0-9]+$` guard), so its interpolation into the
`[[ =~ ]]` ERE is injection-safe; the >=2-digit MM requirement,
`set -euo pipefail` behavior, nullglob save/restore, basename handling,
and precedence/ambiguity semantics all hold. shellcheck clean; both bats
suites green (21 + 184, zero failures).

Empty evidence commit per the QA review process (clean round).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@halindrome halindrome force-pushed the fix/653-per-plan-verification-gate branch from d414a7a to 97c29a4 Compare June 15, 2026 22:51
@halindrome

Copy link
Copy Markdown
Contributor Author

QA Review — Round 1 (against base fix eac1f12b; fixes in cc8f6a47)

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

- What was tested:
  • The new per-plan fallback in phase_level_path() (scripts/resolve-verification-path.sh)
    and its glob/precedence behavior; the new tests in resolve-verification-path.bats and
    the issue-#653 regression test in qa-result-gate.bats.
  • Zero-match behavior, single-digit vs multi-digit MM, ls-vs-glob, filenames with spaces.
  • shellcheck -S warning; full bats suites.

- Expected vs actual:
  1. Per-plan glob `[0-9][0-9]` — Expected: matches every writer-produced per-plan name.
     Actual: hardcodes exactly two MM digits; the writer formats plan numbers with
     `printf %02d`, so the 100th+ plan yields a 3-digit name (`{NN}-100-VERIFICATION.md`)
     that was NOT matched → the artifact was read as missing and the function returned the
     synthetic phase-level name the gate reads as missing — the exact #653 symptom at a
     different plan-number range.
  2. `ls -1 ... | grep -c .` to count/parse files — fragile vs filenames with newlines and
     contrary to the "prefer globs over parsing ls" convention.
  3. Test coverage — no test for the zero-per-plan-file fallthrough (the pre-existing
     behavior the new block must not disturb), nor for 3-digit MM adoption.
  4. Trailing newline — resolve-verification-path.bats lacked a trailing newline.

- Severity:
  • Finding 1: major  • Finding 2: minor  • Finding 3: minor  • Finding 4: nit

- Confirmed vs hypothetical:
  • Finding 1: CONFIRMED (reproduced — 3-digit name not matched).
  • Findings 2-4: CONFIRMED.

Addressed in cc8f6a47 (fix(qa-gate): address QA round 1): widened the glob to [0-9][0-9]* and replaced the ls | grep -c parse with a nullglob array; added the zero-artifact fallthrough regression guard and a 3-digit adoption test; added the trailing newline.

@halindrome

Copy link
Copy Markdown
Contributor Author

QA Review — Round 2 (fresh review against cc8f6a47; one minor finding, fixed in d1eebba9)

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

- What was tested:
  • The round-1 per-plan fallback block in phase_level_path(), the nullglob save/restore,
    the [0-9][0-9]* glob, and branch precedence (canonical-on-disk → plain → wave →
    single per-plan → canonical-synthetic).
  • Empirical runs against crafted phase dirs (over-match, wave, multi-plan, 3-digit,
    zero-artifact); shellcheck -S warning; bash -n; both full bats suites.
  • Cross-checked the writer (resolve-artifact-path.sh / write-verification.sh) and a
    repo-wide grep to determine whether over-match names can occur in practice.

- Expected vs actual:
  1. nullglob save/restore vs set -euo pipefail — Expected: no global state leak, no
     premature exit. Actual: correct; shopt -p captured + eval-restored, both `|| true`
     guarded; subprocess scope; zero-artifact returns exit 0. No discrepancy.
  2. `[0-9][0-9]*` over-match — Expected: matches only `{NN}-{MM}-VERIFICATION.md`. Actual:
     the trailing `*` is greedy, so `{NN}-01a-VERIFICATION.md` and
     `{NN}-01-extra-VERIFICATION.md` are ALSO adopted as a "sole per-plan" artifact
     (reproduced live). No writer in the codebase emits such names (MM is always
     `printf %02d`, no suffix; repo-wide grep found none) — latent looseness, not a
     reachable bug, but worth tightening.
  3. Branch precedence — phase-level and wave win over per-plan; multiple per-plan →
     phase-level. Verified. No discrepancy.
  4. Conventions (bash, set -euo pipefail, glob-not-ls, trailing newline) — compliant.

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

- Confirmed vs hypothetical:
  • Finding 2: CONFIRMED over-match; hypothetical as a real-world defect (no producer of
    such names). Recommend tightening + a negative test.
  • Findings 1, 3, 4: CONFIRMED correct.

Verdict: no blockers. One minor hardening recommendation (tighten the greedy glob).

Addressed in d1eebba9 (fix(qa-gate): address QA round 2): kept the nullglob array as a cheap prefilter, then re-filter with a strict regex ^{prefix}-[0-9][0-9]+-VERIFICATION\.md$ so over-matches are rejected before the single-match decision; added a negative test asserting non-canonical per-plan names are not adopted.

@halindrome

Copy link
Copy Markdown
Contributor Author

QA Review — Round 3 (fresh review against d1eebba9; fully clean — empty evidence commit 97c29a40)

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

- What was tested:
  • All 3 commits vs upstream/main; full diff of the 3 changed files.
  • The round-2 regex re-filter `^${phase_prefix}-[0-9][0-9]+-VERIFICATION\.md$`:
    ERE-injection safety of ${phase_prefix}, the >=2-digit requirement, set -euo pipefail
    / unset-var risk in the new loop, basename/array handling, precedence + multi-plan
    ambiguity after the re-filter.
  • Upstream phase_prefix provenance (resolve-artifact-path.sh) to judge injection safety.
  • Live execution across 9 edge cases; shellcheck -S warning + bash -n; both bats suites.

- Expected vs actual:
  1. ERE injection via ${phase_prefix} — CONFIRMED SAFE. Both derivation paths yield
     digits only: canonical_name from resolve-artifact-path.sh is `${PHASE_NUM}-VERIFICATION.md`
     with PHASE_NUM=`printf "%02d"`, and the fallback sets phase_prefix via `printf '%02d'`
     on a `^[0-9]+$`-guarded value. No metacharacter can reach the pattern.
  2. >=2-digit requirement — CONFIRMED. `[0-9][0-9]+` rejects 1-digit `01-1-...` (falls to
     phase-level); 2- and 3-digit adopted.
  3. set -euo pipefail / unset-var — CONFIRMED SAFE. Arrays initialized; no-match glob
     expands to nothing under nullglob; iteration over empty array is safe; shopt/eval
     `|| true`-guarded; zero-artifact returns rc=0.
  4. nullglob save/restore — CONFIRMED. Restored immediately after the single glob
     expansion, before any later globbing; subprocess scope.
  5. basename/array/precedence + ambiguity — CONFIRMED. Precedence canonical-on-disk →
     plain → wave → per-plan → synthetic; valid+noise re-filters to the single canonical
     match and adopts it; >1 per-plan → phase-level.

  No discrepancies found.

- Severity: none.
- Confirmed vs hypothetical: all CONFIRMED.

ROUND VERDICT: CLEAN. No blockers, suggestions, or nits. shellcheck clean; both bats suites
green (21 + 184, zero failures). Approve.

Clean round → empty evidence commit 97c29a40.

@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.

Spurious QA re-run: write-verification emits per-plan VERIFICATION.md but the QA gate expects phase-level

3 participants