PAPA-430: workspace finalize gates + no-remote-git enforcement (#6969)

## Thinking Path > - Paperclip orchestrates AI agents across isolated execution workspaces; the local cwd is the only persistence boundary between runs. > - Workspace lifecycle (worktree_prepare → execute → workspace_finalize) and the wake/accept flow are what guarantee that dependent issues see a consistent worktree. > - PAPA-380 / PAPA-431 / PAPA-432 / PAPA-440 surfaced three holes in that contract: silent env reuse across assignees, dependent wakes firing before finalize, and `issue.interaction.accept` advancing before finalize landed. > - PAPA-441 / PAPA-442 then needed to document the "no remote git" contract and prevent future adapter/runtime code from quietly reintroducing `git push` as a backdoor sync. > - This pull request lands those server fixes, the static `check-no-git-push` enforcement, the AUTHORING.md cross-link, and the Cody-review follow-ups on the PAPA-430 thread. > - The benefit is that finalize is a real barrier — board accepts, dependent wakes, and operator-set env all respect it — and adapter code can't bypass it via raw `git push`. ## What Changed - **server (PAPA-380, PAPA-431):** `execution-workspace-policy` refuses silent env reuse when the assignee's resolved env disagrees with the workspace it would inherit. The inheritance protection is now scoped to the actual inheritance signal — explicit issue-level `environmentId` is honored even when the agent's default env is `null`. - **server (PAPA-432):** `heartbeat.ts` gates dependent wakes on `listUnfinalizedExecutionWorkspaceIds`, and writes a `workspace_finalize` row on the succeeded path. Write failures now surface instead of being swallowed so dependents aren't silently stranded behind a missing row. - **server (PAPA-440):** `issue-thread-interactions.acceptInteraction` adds a workspace_finalize precondition for `request_confirmation` (not `suggest_tasks`). Accept returns 409 if finalize hasn't succeeded for the latest workspace operation. - **ci (PAPA-442):** new `scripts/check-no-git-push.mjs` static check scans `packages/adapters/`, `packages/adapter-utils/`, `server/src/`, and `cli/src/` for any `git push` invocation (string or args-array). Wired into the `policy` PR job and `test:release-registry`. Operators can opt in per-call with `// paperclip:allow-git-push: <reason>`. Release scripts are out of scope by design. - **docs (PAPA-441):** `AUTHORING.md` documents the no-remote-git contract and cross-links the static check so adapter authors learn the rule and the enforcement together. - **review follow-up (PAPA-430, Cody):** three fixes — env resolver bug, accept-gate scope (request_confirmation only), and finalize record write on the succeeded path. ## Verification - `pnpm exec vitest run server/src/__tests__/execution-workspace-policy.test.ts server/src/__tests__/issue-thread-interactions-service.test.ts` → 33/33 pass - `node scripts/check-no-git-push.test.mjs` → check covers string form, args-array form, comment exclusions, and per-line allow-comment. - Manual: server compiles; the policy job runs the check in <1s before heavier jobs. ## Risks - **Behavioral shift in accept:** boards accepting `request_confirmation` while finalize is in-flight now get 409s. This is intentional — they can retry — but it changes timing on a hot path. `suggest_tasks` is unaffected. - **Workspace policy:** the env-reuse refusal is a new error path. Issues that previously silently reused an env from a different-assignee workspace will now fail-loud; the resolver still honors explicit issue-level `executionWorkspaceSettings.environmentId`. - **CI rule:** any future legitimate `git push` in scoped dirs must be marked with the allow-comment, which is the intended ergonomic. ## Model Used - Claude Opus 4.7 (`claude-opus-4-7`, extended thinking), via Claude Code in the Paperclip executor adapter. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] If this change affects the UI, I have included before/after screenshots (N/A — server/CI/docs only) - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge Closes related issues: PAPA-430, PAPA-380, PAPA-431, PAPA-432, PAPA-440, PAPA-441, PAPA-442 --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
2026-05-29 08:25:29 -07:00
parent 524e18b060
commit 1f70fd9a22
25 changed files with 21133 additions and 95 deletions
@@ -0,0 +1,196 @@
+#!/usr/bin/env node
+/**
+ * check-no-git-push.mjs
+ *
+ * Static check that rejects `git push` (and equivalent remote-mutating git
+ * invocations) inside adapter/runtime source code.
+ *
+ * Adapter and runtime code may never push to a git remote: the local
+ * execution-workspace cwd is the only persistence boundary between runs
+ * (see packages/adapters/AUTHORING.md and PAPA-432). Release tooling and
+ * developer scripts that legitimately push are out of scope because they
+ * live outside the directories scanned here.
+ *
+ * Opt-in mechanism: a line containing `paperclip:allow-git-push` (typically
+ * inside a `// paperclip:allow-git-push: <reason>` comment on the line itself
+ * or the line immediately above) suppresses the match. This is reserved for
+ * operator-configured paths that legitimately push and must be reviewed.
+ */
+
+import { readdirSync, readFileSync, statSync } from "node:fs";
+import path from "node:path";
+import process from "node:process";
+import { fileURLToPath } from "node:url";
+
+const DEFAULT_SCAN_ROOTS = [
+  "packages/adapters",
+  "packages/adapter-utils",
+  "server/src",
+  "cli/src",
+];
+
+const SCANNABLE_EXTENSIONS = new Set([".ts", ".tsx", ".js", ".mjs", ".cjs"]);
+
+const SKIP_DIRECTORY_NAMES = new Set([
+  "node_modules",
+  "dist",
+  "build",
+  ".turbo",
+  ".next",
+  "coverage",
+]);
+
+const SKIP_FILENAME_SUFFIXES = [".d.ts"];
+
+// Matches actual git push invocations in either:
+//   `git push ...` (shell command string)
+//   ["git", "push", ...] (args-array form for execSync)
+//   execFile("git", ["push", ...]) / spawn("git", ["push", ...])
+export const GIT_PUSH_PATTERNS = [
+  /\bgit[\s_-]+push\b/i,
+  /["'`]git["'`]\s*,\s*\[?\s*["'`]push["'`]/i,
+];
+// Kept for backwards-compatibility with existing tests/importers.
+export const GIT_PUSH_PATTERN = GIT_PUSH_PATTERNS[0];
+export const ALLOW_MARKER = "paperclip:allow-git-push";
+
+function lineMatchesGitPush(line) {
+  return GIT_PUSH_PATTERNS.some((pattern) => pattern.test(line));
+}
+
+function stripLineComment(line) {
+  // Strip everything from the first `//` that is not inside a string literal.
+  // This is a lightweight heuristic: we only need to remove obvious doc-style
+  // mentions of "git push" so they do not trip the check. The check still
+  // flags any match that survives comment stripping.
+  let inSingle = false;
+  let inDouble = false;
+  let inBacktick = false;
+
+  for (let index = 0; index < line.length; index += 1) {
+    const char = line[index];
+    // A character is escaped only if it's preceded by an odd number of
+    // backslashes; e.g. `"foo\\"` ends a string because the trailing `\\`
+    // is a single escaped backslash, leaving the closing `"` unescaped.
+    let backslashes = 0;
+    for (let scan = index - 1; scan >= 0 && line[scan] === "\\"; scan -= 1) {
+      backslashes += 1;
+    }
+    const isEscaped = backslashes % 2 === 1;
+
+    if (!inDouble && !inBacktick && char === "'" && !isEscaped) inSingle = !inSingle;
+    else if (!inSingle && !inBacktick && char === '"' && !isEscaped) inDouble = !inDouble;
+    else if (!inSingle && !inDouble && char === "`" && !isEscaped) inBacktick = !inBacktick;
+    else if (!inSingle && !inDouble && !inBacktick && char === "/" && line[index + 1] === "/") {
+      return line.slice(0, index);
+    }
+  }
+
+  return line;
+}
+
+export function findGitPushOffenses(text) {
+  const lines = text.split("\n");
+  const offenses = [];
+
+  for (let index = 0; index < lines.length; index += 1) {
+    const line = lines[index];
+    const stripped = stripLineComment(line);
+    if (!lineMatchesGitPush(stripped)) continue;
+
+    const previousLine = index > 0 ? lines[index - 1] : "";
+    const isAllowed = line.includes(ALLOW_MARKER) || previousLine.includes(ALLOW_MARKER);
+    if (isAllowed) continue;
+
+    offenses.push({ lineNumber: index + 1, line: line.trimEnd() });
+  }
+
+  return offenses;
+}
+
+function shouldScanFile(relativePath) {
+  if (SKIP_FILENAME_SUFFIXES.some((suffix) => relativePath.endsWith(suffix))) return false;
+  const extension = path.extname(relativePath);
+  return SCANNABLE_EXTENSIONS.has(extension);
+}
+
+export function collectScannableFiles(absoluteRoot, repoRoot) {
+  const results = [];
+  let stats;
+  try {
+    stats = statSync(absoluteRoot);
+  } catch {
+    return results;
+  }
+  if (!stats.isDirectory()) return results;
+
+  const stack = [absoluteRoot];
+  while (stack.length > 0) {
+    const current = stack.pop();
+    let entries;
+    try {
+      entries = readdirSync(current, { withFileTypes: true });
+    } catch {
+      continue;
+    }
+    for (const entry of entries) {
+      if (entry.isDirectory()) {
+        if (SKIP_DIRECTORY_NAMES.has(entry.name)) continue;
+        stack.push(path.join(current, entry.name));
+        continue;
+      }
+      const absolute = path.join(current, entry.name);
+      const relative = path.relative(repoRoot, absolute).split(path.sep).join("/");
+      if (shouldScanFile(relative)) results.push({ absolute, relative });
+    }
+  }
+
+  return results;
+}
+
+export function runCheck({ repoRoot, scanRoots = DEFAULT_SCAN_ROOTS, log = console.log, error = console.error } = {}) {
+  const allOffenses = [];
+
+  for (const scanRoot of scanRoots) {
+    const absoluteRoot = path.resolve(repoRoot, scanRoot);
+    const files = collectScannableFiles(absoluteRoot, repoRoot);
+    for (const file of files) {
+      let text;
+      try {
+        text = readFileSync(file.absolute, "utf8");
+      } catch {
+        continue;
+      }
+      const offenses = findGitPushOffenses(text);
+      for (const offense of offenses) {
+        allOffenses.push({ relative: file.relative, ...offense });
+      }
+    }
+  }
+
+  if (allOffenses.length > 0) {
+    error("ERROR: `git push` (or equivalent remote-mutating git command) found in adapter/runtime code:\n");
+    for (const offense of allOffenses) {
+      error(`  ${offense.relative}:${offense.lineNumber}: ${offense.line}`);
+    }
+    error(
+      "\nAdapter and runtime code must not push to a git remote. The local execution-workspace cwd is the only persistence boundary between runs (see packages/adapters/AUTHORING.md and PAPA-432).",
+    );
+    error(
+      `If the operator has explicitly configured a path that must push, add a \`${ALLOW_MARKER}: <reason>\` comment on the matching line or the line immediately above to opt in.`,
+    );
+    return 1;
+  }
+
+  log(`  ✓  No unapproved \`git push\` invocations found in adapter/runtime code.`);
+  return 0;
+}
+
+function isMainModule() {
+  return process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url);
+}
+
+if (isMainModule()) {
+  const repoRoot = process.cwd();
+  process.exit(runCheck({ repoRoot }));
+}
@@ -0,0 +1,170 @@
+import assert from "node:assert/strict";
+import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "node:fs";
+import os from "node:os";
+import path from "node:path";
+import test from "node:test";
+
+import {
+  ALLOW_MARKER,
+  GIT_PUSH_PATTERN,
+  collectScannableFiles,
+  findGitPushOffenses,
+  runCheck,
+} from "./check-no-git-push.mjs";
+
+test("regex matches common git push forms", () => {
+  assert.ok(GIT_PUSH_PATTERN.test("git push"));
+  assert.ok(GIT_PUSH_PATTERN.test("GIT PUSH"));
+  assert.ok(GIT_PUSH_PATTERN.test("git  push origin master"));
+  assert.ok(GIT_PUSH_PATTERN.test("git-push"));
+  assert.ok(GIT_PUSH_PATTERN.test("git_push"));
+});
+
+test("regex ignores unrelated `push` usages", () => {
+  assert.ok(!GIT_PUSH_PATTERN.test("args.push('git')"));
+  assert.ok(!GIT_PUSH_PATTERN.test("notes.push('git remote')"));
+  assert.ok(!GIT_PUSH_PATTERN.test("pushed"));
+  assert.ok(!GIT_PUSH_PATTERN.test("git fetch"));
+});
+
+test("findGitPushOffenses flags a bare invocation in a string", () => {
+  const text = `await exec("git push origin master");\n`;
+  const offenses = findGitPushOffenses(text);
+  assert.equal(offenses.length, 1);
+  assert.equal(offenses[0].lineNumber, 1);
+});
+
+test("findGitPushOffenses ignores mentions inside `//` comments", () => {
+  const text = `// sync-back alone — no \`git push\`, no fetch from any origin.\nconst x = 1;\n`;
+  assert.deepEqual(findGitPushOffenses(text), []);
+});
+
+test("findGitPushOffenses allows opt-in marker on the same line", () => {
+  const text = `await exec("git push origin master"); // ${ALLOW_MARKER}: operator-configured release mirror\n`;
+  assert.deepEqual(findGitPushOffenses(text), []);
+});
+
+test("findGitPushOffenses allows opt-in marker on the line above", () => {
+  const text = `// ${ALLOW_MARKER}: operator-configured release mirror\nawait exec("git push origin master");\n`;
+  assert.deepEqual(findGitPushOffenses(text), []);
+});
+
+test("findGitPushOffenses flags string-literal push even when text is split across mixed quotes", () => {
+  const text = "const cmd = `git push --tags`;\n";
+  const offenses = findGitPushOffenses(text);
+  assert.equal(offenses.length, 1);
+});
+
+test("findGitPushOffenses flags args-array form passed to spawn/execFile", () => {
+  const cases = [
+    `spawn("git", ["push", "origin", "main"]);\n`,
+    `execFile('git', ['push', '--tags']);\n`,
+    "execFile(`git`, [`push`, `--mirror`]);\n",
+  ];
+  for (const text of cases) {
+    const offenses = findGitPushOffenses(text);
+    assert.equal(offenses.length, 1, `expected match for ${text}`);
+  }
+});
+
+test("findGitPushOffenses ignores `git push` in a comment after a string ending with a literal backslash", () => {
+  // The closing `"` after `\\` should end the string (even literal count of
+  // backslashes leaves the quote unescaped), so the `// git push` that
+  // follows is comment text and must be stripped.
+  const text = 'const path = "C:\\\\"; // git push origin master\nconst y = 2;\n';
+  assert.deepEqual(findGitPushOffenses(text), []);
+});
+
+test("findGitPushOffenses does not flag args-array form when allow marker is present", () => {
+  const text = `// ${ALLOW_MARKER}: release tooling adapter\nspawn("git", ["push", "origin", "main"]);\n`;
+  assert.deepEqual(findGitPushOffenses(text), []);
+});
+
+test("runCheck passes when scoped tree has no offenses", () => {
+  const tmpRoot = mkdtempSync(path.join(os.tmpdir(), "no-git-push-pass-"));
+  try {
+    mkdirSync(path.join(tmpRoot, "packages/adapters/sample/src"), { recursive: true });
+    writeFileSync(
+      path.join(tmpRoot, "packages/adapters/sample/src/index.ts"),
+      "export const ok = 1;\n",
+    );
+    const logs = [];
+    const errors = [];
+    const code = runCheck({
+      repoRoot: tmpRoot,
+      scanRoots: ["packages/adapters"],
+      log: (msg) => logs.push(msg),
+      error: (msg) => errors.push(msg),
+    });
+    assert.equal(code, 0);
+    assert.equal(errors.length, 0);
+  } finally {
+    rmSync(tmpRoot, { recursive: true, force: true });
+  }
+});
+
+test("runCheck fails when scoped tree contains an unapproved git push", () => {
+  const tmpRoot = mkdtempSync(path.join(os.tmpdir(), "no-git-push-fail-"));
+  try {
+    mkdirSync(path.join(tmpRoot, "packages/adapters/sample/src"), { recursive: true });
+    writeFileSync(
+      path.join(tmpRoot, "packages/adapters/sample/src/index.ts"),
+      "import { execSync } from 'node:child_process';\nexecSync('git push origin main');\n",
+    );
+    const logs = [];
+    const errors = [];
+    const code = runCheck({
+      repoRoot: tmpRoot,
+      scanRoots: ["packages/adapters"],
+      log: (msg) => logs.push(msg),
+      error: (msg) => errors.push(msg),
+    });
+    assert.equal(code, 1);
+    assert.ok(errors.some((line) => line.includes("packages/adapters/sample/src/index.ts:2")));
+  } finally {
+    rmSync(tmpRoot, { recursive: true, force: true });
+  }
+});
+
+test("runCheck ignores opt-in marker outside the scoped tree", () => {
+  const tmpRoot = mkdtempSync(path.join(os.tmpdir(), "no-git-push-scope-"));
+  try {
+    mkdirSync(path.join(tmpRoot, "scripts"), { recursive: true });
+    writeFileSync(
+      path.join(tmpRoot, "scripts/release.mjs"),
+      "execSync('git push origin v1.2.3');\n",
+    );
+    const code = runCheck({
+      repoRoot: tmpRoot,
+      scanRoots: ["packages/adapters", "server/src"],
+      log: () => {},
+      error: () => {},
+    });
+    assert.equal(code, 0);
+  } finally {
+    rmSync(tmpRoot, { recursive: true, force: true });
+  }
+});
+
+test("collectScannableFiles skips node_modules, dist, and .d.ts", () => {
+  const tmpRoot = mkdtempSync(path.join(os.tmpdir(), "no-git-push-collect-"));
+  try {
+    const adaptersRoot = path.join(tmpRoot, "packages/adapters/sample");
+    mkdirSync(path.join(adaptersRoot, "src"), { recursive: true });
+    mkdirSync(path.join(adaptersRoot, "dist"), { recursive: true });
+    mkdirSync(path.join(adaptersRoot, "node_modules/pkg"), { recursive: true });
+    writeFileSync(path.join(adaptersRoot, "src/index.ts"), "");
+    writeFileSync(path.join(adaptersRoot, "src/types.d.ts"), "");
+    writeFileSync(path.join(adaptersRoot, "dist/index.js"), "");
+    writeFileSync(path.join(adaptersRoot, "node_modules/pkg/index.js"), "");
+
+    const files = collectScannableFiles(
+      path.join(tmpRoot, "packages/adapters"),
+      tmpRoot,
+    );
+    const relatives = files.map((entry) => entry.relative).sort();
+    assert.deepEqual(relatives, ["packages/adapters/sample/src/index.ts"]);
+  } finally {
+    rmSync(tmpRoot, { recursive: true, force: true });
+  }
+});