Skip to content

fix(worktrees): stop runtime-stamped folder PTYs and trust the resolved host on folder deletion - #12427

Closed
brennanb2025 wants to merge 1 commit into
mainfrom
brennanb2025/fence-folder-runtime-followup
Closed

fix(worktrees): stop runtime-stamped folder PTYs and trust the resolved host on folder deletion#12427
brennanb2025 wants to merge 1 commit into
mainfrom
brennanb2025/fence-folder-runtime-followup

Conversation

@brennanb2025

Copy link
Copy Markdown
Contributor

Summary

Fixes two folder-workspace gaps that the post-merge audit of #12388 found in the merged PTY-teardown fence, both with red-before evidence:

  1. removeManagedWorktree treated a runtime stamp as an external desktop-mirror namespace and skipped stopping the selected runtime server's ordinary local PTYs — a PTY leak on runtime-stamped folder removal.
  2. Desktop folder deletion trusted collision-prone global worktree metadata over the already-resolved repo host, risking teardown against the wrong host.

Authorship: the audit reviewer wrote and verified these changes in its worktree; the coordinator committed them verbatim for this gated follow-up. The deliberate residuals from #12388 (local-host fence, stopMissingWorktreeTerminals direct-SSH-only scope) are unchanged on purpose — runtime-server-local PTYs are intentionally not remote-environment-prefixed there.

Screenshots

No visual change.

Testing

  • pnpm lint
  • pnpm typecheck
  • pnpm test
  • pnpm build
  • Added or updated high-quality tests that would catch regressions, or explained why tests were not needed

Audit-run evidence: each fix has a red-before proof (reverting the runtime-folder handling makes the runtime-stamped folder test report zero local-provider shutdowns; restoring old metadata precedence makes the SSH-folder test report zero SSH-provider lookups); green-after 1269 passed / 1 skipped across the affected suites, plus node typecheck, direct oxlint, max-lines ratchet, and diff check. The changed-code-quality wrapper alone hit its known Node 26 engine-warning JSON parser issue.

AI Review Report

This PR is itself the output of a subagents-until-clean post-merge audit of #12388 (adversarial fence verification across local/SSH/runtime hosts and folder/git worktrees). Per the standing merge gate it will additionally receive its own pre-merge review loop before merging. Cross-platform: host-kind logic only, no paths/shortcuts/shell changes.

Security Audit

No new inputs, IPC, command execution, or secrets; the changes narrow which PTYs are stopped (leak fix) and bind teardown to the resolved owner host (wrong-host fix) — both reduce blast radius.

Notes

Rides the next release train; not release-gating.

…ed repo host on folder deletion

Two folder-workspace gaps in the #12388 fence audit, with red-before proof
for each: removeManagedWorktree treated a runtime stamp as an external
desktop-mirror namespace and skipped the selected runtime server's local
PTYs, and desktop folder deletion trusted collision-prone global worktree
metadata over the already-resolved repo host. Authored by the post-merge
audit reviewer; committed by the coordinator for the gated follow-up PR.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing as an exact duplicate of #12429 (byte-identical diff). The audit worker opened #12429 itself after this harvest was created from its then-uncommitted worktree; #12429 carries the author's Electron evidence, so it is the canonical PR. The in-flight gate review is being redirected there.

@brennanb2025
brennanb2025 deleted the brennanb2025/fence-folder-runtime-followup branch August 4, 2026 05:18
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Folder workspace PTY cleanup now uses resolved repository or connection ownership instead of persisted metadata. Runtime deletion selects a local or SSH provider from repo.connectionId and limits SSH inventory inclusion to the resolved provider. IPC cleanup uses the resolved repository host. Tests cover SSH connection scoping and runtime cleanup without SSH provider access.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes both primary fixes: runtime-stamped folder PTY teardown and resolved-host ownership during deletion.
Description check ✅ Passed The description covers the required summary, screenshots, testing, AI review, security audit, and notes sections with relevant details.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/main/runtime/orca-runtime.ts (1)

23514-23532: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Correct fix; consider deduplicating the repeated connectionId→provider resolution pattern.

The change correctly derives PTY ownership from repo.connectionId alone, treating only a direct SSH connection as external. This matches the rationale in the added comment: this OrcaRuntimeService instance is itself the selected runtime for executionHostId: 'runtime:X' repos, so such repos' PTYs already live in this.getLocalProvider()'s namespace.

This brings the folder-repo branch in line with two other call sites in the same file that already use the identical connectionId → sshProvider ?? localProvider pattern with matching includeProviderInventory/includeLocalRegistry gating:

  • stopPtysForDestructiveWorktreeRemoval (used for non-folder git-worktree removal).
  • The "repo not found" orphan branch inside removeManagedWorktree, which derives its provider from orphanHost?.kind === 'ssh'.

The bug this PR fixes is exactly a divergence between this pattern and the folder-repo branch. With the pattern now duplicated a third time, a future change to one site (for example, adding a new host kind) risks the same class of divergence recurring in one of the other two sites.

Consider extracting a small private helper, for example resolveWorktreeTeardownPtyRouting(connectionId: string | null): { provider: IPtyProvider | null; resolvedConnectionId?: string; includeProviderInventory?: boolean; includeLocalRegistry?: boolean }, and using it at all three call sites.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4c455f6e-9b78-4a0e-b91a-32e52cee7540

📥 Commits

Reviewing files that changed from the base of the PR and between 5bd2f59 and f26702a.

📒 Files selected for processing (4)
  • src/main/ipc/worktrees.test.ts
  • src/main/ipc/worktrees.ts
  • src/main/runtime/orca-runtime.test.ts
  • src/main/runtime/orca-runtime.ts

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.

1 participant