Commit Graph

2 Commits

Author SHA1 Message Date
Shrinath Rao 0ee8a667c2
test: E2E Council Refiner — remediate Sentinel findings instead of dead-ending (#12876)
## Why

The E2E Council blocked **#12857** (no test PR opened) because the
post-heal **Sentinel re-audit found 2 critical quality issues** — even
though **9/10 tests passed**. The tests were **not worthless**; 2 just
*under-claimed* (the body asserted something weaker than the test name
promised):

1. `"should not persist FTS default across page reload"` — re-searches
after reload and asserts the column visible, which passes whether it
*persisted* or *re-resolved*. Never verifies non-persistence.
2. `"should clear FTS marker and re-resolve for new stream on stream
change"` — only checks "table rendered", never that FTS actually
re-resolved.

Blocking was correct (green ≠ meaningful), but a **dead-end block throws
away the other 90%**. The pipeline should **fix the problem and make it
right**, not give up. A no-PR outcome should be a *true last resort*.

## What — The Refiner (Phase 5.5)

A remediation loop **inside `verify_heal`**, after the Sentinel re-audit
and before the quality gate (**reuses the already-booted binary** — no
rebuild):

- **Only fires when needed.** If the re-audit is clean (PASS / 0
critical), the step reads the verdict, sets `ran=false`, and exits —
**no DeepSeek call, no test re-run**. Cheap on the happy path.
- **Strengthen-first.** Feeds the exact Sentinel findings to the
Refiner, which **adds/tightens assertions** so each test verifies what
its name promises → re-runs on the live binary (**green must hold**) →
re-checks assertion-integrity → re-audits. Capped at **2 rounds**.
- **Anti-laundering (the whole game).** Assertions may only be
**ADDED/strengthened — never dropped, inverted in place, or
renamed-to-dodge** a missing assertion. If strengthening goes red
because the **feature isn't wired**, it takes the honest `test.fixme` +
evidence exit (no green-wash).
- **`assert_integrity.py --strengthen`**: permits *added* negative
matchers (assert-absence is legit strengthening) while **still blocking
any dropped assertion** (removed/skipped guards + all static checks
stay). Unit-proven in the commit.
- **Quality gate** now judges the **post-refine** state; blocks only if
the Refiner couldn't keep the spec green honestly.
- **No-PR report** now phrases a block as a true last resort ("the
Refiner attempted to strengthen … but could not resolve every finding
honestly").

### Reuse path (clobber off)
`verify_heal` now also runs on **reuse runs**, and the "already covered"
short-circuit is suppressed on reuse — so **re-running `/e2e` on a
previously-BLOCKED spec re-validates + refines the existing tests into a
PR** instead of stopping at "already covered". No new tests are added on
reuse; the Refiner only strengthens what's there. (Cost: a reuse run
rebuilds even when tests were already perfect — acceptable since re-runs
are explicit; can add a "skip if a green test PR already exists" guard
later.)

## Validation
- `assert_integrity.py` unit-checked: added negative → FAIL without
`--strengthen`, PASS with it; dropped assertion → still FAIL with
`--strengthen`.
- Workflow YAML validated; step order confirmed **Sentinel re-audit →
Refiner → Quality gate**.
- Live end-to-end best validated by re-running `/e2e` on **#12857 with
clobber off** — the Refiner should strengthen the 2 flagged tests and
open the PR.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-25 09:20:31 +00:00
Shrinath Rao 900ebe67c6
ci(e2e-council): test-quality gate — stop certifying green-but-worthless specs (#12722)
## The problem (from the #12719 review)

The pipeline's only hard gate was **"the Healer's authoritative run is
green."** That objective *rewards* making a spec pass by gutting the
assertions that test the feature. In #12719 the result was a suite that:
- dropped the fix-query-card assertion,
- asserted `count >= 0` (a tautology),
- buried real assertions inside `if`s whose `else` just logs, and
- **asserted the feature is absent** (`expectFixQueryCardNotVisible`) —
certifying a half-built feature and set to **turn red when the feature
is finished**.

> A passing test and a valuable test are different things, and we only
measured the first.

This PR makes **"done" mean "the tests would fail if the feature were
broken."**

## Deterministic backbone (no LLM) —
`.github/scripts/assert_integrity.py`

The most important guards don't depend on the model behaving:
- **Diff-aware heal guard** — snapshot the spec's assertions *before*
healing; **reject** any heal that **deleted**, **inverted**
(`toBeVisible` → `not.toBeVisible`), or **skipped** assertions to reach
green.
- **Static anti-patterns** — tautologies (`count >= 0`, `expect(true)`)
and **conditional-only** tests (every `expect` inside an `if` → the test
can pass asserting nothing).
- Conservative by design (only high-confidence patterns) — a gate with
false positives gets disabled by humans. Restored from a **trusted ref**
in verify_heal (never the PR tree's copy).

## Workflow

- New **Assertion-integrity gate** after the authoritative run + a
combined **Quality gate decision** (integrity **+** Sentinel verdict).
`pr_back` now requires `quality_status == pass` → a green-but-worthless
spec **never becomes a PR**.
- The post-heal **Sentinel re-audit now feeds the blocking gate** (was
advisory-only). LLM flakiness fails **open**; the deterministic gate is
the hard teeth.
- **`feature-incomplete`** honest exit: when a test can't pass because
the feature isn't wired, the Healer parks it `test.fixme` + writes
**evidence**; the workflow opens **no PR** and posts the gap (file:line)
on the source PR — the single highest-value output.
- `pr_back` **reconciles `test-summary.json` against the FINAL spec**
(drops phantom rows) so the PR body can't claim tests that don't exist.

## Prompts

- **Healer:** hard anti-weakening rules + the `feature-incomplete`
honest exit.
- **Sentinel:** tautology / conditional-only / negative-only-on-feature
/ name-mismatch as **CRITICAL** checks.
- **Engineer:** no dead page-object scaffolding; ban `waitForTimeout` as
a sync primitive; assertions must be real and match the test name.
- **Architect:** one spec = one area (no Frankenstein specs);
de-duplicate scenarios.

## Considered and deferred: mutation-sanity (extra feature-absent build)

Theoretically the cleanest definition of a valuable test, but **dropped
for now**: it overlaps the cheap static checks, has **no clean
"feature-absent" baseline** under merged/multi-PR input, risks false
positives that erode trust, and doubles the build. The static guards
catch the same #12719 failures at near-zero cost; prevention (Healer
can't invert + honest `feature-incomplete`) beats the expensive
detector.

## Testing

- `assert_integrity.py` unit-tested against tautology / conditional-only
(incl. single-line) / heal-removed / heal-inverted fixtures, with no
false positive on a legit `if` + unconditional `expect`.
- Workflow YAML validated; analyzer compiles.
- **First live run still needed** (per the project's "run the path
before done" rule) — to see the gate block a real weak spec and the
`feature-incomplete` comment fire.

Stacked on #12716 (ENT sister flow); the diff collapses to just the
quality program once #12716 lands.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-21 09:30:03 +00:00