mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-19 11:22:21 +02:00
fix(evals): selection under-selection fixes — duplicate keys, self-paths, quotePath
Three under-selection holes: (1) duplicate E2E_TOUCHFILES keys (ship-plan-completion/-verification) — JS keeps the LAST duplicate, so the earlier dep lists were dead; pair deleted and a duplicate-key scan added to the literal-only tripwire. (2) The five rehomed e2e files didn't list themselves in their own dep lists, so editing the test never selected it. (3) git C-escapes non-ASCII paths without core.quotePath=false, so an accented filename matched no glob and deselected its tests. Also updates the stale --retry cost comment.
This commit is contained in:
@@ -107,7 +107,19 @@ describe('getChangedFiles union', () => {
|
|||||||
|
|
||||||
test('missing base ref → throws naming EVALS_ALL and the failing command', () => {
|
test('missing base ref → throws naming EVALS_ALL and the failing command', () => {
|
||||||
expect(() => getChangedFiles('no-such-ref', repo)).toThrow(/EVALS_ALL=1/);
|
expect(() => getChangedFiles('no-such-ref', repo)).toThrow(/EVALS_ALL=1/);
|
||||||
expect(() => getChangedFiles('no-such-ref', repo)).toThrow(/git diff --name-only no-such-ref\.\.\.HEAD/);
|
expect(() => getChangedFiles('no-such-ref', repo)).toThrow(/diff --name-only no-such-ref\.\.\.HEAD/);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('non-ASCII filenames come back as raw UTF-8, not C-escaped (core.quotePath=false)', () => {
|
||||||
|
const name = 'résumé-fixture.md';
|
||||||
|
fs.writeFileSync(path.join(repo, name), 'x');
|
||||||
|
try {
|
||||||
|
const files = getChangedFiles('base', repo);
|
||||||
|
expect(files).toContain(name);
|
||||||
|
expect(files.every((f) => !f.includes('\\303'))).toBe(true);
|
||||||
|
} finally {
|
||||||
|
fs.rmSync(path.join(repo, name), { force: true });
|
||||||
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
test('injected spawn failure → throws with stderr in the message', () => {
|
test('injected spawn failure → throws with stderr in the message', () => {
|
||||||
|
|||||||
@@ -116,16 +116,21 @@ export function getChangedFiles(
|
|||||||
cwd: string,
|
cwd: string,
|
||||||
spawnImpl: typeof spawnSync = spawnSync,
|
spawnImpl: typeof spawnSync = spawnSync,
|
||||||
): string[] {
|
): string[] {
|
||||||
const committed = runGitOrThrow(['diff', '--name-only', `${baseBranch}...HEAD`], cwd, spawnImpl)
|
// core.quotePath=false: without it git C-escapes non-ASCII bytes
|
||||||
|
// ("docs/r\303\251sum\303\251.md"), the escaped string matches no glob,
|
||||||
|
// and the dependent test is silently DESELECTED — under-selection, the
|
||||||
|
// exact direction selection must fail away from.
|
||||||
|
const noQuote = ['-c', 'core.quotePath=false'];
|
||||||
|
const committed = runGitOrThrow([...noQuote, 'diff', '--name-only', `${baseBranch}...HEAD`], cwd, spawnImpl)
|
||||||
.trim().split('\n').filter(Boolean);
|
.trim().split('\n').filter(Boolean);
|
||||||
const uncommitted = runGitOrThrow(['diff', '--name-only', 'HEAD'], cwd, spawnImpl)
|
const uncommitted = runGitOrThrow([...noQuote, 'diff', '--name-only', 'HEAD'], cwd, spawnImpl)
|
||||||
.trim().split('\n').filter(Boolean);
|
.trim().split('\n').filter(Boolean);
|
||||||
const untracked = runGitOrThrow(['status', '--porcelain', '--untracked-files=all'], cwd, spawnImpl)
|
const untracked = runGitOrThrow([...noQuote, 'status', '--porcelain', '--untracked-files=all'], cwd, spawnImpl)
|
||||||
.split('\n')
|
.split('\n')
|
||||||
.filter(line => line.startsWith('?? '))
|
.filter(line => line.startsWith('?? '))
|
||||||
.map(line => {
|
.map(line => {
|
||||||
let p = line.slice(3);
|
let p = line.slice(3);
|
||||||
// git quotes paths containing special characters
|
// residual quoting (embedded quote/newline) — strip the wrapper
|
||||||
if (p.startsWith('"') && p.endsWith('"')) p = p.slice(1, -1);
|
if (p.startsWith('"') && p.endsWith('"')) p = p.slice(1, -1);
|
||||||
return p;
|
return p;
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -52,10 +52,10 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
|
|||||||
'qa-bootstrap': ['qa/**', 'ship/**'],
|
'qa-bootstrap': ['qa/**', 'ship/**'],
|
||||||
|
|
||||||
// Review
|
// Review
|
||||||
'review-sql-injection': ['review/**', 'test/fixtures/review-eval-vuln.rb'],
|
'review-sql-injection': ['review/**', 'test/fixtures/review-eval-vuln.rb', 'test/skill-e2e-review.test.ts'],
|
||||||
'review-enum-completeness': ['review/**', 'test/fixtures/review-eval-enum*.rb'],
|
'review-enum-completeness': ['review/**', 'test/fixtures/review-eval-enum*.rb', 'test/skill-e2e-review.test.ts'],
|
||||||
'review-base-branch': ['review/**'],
|
'review-base-branch': ['review/**'],
|
||||||
'review-design-lite': ['review/**', 'test/fixtures/review-eval-design-slop.*'],
|
'review-design-lite': ['review/**', 'test/fixtures/review-eval-design-slop.*', 'test/skill-e2e-review.test.ts'],
|
||||||
|
|
||||||
// Review Army (specialist dispatch)
|
// Review Army (specialist dispatch)
|
||||||
'review-army-migration-safety': ['review/**', 'scripts/resolvers/review-army.ts', 'bin/gstack-diff-scope'],
|
'review-army-migration-safety': ['review/**', 'scripts/resolvers/review-army.ts', 'bin/gstack-diff-scope'],
|
||||||
@@ -94,7 +94,7 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
|
|||||||
// Covers ceo (preamble misfire) + eng/design (scope-gate bypass must not
|
// Covers ceo (preamble misfire) + eng/design (scope-gate bypass must not
|
||||||
// fire outside plan mode) + the named-target exception case. 4 PTY runs;
|
// fire outside plan mode) + the named-target exception case. 4 PTY runs;
|
||||||
// in CI these run CONCURRENT with the rest of the pty-plan-smoke suite
|
// in CI these run CONCURRENT with the rest of the pty-plan-smoke suite
|
||||||
// (--max-concurrency + --retry 2), so worst-case cost is ~3x a single
|
// (--max-concurrency + --retry 1), so worst-case cost is ~2x a single
|
||||||
// pass of each, sharing the API budget with sibling tests — not the
|
// pass of each, sharing the API budget with sibling tests — not the
|
||||||
// sequential ~+10min a local read suggests.
|
// sequential ~+10min a local read suggests.
|
||||||
'plan-mode-no-op': ['plan-ceo-review/**', 'plan-eng-review/**', 'plan-design-review/**', 'scripts/resolvers/preamble/generate-completion-status.ts', 'scripts/resolvers/preamble.ts', 'test/helpers/claude-pty-runner.ts', 'test/skill-e2e-plan-mode-no-op.test.ts'],
|
'plan-mode-no-op': ['plan-ceo-review/**', 'plan-eng-review/**', 'plan-design-review/**', 'scripts/resolvers/preamble/generate-completion-status.ts', 'scripts/resolvers/preamble.ts', 'test/helpers/claude-pty-runner.ts', 'test/skill-e2e-plan-mode-no-op.test.ts'],
|
||||||
@@ -223,13 +223,11 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
|
|||||||
// Ship
|
// Ship
|
||||||
'ship-base-branch': ['ship/**', 'bin/gstack-repo-mode'],
|
'ship-base-branch': ['ship/**', 'bin/gstack-repo-mode'],
|
||||||
'ship-local-workflow': ['ship/**', 'scripts/gen-skill-docs.ts'],
|
'ship-local-workflow': ['ship/**', 'scripts/gen-skill-docs.ts'],
|
||||||
'review-dashboard-via': ['ship/**', 'scripts/resolvers/review.ts', 'codex/**', 'autoplan/**', 'land-and-deploy/**'],
|
'review-dashboard-via': ['ship/**', 'scripts/resolvers/review.ts', 'codex/**', 'autoplan/**', 'land-and-deploy/**', 'test/skill-e2e-review-attribution.test.ts'],
|
||||||
'ship-plan-completion': ['ship/**', 'scripts/gen-skill-docs.ts'],
|
|
||||||
'ship-plan-verification': ['ship/**', 'scripts/gen-skill-docs.ts'],
|
|
||||||
|
|
||||||
// Retro
|
// Retro
|
||||||
'retro': ['retro/**'],
|
'retro': ['retro/**', 'test/skill-e2e-retro.test.ts'],
|
||||||
'retro-base-branch': ['retro/**'],
|
'retro-base-branch': ['retro/**', 'test/skill-e2e-retro.test.ts'],
|
||||||
|
|
||||||
// Global discover
|
// Global discover
|
||||||
'global-discover': ['bin/gstack-global-discover.ts', 'test/global-discover.test.ts'],
|
'global-discover': ['bin/gstack-global-discover.ts', 'test/global-discover.test.ts'],
|
||||||
@@ -277,9 +275,9 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
|
|||||||
|
|
||||||
// Coverage audit (shared fixture) + triage + gates
|
// Coverage audit (shared fixture) + triage + gates
|
||||||
'ship-coverage-audit': ['ship/**', 'test/fixtures/coverage-audit-fixture.ts', 'bin/gstack-repo-mode'],
|
'ship-coverage-audit': ['ship/**', 'test/fixtures/coverage-audit-fixture.ts', 'bin/gstack-repo-mode'],
|
||||||
'review-coverage-audit': ['review/**', 'test/fixtures/coverage-audit-fixture.ts'],
|
'review-coverage-audit': ['review/**', 'test/fixtures/coverage-audit-fixture.ts', 'test/skill-e2e-coverage-audit.test.ts'],
|
||||||
'plan-eng-coverage-audit': ['plan-eng-review/**', 'test/fixtures/coverage-audit-fixture.ts'],
|
'plan-eng-coverage-audit': ['plan-eng-review/**', 'test/fixtures/coverage-audit-fixture.ts', 'test/skill-e2e-coverage-audit.test.ts'],
|
||||||
'ship-triage': ['ship/**', 'bin/gstack-repo-mode'],
|
'ship-triage': ['ship/**', 'bin/gstack-repo-mode', 'test/skill-e2e-triage.test.ts'],
|
||||||
|
|
||||||
// Plan completion audit + verification
|
// Plan completion audit + verification
|
||||||
'ship-plan-completion': ['ship/**', 'scripts/gen-skill-docs.ts'],
|
'ship-plan-completion': ['ship/**', 'scripts/gen-skill-docs.ts'],
|
||||||
|
|||||||
@@ -105,6 +105,23 @@ describe('touchfiles-data.ts literal-only tripwire', () => {
|
|||||||
expect(sawBacktick, explain).toBe(false);
|
expect(sawBacktick, explain).toBe(false);
|
||||||
expect(code, explain).not.toContain('${');
|
expect(code, explain).not.toContain('${');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('no duplicate keys within any map block', () => {
|
||||||
|
// JS object evaluation silently keeps the LAST duplicate — the earlier
|
||||||
|
// dep list becomes dead weight an editor can update to no effect, and
|
||||||
|
// no runtime assertion can see the collapsed key. Scan the source.
|
||||||
|
const blocks = src.split(/export const /).slice(1);
|
||||||
|
const dupes: string[] = [];
|
||||||
|
for (const block of blocks) {
|
||||||
|
const name = block.slice(0, block.indexOf(' '));
|
||||||
|
const seen = new Set<string>();
|
||||||
|
for (const match of block.matchAll(/^\s{2}'([^']+)':/gm)) {
|
||||||
|
if (seen.has(match[1])) dupes.push(`${name}: '${match[1]}'`);
|
||||||
|
seen.add(match[1]);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
expect(dupes, 'duplicate keys collapse silently — the earlier entry is dead').toEqual([]);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('facade export parity', () => {
|
describe('facade export parity', () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user