v1.57.3.0 fix(ship): always-loaded PR-title-version rule + fork-PR title-sync backstop (#1909)

* fix(ship): restore always-loaded PR-title-version invariant to skeleton The v1.54.0.0 carve moved the 'PR title MUST start with v$NEW_VERSION' rule out of the always-loaded ship skeleton and entirely into the lazily-loaded pr-body.md section. The agent only set the version prefix if it happened to read that section before creating the PR, so PRs landed with bare titles. Restore a one-line invariant (+ helper reference) to ship/SKILL.md.tmpl right before the {{SECTION:pr-body}} pointer, mirroring the AUQ always-loaded precedent. Full procedure stays sectioned. Regenerated all hosts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(ship): guard PR-title-version rule + pull_request_target safety Two free gate tests so a future carve or workflow refactor can't silently regress: - ship-pr-title-version-always-loaded: asserts the invariant lives in the always-loaded ship/SKILL.md skeleton (not only sections/), and that the skeleton+sections union keeps BOTH the create and the existing-PR update title paths. Modeled on test/auq-format-always-loaded.test.ts. - pr-title-sync-workflow-safety: static tripwire that fails CI if pr-title-sync.yml checks out PR-head code or inlines an attacker-controlled ${{ github.event.pull_request.* }} field inside a run: block (the two pull_request_target footguns actionlint cannot catch). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ci): pr-title-sync covers fork PRs via hardened pull_request_target Under plain pull_request the GITHUB_TOKEN is read-only on fork PRs, so the title-sync backstop could never edit a fork/agent PR title. Switch to pull_request_target (write token in base context) and make it safe: - Check out the base repo only (no ref:) — execute trusted infra, never fork-head code. - All attacker-controlled PR fields (title, head repo, head sha) pass via env: and are referenced as shell-quoted "$VAR", never inlined into run:. - Read the PR-head VERSION as data (raw media type) from the head repo at the head sha; guard the assignment under set -e. - Same-repo read failure fails loudly; fork miss warns and skips (the backstop stays green without going silently optional). - Never echo the raw fork title (Actions parses ::workflow-command:: from stdout). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ship): expand binDir path in pr-body Linked Spec block ship/sections/pr-body.md.tmpl:98-99 used ${ctx.paths.binDir}, but the gen-skill-docs generator only resolves {{TOKEN}} syntax in .tmpl files — the ${...} JS-template-literal form is substituted only inside .ts resolver files. So the token passed through literally into the generated pr-body.md, leaving the agent with an unexpandable ${ctx.paths.binDir}/gstack-paths command in the Linked Spec auto-detect block. Use the hardcoded helper path, consistent with every other path reference in this section. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(test): fold ship PR-title skeleton guard into carve-guard registry main shipped a generalized carve-guard system (PR #1907) that is now the single source of truth for carved-skill skeleton invariants. Register the PR-title rule there instead of a standalone test: ship's mustStayInSkeleton asserts v$NEW_VERSION + the rewrite helper stay always-loaded, and mustMoveToSection asserts both the create and update PR paths stay carved into pr-body.md (present in the union, out of the skeleton). Delete the standalone ship-pr-title-version-always-loaded test it replaces. The CI-workflow safety tripwire stays standalone (not a carve concern). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore: bump version and changelog (v1.57.3.0) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-10 20:07:49 +02:00 · 2026-06-07 22:04:18 -07:00
parent 4dfdb7cdc2
commit d8c91c6267
10 changed files with 284 additions and 26 deletions
@@ -112,8 +112,14 @@ export const CARVE_GUARDS: Record<string, CarveGuard> = {
    scenario:
      'This is a FRESH version-changing ship: the branch has a real code change, VERSION still equals the base version (needs a bump), and CHANGELOG.md needs a new entry. Follow the skill flow for a version-changing ship: run the pre-landing review and prepare the CHANGELOG entry. Produce the ship plan / review report. Do NOT actually commit, push, or open a PR.',
    staticInvariants: {
-      mustStayInSkeleton: [],
-      mustMoveToSection: [],
+      // The PR-title-version invariant MUST stay always-loaded: the v1.54.0.0
+      // carve stranded it in pr-body.md and PRs started landing with bare titles
+      // (CI backstop: test/pr-title-sync-workflow-safety.test.ts).
+      mustStayInSkeleton: ['v$NEW_VERSION', 'gstack-pr-title-rewrite'],
+      // ...while the full create/update procedure stays carved into pr-body.md
+      // (out of the skeleton, present in the union). Asserts BOTH PR paths
+      // survive: the create path and the idempotent update path.
+      mustMoveToSection: ['gh pr create --base', 'gh pr edit --title'],
      // ship is operational (multi-STOP, not a plan review); no single post-STOP gate.
      gateAfterStop: undefined,
    },
@@ -0,0 +1,135 @@
+/**
+ * pr-title-sync.yml is a `pull_request_target` workflow — static injection
+ * tripwire (gate, free).
+ *
+ * The anxiety this kills: `pull_request_target` runs with a WRITE token in the
+ * base-repo context, even for fork PRs. That is what lets this workflow rewrite
+ * fork-PR titles (the backstop). It is also the single most dangerous workflow
+ * trigger in GitHub Actions. Two classic footguns turn it into remote code
+ * execution / token theft, and `actionlint` catches NEITHER:
+ *
+ *   1. Checking out the PR head (`actions/checkout` with a `ref:` pointing at
+ *      `pull_request.head` / `head_ref`) and then running anything from it —
+ *      that executes attacker-controlled fork code with the write token.
+ *   2. Interpolating an attacker-controlled `${{ github.event.pull_request.* }}`
+ *      field directly INSIDE a `run:` block — the title/body are attacker-
+ *      controlled and the `${{ }}` is expanded into the shell before execution,
+ *      so a crafted title runs as code. Those fields MUST arrive via `env:` and
+ *      be referenced as `"$VAR"` (shell-quoted), never inlined.
+ *
+ * This tripwire reads the workflow file directly and fails CI if either pattern
+ * reappears. Mirrors the static-grep invariant tests in browse/test
+ * (terminal-agent-pid-identity, server-sanitize-surrogates).
+ *
+ * Note: `gh api ... -q '.head.sha'` inside a run block is SAFE (reading PR
+ * metadata as data via a jq filter string, not `${{ }}` interpolation), so we
+ * ban the interpolation form specifically, not the literal substring `head.sha`.
+ */
+import { describe, test, expect } from 'bun:test';
+import * as fs from 'node:fs';
+import * as path from 'node:path';
+
+const WORKFLOW = path.resolve(__dirname, '..', '.github', 'workflows', 'pr-title-sync.yml');
+
+/** Indentation width (count of leading spaces) of a line. */
+function indent(line: string): number {
+  const m = line.match(/^( *)/);
+  return m ? m[1].length : 0;
+}
+
+/**
+ * Return the lines that live inside a `run:` block, each tagged with its 1-based
+ * line number. Handles both `run: |` (multiline) and `run: <inline command>`.
+ */
+function runBlockLines(content: string): Array<{ n: number; text: string }> {
+  const lines = content.split('\n');
+  const out: Array<{ n: number; text: string }> = [];
+  let inRun = false;
+  let runIndent = -1;
+  for (let i = 0; i < lines.length; i++) {
+    const line = lines[i];
+    const n = i + 1;
+    const inlineRun = line.match(/^(\s*)run:\s*(\S.*)$/); // `run: echo foo`
+    const blockRun = /^(\s*)run:\s*(\|>?[+-]?)?\s*$/.test(line); // `run: |`
+    if (inlineRun && !/^\|/.test(inlineRun[2])) {
+      out.push({ n, text: inlineRun[2] });
+      inRun = false;
+      continue;
+    }
+    if (blockRun) {
+      inRun = true;
+      runIndent = indent(line);
+      continue;
+    }
+    if (inRun) {
+      if (line.trim() === '') {
+        out.push({ n, text: line });
+        continue;
+      }
+      // Block ends when a non-empty line is indented at or below the `run:` key.
+      if (indent(line) <= runIndent) {
+        inRun = false;
+      } else {
+        out.push({ n, text: line });
+      }
+    }
+  }
+  return out;
+}
+
+describe('pr-title-sync.yml pull_request_target safety', () => {
+  const content = fs.readFileSync(WORKFLOW, 'utf-8');
+
+  test('workflow file exists', () => {
+    expect(fs.existsSync(WORKFLOW)).toBe(true);
+  });
+
+  test('does NOT check out the PR head ref (no fork-code execution)', () => {
+    const offenders: string[] = [];
+    content.split('\n').forEach((line, i) => {
+      // A checkout `ref:` (or any `ref:`) pointing at the PR head is the footgun.
+      if (/ref:\s*\$\{\{[^}]*(pull_request\.head|head_ref)/.test(line)) {
+        offenders.push(`  L${i + 1}: ${line.trim()}`);
+      }
+    });
+    if (offenders.length > 0) {
+      throw new Error(
+        `pr-title-sync.yml checks out the PR head under pull_request_target — that ` +
+          `runs attacker-controlled fork code with a write token. Check out the base ` +
+          `repo (no ref:) and read PR-head data via the API instead.\n` +
+          offenders.join('\n'),
+      );
+    }
+  });
+
+  test('does NOT interpolate ${{ github.event.pull_request.* }} inside a run: block', () => {
+    const offenders: string[] = [];
+    for (const { n, text } of runBlockLines(content)) {
+      if (/\$\{\{\s*github\.event\.pull_request/.test(text)) {
+        offenders.push(`  L${n}: ${text.trim()}`);
+      }
+    }
+    if (offenders.length > 0) {
+      throw new Error(
+        `pr-title-sync.yml inlines an attacker-controlled PR field into a run: block ` +
+          `— a crafted PR title/body executes as shell. Pass it via env: and ` +
+          `reference "$VAR" (shell-quoted) instead.\n` +
+          offenders.join('\n'),
+      );
+    }
+  });
+
+  test('uses pull_request_target (the hardening is actually present)', () => {
+    // Positive assertion: if someone reverts to plain pull_request, the fork
+    // backstop silently stops working (read-only token). Keep it intentional.
+    expect(/^on:\s*$/m.test(content) || /\bpull_request_target\b/.test(content)).toBe(true);
+    expect(content).toMatch(/\bpull_request_target\b/);
+  });
+
+  test('passes the PR title through env:, not raw interpolation', () => {
+    // The safe pattern: OLD_TITLE: ${{ github.event.pull_request.title }} in an
+    // env: mapping, consumed as "$OLD_TITLE" in script.
+    expect(content).toMatch(/env:/);
+    expect(content).toMatch(/github\.event\.pull_request\.title/);
+  });
+});