diff --git a/coworker/catalog.py b/coworker/catalog.py index e2ce78a7..feb9d083 100644 --- a/coworker/catalog.py +++ b/coworker/catalog.py @@ -53,17 +53,22 @@ class Capability: def _code_files(context: AgentContext) -> list: - """Repo-oriented files: single-root, line-numbered/windowed `read_file`. Our `grep` and - windowed `read_file` replace aisuite's slower `search_files` / `read_file`/`read_file_lines`. + """Repo-oriented files: line-numbered/windowed `read_file`. Our `grep` and windowed + `read_file` replace aisuite's slower `search_files` / `read_file`/`read_file_lines`. + Multi-root aware (universal scratch): with session roots, writes/reads reach the + scratch and granted dirs too; the workspace stays the relative-path anchor. """ ws = str(context.workspace) replaced = {"search_files", "read_file", "read_file_lines"} + file_kwargs = ( + {"roots": context.roots} if context.roots else {"root": ws, "allow_write": True} + ) files = [ t - for t in ai.toolkits.files(root=ws, allow_write=True) + for t in ai.toolkits.files(**file_kwargs) if getattr(t, "__name__", "") not in replaced ] - return [*files, *file_tools(ws)] + return [*files, *file_tools(ws, roots=context.roots)] def _files(context: AgentContext) -> list: diff --git a/coworker/roots.py b/coworker/roots.py index 54fd52a7..6e155a0f 100644 --- a/coworker/roots.py +++ b/coworker/roots.py @@ -63,13 +63,33 @@ def render_context(roots: list[RootDir]) -> str: if not roots: return "" lines = ["Available directories (you may use file/shell tools within these):"] + has_side_scratch = any(i > 0 and r.label == "scratch" for i, r in enumerate(roots)) for i, r in enumerate(roots): access = "read-write" if r.writable else "read-only" - tag = " — primary scratch, the default place to save files" if i == 0 else "" + if i == 0 and r.label == "scratch": + tag = " — primary scratch, the default place to save files" + elif i == 0: + tag = " — the session's workspace (relative paths resolve here)" + elif r.label == "scratch": + tag = ( + " — your scratch directory: temporary files, and artifacts you don't " + "want to leave inside the workspace" + ) + else: + tag = "" lines.append(f"- {r.path} [{access}]{tag}") - lines.append( - "Relative paths resolve against the primary directory; pass an absolute path to use " - "another directory. Writes are only allowed in read-write directories. If the user " - "cares where a deliverable lands, ask; otherwise save it in the primary scratch." - ) + if has_side_scratch: + lines.append( + "Relative paths resolve against the workspace; pass an absolute path to use " + "another directory. Writes are only allowed in read-write directories. Put " + "reports, analyses, and other non-repo deliverables in the scratch directory " + "(they appear in the user's Artifacts panel) — write into the workspace only " + "for changes that belong in it." + ) + else: + lines.append( + "Relative paths resolve against the primary directory; pass an absolute path to use " + "another directory. Writes are only allowed in read-write directories. If the user " + "cares where a deliverable lands, ask; otherwise save it in the primary scratch." + ) return "\n".join(lines) diff --git a/coworker/server/manager.py b/coworker/server/manager.py index 76322867..4ed7ff83 100644 --- a/coworker/server/manager.py +++ b/coworker/server/manager.py @@ -386,8 +386,14 @@ class SessionManager: DEFAULT_SCRATCH_BASE = "~/OpenWorker" def scratch_base(self) -> Path: - """Common area for per-conversation scratch directories. Configurable via prefs.""" - base = self._prefs.get("scratch_base") or self.DEFAULT_SCRATCH_BASE + """Common area for per-conversation scratch directories. Configurable via prefs; + the env override keeps tests (and any sandboxed run) out of the real home dir — + universal scratch means every session provisions here, not just orphan ones.""" + base = ( + self._prefs.get("scratch_base") + or os.environ.get("COWORKER_SCRATCH_BASE") + or self.DEFAULT_SCRATCH_BASE + ) return Path(base).expanduser() def _provision_scratch(self, session_id: str) -> str: @@ -539,17 +545,35 @@ class SessionManager: if ws: self.session_store.touch_workspace(ws) - # Orphan surfaces are multi-root: the scratch (ws) is the primary writable root, plus any - # folders the user added (persisted per session). Folder-gated personas stay single-root - # (roots=None) until universal scratch lands (workspace-scratch-design.md phase B). + # Universal scratch (workspace-scratch-design.md §4): EVERY session is multi-root + # with a per-conversation scratch dir. Orphan sessions run ON their scratch + # (ws == scratch, primary). Sessions on a real folder — gated personas, or a + # temp-workspace pick that later became a project — keep that folder primary and + # gain scratch as a second writable root, so deliverables/temp files have a home + # that never dirties the user's repo. request_directory rides on roots, so it now + # registers everywhere. roots = None - if not ag.requires_folder and ws: + if ws: extra = [ r for r in ((record.extra_roots if record else []) or []) if Path(str(r.get("path", ""))).is_dir() ] - roots = [{"path": ws, "writable": True, "label": "scratch"}, *extra] + if self.is_temp_workspace(ws): + roots = [{"path": ws, "writable": True, "label": "scratch"}, *extra] + elif self._SESSION_ID_RE.match(session_id or "") and session_id not in {".", ".."}: + roots = [ + {"path": ws, "writable": True, "label": "workspace"}, + { + "path": self._provision_scratch(session_id), + "writable": True, + "label": "scratch", + }, + *extra, + ] + else: + # A session id we won't put in a filesystem path: primary root only. + roots = [{"path": ws, "writable": True, "label": "workspace"}, *extra] engine = build_engine( agent=ag, workspace=ws, @@ -2305,13 +2329,28 @@ class SessionManager: ) return "\n".join(lines) - def list_artifacts(self, session_id: str) -> list[dict[str, Any]]: + def _artifact_scan_root(self, session_id: str) -> Optional[Path]: + """The dir the Artifacts panel lists: the session's SCRATCH surface only + (workspace-scratch-design.md §2.5). For orphan sessions that's the workspace + itself; for folder-gated sessions it's the side scratch root — never the user's + repo, which would list the whole codebase as 'artifacts'.""" 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(): + if workspace and self.is_temp_workspace(workspace): + return Path(workspace).expanduser().resolve() + if self._SESSION_ID_RE.match(session_id or "") and session_id not in {".", ".."}: + d = (self.scratch_base() / session_id).resolve() + if d.is_dir(): + return d + # Legacy fallback (pre-universal-scratch sessions on a custom scratch base): + # a workspace that is itself disposable still scans. + if workspace and not record: + return Path(workspace).expanduser().resolve() + return None + + def list_artifacts(self, session_id: str) -> list[dict[str, Any]]: + root = self._artifact_scan_root(session_id) + if root is None or not root.is_dir(): return [] out: list[dict[str, Any]] = [] suffixes = { @@ -2385,25 +2424,45 @@ class SessionManager: 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).""" + """Resolve an artifact path under one of the session's roots — workspace first, + then the scratch dir, then user-granted extra roots. Universal scratch means a + gated session's artifacts live BESIDE its workspace, so single-root resolution + would orphan every transcript chip pointing at scratch.""" record = self.session_store.load(session_id) workspace = record.workspace if record else self.default_workspace - if not workspace: + candidates: list[Path] = [] + if workspace: + candidates.append(Path(workspace).expanduser().resolve()) + if self._SESSION_ID_RE.match(session_id or "") and session_id not in {".", ".."}: + scratch = (self.scratch_base() / session_id).resolve() + if scratch.is_dir() and scratch not in candidates: + candidates.append(scratch) + for r in (record.extra_roots if record else []) or []: + p = Path(str(r.get("path", ""))).expanduser() + if p.is_dir(): + rp = p.resolve() + if rp not in candidates: + candidates.append(rp) + if not candidates: 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(): + found_missing = False + for root in candidates: + target = (root / path).expanduser().resolve() + try: + target.relative_to(root) + except ValueError: + continue + if allow_dir and target.is_dir(): + return target, None + if target.is_file(): + return target, None + found_missing = True + if found_missing: return None, ( "This isn't in the conversation's folder anymore — it may have been " "moved or deleted." ) - return target, None + return None, "path escapes workspace" 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 diff --git a/coworker/tools/files.py b/coworker/tools/files.py index cd5fe71d..59716a0b 100644 --- a/coworker/tools/files.py +++ b/coworker/tools/files.py @@ -9,7 +9,7 @@ the agent how to continue reading. Read-only, workspace-scoped. from __future__ import annotations from pathlib import Path -from typing import Any +from typing import Any, Optional import aisuite as ai @@ -47,8 +47,12 @@ _SCHEMA = { } -def file_tools(workspace: str) -> list: +def file_tools(workspace: str, roots: Optional[list] = None) -> list: + """Windowed read_file rooted at `workspace`. With `roots` (RootDir list), absolute + paths inside ANY root also resolve — multi-root sessions (universal scratch) address + their scratch/extra dirs by the absolute paths the roots context advertises.""" root = Path(workspace).resolve() + extra_roots = [Path(str(r.path)).resolve() for r in (roots or [])] def read_file( path: str, @@ -63,10 +67,19 @@ def file_tools(workspace: str) -> list: ) n = min(n, _DEFAULT_MAX_LINES) target = (root / path).resolve() + home = root try: target.relative_to(root) # keep reads inside the workspace except ValueError: - return {"error": "path escapes the workspace"} + for r in extra_roots: + try: + target.relative_to(r) + home = r + break + except ValueError: + continue + else: + return {"error": "path escapes the session's directories"} if not target.is_file(): return {"error": f"not a file: {path}"} @@ -87,7 +100,7 @@ def file_tools(workspace: str) -> list: end = start + len(selected) - 1 if selected else start - 1 result: dict[str, Any] = { - "path": str(target.relative_to(root)), + "path": str(target.relative_to(home)) if home == root else str(target), "start_line": start, "end_line": end, "total_lines": total, diff --git a/surfaces/gui/e2e/artifacts.spec.ts b/surfaces/gui/e2e/artifacts.spec.ts index 747195f2..a8706a08 100644 --- a/surfaces/gui/e2e/artifacts.spec.ts +++ b/surfaces/gui/e2e/artifacts.spec.ts @@ -53,3 +53,21 @@ test("a transcript chip opens the viewer on the FIRST click even with the rail h await page.getByTestId("artifact-chip").click(); await expect(page.getByTestId("artifact-frame")).toBeVisible(); }); + +test("Artifacts section renders for a folder-gated coworker too (universal scratch)", async ({ + page, +}) => { + // UX-036: every session has a scratch surface, so the drawer's Artifacts section is no + // longer cowork-only — a security session lists its scratch-side reports the same way. + await page.goto("/"); + await page.getByTestId("coworker-chip").click(); + await page.locator(".setup-menu").getByRole("button", { name: /Security Coworker/ }).click(); + await page.getByPlaceholder(/Ask the coworker/).fill("audit this repo"); + await page.getByRole("button", { name: "Send" }).click(); + await page.getByTestId("send-folder-dialog").getByRole("button", { name: "Choose a folder…" }).click(); + await expect(page.getByText(/Echo: audit this repo/)).toBeVisible(); + + await expect(page.getByTestId("rail-toggle-artifacts")).toBeVisible(); + await page.getByTestId("rail-toggle-artifacts").click(); + await expect(page.locator(".artifact-row", { hasText: "security-review.html" })).toBeVisible(); +}); diff --git a/surfaces/gui/src/App.tsx b/surfaces/gui/src/App.tsx index 9fd31e3f..c76cfb2f 100644 --- a/surfaces/gui/src/App.tsx +++ b/surfaces/gui/src/App.tsx @@ -2065,12 +2065,14 @@ export function App() { todo={todo} running={running} onPreviewChange={onArtifactPreview} - showArtifacts={agent === "cowork"} + // Universal scratch (UX-036): every session has a scratch surface, so the + // Artifacts section always shows — the server lists the scratch root only. + showArtifacts personaId={agent} projectScoped={isProjectScoped(personaOf(agent))} workspace={workspace || undefined} branch={branch} - scratchPrimary={agent === "cowork" || tempWorkspace} + scratchPrimary={tempWorkspace || !isProjectScoped(personaOf(agent))} openAccessKey={accessKey} onOpenIntegrations={() => setSurface("integrations")} board={board} diff --git a/tests/conftest.py b/tests/conftest.py index 61abb579..fa9b4cbf 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -21,6 +21,9 @@ def _isolated_state_dir(tmp_path, monkeypatch): sign-in, which made test session creation emit REAL telemetry to prod (found 2026-07-03 as burst noise in the ocw-connect-telemetry-events table).""" monkeypatch.setenv("COWORKER_STATE_DIR", str(tmp_path / "coworker-state")) + # Universal scratch provisions a per-session dir for EVERY session — without this, + # tests would mkdir under the developer's real ~/OpenWorker. + monkeypatch.setenv("COWORKER_SCRATCH_BASE", str(tmp_path / "coworker-scratch")) monkeypatch.delenv("COWORKER_API_TOKEN", raising=False) diff --git a/tests/test_multiroot.py b/tests/test_multiroot.py index d24aa0ef..e42b7277 100644 --- a/tests/test_multiroot.py +++ b/tests/test_multiroot.py @@ -320,3 +320,90 @@ def test_request_directory_without_requester_is_safe_noop(): next(e for e in events if e.type == EventType.TOOL_FINISHED).data["status"] == "denied" ) + + +# -- Universal scratch (workspace-scratch-design.md §4) ------------------------- + + +def _repo(tmp_path, name="repo"): + d = tmp_path / name + d.mkdir() + (d / "README.md").write_text("hello", encoding="utf-8") + return d + + +def test_gated_session_gets_workspace_plus_scratch_roots(tmp_path): + mgr = _cowork_manager(tmp_path) + repo = _repo(tmp_path) + sid = "sessGated1" + engine = mgr.get_engine(sid, agent="code", workspace=str(repo)) + assert engine is not None + + roots = mgr.get_roots(sid) + assert len(roots) == 2 + assert roots[0]["primary"] and Path(roots[0]["path"]) == repo.resolve() + scratch = mgr.scratch_base() / sid + assert Path(roots[1]["path"]) == scratch.resolve() + assert roots[1]["writable"] is True + assert scratch.is_dir() # provisioned at engine build + + # Both roots are writable to the permission engine; elsewhere is not. + perms = engine.permissions + assert perms._under_writable_root(str(repo / "x.txt")) + assert perms._under_writable_root(str(scratch / "x.txt")) + assert not perms._under_writable_root(str(tmp_path / "elsewhere.txt")) + + +def test_orphan_session_keeps_scratch_primary_single_scratch_root(tmp_path): + mgr = _cowork_manager(tmp_path) + sid = "sessOrphan1" + engine = mgr.get_engine(sid, agent="cowork") + assert engine is not None + roots = mgr.get_roots(sid) + # ws == scratch: exactly one scratch root, never doubled. + assert len(roots) == 1 and roots[0]["primary"] and roots[0]["writable"] + assert Path(roots[0]["path"]) == (mgr.scratch_base() / sid).resolve() + + +def test_gated_session_artifacts_list_scratch_not_repo(tmp_path): + mgr = _cowork_manager(tmp_path) + repo = _repo(tmp_path) + sid = "sessGated2" + assert mgr.get_engine(sid, agent="code", workspace=str(repo)) is not None + scratch = mgr.scratch_base() / sid + (scratch / "report.html").write_text("

posture

", encoding="utf-8") + + arts = mgr.list_artifacts(sid) + names = {a["name"] for a in arts} + # The repo's files are NOT artifacts; the scratch report is. + assert "report.html" in names and "README.md" not in names + + +def test_artifact_chips_resolve_in_workspace_and_scratch(tmp_path): + mgr = _cowork_manager(tmp_path) + repo = _repo(tmp_path) + sid = "sessGated3" + engine = mgr.get_engine(sid, agent="code", workspace=str(repo)) + assert engine is not None + mgr.save(sid, engine) # chips resolve against the persisted record, as after a turn + scratch = mgr.scratch_base() / sid + (scratch / "report.html").write_text("

ok

", encoding="utf-8") + + # Relative chip → workspace file (as before). + assert mgr.read_artifact(sid, "README.md")["ok"] is True + # Relative chip that only exists in scratch → resolves there. + assert mgr.read_artifact(sid, "report.html")["ok"] is True + # Absolute scratch path (what the roots context advertises) → resolves. + assert mgr.read_artifact(sid, str(scratch / "report.html"))["ok"] is True + # Escapes every root → refused. + outside = tmp_path / "outside.txt" + outside.write_text("x", encoding="utf-8") + assert mgr.read_artifact(sid, str(outside))["ok"] is False + + +def test_gated_session_registers_request_directory(tmp_path): + mgr = _cowork_manager(tmp_path) + repo = _repo(tmp_path) + engine = mgr.get_engine("sessGated4", agent="code", workspace=str(repo)) + assert engine is not None + assert "request_directory" in engine.registry.names()