Skip to content

fix(workspaces): prevent ephemeral workspace data from reaching disk - #485

Open
morluto wants to merge 23 commits into
repoprompt:mainfrom
morluto:fix/ephemeral-storage-contract
Open

fix(workspaces): prevent ephemeral workspace data from reaching disk#485
morluto wants to merge 23 commits into
repoprompt:mainfrom
morluto:fix/ephemeral-storage-contract

Conversation

@morluto

@morluto morluto commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

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 WorkspacePersistentStorage capability, 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

  • Mark ephemeral workspaces before any save can be scheduled.
  • Centralize persistence disposition and the typed ephemeralWorkspace error.
  • Restrict construction of workspace-owned persistent storage to manager-authorized paths.
  • Reject ephemeral persistence before directory creation, disk reads, JSON encoding, writer enqueue, normalization writeback, and atomic replacement.
  • Discard queued writes when the authoritative workspace becomes ephemeral.
  • Protect chats, agent sessions, attachments, _git_data, Prompt/MCP Git artifacts, and worktree merge artifacts.
  • Preserve persistent workspace save and reload behavior.

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 passed
  • SwiftFormat and SwiftLint strict: passed
  • RepoPrompt product build: passed

Not included

  • Removing files left behind by older versions
  • Adding a cleanup migration

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@baron baron left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: WorkspaceManagerViewModel correctly 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 ephemeralWorkspace errors, 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 AgentSessionDataService and ChatHistoryManager. 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.

morluto added 2 commits July 10, 2026 20:27
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.
@morluto

morluto commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

@baron F1 and the associated CI fixture regressions are fixed on visible head 79f1471ff9fb673b2b2db7aaf27d2d40cb771e9d. Ephemeral Agent-session/chat reads now return empty/nil without creating sidecars, while mutations still fail at the persistence boundary. The MCP steer/list fixtures are restored to temporary workspaces so they exercise the intended live in-memory contract; the persistent-fixture workaround that caused the shard 2/4 failures was removed. Validation: SwiftFormat was run on the touched files; mandatory commit and outgoing-range push preflights passed. Per the maintainer fast-path request, local heavyweight tests were canceled/skipped and fresh hosted CI is authoritative. F2 (transient attachment/Git-artifact behavior) and F3 (revocable sidecar authorization) are not claimed resolved by this commit and remain open for the next scoped follow-up.

@morluto

morluto commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Correction: the full pushed commit SHA is 79f1471f6a9a12454c9c5a246b49ff7882ab2980 (the short SHA 79f1471f in the preceding comment was correct). All other validation and scope notes stand.

@morluto

morluto commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

@baron F2 and F3 are now addressed on visible head 480dadbbeed429e573754f2f646e5514658312c4. F2: attachments and Git/worktree artifacts remain functional in temporary workspaces, but use a workspace-keyed directory under the process temporary root; that transient directory is removed on workspace deletion and persistence-disposition changes, while chat/session persistence remains rejected. F3: persistent sidecar capabilities now carry a lock-protected revocable authorization; conversion marks the workspace ephemeral and invalidates issued capabilities before awaiting the disk-writer barrier, and attachment writes validate the capability at the mutation boundary. Regression coverage exercises transient artifact resolution and a pre-conversion attachment capability rejected after conversion. SwiftFormat was applied to touched files with unrelated formatter churn removed; mandatory commit and push preflights passed. Per fast-path instruction, fresh hosted CI is the build/test authority.

morluto added 13 commits July 11, 2026 03:43
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Temporary workspaces are written to disk but never indexed or cleaned up

2 participants