mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-09 06:15:21 +02:00
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - Repo-only project workspaces are materialized by a server-side
managed `git clone` into a per-project directory (#10720 added
credentials for private repos)
> - Two issues on the same project routinely wake seconds apart, and
both runs race the same clone target
> - The loser fails with "destination path already exists", and its
failure cleanup removes the directory out from under the winner's
in-progress clone — both runs then fail every round
> - This pull request serializes materialization per target directory
and makes the clone land atomically via a temp sibling + rename, so the
shared target is never partial and never removed
> - The benefit is that concurrent runs on the same project converge:
one clone happens, everyone adopts it, and unrelated failures no longer
blame the GitHub credential
## Linked Issues or Issue Description
**What happened?**
With isolated workspaces enabled on a project whose only workspace is
repo-only, unblocking two issues at once produced lockstep mutual
destruction (observed live, two consecutive rounds): both runs called
the managed-checkout materialization concurrently; one clone created the
target directory, the other's `git clone` failed with `fatal:
destination path '…' already exists and is not an empty directory`, and
that run's failure cleanup deleted the directory while the first clone
was still writing into it (`fatal: could not set 'core…'`). Both runs
failed `workspace_validation_failed`; their staggered retries could race
again. The failure message also wrongly claimed the GitHub credential
"was rejected or lacks access" — the collision had nothing to do with
auth.
**Expected behavior**
Concurrent materializations of the same project checkout share one
clone; a completed checkout is never removed by a failing sibling; the
credential is only blamed for auth-shaped failures.
**Steps to reproduce**
1. Project with a repo-only workspace (private repo, isolated workspaces
on).
2. Move two issues on that project to `todo` at the same time so both
runs start within seconds.
3. Both runs fail workspace validation with "destination path already
exists" / "could not set 'core…'" instead of one clone succeeding.
**Paperclip version or commit**
`master` (75f6256b76).
## What Changed
- `ensureManagedProjectWorkspace` serializes in-flight materializations
per target cwd (a module-level promise map): concurrent callers share
one attempt.
- The clone lands in a `<target>.clone-XXXXXX` temp sibling created with
`mkdtemp`, then moves into place with an atomic `rename`. Clone failure
removes only the temp directory; the shared target is never created
partially and never deleted. If the target appears between the emptiness
check and the rename (another process won), the completed checkout is
adopted instead of failing the run.
- `describeGitAuthFailure` attributes the GitHub credential only when
the error matches the auth-failure pattern; unrelated failures (path
collisions, network errors) no longer claim the token was rejected.
## Verification
- `cd server && npx vitest run
src/__tests__/heartbeat-managed-clone-credentials.test.ts
src/__tests__/git-credentials.test.ts
src/__tests__/heartbeat-workspace-session.test.ts` — 169 tests pass,
including new cases: concurrent materializations of the same checkout
succeed with one shared result and no temp litter; failed clones leave
neither target nor temp directories; an authenticated clone failing for
non-auth reasons does not blame the credential.
- `pnpm --filter @paperclipai/server typecheck` — clean.
## Risks
- Low risk. The serialization is in-process and keyed by exact target
path; the temp+rename pattern stays on the same filesystem (sibling
path) so the rename is atomic. Single-run behavior is byte-identical
apart from the temp-dir intermediate.
- The rename-conflict adoption path accepts a checkout another
materialization completed; the pre-existing `gitDirExists` adoption
semantics are unchanged.
## Model Used
Claude Fable 5 (`claude-fable-5`, extended thinking, agentic tool use
via Claude Code CLI).
## 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 searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b) described the issue in-PR following the relevant
issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [x] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes
- [x] I have considered and documented any risks above
- [ ] All Paperclip CI gates are green
- [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
- [x] I will address all Greptile and reviewer comments before
requesting merge
295 lines
12 KiB
TypeScript
295 lines
12 KiB
TypeScript
import { spawn } from "node:child_process";
|
|
import fs from "node:fs/promises";
|
|
import os from "node:os";
|
|
import path from "node:path";
|
|
import { describe, expect, it, vi } from "vitest";
|
|
import type { Db } from "@paperclipai/db";
|
|
import {
|
|
DEFAULT_GITHUB_TOKEN_SECRET_NAMES,
|
|
GIT_CREDENTIAL_TOKEN_ENV_KEY,
|
|
buildGitAuthInvocation,
|
|
createGitRemoteAuthProvider,
|
|
describeGitAuthFailure,
|
|
isGitHubHttpsRemoteUrl,
|
|
scrubGitCredentialText,
|
|
} from "../services/git-credentials.ts";
|
|
|
|
const fakeDb = null as unknown as Db;
|
|
|
|
function buildSecretsFake(byName: Record<string, string | Error>) {
|
|
const getByName = vi.fn(async (_companyId: string, name: string) => {
|
|
if (!(name in byName)) return null;
|
|
return { id: `secret-${name}` };
|
|
});
|
|
const resolveSecretValue = vi.fn(async (_companyId: string, secretId: string) => {
|
|
const name = secretId.replace(/^secret-/, "");
|
|
const value = byName[name];
|
|
if (value instanceof Error) throw value;
|
|
return value ?? "";
|
|
});
|
|
return { getByName, resolveSecretValue };
|
|
}
|
|
|
|
describe("isGitHubHttpsRemoteUrl", () => {
|
|
it("accepts https github.com and www.github.com URLs", () => {
|
|
expect(isGitHubHttpsRemoteUrl("https://github.com/example/repo.git")).toBe(true);
|
|
expect(isGitHubHttpsRemoteUrl("https://www.github.com/example/repo.git")).toBe(true);
|
|
});
|
|
|
|
it("rejects ssh, http, enterprise hosts, other providers, userinfo URLs, and non-URLs", () => {
|
|
expect(isGitHubHttpsRemoteUrl("git@github.com:example/repo.git")).toBe(false);
|
|
expect(isGitHubHttpsRemoteUrl("ssh://git@github.com/example/repo.git")).toBe(false);
|
|
expect(isGitHubHttpsRemoteUrl("http://github.com/example/repo.git")).toBe(false);
|
|
expect(isGitHubHttpsRemoteUrl("https://github.enterprise.example/org/repo.git")).toBe(false);
|
|
expect(isGitHubHttpsRemoteUrl("https://gitlab.com/example/repo.git")).toBe(false);
|
|
expect(isGitHubHttpsRemoteUrl("https://alice:token@github.com/example/repo.git")).toBe(false);
|
|
expect(isGitHubHttpsRemoteUrl("/local/path/repo.git")).toBe(false);
|
|
});
|
|
});
|
|
|
|
describe("createGitRemoteAuthProvider", () => {
|
|
const githubUrl = "https://github.com/example/repo.git";
|
|
|
|
it("prefers company secrets in declared order", async () => {
|
|
const secrets = buildSecretsFake({ GH_TOKEN: "gh-token", PAPERCLIP_GITHUB_TOKEN: "pc-token" });
|
|
const provider = createGitRemoteAuthProvider(fakeDb, "company-1", undefined, {
|
|
secrets,
|
|
env: { GITHUB_TOKEN: "env-token" },
|
|
});
|
|
const invocation = await provider(githubUrl);
|
|
expect(invocation?.env[GIT_CREDENTIAL_TOKEN_ENV_KEY]).toBe("gh-token");
|
|
expect(invocation?.source).toBe("company_secret");
|
|
expect(invocation?.secretName).toBe("GH_TOKEN");
|
|
// GITHUB_TOKEN is probed first even though only GH_TOKEN exists.
|
|
expect(secrets.getByName.mock.calls.map((call) => call[1])).toEqual(["GITHUB_TOKEN", "GH_TOKEN"]);
|
|
});
|
|
|
|
it("falls back to the server env, GITHUB_TOKEN before GH_TOKEN", async () => {
|
|
const provider = createGitRemoteAuthProvider(fakeDb, "company-1", undefined, {
|
|
secrets: buildSecretsFake({}),
|
|
env: { GITHUB_TOKEN: "env-github", GH_TOKEN: "env-gh" },
|
|
});
|
|
const invocation = await provider(githubUrl);
|
|
expect(invocation?.env[GIT_CREDENTIAL_TOKEN_ENV_KEY]).toBe("env-github");
|
|
expect(invocation?.source).toBe("server_env");
|
|
expect(invocation?.secretName).toBeNull();
|
|
});
|
|
|
|
it("returns null when no token is available anywhere", async () => {
|
|
const provider = createGitRemoteAuthProvider(fakeDb, "company-1", undefined, {
|
|
secrets: buildSecretsFake({}),
|
|
env: {},
|
|
});
|
|
await expect(provider(githubUrl)).resolves.toBeNull();
|
|
});
|
|
|
|
it("returns null for out-of-scope URLs without touching the secret store", async () => {
|
|
const secrets = buildSecretsFake({ GITHUB_TOKEN: "token" });
|
|
const provider = createGitRemoteAuthProvider(fakeDb, "company-1", undefined, {
|
|
secrets,
|
|
env: {},
|
|
});
|
|
await expect(provider("git@github.com:example/repo.git")).resolves.toBeNull();
|
|
await expect(provider("https://gitlab.com/example/repo.git")).resolves.toBeNull();
|
|
expect(secrets.getByName).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("memoizes the credential lookup across calls", async () => {
|
|
const secrets = buildSecretsFake({ GITHUB_TOKEN: "token" });
|
|
const provider = createGitRemoteAuthProvider(fakeDb, "company-1", undefined, {
|
|
secrets,
|
|
env: {},
|
|
});
|
|
await provider(githubUrl);
|
|
await provider(githubUrl);
|
|
await provider("https://github.com/example/another.git");
|
|
expect(secrets.getByName).toHaveBeenCalledTimes(1);
|
|
expect(secrets.resolveSecretValue).toHaveBeenCalledTimes(1);
|
|
});
|
|
|
|
it("passes a system access context so resolution is audited", async () => {
|
|
const secrets = buildSecretsFake({ GITHUB_TOKEN: "token" });
|
|
const provider = createGitRemoteAuthProvider(
|
|
fakeDb,
|
|
"company-1",
|
|
{ issueId: "issue-1", heartbeatRunId: "run-1" },
|
|
{ secrets, env: {} },
|
|
);
|
|
await provider(githubUrl);
|
|
expect(secrets.resolveSecretValue).toHaveBeenCalledWith("company-1", "secret-GITHUB_TOKEN", "latest", {
|
|
accessContext: expect.objectContaining({
|
|
consumerType: "system",
|
|
consumerId: "workspace-git-credential",
|
|
actorType: "system",
|
|
issueId: "issue-1",
|
|
heartbeatRunId: "run-1",
|
|
}),
|
|
});
|
|
});
|
|
|
|
it("continues down the chain when one secret fails to resolve", async () => {
|
|
const secrets = buildSecretsFake({
|
|
GITHUB_TOKEN: new Error("provider outage"),
|
|
GH_TOKEN: "gh-token",
|
|
});
|
|
const provider = createGitRemoteAuthProvider(fakeDb, "company-1", undefined, {
|
|
secrets,
|
|
env: {},
|
|
});
|
|
const invocation = await provider(githubUrl);
|
|
expect(invocation?.secretName).toBe("GH_TOKEN");
|
|
});
|
|
});
|
|
|
|
describe("buildGitAuthInvocation", () => {
|
|
it("keeps the token out of argv and installs the helper URL-scoped to github.com", () => {
|
|
const invocation = buildGitAuthInvocation({
|
|
token: "super-secret-token",
|
|
source: "company_secret",
|
|
secretName: "GITHUB_TOKEN",
|
|
});
|
|
expect(invocation.configArgs.join(" ")).not.toContain("super-secret-token");
|
|
expect(invocation.configArgs[0]).toBe("-c");
|
|
expect(invocation.configArgs[1]).toBe("credential.helper=");
|
|
expect(invocation.configArgs[3]).toContain("credential.https://github.com.helper=");
|
|
expect(invocation.configArgs[3]).toContain("x-access-token");
|
|
expect(invocation.configArgs[5]).toContain("credential.https://www.github.com.helper=");
|
|
expect(invocation.env[GIT_CREDENTIAL_TOKEN_ENV_KEY]).toBe("super-secret-token");
|
|
expect(invocation.env.GIT_TERMINAL_PROMPT).toBe("0");
|
|
});
|
|
});
|
|
|
|
describe("credential helper execution (real git, no network)", () => {
|
|
async function runCredentialFill(description: string) {
|
|
const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-git-cred-fill-"));
|
|
try {
|
|
const invocation = buildGitAuthInvocation({
|
|
token: "abc123",
|
|
source: "company_secret",
|
|
secretName: "GITHUB_TOKEN",
|
|
});
|
|
return await new Promise<{ code: number | null; stdout: string; stderr: string }>(
|
|
(resolve, reject) => {
|
|
const child = spawn("git", [...invocation.configArgs, "credential", "fill"], {
|
|
cwd,
|
|
env: { ...process.env, ...invocation.env },
|
|
stdio: ["pipe", "pipe", "pipe"],
|
|
});
|
|
let stdout = "";
|
|
let stderr = "";
|
|
child.stdout.on("data", (chunk) => { stdout += String(chunk); });
|
|
child.stderr.on("data", (chunk) => { stderr += String(chunk); });
|
|
child.on("error", reject);
|
|
child.on("close", (code) => resolve({ code, stdout, stderr }));
|
|
child.stdin.write(description);
|
|
child.stdin.end();
|
|
},
|
|
);
|
|
} finally {
|
|
await fs.rm(cwd, { recursive: true, force: true });
|
|
}
|
|
}
|
|
|
|
it("answers a github.com https request with the env-carried token", async () => {
|
|
const result = await runCredentialFill("protocol=https\nhost=github.com\n\n");
|
|
expect(result.code).toBe(0);
|
|
expect(result.stdout).toContain("username=x-access-token");
|
|
expect(result.stdout).toContain("password=abc123");
|
|
});
|
|
|
|
it("never hands the token to another host, even if git asks", async () => {
|
|
// Simulates a request whose effective host changed after our pre-invocation URL check
|
|
// (for example a repository-local url.<base>.insteadOf rewrite): the URL-scoped helper
|
|
// config keeps git from consulting the helper, prompts are disabled, so the fill fails
|
|
// and the token is never emitted.
|
|
const result = await runCredentialFill("protocol=https\nhost=evil.example\n\n");
|
|
expect(result.code).not.toBe(0);
|
|
expect(result.stdout).not.toContain("abc123");
|
|
});
|
|
|
|
it("never answers plain-http requests for github.com", async () => {
|
|
const result = await runCredentialFill("protocol=http\nhost=github.com\n\n");
|
|
expect(result.code).not.toBe(0);
|
|
expect(result.stdout).not.toContain("abc123");
|
|
});
|
|
});
|
|
|
|
describe("scrubGitCredentialText", () => {
|
|
it("masks URL userinfo", () => {
|
|
expect(scrubGitCredentialText("https://x-access-token:ghp_secret@github.com/a/b.git")).toBe(
|
|
"https://***@github.com/a/b.git",
|
|
);
|
|
});
|
|
|
|
it("masks userinfo on non-HTTP schemes, leaving scp-style remotes alone", () => {
|
|
expect(scrubGitCredentialText("ssh://deploy:hunter2@internal.example/repo.git")).toBe(
|
|
"ssh://***@internal.example/repo.git",
|
|
);
|
|
expect(scrubGitCredentialText("git@github.com:example/repo.git")).toBe(
|
|
"git@github.com:example/repo.git",
|
|
);
|
|
});
|
|
|
|
it("masks entire URL query strings regardless of parameter names", () => {
|
|
expect(scrubGitCredentialText("https://github.com/a/b.git?access_token=ghs_secret&ref=main")).toBe(
|
|
"https://github.com/a/b.git?***",
|
|
);
|
|
expect(scrubGitCredentialText("https://host.example/r.git?obscure_cred_name=secret")).toBe(
|
|
"https://host.example/r.git?***",
|
|
);
|
|
});
|
|
|
|
it("leaves credential-free text unchanged", () => {
|
|
expect(scrubGitCredentialText("fatal: repository not found")).toBe("fatal: repository not found");
|
|
});
|
|
});
|
|
|
|
describe("describeGitAuthFailure", () => {
|
|
it("names the company secret when a stored credential was used", () => {
|
|
expect(describeGitAuthFailure({
|
|
error: "fatal: Authentication failed",
|
|
used: { source: "company_secret", secretName: "GH_TOKEN" },
|
|
})).toContain("the GH_TOKEN company-secret GitHub credential");
|
|
});
|
|
|
|
it("names the server environment when an env credential was used", () => {
|
|
expect(describeGitAuthFailure({
|
|
error: "fatal: Authentication failed",
|
|
used: { source: "server_env", secretName: null },
|
|
})).toContain("server-environment GitHub credential");
|
|
});
|
|
|
|
it("points at Settings → Secrets for auth-looking failures without a credential", () => {
|
|
expect(describeGitAuthFailure({
|
|
error: "fatal: could not read Username for 'https://github.com': terminal prompts disabled",
|
|
used: null,
|
|
})).toContain("add a GITHUB_TOKEN or GH_TOKEN company secret");
|
|
});
|
|
|
|
it("stays silent for non-auth failures without a credential", () => {
|
|
expect(describeGitAuthFailure({
|
|
error: "fatal: unable to resolve host example.invalid",
|
|
used: null,
|
|
})).toBeNull();
|
|
});
|
|
|
|
it("stays silent for non-auth failures even when a credential was used", () => {
|
|
// A credential present during an unrelated failure (network outage, target-path
|
|
// collision) must not be blamed for it.
|
|
expect(describeGitAuthFailure({
|
|
error: "fatal: destination path '/x/y' already exists and is not an empty directory.",
|
|
used: { source: "company_secret", secretName: "GH_TOKEN" },
|
|
})).toBeNull();
|
|
});
|
|
});
|
|
|
|
describe("DEFAULT_GITHUB_TOKEN_SECRET_NAMES", () => {
|
|
it("keeps the shared name order stable", () => {
|
|
expect([...DEFAULT_GITHUB_TOKEN_SECRET_NAMES]).toEqual([
|
|
"GITHUB_TOKEN",
|
|
"GH_TOKEN",
|
|
"PAPERCLIP_GITHUB_TOKEN",
|
|
]);
|
|
});
|
|
});
|