[joelmoss]

Summary

Fixes #287. Fixes #296.

Delete Project now hides the project until its teardown ends. Tearing down a project's remote workrooms can take minutes. Before this change, any reload in that window, such as another window's create or a failed delete, republished the project with half-destroyed workrooms. The user could then open panes on it or start a second delete.

• The project's path goes into deletingProjects, the project-level twin of deletingWorkrooms. It lives in the shared ProjectStore, so every window's reload drops it.
• While the path is there, the app refuses three things in that project: a second delete (for example from another window's stale confirm sheet), a local create and a remote create.
• On success, --from-disk moves the returned directories to the Bin, the app begins a fresh, quiet load, and only then lifts the tombstone. A list issued before config dropped the project therefore loses claimPublication to the newer generation and can't republish it. A failed list there doesn't report the successful delete as an error.
• On failure, the app lifts the tombstone before reloading, so the project, still in config, reappears.

Separate commit: a flaky test fixed. SessionSaveTests waited a fixed 0.2–0.3s for writes that land on the store's own queue. Two of its tests flaked during this work. Under CPU load (one yes per core) the old code failed 1 run in 3; the new code passed 9 of 9. Tests that expect a write now pump until it lands, capped at 5s. Tests asserting that nothing is written keep their fixed pumps.

Holds the remote-host sweep while a delete is in flight (#296). This was raised in review. A reload during Delete Project or Delete Workroom hands RemoteHosts.adopt a tombstone-filtered list. If that reload ran the launch's one sweep, the sweep treated the deleting item's hosts as unknown and could destroy them. A delete that then failed left config recording hosts that no longer existed. adopt now takes sweep:, false while any tombstone is set. claimSweep(allowed:) leaves the sweep for the next call.

Test Coverage

New tests are in AppStoreDeleteRaceTests, plus one assertion added to WorkroomStatusTests:

• A stale reload during the teardown doesn't bring the project back. A second delete is refused. A fresh reload after the teardown keeps the project gone.
• The tombstone is shared across windows: two AppStores on one ProjectStore.
• A stale read held open (in prepareRepositories) across a successful delete doesn't republish the project.
• A local create is refused while the project is being deleted, and canCreateRemoteWorkroom is false.
• A failed delete brings the project back, clears the tombstone and surfaces the error.
• A delete refused before it starts (an another-build remote workroom) leaves no tombstone.

Each guard was mutation-checked: breaking it turns its test red. The checks covered removing the reload filter, clearing the tombstone after (not before) the failure reload, keeping the tombstone per window, inserting it before the refusal, dropping the success-path reload, and removing each create guard.

Coverage: 73% value-weighted at the first audit (91% any test). The gaps it named are now tested: cross-window sharing, the early-return ordering and the failure surfacing.
Known, not tested:

• The quiet success-path reload with a failing list.

Pre-Landing Review

There were five passes of checklist and specialist review (testing and maintainability, plus simplification once the diff passed 100 lines). Security, performance, migration, API and design were out of scope.

Fixed during review:

• A stale call-site comment.
• An over-long doc comment.
• Three test-coverage extensions.

Skipped, with reasons:

• "A closed window leaves the tombstone stuck": false positive. The completion captures stores, which holds every window's AppStore strongly.
• "pump(until:) waits vacuously": false positive. lastWritten is set synchronously before the write. Breaking the failure latch fails 4 tests.

Adversarial review: three rounds, each with Claude and Codex (gpt-6-astra), then a final focused Codex pass, which reported NO_FINDINGS. Fixed from those rounds:

1. Stale read after lift (Claude and Codex): a read in flight could republish the project once the success path lifted the tombstone. The success path now reloads first.
2. Create during delete (Claude and Codex): local and remote create are now refused while the project is tombstoned.
3. Trash after reload (Codex): --from-disk now trashes before the reload suspends.
4. Clock rollback (Codex): a backward clock step could skip the barrier reload. It's now an unconditional quiet load.

Accepted, matching existing behaviour:

• A teardown that hangs forever hides the project until relaunch. Delete Workroom (Improve workroom creation UI flow #116) already behaves this way.
• No Add Project guard: the CLI canonicalizes the path, and re-adding a project mid-delete is rare.

Follow-ups (not in this PR)

• Delete Workroom: a reload already in flight can republish the workroom once its tombstone lifts #295: Delete Workroom has the same stale-read race (Improve workroom creation UI flow #116). startWorkroomTeardown lifts deletingWorkrooms straight after cli.delete returns, with no newer load. The same reload-before-lift fix applies.

Exploratory QA

App-level behaviour is covered by the unit tests above, which drive AppStore through a gated fake CLI and a gated prepareRepositories. Not run: a manual Delete Project against real remote hosts, and make app-uitest.

Design Review

No frontend files changed, so design review was skipped.

Eval Results

No prompt-related files changed, so evals were skipped.

Scope Drift

Scope Check: CLEAN. The fix is confined to deleteProject, the reload filter and the two create guards. The SessionSaveTests commit is a separately approved fix for a pre-existing flake found while running the suite.

Plan Completion

No plan file detected. The work is defined by #287.

Verification Results

No plan-specific verification items. The automated checks are under Test plan.

TODOS

No TODO items completed in this PR.

Documentation

Status: updated. One factual edit, no blockers.

docs/designs/remote-workrooms.md gets an "As built (#253, part 4: deleting)" bullet for #287. It covers the deletingProjects tombstone, the refusals, and the order of reload and lift on success and on failure. The status paragraph's "follow-ups #283 to #288" sentence is unchanged. README.md, AGENTS.md, CONTRIBUTING.md, macapp/AGENTS.md, macapp/README.md and TODOS.md were reviewed and need no change. No coverage debt: there is no new public surface.

Test plan

• make app-lint: clean
• make app-test: 3038/3038 passed on the final code (the only later change is the design doc)
• make app-test -only-testing:WorkroomAppTests/SessionSaveTests under CPU load: 9/9 passed (old code: 1 failure in 3)
• make app-uitest: not run

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Projects being deleted remotely no longer reappear in the app during a refresh.
  • While project deletion is underway, starting another deletion or creating a workroom in that project is blocked.
  • If deletion fails, the project reappears when it is still present in your configuration.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 19 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 6728a0ed-2eef-4ec2-b1b4-4130aad16d81

📥 Commits

Reviewing files that changed from the base of the PR and between db8cf9f and 65430ec.

📒 Files selected for processing (4)

• macapp/WorkroomApp/Core/AppStore.swift
• macapp/WorkroomApp/Core/RemoteWorkrooms.swift
• macapp/WorkroomAppTests/AppStoreDeleteRaceTests.swift
• macapp/WorkroomAppTests/RemoteHostsTests.swift

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 6351f49b-d6f6-4444-92d3-98f2447de7ae

📥 Commits

Reviewing files that changed from the base of the PR and between 35725fa and db8cf9f.

📒 Files selected for processing (6)

• docs/designs/remote-workrooms.md
• macapp/WorkroomApp/Core/AppStore.swift
• macapp/WorkroomApp/Core/ProjectStore.swift
• macapp/WorkroomAppTests/AppStoreDeleteRaceTests.swift
• macapp/WorkroomAppTests/SessionSaveTests.swift
• macapp/WorkroomAppTests/WorkroomStatusTests.swift

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Project deletion now uses a shared tombstone to exclude projects from reload results and block creation or duplicate deletion during teardown. The app reloads before clearing the tombstone on success and clears it before reloading after failure. Session-save tests now wait for expected asynchronous states.

Changes

Project deletion tombstones

Layer / File(s)	Summary
Shared tombstone and reload filtering macapp/WorkroomApp/Core/ProjectStore.swift, macapp/WorkroomApp/Core/AppStore.swift	ProjectStore publishes paths for projects being deleted. AppStore excludes those projects from reload results.
Deletion guards and lifecycle macapp/WorkroomApp/Core/AppStore.swift, macapp/WorkroomAppTests/AppStoreDeleteRaceTests.swift, macapp/WorkroomAppTests/WorkroomStatusTests.swift, docs/designs/remote-workrooms.md	Workroom creation and duplicate project deletion are refused while deletion is in progress. The flow sets the tombstone before removal, reloads before clearing it on success, and clears it before reloading on failure. Tests cover stale reloads, concurrent windows, creation guards, and failed deletion. The design notes describe this lifecycle.

Session-save test waits

Layer / File(s)	Summary
Condition-based save test waits macapp/WorkroomAppTests/SessionSaveTests.swift	Tests wait for expected session files or states instead of using fixed-duration run-loop waits. The helper uses a five-second timeout.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant AppStore
  participant ProjectStore
  participant CLI
  AppStore->>ProjectStore: Add project path to deletingProjects
  AppStore->>CLI: deleteProject
  CLI-->>AppStore: Teardown result
  AppStore->>AppStore: Reload while tombstone remains set
  AppStore->>ProjectStore: Clear tombstone after successful reload

Loading

Merge Risk: ⚪ Minimal · up to db8cf

No actionable issue remains that should prevent merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to db8cf

The change strengthens protection against using or deleting a project while its teardown is running. No introduced security vulnerability was established. Interrupted teardown and concurrent re-registration remain incompletely validated, so some lifecycle risk remains.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The inspected change coordinates one project path across app windows and its existing local and recorded remote resources. It narrows competing mutation reachability; no additional resource-selection or privilege-granting mechanism was established in the changed flow.

Trust Boundaries and Controls

• observed — The tombstone is an app-lifecycle coordination control, not a replacement for remote ownership or credential checks. Add Project does not consult it; existing configuration registration updates preserve workrooms and do not themselves provision remote resources.

Resilience and Maintainability Implications

• observed — Existing incomplete remote cleanup attempts to record remaining host and grant state as failed before propagating the error. Recording is best-effort, and the project-level failure path restores visibility for recovery rather than guaranteeing transactional rollback.
• inferred — Closing the initiating window does not by itself establish a stranded tombstone: the stop-completion callback captures affected stores, including the deleting store. Process interruption and indefinitely suspended external work remain outside the demonstrated recovery coverage.

Hardening Proposals

• proposed — Consider validating same-path re-registration and interruption after partial remote cleanup, including remaining grants and safe retry. Serializing Add Project against deletion could strengthen path ownership, but this is hardening of an existing overlap rather than an established new vulnerability.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The SessionSaveTests.swift changes replace fixed waits with condition-based waits for asynchronous session writes. This addresses an unrelated test flake and has no demonstrated connection to #287. …	Remove the SessionSaveTests.swift changes from this PR or submit them separately.
Docstring Coverage	⚠️ Warning	Docstring coverage is 78.26% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 5 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	#287 requires Delete Project to prevent reloads from exposing workrooms during teardown and to prevent a second delete. ProjectStore.deletingProjects is shared across windows, and AppStore filters…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: adding a project tombstone while Delete Project tears down the project.

Full details: Out of Scope Changes check

Explanation

The SessionSaveTests.swift changes replace fixed waits with condition-based waits for asynchronous session writes. This addresses an unrelated test flake and has no demonstrated connection to #287. The deletion implementation, its tests, and the deletion documentation are in scope.

Full details: Docstring Coverage

Explanation

Docstring coverage is 78.26% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 5 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Confidence Score: 4/5

[Medium risk] Adds tombstone tracking for projects being deleted.

The PR is not yet safe to merge because the outstanding quiet-reload race can leave the sidebar stale.

Summary

The PR hides projects across windows while deletion is in flight, blocks conflicting actions, and defers remote-host sweeping until deletion ends. The latest change narrows the follow-up reload to cases where a sweep was actually held.

• The existing quiet-reload race remains unresolved: it can supersede a successful user reload and then fail, leaving the sidebar stale.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Delete begins] --> B[Set shared tombstone]
  B --> C[Reload filters deleting project]
  C --> D[Hold host sweep]
  D --> E[Delete ends]
  E --> F[Reload before lifting project tombstone]
  F --> G[Lift tombstone]
  G --> H{Sweep held?}
  H -->|Yes| I[Load fresh config for sweep]
  H -->|No| J[Done]

Loading

_{Reviews (5) · Last reviewed commit: "fix(macapp): read config for a sweep onl..."}