fix(workspaces): prevent ephemeral workspace data from reaching disk - #485
fix(workspaces): prevent ephemeral workspace data from reaching disk#485morluto wants to merge 23 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
baron
left a comment
There was a problem hiding this comment.
Intent Analysis
Problem: Ephemeral workspaces were excluded from the index at creation but not from later persistence paths — autosaves, direct saves, chat history, Agent-session storage, attachments, and published Git artifacts could still create orphaned workspace.json and sidecar directories (issue #468).
Before → After: Persistence prevention moves from creation-time convention to enforced guards: manager-level save rejection, a typed WorkspacePersistentStorage capability gating sidecar writes, and a disk-writer barrier on ephemeral conversion. The change also goes beyond persistence: chat/Agent-session reads now throw ephemeralWorkspace, and attachments and Git artifact publication are disabled in temporary workspaces.
Evidence: Two independent code-first reviews of head 189c5db converging on WorkspaceManagerViewModel.swift (save/enqueue/conversion, ~6785–6793, 6838–6853, 7108–7127, 8207–8219), AgentSessionDataService.swift:1377-1380, ChatHistoryManager.swift:283-285, 442-445, MCPGitToolProvider.swift:1117-1127, AgentModeViewModel+DraftsAttachments.swift:103-117; new WorkspaceEphemeralPersistenceTests; CI failure signatures matching these paths exactly.
Confidence: High on the leak diagnosis, the manager-level boundary design, and the read-path regression (found independently twice, confirmed by CI). Medium on the sidecar capability race (single-source, mechanism-plausible) and on whether feature disablement was intended.
Red Flags: All four required app-test shards failing at head; new tests codify the overly broad rejection (WorkspaceEphemeralPersistenceTests.swift:93-109) rather than catching it; scope expands across 14 files into chat, Agent Mode, attachments, and MCP Git without an established product decision; no human review discussion.
Findings
F1 — High (Confirmed): Reads conflate path resolution with persistence authorization, breaking live MCP semantics. AgentSessionDataService.swift:1377-1380 makes ensureAgentSessionsFolder throw ephemeralWorkspace, but the helper backs reads/lookups (:1101-1103, :1217-1225, :1253-1259): agent_run steer on an unknown session returns ephemeralWorkspace instead of "not found," and AgentManageMCPToolService.swift:163-175 aborts before merging live in-memory sessions at :192+, so agent_manage list fails even for memory-only sessions. Same pattern in ChatHistoryManager.swift:283-285 guarded at :442-445. CI: shard 1 fails at AgentRunMCPToolServiceSteerResumeTests.swift:177 with exactly ephemeralWorkspace. Fix: separate non-mutating resolution from directory creation; ephemeral reads return empty/nil, only mutations reject.
F2 — High (Confirmed): Established ephemeral Context Builder/Agent functionality disabled without a ratified contract decision. MCPGitToolProvider.swift:1117-1127 rejects Git artifact publication; AgentModeViewModel+DraftsAttachments.swift:103-117 disables attachments. CI: shard 2 fails at ContextBuilderWorktreeInheritanceTests.swift:160 after "Temporary workspaces cannot publish persistent Git artifacts." Issue #468 requires avoiding orphaned workspace-owned storage, not removing these workflows. Either provide explicitly transient lifecycle-managed storage or make the contract change intentional and maintainer-ratified across callers.
F3 — Medium (Plausible, single-source): Sidecar authorization is revocable in name only. WorkspacePersistentStorage (WorkspaceManagerViewModel.swift:236-244) is a bare URL with no invalidation; setWorkspaceEphemeral (:8208-8218) awaits the writer barrier before setting isEphemeral, so a capability issued pre-conversion can write post-conversion. The in-flight test covers only WorkspaceDiskWriter, not attachments/artifacts/chats/sessions. Verify and fix, or explicitly accept with a comment — strongly recommended, not the primary blocker.
F4 — High (Confirmed by metadata): All four required Build-and-Test app shards fail at head 189c5db. Shards 1–2 root-cause to F1/F2. Shards 3–4 (TabContextRoutingTests ending in CancellationError during workspace serialization) are attributed but not fully diagnosed. Head is not merge-ready regardless of finding disposition.
Maintainer-guidance check
- User impact/invariant: Temporary workspaces must create no durable state while retaining functional in-memory Agent/MCP/chat behavior; the PR enforces the first half by sacrificing the second.
- Root-cause confidence: Confirmed for the original leak and F1/F2 (code path + exact CI signatures); not confirmed for shards 3–4.
- Authority:
WorkspaceManagerViewModelcorrectly owns durable-storage authorization; read services must not use directory-creating helpers as their authority check — that inversion is the core defect pattern. - State-safety: Workspace-JSON writer barrier is sound and installed pre-transition; the sidecar capability window (F3) remains open under concurrency.
- Scale/observability: Disk-leak amplification fixed; but ephemeral operations now emit repeated opaque
ephemeralWorkspaceerrors, and enqueue adds a full workspace-JSON decode per save. - Recommended scope: Keep the manager save/autosave boundary, capability type, and writer barrier. Split path resolution from directory creation in
AgentSessionDataServiceandChatHistoryManager. Implement transient storage for artifacts/attachments or split that policy into a focused follow-up. Fix affected integration fixtures; address or explicitly defer F3. - Validation boundary: Focused
WorkspaceEphemeralPersistenceTests, ephemeral Agent MCP steer/list tests,ContextBuilderWorktreeInheritanceTests,TabContextRoutingTests, all four app shards green, then the live CE MCP smoke flow (Agent Mode/MCP behavior changed).
Verdict
REQUEST_CHANGES. The centralized persistence boundary is the right design and should be preserved, but the PR demonstrably regresses ephemeral Agent/MCP read semantics (F1), disables established Context Builder functionality without a ratified contract decision (F2), and all four required app-test shards fail at head — two failures directly and mechanically attributable to this patch. These are confirmed behavioral defects with exact file/line and CI evidence, not hypothetical or style concerns. The fix path is well-scoped above.
Tests that exercise git artifacts, workspace serialization, and MCP session persistence were still creating ephemeral workspaces. Align those fixtures with the ephemeral storage contract so CI matches the intended persistence boundaries.
|
@baron F1 and the associated CI fixture regressions are fixed on visible head |
|
Correction: the full pushed commit SHA is |
|
@baron F2 and F3 are now addressed on visible head |
…pers Restore the RepoPrompt product build by matching WorkspacePersistenceDisposition's skipEphemeral case, and satisfy SwiftFormat rules on the new authorization helpers.
…rectory Ephemeral workspaces threw `WorkspacePersistenceError.ephemeralWorkspace` in `ensureGitDataRootLoaded`, which prevented `MAP.txt` and other Git artifacts from being published. Route ephemeral workspaces to the memory-only feature artifact directory (the same temporary, auto-cleaned location where their artifacts are written) instead of throwing, so ingress can complete without touching persistent workspace storage.
Convert the conditional assignment of `gitDataURL` to an if expression to satisfy SwiftFormat's conditionalAssignment rule.
Problem
Ephemeral workspaces are meant to exist only for the current app session, but their creation previously entered the normal persistent-workspace flow before the workspace was marked ephemeral. That allowed directory creation or a save to be scheduled during the transition.
Lower-level APIs could also write workspace-owned sidecars from a workspace model or storage URL without first proving that persistence was allowed. A temporary workspace could therefore leave chats, attachments, agent sessions, Git metadata, or merge artifacts on disk.
Fixes #468.
Approach
Treat ephemeral behavior as a storage contract rather than a creation-time flag.
The workspace is now marked ephemeral before creation can schedule persistence. Lower-level storage paths require an authorized
WorkspacePersistentStoragecapability, and the writer checks authoritative workspace state again immediately before writing. Creation-time checks alone would not cover later sidecar writes or work already queued by a stale model.Changes
ephemeralWorkspaceerror._git_data, Prompt/MCP Git artifacts, and worktree merge artifacts.Performance effect
Ephemeral workspaces now stop before entering the persistence pipeline. They no longer perform workspace-owned directory creation, disk reads, JSON encoding, writer enqueueing, normalization writeback, or atomic replacement.
The same rule prevents chats, attachments, agent sessions, and Git artifacts from scheduling filesystem work for an ephemeral workspace. The result is zero workspace-owned persistence for ephemeral workspaces, removing unnecessary serialization and filesystem I/O.
Testing
Regression coverage includes creation and save races, queued writes, stale persistent references, normalization, sidecar storage, attachments, Git artifacts, and persistent workspace reload behavior.
WorkspaceEphemeralPersistenceTests: 13 passedRepoPromptproduct build: passedNot included