Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .dockerignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
!.devcontainer
!.kearc
!bin
!common/alerting
!common/hogvm
!common/esbuilder
!common/migration_utils
Expand Down
4 changes: 3 additions & 1 deletion products/desktop/packages/agent/src/server/agent-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1575,7 +1575,9 @@ export class AgentServer {
return null;
}),
]);
this.taskRepositories = preTask?.repository ? [preTask.repository] : [];
this.taskRepositories =
preTask?.repositories ??
(preTask?.repository ? [preTask.repository] : []);

this.prewarmedRun =
(preTaskRun?.state as Record<string, unknown> | undefined)?.prewarmed ===
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,25 @@ describe("task response normalization", () => {
},
});
});

// Multi-repo handoff and cloud-run instructions read task.repositories, so
// dropping this fallback silently degrades every consumer to single-repo.
it.each([
[
"keeps the API's repositories list",
{ repository: "posthog/posthog", repositories: ["a/b", "c/d"] },
["a/b", "c/d"],
],
[
"wraps a lone repository",
{ repository: "posthog/posthog" },
["posthog/posthog"],
],
["defaults to empty", {}, []],
])("populates repositories (%s)", (_label, dto, expected) => {
expect(
normalizeTaskResponse({ id: "task-1", ...dto }, { teamId: 1 })
.repositories,
).toEqual(expected);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ export function normalizeTaskResponse(
...(dto.created_by === undefined ? {} : { created_by: dto.created_by }),
origin_product: dto.origin_product ?? "",
...(dto.repository === undefined ? {} : { repository: dto.repository }),
repositories: dto.repositories ?? (dto.repository ? [dto.repository] : []),
...(dto.github_integration === undefined
? {}
: { github_integration: dto.github_integration }),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,43 @@ describe("LocalHandoffService.start", () => {
);
});

// A task's repository is team-writable via the API, so an unsafe entry must
it("reuses local repositories and clones missing ones before handoff", async () => {
const deps = makeDeps();
const multiRepoTask = {
repositories: ["posthog/posthog", "posthog/posthog-js"],
} as Task;
deps.host.getRepositoryByRemoteUrl = vi
.fn()
.mockResolvedValueOnce({ path: "/repos/posthog" })
.mockResolvedValueOnce(null);
deps.sessionService.preflightToLocal.mockResolvedValue({
canHandoff: true,
});

await deps.service.start("task-1", multiRepoTask);

expect(deps.host.cloneRepository).toHaveBeenCalledWith(
expect.objectContaining({
repoUrl: "https://github.com/posthog/posthog-js.git",
targetPath: "/worktrees/linked-repositories/posthog/posthog-js",
}),
);
expect(deps.host.addAdditionalDirectory).toHaveBeenCalledWith({
taskId: "task-1",
path: "/worktrees/linked-repositories/posthog/posthog-js",
});
expect(deps.sessionService.handoffToLocal).toHaveBeenCalledWith(
"task-1",
"/repos/posthog",
{
"posthog/posthog": "/repos/posthog",
"posthog/posthog-js":
"/worktrees/linked-repositories/posthog/posthog-js",
},
);
});

// A task's repositories are team-writable via the API, so an unsafe entry must
// never reach `git clone` (RCE via git's remote-ext transport) or escape the
// clone root through path traversal. The safe repo alongside it still clones.
it.each([
Expand All @@ -210,12 +246,44 @@ describe("LocalHandoffService.start", () => {
canHandoff: true,
});

await deps.service.start("task-1", { repository: malicious } as Task);
await deps.service.start("task-1", {
repositories: ["posthog/posthog", malicious],
} as Task);

expect(deps.host.cloneRepository).not.toHaveBeenCalled();
// Only the safe repo is cloned, and always through an explicit https URL.
expect(deps.host.cloneRepository).toHaveBeenCalledTimes(1);
expect(deps.host.cloneRepository).toHaveBeenCalledWith(
expect.objectContaining({
repoUrl: "https://github.com/posthog/posthog.git",
}),
);
expect(deps.notifier.warn).toHaveBeenCalledWith(
expect.stringContaining(malicious),
);
expect(deps.sessionService.handoffToLocal).not.toHaveBeenCalled();
const [, , repositoryPaths] =
deps.sessionService.handoffToLocal.mock.calls[0];
expect(repositoryPaths).toEqual({
"posthog/posthog": "/worktrees/linked-repositories/posthog/posthog",
});
});

it("de-duplicates case-variant repository aliases before cloning", async () => {
const deps = makeDeps();
deps.host.getRepositoryByRemoteUrl = vi.fn().mockResolvedValue(null);
deps.sessionService.preflightToLocal.mockResolvedValue({
canHandoff: true,
});

await deps.service.start("task-1", {
repositories: ["PostHog/PostHog", "posthog/posthog"],
} as Task);

// Both collapse to one target, so the racing double-clone can't happen.
expect(deps.host.cloneRepository).toHaveBeenCalledTimes(1);
const [, , repositoryPaths] =
deps.sessionService.handoffToLocal.mock.calls[0];
expect(repositoryPaths).toEqual({
"posthog/posthog": "/worktrees/linked-repositories/posthog/posthog",
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,11 @@ export class LocalHandoffService {

public async start(taskId: string, task: Task): Promise<void> {
try {
const repositories = task.repository ? [task.repository] : [];
const repositories = task.repositories?.length
? task.repositories
: task.repository
? [task.repository]
: [];
Comment on lines +135 to +139

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic error: checking task.repositories?.length will treat an explicit empty array [] as falsy and incorrectly fall back to task.repository. If a task is normalized with no repositories (repositories: []), but has a legacy repository field, this will incorrectly use the single repository instead of respecting the empty array.

Fix:

const repositories = task.repositories !== undefined
  ? task.repositories
  : task.repository
    ? [task.repository]
    : [];

This ensures an explicit empty array is respected rather than falling through to the legacy field.

Suggested change
const repositories = task.repositories?.length
? task.repositories
: task.repository
? [task.repository]
: [];
const repositories = task.repositories !== undefined
? task.repositories
: task.repository
? [task.repository]
: [];

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping this as is: it restores the exact semantics of #76448, which #76649 clobbered. An empty repositories list means the multi-repo field wasn't populated, so falling back to the legacy single repository field keeps handoff working for such tasks. normalizeTaskResponse also wraps repository into repositories now, so the empty-plus-legacy combination only arises for Task objects built outside the normalizer, where the fallback is the safe choice.

const repositoryPaths = await this.resolveRepositoryPaths(repositories);
const paths = Object.values(repositoryPaths);
const targetPath =
Expand Down
1 change: 1 addition & 0 deletions products/desktop/packages/shared/src/domain-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export interface Task {
created_by?: UserBasic | null;
origin_product: string;
repository?: string | null; // Format: "organization/repository" (e.g., "posthog/posthog-js")
repositories?: string[];
github_integration?: number | null;
github_user_integration?: string | null;
json_schema?: Record<string, unknown> | null;
Expand Down
Loading
Loading