Skip to content

fix(codex): remove Orca-created pre-trust entries when a worktree is removed - #24803

Open
ggbdpq wants to merge 6 commits into
stablyai:mainfrom
ggbdpq:fix/codex-pretrust-entry-lifecycle
Open

ggbdpq wants to merge 6 commits into
stablyai:mainfrom
ggbdpq:fix/codex-pretrust-entry-lifecycle

Conversation

@ggbdpq

@ggbdpq ggbdpq commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

ELI5

Every time Orca starts a Codex worker in a worktree, it quietly adds a "this path is trusted" entry into the managed Codex account's config file so the worker skips the trust prompt. But when the worktree is deleted, the entry stayed forever — after a few weeks one machine had 95 dead-path entries out of 110. Because Orca reuses worktree names (myfeature-r1, -r2), a stale "trusted" mark means the next thing created at that old path starts out trusted without anyone deciding that. Now Orca remembers which trust entries it created itself, and removes exactly those when the worktree is removed.

What Changed

  • Grant side (agent-trust-presets.ts): the single production entry point that writes Codex pre-trust (markCodexProjectTrusted) now records ownership — but only when the [projects."<path>"] table did not already exist in that config file. An entry the user wrote by hand, or one that already existed before the dispatch, is not recorded and is never a removal candidate.
  • Ledger (new codex-project-trust-ownership.ts): a small Orca-side JSON ledger (codex-project-trust-created.json, mode 0600, atomic writes) mapping each managed-account config path → project paths whose tables Orca created there. Same bookkeeping shape as the existing managed-home resource-copy markers. Unreadable state degrades to "nothing recorded", never to deleting something unrecorded.
  • Removal side (orca-runtime-resolve-worktree-removal-target.ts): where every removal path (managed, orphan, folder, preserved-branch, interrupted) already converges, Orca now deletes the recorded entries for the worktree's bare path — whole [projects."…"] tables, through the existing TOML table-editing helpers, inside the existing trust-config mutation queue. The worktree id is resolved to a filesystem path with splitWorktreeIdForFilesystem first, because a folder session's ::workspace:<uuid> suffix would otherwise hide the path the grant keyed on.

User-visible before/after: orca worktree rm now takes Orca's own pre-trust entry for that path with it; hand-written trust entries and everything Orca did not create are untouched, including all entries written by older Orca builds (they are not in the ledger).

Why

Trust is a security decision, and the entry outliving the path inverts it: worktree names are reused, so the stale entry pre-trusts whatever shows up next. Recording ownership at write time is the reporter's suggested minimum and the only safe deletion basis — "entry for a path that no longer exists" cannot distinguish Orca's entries from the user's, but "entries Orca created" can, exactly. The cleanup hooks into the shared removal funnel rather than individual removal callers, so managed, orphan, folder, preserved-branch and interrupted removals all behave the same. Scope note: sweeping up orphaned entries written before this ledger exists is deliberately left as follow-up (the ledger can only vouch for what it recorded; a separate cleanup verb would need its own consent story).

Linked Issue

Fixes #24697

Visual Proof

N/A — no UI surface change; the observable behavior is the managed account's config.toml content after worktree rm. The regression tests assert the file content level: Orca's entry gone, a hand-written entry at the same path intact, pre-ledger entries untouched, and folder-session ids cleaning the same bare path.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

New tests: codex-project-trust-ownership.test.ts (ledger round-trip, only-new-tables recorded, canonical path matching), worktree-removal-codex-pretrust-cleanup.test.ts (removal deletes Orca's entry; hand-written entry at the same path survives; folder-workspace session id suffix finds the same bare path — this case was red against the first implementation and drove a fix to use splitWorktreeIdForFilesystem), plus content-level cases in the config-toml project-trust tests (whole-table removal, duplicates removed cleanly,邻近 spelling variants untouched). Local matrix: new + adjacent trust suites 83/83, interrupted/background removal neighbors 21/21, pnpm tc clean, oxlint clean on changed files, targeted orca-runtime removal-chain cases 4/4. Manual end-to-end against a real managed Codex account was not run in this environment; file-level behavior is covered by the content assertions above.

AI Disclosure

Implemented by GLM-5.3-Flash (ZCode agent) under direction of ggbdpq, who reviewed the diff and re-ran the new and adjacent test suites.

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Cross-platform: the ledger and config paths go through existing path helpers (getOrcaUserDataPath, managed-account home resolution); no platform branches added. SSH/remote: Codex trust entries on a remote host's disk are written by that host's Orca and keyed by that host's ledger — a local worktree rm does not reach across, same as before. Performance: one small JSON read on removal, one on grant; no hot paths. Backwards compatibility: entries written before this change have no ledger record and are therefore never removed — the failure mode is the old behavior, not a new hazard.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

We couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting @coderabbitai full review.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5c30658b-541b-4c0b-ad9b-48c39621db4a
📥 Commits

Reviewing files that changed from the base of the PR and between df24ad50a661d849d952b9f6d15241db9bb5f5dd and 7345386376ae346601f3fda7a48ca6f31107ba2c.

📒 Files selected for processing (7)
  • src/main/runtime/orca-runtime-preserved-branch-cleanup.ts
  • src/main/runtime/runtime-interrupted-local-worktree-removal.test.ts
  • src/main/runtime/runtime-interrupted-local-worktree-removal.ts
  • src/main/runtime/runtime-project-group-controller-folder-delete.test.ts
  • src/main/runtime/runtime-project-group-controller.ts
  • src/main/runtime/worktree-removal-codex-pretrust-cleanup.test.ts
  • src/main/terminal-history-deletion.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/runtime/worktree-removal-codex-pretrust-cleanup.test.ts

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

📝 Walkthrough

Orca records ownership of Codex project-trust entries it creates. During worktree cleanup, Orca checks the recorded table before removing it from Codex config files. Entries without ownership records and tables that have changed remain untouched. Worktree removal awaits purge operations so cleanup can complete before the removal flow returns. Tests cover ownership, removal safeguards, host ownership, and asynchronous cleanup.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 73453

Worktree removal now waits for trust cleanup on the previously affected path. No actionable merge-blocking issue remains after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 73453

The change reduces stale trust on covered removal paths and protects pre-existing or modified trust entries. Cleanup remains best-effort, however, and complete coverage of removal and recovery paths is not established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — One removal can touch every configuration file recorded for the normalized project path, potentially spanning multiple managed accounts. The new operation deletes matching project-trust tables, not worktree contents or unrelated configuration tables.

Security Findings and Attack Paths

  • inferred — The separate IPC removal implementation can leave recorded trust behind because it invokes a legacy purge without Codex cleanup. The inspected registration chain connects that implementation to worktrees:remove, although deployment reachability remains incomplete. The implementation is unchanged from the PR base, so this preserves the existing stale-trust exposure rather than demonstrating an introduced vulnerability.

Trust Boundaries and Controls

  • observed — The new cleanup authority is constrained by recorded creation and current table-content matching. Runtime cleanup also retains the existing same-identifier, other-host ownership check before starting trust deletion. These controls protect pre-existing tables, modified replacements, and represented conflicting owners.
  • inferred — The ledger identifies ownership by configuration path, normalized project path, and table content rather than a worktree generation. Identical-content replacement is therefore not distinguishable by the deletion check alone; content matching is not a complete proof of lifecycle ownership.

Resilience and Maintainability Implications

  • inferred — The config write precedes the ownership-ledger write, and ledger-write errors are caught after trust is granted. Interruption between those writes can leave an unrecorded trusted table. Individual atomic writes do not make this transition transactional; the outcome retains a pre-existing exposure rather than increasing grant authority.

Hardening Proposals

  • proposed — For a stronger cleanup guarantee, consider a recoverable ownership transition and a shared cleanup contract for reachable removal funnels. Recovery should continue to require positive ownership evidence and preserve user-modified tables rather than adopting unrecorded entries.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 48.72% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 39 functions across 22 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: removing Orca-created Codex pre-trust entries when a worktree is removed.
Description check ✅ Passed The description covers the required sections and explains the change, motivation, linked issue, testing, and cross-platform and compatibility considerations. It states that manual testing was not perf…
Linked Issues check ✅ Passed The PR meets [#24697]. Granting records a project table only when no matching table existed. Removal uses the ledger and current table bytes to avoid deleting user-written or replaced entries. It reso…
Out of Scope Changes check ✅ Passed The ledger, TOML helpers, and cleanup tests directly support [#24697]. Async removal plumbing ensures cleanup completes before removal reports success. The host-state purge extraction preserves existi…
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

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.

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: adef6c1e-9b53-45c7-ae15-d880d1fabfef

📥 Commits

Reviewing files that changed from the base of the PR and between 1fc24d3 and 5dd3c0533126199e034fc2af41a6255269335d49.

📒 Files selected for processing (8)
  • config/tsconfig.cli.json
  • src/main/agent-trust-presets.ts
  • src/main/codex/codex-project-trust-ownership.test.ts
  • src/main/codex/codex-project-trust-ownership.ts
  • src/main/codex/config-toml-project-trust.ts
  • src/main/codex/config-toml-trust.ts
  • src/main/runtime/orca-runtime-resolve-worktree-removal-target.ts
  • src/main/runtime/worktree-removal-codex-pretrust-cleanup.test.ts

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

Comment thread src/main/codex/codex-project-trust-ownership.ts Outdated
Comment thread src/main/codex/config-toml-trust.ts
Comment thread src/main/runtime/orca-runtime-resolve-worktree-removal-target.ts Outdated
@ggbdpq

ggbdpq commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

All three findings addressed in 781667d85d:

  1. Ledger validation — reads narrow an unknown payload at runtime (plain-object created, {path, table} array values) and fail closed to an empty ledger; {"created":null} no longer reaches Object.entries.
  2. Replacement table — each record now vouches for the exact table bytes the grant wrote. Removal compares the table currently at the path against that snapshot (modulo the trailing separator, which belongs to what follows the table) and on mismatch skips the deletion and forgets the record, so a user-written replacement is never deleted on the strength of an old record. Chose verify-at-removal over invalidation-on-replacement because replacement isn't an observable event — any invalidation touchpoint would need the same content comparison anyway. Codex rewriting the trust level changes the bytes, so it reads as "no longer ours": skipped and forgotten (worst case one missed cleanup, never a wrongful delete).
  3. Same-owner guard — the trust cleanup moved below preservesSameIdOwner and runs only when false; pure reposition, host-scoped meta handling untouched.

61/61 across the ownership/cleanup/trust suites, pnpm tc clean.

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes — incremental re-review of commit 781667d, the hardening commit on this Codex pre-trust lifecycle PR. Commit 5dd3c05 (the original grant/ledger/removal work) landed before this run and was covered by an earlier CodeRabbit review; this commit resolves all three of its comments.

  • Recorded table bytes — ownership records now carry the exact [projects."<path>"] table bytes the grant wrote (CreatedProjectTrustEntry.table), so a table rewritten after the grant is no longer Orca's to delete.
  • Removal-time ownership verification — removeOrcaCreatedProjectTrustEntry compares the current table to the recorded bytes (modulo the trailing separator, which belongs to what follows) and drops the record instead of deleting when they diverge.
  • Strict ledger validation — isCreatedTrustEntry / isCreatedTrustEntryMap reject null, arrays, scalars, and wrong-shaped entries; unreadable state still degrades to "nothing recorded".
  • Same-ID owner guard — the Codex cleanup moved inside the !preservesSameIdOwner branch, matching the existing purge gate in worktree-removal-ownership.ts, so a stale host-scoped removal cannot drop another host's trust entry.
  • Tests — malformed ledgers, a user-replaced table, a rewritten trust line, and the same-owner guard; each can fail against the pre-fix behavior.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@ggbdpq

ggbdpq commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto upstream/main (a2896f5) to clear the conflict introduced by #23952's removal-flow rework in orca-runtime-resolve-worktree-removal-target.ts.

  • The cleanup hook's placement is unchanged semantically: it still runs only under !preservesSameIdOwner, at the same removal funnel (managed/orphan/folder/interrupted + the new retry path all converge on removeWorktreeMetadataAndHistory; retry re-runs are idempotent through the ledger, and the cleanup fires only when a removal actually completes via purge).
  • One forced adjustment: upstream's rework left the file one line under the 300-line max-lines gate, so the hook body moved into its pairing module worktree-removal-codex-pretrust-cleanup.ts (repo ratchet forbids suppressing the rule). Net diff +11/-1 vs the pre-rebase shape; everything else rebase-clean.
  • Verified: ownership/cleanup/trust suites 45/45, orca-runtime -t removal 40 passed (incl. fix(worktrees): a worktree delete git fails partway stays listed and can be retried #23952's new retry cases), interrupted-removal 13/13, pnpm tc clean, oxlint clean, max-lines ratchet OK.

@ggbdpq
ggbdpq force-pushed the fix/codex-pretrust-entry-lifecycle branch from 781667d to 0e15180 Compare October 3, 2026 08:27

@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.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟠 Major · Keep the trust table and ownership ledger consistent. · config-toml-trust.ts:178-202

src/main/codex/config-toml-trust.ts:178-202
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Keep the trust table and ownership ledger consistent.

When recordOrcaCreatedProjectTrust fails, upsertOrcaCreatedProjectTrustLevel has already written the new trusted table, logs the error, and lets markCodexProjectTrusted succeed. Normal worktree removal then looks up config paths only through the ledger. It cannot discover this table, so a later occupant of the same path can inherit stale trust.

Make this grant path roll back the newly written table when its bytes still match, and propagate the failure. Preserve the existing byte comparison so a concurrent replacement is not removed.

Suggested fix
   } catch (error) {
-    // Why: the grant already landed; an unrecorded entry only loses cleanup, never safety.
-    console.warn('[codex-config] Failed to record the created project trust entry:', error)
+    const writtenTable = findProjectTrustRemovedBytes(updated, projectPath)
+    const current = observe(() => readTomlFile(configPath))
+    const currentTable =
+      current.kind === 'present'
+        ? findProjectTrustRemovedBytes(current.value, projectPath)
+        : null
+    if (
+      current.kind === 'present' &&
+      writtenTable !== null &&
+      currentTable !== null &&
+      withoutTrailingSeparator(currentTable) === withoutTrailingSeparator(writtenTable)
+    ) {
+      try {
+        writeConfigAtomically(
+          configPath,
+          removeProjectTrustContent(current.value, projectPath)
+        )
+      } catch (rollbackError) {
+        console.warn('[codex-config] Failed to roll back the created project trust entry:', rollbackError)
+      }
+    }
+    throw error
   }

ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8ac33a18-edfa-402b-bc09-58e47d0041dd
📥 Commits

Reviewing files that changed from the base of the PR and between 781667d85dd3ae0b29427b39bfa6e9e7a7e39340 and 0e15180414c6402ad0ec66a880cfef43172f2147.

📒 Files selected for processing (3)
  • config/tsconfig.cli.json
  • src/main/runtime/orca-runtime-resolve-worktree-removal-target.ts
  • src/main/runtime/worktree-removal-codex-pretrust-cleanup.ts

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

Comment thread src/main/runtime/worktree-removal-codex-pretrust-cleanup.ts Outdated

@pullfrog pullfrog 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.

Important

The await migration in this commit missed one interrupted-removal call site, so that path can still report a worktree removed before the Codex pretrust entry is deleted — the exact ordering this commit set out to guarantee.

Reviewed changes — incremental re-review of df24ad50, which makes the Codex pretrust cleanup awaited so a removal ack cannot precede the trust-table deletion. The previously reviewed 0e15180/bd6f679 extraction and hardening are unchanged.

  • Awaitable cleanup — dropOrcaCreatedCodexPretrustForRemovedWorktree now returns Promise<void>; failures are still caught and warned, so the ownership record is kept for a later pass rather than failing an accepted removal.
  • Extracted host-state purge — the removal purge moved into the new worktree-removal-host-state-purge.ts (purgeRemovedWorktreeHostState / removeRuntimeWorktreeMetadataAndHistory, both async), with OrcaRuntimeWithResolveWorktreeRemovalTarget delegating to it.
  • Async removal contracts — finishRemoval/purge widened to void | Promise<void> across the registered local/remote, unregistered, and interrupted flows, and their call sites awaited.
  • Test — a lane-holding case pins that a purge parked behind a held config.toml lane does not resolve before the recorded entry is deleted.

⚠️ Interrupted removal can still ack before the Codex pretrust deletion

finishInterruptedLocalWorktreeRemoval has two completion paths. The registered path delegates to finishRuntimeLocalWorktreeRemoval, which now awaits finishRemoval (and thus host.purge → the pretrust deletion). The unregistered/leftover path calls args.finishRemoval(result, true, record.head) without await, then returns; the job immediately fires host.onRemoved(record). Because finishRemoval is now async and its awaited work is the pretrust deletion, this path reports the worktree removed while the deletion is still in flight — the ordering the commit's JSDoc and new test assert everywhere else.

Technical details
# Interrupted-removal fallback does not await the purge

## Affected sites
- src/main/runtime/runtime-interrupted-local-worktree-removal.ts:198 — `args.finishRemoval(result, true, record.head)` is called without `await`; the function returns immediately.
- src/main/runtime/runtime-interrupted-local-worktree-removal.ts:75-84 — the job's `finishRemoval` is `async` and `await host.purge(record)`, and `host.purge` (wired at orca-runtime-resolve-worktree-removal-target.ts:108) runs `purgeRemovedWorktree` → `dropOrcaCreatedCodexPretrustForRemovedWorktree`.
- src/main/runtime/runtime-interrupted-local-worktree-removal.ts:86 — `host.onRemoved(record)` fires right after `finishInterruptedLocalWorktreeRemoval` resolves.

## Required outcome
- The unregistered/leftover branch's `finishRemoval` call must be awaited so a removal ack and `onRemoved` cannot precede the recorded-entry deletion, matching the registered branch (line 176 → runtime-registered-local-worktree-removal.ts:266).

## Suggested approach
- Change line 198 to `await args.finishRemoval(result, true, record.head)`. The enclosing function is already `async`, and `finishRemoval` cannot throw here (the cleanup helper degrades internally), so no error-handling change is needed.

## Open questions for the human
- Is the folder-workspace forgetting path (`orca-runtime-preserved-branch-cleanup.ts:250`, intentionally `void` because the controller dep is sync) an accepted exception, or should it gain an async seam too?

Pullfrog  | Fix it ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@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.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟠 Major · Await finishRemoval on the… · runtime-interrupted-local-worktree-removal.ts:198

src/main/runtime/runtime-interrupted-local-worktree-removal.ts:198
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Await finishRemoval on the unregistered-leftover path.

Line 75 makes finishRemoval async. The function now awaits host.purge(record), which includes the Codex pretrust deletion. The fallback branch of finishInterruptedLocalWorktreeRemoval still calls args.finishRemoval(result, true, record.head) without await. This branch handles an unregistered leftover, or a checkout that Git cannot remove.

On this path, the job returns and host.onRemoved(record) fires before the trust table is deleted. A path recreated in that gap can still match trust_level = "trusted". The PR intends to close exactly this ordering gap.

A rejection from the purge is also unhandled on this path. For example, store.removeWorktreeMeta can throw.

The registered path in finishRuntimeLocalWorktreeRemoval already awaits the call. Apply the same change here.

🐛 Proposed fix
   await cleanupRemovedWorktreePushTarget(finishArgs)
-  args.finishRemoval(result, true, record.head)
+  await args.finishRemoval(result, true, record.head)
   return result

ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 76eece39-84ee-4c41-9e4b-378c2949e28d
📥 Commits

Reviewing files that changed from the base of the PR and between 0e15180414c6402ad0ec66a880cfef43172f2147 and df24ad50a661d849d952b9f6d15241db9bb5f5dd.

📒 Files selected for processing (13)
  • src/main/runtime/orca-runtime-preserved-branch-cleanup.ts
  • src/main/runtime/orca-runtime-remove-managed-worktree.ts
  • src/main/runtime/orca-runtime-remove-orphan-or-folder-worktree.ts
  • src/main/runtime/orca-runtime-resolve-worktree-removal-target.ts
  • src/main/runtime/runtime-failed-local-worktree-removal.test.ts
  • src/main/runtime/runtime-interrupted-local-worktree-removal.test.ts
  • src/main/runtime/runtime-interrupted-local-worktree-removal.ts
  • src/main/runtime/runtime-registered-local-worktree-removal.ts
  • src/main/runtime/runtime-registered-remote-worktree-removal.ts
  • src/main/runtime/runtime-unregistered-worktree-removal.ts
  • src/main/runtime/worktree-removal-codex-pretrust-cleanup.test.ts
  • src/main/runtime/worktree-removal-codex-pretrust-cleanup.ts
  • src/main/runtime/worktree-removal-host-state-purge.ts

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

Comment thread src/main/runtime/orca-runtime-preserved-branch-cleanup.ts Outdated
@ggbdpq

ggbdpq commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

@pullfrog[bot] — confirmed and addressed in 7345386376: the interrupted-removal leftover branch now awaits finishRemoval like the registered branch (runtime-interrupted-local-worktree-removal.ts), with a regression test holding the purge mid-flight and asserting the leftover is not reported removed before it lands. We also swept every caller of both cleanup functions for the same gap — no others found. Your report's line, fix, and open question all matched the code. Thanks.

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes — incremental re-review of 7345386376, which closes the two remaining unawaited Codex-pretrust cleanup paths flagged in the prior review. The previously reviewed df24ad50 / 0e15180 / bd6f679 work is unchanged.

  • Awaited the interrupted-removal leftover purge — finishInterruptedLocalWorktreeRemoval now awaits finishRemoval on the unregistered/leftover branch, matching the registered branch, so host.onRemoved cannot fire while the recorded trust table is still being deleted.
  • Returned the folder-workspace cleanup promise — cleanupRemovedFolderWorkspaceState no longer voids removeWorktreeMetadataAndHistory; deleteFolderWorkspace captures it and awaits it after the catalog row is removed (call timing unchanged), so a forgotten folder workspace cannot report before the purge lands.
  • Guarded history-root resolution — deleteWorktreeHistoryDir now catches a getHistoryRoot() failure and warns, keeping the removal purge degrading rather than rejecting.
  • Added ordering regression tests — the leftover path holds its purge mid-flight and asserts the removal does not settle early, and the folder-controller / wired-dep cases pin the same ordering.

I swept the callers myself to confirm the scope claim: every finishRemoval call site (runtime-registered-local-worktree-removal.ts:250,266, runtime-unregistered-worktree-removal.ts:93,116,126, runtime-registered-remote-worktree-removal.ts:73, runtime-interrupted-local-worktree-removal.ts:200) is now awaited, every purgeRemovedWorktree call site is awaited, and cleanupRemovedFolderWorkspaceState has a single wiring site that now returns the promise. The prior review's unregistered-leftover concern is fully addressed.

The earlier open question about the folder-workspace forgetting path is resolved by this commit — it gained the async seam rather than remaining a sanctioned exception.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog 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.

ℹ️ No critical issues — one comment-accuracy nit below. All prior feedback is addressed.

Reviewed changes — incremental re-review of 7345386, which closes the two remaining unawaited Codex pretrust cleanup paths flagged in the prior review. The three touched suites pass locally (23/23).

  • Interrupted removal now awaits — finishInterruptedLocalWorktreeRemoval awaits args.finishRemoval, and the job's finishRemoval awaits host.purge(record), so an unregistered/leftover removal can no longer be reported before the pretrust deletion lands. This resolves the prior ⚠️ finding at runtime-interrupted-local-worktree-removal.ts:198.
  • Folder-workspace delete gains an async seam — cleanupRemovedFolderWorkspaceState may now return a promise; RuntimeProjectGroupController.deleteFolderWorkspace captures it and awaits after removeFolderWorkspace/notifyReposChanged, and the wired dep returns the purge promise instead of voiding it. This answers the prior review's open question about the folder-forgetting path.
  • History-root guard — deleteWorktreeHistoryDir catches getHistoryRoot() failures and degrades, so the newly-awaited purge cannot reject an accepted removal on an uninitialized app path.
  • Tests — parked-promise cases pin that the interrupted leftover, the folder delete, and the folder-forgetting dep all wait for the purge; each fails against the pre-fix behavior.

ℹ️ Nitpicks

  • src/main/runtime/runtime-project-group-controller.ts:233 — the comment says the awaited cleanup includes "the Codex pretrust deletion inside it", but this call site passes folder:<uuid> from folderWorkspaceKey, which splitWorktreeIdForFilesystem cannot parse into a path, so dropOrcaCreatedCodexPretrustForRemovedWorktree returns early and no pretrust entry is touched on a project-group folder-workspace delete. Skipping is the safe behavior here (the folder itself is not removed, and sibling workspaces can share its path); only the parenthetical is inaccurate.

Pullfrog  | Fix it ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@ggbdpq
ggbdpq force-pushed the fix/codex-pretrust-entry-lifecycle branch from f319e79 to 7b040f2 Compare October 6, 2026 04:06
@ggbdpq

ggbdpq commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (7f25413): main's new per-worktree agent-status drop on removal now lives inside the extracted worktree-removal-host-state-purge module (the runtime type gained the corresponding hook member), with its call placed before the same-id gate exactly as upstream ordered it — our await-the-cleanup semantics and that drop coexist (74 + 7 related tests green, pnpm tc fully clean on the new base).

@ggbdpq
ggbdpq force-pushed the fix/codex-pretrust-entry-lifecycle branch from 7b040f2 to eff150f Compare October 8, 2026 14:07
@ggbdpq

ggbdpq commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (00a8250). Upstream's #23952 follow-up replaced the global invalidateAuthorizedRootsCache with invalidateAuthorizedRootsCacheForRepo; the extracted purge module now scopes the invalidation to the removed repoId (the scoped helper only reads getRepos(), which the runtime store contract carries — the narrowing cast carries the SAFETY: rationale the changed-casting gate requires, as do the fakes in the pretrust cleanup test now that the gate scans them). Verified after the rebase: ownership/cleanup/trust + removal suites 74/74, tsc --noEmit -p config/tsconfig.node.json clean, changed-code quality gate green against the new baseline.

@ggbdpq
ggbdpq force-pushed the fix/codex-pretrust-entry-lifecycle branch from eff150f to 433ea47 Compare October 11, 2026 16:40
@ggbdpq

ggbdpq commented Oct 11, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto upstream/main (a4a96fee70) to clear the conflict introduced by #27499, which deleted the SSH PTY provider stack and removed the deleteHistory hooks from the worktree-removal flows.

Fusion: upstream's deletion of the deleteHistory members and deleteRemoteWorktreeHistory calls is kept verbatim (the SSH history-cleanup feature is gone), while this PR's async finishRemoval semantics are preserved, so the Codex pre-trust purge is still awaited before a removal is reported. The two changes touch the same statements but are independent.

Verification on the rebased head: worktree-removal suites 40/40 (pre-trust cleanup, interrupted/failed local removal, folder-delete controller, src/main/ipc/worktrees/removal), adjacent IPC/history suites 36/36, typecheck:node, oxlint, oxfmt, and the max-lines ratchet all clean. No behavior change beyond the rebase itself.

@ggbdpq
ggbdpq force-pushed the fix/codex-pretrust-entry-lifecycle branch from 433ea47 to 82f96f2 Compare October 13, 2026 04:16
@ggbdpq

ggbdpq commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main to clear the conflict from #27528 (worktree handlers are local or runtime only).

That refactor removed the SSH/remote removal path, so this PR adapts to it:

  • runtime-registered-remote-worktree-removal.ts and the route.kind === 'ssh' dispatch block are gone upstream; the rebased branch drops both (dead code) and keeps the upstream signatures without connectionId.
  • The route.kind === 'local' guard around the leftover-directory cleanup was removed upstream (the unregistered removal is local-only now); the rebase follows that shape.
  • The PR's core semantics are unchanged: finishRemoval is still awaitable and every remaining removal path (managed/stale/interrupted/retry/unregistered) awaits the Codex pretrust cleanup before reporting the worktree removed.

Verified: 6 targeted suites 74/74 (pretrust cleanup, failed/interrupted local removal, folder-delete, trust ownership, config-toml trust), typecheck:node clean, oxlint/oxfmt clean on the resolved files, max-lines ratchet fine (257/140/131).

…removed

Every `worker-start --agent codex` into a worktree pre-trusts the worktree
path in the managed Codex account's `config.toml`, and nothing ever removed
the entry: weeks of dispatches left 95 of 110 `[projects."…"]` tables pointing
at deleted paths (stablyai#24697). Worktree names are reused (`<feature>-r1`, `-r2`),
so a stale `trusted` entry is a real hazard — whatever appears next at that
path is born trusted.

Orca now records, at grant time, which project tables its preflight created
(only when the table did not already exist there — a user-written or
pre-existing entry is never recorded and never touched), in a small Orca-side
ledger next to the existing agent state, following the same bookkeeping shape
as the managed-home resource-copy markers. When a worktree is removed, the
entries recorded for its bare path are deleted whole-table, exactly where all
removal paths (managed, orphan, folder, preserved-branch, interrupted) already
converge; the grant keyed the bare path, so a folder session's
`::workspace:<uuid>` id suffix is resolved away first. Every failure mode
degrades toward "do not delete": an unreadable ledger or config means nothing
is removed, a failed table removal keeps its record for the next attempt, and
a config file that no longer exists just forgets its records. Cleanup of
orphaned entries from before this ledger exists is left as follow-up work;
the ledger deliberately only ever vouches for what Orca itself wrote.

Fixes stablyai#24697

Generated-by: GLM-5.3-Flash (ZCode)
…vals

Coderabbit review, three findings:

- Ledger reads now narrow an `unknown` payload at runtime (plain-object
  `created`, values being `{path, table}` arrays) and fail closed to an
  empty ledger; `{"created":null}` no longer slips through `typeof ===
  'object'` into `Object.entries(null)`.
- Each record vouches for the exact table bytes the grant wrote. Removal
  compares the table currently at the path against that snapshot (modulo
  the trailing separator, which belongs to what follows the table) and, on
  any mismatch, skips the deletion and forgets the record — a table the
  user wrote to replace ours is never deleted on the strength of an old
  record.
- The trust cleanup moves below the `preservesSameIdOwner` guard, so a
  stale host-scoped removal can no longer drop the new owner's pre-trust
  entry while sparing its metadata.

Generated-by: GLM-5.3-Flash (ZCode)

Rebase note (upstream stablyai#23952): the removal-target file now sits at the oxlint
max-lines cap, so the removal-time cleanup call moves into
runtime/worktree-removal-codex-pretrust-cleanup.ts instead of suppressing the
rule; behavior and the same-owner guard are unchanged.
…moved

Coderabbit review: the cleanup promise was discarded (void) while
purgeRemovedWorktree and its completion callbacks were synchronous, so the
removal response could precede the trusted project table's deletion — a
worktree recreated inside that window would inherit trust_level=trusted,
re-opening the hazard this PR closes.

The cleanup now returns its promise and the await propagates through the
removal graph (managed/orphan-or-folder/interrupted/retry all converge on
removeWorktreeMetadataAndHistory) before removeManagedWorktree resolves.
Await is for ordering, not failure propagation: cleanup failures degrade to
a console warn and keep the ownership record, so an unwritable config.toml
cannot turn an accepted removal into a failed one. Retry semantics are
unchanged — a failed retry never reaches the cleanup, a successful one
awaits it exactly once.

Generated-by: GLM-5.3-Flash (ZCode)
Pullfrog and coderabbit each caught a path where the removal still reported
before the Codex pretrust deletion landed: the interrupted-removal
leftover branch's finishRemoval call (the registered branch already
awaited), and the folder-workspace controller dep whose signature was
declared synchronous, discarding removeWorktreeMetadataAndHistory's
promise. Both verified against the code and fixed: the leftover branch
awaits like the registered one, and the folder controller's dep widens to
'void | Promise<void>' with a fire-then-await so the delete resolves only
after the purge lands. A full sweep of both cleanup functions' callers
found no other gaps.

Also fixes a latent bug the new tests exposed: deleteWorktreeHistoryDir
evaluated getHistoryRoot() outside its try block, so an uninstalled
AppEnvironment turned the whole purge into a rejection against its
non-fatal contract.

Generated-by: GLM-5.3-Flash (ZCode)
The purge's pretrust drop is keyed to worktree filesystem paths; a
folder:<uuid> id parses to none, so it skips here by design — the folder
itself is not removed and sibling workspaces may share its path. The
await still orders the folder-state cleanup behind the purge.

Generated-by: GLM-5.3-Flash (ZCode)
Rebase onto 00a8250: upstream (stablyai#23952 follow-up) replaced the global
invalidateAuthorizedRootsCache with invalidateAuthorizedRootsCacheForRepo, so
the extracted purge module now scopes the invalidation to the removed repoId
instead of evicting every repo's cached roots. The scoped helper is typed for
the full Store but only reads getRepos(), which RuntimeStore carries — the
narrowing cast carries the SAFETY: rationale the changed-casting gate
requires, as do the fakes in the pretrust cleanup test.

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq
ggbdpq force-pushed the fix/codex-pretrust-entry-lifecycle branch from 82f96f2 to 180f5a4 Compare October 13, 2026 12:14
@ggbdpq

ggbdpq commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (a10ca391ff) to clear the new conflict from #28043 (agent-tab restart verdicts), which extends removeWorktreeMetadataAndHistory with failed-launch forgetting and crosses this PR's extraction of that body into worktree-removal-host-state-purge.ts.

Adaptation: the new forgetFailedAgentLaunches(worktreeId, null) step moved with the extracted body, so the !preservesSameIdOwner purge in the paired module now forgets failed launches exactly where main does (import + call ported verbatim). No behavior change on either side. Net branch content is unchanged apart from that port: 5 of 6 commits rebased with identical patch-ids; the await the pretrust cleanup commit carries the only hand-merge.

Verified on the new head 180f5a4747: the six removal/pretrust suites 74/74, tc:node clean, oxlint/oxfmt clean on the two adapted files, and git merge-tree against main is now conflict-free.

This branch has not been deployed

No deployments
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.

[Bug]: Codex pre-trust entries for worktrees stay in the managed account config.toml after worktree rm

1 participant