From 3739653ca679aab463c7c84b4ee81938deaa4c9e Mon Sep 17 00:00:00 2001 From: kudala-bharani Date: Mon, 27 Jul 2026 00:38:38 -0400 Subject: [PATCH] Fix artifact previews across granted roots --- coworker/server/manager.py | 154 ++++++++++++------ surfaces/gui/src/api.ts | 2 +- surfaces/gui/src/artifactPaths.test.ts | 28 ++++ surfaces/gui/src/artifactPaths.ts | 36 ++++ surfaces/gui/src/components/Markdown.test.tsx | 12 ++ surfaces/gui/src/components/Markdown.tsx | 10 +- surfaces/gui/src/components/RightRail.tsx | 9 +- tests/test_multiroot.py | 19 +++ tests/test_server.py | 63 +++++++ 9 files changed, 276 insertions(+), 57 deletions(-) create mode 100644 surfaces/gui/src/artifactPaths.test.ts create mode 100644 surfaces/gui/src/artifactPaths.ts diff --git a/coworker/server/manager.py b/coworker/server/manager.py index ad76e9966d..91e6c73106 100644 --- a/coworker/server/manager.py +++ b/coworker/server/manager.py @@ -1225,15 +1225,51 @@ def browser_screenshot(self) -> dict[str, Any]: def browser_close(self) -> dict[str, Any]: return browser_close_session() + def _artifact_roots(self, session_id: str) -> list[tuple[Path, bool]]: + """Return the session's granted directories in resolution order. + + The live engine is authoritative because ``request_directory`` mutates its shared + roots list immediately. Without a live engine, reconstruct the same list from the + persisted conversation. Unknown sessions retain the historical standalone-server + behaviour of using ``default_workspace``. + """ + engine = self._engines.get(session_id) + if engine is not None and getattr(engine, "roots", None): + raw = [(Path(r.path), bool(r.writable)) for r in engine.roots] + else: + record = self.session_store.load(session_id) + if record and record.workspace: + raw = [(Path(record.workspace), True)] + raw.extend( + (Path(str(r.get("path", ""))), bool(r.get("writable", False))) + for r in (record.extra_roots or []) + if str(r.get("path", "")).strip() + ) + elif self.default_workspace: + raw = [(Path(self.default_workspace), True)] + else: + raw = [] + + out: list[tuple[Path, bool]] = [] + seen: set[Path] = set() + for path, writable in raw: + root = path.expanduser().resolve() + if root in seen: + continue + seen.add(root) + out.append((root, writable)) + return out + def list_artifacts(self, session_id: str) -> list[dict[str, Any]]: - record = self.session_store.load(session_id) - workspace = record.workspace if record else self.default_workspace - if not workspace: - return [] - root = Path(workspace).expanduser().resolve() - if not root.is_dir(): + roots = [ + root + for root, writable in self._artifact_roots(session_id) + if writable and root.is_dir() + ] + if not roots: return [] out: list[dict[str, Any]] = [] + seen_files: set[Path] = set() suffixes = { ".md", ".markdown", @@ -1270,33 +1306,40 @@ def list_artifacts(self, session_id: str) -> list[dict[str, Any]]: from ..tools.search import OS_DATA_DIRS skip = {"node_modules", "target", "dist", "__pycache__"} | OS_DATA_DIRS - for dirpath, dirs, files in os.walk(root): - dirs[:] = [d for d in dirs if not d.startswith(".") and d not in skip] - for name in files: - if name.startswith("."): - continue - path = Path(dirpath) / name - if path.suffix.lower() not in suffixes: - continue - try: - st = path.stat() - if not path.is_file(): + for index, root in enumerate(roots): + for dirpath, dirs, files in os.walk(root): + dirs[:] = [d for d in dirs if not d.startswith(".") and d not in skip] + for name in files: + if name.startswith("."): + continue + path = Path(dirpath) / name + if path.suffix.lower() not in suffixes: + continue + try: + resolved = path.resolve() + if not resolved.is_file() or not _path_is_under(resolved, root): + continue + if resolved in seen_files: + continue + seen_files.add(resolved) + st = resolved.stat() + rel = path.relative_to(root) + out.append( + { + # Preserve workspace-relative identifiers for the primary root. + # Extra-root artifacts use their absolute path so duplicate + # relative names remain unambiguous when the viewer calls + # read/reveal. + "path": str(rel) if index == 0 else str(resolved), + "abs_path": str(resolved), + "name": resolved.name, + "kind": _artifact_kind(resolved), + "size": st.st_size, + "modified_at": st.st_mtime, + } + ) + except (OSError, ValueError): continue - out.append( - { - "path": str(path.relative_to(root)), - # Absolute path for "Copy path" — the relative one is useless - # outside the app (tester catch 2026-07-12: it copied just the - # filename). - "abs_path": str(path), - "name": path.name, - "kind": _artifact_kind(path), - "size": st.st_size, - "modified_at": st.st_mtime, - } - ) - except OSError: - continue out.sort(key=lambda a: a["modified_at"], reverse=True) return out[:80] @@ -1305,25 +1348,32 @@ def list_artifacts(self, session_id: str) -> list[dict[str, Any]]: def _artifact_target( self, session_id: str, path: str, *, allow_dir: bool = False ) -> tuple[Optional[Path], Optional[str]]: - """Resolve an artifact path under the session's workspace, or (None, error).""" - record = self.session_store.load(session_id) - workspace = record.workspace if record else self.default_workspace - if not workspace: + """Resolve an artifact inside any directory granted to this session.""" + roots = [root for root, _writable in self._artifact_roots(session_id)] + if not roots: return None, "no workspace" - root = Path(workspace).expanduser().resolve() - target = (root / path).expanduser().resolve() - try: - target.relative_to(root) - except ValueError: - return None, "path escapes workspace" - if allow_dir and target.is_dir(): - return target, None - if not target.is_file(): - return None, ( + requested = Path(path).expanduser() + candidates = ( + [requested.resolve()] + if requested.is_absolute() + else [(root / requested).resolve() for root in roots] + ) + confined = False + for target in candidates: + if not any(_path_is_under(target, root) for root in roots): + continue + confined = True + if target.is_file() or (allow_dir and target.is_dir()): + return target, None + return ( + ( + None, "This isn't in the conversation's folder anymore — it may have been " - "moved or deleted." + "moved or deleted.", ) - return target, None + if confined + else (None, "path escapes session directories") + ) def read_artifact(self, session_id: str, path: str) -> dict[str, Any]: # Folders are readable too (a model sometimes links a whole package, e.g. a skill @@ -4041,6 +4091,14 @@ def _recent_files(workspace: str, *, since: float, limit: int = 20) -> list[str] return out +def _path_is_under(path: Path, root: Path) -> bool: + try: + path.relative_to(root) + return True + except ValueError: + return False + + def _artifact_kind(path: Path) -> str: suffix = path.suffix.lower() if suffix in {".md", ".markdown"}: diff --git a/surfaces/gui/src/api.ts b/surfaces/gui/src/api.ts index ad9debd57f..90898e0a1d 100644 --- a/surfaces/gui/src/api.ts +++ b/surfaces/gui/src/api.ts @@ -179,7 +179,7 @@ export async function deleteSession(sessionId: string): Promise<{ ok: boolean; e } export interface ArtifactInfo { - path: string; // workspace-relative (the display/API identifier) + path: string; // primary-root relative or absolute for an additional granted root abs_path?: string; // absolute — what "Copy path" copies name: string; kind: "markdown" | "html" | "image" | "code" | "text" | string; diff --git a/surfaces/gui/src/artifactPaths.test.ts b/surfaces/gui/src/artifactPaths.test.ts new file mode 100644 index 0000000000..a6b908e337 --- /dev/null +++ b/surfaces/gui/src/artifactPaths.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, it } from "vitest"; +import { artifactBaseName, decodeArtifactPath, findArtifact } from "./artifactPaths"; + +const external = { + path: "/Users/example/Documents/新闻摘要_2026-07-27.md", + abs_path: "/Users/example/Documents/新闻摘要_2026-07-27.md", + name: "新闻摘要_2026-07-27.md", +}; + +describe("artifact paths", () => { + it("decodes URL-encoded Unicode paths but preserves malformed literal percent names", () => { + expect(decodeArtifactPath("%E6%96%B0%E9%97%BB%E6%91%98%E8%A6%81.md")).toBe("新闻摘要.md"); + expect(decodeArtifactPath("progress-100%.md")).toBe("progress-100%.md"); + }); + + it("extracts names with POSIX or Windows separators", () => { + expect(artifactBaseName("reports/summary.md")).toBe("summary.md"); + expect(artifactBaseName("C:\\Reports\\summary.md")).toBe("summary.md"); + }); + + it("matches encoded bare names and relative paths against external artifacts", () => { + expect( + findArtifact([external], "%E6%96%B0%E9%97%BB%E6%91%98%E8%A6%81_2026-07-27.md"), + ).toBe(external); + expect(findArtifact([external], "Documents/新闻摘要_2026-07-27.md")).toBe(external); + expect(findArtifact([external], external.path)).toBe(external); + }); +}); diff --git a/surfaces/gui/src/artifactPaths.ts b/surfaces/gui/src/artifactPaths.ts new file mode 100644 index 0000000000..19310bedfb --- /dev/null +++ b/surfaces/gui/src/artifactPaths.ts @@ -0,0 +1,36 @@ +type ArtifactPath = { path: string; abs_path?: string; name: string }; + +export function decodeArtifactPath(path: string): string { + try { + return decodeURIComponent(path); + } catch { + // A literal percent in a filename is valid even when it is not URL encoding. + return path; + } +} + +export function normalizeArtifactPath(path: string): string { + return decodeArtifactPath(path).replace(/\\/g, "/").replace(/^\.\/+/, ""); +} + +export function artifactBaseName(path: string): string { + const normalized = normalizeArtifactPath(path); + return normalized.split("/").pop() || normalized; +} + +export function findArtifact(list: T[], path: string): T | undefined { + const requested = normalizeArtifactPath(path); + const bareName = !requested.includes("/"); + return list.find((artifact) => { + const candidates = [artifact.path, artifact.abs_path].filter(Boolean) as string[]; + if ( + candidates.some((candidate) => { + const normalized = normalizeArtifactPath(candidate); + return normalized === requested || normalized.endsWith("/" + requested); + }) + ) { + return true; + } + return bareName && normalizeArtifactPath(artifact.name) === requested; + }); +} diff --git a/surfaces/gui/src/components/Markdown.test.tsx b/surfaces/gui/src/components/Markdown.test.tsx index b2d4fee0a1..171c741773 100644 --- a/surfaces/gui/src/components/Markdown.test.tsx +++ b/surfaces/gui/src/components/Markdown.test.tsx @@ -35,4 +35,16 @@ describe("Markdown artifact links", () => { render(); expect(screen.getByTestId("artifact-chip").textContent).toContain("report.pdf"); }); + + it("dispatches a decoded Unicode filename", () => { + const seen: string[] = []; + const listener = (e: Event) => seen.push((e as CustomEvent).detail.path); + window.addEventListener(OPEN_ARTIFACT_EVENT, listener); + + render(); + fireEvent.click(screen.getByTestId("artifact-chip")); + expect(seen).toEqual(["新闻摘要.md"]); + + window.removeEventListener(OPEN_ARTIFACT_EVENT, listener); + }); }); diff --git a/surfaces/gui/src/components/Markdown.tsx b/surfaces/gui/src/components/Markdown.tsx index 3367042784..a8bb22a9b6 100644 --- a/surfaces/gui/src/components/Markdown.tsx +++ b/surfaces/gui/src/components/Markdown.tsx @@ -1,5 +1,6 @@ import ReactMarkdown, { defaultUrlTransform } from "react-markdown"; import remarkGfm from "remark-gfm"; +import { artifactBaseName, decodeArtifactPath } from "../artifactPaths"; import { Icon } from "./Icon"; // §34 (UX-016): the agent ends a deliverable turn with plain markdown — @@ -10,14 +11,17 @@ import { Icon } from "./Icon"; export const OPEN_ARTIFACT_EVENT = "ocw-open-artifact"; function ArtifactChip({ path, title }: { path: string; title: string }) { - const file = path.split("/").pop() || path; + const decodedPath = decodeArtifactPath(path); + const file = artifactBaseName(decodedPath); return (