From e79b23f24fb6a45179dd134aaba3f197a7eb3922 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 6 Oct 2026 21:02:06 +0000 Subject: [PATCH] fix(ship): PR body drafts and rejected test paths are agent-written files Co-authored-by: dgrant The PR body is written into two private drafts (through ## Documentation, and from ## Test plan on); the scan block splices Step 14.5's section between them into the scanned temp file and deletes the drafts. Rejected test paths are read from a written list, and paths outside the repository are skipped. Tests that ran the old heredoc blocks now write the drafts and list first. --- ship/sections/pr-body.md | 34 +++++++++++++++++++---------- ship/sections/pr-body.md.tmpl | 26 ++++++++++++---------- ship/sections/test-coverage.md | 22 +++++++++++++++---- ship/sections/test-coverage.md.tmpl | 15 +++++++++---- test/ship-publication-gates.test.ts | 25 +++++++++++++++++---- test/test-value-bar.test.ts | 15 +++++++++---- 6 files changed, 99 insertions(+), 38 deletions(-) diff --git a/ship/sections/pr-body.md b/ship/sections/pr-body.md index e99ebebe4..9b7ebcf18 100644 --- a/ship/sections/pr-body.md +++ b/ship/sections/pr-body.md @@ -94,7 +94,8 @@ Unavailable/inconclusive is never PASS.> +blocked status, scope and exception here and drop the block's `DOCS_SECTION_FILE` guard +and its `cat -- "$DOCS_SECTION_FILE" && echo &&` step.> ## Test plan @@ -106,15 +107,29 @@ blocked status, scope and exception here and omit those lines.> #### Redaction scan (PR body + title) — runs before create AND edit The PR body is world-readable on a public repo. Scan-at-sink before sending: -write the composed body to a temp file, scan THAT file with the shared engine, +compose the body into a temp file, scan THAT file with the shared engine, and pass the same file to `gh`/`glab`. Wrap any Codex / Greptile / eval output sections in tool-attributed fences (` ```codex-review ` / ` ```greptile `) so the engine WARN-degrades the example credentials those tools quote instead of blocking the PR (a live-format credential inside the fence still blocks). +Write the body from above into two private files: the first through the +`## Documentation` heading line, the second from `## Test plan` on. + +```bash +_GT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)/.gstack/tmp" +mkdir -p "$_GT" && chmod 700 "$_GT" || { echo "Not sent: cannot create $_GT for the text file." >&2; exit 1; } +_EX=$(git rev-parse --git-path info/exclude 2>/dev/null) && mkdir -p "$(dirname "$_EX")" && { grep -qxF '/.gstack/tmp/' "$_EX" 2>/dev/null || echo '/.gstack/tmp/' >> "$_EX"; } +BODY_TOP_FILE=$(mktemp "$_GT/pr-body-top.XXXXXX") && echo "BODY_TOP_FILE: $BODY_TOP_FILE (name: ${BODY_TOP_FILE##*/})" +BODY_REST_FILE=$(mktemp "$_GT/pr-body-rest.XXXXXX") && echo "BODY_REST_FILE: $BODY_REST_FILE (name: ${BODY_REST_FILE##*/})" +``` + +Write the text into each printed file with your file-write tool (Claude Code's Write tool needs a Read of the empty file first), exactly as it should appear. The text never goes into a shell command, heredoc or quoted argument. If a write fails or is refused, do not send: print the cause, the file path and the command below for sending by hand. + Use Step 18's `NEW_TITLE` unchanged; its version prefix is already present. In a new shell, restore the saved literal title before this block, and Step 14.5's -saved section file path as `DOCS_SECTION_FILE`. +saved section file path as `DOCS_SECTION_FILE`. Substitute the printed names. The +block deletes both drafts; to change the body, write fresh ones. ```bash : "${NEW_TITLE:?Restore the saved Step 18 title before scanning}" @@ -123,14 +138,11 @@ REDACT_VIS=$(~/.claude/skills/gstack/bin/gstack-config get redact_repo_visibilit [ -z "$REDACT_VIS" ] && REDACT_VIS=$(gh repo view --json visibility -q .visibility 2>/dev/null | tr 'A-Z' 'a-z') REDACT_VIS="${REDACT_VIS:-unknown}" PR_BODY_FILE=$(mktemp "${TMPDIR:-/tmp}/gstack-pr-body.XXXXXX") || { echo "ERROR: mktemp failed — cannot scan the PR body; refusing to create the PR unscanned." >&2; exit 1; } -{ cat <<'PR_BODY_EOF' - -PR_BODY_EOF -cat -- "$DOCS_SECTION_FILE" && echo || exit 1 -cat <<'PR_BODY_EOF' - -PR_BODY_EOF -} > "$PR_BODY_FILE" || exit 1 +BODY_TOP_FILE="$(git rev-parse --show-toplevel 2>/dev/null || pwd)/.gstack/tmp/" +BODY_REST_FILE="$(git rev-parse --show-toplevel 2>/dev/null || pwd)/.gstack/tmp/" +[ -s "$BODY_TOP_FILE" ] && [ -s "$BODY_REST_FILE" ] || { echo "Not scanned: $BODY_TOP_FILE or $BODY_REST_FILE is empty, so the body was never written. Write both, then rerun." >&2; exit 1; } +{ awk 1 "$BODY_TOP_FILE" && cat -- "$DOCS_SECTION_FILE" && echo && awk 1 "$BODY_REST_FILE"; } > "$PR_BODY_FILE" || exit 1 +rm -f "$BODY_TOP_FILE" "$BODY_REST_FILE" ~/.claude/skills/gstack/bin/gstack-redact --from-file "$PR_BODY_FILE" --repo-visibility "$REDACT_VIS" --self-email "$(git config user.email 2>/dev/null)" --json case $? in 0) ;; diff --git a/ship/sections/pr-body.md.tmpl b/ship/sections/pr-body.md.tmpl index 7c802d6f1..807155261 100644 --- a/ship/sections/pr-body.md.tmpl +++ b/ship/sections/pr-body.md.tmpl @@ -92,7 +92,8 @@ Unavailable/inconclusive is never PASS.> +blocked status, scope and exception here and drop the block's `DOCS_SECTION_FILE` guard +and its `cat -- "$DOCS_SECTION_FILE" && echo &&` step.> ## Test plan @@ -104,15 +105,21 @@ blocked status, scope and exception here and omit those lines.> #### Redaction scan (PR body + title) — runs before create AND edit The PR body is world-readable on a public repo. Scan-at-sink before sending: -write the composed body to a temp file, scan THAT file with the shared engine, +compose the body into a temp file, scan THAT file with the shared engine, and pass the same file to `gh`/`glab`. Wrap any Codex / Greptile / eval output sections in tool-attributed fences (` ```codex-review ` / ` ```greptile `) so the engine WARN-degrades the example credentials those tools quote instead of blocking the PR (a live-format credential inside the fence still blocks). +Write the body from above into two private files: the first through the +`## Documentation` heading line, the second from `## Test plan` on. + +{{FREE_TEXT_FILE:BODY_TOP_FILE=pr-body-top:BODY_REST_FILE=pr-body-rest}} + Use Step 18's `NEW_TITLE` unchanged; its version prefix is already present. In a new shell, restore the saved literal title before this block, and Step 14.5's -saved section file path as `DOCS_SECTION_FILE`. +saved section file path as `DOCS_SECTION_FILE`. Substitute the printed names. The +block deletes both drafts; to change the body, write fresh ones. ```bash : "${NEW_TITLE:?Restore the saved Step 18 title before scanning}" @@ -121,14 +128,11 @@ REDACT_VIS=$(~/.claude/skills/gstack/bin/gstack-config get redact_repo_visibilit [ -z "$REDACT_VIS" ] && REDACT_VIS=$(gh repo view --json visibility -q .visibility 2>/dev/null | tr 'A-Z' 'a-z') REDACT_VIS="${REDACT_VIS:-unknown}" PR_BODY_FILE=$(mktemp "${TMPDIR:-/tmp}/gstack-pr-body.XXXXXX") || { echo "ERROR: mktemp failed — cannot scan the PR body; refusing to create the PR unscanned." >&2; exit 1; } -{ cat <<'PR_BODY_EOF' - -PR_BODY_EOF -cat -- "$DOCS_SECTION_FILE" && echo || exit 1 -cat <<'PR_BODY_EOF' - -PR_BODY_EOF -} > "$PR_BODY_FILE" || exit 1 +BODY_TOP_FILE="$(git rev-parse --show-toplevel 2>/dev/null || pwd)/.gstack/tmp/" +BODY_REST_FILE="$(git rev-parse --show-toplevel 2>/dev/null || pwd)/.gstack/tmp/" +[ -s "$BODY_TOP_FILE" ] && [ -s "$BODY_REST_FILE" ] || { echo "Not scanned: $BODY_TOP_FILE or $BODY_REST_FILE is empty, so the body was never written. Write both, then rerun." >&2; exit 1; } +{ awk 1 "$BODY_TOP_FILE" && cat -- "$DOCS_SECTION_FILE" && echo && awk 1 "$BODY_REST_FILE"; } > "$PR_BODY_FILE" || exit 1 +rm -f "$BODY_TOP_FILE" "$BODY_REST_FILE" ~/.claude/skills/gstack/bin/gstack-redact --from-file "$PR_BODY_FILE" --repo-visibility "$REDACT_VIS" --self-email "$(git config user.email 2>/dev/null)" --json case $? in 0) ;; diff --git a/ship/sections/test-coverage.md b/ship/sections/test-coverage.md index 348470a07..bbce19e9f 100644 --- a/ship/sections/test-coverage.md +++ b/ship/sections/test-coverage.md @@ -336,15 +336,29 @@ After your analysis, output a single JSON object on the LAST LINE of your respon caller check is `needs_seam`). Move each failure to `tests_rejected` with its `reason_code`, then remove it before anything else reads the diff: an untracked new file is deleted; for a tracked file, revert only this run's hunk with Edit, never - the whole file. + the whole file. Write one rejected test path per line, relative to the repository + root, into a private file: + +```bash +_GT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)/.gstack/tmp" +mkdir -p "$_GT" && chmod 700 "$_GT" || { echo "Not sent: cannot create $_GT for the text file." >&2; exit 1; } +_EX=$(git rev-parse --git-path info/exclude 2>/dev/null) && mkdir -p "$(dirname "$_EX")" && { grep -qxF '/.gstack/tmp/' "$_EX" 2>/dev/null || echo '/.gstack/tmp/' >> "$_EX"; } +REJECTED_FILE=$(mktemp "$_GT/rejected-tests.XXXXXX") && echo "REJECTED_FILE: $REJECTED_FILE (name: ${REJECTED_FILE##*/})" +``` + +Write the text into each printed file with your file-write tool (Claude Code's Write tool needs a Read of the empty file first), exactly as it should appear. The text never goes into a shell command, heredoc or quoted argument. If a write fails or is refused, do not send: print the cause, the file path and the command below for sending by hand. + + Then run, substituting the printed name for ``: ```bash + REJECTED_FILE="$(git rev-parse --show-toplevel 2>/dev/null || pwd)/.gstack/tmp/" + [ -s "$REJECTED_FILE" ] || { echo "Nothing removed: $REJECTED_FILE is empty; write the paths, then rerun." >&2; exit 1; } while IFS= read -r f; do [ -n "$f" ] || continue + case "$f" in /*|..|../*|*/..|*/../*) echo "SKIPPED (outside repo): $f"; continue ;; esac if git ls-files --error-unmatch -- "$f" >/dev/null 2>&1; then echo "REVERT_HUNK: $f"; else rm -f -- "$f" && echo "REMOVED: $f"; fi - done <<'REJECTED' - - REJECTED + done < "$REJECTED_FILE" + rm -f "$REJECTED_FILE" ``` No `tests_rejected` path may remain on disk as a new file. If every test written in diff --git a/ship/sections/test-coverage.md.tmpl b/ship/sections/test-coverage.md.tmpl index 9a897c401..e3e7f2e69 100644 --- a/ship/sections/test-coverage.md.tmpl +++ b/ship/sections/test-coverage.md.tmpl @@ -58,15 +58,22 @@ After your analysis, output a single JSON object on the LAST LINE of your respon caller check is `needs_seam`). Move each failure to `tests_rejected` with its `reason_code`, then remove it before anything else reads the diff: an untracked new file is deleted; for a tracked file, revert only this run's hunk with Edit, never - the whole file. + the whole file. Write one rejected test path per line, relative to the repository + root, into a private file: + +{{FREE_TEXT_FILE:REJECTED_FILE=rejected-tests}} + + Then run, substituting the printed name for ``: ```bash + REJECTED_FILE="$(git rev-parse --show-toplevel 2>/dev/null || pwd)/.gstack/tmp/" + [ -s "$REJECTED_FILE" ] || { echo "Nothing removed: $REJECTED_FILE is empty; write the paths, then rerun." >&2; exit 1; } while IFS= read -r f; do [ -n "$f" ] || continue + case "$f" in /*|..|../*|*/..|*/../*) echo "SKIPPED (outside repo): $f"; continue ;; esac if git ls-files --error-unmatch -- "$f" >/dev/null 2>&1; then echo "REVERT_HUNK: $f"; else rm -f -- "$f" && echo "REMOVED: $f"; fi - done <<'REJECTED' - - REJECTED + done < "$REJECTED_FILE" + rm -f "$REJECTED_FILE" ``` No `tests_rejected` path may remain on disk as a new file. If every test written in diff --git a/test/ship-publication-gates.test.ts b/test/ship-publication-gates.test.ts index eac40327c..452b581b3 100644 --- a/test/ship-publication-gates.test.ts +++ b/test/ship-publication-gates.test.ts @@ -1,10 +1,18 @@ import { expect, test } from 'bun:test'; -import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join, resolve } from 'node:path'; import { expectMentions } from './helpers/prompt-structure'; const template = readFileSync(new URL('../ship/sections/pr-body.md.tmpl', import.meta.url), 'utf8'); + +/** The agent writes the body drafts with its file-write tool; the block reads them by printed name. */ +function writeDrafts(root: string, top = '## Summary\n\n## Documentation', rest = '## Test plan\n- [x] bun test: pass'): (block: string) => string { + mkdirSync(join(root, '.gstack/tmp'), { recursive: true }); + writeFileSync(join(root, '.gstack/tmp/pr-body-top.abc123'), top); + writeFileSync(join(root, '.gstack/tmp/pr-body-rest.abc123'), rest); + return block => block.replace('', 'pr-body-top.abc123').replace('', 'pr-body-rest.abc123'); +} const scanner = resolve(import.meta.dir, '../bin/gstack-redact'); for (const failure of ['none', 'body', 'title'] as const) { @@ -26,7 +34,7 @@ exec ${JSON.stringify(process.execPath)} ${JSON.stringify(scanner)} "$@" const block = template.match(/```bash\n(: "\$\{NEW_TITLE:[\s\S]*?)\n```/)?.[1]; expect(block).toBeDefined(); writeFileSync(join(root, 'section.md'), '**Status:** current — no edits.'); - const result = Bun.spawnSync(['bash', '-c', block!], { + const result = Bun.spawnSync(['bash', '-c', writeDrafts(root)(block!)], { cwd: root, env: { ...process.env, HOME: root, NEW_TITLE: 'v1.2.3.4 fix: publication checks', FAIL_AT: failure, DOCS_SECTION_FILE: join(root, 'section.md') }, @@ -58,15 +66,24 @@ test('publication composes the saved documentation section unchanged and refuses try { const section = '**Status:** current — no edits.\n\n- Diagram drift: none (no diagrams in any doc).'; writeFileSync(join(root, 'section.md'), section); - const run = (file: string) => Bun.spawnSync(['bash', '-c', `${compose}cat -- "$PR_BODY_FILE"`], { + const run = (file: string) => Bun.spawnSync(['bash', '-c', `${writeDrafts(root)(compose)}cat -- "$PR_BODY_FILE"`], { cwd: root, env: { ...process.env, HOME: root, DOCS_SECTION_FILE: file }, stdout: 'pipe', stderr: 'pipe', timeout: 10_000, }); const composed = run(join(root, 'section.md')); expect(composed.exitCode).toBe(0); - expect(composed.stdout.toString()).toContain(`## Documentation" heading line>\n${section}\n', 'never-written').replace('', 'never-written')}cat -- "$PR_BODY_FILE"`], { + cwd: root, env: { ...process.env, HOME: root, DOCS_SECTION_FILE: join(root, 'section.md') }, stdout: 'pipe', stderr: 'pipe', timeout: 10_000, + }); + expect(unwritten.exitCode).toBe(1); + expect(unwritten.stdout.toString()).toBe(''); + expect(unwritten.stderr.toString()).toContain('the body was never written'); const unset = Bun.spawnSync(['bash', '-c', block], { cwd: root, env: { ...process.env, HOME: root, NEW_TITLE: 'v1.2.3.4 fix: publication checks', DOCS_SECTION_FILE: '' }, stdout: 'pipe', stderr: 'pipe', timeout: 10_000, diff --git a/test/test-value-bar.test.ts b/test/test-value-bar.test.ts index 2438abec1..21ca74615 100644 --- a/test/test-value-bar.test.ts +++ b/test/test-value-bar.test.ts @@ -149,9 +149,11 @@ describe('ship parent decision rules', () => { }); test('the rejection step removes new rejected files and reports tracked ones for a hunk revert', () => { - const block = shipSection.slice(shipSection.indexOf(' while IFS= read -r f; do'), shipSection.indexOf(' REJECTED\n') + ' REJECTED\n'.length) - .split('\n').map(line => line.replace(/^ {3}/, '')).join('\n'); - expect(block).toContain(''); + const start = shipSection.indexOf(' REJECTED_FILE="$(git rev-parse'); + const end = shipSection.indexOf(' rm -f "$REJECTED_FILE"\n', start) + ' rm -f "$REJECTED_FILE"\n'.length; + const block = shipSection.slice(start, end).split('\n').map(line => line.replace(/^ {3}/, '')).join('\n'); + expect(block).toContain(''); + expect(block).toContain('done < "$REJECTED_FILE"'); const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'test-value-rejected-')); try { const git = (...args: string[]) => spawnSync('git', args, { cwd: dir, encoding: 'utf8', timeout: 10_000 }); @@ -162,13 +164,18 @@ describe('ship parent decision rules', () => { git('-c', 'user.name=t', '-c', 'user.email=t@t', 'commit', '-q', '-m', 'seed'); fs.appendFileSync(path.join(dir, 'test/kept.test.ts'), 'test("dup", () => {});\n'); fs.writeFileSync(path.join(dir, 'test/new dup.test.ts'), 'test("dup", () => {});\n'); - const script = block.replace('', 'test/new dup.test.ts\ntest/kept.test.ts'); + fs.mkdirSync(path.join(dir, '.gstack/tmp'), { recursive: true }); + const list = path.join(dir, '.gstack/tmp/rejected-tests.abc123'); + fs.writeFileSync(list, 'test/new dup.test.ts\ntest/kept.test.ts\n../outside.test.ts\n'); + const script = block.replace('', 'rejected-tests.abc123'); const run = spawnSync('bash', ['-c', script], { cwd: dir, encoding: 'utf8', timeout: 10_000 }); expect(run.status).toBe(0); expect(run.stdout).toContain('REMOVED: test/new dup.test.ts'); expect(run.stdout).toContain('REVERT_HUNK: test/kept.test.ts'); + expect(run.stdout).toContain('SKIPPED (outside repo): ../outside.test.ts'); expect(fs.existsSync(path.join(dir, 'test/new dup.test.ts'))).toBe(false); expect(fs.existsSync(path.join(dir, 'test/kept.test.ts'))).toBe(true); + expect(fs.existsSync(list)).toBe(false); } finally { fs.rmSync(dir, { recursive: true, force: true }); }