From 14ee421fb121e59c584b55262dfdb8bbbac61fa6 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 26 Aug 2026 15:32:55 +0000 Subject: [PATCH] fix: pre-landing review fixes Testing-specialist findings, all mechanical: (1) pin the E2E fixture's git branch (-b main / init.defaultBranch=main) and assert every setup command's exit status so operator git config can't silently corrupt a paid run; (2) tighten the dispatch matcher to Step 18-prompt-specific markers (document-release/SKILL.md | executing the /document-release workflow) so a subagent merely quoting section text can't false-pass the regression assert (verified against recorded burn-in transcripts); (3) replace the subsumed carve-guards anchor with three non-overlapping per-touchpoint anchors (gerund/imperative/3rd-person) so each touchpoint is independently enforced. Co-Authored-By: Claude Fable 5 --- test/helpers/carve-guards.ts | 13 ++++++---- test/skill-e2e-ship-docsync.test.ts | 38 ++++++++++++++++++++++++++--- 2 files changed, 42 insertions(+), 9 deletions(-) diff --git a/test/helpers/carve-guards.ts b/test/helpers/carve-guards.ts index 25243a8da..6caa90872 100644 --- a/test/helpers/carve-guards.ts +++ b/test/helpers/carve-guards.ts @@ -118,17 +118,20 @@ export const CARVE_GUARDS: Record = { // (CI backstop: test/pr-title-sync-workflow-safety.test.ts). // Same carve also stranded the Step 18 /document-release dispatch out of // sight — the skeleton never named it and the handoff "got lost" (#2666 - // follow-up). The two subagent anchors pin the restored visibility: - // 'the /document-release subagent' matches all three touchpoints (trigger - // via section-index + STOP pointer, Step 17 handoff, hoisted invariant); - // 'dispatches the /document-release subagent' pins the invariant itself. + // follow-up). Three NON-OVERLAPPING anchors pin the restored visibility, + // one per touchpoint (no anchor is a substring of another, so each is + // independently enforced — a subsumed anchor adds zero enforcement): + // gerund form → manifest trigger (renders 2x: section index + STOP) + // imperative → Step 17 handoff line + // 3rd person → hoisted doc-sync invariant // Matching is case-sensitive String.includes — "dispatching the" does NOT // contain "dispatch the" — so update anchors in lockstep with any // touchpoint rewording. mustStayInSkeleton: [ 'v$NEW_VERSION', 'gstack-pr-title-rewrite', - 'the /document-release subagent', + 'dispatching the /document-release subagent to sync docs', + 'dispatch the /document-release subagent to sync docs', 'dispatches the /document-release subagent', ], // ...while the full create/update procedure stays carved into pr-body.md diff --git a/test/skill-e2e-ship-docsync.test.ts b/test/skill-e2e-ship-docsync.test.ts index bc2cfd217..853b8859f 100644 --- a/test/skill-e2e-ship-docsync.test.ts +++ b/test/skill-e2e-ship-docsync.test.ts @@ -91,10 +91,32 @@ describeE2E('Ship doc-sync dispatch E2E (gate)', () => { // Bare remote + clone; Steps 0-16 "already done": feature branch with a // committed change, VERSION bumped, CHANGELOG entry written. Not pushed — // Step 17 (the slice's first step) does that. - spawnSync('git', ['init', '--bare'], { cwd: remoteDir, stdio: 'pipe' }); - spawnSync('git', ['clone', remoteDir, repoDir], { stdio: 'pipe' }); + // Branch pinned with -b main / -c init.defaultBranch=main so operator git + // config never leaks into the fixture (default-config machines would + // otherwise create master and the later `push -u origin main` would fail). + // Every setup command asserts status — a broken fixture must fail loud + // and free here, never burn a paid run downstream. + const assertOk = (r: ReturnType, what: string) => { + if (r.status !== 0) { + throw new Error( + `ship-docsync fixture setup failed: ${what} → exit ${r.status}\n${r.stderr?.toString() ?? ''}` + ); + } + return r; + }; + assertOk( + spawnSync('git', ['init', '--bare', '-b', 'main'], { cwd: remoteDir, stdio: 'pipe', timeout: 15000 }), + 'git init --bare -b main' + ); + assertOk( + spawnSync('git', ['-c', 'init.defaultBranch=main', 'clone', remoteDir, repoDir], { stdio: 'pipe', timeout: 15000 }), + 'git clone' + ); const run = (cmd: string, args: string[]) => - spawnSync(cmd, args, { cwd: repoDir, stdio: 'pipe', timeout: 10000 }); + assertOk( + spawnSync(cmd, args, { cwd: repoDir, stdio: 'pipe', timeout: 10000 }), + `${cmd} ${args.join(' ')}` + ); run('git', ['config', 'user.email', 'test@test.com']); run('git', ['config', 'user.name', 'Test']); run('git', ['config', 'commit.gpgsign', 'false']); @@ -180,10 +202,18 @@ describeE2E('Ship doc-sync dispatch E2E (gate)', () => { // the transcript and would false-positive any transcript-wide match // (trap documented in skill-e2e-autoplan-dual-voice.test.ts). const calls = Array.isArray(result.toolCalls) ? result.toolCalls : []; + // Matcher is dispatch-SPECIFIC, not mention-specific: both markers come + // verbatim from the Step 18 subagent prompt dictated by pr-body.md. A + // subagent that merely quotes section text mentioning "document-release" + // (e.g. a PR-body drafter) must NOT count — that false-pass would mask + // the exact regression this test exists to catch. Verified against + // recorded burn-in transcripts: real dispatch inputs carry both markers. const dispatchIdx = calls.findIndex( (tc) => (tc.tool === 'Agent' || tc.tool === 'Task') && - /document-release/i.test(JSON.stringify(tc.input ?? {})) + /document-release\/SKILL\.md|executing the \/document-release workflow/i.test( + JSON.stringify(tc.input ?? {}) + ) ); const prCreateIdx = calls.findIndex( (tc) =>