fix(ship-bins): gh gets the branch as argv, never a shell; a local-path origin skips gh; RUNTIME_LINES leaves data files out

/ship review of #3633 (security CRITICAL x2): gh pr list --head "$branch" ran through sh(), so a branch named x$(cmd) executed. Replay 3 of gstack-evals #34: a local bare origin under GH_HOST failed the carry check, and RUNTIME_LINES counted JSON. The fixture's origin is now a GitHub URL mapped to the bare repo by insteadOf. Failing-first tests.

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

lib/ship-bin-common.ts

@@ -19,6 +19,15 @@ export function git(cwd: string, args: string[], timeout = 120_000): Run {
 export function bin(cwd: string, name: string, args: string[], timeout = 600_000): Run {
   return done(runScriptSync(path.join(BIN_DIR, name), args, { cwd, encoding: 'utf8', timeout, maxBuffer: 64 * 1024 * 1024 }));
 }
+/** Whether origin's configured URL names a host (https, ssh or scp form), before any insteadOf rewrite; a local path or file:// remote has none, so gh cannot map it to a repository. */
+export function hostedOrigin(cwd: string): boolean {
+  const url = git(cwd, ['config', '--get', 'remote.origin.url']).stdout.trim();
+  return /^(?:[a-z][a-z0-9+.-]*:\/\/(?:[^@/\s]+@)?[^/:\s]+[:/]|[^@/\s]+@[^/:\s]+:)\S+$/i.test(url) && !/^file:/i.test(url);
+}
+/** gh with argv, never a shell, so a branch name cannot change what runs. */
+export function gh(cwd: string, args: string[], timeout = 60_000): Run {
+  return done(spawnSync('gh', args, { cwd, encoding: 'utf8', timeout, maxBuffer: 64 * 1024 * 1024, env: { ...process.env, GIT_TERMINAL_PROMPT: '0' } }));
+}
 export function sh(cwd: string, command: string, timeout = 1_800_000): Run {
   return done(runBashSync(command, { cwd, encoding: 'utf8', timeout, maxBuffer: 64 * 1024 * 1024 }));
 }

lib/ship-preflight.ts

@@ -7,7 +7,9 @@
  */
 import * as fs from 'node:fs';
 import * as path from 'node:path';
-import { bin, firstLine, flagReader, git, laneLine, lanes, Report, runLanes, shipPolicy, short, sh } from './ship-bin-common';
+import { auditCodePaths } from './review-packet';
+import { matchesAny } from './ship-policy';
+import { bin, firstLine, flagReader, gh, git, hostedOrigin, laneLine, lanes, Report, runLanes, shipPolicy, short } from './ship-bin-common';
 
 export const PREFLIGHT_HELP = `gstack-ship-preflight — /ship Steps 1-3 and Step 5's pregate and lanes in one call (ship/SKILL.md)
 
@@ -34,9 +36,10 @@ function integrate(cwd: string, base: string, r: Report): boolean {
   const lines = [...stat.matchAll(/(\d+) (insertion|deletion)/g)].reduce((n, m) => n + Number(m[1]), 0);
   const commits = git(cwd, ['rev-list', '--count', `origin/${base}..HEAD`]).stdout.trim();
   r.add(`DIFF: ${stat || 'no changes'}; commits=${commits || 0}${lines > 200 ? '; over 200 lines: recommend /plan-eng-review or /autoplan' : ''}`);
-  const pr = sh(cwd, `gh pr list --head ${JSON.stringify(branch)} --state open --json number --jq '.[0].number // empty'`, 60_000);
-  const n = pr.stdout.trim();
-  if (pr.status !== 0) r.add(`CARRY: not checked (gh pr list failed: ${firstLine(pr.stderr)})`);
+  const pr = hostedOrigin(cwd) ? gh(cwd, ['pr', 'list', '--head', branch, '--state', 'open', '--json', 'number', '--jq', '.[0].number // empty']) : null;
+  const n = pr?.stdout.trim() ?? '';
+  if (!pr) r.add('PR: not checked (origin is a local path, not a hosted remote gh can map; Step 0 uses git-native commands)');
+  else if (pr.status !== 0) r.add(`CARRY: not checked (gh pr list failed: ${firstLine(pr.stderr)})`);
   else if (/^\d+$/.test(n)) {
     const carry = bin(cwd, 'gstack-ship-receipt', ['carry', '--pr', n, '--base', base, '--dry-run'], 120_000);
     const line = (carry.stdout.match(/^CARRY: .*$/m) ?? [`CARRY: check failed (${firstLine(carry.stderr)})`])[0];
@@ -86,7 +89,13 @@ function merge(cwd: string, base: string, r: Report): boolean {
   r.add(`POLICY: ${['bump', 'queue_mode', 'review_scope', 'history'].map(k => `${k}=${policy?.[k] ?? '?'}`).join(' ')} (binds Steps 12, 16 and history)`);
   const plan = bin(cwd, 'gstack-review-plan', ['--base', base, '--json'], 180_000);
   let runtime = 'unknown';
-  try { runtime = String(JSON.parse(plan.stdout).runtime_lines ?? 'unknown'); } catch { /* unknown */ }
+  try {
+    const j = JSON.parse(plan.stdout);
+    const outputs: string[] = j.registry?.outputs ?? [];
+    const counted = (j.per_path as { path: string; lines: number; binary: boolean }[]).filter(e => !e.binary && !(j.stamp_only ?? []).includes(e.path)
+      && !(outputs.length && matchesAny(outputs, e.path)) && auditCodePaths([e.path], cwd).length);
+    runtime = String(counted.reduce((n, e) => n + e.lines, 0));
+  } catch { /* unknown */ }
   r.add(`RUNTIME_LINES: ${runtime} (gstack-ship-budget check --runtime-lines at the Step 7 and 12 boundaries)`);
   return true;
 }

lib/ship-release.ts

@@ -14,7 +14,7 @@ import * as path from 'node:path';
 import { classifyPath } from './review-depth';
 import { resolveStateRoot } from './state-root';
 import { bumpVersion, cmpVersion, fmtVersion, parseVersion, versionWidth, type Bump } from './version-source';
-import { bin, firstLine, flagReader, git, laneLine, lanes, releasePaths, Report, runLanes, sh, shipPolicy, short } from './ship-bin-common';
+import { bin, firstLine, flagReader, gh, git, hostedOrigin, laneLine, lanes, releasePaths, Report, runLanes, sh, shipPolicy, short } from './ship-bin-common';
 
 export const RELEASE_HELP = `gstack-ship-release — /ship Steps 12 and 15-17 (ship/SKILL.md)
 
@@ -202,7 +202,8 @@ export function push(argv: readonly string[], cwd: string): Report {
   if (guard !== 'true' && !prompted) return r.stop('Step 17\'s one-time pre-push guard offer is unanswered: run the guard block and ask it, then rerun push');
   const gateOpen = Number(flag('--gate-open') ?? '0');
   if (gateOpen > 0) {
-    const pr = sh(cwd, `gh pr list --head ${JSON.stringify(branch)} --state open --json number,isDraft --jq '[.[] | select(.isDraft | not) | .number] | first // empty'`, 60_000);
+    if (!hostedOrigin(cwd)) return r.stop('open-PR lookup unavailable: origin is a local path, not a hosted remote gh can map; it must succeed before pushing open gate findings');
+    const pr = gh(cwd, ['pr', 'list', '--head', branch, '--state', 'open', '--json', 'number,isDraft', '--jq', '[.[] | select(.isDraft | not) | .number] | first // empty']);
     if (pr.status !== 0) return r.stop(`open-PR lookup failed (${firstLine(pr.stderr)}): it must succeed before pushing open gate findings`);
     if (pr.stdout.trim() && !has('--gate-open-approved')) return r.stop(`${gateOpen} open gate finding(s) and ready PR #${pr.stdout.trim()}: spawned/headless sessions stop here; otherwise ask (resolve now, or publish with the banner and rerun with --gate-open-approved)`);
   }

test/helpers/ship-bin-fixture.ts

@@ -21,10 +21,13 @@ export interface ShipFx {
   /** Push to origin/main from a second clone, moving the base under the feature branch. */
   advanceBase(files: Record<string, string>): void;
   gh(script: string): void;
-  run(bin: string, args: string[]): { status: number | null; stdout: string; stderr: string };
+  run(bin: string, args: string[], extraEnv?: Record<string, string>): { status: number | null; stdout: string; stderr: string };
   cleanup(): void;
 }
 
+/** origin's URL as a GitHub remote; the clone's insteadOf maps it to the bare repository, so gh calls see a GitHub repo and git still works offline. */
+export const GITHUB_URL = 'https://github.com/fx/fx.git';
+
 const IDENTITY = ['-c', 'user.email=fixture@example.com', '-c', 'user.name=Fixture', '-c', 'commit.gpgsign=false'];
 
 export function shipFixture(base: Record<string, string> = {}, feat: Record<string, string> = { 'src/b.ts': 'export const b = 2;\n' }): ShipFx {
@@ -49,6 +52,7 @@ export function shipFixture(base: Record<string, string> = {}, feat: Record<stri
     'README.md': '# Fx\n', 'src/a.ts': 'export const a = 1;\n', ...base });
   must(gitIn(work, ['add', '-A']), 'add'); must(gitIn(work, ['commit', '-qm', 'base']), 'seed commit');
   must(gitIn(work, ['push', '-q', 'origin', 'main']), 'push base'); gitIn(work, ['remote', 'set-head', 'origin', 'main']);
+  must(gitIn(work, ['config', `url.${origin}.insteadOf`, GITHUB_URL]), 'insteadOf'); must(gitIn(work, ['config', 'remote.origin.url', GITHUB_URL]), 'github origin');
   must(gitIn(work, ['checkout', '-qb', 'feat']), 'branch');
   writeIn(work, feat);
   must(gitIn(work, ['add', '-A']), 'add feat'); must(gitIn(work, ['commit', '-qm', 'feat']), 'feat commit');
@@ -61,14 +65,14 @@ export function shipFixture(base: Record<string, string> = {}, feat: Record<stri
     write: files => writeIn(work, files),
     commit: msg => { must(gitIn(work, ['add', '-A']), 'add'); must(gitIn(work, ['commit', '-qm', msg]), msg); },
     advanceBase: files => {
-      const other = path.join(root, `other-${Date.now()}`);
+      const other = fs.mkdtempSync(path.join(root, 'other-'));
       must(gitIn(root, ['clone', '-q', origin, other]), 'clone other');
       writeIn(other, files);
       must(gitIn(other, ['add', '-A']), 'add'); must(gitIn(other, ['commit', '-qm', 'base moves']), 'base moves'); must(gitIn(other, ['push', '-q', 'origin', 'main']), 'push');
     },
     gh: script => { fs.writeFileSync(path.join(stub, 'gh'), `#!/bin/sh\n${script}\n`, { mode: 0o755 }); },
-    run: (bin, args) => {
-      const r = spawnSync(bin, args, { cwd: work, encoding: 'utf8', timeout: 300_000, env });
+    run: (bin, args, extraEnv = {}) => {
+      const r = spawnSync(bin, args, { cwd: work, encoding: 'utf8', timeout: 300_000, env: { ...env, ...extraEnv } });
       return { status: r.status, stdout: r.stdout ?? '', stderr: r.stderr ?? '' };
     },
     cleanup: () => fs.rmSync(root, { recursive: true, force: true }),

test/ship-preflight.test.ts

@@ -120,6 +120,36 @@ describe('gstack-ship-preflight', () => {
     expect(r.stdout).toContain('NEXT: Step 4');
   });
 
+  // Value: protects=the branch name reaches gh as one argv item, never through a shell; fails_when=a branch named with $(...) runs a command on the shipper's machine;
+  //   why_new=GSTA-167 /ship review of #3633 (security, CRITICAL); seam=gh; no_claim=gh's own handling of the name
+  test('a branch name with shell syntax is never executed', () => {
+    const f = fx();
+    const name = 'x$(touch${IFS}PWNED)';
+    expect(f.git('checkout', '-qb', name).status).toBe(0);
+    f.gh('echo 7');
+    f.run(PREFLIGHT, ['--base', 'main']);
+    expect(fs.existsSync(path.join(f.work, 'PWNED'))).toBe(false);
+  });
+
+  // Value: protects=a local-path origin skips the open-PR and carry checks with a reason instead of asking gh, which cannot map it to a repository (and fails outright under a GH_HOST); fails_when=gh runs against a local remote and CARRY reports a GH_HOST failure;
+  //   why_new=GSTA-167 replay 3 of gstack-evals #34 (CARRY: not checked, GH_HOST); seam=gh; no_claim=hosted remotes, which gh resolves itself
+  test('a local-path origin is not checked by gh', () => {
+    const f = fx();
+    f.gh('echo "none of the git remotes configured for this repository correspond to the GH_HOST environment variable" >&2; exit 1');
+    f.git('config', 'remote.origin.url', f.origin);
+    const local = f.run(PREFLIGHT, ['--base', 'main'], { GH_HOST: 'ghe.example.com' });
+    expect(lineOf(local.stdout, 'PR')).toMatch(/^PR: not checked \(origin is a local path/);
+    expect(local.stdout).not.toContain('CARRY:');
+    expect(local.status).toBe(0);
+  });
+
+  // Value: protects=RUNTIME_LINES (the small-run delegation input) counts code lines the way the packets classify paths, leaving data files out; fails_when=a JSON attachment inflates the runtime count;
+  //   why_new=GSTA-167 replay 3 of gstack-evals #34 (RUNTIME_LINES 6687 counted JSON); seam=gstack-review-plan --json per_path; no_claim=the review depth's own count
+  test('RUNTIME_LINES leaves data files out', () => {
+    const f = fx({}, { 'src/b.ts': 'export const b = 2;\n', 'data/rows.json': Array.from({ length: 100 }, (_, i) => `{"i":${i}}`).join('\n') + '\n' });
+    expect(lineOf(f.run(PREFLIGHT, ['--base', 'main']).stdout, 'RUNTIME_LINES')).toMatch(/^RUNTIME_LINES: 1 /);
+  });
+
   test('usage: --base is required and --tests-only needs a lane', () => {
     const f = fx();
     expect(f.run(PREFLIGHT, []).status).toBe(2);

test/ship-release.test.ts

@@ -154,3 +154,18 @@ describe('push (Step 17)', () => {
     expect(r.stdout).toContain('never force-push');
   });
 });
+
+describe('gstack-ship-release push: branch names', () => {
+  // Value: protects=push's open-PR lookup passes the branch to gh as one argv item; fails_when=a branch named with $(...) runs a command before the push guards;
+  //   why_new=GSTA-167 /ship review of #3633 (security, CRITICAL); seam=gh; no_claim=gh's own handling of the name
+  test('a branch name with shell syntax is never executed by the gate lookup', () => {
+    const f = fx();
+    const name = 'x$(touch${IFS}PWNED)';
+    expect(f.git('checkout', '-qb', name).status).toBe(0);
+    fs.mkdirSync(f.state, { recursive: true });
+    fs.writeFileSync(path.join(f.state, '.redact-prepush-prompted'), '');
+    f.gh('echo 12');
+    f.run(RELEASE, ['push', '--branch', name, '--gate-open', '1']);
+    expect(fs.existsSync(path.join(f.work, 'PWNED'))).toBe(false);
+  });
+});