mirror of https://github.com/garrytan/gstack.git
fix: harden data-model E2E fixtures against reading the operator's real gstack install
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.
This commit is contained in:
parent
b3acffdf58
commit
02e312c057
|
|
@ -78,6 +78,35 @@ describe('matchesUnnegated', () => {
|
||||||
expect(matchesUnnegated('This cannot promote the payload into columns.', splitPattern)).toBe(false);
|
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', () => {
|
test('catches a positive match even when an unrelated negation appears earlier in the text', () => {
|
||||||
expect(matchesUnnegated(
|
expect(matchesUnnegated(
|
||||||
'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.',
|
'This is not a JSONField concern. Separately, I recommend you promote the payload into explicit columns.',
|
||||||
|
|
|
||||||
|
|
@ -133,6 +133,28 @@ export function setupPlanEngReviewFixture(tmpPrefix: string, planMarkdown: strin
|
||||||
return planDir;
|
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
|
* Case-insensitive check for `pattern` in `text` that ignores matches
|
||||||
* immediately preceded by a negation word (not/n't/no/never/without/...).
|
* 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
|
// alternative can never match mid-word (there's no word boundary between
|
||||||
// the letters immediately before "n" and "n" itself), so it silently
|
// the letters immediately before "n" and "n" itself), so it silently
|
||||||
// covered nothing beyond the contractions already spelled out explicitly.
|
// 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 flags = pattern.flags.includes('g') ? pattern.flags : pattern.flags + 'g';
|
||||||
const re = new RegExp(pattern.source, flags);
|
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;
|
let m: RegExpExecArray | null;
|
||||||
while ((m = re.exec(text))) {
|
while ((m = re.exec(text))) {
|
||||||
const sentenceStart = Math.max(
|
let sentenceStart = 0;
|
||||||
text.lastIndexOf('.', m.index),
|
let em: RegExpExecArray | null;
|
||||||
text.lastIndexOf('!', m.index),
|
SENTENCE_END.lastIndex = 0;
|
||||||
text.lastIndexOf('?', m.index),
|
while ((em = SENTENCE_END.exec(text)) && em.index < m.index) {
|
||||||
) + 1;
|
sentenceStart = em.index + 1;
|
||||||
|
}
|
||||||
const preceding = text.slice(sentenceStart, m.index);
|
const preceding = text.slice(sentenceStart, m.index);
|
||||||
if (!NEGATION.test(preceding)) return true;
|
if (!NEGATION.test(preceding)) return true;
|
||||||
if (re.lastIndex === m.index) re.lastIndex++; // avoid infinite loop on zero-width matches
|
if (re.lastIndex === m.index) re.lastIndex++; // avoid infinite loop on zero-width matches
|
||||||
|
|
|
||||||
|
|
@ -89,10 +89,13 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
|
||||||
'plan-ceo-review-expansion-energy': ['plan-ceo-review/**', 'scripts/resolvers/preamble.ts', 'test/fixtures/mode-posture/**', 'test/helpers/llm-judge.ts'],
|
'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': ['plan-eng-review/**'],
|
||||||
'plan-eng-review-artifact': ['plan-eng-review/**'],
|
'plan-eng-review-artifact': ['plan-eng-review/**'],
|
||||||
'plan-eng-review-data-model-bias': ['plan-eng-review/**'],
|
// Behavior lives partly in test/helpers/e2e-helpers.ts (setupPlanEngReviewFixture,
|
||||||
'plan-eng-review-data-model-legitimate-json': ['plan-eng-review/**'],
|
// matchesUnnegated, planEngReviewDataModelPrompt) — without this, a regression
|
||||||
'plan-eng-review-data-model-measured-denorm': ['plan-eng-review/**'],
|
// in that shared helper wouldn't get these tests re-selected by diff-based CI.
|
||||||
'plan-eng-review-data-model-minimal-change': ['plan-eng-review/**'],
|
'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-review-report': ['plan-eng-review/**', 'scripts/gen-skill-docs.ts'],
|
||||||
|
|
||||||
// Plan-mode smoke tests — gate-tier safety regression tests. Each test file
|
// Plan-mode smoke tests — gate-tier safety regression tests. Each test file
|
||||||
|
|
|
||||||
|
|
@ -5,7 +5,7 @@ import {
|
||||||
describeIfSelected, testConcurrentIfSelected,
|
describeIfSelected, testConcurrentIfSelected,
|
||||||
copyDirSync, setupBrowseShims, logCost, recordE2E,
|
copyDirSync, setupBrowseShims, logCost, recordE2E,
|
||||||
createEvalCollector, finalizeEvalCollector,
|
createEvalCollector, finalizeEvalCollector,
|
||||||
setupPlanEngReviewFixture, matchesUnnegated,
|
setupPlanEngReviewFixture, matchesUnnegated, planEngReviewDataModelPrompt,
|
||||||
} from './helpers/e2e-helpers';
|
} from './helpers/e2e-helpers';
|
||||||
import { judgePosture } from './helpers/llm-judge';
|
import { judgePosture } from './helpers/llm-judge';
|
||||||
import { spawnSync } from 'child_process';
|
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 () => {
|
testConcurrentIfSelected('plan-eng-review-data-model-bias', async () => {
|
||||||
const result = await runSkillTest({
|
const result = await runSkillTest({
|
||||||
prompt: `Read plan-eng-review/SKILL.md for the review workflow.
|
prompt: planEngReviewDataModelPrompt(
|
||||||
|
planDir,
|
||||||
Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps.
|
'Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.',
|
||||||
|
),
|
||||||
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.`,
|
|
||||||
workingDirectory: planDir,
|
workingDirectory: planDir,
|
||||||
maxTurns: 15,
|
maxTurns: 15,
|
||||||
timeout: 360_000,
|
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 () => {
|
testConcurrentIfSelected('plan-eng-review-data-model-legitimate-json', async () => {
|
||||||
const result = await runSkillTest({
|
const result = await runSkillTest({
|
||||||
prompt: `Read plan-eng-review/SKILL.md for the review workflow.
|
prompt: planEngReviewDataModelPrompt(
|
||||||
|
planDir,
|
||||||
Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps.
|
'Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.',
|
||||||
|
),
|
||||||
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.`,
|
|
||||||
workingDirectory: planDir,
|
workingDirectory: planDir,
|
||||||
maxTurns: 15,
|
maxTurns: 15,
|
||||||
timeout: 360_000,
|
timeout: 360_000,
|
||||||
|
|
@ -643,13 +637,10 @@ not a general schema change.
|
||||||
|
|
||||||
testConcurrentIfSelected('plan-eng-review-data-model-measured-denorm', async () => {
|
testConcurrentIfSelected('plan-eng-review-data-model-measured-denorm', async () => {
|
||||||
const result = await runSkillTest({
|
const result = await runSkillTest({
|
||||||
prompt: `Read plan-eng-review/SKILL.md for the review workflow.
|
prompt: planEngReviewDataModelPrompt(
|
||||||
|
planDir,
|
||||||
Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps.
|
'Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.',
|
||||||
|
),
|
||||||
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.`,
|
|
||||||
workingDirectory: planDir,
|
workingDirectory: planDir,
|
||||||
maxTurns: 15,
|
maxTurns: 15,
|
||||||
timeout: 360_000,
|
timeout: 360_000,
|
||||||
|
|
@ -738,13 +729,10 @@ when the verification email link is clicked.
|
||||||
|
|
||||||
testConcurrentIfSelected('plan-eng-review-data-model-minimal-change', async () => {
|
testConcurrentIfSelected('plan-eng-review-data-model-minimal-change', async () => {
|
||||||
const result = await runSkillTest({
|
const result = await runSkillTest({
|
||||||
prompt: `Read plan-eng-review/SKILL.md for the review workflow.
|
prompt: planEngReviewDataModelPrompt(
|
||||||
|
planDir,
|
||||||
Read plan.md — that's the plan to review. This is a standalone plan document, not a codebase — skip any codebase exploration steps.
|
'Focus specifically on the data model design in the plan. Apply the data model review checklist from the skill.',
|
||||||
|
),
|
||||||
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.`,
|
|
||||||
workingDirectory: planDir,
|
workingDirectory: planDir,
|
||||||
maxTurns: 15,
|
maxTurns: 15,
|
||||||
timeout: 360_000,
|
timeout: 360_000,
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue