From 2ea0a84f18a0ca827e7402b39ebb4ad9a70f01b9 Mon Sep 17 00:00:00 2001 From: Martin Bens Date: Sat, 5 Sep 2026 19:49:21 +0200 Subject: [PATCH 1/4] fix(test-writing): drop malformed defense stances instead of aborting The adversarial stage threw at the wave boundary when a defense reconciler listed an adopted finding under both `adopted_new` and `findings`, discarding every completed agent in the run. The duplicate is now skipped as an already-cast vote, and `defensePrompt` states explicitly that the four response arrays are disjoint, removing the ambiguity that produced the duplication. Beyond that fast path, any defense stance entry that fails an integrity guard now takes the degrade-by-role path from `references/error-handling.md` instead of throwing. The offending entry is dropped, the finding it named keeps its prior consensus binding, and each drop is recorded in `red_team.defense_degraded` so an incomplete defense wave is never rendered as a complete one. The defense fold also lost re-characterizations. `mergeRemediations` picks the descriptive owner by remediation length, so a defender correcting a finding's location, method, and remediation with a terser fix lost every corrected field back to the stale original. Maintained findings now merge through `recharacterize`, which makes the defender's payload the descriptive owner whenever it proposed a remediation. A payload without one keeps the original's fields so `current` and `suggested` still describe one change. Co-Authored-By: Claude Fable 5 --- .../references/error-handling.md | 3 +- .../workflow/team-review.workflow.mjs | 115 +++++++++++++++--- 2 files changed, 100 insertions(+), 18 deletions(-) diff --git a/plugins/test-writing/skills/phpunit-test-team-reviewing/references/error-handling.md b/plugins/test-writing/skills/phpunit-test-team-reviewing/references/error-handling.md index 02c93dc6..946f5729 100644 --- a/plugins/test-writing/skills/phpunit-test-team-reviewing/references/error-handling.md +++ b/plugins/test-writing/skills/phpunit-test-team-reviewing/references/error-handling.md @@ -71,7 +71,7 @@ When a wave of ≥ `WAVE_NULL_MIN` agents loses ≥ `WAVE_NULL_RATE` of them to ### Degrade by role (after re-spawn is exhausted) -Only after a unit burns `RESPAWN_MAX` does its role's graceful degradation apply: every loss is either covered (reviewer 2-of-2) or **loudly flagged** (adversary coverage gap). +Only after a unit burns `RESPAWN_MAX` does its role's graceful degradation apply: every loss is either covered (reviewer 2-of-2) or **loudly flagged** (adversary coverage gap). One row below is not a death: a defense stance that returns but fails an integrity guard loses the same voice a dead defender would, so it takes the same path — no re-spawn precedes it, because the agent did answer. | Dead role (after re-spawn) | Action | |---|---| @@ -80,6 +80,7 @@ Only after a unit burns `RESPAWN_MAX` does its role's graceful degradation apply | One lens adversary of a file | No action — the file is still covered by its other lens adversaries (a file needs ≥ 1 of its K adversaries to survive). | | **All K** lens adversaries of a file | Mark that file **un-red-teamed** and raise the `red_team` coverage-gap flag for it — never substitute peer stances as if adversarial coverage were complete. | | A defense reconciler | Keep that reviewer's peer stance; the adversary challenges have no effect on it. | +| A defense reconciler's stance that fails an integrity guard (an entry with no `finding_id`, or one quoting a `finding_id` that resolves to no known record) | Same path as a dead one, never a run failure — a throw at the wave boundary would discard every agent the run already completed. Drop the offending **entry** only; the rest of that stance still votes. The finding the entry named keeps the consensus binding the review stage gave it. Record each drop in `red_team.defense_degraded` (`null` when clean, else `{dropped: [...], note}` — the same shape as `coverage_gap`), naming the file, the defender, the `finding_id`, what the entry was, and the guard that refused it. A dropped promotion is credited in no metric: it moved nothing. | | The cross-file agent | Omit the consistency section; note it in the report. | | A single arbiter (should-fix / consider) | Leave the finding contested; do not include it in the body. | | All 3 arbiters of a contested must-fix | Leave the finding contested (still shown in the contested section). A *partial* vote with no majority keeps it in the body marked `split` — never silently dropped. | diff --git a/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/team-review.workflow.mjs b/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/team-review.workflow.mjs index d576b23e..7cd8abef 100644 --- a/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/team-review.workflow.mjs +++ b/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/team-review.workflow.mjs @@ -847,6 +847,28 @@ function unionRecords(base, other, locFirst = base, locSecond = other) { implies_src_change: base.implies_src_change === true || other.implies_src_change === true, }; } +// A defender's maintained finding is a RE-CHARACTERIZATION of the record it names: the same +// defect restated with a corrected location/method and a corrected remediation after the +// defender re-read the file. `mergeRemediations` picks its owner by remediation LENGTH, so a +// terser correction loses `location`/`method`/`current`/`summary` back to the stale original +// AND lands behind the original in `suggested_variants` — the report then prints the original's +// line next to the original's fix while the correction survives only as a variant nothing +// renders first. Field precedence is unchanged (owner first, paired record fallback); this +// only names the defender's payload as the owner whenever it actually proposed a remediation, +// which keeps the invariant that `current` and `suggested` describe ONE change. A defender that +// proposed no remediation owns nothing for its fields to pair with, so the original keeps them. +// `base` stays the original throughout: votes, consensus, dissent, outcome, arbitration and +// adversary_impact are the fold's, never the defender payload's. +function recharacterize(original, payload) { + const merged = unionRecords(original, payload, original, payload); + const lead = variantsOf(payload).map((s) => String(s)).find((s) => normText(s) !== ''); + if (lead === undefined) return merged; + return { + ...merged, ...descriptiveFrom(payload, original), + suggested: lead, + suggested_variants: [lead, ...merged.suggested_variants.filter((s) => normText(s) !== normText(lead))], + }; +} // A promotion's finding_id may already be a live record — the id can already sit in `kept` // (a defender re-cites a still-kept finding) or in `contested` (a withdrawal moved it there // earlier in this same fold). Either way it is one finding, never a duplicate: merge its @@ -1089,6 +1111,7 @@ function defensePrompt(file, label, consensus, challenges, subsetRules) { `STEP 1 — Invoke the Skill tool with skill="${RECONCILE_SKILL}" in ADVERSARY mode.`, 'Defend each consensus finding the adversary challenged (keep it only if the detection algorithm still holds), withdraw any the challenge overturned, re-adopt any resurrected finding the evidence supports, and adopt any adversary-introduced finding the majority should accept. Tag every entry with an adversary_impact. The ## RULES block holds only the rules under dispute; look up by ID — do NOT call get_rules.', 'Every finding, challenge, and resurrection in the payloads below carries a `finding_id`. Quote it verbatim on each withdrawal, re-adoption, maintained finding, and adopted adversary finding — never invent or alter one.', + 'The four arrays are DISJOINT: every finding_id you return appears in exactly ONE of `findings`, `withdrawn`, `re_adopted`, `adopted_new`. `findings` holds ONLY findings already listed in the "Current consensus findings" payload above that you are maintaining. An adversary-introduced finding you accept goes in `adopted_new` ALONE — do NOT also repeat it under `findings`. A resurrected finding you accept goes in `re_adopted` ALONE. A finding_id that is not in the consensus payload never belongs in `findings`.', '', `Current consensus findings for this file:\n${JSON.stringify(consensus, null, 1)}`, '', @@ -1815,6 +1838,33 @@ const advLandedIds = new Set(); // what makes `advLandedIds` a strict subset by construction. const landIfProposed = (key) => { if (advProposedIds.has(key)) advLandedIds.add(key); }; const coverageGapFiles = []; +// ---- Defense-stance integrity: degrade by role, never throw ---- +// A defense payload that fails an integrity guard is the same loss as a defense reconciler +// that died (error-handling.md, degrade-by-role): that voice does not count, the prior +// consensus binding stands, and the loss is recorded loudly rather than hidden. Throwing +// would instead discard every completed agent in the run over one agent's malformed entry — +// a whole-run failure for a single-agent fault, which the degrade-by-role table exists to +// prevent. The degradation cannot fabricate a result: it only ever removes a defender's own +// vote, so it can neither invent a finding nor move one, and every drop is reported in +// `red_team.defense_degraded`. +const defenseDrops = []; +function dropDefense(path, defender, findingId, guard, scope) { + defenseDrops.push({ path, defender, finding_id: findingId || null, scope, guard }); + log(`Wave 3: dropped ${scope} from defender ${defender} on ${path} — ${guard}; prior consensus binding kept`); +} +// Entry-level, never stance-level: each entry is an independent vote, so one malformed entry +// costs its own vote and nothing else. The guard logic stays single-sourced in +// `ingestFinding`/`requireFindingId` — restating their checks here to avoid the catch would +// drift from them silently — so their throw is caught at this one boundary and turned into +// the drop. This is the ONLY place a defense-stage guard failure is caught. +function keepValidEntries(list, validate, ctx, path, defender, scope) { + const kept = []; + for (const e of (list || [])) { + try { validate(e, ctx); kept.push(e); } + catch (err) { dropDefense(path, defender, (e && typeof e.finding_id === 'string' ? e.finding_id.trim() : '') || null, err.message, scope); } + } + return kept; +} const allAdvSignals = []; // ---- WAVE 2 — red team (per file × K lenses; full catalog) ---- @@ -1945,11 +1995,13 @@ if (defenseTasks.length > 0) { waveCheck('Wave 3: Defense', defenseRaw); if (HALT.halted) return partialResult({ files: [] }); defense = defenseRaw.filter(Boolean); + // Ingestion is where a defense payload's identity guards run, so it is also where a failed + // one degrades: the offending entry is dropped and recorded, the rest of the stance stands. for (const d of defense) { - ingestFindings(d.findings, `defense stance on ${d.path}`); - ingestFindings(d.re_adopted, `defense re-adoption on ${d.path}`); - ingestFindings(d.adopted_new, `defense adoption of an adversary finding on ${d.path}`); - assertFindingIds(d.withdrawn, `defense withdrawal on ${d.path}`); + d.findings = keepValidEntries(d.findings, ingestFinding, `defense stance on ${d.path}`, d.path, d.reviewer, 'maintained finding'); + d.re_adopted = keepValidEntries(d.re_adopted, ingestFinding, `defense re-adoption on ${d.path}`, d.path, d.reviewer, 're-adoption'); + d.adopted_new = keepValidEntries(d.adopted_new, ingestFinding, `defense adoption of an adversary finding on ${d.path}`, d.path, d.reviewer, 'adoption'); + d.withdrawn = keepValidEntries(d.withdrawn, requireFindingId, `defense withdrawal on ${d.path}`, d.path, d.reviewer, 'withdrawal'); } } else { log('Wave 3: no files drew actionable challenges — defense skipped'); } @@ -1960,8 +2012,10 @@ if (defenseTasks.length > 0) { // withdrawn original for a re-adoption of one peer reconciliation removed from both sets, // and the red team's own new_findings entry (identity-complete — REDTEAM_SCHEMA requires // `method` there) for an adoption. A quoted id resolving to none of them is a broken -// back-reference, not a new finding to invent identity for. -function resolveOriginal(c, id, newFindings, withdrawnOriginals, ctx) { +// back-reference, not a new finding to invent identity for. `null` says exactly that, and the +// caller degrades on it (drops the promotion, keeps the prior consensus binding, records the +// drop) instead of failing the whole run over one defender's broken quote. +function resolveOriginal(c, id, newFindings, withdrawnOriginals) { const k = c.kept.find((x) => x.finding_id === id); if (k) return k; const ct = c.contested.find((x) => x.finding_id === id); @@ -1970,7 +2024,7 @@ function resolveOriginal(c, id, newFindings, withdrawnOriginals, ctx) { if (w) return w; const nf = newFindings.get(id); if (nf) return nf; - throw new Error(`Promoted finding quotes finding_id ${id} that resolves to no known record — ${ctx}`); + return null; } // Fold defense into consensus (majority of 3 defenders per file). const overturnedMustFix = []; @@ -1989,20 +2043,24 @@ for (const c of consensus) { // defender's first — that ordering is what `locations` reads. `items` holds only the // vote-casting entry per defender: a repeat from a defender that already voted for this id // casts no second vote and is deliberately absent from the enforce tally. - const castVote = (map, id, rec, seen) => { + // `defenders` names who voted, read only when a promotion has to be dropped — the + // degradation record must name the defenders whose votes it discards, and the entries in + // `items`/`voices` carry no label of their own. + const castVote = (map, id, rec, seen, defender) => { let e = map.get(id); - if (!e) { e = { n: 0, items: [], voices: [] }; map.set(id, e); } + if (!e) { e = { n: 0, items: [], voices: [], defenders: [] }; map.set(id, e); } e.voices.push(rec); if (seen.has(id)) return; seen.add(id); e.n++; e.items.push(rec); + if (!e.defenders.includes(defender)) e.defenders.push(defender); }; for (const d of defs) { const seenW = new Set(), seenA = new Set(), seenR = new Set(); - for (const w of (d.withdrawn || [])) castVote(withdrawVotes, requireFindingId(w, `defense withdrawal on ${c.path}`), w, seenW); - for (const a of (d.adopted_new || [])) castVote(adoptVotes, requireFindingId(a, `defense adoption on ${c.path}`), a, seenA); - for (const r of (d.re_adopted || [])) castVote(readoptVotes, requireFindingId(r, `defense re-adoption on ${c.path}`), r, seenR); + for (const w of (d.withdrawn || [])) castVote(withdrawVotes, requireFindingId(w, `defense withdrawal on ${c.path}`), w, seenW, d.reviewer); + for (const a of (d.adopted_new || [])) castVote(adoptVotes, requireFindingId(a, `defense adoption on ${c.path}`), a, seenA, d.reviewer); + for (const r of (d.re_adopted || [])) castVote(readoptVotes, requireFindingId(r, `defense re-adoption on ${c.path}`), r, seenR, d.reviewer); // `findings` is a defender's maintained stance on an existing kept/contested record — // its `adversary_impact` is `defended`/`unchanged`, never `introduced`, so it casts no // vote and moves nothing between kept and contested. Its remediation still merges in, @@ -2015,9 +2073,17 @@ for (const c of consensus) { // implies_src_change is OR'd across both records by unionRecords itself. for (const f of (d.findings || [])) { const fid = requireFindingId(f, `defense maintained finding on ${c.path}`); + // This defender already voted to adopt or re-adopt this id, so the entry here is a + // duplicate of that vote rather than a second stance. The promotion fold owns the id — + // it resolves the original and merges every voter's remediation — so skipping the + // duplicate loses nothing, and it avoids resolving an adopted red-team finding (which + // lives in neither kept nor contested) against this file's records. + if (seenA.has(fid) || seenR.has(fid)) continue; const rec = c.kept.find((k) => k.finding_id === fid) || c.contested.find((k) => k.finding_id === fid); - if (!rec) throw new Error(`Defense-maintained finding quotes finding_id ${fid} that resolves to no known record — defense maintained finding on ${c.path}`); - Object.assign(rec, unionRecords(rec, f, rec, f)); + // Degrade rather than throw: an unresolvable back-reference costs this one entry, and + // the record it names keeps the binding the review stage gave it. + if (!rec) { dropDefense(c.path, d.reviewer, fid, 'quoted finding_id resolves to no kept or contested record on this file', 'maintained finding'); continue; } + Object.assign(rec, recharacterize(rec, f)); } } c.kept = c.kept.filter((k) => { @@ -2045,7 +2111,13 @@ for (const c of consensus) { const newFindings = newFindingsByPath.get(c.path) || new Map(); const withdrawnOriginals = withdrawnOriginalsByPath.get(c.path) || new Map(); const promote = (id, v, ctx, impact) => { - const orig = resolveOriginal(c, id, newFindings, withdrawnOriginals, ctx); + const orig = resolveOriginal(c, id, newFindings, withdrawnOriginals); + // Nothing to promote from: the id names no consensus entry, no persisted withdrawn + // original and no red-team new finding, so there is no identity to give the promoted + // record. Drop the promotion — the finding stays exactly where the review stage left it — + // and record it; inventing identity here would put a finding in the body that no reviewer + // and no adversary can be shown to have raised. + if (!orig) { dropDefense(c.path, v.defenders.join(', ') || 'unknown', id, `quoted finding_id resolves to no known record — ${ctx}`, 'promotion'); return false; } // Every defender entry for this id — the voting ones and the repeats that cast no // second vote — contributes its remediation, location and src-change flag, merged in // the order the entries were seen (`v.voices`) rather than all votes ahead of all @@ -2064,14 +2136,18 @@ for (const c of consensus) { consensus: 'majority', adversary_impact: impact, implies_src_change: orig.implies_src_change === true || voices.some((it) => it.implies_src_change === true), }); + return true; }; + // A dropped promotion moved nothing, so it counts in neither the red-team metric nor the + // change_rate numerator — crediting it would report a finding as adopted that is not in the + // result. for (const [id, v] of adoptVotes) if (v.n >= 2) { - promote(id, v, `adoption on ${c.path}`, 'introduced'); + if (!promote(id, v, `adoption on ${c.path}`, 'introduced')) continue; redTeamMetrics.new_findings_adopted++; landIfProposed(`${c.path}|new|${id}`); } for (const [id, v] of readoptVotes) if (v.n >= 2) { - promote(id, v, `re-adoption on ${c.path}`, 'resurrected'); + if (!promote(id, v, `re-adoption on ${c.path}`, 'resurrected')) continue; redTeamMetrics.resurrections++; landIfProposed(`${c.path}|resurrection|${id}`); } @@ -2213,6 +2289,11 @@ const red_team = { new_findings_adopted: redTeamMetrics.new_findings_adopted, change_rate: advProposed === 0 ? null : Math.round((advLanded / advProposed) * 100), coverage_gap: uniqueCoverageGap.length ? { files: uniqueCoverageGap, note: 'in-scope files left un-red-teamed after re-spawn — adversary coverage is incomplete' } : null, + // Same shape and same purpose as `coverage_gap`: a null-when-clean record of what the run + // could not process, so a degraded defense wave is never rendered as a complete one. Each + // entry names the file, the defender(s) whose vote was discarded, the finding_id quoted, + // what the entry was, and the guard that refused it. + defense_degraded: defenseDrops.length ? { dropped: defenseDrops, note: 'defense stance entries dropped for a failed integrity guard — the prior consensus binding was kept for the findings they named; the defense wave is incomplete for those findings' } : null, }; const advOutputTokens = outputTokensNow(); log(`Adversarial verdict (mode=adversarial): ${advOverall} | ${allFileResults.filter((f) => f.status !== 'PASS').length}/${FILES.length} files with issues | ${redTeamMetrics.challenges_made} challenge(s), ${redTeamMetrics.challenges_overturned} overturned, ${redTeamMetrics.new_findings_adopted} new finding(s) adopted | ${adaptation.arbiters} arbiter(s) | ${advSrcChange.length} src-change escalation(s) | ${agentsSpawned} agents spawned | ${advOutputTokens == null ? 'n/a' : Math.round(advOutputTokens / 1000) + 'k'} output tokens`); From 0c5bc19b3ece6276758f02de0fbf4de106e8c116 Mon Sep 17 00:00:00 2001 From: Martin Bens Date: Sat, 5 Sep 2026 19:59:40 +0200 Subject: [PATCH 2/4] fix(test-writing): add deterministic manifest and evidence gates The team-review pipeline trusted two agent-reported values it can verify itself. Extraction subagents' `method_count` and `test_methods` drove the track decision and shard scoping unchecked, so one wrong count silently misrouted a file. `verify-method-counts.sh` now re-extracts the method names from disk before the manifest freezes, replaces mismatches with the extracted truth, logs each replacement, and fails hard on a corrupted entry instead of repairing it. A reviewer finding's `current` block was adjudicated by consensus, the red team, and the report without anyone checking the quoted code exists. `verify-finding-evidence.sh` runs over each persisted shard result before the merge. A kept finding whose non-empty `current` does not occur in the target file under whitespace normalization moves to `contested` with an outcome naming the failed match, and the demotion is synced into `adversarial_input`, so a fabricated quote never reaches the merge, the red team, or the report as kept. A finding that quoted no code is exempt. The rendered report now labels every finding with the scrutiny it actually received, `adversary-tested` or `consensus-only`, derived from which stage produced the finding's final state. The new `fix-application.md` reference carries the contract for whoever applies a report's remediations: apply `suggested` verbatim, self-review the fix diff for redundancy and tautological tests plus the static gates, judge mutants against the whole suite only, re-verify the premise of a `consensus-only` must-fix finding before applying it, and confirm reported commits with `git merge-base --is-ancestor` before calling them landed. Both gate scripts validate their input shape per entry and are covered by 18 BATS tests in plugin-tests/test-writing, wired into the skill's Phase 1 and Phase 5 with matching digraph nodes. Co-Authored-By: Claude Fable 5 --- .../test-writing/verify_finding_evidence.bats | 189 ++++++++++++++ .../test-writing/verify_method_counts.bats | 154 +++++++++++ plugins/test-writing/AGENTS.md | 4 +- plugins/test-writing/README.md | 2 +- .../phpunit-test-team-reviewing/SKILL.md | 23 +- .../references/fix-application.md | 28 ++ .../references/input-resolution.md | 2 +- .../references/report-format.md | 16 +- .../workflow/verify-finding-evidence.sh | 243 ++++++++++++++++++ .../workflow/verify-method-counts.sh | 174 +++++++++++++ 10 files changed, 824 insertions(+), 11 deletions(-) create mode 100644 plugin-tests/test-writing/verify_finding_evidence.bats create mode 100644 plugin-tests/test-writing/verify_method_counts.bats create mode 100644 plugins/test-writing/skills/phpunit-test-team-reviewing/references/fix-application.md create mode 100755 plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/verify-finding-evidence.sh create mode 100755 plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/verify-method-counts.sh diff --git a/plugin-tests/test-writing/verify_finding_evidence.bats b/plugin-tests/test-writing/verify_finding_evidence.bats new file mode 100644 index 00000000..2e7463b6 --- /dev/null +++ b/plugin-tests/test-writing/verify_finding_evidence.bats @@ -0,0 +1,189 @@ +#!/usr/bin/env bats +# bats file_tags=test-writing,team-review,verify-finding-evidence +# Tests for verify-finding-evidence.sh — the merge/adversarial-stage gate that +# checks every kept finding's `current` block against the reviewed file's real +# content (whitespace-normalized substring containment) and demotes a finding +# that quotes code the file does not contain from its kept bucket into +# `contested`, rather than letting a fabricated quote reach the report. +bats_require_minimum_version 1.11.0 + +load 'test_helper/common_setup' + +setup() { + WORKFLOW_DIR="${PLUGIN_DIR}/skills/phpunit-test-team-reviewing/workflow" + SCRIPT="${WORKFLOW_DIR}/verify-finding-evidence.sh" + REPO_DIR="${BATS_TEST_TMPDIR}/repo" + mkdir -p "${REPO_DIR}/tests/unit" + # shellcheck source=/dev/null # SCRIPT is derived from PLUGIN_DIR at runtime + source "${SCRIPT}" + + _write_bar_test_file +} + +# The fixture file every test's finding is checked against: a real test +# method containing one real assertion. +_write_bar_test_file() { + { + echo " "${REPO_DIR}/tests/unit/BarTest.php" +} + +# Write a one-file result JSON with a single finding of the given `current` +# in the given kept bucket (errors|warnings|informational). +_write_result() { + local path="$1" bucket="$2" current="$3" + jq -n --arg bucket "${bucket}" --arg current "${current}" \ + '{files: [{path: "tests/unit/BarTest.php", + errors: [], warnings: [], informational: [], contested: []} + | .[$bucket] = [{finding_id: "CONV-001|testReal", rule_id: "CONV-001", current: $current}]]}' \ + > "${path}" +} + +# ============================================================================ +# Happy path — a finding whose `current` is a real substring of the file is +# left in place; a finding with empty `current` is exempt from the check. +# ============================================================================ + +@test "keeps a finding whose current matches the file exactly" { + _write_result "${BATS_TEST_TMPDIR}/result.json" errors 'static::assertSame(1, $x);' + + run --separate-stderr verify_finding_evidence "${BATS_TEST_TMPDIR}/result.json" "${REPO_DIR}" + assert_success + assert_equal "${stderr}" "" + + run jq -c '.files[0] | {errors: (.errors | length), contested: (.contested | length)}' <<< "${output}" + assert_output '{"errors":1,"contested":0}' +} + +@test "exempts a finding with empty current from the check" { + jq -n '{files: [{path: "tests/unit/BarTest.php", + errors: [{finding_id: "TEAM-SPLIT|class-level", rule_id: "TEAM-SPLIT", current: ""}], + warnings: [], informational: [], contested: []}]}' > "${BATS_TEST_TMPDIR}/result.json" + + run --separate-stderr verify_finding_evidence "${BATS_TEST_TMPDIR}/result.json" "${REPO_DIR}" + assert_success + assert_equal "${stderr}" "" + + run jq -c '.files[0] | {errors: (.errors | length), contested: (.contested | length)}' <<< "${output}" + assert_output '{"errors":1,"contested":0}' +} + +# ============================================================================ +# Whitespace normalization — a quote reformatted with different indentation +# or line breaks still passes, since both sides collapse whitespace runs to +# a single space before the containment check. +# ============================================================================ + +@test "matches under whitespace normalization despite different indentation" { + _write_result "${BATS_TEST_TMPDIR}/result.json" errors "$(printf 'static::assertSame(1,\n $x);')" + + run --separate-stderr verify_finding_evidence "${BATS_TEST_TMPDIR}/result.json" "${REPO_DIR}" + assert_success + assert_equal "${stderr}" "" + + run jq -c '.files[0] | {errors: (.errors | length), contested: (.contested | length)}' <<< "${output}" + assert_output '{"errors":1,"contested":0}' +} + +# ============================================================================ +# Demotion — a fabricated quote (code the file does not contain) is moved +# from its kept bucket into `contested`, tagged with an `outcome` reason, and +# logged to stderr. +# ============================================================================ + +@test "demotes a fabricated quote into contested with an outcome and a log line" { + _write_result "${BATS_TEST_TMPDIR}/result.json" errors 'static::assertSame(999, $doesNotExist);' + + run --separate-stderr verify_finding_evidence "${BATS_TEST_TMPDIR}/result.json" "${REPO_DIR}" + assert_success + assert_equal "${stderr}" "verify-finding-evidence: demoted CONV-001|testReal in tests/unit/BarTest.php: current block not found under whitespace normalization" + + run jq -c '.files[0] | {errors: (.errors | length), contested}' <<< "${output}" + assert_output --partial '"errors":0' + assert_output --partial '"finding_id":"CONV-001|testReal"' + assert_output --partial '"outcome":"evidence check: current block not found in tests/unit/BarTest.php under whitespace normalization"' +} + +@test "syncs a demotion into adversarial_input, moving it from kept to contested there too" { + jq -n '{files: [{path: "tests/unit/BarTest.php", + errors: [{finding_id: "CONV-001|testReal", rule_id: "CONV-001", current: "static::assertSame(999, $doesNotExist);"}], + warnings: [], informational: [], contested: [], + adversarial_input: { + kept: [{finding_id: "CONV-001|testReal", rule_id: "CONV-001", current: "static::assertSame(999, $doesNotExist);"}], + contested: [] + }}]}' > "${BATS_TEST_TMPDIR}/result.json" + + run --separate-stderr verify_finding_evidence "${BATS_TEST_TMPDIR}/result.json" "${REPO_DIR}" + assert_success + assert_equal "${stderr}" "verify-finding-evidence: demoted CONV-001|testReal in tests/unit/BarTest.php: current block not found under whitespace normalization" + + run jq -c '.files[0].adversarial_input | {kept: (.kept | length), contested}' <<< "${output}" + assert_output --partial '"kept":0' + assert_output --partial '"finding_id":"CONV-001|testReal"' + assert_output --partial '"outcome":"evidence check: current block not found in tests/unit/BarTest.php under whitespace normalization"' +} + +# ============================================================================ +# Per-entry shape validation — a bucket that is not an array (e.g. `errors: {}`) +# is a corrupted result, not zero candidates to skip past: it must fail hard +# and name the entry and the offending field, never silently pass through as +# a clean review. +# ============================================================================ + +@test "fails hard when errors is an object instead of an array, rather than silently passing through" { + jq -n '{files: [{path: "tests/unit/BarTest.php", errors: {}, warnings: [], informational: [], contested: []}]}' \ + > "${BATS_TEST_TMPDIR}/result.json" + + run verify_finding_evidence "${BATS_TEST_TMPDIR}/result.json" "${REPO_DIR}" + assert_failure + assert_output --partial "entry 0" + assert_output --partial "tests/unit/BarTest.php" + assert_output --partial "errors is not an array" +} + +@test "demotes from the warnings bucket the same way as errors" { + _write_result "${BATS_TEST_TMPDIR}/result.json" warnings 'this code does not exist anywhere' + + run --separate-stderr verify_finding_evidence "${BATS_TEST_TMPDIR}/result.json" "${REPO_DIR}" + assert_success + + run jq -c '.files[0] | {warnings: (.warnings | length), contested: (.contested | length)}' <<< "${output}" + assert_output '{"warnings":0,"contested":1}' +} + +# ============================================================================ +# Hard failures — a target file that does not exist, or invalid result JSON, +# both abort with a non-zero exit and a clear message; never a silent skip. +# ============================================================================ + +@test "fails hard when the referenced file does not exist on disk" { + jq -n '{files: [{path: "tests/unit/NoSuch.php", + errors: [{finding_id: "CONV-001|testX", rule_id: "CONV-001", current: "something"}], + warnings: [], informational: [], contested: []}]}' > "${BATS_TEST_TMPDIR}/result.json" + + run verify_finding_evidence "${BATS_TEST_TMPDIR}/result.json" "${REPO_DIR}" + assert_failure + assert_output --partial "target file not found" +} + +@test "fails hard on invalid result JSON" { + printf '%s\n' '{bad json' > "${BATS_TEST_TMPDIR}/result.json" + + run verify_finding_evidence "${BATS_TEST_TMPDIR}/result.json" "${REPO_DIR}" + assert_failure + assert_output --partial "not valid JSON" +} + +@test "fails hard when the result has no top-level files array" { + printf '%s\n' '{}' > "${BATS_TEST_TMPDIR}/result.json" + + run verify_finding_evidence "${BATS_TEST_TMPDIR}/result.json" "${REPO_DIR}" + assert_failure + assert_output --partial "no top-level" +} diff --git a/plugin-tests/test-writing/verify_method_counts.bats b/plugin-tests/test-writing/verify_method_counts.bats new file mode 100644 index 00000000..e6617f66 --- /dev/null +++ b/plugin-tests/test-writing/verify_method_counts.bats @@ -0,0 +1,154 @@ +#!/usr/bin/env bats +# bats file_tags=test-writing,team-review,verify-method-counts +# Tests for verify-method-counts.sh — the Phase-1 manifest gate that +# deterministically re-counts each entry's test methods from disk before the +# manifest freezes, replacing a subagent-reported mismatch with the extracted +# truth rather than merely warning about it. +bats_require_minimum_version 1.11.0 + +load 'test_helper/common_setup' + +setup() { + WORKFLOW_DIR="${PLUGIN_DIR}/skills/phpunit-test-team-reviewing/workflow" + SCRIPT="${WORKFLOW_DIR}/verify-method-counts.sh" + REPO_DIR="${BATS_TEST_TMPDIR}/repo" + mkdir -p "${REPO_DIR}/tests/unit" + # shellcheck source=/dev/null # SCRIPT is derived from PLUGIN_DIR at runtime + source "${SCRIPT}" +} + +# Write a minimal test-class fixture with the given (bare) test method names. +_write_test_class() { + local path="$1" + shift + { + echo " "${path}" +} + +_write_manifest() { + local path="$1" test_file="$2" method_count="$3" methods_json="$4" + jq -n --arg path "${test_file}" --argjson count "${method_count}" --argjson methods "${methods_json}" \ + '[{path: $path, method_count: $count, test_methods: $methods}]' > "${path}" +} + +# ============================================================================ +# Happy path — the manifest already matches the extracted truth: no change, +# no stderr log line. +# ============================================================================ + +@test "leaves a matching entry unchanged and logs nothing" { + _write_test_class "${REPO_DIR}/tests/unit/FooTest.php" testAlpha testBeta testGamma + _write_manifest "${BATS_TEST_TMPDIR}/manifest.json" "tests/unit/FooTest.php" 3 '["testAlpha","testBeta","testGamma"]' + + run --separate-stderr verify_method_counts "${BATS_TEST_TMPDIR}/manifest.json" "${REPO_DIR}" + assert_success + assert_equal "${stderr}" "" + + run jq -c '.[0] | {method_count, test_methods}' <<< "${output}" + assert_output '{"method_count":3,"test_methods":["testAlpha","testBeta","testGamma"]}' +} + +# ============================================================================ +# Mismatch — a subagent under- or over-counted; the script replaces both +# fields with the extracted truth and logs exactly one line naming the file, +# the old count, and the new count. +# ============================================================================ + +@test "corrects a mismatched entry, logs one line, and writes the extracted truth" { + _write_test_class "${REPO_DIR}/tests/unit/FooTest.php" testAlpha testBeta testGamma + _write_manifest "${BATS_TEST_TMPDIR}/manifest.json" "tests/unit/FooTest.php" 2 '["testAlpha","testBeta"]' + + run --separate-stderr verify_method_counts "${BATS_TEST_TMPDIR}/manifest.json" "${REPO_DIR}" + assert_success + assert_equal "${stderr}" "verify-method-counts: tests/unit/FooTest.php: method_count 2 -> 3" + + run jq -c '.[0] | {method_count, test_methods}' <<< "${output}" + assert_output '{"method_count":3,"test_methods":["testAlpha","testBeta","testGamma"]}' +} + +# ============================================================================ +# Hard failures — a missing entry file, or invalid manifest JSON, both abort +# with a non-zero exit and a clear message; never a silent skip. +# ============================================================================ + +@test "fails hard when an entry's file cannot be read, rather than reporting a false zero count" { + _write_test_class "${REPO_DIR}/tests/unit/FooTest.php" testAlpha testBeta + chmod 000 "${REPO_DIR}/tests/unit/FooTest.php" + if [[ -r "${REPO_DIR}/tests/unit/FooTest.php" ]]; then + skip "running as a user that bypasses file permissions (e.g. root) — chmod 000 did not deny read" + fi + _write_manifest "${BATS_TEST_TMPDIR}/manifest.json" "tests/unit/FooTest.php" 2 '["testAlpha","testBeta"]' + + run verify_method_counts "${BATS_TEST_TMPDIR}/manifest.json" "${REPO_DIR}" + assert_failure + assert_output --partial "grep failed reading" + + chmod 644 "${REPO_DIR}/tests/unit/FooTest.php" +} + +@test "fails hard when an entry's file does not exist on disk" { + _write_manifest "${BATS_TEST_TMPDIR}/manifest.json" "tests/unit/NoSuchTest.php" 1 '["testX"]' + + run verify_method_counts "${BATS_TEST_TMPDIR}/manifest.json" "${REPO_DIR}" + assert_failure + assert_output --partial "entry file not found" +} + +@test "fails hard on invalid manifest JSON" { + printf '%s\n' '{bad json' > "${BATS_TEST_TMPDIR}/manifest.json" + + run verify_method_counts "${BATS_TEST_TMPDIR}/manifest.json" "${REPO_DIR}" + assert_failure + assert_output --partial "not valid JSON" +} + +@test "fails hard when the manifest top level is not an array" { + printf '%s\n' '{}' > "${BATS_TEST_TMPDIR}/manifest.json" + + run verify_method_counts "${BATS_TEST_TMPDIR}/manifest.json" "${REPO_DIR}" + assert_failure + assert_output --partial "not an array" +} + +# ============================================================================ +# Order-sensitivity — a reordered-but-identical-set test_methods list is still +# a mismatch: the extracted file-order list is the truth, not merely the set. +# ============================================================================ + +# ============================================================================ +# Per-entry shape validation — a wrongly-typed field (method_count as a +# string, test_methods as an object) is a corrupted manifest, not a mismatch +# to "correct": it must fail hard and name the entry and the offending field, +# never silently pass through with method_count 0. +# ============================================================================ + +@test "fails hard when method_count is a string and test_methods is an object, rather than silently correcting" { + _write_test_class "${REPO_DIR}/tests/unit/FooTest.php" testAlpha testBeta testGamma + jq -n '[{path: "tests/unit/FooTest.php", method_count: "three", test_methods: {}}]' > "${BATS_TEST_TMPDIR}/manifest.json" + + run verify_method_counts "${BATS_TEST_TMPDIR}/manifest.json" "${REPO_DIR}" + assert_failure + assert_output --partial "entry 0" + assert_output --partial "tests/unit/FooTest.php" + assert_output --partial "method_count is not a number" + assert_output --partial "test_methods is not an array" +} + +@test "corrects a reordered test_methods list to file order even though the set matches" { + _write_test_class "${REPO_DIR}/tests/unit/FooTest.php" testAlpha testBeta + _write_manifest "${BATS_TEST_TMPDIR}/manifest.json" "tests/unit/FooTest.php" 2 '["testBeta","testAlpha"]' + + run --separate-stderr verify_method_counts "${BATS_TEST_TMPDIR}/manifest.json" "${REPO_DIR}" + assert_success + assert_equal "${stderr}" "verify-method-counts: tests/unit/FooTest.php: method_count 2 -> 2" + + run jq -c '.[0].test_methods' <<< "${output}" + assert_output '["testAlpha","testBeta"]' +} diff --git a/plugins/test-writing/AGENTS.md b/plugins/test-writing/AGENTS.md index ccb2691c..0654afa7 100644 --- a/plugins/test-writing/AGENTS.md +++ b/plugins/test-writing/AGENTS.md @@ -76,7 +76,9 @@ plugins/test-writing/ │ ├── SKILL.md │ ├── workflow/team-review.workflow.mjs # committed parameterized Workflow script (reads its manifest from `const manifest = args;`) │ ├── workflow/build-run-script.sh # splices the on-disk manifest into a flat run-script; launched via scriptPath (no args) - │ └── references/{input-resolution,workflow-design,agent-guardrails,reviewer-allocation,red-team-context,consensus-and-verdicts,report-format,error-handling}.md + │ ├── workflow/verify-method-counts.sh # deterministic Phase-1 gate: re-counts test methods, corrects manifest entries + │ ├── workflow/verify-finding-evidence.sh # deterministic Phase-5 gate: demotes findings whose `current` block is not in the file + │ └── references/{input-resolution,workflow-design,agent-guardrails,reviewer-allocation,red-team-context,consensus-and-verdicts,report-format,error-handling,fix-application}.md ├── phpunit-migration-test-generation/ │ ├── SKILL.md │ ├── references/{source-analysis,output-format}.md diff --git a/plugins/test-writing/README.md b/plugins/test-writing/README.md index 573d76aa..77ba3789 100644 --- a/plugins/test-writing/README.md +++ b/plugins/test-writing/README.md @@ -511,7 +511,7 @@ Reference files provide detailed guidance: - **Output format**: `skills/phpunit-unit-test-reviewing/references/output-format.md` - **Report formats**: `skills/phpunit-unit-test-writing/references/report-formats.md` - **Oscillation handling**: `skills/phpunit-unit-test-writing/references/oscillation-handling.md` -- **Team review**: `skills/phpunit-test-team-reviewing/references/` (input-resolution, workflow-design, agent-guardrails, reviewer-allocation, red-team-context, consensus-and-verdicts, report-format, error-handling) +- **Team review**: `skills/phpunit-test-team-reviewing/references/` (input-resolution, workflow-design, agent-guardrails, reviewer-allocation, red-team-context, consensus-and-verdicts, report-format, error-handling, fix-application) - **Reconciling**: `skills/phpunit-test-reconciling/references/` (reconciliation-rules, output-format) ### Rule Files diff --git a/plugins/test-writing/skills/phpunit-test-team-reviewing/SKILL.md b/plugins/test-writing/skills/phpunit-test-team-reviewing/SKILL.md index a267021f..09d603ee 100644 --- a/plugins/test-writing/skills/phpunit-test-team-reviewing/SKILL.md +++ b/plugins/test-writing/skills/phpunit-test-team-reviewing/SKILL.md @@ -19,6 +19,7 @@ digraph team_review { "Abort: no valid test files" [shape=octagon, style=filled, fillcolor=red]; "Fan out per-file extraction (parallel haiku subagents)" [shape=box]; "Resolve ambiguous entries (AskUserQuestion)" [shape=box]; + "Verify method counts (workflow/verify-method-counts.sh)" [shape=box]; "Project agent cost (dry-run workflow) + select preset/models" [shape=box]; "Build shard plan (per-file weights, S_max)" [shape=box]; "Assemble campaign dir (campaign.json, args per shard + signals)" [shape=box]; @@ -27,6 +28,7 @@ digraph team_review { "Shard partial or failed?" [shape=diamond]; "Stop campaign: report completed shards + resume policy" [shape=octagon, style=filled, fillcolor=red]; "More shards?" [shape=diamond]; + "Verify finding evidence per shard result (workflow/verify-finding-evidence.sh)" [shape=box]; "Merge: verdicts + coverage map + placement flags" [shape=box]; "Adversarial gate: run red team?" [shape=diamond]; "Launch adversarial run (mode=adversarial); persist result" [shape=box]; @@ -39,7 +41,8 @@ digraph team_review { "File list empty?" -> "Abort: no valid test files" [label="yes"]; "File list empty?" -> "Fan out per-file extraction (parallel haiku subagents)" [label="no"]; "Fan out per-file extraction (parallel haiku subagents)" -> "Resolve ambiguous entries (AskUserQuestion)"; - "Resolve ambiguous entries (AskUserQuestion)" -> "Project agent cost (dry-run workflow) + select preset/models"; + "Resolve ambiguous entries (AskUserQuestion)" -> "Verify method counts (workflow/verify-method-counts.sh)"; + "Verify method counts (workflow/verify-method-counts.sh)" -> "Project agent cost (dry-run workflow) + select preset/models"; "Project agent cost (dry-run workflow) + select preset/models" -> "Build shard plan (per-file weights, S_max)"; "Build shard plan (per-file weights, S_max)" -> "Assemble campaign dir (campaign.json, args per shard + signals)"; "Assemble campaign dir (campaign.json, args per shard + signals)" -> "Launch signals run (mode=signals, background)"; @@ -48,7 +51,8 @@ digraph team_review { "Shard partial or failed?" -> "Stop campaign: report completed shards + resume policy" [label="yes"]; "Shard partial or failed?" -> "More shards?" [label="no"]; "More shards?" -> "Launch next review shard (mode=review); persist result" [label="yes"]; - "More shards?" -> "Merge: verdicts + coverage map + placement flags" [label="no"]; + "More shards?" -> "Verify finding evidence per shard result (workflow/verify-finding-evidence.sh)" [label="no"]; + "Verify finding evidence per shard result (workflow/verify-finding-evidence.sh)" -> "Merge: verdicts + coverage map + placement flags"; "Merge: verdicts + coverage map + placement flags" -> "Adversarial gate: run red team?"; "Adversarial gate: run red team?" -> "Launch adversarial run (mode=adversarial); persist result" [label="run"]; "Adversarial gate: run red team?" -> "Render combined report" [label="skip"]; @@ -68,6 +72,8 @@ Then build each file's entry **in parallel**: spawn one `general-purpose` subage Aggregate the returned entries. For every entry flagged `ambiguous`, resolve it with `AskUserQuestion` and refill its fields from the answer — a guessed source size silently flips the track decision, so nothing ambiguous may reach the run. Let N = number of files. +Before the manifest freezes, `Write` the aggregated entries to a manifest-core JSON file and run `${CLAUDE_SKILL_DIR}/workflow/verify-method-counts.sh `; replace the manifest with its stdout. A subagent-reported `method_count`/`test_methods` mismatch is corrected to the extracted truth and logged to stderr — never merely a warning (references/input-resolution.md §Per-File Extraction). A missing entry file or invalid manifest JSON fails the script hard; treat it as an input-resolution failure (references/error-handling.md). + Output: a manifest of validated entries, each with `test_type`, method scope (`methods`, plus the diff-touched `changed_methods` on diff runs), the full `test_methods` list, resolved `source_path`/`source_paths`, decomposition measurements (`test_lines`, `source_lines`, `method_count`), `fingerprint`, a `digest` when combined lines exceed the threshold, and `baseline` (`pass`/`fail`/`unavailable`, supplied with the manifest — `unavailable` when not supplied; this skill does not execute tests to obtain it) (references/input-resolution.md). ## Phase 2: Project the Cost, Select the Preset, Build the Shard Plan @@ -125,9 +131,10 @@ Build each stage's run-script with `${CLAUDE_SKILL_DIR}/workflow/build-run-scrip When all shards completed, merge on disk — no agents: -1. **Combined verdicts.** Concatenate the shard results' `files` arrays; aggregate `kept_findings`, `contested_findings`, and `concession_rate` (weighted by each shard's `wave0` finding keys) from the shard summaries. -2. **SUT-coverage map.** Join every manifest entry's `source_paths` to its test path; report each SUT covered by ≥ 2 test files as `{ sut, covered_by: [{path, test_type}], note }`, noting `integration test redundant with existing unit coverage of this SUT` when the covering set mixes unit and integration (references/report-format.md §Coverage Map). -3. **Placement flags.** Flag an integration file when (a) its merged result carries an `INTEGRATION-008` informational finding, and/or (b) the coverage map shows it redundant with unit coverage. Each flag points at `phpunit-integration-to-unit-migrating` and never raises status. +1. **Verify finding evidence.** For every persisted `$CAMPAIGN/shard-k.result.json`, run `${CLAUDE_SKILL_DIR}/workflow/verify-finding-evidence.sh $CAMPAIGN/shard-k.result.json ` and overwrite the file with its stdout. A kept finding whose `current` fails the evidence check is moved into `contested` (tagged with an `outcome` reason) and synced out of that file's `adversarial_input.kept` — so a fabricated quote never reaches this merge, Phase 6's `args-adversarial.json`, or the report as kept. A referenced file missing on disk or an invalid result JSON fails the script hard; treat it as a stage result failure (references/error-handling.md). +2. **Combined verdicts.** Concatenate the (now evidence-checked) shard results' `files` arrays; aggregate `kept_findings`, `contested_findings`, and `concession_rate` (weighted by each shard's `wave0` finding keys) from the shard summaries. +3. **SUT-coverage map.** Join every manifest entry's `source_paths` to its test path; report each SUT covered by ≥ 2 test files as `{ sut, covered_by: [{path, test_type}], note }`, noting `integration test redundant with existing unit coverage of this SUT` when the covering set mixes unit and integration (references/report-format.md §Coverage Map). +4. **Placement flags.** Flag an integration file when (a) its merged result carries an `INTEGRATION-008` informational finding, and/or (b) the coverage map shows it redundant with unit coverage. Each flag points at `phpunit-integration-to-unit-migrating` and never raises status. Render the consensus-stage report section now (report-format.md) — it survives even if the adversarial stage never runs. @@ -138,7 +145,7 @@ The adversarial stage (red team + defense + arbitration) is the opus-priced part 1. Aggregate the shard summaries' `adversarial_gate` signals. If every shard recommends skip (zero kept findings, or concession ≥ 50%), recommend skipping. 2. Present the gate as an `AskUserQuestion`: kept/contested totals, the skip signals, and the chosen preset's `adversarial_agents_bound` from the Phase-2 projection. Default to run when findings exist and no skip signal fired. 3. On **skip**: the consensus-stage results are final; go to step 5. -4. On **run**: assemble `$CAMPAIGN/args-adversarial.json` — `mode: "adversarial"`, ALL files, the same `rule_packages` / `preset` / `models` / `base`, plus `consensus`: the array of every file's `adversarial_input` object extracted from the shard results (`jq`, by path). Build, launch, persist to `$CAMPAIGN/adversarial.result.json` with the same stop-on-partial policy as Phase 4. +4. On **run**: assemble `$CAMPAIGN/args-adversarial.json` — `mode: "adversarial"`, ALL files, the same `rule_packages` / `preset` / `models` / `base`, plus `consensus`: the array of every file's `adversarial_input` object extracted from the shard results (`jq`, by path) — Phase 5 already ran `verify-finding-evidence.sh` over these shard results, so a demoted finding is already out of `adversarial_input.kept` before this extraction. Build, launch, persist to `$CAMPAIGN/adversarial.result.json` with the same stop-on-partial policy as Phase 4. ## Phase 7: Render the Report `Read` references/report-format.md and render the combined report from the persisted stage results. The stage results carry fields only — every heading, label, and field line comes from that template. Render: per-file verdicts (the adversarial result's `files` supersede the consensus-stage entries for files it processed), the signals result's `consistency` and `adoption_opportunities`, the Phase-5 coverage map and placement flags, and the per-stage cost lines (each stage result's `agents_spawned` and `output_tokens`). @@ -147,6 +154,10 @@ Each file's section states that file entry's `baseline` directly under its `## F Every finding heading is exactly `#### [RULE-ID] Title`. Consensus, provenance (`adversary_impact`), branch scope (`branch_touched`), arbitration and source-change status are field lines under the heading, never heading suffixes; a finding carrying more than one `suggested_variants` entry renders each under its own numbered `- **Suggested Fix**` entry (report-format.md §Per-finding render conventions). +Every finding also carries a **Scrutiny** field line: `adversary-tested` when its file's findings passed through the adversarial stage's superseding verdicts, `consensus-only` otherwise — derived deterministically from which stage produced the file's final per-finding state, never asserted independently per finding (report-format.md §Per-finding render conventions). + +This review has no fix phase — it only reports. For applying a report's remediations, `Read` references/fix-application.md. + ## Error Handling For input-resolution failures, stage start-up or run failures, partial results (`partial: true` + `halted_at`), the campaign stop/resume policy, and consensus edge cases, `Read` references/error-handling.md. diff --git a/plugins/test-writing/skills/phpunit-test-team-reviewing/references/fix-application.md b/plugins/test-writing/skills/phpunit-test-team-reviewing/references/fix-application.md new file mode 100644 index 00000000..cb335a3e --- /dev/null +++ b/plugins/test-writing/skills/phpunit-test-team-reviewing/references/fix-application.md @@ -0,0 +1,28 @@ +# Fix-Application Contract + +Team review is read-only; it never applies a remediation itself. This contract governs whoever applies a team-review report's remediations in a fix phase. + +## Remediation Fidelity + +- Apply a finding's `suggested` field verbatim — the report's final value for that finding (the adversarial stage's, when it superseded the file's consensus-stage entry; the consensus stage's otherwise) — never a paraphrase, never a re-derivation from `current` and `summary`. +- A finding's remaining `suggested_variants` entries are alternatives for a human to choose between. Never apply one as a remediation in its place, and never drop it from what you show the human. + +## Self-Review Before Committing + +Before committing the fix diff, run a scoped self-review over it: + +- **Redundancy.** Apply DESIGN-003 (data-provider consolidation for 3+ similar variations) and DESIGN-004 (unjustified case/method redundancy) across every file the fix touches together, not only the file the finding named. +- **Tautology.** For every test the fix adds, check that the assertion's expected value does not derive from the code under test or from the test's own fixture computation. An expected value computed the same way the SUT computes it passes on a broken SUT. +- **Static gates.** Run PHPStan (`mcp__plugin_dev-tooling_php-tooling__phpstan_analyze`) and php-cs-fixer (`mcp__plugin_dev-tooling_php-tooling__ecs_check` / `ecs_fix`) over the fix diff — the same gates the reviewed code passes through. + +## Mutation Checks + +A mutation-based check judges a mutant against the whole test suite, never against only the single test under discussion. + +## Re-Verifying a Consensus-Only Must-Fix Finding + +A finding whose `scrutiny` is `consensus-only` (report-format.md) and whose `enforce` is `must-fix` gets its factual premise re-verified — does the quoted defect exist as described in `current`, against the file on disk, right now — before its remediation is applied. A finding whose `scrutiny` is `adversary-tested` already survived that scrutiny in the adversarial stage and does not repeat this check. + +## Verifying Landed Commits Before Reporting Completion + +Before reporting the fix done, verify every commit the report names as landed is an ancestor of the branch it names: `git merge-base --is-ancestor `. A failed check reports that branch/commit pair as unverified — never as landed. diff --git a/plugins/test-writing/skills/phpunit-test-team-reviewing/references/input-resolution.md b/plugins/test-writing/skills/phpunit-test-team-reviewing/references/input-resolution.md index 5e7feb26..babf9af9 100644 --- a/plugins/test-writing/skills/phpunit-test-team-reviewing/references/input-resolution.md +++ b/plugins/test-writing/skills/phpunit-test-team-reviewing/references/input-resolution.md @@ -35,7 +35,7 @@ The orchestrator resolves the file list (Resolution Strategies) and classifies e **Procedure (this one file only):** run Diff-to-Method Resolution, Post-Resolution Validation & Per-Type Source Resolution, and Decomposition Measurement (below); then compute the cross-file `fingerprint` and, when `test_lines + source_lines > 800` (the fixed digest floor — the lowest preset `C`; see workflow-design.md §Pre-Run Collect), the body-free `digest` (both defined in workflow-design.md §Pre-Run Collect). Compute the digest at this fixed floor regardless of the run's selected preset, so any preset's `L > C` digest-track files have a digest. Return the Output entry. **Hard rules:** -- Counts come from `wc -l` / exhaustive `grep`, never estimation. Enumerate **every** `public function test*` into `test_methods` — this list drives the shard count. +- Counts come from `wc -l` / exhaustive `grep`, never estimation. Enumerate **every** `public function test*` into `test_methods` — this list drives the shard count. These subagent-reported `method_count`/`test_methods` values are provisional: before the manifest freezes, the skill re-counts them deterministically from the file on disk via `workflow/verify-method-counts.sh` (SKILL.md Phase 1), replacing a mismatch with the extracted truth and logging the replacement — never merely a warning. - `baseline` is not produced by this extraction. It is an input to the review, not an action this contract performs: no step here runs the file's tests. The orchestrator attaches each entry's `baseline` from the value supplied with the manifest, defaulting to `unavailable` when none is supplied. - **Emit every path repo-relative.** `path`, `source_path`, and every `source_paths` entry should be relative to the repository root — forward slashes, no leading `./`, no absolute prefix. Compute the relative form explicitly, e.g. `realpath --relative-to="$(git rev-parse --show-toplevel)" `. Downstream string-keyed joins (the cross-cutting coverage map, the adoption signal) key on these strings, so one SUT spelled absolute in one entry and relative in another would split into two identities and drop the coverage overlap. The workflow canonicalizes any path under `src/`/`tests/` to repo-relative as a safety net and aborts pre-launch only on a path it cannot resolve under those roots — but emit repo-relative directly so the manifest and report read cleanly. - Apply the narrow/keep change-impact gate (Diff-to-Method Resolution step 6) when a diff touches `setUp`/`tearDown`, a private helper, a data provider, or a class property: keep `methods: []` (full-class) by default; narrow to changed + added ONLY when the change is backward-compatible with no rule-relevant shape change; uncertain ⇒ keep (fail-safe). diff --git a/plugins/test-writing/skills/phpunit-test-team-reviewing/references/report-format.md b/plugins/test-writing/skills/phpunit-test-team-reviewing/references/report-format.md index 55c28c8f..dde3f8e3 100644 --- a/plugins/test-writing/skills/phpunit-test-team-reviewing/references/report-format.md +++ b/plugins/test-writing/skills/phpunit-test-team-reviewing/references/report-format.md @@ -79,6 +79,7 @@ Model combos ({model_combos}) change cost, not agent count. The bounds are upper #### [CONV-001] Title - **Method**: `testRendersLabel` · `ProductTest.php:45` (method is the stable locator; line is a hint that drifts) - **Consensus**: UNANIMOUS +- **Scrutiny**: adversary-tested - **Provenance**: UNCHANGED - **Branch scope**: n/a - **Arbitration**: none @@ -96,6 +97,7 @@ Model combos ({model_combos}) change cost, not agent count. The bounds are upper #### [DESIGN-003] Title - **Method**: `testAppliesDiscount` · `ProductTest.php:78` - **Consensus**: MAJORITY +- **Scrutiny**: consensus-only - **Provenance**: UNCHANGED - **Branch scope**: untouched (the diff did not touch this method — `branch_touched: false`) - **Arbitration**: none @@ -106,6 +108,7 @@ Model combos ({model_combos}) change cost, not agent count. The bounds are upper #### [DESIGN-005] Title - **Method**: `testHandlesNullCustomer` · `ProductTest.php:72` - **Consensus**: MAJORITY +- **Scrutiny**: adversary-tested - **Provenance**: ADVERSARY_RESURRECTED (an adversary resurrected it after peer reconciliation withdrew it) - **Branch scope**: n/a - **Arbitration**: none @@ -116,6 +119,7 @@ Model combos ({model_combos}) change cost, not agent count. The bounds are upper #### [UNIT-001] Title - **Method**: `testPrivateHelper` · `ProductTest.php:90` - **Consensus**: MAJORITY +- **Scrutiny**: adversary-tested - **Provenance**: UNCHANGED - **Branch scope**: n/a - **Arbitration**: confirmed — contested 1-of-3; arbiter confirmed: "reasoning" @@ -125,6 +129,7 @@ Model combos ({model_combos}) change cost, not agent count. The bounds are upper #### [DESIGN-002] Title - **Method**: `testComputesTotal` · `ProductTest.php:120` - **Consensus**: MAJORITY +- **Scrutiny**: adversary-tested - **Provenance**: UNCHANGED - **Branch scope**: n/a - **Arbitration**: split (needs human judgment) — contested must-fix; 3 adversary-tier arbiters reached no majority (e.g. 1 confirmed / 1 refuted / 1 uncertain, of 3): "reasoning". A `split` finding stays in the body for a human to settle, never silently dropped. @@ -156,6 +161,7 @@ Findings reported by only 1 reviewer, or refuted by an arbiter (excluded from ab #### [RULE-ID] Title - **Method**: `testComputesTotal` · `ProductTest.php:120` - **Consensus**: CONTESTED +- **Scrutiny**: consensus-only | adversary-tested - **Provenance**: UNCHANGED - **Branch scope**: touched | untouched | n/a - **Arbitration**: none | refuted — "reasoning" @@ -163,7 +169,7 @@ Findings reported by only 1 reviewer, or refuted by an arbiter (excluded from ab - **Removed assertions**: `static::assertSame(0, $cart->getPrice())` → covered by `testComputesEmptyTotal` - **Reported by**: reviewer-{n} - **Reason**: "why they flagged it" -- **Outcome**: not flagged by reviewer-{a}, reviewer-{b} / arbiter refuted: "reasoning" +- **Outcome**: not flagged by reviewer-{a}, reviewer-{b} / arbiter refuted: "reasoning" / evidence check: current block not found in `ProductTest.php` under whitespace normalization (a `verify-finding-evidence.sh` demotion — the finding's `current` did not match the file's real content) - **Current Code**: ```php // problematic code @@ -249,6 +255,9 @@ Findings whose fix cannot be made in the test alone — they imply a production > [!CAUTION] > **Adversary coverage gap.** In-scope files left un-red-teamed after re-spawn — adversary coverage is incomplete: {red_team.coverage_gap.files}. (Render only when `red_team.coverage_gap` is set.) +> [!CAUTION] +> **Defense wave degraded.** Defense stance entries were dropped for a failed integrity guard; the prior consensus binding was kept for the findings they named: {per red_team.defense_degraded.dropped entry: `{path}` — {defender}, `{finding_id}`, {guard}}. (Render only when `red_team.defense_degraded` is set.) + _Adversarial stage was skipped: {gate signal (zero kept findings / concession ≥ 50%) or the user's gate decision}_ (only when skipped — the per-file verdicts are then the consensus-stage results) --- @@ -268,6 +277,7 @@ Apply to every finding (errors, warnings, informational, contested): - **The heading is the defect, nothing else** — a finding's heading is exactly `#### [RULE-ID] Title`. Consensus, provenance, branch scope, arbitration, and source-change status are field lines under it; no heading suffix carries any of them. - **Consensus on every finding** — render `**Consensus**: UNANIMOUS | MAJORITY | CONTESTED` (from `consensus`) on ALL findings, not just the high-severity ones, and keep the `Contested Findings` section. Do not collapse the convention bulk into bare location lists — a contested CONV finding must read differently from a unanimous one. +- **Scrutiny on every finding** — render `**Scrutiny**: adversary-tested | consensus-only` on ALL findings. `adversary-tested` when the file's findings passed through the adversarial stage's superseding verdicts (an `adversarial.result.json` file entry replaced the consensus-stage one, per the Merge rule above); `consensus-only` otherwise (the adversarial stage was skipped, or never reached this file). Derived deterministically from which stage produced the finding's final state — never asserted independently per finding. - **Method-primary locator** — render `**Method**: \`testName\` · \`File:line\``. The method is the stable locator; the `:line` is a drift-prone hint. `method` is `class-level` for whole-class/structural findings. - **Provenance** — render `**Provenance**:` holding the finding's `adversary_impact` value, upper-cased: `UNCHANGED`, `DEFENDED`, `OVERTURNED`, `ADVERSARY_RESURRECTED` (`resurrected`), `ADVERSARY_INTRODUCED` (`introduced`). - **Branch scope** — render `**Branch scope**: touched` when `branch_touched` is `true`, `untouched` when `false`, `n/a` when `null` (non-diff run, or a `class-level` finding). A modified file is reviewed full-class so ripple is covered, so `untouched` findings are expected and this field is the triage signal. @@ -310,6 +320,7 @@ files: branch_touched: true | false | null # diff-scoped runs: is method in changed_methods? null = non-diff / class-level implies_src_change: false # true when the fix needs a production src/ change consensus: unanimous|majority + scrutiny: consensus-only | adversary-tested # adversary-tested iff this file's entry came from adversarial.result.json (Merge rule above); derived at merge/render, never a field a reviewer or adversary sets adversary_impact: unchanged|defended|overturned|resurrected|introduced arbitration: null | {verdict: confirmed|refuted|uncertain|split, reasoning} # split = contested must-fix, no arbiter majority, kept for human judgment summary: "what the defect is" # the Issue text the reviewing sub-skills render; names every line `current` holds and `suggested` drops @@ -325,7 +336,7 @@ files: dissent: null | {reviewer: reason} warnings: [...] informational: [...] - contested: [...] + contested: [...] # an entry's `outcome` may name a `verify-finding-evidence.sh` demotion: "evidence check: current block not found in under whitespace normalization" consensus: unanimous: {count} majority: {count} @@ -386,6 +397,7 @@ red_team: # from the adversarial stage new_findings_adopted: {count} # per adopted finding, deduped change_rate: {pct} | null # integer percentage, computed from its OWN deduped sets, not from the counters above (those count different units and a ratio over them would be fabricated). Denominator: distinct findings the red team put to the defense wave, keyed (file, kind, finding_id) so K lenses raising one finding count once. Numerator: those the defense moved — overturned (all, not just must-fix), re-adopted, or adopted — counted only when the red team proposed that same key, since the defense wave may also withdraw a finding nobody challenged. The numerator set is therefore a strict subset of the denominator set and the result cannot exceed 100. null (never 0) when the red team proposed nothing at all coverage_gap: null | {files: [...], note: "in-scope files left un-red-teamed after re-spawn — adversary coverage is incomplete"} + defense_degraded: null | {dropped: [{path, defender, finding_id, scope, guard}], note: "defense stance entries dropped for a failed integrity guard — the prior consensus binding was kept for the findings they named; the defense wave is incomplete for those findings"} adaptation: extra_peer_pass_reviewers: {count} extra_reviewers_by_file: {ProductTest.php: 2} diff --git a/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/verify-finding-evidence.sh b/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/verify-finding-evidence.sh new file mode 100755 index 00000000..a932b1aa --- /dev/null +++ b/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/verify-finding-evidence.sh @@ -0,0 +1,243 @@ +#!/usr/bin/env bash +# verify-finding-evidence.sh — deterministic check that every kept finding +# quotes code that exists. +# +# A persisted review-stage result's `files[]` entries carry findings in +# `errors`/`warnings`/`informational` (collectively "kept" — the consensus- +# stage or adversarial-stage disposition) and `contested`. This script checks +# every kept finding whose `current` is non-empty against the reviewed file's +# actual content: whitespace-normalize both (collapse whitespace runs to a +# single space, trim), then test literal substring containment. A finding that +# fails the check is moved from its kept bucket into `contested`, tagged with +# an `outcome` field naming the failed match — the same field the merge uses +# to record why a contested finding did not reach consensus (see +# report-format.md's Contested Findings render: "arbiter refuted: reasoning"). +# A finding with empty or absent `current` is exempt (nothing to verify). +# +# Every kept finding also sits, in the same review-stage result, inside its +# file's `adversarial_input.kept` (the raw, unbucketed payload the campaign's +# adversarial stage consumes to build args-adversarial.json) — the same +# records `errors`/`warnings`/`informational` bucket by enforce level. A +# demotion is therefore synced there by `finding_id`, not re-checked: a +# finding demoted from the top-level buckets is also removed from +# `adversarial_input.kept` and appended to `adversarial_input.contested`, so +# it never reaches the red team either. A file entry with no `adversarial_input` +# field is left without one. +# +# Usage: verify-finding-evidence.sh +# result.json a persisted review/adversarial-stage result JSON +# (shard-k.result.json or adversarial.result.json), carrying +# a top-level `files` array +# repo_root repo root of the project under review; each file entry's +# `path` is resolved relative to it +# +# Writes the corrected result JSON to stdout. Writes one line per demotion to +# stderr: "verify-finding-evidence: demoted in : current +# block not found under whitespace normalization". Exit 0 on success (a +# demotion is a successful run, not a partial one). Non-zero, with a message +# on stderr, when a file entry naming a kept finding to verify does not exist +# on disk, or when the input is not valid JSON. +# +# Sourcing this file defines the functions without running main, so a bats +# suite can exercise them directly. Functions use explicit `|| return 1` so +# they fail correctly whether or not errexit is active in the sourcing shell. + +# The jq program applied per file entry. Reads $entry (the file's JSON object), +# $content (the raw target-file text, via --rawfile), and $path (the entry's +# `path`, for the outcome message). Emits { updated, demotions }: `updated` is +# the file entry with demoted findings moved into `contested`; `demotions` is +# a list of { finding_id, path } for the stderr log lines. +_VERIFY_FINDING_EVIDENCE_JQ_PROGRAM=$(cat <<'EOF' +def norm: gsub("[[:space:]]+"; " ") | gsub("^ +| +$"; ""); +def checkBucket(arr): + reduce arr[] as $f ({kept: [], demoted: []}; + if (($f.current // "") == "") then + .kept += [$f] + elif (($content | norm) | contains($f.current | norm)) then + .kept += [$f] + else + .demoted += [$f] + end + ); +def tagOutcome: . + {outcome: ("evidence check: current block not found in " + $path + " under whitespace normalization")}; +($entry.errors // []) as $errs +| ($entry.warnings // []) as $warns +| ($entry.informational // []) as $infos +| checkBucket($errs) as $E +| checkBucket($warns) as $W +| checkBucket($infos) as $I +| ($E.demoted + $W.demoted + $I.demoted) as $demoted +| ($demoted | map(.finding_id)) as $demotedIds +| ($demoted | map(tagOutcome)) as $demotedTagged +| ($entry.adversarial_input.kept // []) as $aiKeptOrig +| ($entry.adversarial_input.contested // []) as $aiContestedOrig +| ($aiKeptOrig | map(select(.finding_id as $fid | $demotedIds | index($fid)))) as $aiDemoted +| ($aiKeptOrig | map(select(.finding_id as $fid | ($demotedIds | index($fid)) | not))) as $aiKeptRemaining +| ($entry + | .errors = $E.kept + | .warnings = $W.kept + | .informational = $I.kept + | .contested = (($entry.contested // []) + $demotedTagged) + | if ($entry | has("adversarial_input")) then + .adversarial_input.kept = $aiKeptRemaining + | .adversarial_input.contested = ($aiContestedOrig + ($aiDemoted | map(tagOutcome))) + else . end + ) as $updated +| {updated: $updated, demotions: [$demotedTagged[] | {finding_id: (.finding_id // "unknown"), path: $path}]} +EOF +) + +# The jq program applied to the whole result document by assert_valid_file_entries. Emits one +# line per invalid `files[]` entry: "entry ('>): ; ...". +# Checks only what the emitted result shape guarantees (report-format.md / the workflow's +# `bucketFile`/`contestedView`): `errors`/`warnings`/`informational`/`contested` are always +# present arrays in both mode=review and mode=adversarial output, so absence itself is not +# flagged here — only a present-but-wrongly-typed bucket or finding is. `adversarial_input` +# is present only in mode=review output, so it is checked only when present. +_VERIFY_FINDING_EVIDENCE_ENTRY_CHECK_JQ=$(cat <<'EOF' +def path_errors($v): + if ($v | has("path") | not) then ["path missing"] + elif ($v.path | type) != "string" then ["path is not a string"] + elif ($v.path | length) == 0 then ["path is empty"] + else [] end; +def bucket_type_errors($v; $field): + if ($v | has($field) | not) then [] + elif ($v[$field] | type) != "array" then ["\($field) is not an array"] + else [] end; +def finding_errors($v; $field): + if ($v | has($field) | not) then [] + elif ($v[$field] | type) != "array" then [] + else + [ ($v[$field] | to_entries[]) as $e + | ($e.value) as $f + | ( + (if ($f | has("finding_id") | not) then ["\($field)[\($e.key)].finding_id missing"] + elif ($f.finding_id | type) != "string" then ["\($field)[\($e.key)].finding_id is not a string"] + else [] end) + + + (if ($f | has("current")) and (($f.current | type) != "string") then ["\($field)[\($e.key)].current is not a string"] + else [] end) + )[] + ] + end; +def adversarial_input_errors($v): + if ($v | has("adversarial_input") | not) then [] + else + (bucket_type_errors($v.adversarial_input; "kept") | map("adversarial_input." + .)) + + (bucket_type_errors($v.adversarial_input; "contested") | map("adversarial_input." + .)) + end; +.files +| to_entries[] as $e +| ($e.key) as $idx | ($e.value) as $v +| ( + path_errors($v) + + bucket_type_errors($v; "errors") + bucket_type_errors($v; "warnings") + + bucket_type_errors($v; "informational") + bucket_type_errors($v; "contested") + + finding_errors($v; "errors") + finding_errors($v; "warnings") + finding_errors($v; "informational") + + adversarial_input_errors($v) + ) as $errs +| select(($errs | length) > 0) +| "entry \($idx) (\($v.path // "")): " + ($errs | join("; ")) +EOF +) + +# assert_valid_file_entries +# Fail (return 1) unless every `files[]` entry has a non-empty string `path`; +# `errors`/`warnings`/`informational`/`contested`, when present, are arrays; +# every finding inside the three kept buckets has a string `finding_id` (and a +# string `current` when present); and, when present, `adversarial_input.kept` +# and `adversarial_input.contested` are arrays. A wrongly-typed bucket (e.g. +# `errors: {}`) means the result is corrupted — this is a hard failure, never +# a silent pass-through. +assert_valid_file_entries() { + local result_file="$1" + local violations + violations=$(jq -r "${_VERIFY_FINDING_EVIDENCE_ENTRY_CHECK_JQ}" -- "${result_file}") || return 1 + if [[ -n "${violations}" ]]; then + local line + while IFS= read -r line; do + printf 'verify-finding-evidence: invalid result file entry: %s\n' "${line}" >&2 + done <<< "${violations}" + return 1 + fi +} + +# assert_valid_result +# Fail (return 1) unless the file exists, is valid JSON, and carries a +# top-level `files` array — a syntactically valid document missing or +# misshaping `files` (e.g. `{}` or `{"files":{}}`) would otherwise pass +# `jq empty`, iterate zero times below, and print back unchanged as if every +# finding had been evidence-checked. +assert_valid_result() { + local result_file="$1" + if [[ ! -f "${result_file}" ]]; then + printf 'verify-finding-evidence: result file not found: %s\n' "${result_file}" >&2 + return 1 + fi + if ! jq empty -- "${result_file}" 2>/dev/null; then + printf 'verify-finding-evidence: result file is not valid JSON: %s\n' "${result_file}" >&2 + return 1 + fi + if [[ "$(jq -r '.files | type' -- "${result_file}")" != "array" ]]; then + printf 'verify-finding-evidence: result file has no top-level "files" array: %s\n' "${result_file}" >&2 + return 1 + fi +} + +# verify_finding_evidence +# Check every kept finding's `current` against its file's real content and +# print the corrected result JSON to stdout. +verify_finding_evidence() { + local result_file="$1" repo_root="$2" + assert_valid_result "${result_file}" || return 1 + assert_valid_file_entries "${result_file}" || return 1 + + local result_json file_count + result_json=$(cat -- "${result_file}") || return 1 + file_count=$(printf '%s' "${result_json}" | jq '.files | length') || return 1 + + local i + for ((i = 0; i < file_count; i++)); do + local entry path candidate_count full_path check_result updated demotions_line fid + entry=$(printf '%s' "${result_json}" | jq -c ".files[${i}]") || return 1 + path=$(printf '%s' "${entry}" | jq -r '.path') || return 1 + candidate_count=$(printf '%s' "${entry}" \ + | jq '[.errors[]?, .warnings[]?, .informational[]?] | map(select((.current // "") != "")) | length') || return 1 + if [[ "${candidate_count}" -eq 0 ]]; then + continue + fi + full_path="${repo_root}/${path}" + if [[ ! -f "${full_path}" ]]; then + printf 'verify-finding-evidence: target file not found: %s (referenced by a kept finding requiring evidence check)\n' "${full_path}" >&2 + return 1 + fi + check_result=$(jq -n \ + --argjson entry "${entry}" \ + --rawfile content "${full_path}" \ + --arg path "${path}" \ + "${_VERIFY_FINDING_EVIDENCE_JQ_PROGRAM}") || return 1 + updated=$(printf '%s' "${check_result}" | jq -c '.updated') || return 1 + while IFS= read -r demotions_line; do + [[ -n "${demotions_line}" ]] || continue + fid=$(printf '%s' "${demotions_line}" | jq -r '.finding_id') || return 1 + printf 'verify-finding-evidence: demoted %s in %s: current block not found under whitespace normalization\n' "${fid}" "${path}" >&2 + done < <(printf '%s' "${check_result}" | jq -c '.demotions[]') + result_json=$(printf '%s' "${result_json}" | jq --argjson upd "${updated}" --argjson idx "${i}" '.files[$idx] = $upd') || return 1 + done + + printf '%s\n' "${result_json}" +} + +main() { + set -euo pipefail + local result_file="${1:-}" repo_root="${2:-}" + if [[ -z "${result_file}" || -z "${repo_root}" ]]; then + printf 'usage: verify-finding-evidence.sh \n' >&2 + return 2 + fi + verify_finding_evidence "${result_file}" "${repo_root}" +} + +if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then + main "$@" +fi diff --git a/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/verify-method-counts.sh b/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/verify-method-counts.sh new file mode 100755 index 00000000..1a9658b7 --- /dev/null +++ b/plugins/test-writing/skills/phpunit-test-team-reviewing/workflow/verify-method-counts.sh @@ -0,0 +1,174 @@ +#!/usr/bin/env bash +# verify-method-counts.sh — deterministic re-count of a Phase-1 manifest-core's +# test methods before the manifest freezes. +# +# The per-file extraction subagents (input-resolution.md §Per-File Extraction) +# self-report `method_count` and `test_methods` by reading their own file — a +# subagent-produced count is provisional, not a source of truth. This script +# re-derives both fields deterministically by grepping the file on disk for +# every `public function test*` declaration, in file order, and REPLACES a +# mismatching entry's `method_count`/`test_methods` with the extracted values. +# A corrected entry is a successful run, not a partial one; only a missing +# entry file or invalid input JSON is a hard failure. +# +# Usage: verify-method-counts.sh +# manifest.json Phase-1 manifest-core JSON: an array of entries, each with +# at least `path`, `method_count`, `test_methods` +# repo_root repo root of the project under review; entry `path` values +# are resolved relative to it +# +# Writes the corrected manifest JSON to stdout. Writes one line per corrected +# entry to stderr: "verify-method-counts: : method_count -> ". +# Exit 0 on success (corrected mismatches included). Non-zero, with a message +# on stderr, when an entry's file is missing or the input is not valid JSON — +# never a silent skip and never a best-guess count. +# +# Sourcing this file defines the functions without running main, so a bats +# suite can exercise them directly. Functions use explicit `|| return 1` so +# they fail correctly whether or not errexit is active in the sourcing shell. + +# assert_valid_manifest +# Fail (return 1) unless the file exists, is valid JSON, and its top level is +# an array — a syntactically valid non-array (e.g. `{}`) would otherwise pass +# `jq empty`, iterate zero times below, and print back unchanged as if it were +# a valid empty manifest. +assert_valid_manifest() { + local manifest_file="$1" + if [[ ! -f "${manifest_file}" ]]; then + printf 'verify-method-counts: manifest file not found: %s\n' "${manifest_file}" >&2 + return 1 + fi + if ! jq empty -- "${manifest_file}" 2>/dev/null; then + printf 'verify-method-counts: manifest file is not valid JSON: %s\n' "${manifest_file}" >&2 + return 1 + fi + if [[ "$(jq -r 'type' -- "${manifest_file}")" != "array" ]]; then + printf 'verify-method-counts: manifest file top level is not an array: %s\n' "${manifest_file}" >&2 + return 1 + fi +} + +# The jq program applied to the whole manifest array by assert_valid_manifest_entries. Emits +# one line per invalid entry: "entry ('>): ; ...". +# `path`/`method_count`/`test_methods` come from an extraction contract that fixes their +# types; a wrongly-typed field means the manifest is corrupted, not merely miscounted. +_VERIFY_METHOD_COUNTS_ENTRY_CHECK_JQ=$(cat <<'EOF' +def path_errors($v): + if ($v | has("path") | not) then ["path missing"] + elif ($v.path | type) != "string" then ["path is not a string"] + elif ($v.path | length) == 0 then ["path is empty"] + else [] end; +def method_count_errors($v): + if ($v | has("method_count") | not) then ["method_count missing"] + elif ($v.method_count | type) != "number" then ["method_count is not a number"] + else [] end; +def test_methods_errors($v): + if ($v | has("test_methods") | not) then ["test_methods missing"] + elif ($v.test_methods | type) != "array" then ["test_methods is not an array"] + elif ([$v.test_methods[] | type != "string"] | any) then ["test_methods contains a non-string element"] + else [] end; +to_entries[] as $e +| ($e.key) as $idx | ($e.value) as $v +| (path_errors($v) + method_count_errors($v) + test_methods_errors($v)) as $errs +| select(($errs | length) > 0) +| "entry \($idx) (\($v.path // "")): " + ($errs | join("; ")) +EOF +) + +# assert_valid_manifest_entries +# Fail (return 1) unless every entry has a non-empty string `path`, a numeric +# `method_count`, and a `test_methods` array of strings. A wrongly-typed field +# (e.g. `method_count: "three"`) means the manifest is corrupted — this is a +# hard failure, never a silent "correction". +assert_valid_manifest_entries() { + local manifest_file="$1" + local violations + violations=$(jq -r "${_VERIFY_METHOD_COUNTS_ENTRY_CHECK_JQ}" -- "${manifest_file}") || return 1 + if [[ -n "${violations}" ]]; then + local line + while IFS= read -r line; do + printf 'verify-method-counts: invalid manifest entry: %s\n' "${line}" >&2 + done <<< "${violations}" + return 1 + fi +} + +# extract_test_methods_json +# Print the file's `public function test*` method names, in file order, as a +# JSON array of strings. Pattern-based (no PHP parsing), matching the same +# `public function test*` convention input-resolution.md defines for the +# subagent extraction this script re-checks. +# `grep` exits 1 on a legitimate zero-match file (e.g. a data-provider-only +# helper) and >1 on a real read error (unreadable file, I/O failure) — the two +# are distinguished explicitly so a read error fails hard instead of silently +# producing a false `method_count: 0`. +extract_test_methods_json() { + local file="$1" + local raw grep_status names + raw=$(grep -oE 'public[[:space:]]+function[[:space:]]+test[A-Za-z0-9_]+' -- "${file}") && grep_status=0 || grep_status=$? + if [[ "${grep_status}" -gt 1 ]]; then + printf 'verify-method-counts: grep failed reading %s (exit %s)\n' "${file}" "${grep_status}" >&2 + return 1 + fi + names=$(printf '%s' "${raw}" | sed -E 's/^public[[:space:]]+function[[:space:]]+//') || return 1 + printf '%s' "${names}" | jq -R -s 'split("\n") | map(select(length > 0))' || return 1 +} + +# verify_method_counts +# Re-count every entry and print the corrected manifest JSON to stdout. +verify_method_counts() { + local manifest_file="$1" repo_root="$2" + assert_valid_manifest "${manifest_file}" || return 1 + assert_valid_manifest_entries "${manifest_file}" || return 1 + + local manifest_json entry_count + manifest_json=$(cat -- "${manifest_file}") || return 1 + entry_count=$(printf '%s' "${manifest_json}" | jq 'length') || return 1 + + local i + for ((i = 0; i < entry_count; i++)); do + local entry path full_path old_count new_methods_json new_count matches updated + entry=$(printf '%s' "${manifest_json}" | jq -c ".[${i}]") || return 1 + path=$(printf '%s' "${entry}" | jq -r '.path') || return 1 + full_path="${repo_root}/${path}" + if [[ ! -f "${full_path}" ]]; then + printf 'verify-method-counts: entry file not found: %s\n' "${full_path}" >&2 + return 1 + fi + old_count=$(printf '%s' "${entry}" | jq '.method_count') || return 1 + new_methods_json=$(extract_test_methods_json "${full_path}") || return 1 + new_count=$(printf '%s' "${new_methods_json}" | jq 'length') || return 1 + matches=$(jq -n \ + --argjson entry "${entry}" \ + --argjson newm "${new_methods_json}" \ + --argjson newc "${new_count}" \ + '(($entry.test_methods // []) == $newm) and ($entry.method_count == $newc)') || return 1 + if [[ "${matches}" != "true" ]]; then + printf 'verify-method-counts: %s: method_count %s -> %s\n' "${path}" "${old_count}" "${new_count}" >&2 + updated=$(jq -n \ + --argjson entry "${entry}" \ + --argjson newm "${new_methods_json}" \ + --argjson newc "${new_count}" \ + '$entry | .method_count = $newc | .test_methods = $newm') || return 1 + else + updated="${entry}" + fi + manifest_json=$(printf '%s' "${manifest_json}" | jq --argjson upd "${updated}" --argjson idx "${i}" '.[$idx] = $upd') || return 1 + done + + printf '%s\n' "${manifest_json}" +} + +main() { + set -euo pipefail + local manifest_file="${1:-}" repo_root="${2:-}" + if [[ -z "${manifest_file}" || -z "${repo_root}" ]]; then + printf 'usage: verify-method-counts.sh \n' >&2 + return 2 + fi + verify_method_counts "${manifest_file}" "${repo_root}" +} + +if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then + main "$@" +fi From 519ada0df2b197a37bed8e86e1c6e1133ec1d224 Mon Sep 17 00:00:00 2001 From: Martin Bens Date: Sat, 5 Sep 2026 20:01:16 +0200 Subject: [PATCH 3/4] chore(test-writing): release 5.1.1 Patch release carrying the adversarial-stage degradation fixes and the deterministic manifest and evidence gates; CHANGELOG entry derived against main. Co-Authored-By: Claude Fable 5 --- plugins/test-writing/.claude-plugin/plugin.json | 2 +- plugins/test-writing/CHANGELOG.md | 12 ++++++++++++ .../phpunit-integration-test-generation/SKILL.md | 2 +- .../phpunit-integration-test-reviewing/SKILL.md | 2 +- .../phpunit-integration-to-unit-migrating/SKILL.md | 2 +- .../phpunit-migration-test-generation/SKILL.md | 2 +- .../skills/phpunit-migration-test-reviewing/SKILL.md | 2 +- .../phpunit-test-adversarial-reviewing/SKILL.md | 2 +- .../skills/phpunit-test-reconciling/SKILL.md | 2 +- .../skills/phpunit-test-team-reviewing/SKILL.md | 2 +- .../skills/phpunit-unit-test-generation/SKILL.md | 2 +- .../skills/phpunit-unit-test-reviewing/SKILL.md | 2 +- .../skills/phpunit-unit-test-writing/SKILL.md | 2 +- 13 files changed, 24 insertions(+), 12 deletions(-) diff --git a/plugins/test-writing/.claude-plugin/plugin.json b/plugins/test-writing/.claude-plugin/plugin.json index f45c8996..15def37e 100644 --- a/plugins/test-writing/.claude-plugin/plugin.json +++ b/plugins/test-writing/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "test-writing", - "version": "5.1.0", + "version": "5.1.1", "description": "Generate and validate PHPUnit tests for Shopware 6. Unit tests: analyzes source classes to detect category (DTO, Service, Flow/Event, DAL, Exception) and applies category-specific templates. Migration tests: analyzes SQL operations to generate pattern-appropriate migration tests with 8 migration-specific rules. Integration tests: generates wired-up tests via phpunit-integration-test-generation for controller, message-handler, indexer, DAL-flow, and multi-service patterns (defers to unit generation when the SUT is unit-shape); 8 quality rules plus a placement smoke check via phpunit-integration-test-reviewing; a separate user-invoked phpunit-integration-to-unit-migrating skill audits placement with 8 deep-reasoning rules and migrates load-bearing-free tests to the unit suite. Workflow-based team review: wave-orchestrated agents coordinated through a shared blackboard (no agent-to-agent messaging), with adversarial red team and defense rounds. Bundles test-rules MCP server for Shopware compliance rules. Orchestrator runs inline fix loop (max 4 iterations) with oscillation detection. Optional dev-tooling plugin enables PHPStan/PHPUnit/ECS validation in the fix loop.", "author": { "name": "Shopware Labs" diff --git a/plugins/test-writing/CHANGELOG.md b/plugins/test-writing/CHANGELOG.md index 9b2e948b..19168895 100644 --- a/plugins/test-writing/CHANGELOG.md +++ b/plugins/test-writing/CHANGELOG.md @@ -5,6 +5,18 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [5.1.1] - 2026-09-05 + +### Fixed +- **A malformed defense stance no longer aborts the adversarial run.** A defense reconciler listing an adopted finding under both `adopted_new` and `findings` hit an integrity throw at the wave boundary that discarded every completed agent in the run. The duplicate entry is now skipped as an already-cast vote, `defensePrompt` states explicitly that the four response arrays are disjoint, and any defense stance entry failing an integrity guard takes the degrade-by-role path from `references/error-handling.md` instead of throwing: the entry is dropped, the finding it named keeps its prior consensus binding, and each drop is recorded in `red_team.defense_degraded` and rendered as a CAUTION block in the report. +- **A defender's re-characterization of a maintained finding now survives into the report.** The merge picked the descriptive owner by remediation length, so a defender correcting a finding's location, method, and remediation with a terser fix lost every corrected field back to the stale original. Maintained findings now merge through `recharacterize`, which makes the defender's payload the descriptive owner whenever it proposed a remediation; a payload without one keeps the original's fields so `current` and `suggested` still describe one change. + +### Added +- **Deterministic manifest gate** (`workflow/verify-method-counts.sh`): before the Phase-1 manifest freezes, the skill re-extracts every entry's test method names from disk, replaces a mismatched `method_count`/`test_methods` with the extracted truth, logs each replacement, and fails hard on a corrupted entry instead of repairing it. +- **Deterministic evidence gate** (`workflow/verify-finding-evidence.sh`): before the Phase-5 merge, a kept finding whose non-empty `current` block does not occur in the target file under whitespace normalization is demoted to `contested` with an outcome naming the failed match, synced into `adversarial_input` so it never reaches the red team; findings that quoted no code are exempt. +- **Scrutiny labels**: every rendered finding carries `adversary-tested` or `consensus-only`, derived from which stage produced the finding's final state, so consensus-only findings are distinguishable from red-team-survived ones. +- **`references/fix-application.md`**: the contract for applying a report's remediations — verbatim `suggested` application, scoped self-review of the fix diff (redundancy, tautological tests, static gates), whole-suite-only mutation judgments, premise re-verification for `consensus-only` must-fix findings, and `git merge-base --is-ancestor` verification of reported commits. + ## [5.1.0] - 2026-09-03 ### Changed diff --git a/plugins/test-writing/skills/phpunit-integration-test-generation/SKILL.md b/plugins/test-writing/skills/phpunit-integration-test-generation/SKILL.md index da1c83c4..32d53c8c 100644 --- a/plugins/test-writing/skills/phpunit-integration-test-generation/SKILL.md +++ b/plugins/test-writing/skills/phpunit-integration-test-generation/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-integration-test-generation -version: 5.1.0 +version: 5.1.1 description: Use this skill when the user asks to generate, write, or create integration tests for a Shopware 6 source class whose contract requires wired-up code — phrases like "generate integration tests for X", "write an integration test for this controller", "test this indexer", "create an integration test for the message handler". Detects supported integration patterns (controller/route, scheduled-task, message-handler, indexer, DAL-persistence flow, multi-service coordinator) and applies a template producing an INTEGRATION-001..008-compliant test using IntegrationTestBehaviour against the real DAL, container, and HTTP/messaging. When the source class is unit-shape (no persistence, no kernel state, no wiring under test), returns SKIPPED and points at phpunit-unit-test-writing. Do NOT activate for unit tests or migration tests (use phpunit-migration-test-generation). user-invocable: true context: fork diff --git a/plugins/test-writing/skills/phpunit-integration-test-reviewing/SKILL.md b/plugins/test-writing/skills/phpunit-integration-test-reviewing/SKILL.md index 4c948a24..c26e633e 100644 --- a/plugins/test-writing/skills/phpunit-integration-test-reviewing/SKILL.md +++ b/plugins/test-writing/skills/phpunit-integration-test-reviewing/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-integration-test-reviewing -version: 5.1.0 +version: 5.1.1 description: Internal sub-skill. Do not auto-activate. Use only when explicitly invoked by name by another skill or agent. user-invocable: false allowed-tools: Glob, Grep, Read, mcp__plugin_test-writing_test-rules__get_rules diff --git a/plugins/test-writing/skills/phpunit-integration-to-unit-migrating/SKILL.md b/plugins/test-writing/skills/phpunit-integration-to-unit-migrating/SKILL.md index a3f89c53..1e6fd86f 100644 --- a/plugins/test-writing/skills/phpunit-integration-to-unit-migrating/SKILL.md +++ b/plugins/test-writing/skills/phpunit-integration-to-unit-migrating/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-integration-to-unit-migrating -version: 5.1.0 +version: 5.1.1 description: Use this skill ONLY when the user explicitly requests an audit, migration, or evaluation of whether a Shopware integration test belongs in the unit suite — trigger phrases like "audit integration tests", "migrate integration tests to unit", "is this an integration test or a unit test", "evaluate integration tests for migration", "should this be a unit test instead". Audits tests under tests/integration/ for misplacement and migrates load-bearing-free tests to tests/unit/ using one of six codified refactoring patterns. NOT invoked automatically by reviewing skills — phpunit-integration-test-reviewing emits a placement smoke-alarm hint pointing here, but the user must invoke this skill explicitly to run the deep audit. user-invocable: true allowed-tools: Glob, Grep, Read, Edit, Write, AskUserQuestion, Bash, mcp__plugin_test-writing_test-rules__get_rules diff --git a/plugins/test-writing/skills/phpunit-migration-test-generation/SKILL.md b/plugins/test-writing/skills/phpunit-migration-test-generation/SKILL.md index aff8fc73..7229b29e 100644 --- a/plugins/test-writing/skills/phpunit-migration-test-generation/SKILL.md +++ b/plugins/test-writing/skills/phpunit-migration-test-generation/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-migration-test-generation -version: 5.1.0 +version: 5.1.1 description: Use this skill when the user asks to generate, write, or create migration tests for a Shopware 6 migration class — phrases like "generate migration tests", "write a migration test", "create migration test", "test this migration", "test Migration1234Foo". Analyzes the source migration's SQL operations to pick an appropriate pattern (schema-add, schema-remove, data-update, config, mail template), then generates a test that runs against a real database and passes PHPStan and PHPUnit validation. Do NOT activate for unit tests (use phpunit-unit-test-writing) or integration tests of non-migration source classes (use phpunit-integration-test-generation). user-invocable: true context: fork diff --git a/plugins/test-writing/skills/phpunit-migration-test-reviewing/SKILL.md b/plugins/test-writing/skills/phpunit-migration-test-reviewing/SKILL.md index 07b03057..958205d0 100644 --- a/plugins/test-writing/skills/phpunit-migration-test-reviewing/SKILL.md +++ b/plugins/test-writing/skills/phpunit-migration-test-reviewing/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-migration-test-reviewing -version: 5.1.0 +version: 5.1.1 description: Internal sub-skill. Do not auto-activate. Use only when explicitly invoked by name by another skill or agent. user-invocable: false allowed-tools: Glob, Grep, Read, mcp__plugin_test-writing_test-rules__get_rules diff --git a/plugins/test-writing/skills/phpunit-test-adversarial-reviewing/SKILL.md b/plugins/test-writing/skills/phpunit-test-adversarial-reviewing/SKILL.md index 67fec878..30e6e3ee 100644 --- a/plugins/test-writing/skills/phpunit-test-adversarial-reviewing/SKILL.md +++ b/plugins/test-writing/skills/phpunit-test-adversarial-reviewing/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-test-adversarial-reviewing -version: 5.1.0 +version: 5.1.1 description: Internal sub-skill. Do not auto-activate. Use only when explicitly invoked by name by another skill or agent. user-invocable: false allowed-tools: Glob, Grep, Read, mcp__plugin_test-writing_test-rules__get_rules diff --git a/plugins/test-writing/skills/phpunit-test-reconciling/SKILL.md b/plugins/test-writing/skills/phpunit-test-reconciling/SKILL.md index 12992698..6aea64d9 100644 --- a/plugins/test-writing/skills/phpunit-test-reconciling/SKILL.md +++ b/plugins/test-writing/skills/phpunit-test-reconciling/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-test-reconciling -version: 5.1.0 +version: 5.1.1 description: Internal sub-skill. Do not auto-activate. Use only when explicitly invoked by name by another skill or agent. user-invocable: false allowed-tools: Read, Glob, Grep, mcp__plugin_test-writing_test-rules__get_rules diff --git a/plugins/test-writing/skills/phpunit-test-team-reviewing/SKILL.md b/plugins/test-writing/skills/phpunit-test-team-reviewing/SKILL.md index 09d603ee..f9cb2ad9 100644 --- a/plugins/test-writing/skills/phpunit-test-team-reviewing/SKILL.md +++ b/plugins/test-writing/skills/phpunit-test-team-reviewing/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-test-team-reviewing -version: 5.1.0 +version: 5.1.1 description: Use this skill when the user asks for a team-based, consensus, multi-reviewer, or red-team review of Shopware PHPUnit tests — trigger phrases like "team review these tests", "consensus review the tests in PR #N", "red-team this test suite", "multi-reviewer audit of tests/...". Reviews unit (tests/unit/), integration (tests/integration/), and migration (tests/migration/) tests in one run over a mixed manifest, routing each file by test type. Accepts file paths, directories, commits, branches, and PRs as input. For a single-reviewer pass, use the matching per-type reviewing skill instead. allowed-tools: Bash, Read, Glob, Grep, AskUserQuestion, Workflow, mcp__plugin_test-writing_test-rules__build_rule_package --- diff --git a/plugins/test-writing/skills/phpunit-unit-test-generation/SKILL.md b/plugins/test-writing/skills/phpunit-unit-test-generation/SKILL.md index b3267f5f..60b32ecf 100644 --- a/plugins/test-writing/skills/phpunit-unit-test-generation/SKILL.md +++ b/plugins/test-writing/skills/phpunit-unit-test-generation/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-unit-test-generation -version: 5.1.0 +version: 5.1.1 description: Internal sub-skill. Do not auto-activate. Use only when explicitly invoked by name by another skill or agent. user-invocable: false context: fork diff --git a/plugins/test-writing/skills/phpunit-unit-test-reviewing/SKILL.md b/plugins/test-writing/skills/phpunit-unit-test-reviewing/SKILL.md index 77243815..49f36dc3 100644 --- a/plugins/test-writing/skills/phpunit-unit-test-reviewing/SKILL.md +++ b/plugins/test-writing/skills/phpunit-unit-test-reviewing/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-unit-test-reviewing -version: 5.1.0 +version: 5.1.1 description: Internal sub-skill. Do not auto-activate. Use only when explicitly invoked by name by another skill or agent. user-invocable: false allowed-tools: Glob, Grep, Read, mcp__plugin_test-writing_test-rules__get_rules diff --git a/plugins/test-writing/skills/phpunit-unit-test-writing/SKILL.md b/plugins/test-writing/skills/phpunit-unit-test-writing/SKILL.md index 640204cd..e1578f66 100644 --- a/plugins/test-writing/skills/phpunit-unit-test-writing/SKILL.md +++ b/plugins/test-writing/skills/phpunit-unit-test-writing/SKILL.md @@ -1,6 +1,6 @@ --- name: phpunit-unit-test-writing -version: 5.1.0 +version: 5.1.1 description: Use this skill when the user asks to write, generate, create, or add PHPUnit unit tests for a Shopware 6 source class — phrases like "write unit tests for X", "generate tests for ClassName", "create PHPUnit tests", "add test coverage", "test this class", "cover this with tests", "I need tests for", "unit test this", "SW6 unit tests", "Shopware unit tests", "PHPUnit tests for Shopware". Orchestrates the full workflow — source-class category detection (DTO, Service, Flow/Event, DAL, Exception), test generation, MCP-driven review against Shopware unit-test rules, and an inline fix loop that iterates until tests pass. Do NOT activate for integration tests (use phpunit-integration-test-generation), migration tests (use phpunit-migration-test-generation), e2e tests, or non-PHP testing. allowed-tools: Skill, Edit, Read, Glob, TodoWrite, AskUserQuestion, mcp__plugin_dev-tooling_php-tooling --- From a4d2ac605a2cad8ac0c568de2b9f5e9e301df04b Mon Sep 17 00:00:00 2001 From: Martin Bens Date: Sat, 5 Sep 2026 20:28:27 +0200 Subject: [PATCH 4/4] test(test-writing): silence intentional SC2016 notes in evidence gate tests Three fixtures quote PHP code containing `$x` and `$doesNotExist` in single quotes on purpose, and CI's shellcheck run over `.bats` files fails on the resulting SC2016 notes. Each site gets a per-line disable directive with the reason, matching the convention in `plugin-tests/mcp-shared/environment.bats`. Co-Authored-By: Claude Fable 5 --- plugin-tests/test-writing/verify_finding_evidence.bats | 3 +++ 1 file changed, 3 insertions(+) diff --git a/plugin-tests/test-writing/verify_finding_evidence.bats b/plugin-tests/test-writing/verify_finding_evidence.bats index 2e7463b6..852f66d7 100644 --- a/plugin-tests/test-writing/verify_finding_evidence.bats +++ b/plugin-tests/test-writing/verify_finding_evidence.bats @@ -51,6 +51,7 @@ _write_result() { # ============================================================================ @test "keeps a finding whose current matches the file exactly" { + # shellcheck disable=SC2016 # the literal $x is PHP code under test, not a shell expansion _write_result "${BATS_TEST_TMPDIR}/result.json" errors 'static::assertSame(1, $x);' run --separate-stderr verify_finding_evidence "${BATS_TEST_TMPDIR}/result.json" "${REPO_DIR}" @@ -81,6 +82,7 @@ _write_result() { # ============================================================================ @test "matches under whitespace normalization despite different indentation" { + # shellcheck disable=SC2016 # the literal $x is PHP code under test, not a shell expansion _write_result "${BATS_TEST_TMPDIR}/result.json" errors "$(printf 'static::assertSame(1,\n $x);')" run --separate-stderr verify_finding_evidence "${BATS_TEST_TMPDIR}/result.json" "${REPO_DIR}" @@ -98,6 +100,7 @@ _write_result() { # ============================================================================ @test "demotes a fabricated quote into contested with an outcome and a log line" { + # shellcheck disable=SC2016 # the literal $doesNotExist is PHP code under test, not a shell expansion _write_result "${BATS_TEST_TMPDIR}/result.json" errors 'static::assertSame(999, $doesNotExist);' run --separate-stderr verify_finding_evidence "${BATS_TEST_TMPDIR}/result.json" "${REPO_DIR}"