Address PR review feedback (#1747)

- core/store: include workspaceWorktrees in the slim and activity-log-limited
  SELECT lists (rowToTask reads it, but the explicit column lists omitted it, so
  slim/limited reads dropped the field and could misclassify workspace tasks);
  add regression tests for both read surfaces
- dashboard/register-git-github: validate caller-supplied repoPath in resolveGitDir
  via isPathWithin containment check (path-traversal hardening for all git
  endpoints); make loadWorkspaceConfig a static @fusion/core import per AGENTS.md
- dashboard/legacy: preserve repoPath in the string-form pullBranch overload
- dashboard/GitManagerModal: revalidate selectedRepo against the fetched repo list
  so a stale selection can't persist across project switches

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-06-24 15:43:12 -07:00
parent d9efea9e27
commit 28ceca2cbd
5 changed files with 67 additions and 15 deletions

View File

@@ -102,6 +102,25 @@ describe("TaskStore", () => {
expect(detail.workspaceWorktrees).toEqual(sampleMap);
});
// Surface enumeration (PR #1747 review): rowToTask reads row.workspaceWorktrees, but the
// explicit slim and activity-log-limited SELECT lists are separate from `*` — if the column is
// omitted there, slim/limited reads silently drop the field even though getTask("*") works.
it("survives the activity-log-limited read (explicit limited SELECT clause)", async () => {
const task = await harness.store().createTask({ description: "Workspace task limited read" });
await harness.store().updateTask(task.id, { workspaceWorktrees: sampleMap });
const detail = await harness.store().getTask(task.id, { activityLogLimit: 1 });
expect(detail.workspaceWorktrees).toEqual(sampleMap);
});
it("survives the slim search read (explicit slim SELECT clause)", async () => {
const task = await harness.store().createTask({ description: "Workspace slimsearchmarker task" });
await harness.store().updateTask(task.id, { workspaceWorktrees: sampleMap });
const found = (await harness.store().searchTasks("slimsearchmarker", { slim: true })).find((t) => t.id === task.id);
expect(found?.workspaceWorktrees).toEqual(sampleMap);
});
it("normalizes an empty map to undefined so isWorkspaceTask stays false", async () => {
const task = await harness.store().createTask({ description: "Empty workspace map" });
const updated = await harness.store().updateTask(task.id, { workspaceWorktrees: {} });

View File

@@ -2607,7 +2607,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
"tokenUsageInputTokens", "tokenUsageOutputTokens", "tokenUsageCachedTokens", "tokenUsageCacheWriteTokens", "tokenUsageTotalTokens", "tokenUsageFirstUsedAt", "tokenUsageLastUsedAt", "tokenUsageModelProvider", "tokenUsageModelId", "tokenUsagePerModel", "tokenBudgetSoftAlertedAt", "tokenBudgetHardAlertedAt", "tokenBudgetOverride",
"createdAt", "updatedAt", "columnMovedAt", "firstExecutionAt", "cumulativeActiveMs", "executionStartedAt", "executionCompletedAt",
"dependencies", "steps", "customFields", "comments", "review", "reviewState", "workflowStepResults", "steeringComments",
"attachments", "prInfo", "prInfos", "issueInfo", "githubTracking", "sourceIssueProvider", "sourceIssueRepository", "sourceIssueExternalIssueId", "sourceIssueNumber", "sourceIssueUrl", "sourceIssueClosedAt", "mergeDetails",
"attachments", "prInfo", "prInfos", "issueInfo", "githubTracking", "sourceIssueProvider", "sourceIssueRepository", "sourceIssueExternalIssueId", "sourceIssueNumber", "sourceIssueUrl", "sourceIssueClosedAt", "mergeDetails", "workspaceWorktrees",
"breakIntoSubtasks", "noCommitsExpected", "enabledWorkflowSteps", "modifiedFiles",
"missionId", "sliceId", "scopeOverride", "scopeOverrideReason", "scopeAutoWiden", "assignedAgentId", "pausedByAgentId", "assigneeUserId", "nodeId", "effectiveNodeId", "effectiveNodeSource",
"sourceType", "sourceAgentId", "sourceRunId", "sourceSessionId", "sourceMessageId", "sourceParentTaskId", "sourceMetadata",
@@ -2656,7 +2656,7 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
"tokenUsageInputTokens", "tokenUsageOutputTokens", "tokenUsageCachedTokens", "tokenUsageCacheWriteTokens", "tokenUsageTotalTokens", "tokenUsageFirstUsedAt", "tokenUsageLastUsedAt", "tokenUsageModelProvider", "tokenUsageModelId", "tokenUsagePerModel", "tokenBudgetSoftAlertedAt", "tokenBudgetHardAlertedAt", "tokenBudgetOverride",
"createdAt", "updatedAt", "columnMovedAt", "firstExecutionAt", "cumulativeActiveMs", "executionStartedAt", "executionCompletedAt",
"dependencies", "steps", "customFields", "attachments", "steeringComments",
"comments", "review", "reviewState", "workflowStepResults", "prInfo", "prInfos", "issueInfo", "githubTracking", "sourceIssueProvider", "sourceIssueRepository", "sourceIssueExternalIssueId", "sourceIssueNumber", "sourceIssueUrl", "sourceIssueClosedAt", "mergeDetails",
"comments", "review", "reviewState", "workflowStepResults", "prInfo", "prInfos", "issueInfo", "githubTracking", "sourceIssueProvider", "sourceIssueRepository", "sourceIssueExternalIssueId", "sourceIssueNumber", "sourceIssueUrl", "sourceIssueClosedAt", "mergeDetails", "workspaceWorktrees",
"breakIntoSubtasks", "noCommitsExpected", "enabledWorkflowSteps", "modifiedFiles",
"missionId", "sliceId", "scopeOverride", "scopeOverrideReason", "scopeAutoWiden", "assignedAgentId", "pausedByAgentId", "assigneeUserId", "nodeId", "effectiveNodeId", "effectiveNodeSource",
"sourceType", "sourceAgentId", "sourceRunId", "sourceSessionId", "sourceMessageId", "sourceParentTaskId", "sourceMetadata",

View File

@@ -3128,10 +3128,16 @@ export function pullBranch(
projectId?: string,
repoPath?: string,
): Promise<GitPullResult> {
const options = typeof optionsOrProjectId === "string" ? undefined : optionsOrProjectId;
const resolvedProjectId = typeof optionsOrProjectId === "string" ? optionsOrProjectId : projectId;
// FNXC:DashboardGitApi 2026-06-24-00:00:
// pullBranch has two overloads. In the string-arg style pullBranch(projectId, repoPath),
// the second positional carries repoPath (not the 3rd parameter), so resolve it from `projectId`
// to avoid dropping repoPath; otherwise multi-repo workspace pulls hit the wrong repo.
const isStringForm = typeof optionsOrProjectId === "string";
const options = isStringForm ? undefined : optionsOrProjectId;
const resolvedProjectId = isStringForm ? optionsOrProjectId : projectId;
const resolvedRepoPath = isStringForm ? projectId : repoPath;
return api<GitPullResult>(withRepoPath(withProjectId("/git/pull", resolvedProjectId), repoPath), {
return api<GitPullResult>(withRepoPath(withProjectId("/git/pull", resolvedProjectId), resolvedRepoPath), {
method: "POST",
body: JSON.stringify({ rebase: options?.rebase ?? false }),
});

View File

@@ -898,16 +898,30 @@ export function GitManagerModal({ isOpen, onClose, tasks: _tasks, addToast, proj
}, [projectId]);
// Fetch workspace repos on mount to determine if this is a multi-repo project.
/*
FNXC:Workspace 2026-06-24-21:30:
Revalidate selectedRepo against the freshly fetched repo list. When projectId
changes (or a project has no workspace repos), a stale selection from the prior
project would otherwise persist and keep sending a stale repoPath to git
endpoints. Keep the current selection only if it still exists in the new list;
otherwise fall back to repos[0], or clear to null when the list is empty (and on
fetch error). The functional updater lets us revalidate without depending on
selectedRepo in the effect deps, preserving the projectId-keyed intent.
*/
useEffect(() => {
fetchWorkspaceRepos(projectId)
.then((result) => {
setWorkspaceRepos(result.repos);
if (result.repos.length > 0 && !selectedRepo) {
setSelectedRepo(result.repos[0]!);
}
const repos = result.repos;
setWorkspaceRepos(repos);
setSelectedRepo((current) =>
current && repos.includes(current) ? current : (repos[0] ?? null),
);
})
.catch(() => setWorkspaceRepos([]));
}, [projectId]); // intentionally omit selectedRepo to avoid resetting on repo switch
.catch(() => {
setWorkspaceRepos([]);
setSelectedRepo(null);
});
}, [projectId]); // keyed on projectId; selectedRepo is revalidated via the functional updater
const handleSyncIntegrationTip = useCallback(async () => {
if (!status?.integrationBranch || status.isOnIntegrationBranch === false) return;

View File

@@ -1,5 +1,5 @@
import { type NextFunction, type Request, type Response } from "express";
import { isAbsolute, resolve, relative, join } from "node:path";
import { isAbsolute, resolve, relative } from "node:path";
import { realpathSync } from "node:fs";
import { exec as execCb, spawn } from "node:child_process";
import { promisify } from "node:util";
@@ -17,7 +17,7 @@ import type {
Task,
TaskStore,
} from "@fusion/core";
import { classifyGhError, getCurrentRepo, isGhAuthenticated } from "@fusion/core";
import { classifyGhError, getCurrentRepo, isGhAuthenticated, loadWorkspaceConfig } from "@fusion/core";
import {
dropAutostashHandle,
generateSyntheticRunId,
@@ -2474,11 +2474,25 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void {
In workspace mode (multi-repo), git operations target a specific sub-repo.
The `repoPath` query param selects which sub-repo. When absent, the project
root directory is used (existing single-repo behavior).
FNXC:Workspace 2026-06-24-22:30:
`repoPath` is caller-supplied and untrusted. It must resolve to a directory
contained within the project root; a `../`-prefixed or absolute value would
otherwise redirect every git endpoint (read remote URLs, commit/push/discard)
at an arbitrary repo on disk. Resolve to an absolute path and reject anything
that escapes `projectRoot` via the shared `isPathWithin` containment check
(the empty / `.` / exact-root case stays allowed — that is the root itself).
*/
function resolveGitDir(req: Request, projectRoot: string): string {
const repoPath = req.query.repoPath;
if (typeof repoPath === "string" && repoPath.trim()) {
return join(projectRoot, repoPath.trim());
const resolved = resolve(projectRoot, repoPath.trim());
if (!isPathWithin(projectRoot, resolved)) {
throw new ApiError(400, "Invalid repoPath: resolves outside the project root", {
reason: "repo-path-escape",
});
}
return resolved;
}
return projectRoot;
}
@@ -2492,7 +2506,6 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void {
try {
const { store: scopedStore } = await getProjectContext(req);
const rootDir = resolveGitDir(req, scopedStore.getRootDir());
const { loadWorkspaceConfig } = await import("@fusion/core");
const config = await loadWorkspaceConfig(rootDir);
res.json({ repos: config?.repos ?? [] });
} catch (err: unknown) {