Skip to content

Team-review pipeline lacks deterministic gates around finding evidence, fix application, and manifest counts #176

Open

Description

The phpunit-test-team-reviewing pipeline delegates several verifications to agents that a script can perform, and applies remediations without any gate equivalent to the one the reviewed code passed through. Each gap below states the defect and the required change. Rule-catalog changes are #174; the adversarial-stage crash is #173. Guiding constraint throughout: a deterministic operation (script execution, lint run, string comparison, git query) always takes precedence over an AI-based check for the same property.

  • Manifest method counts are agent-reported and unvalidated. The per-file extraction fan-out (Phase 1) returns each file's method count, and that count drives the track decision (whole-class vs decomposed vs digest) and the review's method scope. The extraction contract forbids estimation, but nothing verifies the returned count against the file, so a wrong count silently misroutes the file — a class can land on a track whose rules never evaluate its bodies, or shard weights can misprice a run. Counting methods is a mechanical operation. The skill re-counts each file's test methods deterministically (parser or pattern-based) before the manifest is frozen; a mismatch replaces the agent's count and is logged, it never merely warns. (Shipped in fix(test-writing): degrade malformed defense stances and add deterministic evidence gates #177 as workflow/verify-method-counts.sh.)

  • Findings are not verified to quote code that exists. A reviewer's finding carries a current block presented as the defective code, and consensus, reconciliation, and the report all build on it. A reviewer can emit a finding whose quoted body does not occur in the file at all; downstream stages then adjudicate fabricated evidence, and withdrawal depends on another agent noticing. The check is mechanical: before a finding enters consensus, the workflow verifies that its current block occurs literally in the target file under whitespace normalization. A finding that fails the check is demoted to contested automatically — it never counts toward a majority — and the demotion reason names the failed match. A finding with an empty current (structural or class-level) is exempt. (Shipped in fix(test-writing): degrade malformed defense stances and add deterministic evidence gates #177 as workflow/verify-finding-evidence.sh, with a placement deviation — see the update comment below.)

  • The fix phase has no review gate. Review findings pass three independent reviewers, reconciliation, consensus, and optionally an adversarial stage — then the remediations are applied by a single agent whose output nothing reviews. Fix application introduces its own defect classes: near-duplicate tests created while addressing separate findings on sibling files (redundancy the next review flags as new findings), tautological tests whose assertion derives from the same source it checks, and drift between a finding's suggested block and what was actually written. The fix workflow runs a scoped verification over its own diff before committing: at minimum the redundancy rules (DESIGN-003/DESIGN-004) across the files it touched together, a tautology check on every newly added test (does the assertion's expected value derive from the code under test or its own fixture?), and the existing static gates. Any mutation-based check in this gate evaluates the mutant against the whole test suite, never against the single test under discussion — a mutant the changed test no longer kills is a coverage loss only if no other test kills it, and judging the test in isolation manufactures false weakening findings. (Shipped in fix(test-writing): degrade malformed defense stances and add deterministic evidence gates #177 as the binding contract references/fix-application.md — see the update comment below.)

  • Structure-only remediations churn across review rounds. Ordering and grouping findings (method order, member placement, provider position) prescribe layouts that are not uniquely determined, so a later review of the same file legitimately proposes a different layout, and successive rounds trade coverage-neutral reorderings indefinitely. When a file's surviving findings are exclusively ordering/grouping, the report emits one consolidated target layout authored by a deterministic script (see the CONV-005 item in Test-rules catalog rules produce findings that cannot be applied as written #174 for the catalog half), marks it coverage-neutral, and a subsequent review treats a file matching a script-authored layout as satisfying the ordering rules — it does not re-derive an alternative.

  • Fix commits are reported without a reachability check. A fix session reports commit hashes as its deliverable, but a checkout or reset during the session can leave a reported commit dangling — reachable only via reflog, ancestor of no branch — and nothing detects the loss. Before the fix workflow reports completion, it verifies each commit it names is an ancestor of the branch it names (git merge-base --is-ancestor), and reports the branch/commit pair as unverified when the check fails instead of listing the commit as landed. (Shipped in fix(test-writing): degrade malformed defense stances and add deterministic evidence gates #177 in references/fix-application.md.)

  • Skipping the adversarial stage leaves consensus findings without any refutation signal, silently. The adversarial stage is cost-gated and can be skipped, but when it runs it overturns findings that passed consensus unanimously — factually false premises, no-op remediations, semantics-changing fixes. A skipped stage means every kept finding reaches the report and the fix phase with zero adversarial scrutiny, and the report does not distinguish these from findings that survived a red team. Two changes: the report labels every finding with the scrutiny it actually received (consensus-only vs adversary-tested), and the fix workflow treats consensus-only must-fix findings as requiring its own verification of the factual premise (does the quoted defect exist as described?) before applying the remediation. (Shipped in fix(test-writing): degrade malformed defense stances and add deterministic evidence gates #177: Scrutiny field in the report, premise re-verification in references/fix-application.md.)

  • Verify that a defense-wave re-characterization survives into the rendered report. The defense wave can re-characterize a finding: the originally cited location or method pair is not a violation, but a real violation of the same rule exists elsewhere in the file, and the defender's stance carries the corrected location and remediation. If the rendered report presents the original finding text rather than the re-characterized version, a fix applier working from the report changes code that was never defective and leaves the real defect in place. Unverified whether the merge and render path currently prefers the re-characterized payload; check it, and if the original text wins anywhere (merge precedence, fallback pairing, arbitration rendering), make the re-characterized location and remediation authoritative for every downstream consumer. (Verified in fix(test-writing): degrade malformed defense stances and add deterministic evidence gates #177: the original text did win — mergeRemediations picked the descriptive owner by remediation length. Fixed via recharacterize().)

Activity

  1. SpiGAndromeda commented on Sep 5, 2026

    @SpiGAndromeda
    CollaboratorAuthor

    Six of the seven items shipped in #177 (test-writing 5.1.1, merged as 07e2148); the checkboxes above reflect that. Two items landed with deviations from this issue's letter, recorded here so the ticks don't read as implemented-as-written:

    Evidence check placement. The issue asks the workflow to verify a finding's current block "before a finding enters consensus". The workflow script runs filesystem-sandboxed, and carrying every file's body in the manifest would breach the run-script size cap, so verify-finding-evidence.sh runs skill-side instead: once per persisted shard result, before the Phase-5 merge and before args-adversarial.json is assembled, with each demotion synced into the shard's adversarial_input. Consequence of the deviation: a fabricated quote can still contribute its own votes inside the run's consensus tally, but it never leaves the stage as kept — it cannot reach the merge, the red team, the report body, or a fix phase. Since a finding's votes only promote that same finding, the residual exposure is the in-run journal, not any consumed output.

    Fix gate shape. The issue phrases the fix-phase items as behavior of "the fix workflow". The plugin has no fix phase — team review is read-only — so the scoped self-review, whole-suite mutation rule, consensus-only premise re-verification, and git merge-base --is-ancestor reachability check shipped as the binding contract references/fix-application.md, referenced from the skill's report handoff. Any future fix phase implements against that contract; until one exists, the contract governs whoever applies a report manually.

    Remaining scope of this issue is the structure-only-churn item alone. It depends on the deterministic layout script specified in #174's CONV-005 item, so it lands with the rule-catalog work rather than standalone.

    Related: #178 tracks two defense-stance observability gaps adjudicated as accepted pre-existing behavior during #177.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions