Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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