fix: keep file tree paths workspace-relative (#3855)
Co-authored-by: Kandev Agent <agent@kandev.dev>
This commit is contained in:
@@ -10,6 +10,7 @@ import (
|
||||
agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl"
|
||||
"github.com/kandev/kandev/internal/agentctl/types/streams"
|
||||
"github.com/kandev/kandev/internal/common/logger"
|
||||
"github.com/kandev/kandev/internal/common/workspacepath"
|
||||
ws "github.com/kandev/kandev/pkg/websocket"
|
||||
"go.uber.org/zap"
|
||||
)
|
||||
@@ -62,6 +63,9 @@ func (h *WorkspaceFileHandlers) wsGetFileTree(ctx context.Context, msg *ws.Messa
|
||||
if req.SessionID == "" {
|
||||
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "session_id is required", nil)
|
||||
}
|
||||
if err := workspacepath.ValidateTreePath(req.Path); err != nil {
|
||||
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, err.Error(), nil)
|
||||
}
|
||||
// Get agent execution for this session
|
||||
execution, err := h.lifecycle.GetOrEnsureExecution(ctx, req.SessionID)
|
||||
if err != nil {
|
||||
|
||||
@@ -19,6 +19,19 @@ import (
|
||||
"go.uber.org/zap/zaptest/observer"
|
||||
)
|
||||
|
||||
type countingWorkspaceExecutionLookup struct {
|
||||
calls int
|
||||
}
|
||||
|
||||
func (l *countingWorkspaceExecutionLookup) GetExecutionBySessionID(string) (*lifecycle.AgentExecution, bool) {
|
||||
return nil, false
|
||||
}
|
||||
|
||||
func (l *countingWorkspaceExecutionLookup) GetOrEnsureExecution(context.Context, string) (*lifecycle.AgentExecution, error) {
|
||||
l.calls++
|
||||
return nil, context.Canceled
|
||||
}
|
||||
|
||||
func TestWorkspaceFileHandlersValidateRequestsBeforeLookup(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
@@ -50,6 +63,35 @@ func TestWorkspaceFileHandlersValidateRequestsBeforeLookup(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestWorkspaceFileHandlersRejectAbsoluteTreePathBeforeLookup(t *testing.T) {
|
||||
core, logs := observer.New(zapcore.DebugLevel)
|
||||
log, err := commonlogger.NewFromZap(zap.New(core))
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
lookup := &countingWorkspaceExecutionLookup{}
|
||||
handler := NewWorkspaceFileHandlers(lookup, log)
|
||||
msg, err := ws.NewRequest("id", ws.ActionWorkspaceFileTreeGet, map[string]any{
|
||||
"session_id": "s",
|
||||
"path": "/home/jcfs/project/public/assets",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
response, err := handler.wsGetFileTree(context.Background(), msg)
|
||||
if err != nil {
|
||||
t.Fatalf("wsGetFileTree returned error: %v", err)
|
||||
}
|
||||
assertWorkspaceContentSearchErrorCode(t, response, ws.ErrorCodeValidation)
|
||||
if lookup.calls != 0 {
|
||||
t.Fatalf("execution lookup calls = %d, want 0", lookup.calls)
|
||||
}
|
||||
if logs.FilterLevelExact(zap.ErrorLevel).Len() != 0 {
|
||||
t.Fatalf("error logs = %v, want none", logs.FilterLevelExact(zap.ErrorLevel).All())
|
||||
}
|
||||
}
|
||||
|
||||
func TestWorkspaceFileHandlersSuccessContracts(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
|
||||
@@ -22,6 +22,7 @@ import (
|
||||
"github.com/kandev/kandev/internal/agentctl/types"
|
||||
"github.com/kandev/kandev/internal/common/readselector"
|
||||
"github.com/kandev/kandev/internal/common/subproc"
|
||||
"github.com/kandev/kandev/internal/common/workspacepath"
|
||||
storageworkspaces "github.com/kandev/kandev/internal/system/storage/workspaces"
|
||||
"go.uber.org/zap"
|
||||
)
|
||||
@@ -126,6 +127,9 @@ func (wt *WorkspaceTracker) getFileListClass(ctx context.Context, class subproc.
|
||||
|
||||
// GetFileTree returns the file tree for a given path and depth
|
||||
func (wt *WorkspaceTracker) GetFileTree(reqPath string, depth int) (*types.FileTreeNode, error) {
|
||||
if err := workspacepath.ValidateTreePath(reqPath); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
// Resolve the full path with path traversal protection
|
||||
safePath := filepath.Join(wt.workDir, filepath.Clean(reqPath))
|
||||
cleanWorkDir := filepath.Clean(wt.workDir)
|
||||
|
||||
@@ -13,6 +13,7 @@ import (
|
||||
|
||||
"github.com/kandev/kandev/internal/agentctl/types"
|
||||
"github.com/kandev/kandev/internal/common/logger"
|
||||
"github.com/kandev/kandev/internal/common/workspacepath"
|
||||
storageworkspaces "github.com/kandev/kandev/internal/system/storage/workspaces"
|
||||
"go.uber.org/zap"
|
||||
)
|
||||
@@ -509,6 +510,45 @@ func TestGetFileTree_HidesOnlyRootOwnershipMarker(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetFileTree_RejectsAbsolutePathBeforeFilesystemAccess(t *testing.T) {
|
||||
workspace := t.TempDir()
|
||||
external := t.TempDir()
|
||||
log, err := logger.NewFromZap(zap.NewNop())
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
_, err = (&WorkspaceTracker{workDir: workspace, logger: log}).GetFileTree(external, 1)
|
||||
if !errors.Is(err, workspacepath.ErrTreePathNotRelative) {
|
||||
t.Fatalf("GetFileTree error = %v, want %v", err, workspacepath.ErrTreePathNotRelative)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetFileTree_AllowsLiteralColonInRelativePath(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("Windows does not permit literal colons in path components")
|
||||
}
|
||||
workspace := t.TempDir()
|
||||
directory := filepath.Join(workspace, "config:dev")
|
||||
if err := os.Mkdir(directory, 0o755); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := os.WriteFile(filepath.Join(directory, "settings.json"), []byte("{}"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
tree, err := (&WorkspaceTracker{workDir: workspace}).GetFileTree("config:dev", 1)
|
||||
if err != nil {
|
||||
t.Fatalf("GetFileTree failed: %v", err)
|
||||
}
|
||||
if tree.Path != "config:dev" {
|
||||
t.Fatalf("GetFileTree root path = %q, want %q", tree.Path, "config:dev")
|
||||
}
|
||||
if findChild(tree, "settings.json") == nil {
|
||||
t.Fatal("file beneath colon-containing directory should be visible")
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetFileList_HidesOnlyRootOwnershipMarker(t *testing.T) {
|
||||
taskRoot := createOwnershipMarkerFixture(t)
|
||||
initGitRepoAt(t, taskRoot)
|
||||
|
||||
@@ -0,0 +1,61 @@
|
||||
// Package workspacepath validates path identities used by workspace services.
|
||||
package workspacepath
|
||||
|
||||
import (
|
||||
"errors"
|
||||
pathpkg "path"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
)
|
||||
|
||||
// ErrTreePathNotRelative identifies a path that cannot address the workspace tree.
|
||||
var ErrTreePathNotRelative = errors.New("file tree path must be workspace-relative")
|
||||
|
||||
// ValidateTreePath accepts the empty workspace root and canonical slash-separated relative paths.
|
||||
func ValidateTreePath(requested string) error {
|
||||
if requested == "" {
|
||||
return nil
|
||||
}
|
||||
if strings.IndexByte(requested, 0) >= 0 ||
|
||||
strings.Contains(requested, `\`) ||
|
||||
filepath.IsAbs(requested) ||
|
||||
pathpkg.IsAbs(filepath.ToSlash(requested)) ||
|
||||
filepath.VolumeName(requested) != "" ||
|
||||
hasWindowsDrivePrefix(requested) ||
|
||||
hasAbsoluteURIForm(requested) {
|
||||
return ErrTreePathNotRelative
|
||||
}
|
||||
cleaned := pathpkg.Clean(requested)
|
||||
if cleaned != requested || cleaned == "." || cleaned == ".." || strings.HasPrefix(cleaned, "../") {
|
||||
return ErrTreePathNotRelative
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
func hasAbsoluteURIForm(path string) bool {
|
||||
colon := strings.IndexByte(path, ':')
|
||||
if colon <= 0 || !isASCIILetter(path[0]) {
|
||||
return false
|
||||
}
|
||||
for index := 1; index < colon; index++ {
|
||||
char := path[index]
|
||||
if !isASCIILetter(char) && (char < '0' || char > '9') && char != '+' && char != '-' && char != '.' {
|
||||
return false
|
||||
}
|
||||
}
|
||||
remainder := path[colon+1:]
|
||||
return strings.HasPrefix(remainder, "//") ||
|
||||
strings.EqualFold(path[:colon], "file") && strings.HasPrefix(remainder, "/")
|
||||
}
|
||||
|
||||
func hasWindowsDrivePrefix(path string) bool {
|
||||
if len(path) < 2 || path[1] != ':' {
|
||||
return false
|
||||
}
|
||||
first := path[0]
|
||||
return isASCIILetter(first)
|
||||
}
|
||||
|
||||
func isASCIILetter(char byte) bool {
|
||||
return char >= 'A' && char <= 'Z' || char >= 'a' && char <= 'z'
|
||||
}
|
||||
@@ -0,0 +1,35 @@
|
||||
package workspacepath
|
||||
|
||||
import "testing"
|
||||
|
||||
func TestValidateTreePath(t *testing.T) {
|
||||
for _, path := range []string{"", "src", "src/components", ".codex/agents", "config:dev", "dir/.env:"} {
|
||||
t.Run("valid_"+path, func(t *testing.T) {
|
||||
if err := ValidateTreePath(path); err != nil {
|
||||
t.Fatalf("ValidateTreePath(%q) = %v, want nil", path, err)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
for _, path := range []string{
|
||||
"/home/jcfs/project/src",
|
||||
`C:\workspace\src`,
|
||||
"C:/workspace/src",
|
||||
"file:///workspace/src",
|
||||
"file:/workspace/src",
|
||||
".",
|
||||
"..",
|
||||
"../src",
|
||||
"src/../outside",
|
||||
"src//components",
|
||||
"src/components/",
|
||||
"src\\components",
|
||||
"src\x00components",
|
||||
} {
|
||||
t.Run("invalid_"+path, func(t *testing.T) {
|
||||
if err := ValidateTreePath(path); err != ErrTreePathNotRelative {
|
||||
t.Fatalf("ValidateTreePath(%q) = %v, want %v", path, err, ErrTreePathNotRelative)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -57,3 +57,21 @@ describe("ToolEditMessage write summary", () => {
|
||||
expect(html).not.toContain("line)");
|
||||
});
|
||||
});
|
||||
|
||||
describe("ToolEditMessage file action", () => {
|
||||
// @covers AC-UI-FILE-TREE-PATH-SCOPE-001.1
|
||||
it("does not treat a workspace prefix collision as an in-workspace file", () => {
|
||||
const comment = writeComment("Write utils.ts", "only line");
|
||||
const metadata = comment.metadata as {
|
||||
normalized: { modify_file: { file_path: string } };
|
||||
};
|
||||
metadata.normalized.modify_file.file_path = "/workspace-old/apps/web/lib/utils.ts";
|
||||
|
||||
const html = renderToStaticMarkup(
|
||||
<ToolEditMessage comment={comment} worktreePath="/workspace" onOpenFile={() => undefined} />,
|
||||
);
|
||||
|
||||
expect(html).toContain('title="Copy path"');
|
||||
expect(html).not.toContain('title="Open file"');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -18,6 +18,7 @@ import { ExpandableRow } from "./expandable-row";
|
||||
import { transformFileMutation, type FileMutation } from "@/lib/diff";
|
||||
import { useExpandState } from "./use-expand-state";
|
||||
import { useOpenFileAtLine } from "@/hooks/use-file-editors";
|
||||
import { workspaceRelativeFilePath } from "@/lib/workspace-file-path";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import type { TFunction } from "i18next";
|
||||
|
||||
@@ -79,7 +80,7 @@ function FileActionButton({
|
||||
onCopyPath,
|
||||
}: FileActionButtonProps) {
|
||||
const { t } = useTranslation();
|
||||
const isFileInWorktree = worktreePath && filePath.startsWith(worktreePath);
|
||||
const isFileInWorktree = workspaceRelativeFilePath(filePath, worktreePath) !== null;
|
||||
if (onOpenFile && isFileInWorktree) {
|
||||
return (
|
||||
<button
|
||||
|
||||
@@ -20,6 +20,8 @@ const SESSION_ID = "sess";
|
||||
const REFRESH_OP = "refresh";
|
||||
const THM_OLD = "thm/old.txt";
|
||||
const THM_NEW = "thm/new.txt";
|
||||
const SRC_PATH = "src";
|
||||
const SRC_COMPONENTS_PATH = `${SRC_PATH}/components`;
|
||||
|
||||
beforeEach(async () => {
|
||||
vi.resetModules();
|
||||
@@ -84,7 +86,7 @@ describe("applyFileChanges — refresh operation expands to all expanded folders
|
||||
applyFileChanges({
|
||||
client: client(),
|
||||
sessionId: SESSION_ID,
|
||||
expandedPaths: new Set(["src", "src/components"]),
|
||||
expandedPaths: new Set([SRC_PATH, SRC_COMPONENTS_PATH]),
|
||||
changes: [{ path: "", operation: REFRESH_OP }],
|
||||
setTree: vi.fn(),
|
||||
setLoadState: vi.fn(),
|
||||
@@ -92,8 +94,8 @@ describe("applyFileChanges — refresh operation expands to all expanded folders
|
||||
await new Promise<void>((r) => setTimeout(r, 0));
|
||||
expect(requestFileTreeMock.mock.calls.map((c) => c[2]).sort()).toEqual([
|
||||
"",
|
||||
"src",
|
||||
"src/components",
|
||||
SRC_PATH,
|
||||
SRC_COMPONENTS_PATH,
|
||||
]);
|
||||
});
|
||||
|
||||
@@ -151,6 +153,52 @@ describe("applyFileChanges — refresh operation expands to all expanded folders
|
||||
});
|
||||
});
|
||||
|
||||
it("skips invalid expanded paths during a generic refresh", async () => {
|
||||
mockEmptyTree();
|
||||
applyFileChanges({
|
||||
client: client(),
|
||||
sessionId: SESSION_ID,
|
||||
expandedPaths: new Set([SRC_PATH, SRC_COMPONENTS_PATH, "/home/jcfs/project/src"]),
|
||||
changes: [{ path: "", operation: REFRESH_OP }],
|
||||
setTree: vi.fn(),
|
||||
setLoadState: vi.fn(),
|
||||
});
|
||||
await new Promise<void>((r) => setTimeout(r, 0));
|
||||
expect(requestFileTreeMock.mock.calls.map((c) => c[2]).sort()).toEqual([
|
||||
"",
|
||||
SRC_PATH,
|
||||
SRC_COMPONENTS_PATH,
|
||||
]);
|
||||
});
|
||||
|
||||
it("refreshes an expanded path containing a literal colon", async () => {
|
||||
mockEmptyTree();
|
||||
applyFileChanges({
|
||||
client: client(),
|
||||
sessionId: SESSION_ID,
|
||||
expandedPaths: new Set(["config:dev"]),
|
||||
changes: [{ path: "", operation: REFRESH_OP }],
|
||||
setTree: vi.fn(),
|
||||
setLoadState: vi.fn(),
|
||||
});
|
||||
await new Promise<void>((r) => setTimeout(r, 0));
|
||||
expect(requestFileTreeMock.mock.calls.map((c) => c[2]).sort()).toEqual(["", "config:dev"]);
|
||||
});
|
||||
|
||||
it("ignores a specific change with an invalid path", async () => {
|
||||
mockEmptyTree();
|
||||
applyFileChanges({
|
||||
client: client(),
|
||||
sessionId: SESSION_ID,
|
||||
expandedPaths: new Set([SRC_PATH]),
|
||||
changes: [{ path: "/home/jcfs/project/src/app.ts", operation: "create" }],
|
||||
setTree: vi.fn(),
|
||||
setLoadState: vi.fn(),
|
||||
});
|
||||
await new Promise<void>((r) => setTimeout(r, 0));
|
||||
expect(requestFileTreeMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// Regression (#982): refresh scoped to one repo must not wipe sibling repos' loaded subtrees.
|
||||
describe("applyFileChanges — cross-repo subtree preservation", () => {
|
||||
it("preserves kandev's loaded children when refresh is scoped to thm", async () => {
|
||||
|
||||
@@ -13,6 +13,7 @@ import { compareTreeNodes, sortRootChildren } from "./file-tree-utils";
|
||||
import { restoredExpandedPaths } from "./file-browser-restore";
|
||||
import { useTreeLoader } from "./file-browser-tree-loader";
|
||||
import { createDebugLogger, isDebug } from "@/lib/debug/log";
|
||||
import { isWorkspaceTreePath } from "@/lib/workspace-file-path";
|
||||
|
||||
const debugLoad = createDebugLogger("file-browser:load");
|
||||
const debugChanges = createDebugLogger("file-browser:changes");
|
||||
@@ -153,6 +154,11 @@ function nearestExpandedFolder(parentPath: string, expandedPaths: ReadonlySet<st
|
||||
return "";
|
||||
}
|
||||
|
||||
function isExpandedPathInRefreshScope(path: string, repositoryName?: string): boolean {
|
||||
if (!isWorkspaceTreePath(path)) return false;
|
||||
return !repositoryName || path === repositoryName || path.startsWith(`${repositoryName}/`);
|
||||
}
|
||||
|
||||
/** Apply incoming file changes to the tree by refreshing affected folders. */
|
||||
export function applyFileChanges(ctx: {
|
||||
client: ReturnType<typeof getWebSocketClient>;
|
||||
@@ -169,13 +175,14 @@ export function applyFileChanges(ctx: {
|
||||
foldersToRefresh.add("");
|
||||
const repo = change.repository_name;
|
||||
for (const exp of expandedPaths) {
|
||||
if (!repo || exp === repo || exp.startsWith(repo + "/")) {
|
||||
if (isExpandedPathInRefreshScope(exp, repo)) {
|
||||
foldersToRefresh.add(exp);
|
||||
}
|
||||
}
|
||||
continue;
|
||||
}
|
||||
const p = change.path;
|
||||
if (!isWorkspaceTreePath(p)) continue;
|
||||
const lastSlash = p.lastIndexOf("/");
|
||||
const parent = lastSlash === -1 ? "" : p.substring(0, lastSlash);
|
||||
foldersToRefresh.add(nearestExpandedFolder(parent, expandedPaths));
|
||||
|
||||
@@ -81,6 +81,22 @@ describe("useFileBrowserTree persisted expansion", () => {
|
||||
expect(result.current.expandedPaths).toEqual(new Set(EXPANDED_PATHS));
|
||||
});
|
||||
|
||||
it("discards invalid persisted paths while restoring valid siblings", async () => {
|
||||
sessionStorage.setItem(
|
||||
STORAGE_KEY,
|
||||
JSON.stringify([CODEX_PATH, AGENTS_PATH, "/home/jcfs/project/public/assets"]),
|
||||
);
|
||||
const { result } = renderHook(() => useFileBrowserTree(SESSION, ENVIRONMENT));
|
||||
|
||||
await waitFor(() => expect(result.current.loadState).toBe("loaded"));
|
||||
|
||||
expect(requestFileTreeMock.mock.calls.map((call) => call[2])).toEqual([
|
||||
ROOT_PATH,
|
||||
...EXPANDED_PATHS,
|
||||
]);
|
||||
expect(result.current.expandedPaths).toEqual(new Set(EXPANDED_PATHS));
|
||||
});
|
||||
|
||||
it("retries a transient restored-folder failure without pruning persisted expansion", async () => {
|
||||
vi.useFakeTimers();
|
||||
let codexAttempts = 0;
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import { getWebSocketClient } from "@/lib/ws/connection";
|
||||
import { requestFileTree } from "@/lib/ws/workspace-files";
|
||||
import type { FileTreeNode } from "@/lib/types/backend";
|
||||
import { isWorkspaceTreePath } from "@/lib/workspace-file-path";
|
||||
import { findNodeByPath, mergeTreeNodes } from "./file-tree-utils";
|
||||
import type { TreeLoadOwner } from "./file-browser-tree-loader";
|
||||
export type RestoredTree = {
|
||||
@@ -12,7 +13,7 @@ export type RestoredTree = {
|
||||
export function restoredExpandedPaths(paths: string[]): string[] {
|
||||
const restored = new Set<string>();
|
||||
for (const path of paths) {
|
||||
if (!path) continue;
|
||||
if (!path || !isWorkspaceTreePath(path)) continue;
|
||||
const parts = path.split("/");
|
||||
for (let i = 1; i <= parts.length; i++) restored.add(parts.slice(0, i).join("/"));
|
||||
}
|
||||
|
||||
@@ -273,3 +273,23 @@ it("does not reopen an active branch after the user collapses it", () => {
|
||||
expect(expanded.current.size).toBe(0);
|
||||
expect(setExpandedPaths).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("ignores an absolute active file path", async () => {
|
||||
const loadChildren = vi.fn<(node: FileTreeNode) => Promise<boolean>>();
|
||||
const { expanded, setExpandedPaths } = expansionState();
|
||||
|
||||
renderHook(() =>
|
||||
useFileTreeReveal({
|
||||
activeFilePath: "/home/jcfs/project/src/main/Greeting.kt",
|
||||
sessionId: "session-1",
|
||||
tree: srcOnlyTree(),
|
||||
setExpandedPaths,
|
||||
isLoading: () => false,
|
||||
loadChildren,
|
||||
}),
|
||||
);
|
||||
await act(async () => Promise.resolve());
|
||||
|
||||
expect(expanded.current).toEqual(new Set());
|
||||
expect(loadChildren).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
import { useEffect, useRef, useState } from "react";
|
||||
import type React from "react";
|
||||
import type { FileTreeNode } from "@/lib/types/backend";
|
||||
import { isWorkspaceTreePath } from "@/lib/workspace-file-path";
|
||||
import { findUnloadedAncestor, getAncestorPaths } from "./file-tree-utils";
|
||||
|
||||
const DEFAULT_RETRY_DELAYS_MS = [250, 1_000] as const;
|
||||
@@ -62,16 +63,17 @@ export function useFileTreeReveal(params: FileTreeRevealParams) {
|
||||
latestRef.current = { isLoading, loadChildren, retryDelaysMs };
|
||||
const runRef = useRef<RevealRun | null>(null);
|
||||
const [revision, setRevision] = useState(0);
|
||||
const revealKey = activeFilePath ? `${sessionId}\0${activeFilePath}` : null;
|
||||
const targetPath = activeFilePath && isWorkspaceTreePath(activeFilePath) ? activeFilePath : null;
|
||||
const revealKey = targetPath ? `${sessionId}\0${targetPath}` : null;
|
||||
const treeAvailable = tree !== null;
|
||||
|
||||
useEffect(() => {
|
||||
if (!activeFilePath || !treeAvailable) return;
|
||||
expandAncestors(activeFilePath, setExpandedPaths);
|
||||
}, [activeFilePath, revealKey, setExpandedPaths, treeAvailable]);
|
||||
if (!targetPath || !treeAvailable) return;
|
||||
expandAncestors(targetPath, setExpandedPaths);
|
||||
}, [revealKey, setExpandedPaths, targetPath, treeAvailable]);
|
||||
|
||||
useEffect(() => {
|
||||
if (!activeFilePath || !revealKey || !tree) {
|
||||
if (!targetPath || !revealKey || !tree) {
|
||||
clearRetry(runRef.current);
|
||||
runRef.current = null;
|
||||
return;
|
||||
@@ -82,7 +84,7 @@ export function useFileTreeReveal(params: FileTreeRevealParams) {
|
||||
runRef.current = createRun(revealKey);
|
||||
}
|
||||
const run = runRef.current;
|
||||
const ancestor = findUnloadedAncestor(tree, activeFilePath, getAncestorPaths(activeFilePath));
|
||||
const ancestor = findUnloadedAncestor(tree, targetPath, getAncestorPaths(targetPath));
|
||||
if (!ancestor) {
|
||||
clearRetry(run);
|
||||
return;
|
||||
@@ -125,7 +127,7 @@ export function useFileTreeReveal(params: FileTreeRevealParams) {
|
||||
run.inFlightPath = null;
|
||||
console.error("Failed to reveal file tree path", error);
|
||||
});
|
||||
}, [activeFilePath, revealKey, revision, tree]);
|
||||
}, [revealKey, revision, targetPath, tree]);
|
||||
|
||||
useEffect(
|
||||
() => () => {
|
||||
|
||||
@@ -57,6 +57,24 @@ export async function waitForSessionDone(
|
||||
);
|
||||
}
|
||||
|
||||
export async function waitForWorkspacePath(
|
||||
apiClient: ApiClient,
|
||||
taskId: string,
|
||||
sessionId: string,
|
||||
timeout = 30_000,
|
||||
): Promise<string> {
|
||||
return pollUntil(
|
||||
async () => {
|
||||
const { sessions } = await apiClient.listTaskSessions(taskId);
|
||||
const session = sessions.find((candidate) => candidate.id === sessionId);
|
||||
return session?.workspace_path ?? session?.worktree_path ?? "";
|
||||
},
|
||||
(workspacePath) => workspacePath.length > 0,
|
||||
timeout,
|
||||
"Waiting for task workspace path",
|
||||
);
|
||||
}
|
||||
|
||||
export async function waitForAgentMessage(
|
||||
apiClient: ApiClient,
|
||||
sessionId: string,
|
||||
|
||||
@@ -0,0 +1,101 @@
|
||||
import path from "node:path";
|
||||
import type { Page } from "@playwright/test";
|
||||
import { test, expect } from "../../fixtures/test-base";
|
||||
import { GitHelper, makeGitEnv, createStandardProfile } from "../../helpers/git-helper";
|
||||
import { waitForWorkspacePath } from "../../helpers/session";
|
||||
import { SessionPage } from "../../pages/session-page";
|
||||
|
||||
function observeFileTreeRequestPaths(page: Page): string[] {
|
||||
const paths: string[] = [];
|
||||
page.on("websocket", (socket) => {
|
||||
if (!socket.url().endsWith("/ws")) return;
|
||||
socket.on("framesent", ({ payload }) => {
|
||||
const text = typeof payload === "string" ? payload : payload.toString();
|
||||
try {
|
||||
const frame = JSON.parse(text) as {
|
||||
action?: string;
|
||||
payload?: { path?: unknown };
|
||||
};
|
||||
if (frame.action === "workspace.tree.get" && typeof frame.payload?.path === "string") {
|
||||
paths.push(frame.payload.path);
|
||||
}
|
||||
} catch {
|
||||
// Gateway-adjacent sockets can carry non-JSON frames.
|
||||
}
|
||||
});
|
||||
});
|
||||
return paths;
|
||||
}
|
||||
|
||||
test.describe("Absolute workspace read links", () => {
|
||||
// @covers AC-UI-FILE-TREE-PATH-SCOPE-001.1
|
||||
// @covers AC-UI-FILE-TREE-PATH-SCOPE-001.3
|
||||
test("opens with a relative editor and file-tree identity", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
seedData,
|
||||
backend,
|
||||
}) => {
|
||||
test.setTimeout(90_000);
|
||||
const suffix = `${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;
|
||||
const directory = `absolute-read-${suffix}`;
|
||||
const fileName = "case.ts";
|
||||
const filePath = `${directory}/${fileName}`;
|
||||
const git = new GitHelper(
|
||||
path.join(backend.tmpDir, "repos", "e2e-repo"),
|
||||
makeGitEnv(backend.tmpDir),
|
||||
);
|
||||
git.createFile(filePath, 'export const suspect = "Colonel Mustard";');
|
||||
git.stageAll();
|
||||
git.commit(`add ${filePath}`);
|
||||
|
||||
const profile = await createStandardProfile(apiClient, `absolute-read-${Date.now()}`);
|
||||
const task = await apiClient.createTaskWithAgent(
|
||||
seedData.workspaceId,
|
||||
"Absolute Workspace Read",
|
||||
profile.id,
|
||||
{
|
||||
description: 'e2e:message("ready")',
|
||||
workflow_id: seedData.workflowId,
|
||||
workflow_step_id: seedData.startStepId,
|
||||
repository_ids: [seedData.repositoryId],
|
||||
},
|
||||
);
|
||||
if (!task.session_id) throw new Error("createTaskWithAgent did not return a session_id");
|
||||
const workspacePath = await waitForWorkspacePath(apiClient, task.id, task.session_id);
|
||||
const absoluteFilePath = path.join(workspacePath, filePath);
|
||||
await apiClient.seedSessionMessage(task.session_id, {
|
||||
type: "tool_read",
|
||||
content: "Read file",
|
||||
metadata: {
|
||||
status: "complete",
|
||||
tool_call_id: "tc-absolute-workspace-read",
|
||||
normalized: {
|
||||
read_file: {
|
||||
file_path: absoluteFilePath,
|
||||
offset: 1,
|
||||
limit: 1,
|
||||
output: { content: 'export const suspect = "Colonel Mustard";', line_count: 1 },
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const treeRequestPaths = observeFileTreeRequestPaths(testPage);
|
||||
await testPage.goto(`/t/${task.id}`);
|
||||
const session = new SessionPage(testPage);
|
||||
await session.waitForLoad();
|
||||
await session.waitForChatIdle({ timeout: 45_000 });
|
||||
|
||||
const link = session.activeChat().locator(`button[title="${absoluteFilePath}"]`);
|
||||
await expect(link).toBeVisible({ timeout: 15_000 });
|
||||
await link.click();
|
||||
await expect(testPage.getByTestId("preview-tab-file-editor")).toBeVisible({ timeout: 15_000 });
|
||||
await expect(testPage.locator(".dv-tab.dv-active-tab", { hasText: fileName })).toBeVisible();
|
||||
|
||||
await session.clickTab("Files");
|
||||
await expect(session.fileTreeNode(filePath)).toBeVisible({ timeout: 15_000 });
|
||||
expect(treeRequestPaths.length).toBeGreaterThan(0);
|
||||
expect(treeRequestPaths.every((requestPath) => !path.isAbsolute(requestPath))).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -12,6 +12,7 @@ import type { ApiClient } from "../../helpers/api-client";
|
||||
import type { BackendContext } from "../../fixtures/backend";
|
||||
import { GitHelper, makeGitEnv, createStandardProfile } from "../../helpers/git-helper";
|
||||
import { selectMarkdownPreviewText } from "../../helpers/markdown-preview";
|
||||
import { waitForWorkspacePath } from "../../helpers/session";
|
||||
import { SessionPage } from "../../pages/session-page";
|
||||
|
||||
function createLongFileContent(lines = 500): string {
|
||||
@@ -521,13 +522,11 @@ test.describe("Mobile file viewer panel", () => {
|
||||
await expect(viewer.getByText(filePath)).toBeVisible();
|
||||
});
|
||||
|
||||
// A chat read/edit file link can target a line deep in a file. Tapping it sets
|
||||
// a pending cursor position (use-file-editors), then on mobile the file opens
|
||||
// through MobileFileViewerPanel → FileViewerContent (CodeMirror), which must
|
||||
// consume that pending entry and scroll the target line into view. Regression
|
||||
// guard for the mobile half of "open-and-scroll-to-line": before the fix
|
||||
// CodeMirror ignored the pending map and the file opened pinned at the top.
|
||||
test("tapping a chat read link scrolls the CodeMirror viewer to the target line", async ({
|
||||
// @covers AC-UI-FILE-TREE-PATH-SCOPE-001.1
|
||||
// @covers AC-UI-FILE-TREE-PATH-SCOPE-001.6
|
||||
// An absolute workspace path from a read tool must use the same relative
|
||||
// identity for the native viewer and its pending cursor position.
|
||||
test("tapping an absolute workspace read link scrolls to the target line", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
seedData,
|
||||
@@ -562,11 +561,11 @@ test.describe("Mobile file viewer panel", () => {
|
||||
},
|
||||
);
|
||||
if (!task.session_id) throw new Error("createTaskWithAgent did not return a session_id");
|
||||
const workspacePath = await waitForWorkspacePath(apiClient, task.id, task.session_id);
|
||||
const absoluteFilePath = path.join(workspacePath, filePath);
|
||||
|
||||
// Seed a real read card pointing deep into the file. The chat read-message
|
||||
// component sets the pending cursor position from `offset` before opening,
|
||||
// keyed by this exact `file_path` — the same string the mobile viewer keys
|
||||
// FileViewerContent on, so the consume matches.
|
||||
// Seed the absolute path emitted by an agent tool. The opener must normalize
|
||||
// it before keying the pending cursor and native viewer with the same path.
|
||||
const targetLine = 300;
|
||||
await apiClient.seedSessionMessage(task.session_id, {
|
||||
type: "tool_read",
|
||||
@@ -576,7 +575,7 @@ test.describe("Mobile file viewer panel", () => {
|
||||
tool_call_id: "tc-chat-read-scroll",
|
||||
normalized: {
|
||||
read_file: {
|
||||
file_path: filePath,
|
||||
file_path: absoluteFilePath,
|
||||
offset: targetLine,
|
||||
limit: 20,
|
||||
output: { content: 'export const line_299 = "line 299";', line_count: 20 },
|
||||
@@ -590,14 +589,16 @@ test.describe("Mobile file viewer panel", () => {
|
||||
await session.waitForLoad();
|
||||
await session.waitForChatIdle({ timeout: 45_000 });
|
||||
|
||||
// FilePathButton renders the openable link with the seeded path as `title`.
|
||||
// FilePathButton preserves the recorded path in its title while displaying
|
||||
// the relative suffix.
|
||||
const chat = session.activeChat();
|
||||
const fileLink = chat.locator(`button[title="${filePath}"]`);
|
||||
const fileLink = chat.locator(`button[title="${absoluteFilePath}"]`);
|
||||
await expect(fileLink).toBeVisible({ timeout: 15_000 });
|
||||
await fileLink.tap();
|
||||
|
||||
const viewer = testPage.getByTestId("mobile-file-viewer-panel");
|
||||
await expect(viewer).toBeVisible({ timeout: 10_000 });
|
||||
await expect(viewer.getByText(filePath)).toBeVisible();
|
||||
|
||||
const cmScroller = viewer.locator(".cm-scroller").first();
|
||||
await expect(cmScroller).toBeVisible();
|
||||
|
||||
@@ -271,6 +271,32 @@ describe("useOpenFileAtLine", () => {
|
||||
expect(consumePendingCursorPosition(APP_PATH)).toBeUndefined();
|
||||
});
|
||||
|
||||
// @covers AC-UI-FILE-TREE-PATH-SCOPE-001.1
|
||||
it("uses one workspace-relative identity for an absolute tool path", () => {
|
||||
const openFile = vi.fn();
|
||||
const editor = createEditor(WORKTREE_APP_PATH);
|
||||
getMonacoInstance.mockReturnValue({ editor: { getEditors: () => [editor] } });
|
||||
const { result } = renderHook(() => useOpenFileAtLine(openFile, 88, WORKTREE_PATH));
|
||||
|
||||
act(() => result.current(WORKTREE_APP_PATH));
|
||||
|
||||
expect(openFile).toHaveBeenCalledWith(APP_PATH);
|
||||
expect(editor.setPosition).toHaveBeenCalledWith({ lineNumber: 88, column: 1 });
|
||||
expect(consumePendingCursorPosition(APP_PATH)).toBeUndefined();
|
||||
expect(consumePendingCursorPosition(WORKTREE_APP_PATH)).toBeUndefined();
|
||||
});
|
||||
|
||||
it("normalizes Windows workspace aliases case-insensitively", () => {
|
||||
const openFile = vi.fn();
|
||||
const { result } = renderHook(() =>
|
||||
useOpenFileAtLine(openFile, undefined, String.raw`C:\Workspace`),
|
||||
);
|
||||
|
||||
act(() => result.current(String.raw`c:\workspace\src\app.ts`));
|
||||
|
||||
expect(openFile).toHaveBeenCalledWith(APP_PATH);
|
||||
});
|
||||
|
||||
it("scrolls only the requested session-qualified model", () => {
|
||||
const firstModelUri = modelUriForDocument(DOCUMENT_URI, FIRST_SESSION_ID);
|
||||
const secondModelUri = modelUriForDocument(DOCUMENT_URI, SECOND_SESSION_ID);
|
||||
|
||||
@@ -13,6 +13,7 @@ import {
|
||||
} from "@/lib/local-storage";
|
||||
import { calculateHash } from "@/lib/utils/file-diff";
|
||||
import { getFilePreviewKind } from "@/lib/utils/file-types";
|
||||
import { normalizeWorkspaceFilePath } from "@/lib/workspace-file-path";
|
||||
import { useToast } from "@/components/toast-provider";
|
||||
import { useSessionGitStatus } from "@/hooks/domains/session/use-session-git-status";
|
||||
import { useSaveDeleteActions } from "./use-file-save-delete";
|
||||
@@ -45,13 +46,14 @@ export function useOpenFileAtLine(
|
||||
) {
|
||||
return useCallback(
|
||||
(path: string) => {
|
||||
const openPath = normalizeWorkspaceFilePath(path, worktreePath);
|
||||
if (startLine && startLine > 0) {
|
||||
setPendingCursorPosition(path, startLine, 1, undefined, sessionId);
|
||||
onOpenFile?.(path);
|
||||
scrollEditorIfMounted(path, worktreePath ?? null, startLine, 1, { sessionId });
|
||||
setPendingCursorPosition(openPath, startLine, 1, undefined, sessionId);
|
||||
onOpenFile?.(openPath);
|
||||
scrollEditorIfMounted(openPath, worktreePath ?? null, startLine, 1, { sessionId });
|
||||
return;
|
||||
}
|
||||
onOpenFile?.(path);
|
||||
onOpenFile?.(openPath);
|
||||
},
|
||||
[onOpenFile, startLine, worktreePath, sessionId],
|
||||
);
|
||||
|
||||
@@ -18,12 +18,10 @@ import {
|
||||
type WorkspaceFileLocation,
|
||||
} from "@/lib/lsp/file-uri";
|
||||
import { useDockviewStore } from "@/lib/state/dockview-store";
|
||||
import { normalizeWorkspaceFilePath } from "@/lib/workspace-file-path";
|
||||
|
||||
export function toWorkspaceRelativePath(filePath: string, workspacePath: string | null): string {
|
||||
const normalizedWorkspacePath = workspacePath?.replace(/\/+$/, "") ?? "";
|
||||
if (!normalizedWorkspacePath) return filePath;
|
||||
const workspacePrefix = `${normalizedWorkspacePath}/`;
|
||||
return filePath.startsWith(workspacePrefix) ? filePath.slice(workspacePrefix.length) : filePath;
|
||||
return normalizeWorkspaceFilePath(filePath, workspacePath);
|
||||
}
|
||||
|
||||
export function preserveExistingEditorLocation(
|
||||
|
||||
@@ -0,0 +1,90 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import {
|
||||
isWorkspaceTreePath,
|
||||
normalizeWorkspaceFilePath,
|
||||
workspaceRelativeFilePath,
|
||||
} from "./workspace-file-path";
|
||||
|
||||
const POSIX_WORKSPACE_ROOT = "/workspace";
|
||||
const POSIX_WORKSPACE_FILE = "/workspace/src/app.ts";
|
||||
const RELATIVE_WORKSPACE_FILE = "src/app.ts";
|
||||
|
||||
describe("workspaceRelativeFilePath", () => {
|
||||
it.each([
|
||||
["POSIX path", POSIX_WORKSPACE_FILE, POSIX_WORKSPACE_ROOT, RELATIVE_WORKSPACE_FILE],
|
||||
["trailing root separator", POSIX_WORKSPACE_FILE, "/workspace/", RELATIVE_WORKSPACE_FILE],
|
||||
[
|
||||
"Windows path",
|
||||
String.raw`c:\workspace\src\app.ts`,
|
||||
String.raw`C:\Workspace`,
|
||||
RELATIVE_WORKSPACE_FILE,
|
||||
],
|
||||
["Windows drive root", String.raw`C:\src\app.ts`, "c:\\", RELATIVE_WORKSPACE_FILE],
|
||||
["file URI", "file:///workspace/src/app.ts", "file:///workspace", RELATIVE_WORKSPACE_FILE],
|
||||
["file URI root", "file:///src/app.ts", "file:///", RELATIVE_WORKSPACE_FILE],
|
||||
["Windows file URI", "file:///c:/workspace/SRC/App.ts", "file:///C:/Workspace", "SRC/App.ts"],
|
||||
[
|
||||
"UNC path",
|
||||
String.raw`\\BUILD-SERVER\WORK\src\app.ts`,
|
||||
String.raw`\\build-server\work`,
|
||||
RELATIVE_WORKSPACE_FILE,
|
||||
],
|
||||
[
|
||||
"UNC file URI",
|
||||
"file://BUILD-SERVER/WORK/src/app.ts",
|
||||
"file://build-server/work",
|
||||
RELATIVE_WORKSPACE_FILE,
|
||||
],
|
||||
])("resolves a contained %s", (_label, filePath, workspaceRoot, expected) => {
|
||||
expect(workspaceRelativeFilePath(filePath, workspaceRoot)).toBe(expected);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["prefix collision", "/workspace-old/src/app.ts", POSIX_WORKSPACE_ROOT],
|
||||
["external absolute", "/opt/reference.md", POSIX_WORKSPACE_ROOT],
|
||||
["absolute traversal", "/workspace/../outside/app.ts", POSIX_WORKSPACE_ROOT],
|
||||
["case-distinct POSIX file URI", "file:///Workspace/src/app.ts", "file:///workspace"],
|
||||
["missing root", POSIX_WORKSPACE_FILE, null],
|
||||
])("rejects a %s", (_label, filePath, workspaceRoot) => {
|
||||
expect(workspaceRelativeFilePath(filePath, workspaceRoot)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("normalizeWorkspaceFilePath", () => {
|
||||
it("preserves an external absolute path", () => {
|
||||
expect(normalizeWorkspaceFilePath("/opt/reference.md", POSIX_WORKSPACE_ROOT)).toBe(
|
||||
"/opt/reference.md",
|
||||
);
|
||||
});
|
||||
|
||||
it("canonicalizes separators in an already-relative path", () => {
|
||||
expect(normalizeWorkspaceFilePath(String.raw`src\app.ts`, String.raw`C:\Workspace`)).toBe(
|
||||
"src/app.ts",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("isWorkspaceTreePath", () => {
|
||||
it.each(["", "src", "src/components", ".codex/agents", "config:dev", "dir/.env:"])(
|
||||
"accepts the workspace-relative tree path %j",
|
||||
(path) => {
|
||||
expect(isWorkspaceTreePath(path)).toBe(true);
|
||||
},
|
||||
);
|
||||
|
||||
it.each([
|
||||
"/home/jcfs/project/src",
|
||||
String.raw`C:\workspace\src`,
|
||||
"file:///workspace/src",
|
||||
"file:/workspace/src",
|
||||
".",
|
||||
"..",
|
||||
"../src",
|
||||
"src/../outside",
|
||||
"src//components",
|
||||
String.raw`src\components`,
|
||||
"src\0components",
|
||||
])("rejects the non-canonical tree path %j", (path) => {
|
||||
expect(isWorkspaceTreePath(path)).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,73 @@
|
||||
function normalizeSeparators(path: string): string {
|
||||
return path.replaceAll("\\", "/");
|
||||
}
|
||||
|
||||
function trimTrailingSeparators(path: string): string {
|
||||
if (path === "/" || /^[A-Za-z]:\/$/.test(path) || /^file:\/\/\/(?:[A-Za-z]:\/)?$/i.test(path)) {
|
||||
return path;
|
||||
}
|
||||
return path.replace(/\/+$/, "");
|
||||
}
|
||||
|
||||
function usesWindowsPathSemantics(path: string): boolean {
|
||||
return (
|
||||
/^[A-Za-z]:\//.test(path) ||
|
||||
path.startsWith("//") ||
|
||||
/^file:\/\/\/[A-Za-z]:(?:\/|$)/i.test(path) ||
|
||||
/^file:\/\/[^/]/i.test(path)
|
||||
);
|
||||
}
|
||||
|
||||
function hasAbsoluteURIForm(path: string): boolean {
|
||||
return /^[A-Za-z][A-Za-z\d+.-]*:\/\//.test(path) || /^file:\//i.test(path);
|
||||
}
|
||||
|
||||
function isAbsoluteFilePath(path: string): boolean {
|
||||
return path.startsWith("/") || usesWindowsPathSemantics(path) || hasAbsoluteURIForm(path);
|
||||
}
|
||||
|
||||
/** Return whether a path is a canonical workspace-relative tree identity. */
|
||||
export function isWorkspaceTreePath(path: string): boolean {
|
||||
if (path === "") return true;
|
||||
if (
|
||||
path.startsWith("/") ||
|
||||
path.includes("\\") ||
|
||||
path.includes("\0") ||
|
||||
/^[A-Za-z]:/.test(path) ||
|
||||
hasAbsoluteURIForm(path)
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
return path.split("/").every((segment) => segment !== "" && segment !== "." && segment !== "..");
|
||||
}
|
||||
|
||||
/** Return a workspace-relative alias for a contained path, or null when it is outside. */
|
||||
export function workspaceRelativeFilePath(
|
||||
filePath: string,
|
||||
workspaceRoot: string | null | undefined,
|
||||
): string | null {
|
||||
if (!workspaceRoot) return null;
|
||||
const path = trimTrailingSeparators(normalizeSeparators(filePath));
|
||||
const root = trimTrailingSeparators(normalizeSeparators(workspaceRoot));
|
||||
if (!root) return null;
|
||||
|
||||
const windowsSemantics = usesWindowsPathSemantics(path) || usesWindowsPathSemantics(root);
|
||||
const comparablePath = windowsSemantics ? path.toLocaleLowerCase("en-US") : path;
|
||||
const comparableRoot = windowsSemantics ? root.toLocaleLowerCase("en-US") : root;
|
||||
if (comparablePath === comparableRoot) return "";
|
||||
const rootPrefix = comparableRoot.endsWith("/") ? comparableRoot : `${comparableRoot}/`;
|
||||
if (!comparablePath.startsWith(rootPrefix)) return null;
|
||||
const relativePath = path.slice(rootPrefix.length);
|
||||
return isWorkspaceTreePath(relativePath) ? relativePath : null;
|
||||
}
|
||||
|
||||
/** Normalize a workspace-contained path while preserving external absolute paths. */
|
||||
export function normalizeWorkspaceFilePath(
|
||||
filePath: string,
|
||||
workspaceRoot: string | null | undefined,
|
||||
): string {
|
||||
const relativePath = workspaceRelativeFilePath(filePath, workspaceRoot);
|
||||
if (relativePath !== null) return relativePath;
|
||||
const normalizedPath = normalizeSeparators(filePath);
|
||||
return isAbsoluteFilePath(normalizedPath) ? filePath : normalizedPath;
|
||||
}
|
||||
@@ -0,0 +1,184 @@
|
||||
---
|
||||
created: 2026-09-21
|
||||
status: implemented
|
||||
requirements:
|
||||
- REQ-UI-FILE-TREE-PATH-SCOPE-001
|
||||
system_design:
|
||||
- ../../specs/ui/system-design/file-tree-path-scope.md
|
||||
legacy_specs: []
|
||||
---
|
||||
|
||||
# Implementation Plan: File Tree Path Scope
|
||||
|
||||
## Overview
|
||||
|
||||
Keep agent-referenced workspace files and the task Files tree on one relative path identity, while
|
||||
preserving the separate read-only absolute-file content contract. The implementation first
|
||||
normalizes tool-originated paths before editor and cursor state diverge. It then validates every
|
||||
source of file-tree state and rejects invalid direct requests at both backend boundaries.
|
||||
|
||||
## Confirmed root cause
|
||||
|
||||
- Agent read and edit metadata can contain `/home/jcfs/playground/mystery/...`, even though the
|
||||
current task workspace is already `/home/jcfs/playground/mystery`.
|
||||
- `FilePathButton` displays the relative suffix but forwards the original absolute path.
|
||||
- `useOpenFileAtLine` keys pending cursor and editor state with that unnormalized value.
|
||||
- `FilesPanel` forwards the active editor path to `useFileTreeReveal`, which derives and persists
|
||||
`/home`, `/home/jcfs`, and every deeper absolute ancestor.
|
||||
- A generic file refresh fans out one `workspace.tree.get` request for every expanded entry.
|
||||
- `WorkspaceTracker.GetFileTree` joins each request to `workDir`; the absolute-looking request is
|
||||
therefore evaluated as `<workDir>/home/jcfs/...` and fails `stat`.
|
||||
- `WorkspaceFileHandlers.wsGetFileTree` logs every agentctl failure at ERROR, so one contaminated
|
||||
expansion set produces the repeated stack traces in the incident.
|
||||
|
||||
## Scope
|
||||
|
||||
### In scope
|
||||
|
||||
- Normalize agent tool file paths contained by the active task workspace before editor, tab, and
|
||||
cursor state is written.
|
||||
- Preserve absolute external file content reads without treating them as tree nodes.
|
||||
- Validate reveal, restored expansion, and refresh-derived tree paths.
|
||||
- Clean older invalid expansion state while retaining valid entries.
|
||||
- Reject absolute and escaping tree paths before backend filesystem lookup or ERROR logging.
|
||||
- Add focused frontend, backend, desktop, and phone regression coverage.
|
||||
|
||||
### Out of scope
|
||||
|
||||
- Changing ADR 0016 or restricting external read-only file content.
|
||||
- Changing Files-panel or editor layout, controls, styling, copy, or navigation.
|
||||
- Mapping arbitrary external directories into a task workspace.
|
||||
- Changing Markdown source-checkout aliases or repository materialization.
|
||||
|
||||
## Technical approach
|
||||
|
||||
Add a pure frontend path module that owns containment and tree-path admission for POSIX and Windows
|
||||
forms. Use it in the tool file opener before cursor state is keyed, then at reveal, restore, and
|
||||
refresh boundaries. Keep external absolute opens unchanged and filter them only from tree state.
|
||||
|
||||
Validate `workspace.tree.get` in the WebSocket handler before execution lookup so invalid client
|
||||
input returns a validation response without an ERROR log. Repeat the validation in agentctl before
|
||||
joining or statting the path. Keep current behavior for valid relative directories and genuine
|
||||
dependency failures.
|
||||
|
||||
## ASCII UI preview
|
||||
|
||||
### UI-01: Open a workspace file from an agent tool card
|
||||
|
||||
Entry point: an agent read or edit card whose recorded path is absolute. State: the file is inside
|
||||
the active task workspace.
|
||||
|
||||
Current desktop behavior:
|
||||
|
||||
```text
|
||||
Chat Files Editor
|
||||
+----------------------+ +------------------------+ +---------------------------+
|
||||
| Read public/...json | -> | hidden state: /home/...| | death-on-the-nile/case.json|
|
||||
+----------------------+ +------------------------+ +---------------------------+
|
||||
|
|
||||
+-- refresh sends /home/... as tree paths
|
||||
```
|
||||
|
||||
Proposed desktop behavior:
|
||||
|
||||
```text
|
||||
Chat Files Editor
|
||||
+----------------------+ +------------------------+ +---------------------------+
|
||||
| Read public/...json | -> | public | | public/.../case.json |
|
||||
+----------------------+ | assets | +---------------------------+
|
||||
| cases |
|
||||
| death-on-the-nile|
|
||||
+------------------------+
|
||||
```
|
||||
|
||||
Proposed phone behavior:
|
||||
|
||||
```text
|
||||
+--------------------------------+
|
||||
| Chat |
|
||||
| Read public/.../case.json |
|
||||
+--------------------------------+
|
||||
tap
|
||||
v
|
||||
+--------------------------------+
|
||||
| < Files public/.../case.json |
|
||||
| |
|
||||
| existing native file viewer |
|
||||
+--------------------------------+
|
||||
```
|
||||
|
||||
An external absolute file still opens in the existing editor or phone viewer, but the Files tree
|
||||
does not move or store its ancestors. The path identity and unchanged navigation are requirements;
|
||||
the spacing and example filenames are illustrative. No new copy or control is introduced.
|
||||
|
||||
## Work orders
|
||||
|
||||
- [done] [Task 01: Normalize Tool File Paths](task-01-normalize-tool-file-paths.md)
|
||||
- [done] [Task 02: Enforce Tree Request Scope](task-02-enforce-tree-request-scope.md)
|
||||
|
||||
## Dependency order
|
||||
|
||||
```text
|
||||
Task 01 -> Task 02
|
||||
```
|
||||
|
||||
Task 01 establishes the shared path classifier and editor identity. Task 02 uses that classifier at
|
||||
the Files-tree boundaries and adds the backend defense in depth.
|
||||
|
||||
## Verification strategy
|
||||
|
||||
- Pure frontend tests prove containment and canonical identity on POSIX and Windows paths.
|
||||
- Hook tests prove editor open, pending cursor, and mounted-editor scroll use the same relative key.
|
||||
- File-tree tests prove absolute active and stored paths never reach child loads or refresh requests.
|
||||
- Backend process and handler tests prove validation happens before filesystem and lifecycle work and
|
||||
does not produce an ERROR log.
|
||||
- A desktop Playwright scenario observes the absolute workspace path from a real seeded tool card,
|
||||
the relative editor result, and all file-tree request payloads.
|
||||
- A phone Playwright scenario proves the same tool path and line target reach the existing native
|
||||
viewer without a composition change.
|
||||
- Frontend typecheck/lint, backend test/lint, specification validators, and `git diff --check` cover
|
||||
the combined package.
|
||||
|
||||
## Risks
|
||||
|
||||
- String-prefix containment can misclassify `/workspace-old` as inside `/workspace`. The shared
|
||||
helper must enforce a complete path segment and test the collision.
|
||||
- Normalizing only the editor open call while keeping the cursor key absolute would break
|
||||
scroll-to-line. Normalize before both operations.
|
||||
- Rejecting all absolute paths in the file editor would regress ADR 0016. Tree admission and file
|
||||
content admission must remain separate.
|
||||
- Filtering reveal alone leaves stale expansion entries available to generic refresh. Restore and
|
||||
refresh need their own guards.
|
||||
- Backend validation only in the WebSocket handler leaves direct agentctl HTTP callers exposed.
|
||||
Validate again at `WorkspaceTracker.GetFileTree`.
|
||||
- Windows drive, UNC, and Windows file URI paths need case-insensitive root containment, while POSIX
|
||||
paths remain case-sensitive.
|
||||
- URI detection must distinguish absolute URI forms from repository-relative names containing
|
||||
literal colons, including `config:dev`.
|
||||
|
||||
## Package handoff
|
||||
|
||||
Implementation follows the two sequential TDD work orders. Begin each work order with its named
|
||||
failing regression, keep the requirement and design draft until delivery is verified, and record
|
||||
results in the work orders and this plan before creating the PR.
|
||||
|
||||
## Results
|
||||
|
||||
Implemented both work orders. Tool-originated workspace aliases now become canonical relative
|
||||
editor identities before cursor state is written. Reveal, restore, refresh, the WebSocket handler,
|
||||
and agentctl all reject invalid tree identities before issuing or executing a tree request.
|
||||
|
||||
PR review aligned UNC and Windows file URI containment with existing LSP case semantics, preserved
|
||||
literal colons in canonical relative tree paths, shared the E2E workspace polling helper, and made
|
||||
the agentctl regression assert the exported validation sentinel.
|
||||
|
||||
Validation completed with 90 focused frontend tests, frontend typecheck and changed-file lint,
|
||||
focused backend tree and handler tests, backend lint, and one desktop plus one phone Playwright
|
||||
scenario. The broad process package also exposed two unchanged process-runner fixture timing tests
|
||||
that fail on this host; the affected `TestGetFileTree` tests pass independently.
|
||||
|
||||
Post-review verification passed the six-file focused frontend suite with 78 tests, typecheck,
|
||||
changed-file ESLint, the workspace-path and handler Go tests, backend lint, all specification
|
||||
validators, and the focused desktop and phone Playwright scenarios. The real colon-directory tree
|
||||
fixture runs on POSIX because Windows does not permit colons in path components; pure validation
|
||||
coverage remains cross-platform.
|
||||
@@ -0,0 +1,145 @@
|
||||
---
|
||||
id: "01-normalize-tool-file-paths"
|
||||
title: "Normalize tool file paths"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: []
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-UI-FILE-TREE-PATH-SCOPE-001
|
||||
acceptance_criteria:
|
||||
- AC-UI-FILE-TREE-PATH-SCOPE-001.1
|
||||
- AC-UI-FILE-TREE-PATH-SCOPE-001.2
|
||||
- AC-UI-FILE-TREE-PATH-SCOPE-001.6
|
||||
system_design:
|
||||
- ../../specs/ui/system-design/file-tree-path-scope.md
|
||||
---
|
||||
|
||||
# Task 01: Normalize Tool File Paths
|
||||
|
||||
## Summary
|
||||
|
||||
Give tool-originated workspace files one relative identity before editor, pending-cursor, and scroll
|
||||
state is written, while leaving external absolute content reads unchanged.
|
||||
|
||||
## In scope
|
||||
|
||||
- Add the shared frontend workspace-path classifier and normalizer.
|
||||
- Handle POSIX and Windows roots, separator normalization, drive, UNC, and Windows file URI casing,
|
||||
and complete-segment containment.
|
||||
- Normalize read and edit tool opens inside `useOpenFileAtLine` before every editor and cursor call.
|
||||
- Use the same containment decision for the edit-card open affordance instead of raw string prefix
|
||||
matching.
|
||||
- Reuse the helper in the LSP opener where it replaces equivalent local path logic without changing
|
||||
LSP behavior.
|
||||
- Add desktop and phone end-to-end coverage for an agent read card containing the active workspace's
|
||||
absolute file path and a line target.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- File-tree expansion, restore, refresh, or backend validation from Task 02.
|
||||
- Blocking an external absolute path from the read-only content endpoint.
|
||||
- New editor or phone-viewer UI.
|
||||
|
||||
## Acceptance
|
||||
|
||||
- A contained absolute tool path opens, keys its pending cursor, and scrolls as the corresponding
|
||||
workspace-relative path.
|
||||
- A path outside the workspace or sharing only its string prefix remains absolute and does not gain
|
||||
workspace identity.
|
||||
- Desktop and phone open the same relative workspace file at the requested line through their
|
||||
existing editor/viewer surfaces.
|
||||
|
||||
## TDD start
|
||||
|
||||
First add a `useOpenFileAtLine` regression that passes `/workspace/src/app.ts` with workspace root
|
||||
`/workspace` and expects `src/app.ts` in `setPendingCursorPosition`, `onOpenFile`, and
|
||||
`scrollEditorIfMounted`. It fails today because all three receive the absolute path. Add the prefix
|
||||
collision and external absolute cases before changing production code.
|
||||
|
||||
## ASCII UI preview
|
||||
|
||||
Relevant excerpt from [UI-01](plan.md#ui-01-open-a-workspace-file-from-an-agent-tool-card):
|
||||
|
||||
```text
|
||||
Desktop: Read /workspace/public/.../case.json -> Editor public/.../case.json
|
||||
Phone: Read /workspace/public/.../case.json -> native viewer public/.../case.json
|
||||
External /opt/reference.md -> existing read-only viewer; Files unchanged
|
||||
```
|
||||
|
||||
The rendered components and navigation remain unchanged. Only the path identity crossing the
|
||||
existing open action changes.
|
||||
|
||||
## Verification
|
||||
|
||||
Run from the repository root:
|
||||
|
||||
```bash
|
||||
(cd apps && pnpm install --frozen-lockfile)
|
||||
(cd apps/web && pnpm exec vitest run lib/workspace-file-path.test.ts \
|
||||
hooks/use-file-editors.test.tsx components/task/chat/messages/tool-edit-message.test.tsx \
|
||||
hooks/use-lsp-file-opener.test.ts)
|
||||
(cd apps/web && pnpm run typecheck)
|
||||
(cd apps/web && pnpm exec eslint lib/workspace-file-path.ts hooks/use-file-editors.ts \
|
||||
components/task/chat/messages/tool-edit-message.tsx hooks/use-lsp-file-opener.ts \
|
||||
--max-warnings 0)
|
||||
(cd apps/web && pnpm e2e:run --project chromium \
|
||||
tests/task/chat-read-absolute-workspace-path.spec.ts)
|
||||
(cd apps/web && pnpm e2e:run --project mobile-chrome \
|
||||
tests/task/mobile-file-viewer.spec.ts --grep "absolute workspace read link")
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/web/lib/workspace-file-path.ts`
|
||||
- `apps/web/lib/workspace-file-path.test.ts`
|
||||
- `apps/web/hooks/use-file-editors.ts`
|
||||
- `apps/web/hooks/use-file-editors.test.tsx`
|
||||
- `apps/web/hooks/use-lsp-file-opener.ts`
|
||||
- `apps/web/hooks/use-lsp-file-opener.test.ts`
|
||||
- `apps/web/components/task/chat/messages/tool-edit-message.tsx`
|
||||
- `apps/web/components/task/chat/messages/tool-edit-message.test.tsx`
|
||||
- `apps/web/e2e/tests/task/chat-read-absolute-workspace-path.spec.ts`
|
||||
- `apps/web/e2e/tests/task/mobile-file-viewer.spec.ts`
|
||||
|
||||
## Dependencies
|
||||
|
||||
None.
|
||||
|
||||
## Risks
|
||||
|
||||
- The pending cursor map must use the normalized identity before the editor mounts.
|
||||
- Tool edit cards currently use a permissive prefix test; changing it must preserve their existing
|
||||
copy-path fallback for external files.
|
||||
- `file:///` roots used by Monaco tests need canonical handling without treating an arbitrary URI as
|
||||
a tree path.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- `AC-UI-FILE-TREE-PATH-SCOPE-001.1`, `.2`, and `.6`.
|
||||
- Path identity and tool-file open flow in the system design.
|
||||
- ADR 0016's external absolute content-read exception.
|
||||
|
||||
## Results
|
||||
|
||||
Implemented the shared path classifier and normalized workspace-contained read and edit paths before
|
||||
editor, pending-cursor, and scroll state is written. The edit-card containment check and LSP alias
|
||||
conversion now use the same segment-aware helper. External absolute paths remain unchanged.
|
||||
|
||||
PR review extended the Windows comparison rule to UNC paths, drive file URIs, and hosted file URIs,
|
||||
matching the existing LSP identity semantics. The duplicated desktop and phone workspace polling
|
||||
helper now lives in the shared E2E session helper.
|
||||
|
||||
Validation passed:
|
||||
|
||||
- Four focused unit files passed with 56 tests.
|
||||
- Frontend typecheck and changed-file ESLint passed.
|
||||
- The Chromium desktop regression passed with one test and observed only relative tree requests.
|
||||
- The Pixel 5 phone regression passed with one test and reached the requested line in the existing
|
||||
native viewer.
|
||||
- Post-review verification passed the six-file focused frontend suite with 78 tests, frontend
|
||||
typecheck and changed-file ESLint, and both focused Playwright scenarios.
|
||||
@@ -0,0 +1,172 @@
|
||||
---
|
||||
id: "02-enforce-tree-request-scope"
|
||||
title: "Enforce tree request scope"
|
||||
status: done
|
||||
wave: 2
|
||||
depends_on:
|
||||
- "01-normalize-tool-file-paths"
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-UI-FILE-TREE-PATH-SCOPE-001
|
||||
acceptance_criteria:
|
||||
- AC-UI-FILE-TREE-PATH-SCOPE-001.2
|
||||
- AC-UI-FILE-TREE-PATH-SCOPE-001.3
|
||||
- AC-UI-FILE-TREE-PATH-SCOPE-001.4
|
||||
- AC-UI-FILE-TREE-PATH-SCOPE-001.5
|
||||
system_design:
|
||||
- ../../specs/ui/system-design/file-tree-path-scope.md
|
||||
---
|
||||
|
||||
# Task 02: Enforce Tree Request Scope
|
||||
|
||||
## Summary
|
||||
|
||||
Admit only workspace-relative identities to reveal, stored expansion, and refresh requests, then
|
||||
enforce the same tree contract before backend execution lookup and filesystem access.
|
||||
|
||||
## In scope
|
||||
|
||||
- Return early from active-file reveal for absolute, absolute-URI, or traversing paths while
|
||||
preserving literal colons in canonical relative names.
|
||||
- Sanitize restored expansion entries before deriving ancestors and retain valid siblings.
|
||||
- Filter refresh fan-out and specific change paths before calling `requestFileTree`.
|
||||
- Reject invalid tree paths in the WebSocket handler as validation failures without acquiring an
|
||||
execution or writing an ERROR log.
|
||||
- Reject invalid tree paths in `WorkspaceTracker.GetFileTree` before join, stat, or filesystem
|
||||
failure telemetry.
|
||||
- Preserve root, valid nested directory, symlink, missing relative path, and subtree merge behavior.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Editor path normalization delivered by Task 01.
|
||||
- Changing file-content path rules.
|
||||
- Suppressing logs for genuine backend dependency or relative filesystem failures.
|
||||
|
||||
## Acceptance
|
||||
|
||||
- The reported `/home/jcfs/...` active path never adds absolute ancestors, loads a tree node, or
|
||||
reaches `requestFileTree`, including after a generic refresh.
|
||||
- Stored invalid entries are removed while valid relative expansion restores and persists normally.
|
||||
- Direct absolute or parent-escaping tree requests fail validation before lifecycle or filesystem
|
||||
work and produce no ERROR log.
|
||||
|
||||
## TDD start
|
||||
|
||||
Add these failing regressions before production changes:
|
||||
|
||||
1. `file-tree-reveal.test.ts` supplies the reported absolute active file and expects no expansion or
|
||||
child load.
|
||||
2. `file-browser-restore-expanded.test.tsx` stores valid relative paths beside the absolute chain and
|
||||
expects only the valid paths to be requested and retained.
|
||||
3. `file-browser-apply-changes.test.ts` sends a generic refresh with absolute expanded paths and
|
||||
expects only `""` plus valid relative folders.
|
||||
4. `workspace_files_test.go` calls `GetFileTree` with an existing absolute directory and expects a
|
||||
validation error rather than a joined lookup.
|
||||
5. `workspace_file_handlers_test.go` supplies an absolute tree path with no lifecycle and expects
|
||||
`ErrorCodeValidation` and zero ERROR entries.
|
||||
|
||||
## ASCII UI preview
|
||||
|
||||
Relevant state transition from [UI-01](plan.md#ui-01-open-a-workspace-file-from-an-agent-tool-card):
|
||||
|
||||
```text
|
||||
Workspace file active: public/.../case.json -> Files reveals public > ... > case.json
|
||||
External file active: /opt/reference.md -> Files keeps its current expansion
|
||||
Stored mixed state: src, src/ui, /home/... -> restore src and src/ui only
|
||||
```
|
||||
|
||||
No rendered layout or control changes. Desktop and phone consume the same sanitized Files-tree
|
||||
state.
|
||||
|
||||
## Verification
|
||||
|
||||
Run from the repository root:
|
||||
|
||||
```bash
|
||||
(cd apps/web && pnpm exec vitest run components/task/file-tree-reveal.test.ts \
|
||||
components/task/file-browser-restore-expanded.test.tsx \
|
||||
components/task/file-browser-restore-loader.test.tsx \
|
||||
components/task/file-browser-apply-changes.test.ts)
|
||||
(cd apps/web && pnpm run typecheck)
|
||||
(cd apps/web && pnpm exec eslint components/task/file-tree-reveal.ts \
|
||||
components/task/file-browser-restore.ts components/task/file-browser-hooks.ts \
|
||||
--max-warnings 0)
|
||||
(cd apps/backend && go test ./internal/agentctl/server/process \
|
||||
-run 'TestGetFileTree' -count=1)
|
||||
(cd apps/backend && go test ./internal/agent/handlers \
|
||||
-run 'TestWorkspaceFileHandlers.*Tree' -count=1)
|
||||
(cd apps/backend && go test ./internal/agentctl/server/process \
|
||||
./internal/agent/handlers -count=1)
|
||||
make -C apps/backend lint
|
||||
```
|
||||
|
||||
After both work orders:
|
||||
|
||||
```bash
|
||||
python3 scripts/list-docs.py validate
|
||||
python3 scripts/lint-spec-files.test.py
|
||||
python3 scripts/lint-spec-files.py --all
|
||||
git diff --check
|
||||
git status --short
|
||||
```
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/web/components/task/file-tree-reveal.ts`
|
||||
- `apps/web/components/task/file-tree-reveal.test.ts`
|
||||
- `apps/web/components/task/file-browser-restore.ts`
|
||||
- `apps/web/components/task/file-browser-restore-expanded.test.tsx`
|
||||
- `apps/web/components/task/file-browser-restore-loader.test.tsx`
|
||||
- `apps/web/components/task/file-browser-hooks.ts`
|
||||
- `apps/web/components/task/file-browser-apply-changes.test.ts`
|
||||
- `apps/backend/internal/agent/handlers/workspace_file_handlers.go`
|
||||
- `apps/backend/internal/agent/handlers/workspace_file_handlers_test.go`
|
||||
- `apps/backend/internal/agentctl/server/process/workspace_files.go`
|
||||
- `apps/backend/internal/agentctl/server/process/workspace_files_test.go`
|
||||
|
||||
## Dependencies
|
||||
|
||||
- Task 01 supplies the shared frontend path classifier and prevents new tool opens from contaminating
|
||||
expansion state.
|
||||
|
||||
## Risks
|
||||
|
||||
- The empty root path is valid and must not be filtered with invalid empty segments.
|
||||
- Hidden directories beginning with `.` are valid; only standalone `.` and `..` segments are
|
||||
rejected.
|
||||
- A repository-scoped refresh must keep its current sibling-subtree preservation behavior.
|
||||
- The handler guard is for invalid client input. Dependency failures after a valid request must keep
|
||||
their operational error signal.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- `AC-UI-FILE-TREE-PATH-SCOPE-001.2` through `.5`.
|
||||
- Tree-state admission, refresh flow, and backend validation in the system design.
|
||||
- Existing persisted-expansion and cross-repository refresh regressions.
|
||||
|
||||
## Results
|
||||
|
||||
Implemented canonical tree-path admission for active-file reveal, stored expansion restoration,
|
||||
generic refresh fan-out, and specific file-change refreshes. A shared Go validator now rejects the
|
||||
same invalid identities before WebSocket execution lookup and before agentctl joins or stats a
|
||||
filesystem path. Validation failures produce no operational ERROR log.
|
||||
|
||||
PR review narrowed URI detection to absolute URI forms so repository-relative names such as
|
||||
`config:dev` remain valid in both frontend and backend validation. The agentctl regression now
|
||||
asserts the exported validation sentinel with `errors.Is`.
|
||||
|
||||
Validation passed:
|
||||
|
||||
- The combined focused frontend suite passed 90 tests across eight files.
|
||||
- `go test ./internal/common/workspacepath -count=1` passed.
|
||||
- Focused `TestGetFileTree` and `TestWorkspaceFileHandlers.*Tree` runs passed.
|
||||
- The complete handler package passed, and `make -C apps/backend lint` reported zero issues.
|
||||
- Post-review coverage passed a real frontend refresh and POSIX agentctl tree read for `config:dev`,
|
||||
plus cross-platform validator cases, the focused Go packages, and backend lint.
|
||||
- The full process package reached two unchanged runner-fixture timing failures on this host:
|
||||
`TestProcessRunnerCapturesOutput` and `TestProcessRunnerStopLogsSignalAttempts`. The branch has no
|
||||
changes to those tests or their production code; both fail identically when run alone.
|
||||
@@ -0,0 +1,96 @@
|
||||
---
|
||||
status: active
|
||||
system: ui
|
||||
created: 2026-09-21
|
||||
owners:
|
||||
- kandev
|
||||
---
|
||||
|
||||
# File Tree Path Scope Requirements
|
||||
|
||||
## Overview
|
||||
|
||||
Agent tool messages can identify a file by its absolute host path, while the task Files tree and
|
||||
`workspace.tree.get` address nodes relative to the task workspace root. Users must be able to open
|
||||
an agent-referenced workspace file without that absolute identity entering file-tree expansion,
|
||||
persistence, or refresh state and producing repeated failed tree requests.
|
||||
|
||||
The UI system owns this contract because it owns file-open identity, Files-panel expansion state,
|
||||
and the requests derived from that state. The workspace file service remains the final validation
|
||||
boundary for tree reads. Its separate read-only absolute-file content contract remains unchanged.
|
||||
|
||||
## Terminology
|
||||
|
||||
- **Workspace file path:** A normalized path relative to the active task workspace root, using `/`
|
||||
separators and containing no parent traversal. Literal colons that the task host can represent
|
||||
remain valid path characters unless the value has an absolute URI or Windows drive form.
|
||||
- **Workspace absolute alias:** An absolute path contained beneath the active task workspace root
|
||||
on a complete path-segment boundary.
|
||||
- **External absolute path:** An absolute path outside the active task workspace. It can be opened
|
||||
read-only under ADR 0016, but it has no node identity in the task Files tree.
|
||||
- **Tree request path:** The `path` supplied to `workspace.tree.get`. The empty string addresses the
|
||||
root; every other value is a workspace file path for a directory.
|
||||
|
||||
## Requirements
|
||||
|
||||
### REQ-UI-FILE-TREE-PATH-SCOPE-001: Keep file-tree paths workspace-relative
|
||||
|
||||
**Intent:** Opening files referenced by an agent must not corrupt file-tree state or cause repeated
|
||||
backend errors.
|
||||
|
||||
**User story:** As a user inspecting an agent-referenced file, I want the file and Files tree to use
|
||||
the active task workspace consistently, so that opening or refreshing the file does not issue
|
||||
invalid directory requests.
|
||||
|
||||
#### Acceptance criteria
|
||||
|
||||
- **AC-UI-FILE-TREE-PATH-SCOPE-001.1:** When an agent read or edit message identifies a file by a
|
||||
workspace absolute alias, activating that file shall open the corresponding workspace file path
|
||||
and preserve any requested line target.
|
||||
- **AC-UI-FILE-TREE-PATH-SCOPE-001.2:** When the active editor displays an external absolute path,
|
||||
the Files tree shall leave its expansion state unchanged and shall not attempt to reveal, restore,
|
||||
or refresh that path. The existing read-only external file content behavior shall remain
|
||||
available.
|
||||
- **AC-UI-FILE-TREE-PATH-SCOPE-001.3:** Every tree request derived from active-file reveal,
|
||||
persisted expansion, or a workspace refresh shall use either the empty root path or a normalized
|
||||
workspace file path. Absolute filesystem paths, absolute URI forms, Windows drive prefixes, and
|
||||
paths containing parent traversal shall not cross the frontend tree-request boundary. A literal
|
||||
colon in an otherwise canonical relative path shall remain valid.
|
||||
- **AC-UI-FILE-TREE-PATH-SCOPE-001.4:** When stored expansion state contains an invalid tree path,
|
||||
the Files tree shall discard that path and its derived ancestors before hydration or refresh,
|
||||
while retaining valid expanded paths for the same task environment.
|
||||
- **AC-UI-FILE-TREE-PATH-SCOPE-001.5:** The backend tree endpoint shall reject an absolute or
|
||||
escaping request path as a validation failure before filesystem lookup. A rejected client path
|
||||
shall not be reported as an operational ERROR log.
|
||||
- **AC-UI-FILE-TREE-PATH-SCOPE-001.6:** Desktop and phone shall keep their existing editor and file
|
||||
viewer compositions. A workspace absolute alias shall display and reopen with its workspace file
|
||||
path on both, with no new control or navigation step.
|
||||
|
||||
## Scenarios
|
||||
|
||||
- **GIVEN** the task root `/workspace` and an agent read of
|
||||
`/workspace/public/assets/case.json`, **WHEN** the user activates the read card, **THEN** the editor
|
||||
opens `public/assets/case.json` and the Files tree may reveal only that relative path.
|
||||
- **GIVEN** a read-only editor for `/opt/reference/guide.md`, **WHEN** the Files panel is mounted or a
|
||||
refresh event arrives, **THEN** the editor remains usable and the Files tree does not expand or
|
||||
request `/opt` or any descendant.
|
||||
- **GIVEN** stored expansion entries `src`, `src/components`, and `/home/user/project`, **WHEN** the
|
||||
Files tree restores, **THEN** it restores the two valid entries and removes the absolute entry
|
||||
without requesting it.
|
||||
- **GIVEN** the relative directory `config:dev` in a POSIX task workspace, **WHEN** the user expands
|
||||
or refreshes it, **THEN** the Files tree requests `config:dev` without treating the literal colon
|
||||
as a URI scheme.
|
||||
- **GIVEN** a direct `workspace.tree.get` request with `/home/user/project`, **WHEN** the backend
|
||||
validates it, **THEN** it returns a validation response without constructing
|
||||
`<workspace-root>/home/user/project`, calling `stat`, or emitting an ERROR log.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Removing the read-only absolute-file content behavior accepted by ADR 0016.
|
||||
- Mapping an external host directory into the task Files tree.
|
||||
- Changing file-tree layout, row styling, editor chrome, phone navigation, or localized copy.
|
||||
- Changing workspace source aliases used by Markdown links.
|
||||
|
||||
## Implementation plan
|
||||
|
||||
- [File tree path scope](../../../plans/file-tree-path-scope/plan.md)
|
||||
@@ -0,0 +1,192 @@
|
||||
---
|
||||
status: current
|
||||
system: ui
|
||||
requirements:
|
||||
- REQ-UI-FILE-TREE-PATH-SCOPE-001
|
||||
---
|
||||
|
||||
# File Tree Path Scope System Design
|
||||
|
||||
## Purpose and boundaries
|
||||
|
||||
The UI owns the path identity passed from agent tool cards into file editors and the relative
|
||||
directory identities retained by the task Files tree. The agentctl workspace file service owns the
|
||||
filesystem boundary and provides defense in depth for direct tree requests.
|
||||
|
||||
This design distinguishes two existing contracts. File content reads may use an external absolute
|
||||
path under [ADR 0016](../../../decisions/0016-observed-external-file-reads.md). File-tree reads are
|
||||
directory enumeration inside the task workspace and accept only workspace-relative paths. An
|
||||
absolute editor identity therefore never implies an absolute tree identity.
|
||||
|
||||
## Requirement mapping
|
||||
|
||||
| Acceptance criterion | Design sections |
|
||||
| ---------------------------------- | -------------------------------------------------------------------------------------------- |
|
||||
| `AC-UI-FILE-TREE-PATH-SCOPE-001.1` | [Path identities](#path-identities), [Tool-file open flow](#tool-file-open-flow) |
|
||||
| `AC-UI-FILE-TREE-PATH-SCOPE-001.2` | [Tree-state admission](#tree-state-admission), [Failure and recovery](#failure-and-recovery) |
|
||||
| `AC-UI-FILE-TREE-PATH-SCOPE-001.3` | [Tree-state admission](#tree-state-admission), [Refresh flow](#refresh-flow) |
|
||||
| `AC-UI-FILE-TREE-PATH-SCOPE-001.4` | [Persistence](#persistence) |
|
||||
| `AC-UI-FILE-TREE-PATH-SCOPE-001.5` | [Backend validation](#backend-validation), [Observability](#observability) |
|
||||
| `AC-UI-FILE-TREE-PATH-SCOPE-001.6` | [Responsive behavior](#responsive-behavior), [Verification](#verification) |
|
||||
|
||||
## Components and responsibilities
|
||||
|
||||
- A shared pure frontend path helper classifies POSIX and Windows absolute paths, performs
|
||||
segment-boundary containment against the active task workspace root, converts contained aliases
|
||||
to slash-separated relative paths, and validates paths admitted to tree state.
|
||||
- `useOpenFileAtLine` in `apps/web/hooks/use-file-editors.ts` normalizes tool-originated paths before
|
||||
it keys pending cursor state, calls the editor opener, or scrolls an existing editor. Tool read and
|
||||
edit cards therefore use one path identity for file content, tabs, cursor state, and the Files
|
||||
panel.
|
||||
- `useFileTreeReveal` admits only valid workspace-relative active paths before calculating ancestors
|
||||
or loading children.
|
||||
- `restoredExpandedPaths` validates stored entries before synthesizing their ancestor chains.
|
||||
- `applyFileChanges` filters expanded paths through the same tree-path predicate before a generic
|
||||
refresh fans out `workspace.tree.get` requests.
|
||||
- `WorkspaceFileHandlers.wsGetFileTree` rejects invalid client paths before execution lookup and
|
||||
returns a WebSocket validation error without logging an operational failure.
|
||||
- `WorkspaceTracker.GetFileTree` repeats the absolute and traversal check before joining the request
|
||||
to `workDir`, so direct agentctl HTTP callers cannot reach a filesystem lookup with an invalid
|
||||
tree path.
|
||||
|
||||
## Path identities
|
||||
|
||||
The frontend path helper returns one of these outcomes:
|
||||
|
||||
| Input | Workspace root | Open identity | Tree eligible |
|
||||
| --------------------------- | -------------- | ----------------------- | ------------- |
|
||||
| `src/app.ts` | `/workspace` | `src/app.ts` | yes |
|
||||
| `/workspace/src/app.ts` | `/workspace` | `src/app.ts` | yes |
|
||||
| `/workspace-old/src/app.ts` | `/workspace` | unchanged absolute path | no |
|
||||
| `/opt/reference.md` | `/workspace` | unchanged absolute path | no |
|
||||
| `../sibling/app.ts` | `/workspace` | unchanged invalid path | no |
|
||||
|
||||
Containment uses a complete path segment, normalizes `\` to `/`, and compares Windows drive, UNC,
|
||||
drive-file-URI, and hosted-file-URI paths case-insensitively. POSIX paths and hostless non-drive file
|
||||
URIs remain case-sensitive. A sibling whose prefix happens to share the workspace root string is not
|
||||
contained. The empty string is valid only as the tree root. A relative tree path rejects empty
|
||||
segments introduced by a leading slash, `.` and `..` segments, absolute URI forms, and Windows
|
||||
drive roots. Literal colons in otherwise canonical relative segments remain valid.
|
||||
The host filesystem can still reject names it cannot represent; the real `config:dev` tree fixture
|
||||
therefore runs on POSIX, while the pure frontend and Go validators cover the path contract on every
|
||||
platform.
|
||||
|
||||
The helper does not rewrite a legitimate external absolute file into a task-relative lookalike.
|
||||
That path continues to the read-only content endpoint unchanged and remains ineligible for the tree.
|
||||
|
||||
## Tool-file open flow
|
||||
|
||||
1. The normalized ACP read or edit message supplies its recorded file path and optional source line.
|
||||
2. The tool renderer passes the path through `useOpenFileAtLine` with the active session workspace
|
||||
root.
|
||||
3. If the path is contained by that root, the hook derives the workspace-relative identity before
|
||||
writing pending cursor state and calling `onOpenFile`.
|
||||
4. The existing file editor requests content, creates or focuses the tab, and scrolls using the same
|
||||
normalized identity.
|
||||
5. `FilesPanel` observes the active relative editor identity and may reveal its valid ancestor chain.
|
||||
|
||||
An external absolute path follows the same file-content route without step 3's rewrite. The editor
|
||||
can display it, while the reveal boundary ignores it.
|
||||
|
||||
## Tree-state admission
|
||||
|
||||
Tree state uses one predicate at each source boundary rather than assuming every caller has already
|
||||
normalized its input:
|
||||
|
||||
- Active-file reveal returns before changing expansion state when the path is invalid.
|
||||
- Restored session storage drops invalid entries before deriving parents.
|
||||
- Generic refresh ignores invalid entries already present in memory and always retains the valid
|
||||
root refresh.
|
||||
- Specific file-change events continue to refresh the nearest valid expanded ancestor. Invalid
|
||||
change paths are ignored rather than converted into a host path.
|
||||
|
||||
The predicate applies to state and request identities, not ordinary filenames shown in labels.
|
||||
Hidden paths such as `.codex/agents` remain valid.
|
||||
|
||||
## Refresh flow
|
||||
|
||||
A generic file refresh starts with the root request `""`. For each valid expanded directory in the
|
||||
affected repository scope, `applyFileChanges` schedules one relative child request. Invalid entries
|
||||
do not enter the request set. Responses continue to merge with loaded descendants through the
|
||||
existing `mergeTreeNodes` behavior.
|
||||
|
||||
This filter is required even after tool-path normalization because an existing browser tab may hold
|
||||
state written by an older build or by another editor entry point.
|
||||
|
||||
## Backend validation
|
||||
|
||||
The WebSocket handler validates the tree path before it acquires an agentctl client. Invalid paths
|
||||
return `ErrorCodeValidation`; they are client contract failures and do not emit the
|
||||
`failed to get file tree` ERROR log.
|
||||
|
||||
The agentctl process validates again because its HTTP endpoint is independently callable. It rejects
|
||||
an absolute path or a cleaned parent escape before `filepath.Join`, `os.Stat`, or filesystem-failure
|
||||
telemetry. Valid relative paths retain the existing containment check, missing-path behavior, depth
|
||||
handling, symlink behavior, and tree construction.
|
||||
|
||||
## Failure and recovery
|
||||
|
||||
- An invalid active editor path has no tree side effect. The editor's own content result remains the
|
||||
only user-visible success or failure.
|
||||
- Invalid stored entries are pruned locally; valid siblings restore normally, and the cleaned set is
|
||||
written back through the existing session-storage effect after load.
|
||||
- A malformed direct WebSocket request receives a validation response. A direct agentctl HTTP
|
||||
request receives the existing 400 response with a bounded validation message.
|
||||
- A valid relative tree path that disappears can still return the existing missing-path error. That
|
||||
operational behavior is outside this repair.
|
||||
|
||||
## Persistence
|
||||
|
||||
No storage schema changes. Files-panel expansion remains scoped by task environment in
|
||||
`sessionStorage`. Restore sanitization is a forward cleanup for entries produced by older clients;
|
||||
the next normal persistence pass replaces the contaminated array with the accepted relative set.
|
||||
|
||||
Open external absolute file tabs retain their existing persistence and read-only reopen behavior.
|
||||
They are not copied into expansion storage.
|
||||
|
||||
## Security
|
||||
|
||||
Frontend validation limits accidental and stale requests but is not an authorization boundary. The
|
||||
agentctl process remains authoritative and rejects absolute or escaping tree enumeration. Symlink
|
||||
containment continues through existing workspace tracker behavior.
|
||||
|
||||
ADR 0016 remains limited to current-file content reads. This design does not expand absolute access
|
||||
to directory enumeration, mutation, search, Git-ref reads, or any other workspace operation.
|
||||
|
||||
## Responsive behavior
|
||||
|
||||
No component anatomy, styling, touch target, copy, focus path, or scroll owner changes. Desktop tool
|
||||
links continue to open or focus the Dockview editor. Phone tool links continue to switch to the
|
||||
native `MobileFileViewerPanel`. Both receive the same normalized workspace-relative identity and
|
||||
line target.
|
||||
|
||||
The phone regression uses the existing chat-read-to-CodeMirror flow as its nearest exemplar. A
|
||||
separate mobile layout implementation is unnecessary because normalization happens before the
|
||||
desktop and phone openers diverge.
|
||||
|
||||
## Verification
|
||||
|
||||
- Pure helper tests cover POSIX paths, Windows drive and UNC paths, segment-boundary prefix
|
||||
collisions, separator normalization, external absolutes, Windows and POSIX file URI casing,
|
||||
literal colons, and traversal.
|
||||
- `useOpenFileAtLine` tests first reproduce the defect by expecting `/workspace/src/app.ts` to open
|
||||
and scroll as `src/app.ts`; this assertion fails before the production change. They also prove an
|
||||
external absolute path remains unchanged.
|
||||
- Reveal, restore, and refresh tests seed the reported absolute expansion chain and assert no load or
|
||||
request receives `/home`, `/home/user`, or descendants while valid relative siblings remain.
|
||||
- Backend process tests assert invalid paths fail before lookup. Handler tests assert an absolute
|
||||
path returns `ErrorCodeValidation`, does not consult lifecycle, and produces no ERROR log.
|
||||
- Desktop and phone Playwright scenarios seed an agent read card with the active workspace's
|
||||
absolute file path, activate it, and assert the existing editor/viewer opens the relative path at
|
||||
the requested line. The desktop scenario also observes `workspace.tree.get` traffic and fails if
|
||||
any request carries an absolute path.
|
||||
|
||||
## Observability
|
||||
|
||||
No new metric or routine log is needed. Invalid direct tree requests are represented by their
|
||||
validation response. Genuine dependency or filesystem failures keep their current logs. The
|
||||
absence of absolute tree requests is enforced by frontend and backend regression tests.
|
||||
|
||||
## Related decisions
|
||||
|
||||
- [ADR 0016: Read-Only Absolute File Paths](../../../decisions/0016-observed-external-file-reads.md)
|
||||
Reference in New Issue
Block a user