From f92c5f6c9dcae47c763ddd39dde4bc8e9f18f724 Mon Sep 17 00:00:00 2001 From: Denis Darii Date: Fri, 3 Apr 2026 09:59:58 +0200 Subject: [PATCH] Validate Linear branchName before spawn --- .../__tests__/session-manager/spawn.test.ts | 76 +++++++++++++++++++ packages/core/src/__tests__/utils.test.ts | 31 +++++++- packages/core/src/index.ts | 1 + packages/core/src/session-manager.ts | 9 ++- packages/core/src/utils.ts | 22 ++++++ .../src/tracker-linear.integration.test.ts | 2 +- packages/plugins/tracker-linear/src/index.ts | 6 +- scripts/claude-batch-spawn | 4 +- 8 files changed, 141 insertions(+), 10 deletions(-) diff --git a/packages/core/src/__tests__/session-manager/spawn.test.ts b/packages/core/src/__tests__/session-manager/spawn.test.ts index 1803ff714..3e8006b4e 100644 --- a/packages/core/src/__tests__/session-manager/spawn.test.ts +++ b/packages/core/src/__tests__/session-manager/spawn.test.ts @@ -137,6 +137,82 @@ describe("spawn", () => { expect(session.branch).toBe("ABC-1234"); }); + it("uses tracker.branchName when Issue omits branchName", async () => { + const mockTracker: Tracker = { + name: "mock-tracker", + getIssue: vi.fn().mockResolvedValue({ + id: "INT-100", + title: "T", + description: "", + url: "https://tracker.test/INT-100", + state: "open", + labels: [], + }), + isCompleted: vi.fn().mockResolvedValue(false), + issueUrl: vi.fn().mockReturnValue(""), + branchName: vi.fn().mockReturnValue("custom/INT-100-my-feature"), + generatePrompt: vi.fn().mockResolvedValue(""), + }; + + const registryWithTracker: PluginRegistry = { + ...mockRegistry, + get: vi.fn().mockImplementation((slot: string) => { + if (slot === "runtime") return mockRuntime; + if (slot === "agent") return mockAgent; + if (slot === "workspace") return mockWorkspace; + if (slot === "tracker") return mockTracker; + return null; + }), + }; + + const sm = createSessionManager({ + config, + registry: registryWithTracker, + }); + + const session = await sm.spawn({ projectId: "my-app", issueId: "INT-100" }); + expect(session.branch).toBe("custom/INT-100-my-feature"); + expect(mockTracker.branchName).toHaveBeenCalledWith("INT-100", expect.anything()); + }); + + it("falls back to tracker.branchName when Issue.branchName is not git-safe", async () => { + const mockTracker: Tracker = { + name: "mock-tracker", + getIssue: vi.fn().mockResolvedValue({ + id: "INT-100", + title: "T", + description: "", + url: "https://tracker.test/INT-100", + state: "open", + labels: [], + branchName: "bad branch with spaces", + }), + isCompleted: vi.fn().mockResolvedValue(false), + issueUrl: vi.fn().mockReturnValue(""), + branchName: vi.fn().mockReturnValue("feat/INT-100"), + generatePrompt: vi.fn().mockResolvedValue(""), + }; + + const registryWithTracker: PluginRegistry = { + ...mockRegistry, + get: vi.fn().mockImplementation((slot: string) => { + if (slot === "runtime") return mockRuntime; + if (slot === "agent") return mockAgent; + if (slot === "workspace") return mockWorkspace; + if (slot === "tracker") return mockTracker; + return null; + }), + }; + + const sm = createSessionManager({ + config, + registry: registryWithTracker, + }); + + const session = await sm.spawn({ projectId: "my-app", issueId: "INT-100" }); + expect(session.branch).toBe("feat/INT-100"); + }); + it("sanitizes free-text issueId into a valid branch slug", async () => { const sm = createSessionManager({ config, registry: mockRegistry }); diff --git a/packages/core/src/__tests__/utils.test.ts b/packages/core/src/__tests__/utils.test.ts index 341040f39..85f12643f 100644 --- a/packages/core/src/__tests__/utils.test.ts +++ b/packages/core/src/__tests__/utils.test.ts @@ -2,7 +2,12 @@ import { describe, it, expect, afterEach } from "vitest"; import { mkdtempSync, writeFileSync, rmSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; -import { isRetryableHttpStatus, normalizeRetryConfig, readLastJsonlEntry } from "../utils.js"; +import { + isGitBranchNameSafe, + isRetryableHttpStatus, + normalizeRetryConfig, + readLastJsonlEntry, +} from "../utils.js"; import { parsePrFromUrl } from "../utils/pr.js"; describe("readLastJsonlEntry", () => { @@ -87,6 +92,30 @@ describe("readLastJsonlEntry", () => { }); }); +describe("isGitBranchNameSafe", () => { + it("accepts typical Linear-style branch names", () => { + expect(isGitBranchNameSafe("feature/foo-bar-123")).toBe(true); + expect(isGitBranchNameSafe("feat/INT-123")).toBe(true); + }); + + it("rejects empty, @, lock suffix, double dots, and leading dot", () => { + expect(isGitBranchNameSafe("")).toBe(false); + expect(isGitBranchNameSafe("@")).toBe(false); + expect(isGitBranchNameSafe("foo.lock")).toBe(false); + expect(isGitBranchNameSafe("a..b")).toBe(false); + expect(isGitBranchNameSafe(".hidden")).toBe(false); + }); + + it("rejects characters invalid in git refs", () => { + expect(isGitBranchNameSafe("bad branch")).toBe(false); + expect(isGitBranchNameSafe("x:y")).toBe(false); + expect(isGitBranchNameSafe("x~y")).toBe(false); + expect(isGitBranchNameSafe("x?y")).toBe(false); + expect(isGitBranchNameSafe("x[y]")).toBe(false); + expect(isGitBranchNameSafe("a\nb")).toBe(false); + }); +}); + describe("retry utilities", () => { it("marks 429 and 5xx statuses as retryable", () => { expect(isRetryableHttpStatus(429)).toBe(true); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index ef028390a..c8ddac0c5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -98,6 +98,7 @@ export { shellEscape, escapeAppleScript, validateUrl, + isGitBranchNameSafe, isRetryableHttpStatus, normalizeRetryConfig, readLastJsonlEntry, diff --git a/packages/core/src/session-manager.ts b/packages/core/src/session-manager.ts index e1d22f987..dfd53b4fd 100644 --- a/packages/core/src/session-manager.ts +++ b/packages/core/src/session-manager.ts @@ -73,6 +73,7 @@ import { } from "./global-pause.js"; import { sessionFromMetadata } from "./utils/session-from-metadata.js"; import { safeJsonParse } from "./utils/validation.js"; +import { isGitBranchNameSafe } from "./utils.js"; import { resolveAgentSelection, resolveSessionRole } from "./agent-selection.js"; const execFileAsync = promisify(execFile); @@ -952,9 +953,11 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM if (spawnConfig.branch) { branch = spawnConfig.branch; } else if (spawnConfig.issueId && plugins.tracker && resolvedIssue) { - branch = resolvedIssue.branchName - ? resolvedIssue.branchName - : plugins.tracker.branchName(spawnConfig.issueId, project); + const fromIssue = resolvedIssue.branchName; + branch = + fromIssue && isGitBranchNameSafe(fromIssue) + ? fromIssue + : plugins.tracker.branchName(spawnConfig.issueId, project); } else if (spawnConfig.issueId) { // If the issueId is already branch-safe (e.g. "INT-9999"), use as-is. // Otherwise sanitize free-text (e.g. "fix login bug") into a valid slug. diff --git a/packages/core/src/utils.ts b/packages/core/src/utils.ts index ad27fd858..ee3bb7465 100644 --- a/packages/core/src/utils.ts +++ b/packages/core/src/utils.ts @@ -33,6 +33,28 @@ export function validateUrl(url: string, label: string): void { } } +/** + * Conservative subset of git `check-ref-format` rules for branch-like names. + * Used before passing tracker-supplied names to `git worktree` / `checkout -b`. + * + * Slashes are allowed (e.g. `feature/foo-bar`). + */ +export function isGitBranchNameSafe(name: string): boolean { + if (!name) return false; + if (name === "@" || name.startsWith(".") || name.endsWith(".") || name.endsWith("/")) return false; + if (name.endsWith(".lock")) return false; + if (name.includes("..")) return false; + if (name.includes("@{")) return false; + if (name.startsWith("/")) return false; + for (let i = 0; i < name.length; i++) { + const c = name.charCodeAt(i); + if (c <= 0x1f || c === 0x7f) return false; + } + // Space and git-forbidden punctuation (see git-check-ref-format) + if (/[\s~^:?*[\\]/.test(name)) return false; + return true; +} + /** * Returns true if an HTTP status code should be retried. * Retry only 429 (rate-limit) and 5xx (server) failures. diff --git a/packages/integration-tests/src/tracker-linear.integration.test.ts b/packages/integration-tests/src/tracker-linear.integration.test.ts index 41d0ed0c5..6aedde21f 100644 --- a/packages/integration-tests/src/tracker-linear.integration.test.ts +++ b/packages/integration-tests/src/tracker-linear.integration.test.ts @@ -220,7 +220,7 @@ describe.skipIf(!canRun)("tracker-linear (integration)", () => { expect(issue.branchName).toBeDefined(); expect(typeof issue.branchName).toBe("string"); - expect(issue.branchName).toContain(issueIdentifier); + expect(issue.branchName!.length).toBeGreaterThan(0); }); it("isCompleted returns false for an open issue", async () => { diff --git a/packages/plugins/tracker-linear/src/index.ts b/packages/plugins/tracker-linear/src/index.ts index 8e61d985e..bef407675 100644 --- a/packages/plugins/tracker-linear/src/index.ts +++ b/packages/plugins/tracker-linear/src/index.ts @@ -296,7 +296,7 @@ function createLinearTracker(query: GraphQLTransport): Tracker { labels: node.labels.nodes.map((l) => l.name), assignee: node.assignee?.displayName ?? node.assignee?.name, priority: node.priority, - ...(node.branchName ? { branchName: node.branchName } : {}), + branchName: node.branchName ?? undefined, }; }, @@ -429,7 +429,7 @@ function createLinearTracker(query: GraphQLTransport): Tracker { labels: node.labels.nodes.map((l) => l.name), assignee: node.assignee?.displayName ?? node.assignee?.name, priority: node.priority, - ...(node.branchName ? { branchName: node.branchName } : {}), + branchName: node.branchName ?? undefined, })); }, @@ -622,7 +622,7 @@ function createLinearTracker(query: GraphQLTransport): Tracker { labels: node.labels.nodes.map((l) => l.name), assignee: node.assignee?.displayName ?? node.assignee?.name, priority: node.priority, - ...(node.branchName ? { branchName: node.branchName } : {}), + branchName: node.branchName ?? undefined, }; // Assign after creation (Linear's issueCreate uses assigneeId, not display name) diff --git a/scripts/claude-batch-spawn b/scripts/claude-batch-spawn index efd4308a0..e097081f1 100755 --- a/scripts/claude-batch-spawn +++ b/scripts/claude-batch-spawn @@ -136,9 +136,9 @@ for ISSUE in "${ISSUES[@]}"; do # Build initial prompt if [ "$SESSION_PREFIX" = "splitly" ]; then - INITIAL_PROMPT="Please start working on GitHub issue #$ISSUE_NUM. First, fetch the issue details using: gh issue view $ISSUE_NUM --repo $GITHUB_REPO. Then implement the solution in the branch already prepared for this session." + INITIAL_PROMPT="Please start working on GitHub issue #$ISSUE_NUM. First, fetch the issue details using: gh issue view $ISSUE_NUM --repo $GITHUB_REPO. Then implement the solution in the branch already prepared for this session (confirm with \`git branch --show-current\` in the worktree if needed)." else - INITIAL_PROMPT="Please start working on $ISSUE, fetch ticket info, and implement the task in the branch already prepared for this session." + INITIAL_PROMPT="Please start working on $ISSUE, fetch ticket info, and implement the task in the branch already prepared for this session (confirm with \`git branch --show-current\` in the worktree if needed)." fi # Send prompt to the running Claude session