From 02e312c0575f69053c9bc62aae03513c561fbeed Mon Sep 17 00:00:00 2001 From: David Grant Date: Wed, 15 Jul 2026 18:38:49 -0700 Subject: [PATCH] fix: harden data-model E2E fixtures against reading the operator's real gstack install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex adversarial review (Step 11) flagged that these 4 tests were not hermetic: the prompt only said "Read plan-eng-review/SKILL.md" (relative, unpinned), and the copied SKILL.md's own STOP-Read directive references an ABSOLUTE path (~/.claude/skills/gstack/plan-eng-review/sections/review-sections.md) for its carved sections file. On any machine with gstack actually installed at that location — which includes the maintainer's own dev machine — the agent could read and get graded against the REAL globally-installed skill instead of this branch's sandboxed copy, silently defeating the point of the isolated fixture. This is the exact failure mode test/helpers/auq-sdk-capture.ts's setupSkillDir()/captureSectionReads() already guards against for other carved-skill tests; these 4 blocks predated that pattern and didn't use it. Fixed with a targeted change rather than a full migration to the auq-sdk-capture harness: extracted planEngReviewDataModelPrompt() into e2e-helpers.ts, pinning the sandboxed SKILL.md's absolute path and explicitly forbidding the agent from reading any other SKILL.md ("especially nothing under ~/.claude or /Users") — matching auq-sdk-capture.ts's own phrasing. This also resolves the maintainability specialist's earlier DRY finding (the identical prompt string was duplicated across all 4 blocks) in the same change, since all 4 now call the one shared function. Also fixed a second Codex finding: touchfiles.ts only listed plan-eng-review/** as these tests' dependency, but their actual behavior lives partly in test/helpers/e2e-helpers.ts (setupPlanEngReviewFixture, matchesUnnegated) — a future bug there wouldn't have gotten these 4 tests re-selected by the diff-based selector. Added the file to each test's touchfile entry (scoped to just these 4, not GLOBAL_TOUCHFILES, since the new exports aren't used elsewhere yet). Two other Codex findings reviewed and accepted as-is, not fixed: - matchesUnnegated can still mis-score idiom-based false negation ("no doubt", "not only... but also") — a genuine lexical-heuristic limitation, but these idioms are exceedingly unlikely to co-occur with the specific verb+object patterns these checks require in real review prose; chasing every conceivable idiom is diminishing returns for a best-effort matcher. - The new evals are periodic tier, not gate — consistent with this repo's existing convention for all Opus-model E2E tests (documented in CLAUDE.md's two-tier system), not a gap introduced by this branch. 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 — 786 pass, 42 skip (paid E2E), 0 fail. --- test/helpers/e2e-helpers.test.ts | 29 ++++++++++++++++++++ test/helpers/e2e-helpers.ts | 45 ++++++++++++++++++++++++++----- test/helpers/touchfiles.ts | 11 +++++--- test/skill-e2e-plan.test.ts | 46 ++++++++++++-------------------- 4 files changed, 92 insertions(+), 39 deletions(-) diff --git a/test/helpers/e2e-helpers.test.ts b/test/helpers/e2e-helpers.test.ts index 0dc1b8ce0..a5d252daf 100644 --- a/test/helpers/e2e-helpers.test.ts +++ b/test/helpers/e2e-helpers.test.ts @@ -78,6 +78,35 @@ describe('matchesUnnegated', () => { expect(matchesUnnegated('This cannot promote the payload into columns.', splitPattern)).toBe(false); }); + test('recognizes a typographic curly apostrophe ("doesn’t") as a negation', () => { + // LLM prose commonly renders contractions with U+2019 rather than the + // ASCII apostrophe — a plain [a-z]+n't pattern misses this entirely. + expect(matchesUnnegated( + 'This doesn’t need extraction — promote the payload to explicit columns is unnecessary here.', + /promote the payload to explicit columns/i, + )).toBe(false); + }); + + test('does not let an abbreviation period (e.g., i.e., etc.) hide an earlier negation', () => { + // A bare lastIndexOf('.') treats every period as a sentence break, + // including ones inside abbreviations — which would incorrectly cut the + // negation lookback window short and hide "not" from the scan. + expect(matchesUnnegated( + 'I would not recommend, e.g., promoting this payload to explicit columns.', + /promoting this payload to explicit columns/i, + )).toBe(false); + }); + + test('still treats a real sentence boundary as a boundary (does not over-widen the window)', () => { + // Guards against overcorrecting the abbreviation fix into ignoring + // genuine sentence breaks — an earlier, unrelated negation must NOT + // suppress a later, unnegated positive match. + expect(matchesUnnegated( + 'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.', + /promote the payload into explicit columns/i, + )).toBe(true); + }); + test('catches a positive match even when an unrelated negation appears earlier in the text', () => { expect(matchesUnnegated( 'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.', diff --git a/test/helpers/e2e-helpers.ts b/test/helpers/e2e-helpers.ts index b0187c2c0..c675d0319 100644 --- a/test/helpers/e2e-helpers.ts +++ b/test/helpers/e2e-helpers.ts @@ -133,6 +133,28 @@ export function setupPlanEngReviewFixture(tmpPrefix: string, planMarkdown: strin return planDir; } +/** + * Prompt for the data-model-bias/legitimate-json/measured-denorm/minimal-change + * E2E blocks (paired with setupPlanEngReviewFixture). Pins the ABSOLUTE path to + * the sandboxed SKILL.md and explicitly forbids reading any other SKILL.md — + * especially not the operator's globally-installed ~/.claude/skills/gstack — + * because plan-eng-review/SKILL.md's own STOP-Read directive references that + * absolute path for its carved sections/ file. Without this, a machine that + * has gstack installed globally could have the agent read (and get graded + * against) the REAL installed skill instead of this branch's copy, silently + * defeating the whole point of the sandbox. + */ +export function planEngReviewDataModelPrompt(planDir: string, focusSentence: string): string { + const skillPath = path.join(planDir, 'plan-eng-review', 'SKILL.md'); + return `Read the ONLY skill file you may read, this absolute path: ${skillPath}. Do NOT Glob, find, or search for any other SKILL.md anywhere — especially nothing under ~/.claude or /Users. Follow its review workflow for this scenario. + +Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. + +Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md + +${focusSentence}`; +} + /** * Case-insensitive check for `pattern` in `text` that ignores matches * immediately preceded by a negation word (not/n't/no/never/without/...). @@ -161,16 +183,27 @@ export function matchesUnnegated(text: string, pattern: RegExp): boolean { // alternative can never match mid-word (there's no word boundary between // the letters immediately before "n" and "n" itself), so it silently // covered nothing beyond the contractions already spelled out explicitly. - const NEGATION = /\b(not|no|never|cannot|without|against|rather than|instead of)\b|[a-z]+n't\b/i; + // [a-z]+n['’]t matches both the ASCII apostrophe and the typographic + // curly one (’, U+2019) — LLM prose commonly renders contractions with the + // curly form, and this codebase already hit this exact gotcha once before + // (see the apostrophe wildcard note in test/spec-template-invariants.test.ts). + const NEGATION = /\b(not|no|never|cannot|without|against|rather than|instead of)\b|[a-z]+n['’]t\b/i; const flags = pattern.flags.includes('g') ? pattern.flags : pattern.flags + 'g'; const re = new RegExp(pattern.source, flags); + // Real sentence-ending punctuation is followed by whitespace + a capital + // letter, or sits at the end of the string — this excludes periods inside + // abbreviations ("e.g.", "i.e.", "etc."), decimals, and version strings, + // which a bare lastIndexOf('.') would wrongly treat as a sentence break + // and use to hide an earlier negation word from the lookback window. + const SENTENCE_END = /[.!?](?=\s+[A-Z]|\s*$)/g; let m: RegExpExecArray | null; while ((m = re.exec(text))) { - const sentenceStart = Math.max( - text.lastIndexOf('.', m.index), - text.lastIndexOf('!', m.index), - text.lastIndexOf('?', m.index), - ) + 1; + let sentenceStart = 0; + let em: RegExpExecArray | null; + SENTENCE_END.lastIndex = 0; + while ((em = SENTENCE_END.exec(text)) && em.index < m.index) { + sentenceStart = em.index + 1; + } const preceding = text.slice(sentenceStart, m.index); if (!NEGATION.test(preceding)) return true; if (re.lastIndex === m.index) re.lastIndex++; // avoid infinite loop on zero-width matches diff --git a/test/helpers/touchfiles.ts b/test/helpers/touchfiles.ts index ea58bf4ab..accb47962 100644 --- a/test/helpers/touchfiles.ts +++ b/test/helpers/touchfiles.ts @@ -89,10 +89,13 @@ export const E2E_TOUCHFILES: Record = { 'plan-ceo-review-expansion-energy': ['plan-ceo-review/**', 'scripts/resolvers/preamble.ts', 'test/fixtures/mode-posture/**', 'test/helpers/llm-judge.ts'], 'plan-eng-review': ['plan-eng-review/**'], 'plan-eng-review-artifact': ['plan-eng-review/**'], - 'plan-eng-review-data-model-bias': ['plan-eng-review/**'], - 'plan-eng-review-data-model-legitimate-json': ['plan-eng-review/**'], - 'plan-eng-review-data-model-measured-denorm': ['plan-eng-review/**'], - 'plan-eng-review-data-model-minimal-change': ['plan-eng-review/**'], + // Behavior lives partly in test/helpers/e2e-helpers.ts (setupPlanEngReviewFixture, + // matchesUnnegated, planEngReviewDataModelPrompt) — without this, a regression + // in that shared helper wouldn't get these tests re-selected by diff-based CI. + 'plan-eng-review-data-model-bias': ['plan-eng-review/**', 'test/helpers/e2e-helpers.ts'], + 'plan-eng-review-data-model-legitimate-json': ['plan-eng-review/**', 'test/helpers/e2e-helpers.ts'], + 'plan-eng-review-data-model-measured-denorm': ['plan-eng-review/**', 'test/helpers/e2e-helpers.ts'], + 'plan-eng-review-data-model-minimal-change': ['plan-eng-review/**', 'test/helpers/e2e-helpers.ts'], 'plan-review-report': ['plan-eng-review/**', 'scripts/gen-skill-docs.ts'], // Plan-mode smoke tests — gate-tier safety regression tests. Each test file diff --git a/test/skill-e2e-plan.test.ts b/test/skill-e2e-plan.test.ts index 4004b6197..76dfe110b 100644 --- a/test/skill-e2e-plan.test.ts +++ b/test/skill-e2e-plan.test.ts @@ -5,7 +5,7 @@ import { describeIfSelected, testConcurrentIfSelected, copyDirSync, setupBrowseShims, logCost, recordE2E, createEvalCollector, finalizeEvalCollector, - setupPlanEngReviewFixture, matchesUnnegated, + setupPlanEngReviewFixture, matchesUnnegated, planEngReviewDataModelPrompt, } from './helpers/e2e-helpers'; import { judgePosture } from './helpers/llm-judge'; import { spawnSync } from 'child_process'; @@ -424,13 +424,10 @@ column-add, no new FK navigation in the codebase. testConcurrentIfSelected('plan-eng-review-data-model-bias', async () => { const result = await runSkillTest({ - prompt: `Read plan-eng-review/SKILL.md for the review workflow. - -Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. - -Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md - -Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.`, + prompt: planEngReviewDataModelPrompt( + planDir, + 'Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.', + ), workingDirectory: planDir, maxTurns: 15, timeout: 360_000, @@ -543,13 +540,10 @@ response verbatim for audit/replay, not to model our own domain state. testConcurrentIfSelected('plan-eng-review-data-model-legitimate-json', async () => { const result = await runSkillTest({ - prompt: `Read plan-eng-review/SKILL.md for the review workflow. - -Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. - -Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md - -Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.`, + prompt: planEngReviewDataModelPrompt( + planDir, + 'Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.', + ), workingDirectory: planDir, maxTurns: 15, timeout: 360_000, @@ -643,13 +637,10 @@ not a general schema change. testConcurrentIfSelected('plan-eng-review-data-model-measured-denorm', async () => { const result = await runSkillTest({ - prompt: `Read plan-eng-review/SKILL.md for the review workflow. - -Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. - -Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md - -Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.`, + prompt: planEngReviewDataModelPrompt( + planDir, + 'Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.', + ), workingDirectory: planDir, maxTurns: 15, timeout: 360_000, @@ -738,13 +729,10 @@ when the verification email link is clicked. testConcurrentIfSelected('plan-eng-review-data-model-minimal-change', async () => { const result = await runSkillTest({ - prompt: `Read plan-eng-review/SKILL.md for the review workflow. - -Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps. - -Proceed directly to the architecture review section. Skip any AskUserQuestion calls — this is non-interactive. Write your complete architecture review directly to ${planDir}/review-output.md - -Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.`, + prompt: planEngReviewDataModelPrompt( + planDir, + 'Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.', + ), workingDirectory: planDir, maxTurns: 15, timeout: 360_000,