Maintainability specialist re-dispatch found the standalone n't alternative
in NEGATION was unreachable: \b n't \b can never match mid-word (no word
boundary exists between the letters immediately before "n" and "n" itself
in "can't", "won't", "aren't", "hasn't"), so only the explicitly-spelled-out
contractions (don't, doesn't, didn't, shouldn't, wouldn't, isn't) were ever
detected — very common ones like "can't"/"won't"/"cannot" fell through
silently. Replaced with a generic [a-z]+n't pattern plus an explicit
"cannot" alternative (which has no apostrophe to match generically).
Also brought the original data-model-bias test's recommendsSeparateModel
check in line with its three newer sibling counterexample tests by routing
it through matchesUnnegated — it was the one E2E block still un-guarded
against a negated false-positive ("I would NOT recommend a separate tier
model" would previously have registered as a pass).
Did not touch rejectsAsUnjustified (measured-denorm test) — it directly
models rejection language ("instead of denormalizing" IS the signal, not a
positive claim needing negation-checking), so wrapping it in
matchesUnnegated would conflict with its own tuned alternatives for no
clear benefit. Did not extract a shared prompt-string constant across the
4 E2E blocks (maintainability finding) — the identical prompt string already
appears verbatim in 3 pre-existing, untouched blocks in this same file;
extracting a constant for only the 4 new blocks would create inconsistency
rather than resolve it, and touching the pre-existing blocks is outside
this PR's scope.
Added unit tests reproducing both contraction gaps directly.
bun test test/helpers/e2e-helpers.test.ts test/skill-e2e-plan.test.ts
test/skill-validation.test.ts test/parity-suite.test.ts test/touchfiles.test.ts
test/gen-skill-docs.test.ts — 783 pass, 42 skip (paid E2E), 0 fail.
Ship-workflow review re-dispatched the testing specialist against the full
updated diff (including the new minimal-change eval) and found 3 more real
bugs in the same family as the earlier fix pass:
- matchesUnnegated's fixed 20-char negation lookback was too narrow: the
patterns it guards have their own internal gaps up to 80 chars, so "I do
not think it's worth extracting the payload into columns" (negation >20
chars before the match) would be misread as an unnegated recommendation.
Now scans back to the start of the current sentence (last ./!/? before the
match) instead of a fixed count — matches the same period-bounded
assumption the calling patterns already make.
- Verified during the fix: neither "not"/"never"/etc. nor the wider window
catches contrastive phrasing ("add this field to the model RATHER THAN
create a separate table") — the rejected alternative matches the pattern
just as strongly as a genuine recommendation, with no negation word
anywhere nearby. Added "rather than"/"instead of" to the shared negation
list, benefiting all four callers, not just the new eval.
- The new minimal-change eval's acceptsInlineAddition check was missing the
matchesUnnegated guard entirely on its first alternative (unlike its
sibling recommendsSeparateModel a few lines above), and
recommendsSeparateModel's verb list missed common recommendation phrasings
(introduce, build, spin out, break out) — deliberately did NOT add "add",
since "add this new field to the existing model" is exactly how the
CORRECT answer gets phrased.
Added 3 unit tests reproducing the sentence-boundary and contrastive-phrasing
fixes directly (not just via the paid E2E path).
bun test test/helpers/e2e-helpers.test.ts test/skill-e2e-plan.test.ts
test/skill-validation.test.ts test/parity-suite.test.ts test/touchfiles.test.ts
test/gen-skill-docs.test.ts — 781 pass, 42 skip (paid E2E), 0 fail.
garrytan/gstack#1048 comment asked for concrete evaluations covering three
cases: normalized models, justified JSON fields, and minimal-change cases.
The prior commits covered the first two (data-model-bias, legitimate-json,
measured-denorm) but not the third — a plan where keeping a single trivial
field inline (not extracting a new model) is the correct call. This was
independently flagged by the ship-workflow coverage audit as the highest-
severity gap in this branch's test coverage.
Adds plan-eng-review-data-model-minimal-change: a synthetic plan adding one
nullable timestamp field to an existing model, no polymorphism, no JSON, no
independent query pattern or consumer. Asserts the skill does NOT reflexively
recommend extracting a separate model for it — exercising the "unless the
extra job is a single trivial field..." exception added to cognitive pattern
#12 and the SRP checklist item in 5374987c.
Also adds test/helpers/e2e-helpers.test.ts: free, deterministic unit tests
for matchesUnnegated() and setupPlanEngReviewFixture(), which were previously
only exercised indirectly by the paid, EVALS=1-gated E2E tests (also flagged
by the coverage audit — a bug in either could silently flip an eval's
pass/fail verdict and look like ordinary LLM wording variance). Along the
way, found and documented a real limitation: the negation-window check
correctly handles multiple sentences (period-bounded), but a single
comma-spliced sentence containing both a negated and a positive match can
still be absorbed into one greedy match — accepted as a known edge case,
since real review prose reliably separates points with periods/bullets.
bun test test/skill-validation.test.ts test/parity-suite.test.ts
test/touchfiles.test.ts test/skill-e2e-plan.test.ts test/helpers/e2e-helpers.test.ts
test/gen-skill-docs.test.ts — all pass, 0 fail.
David Grant