Repository navigation
Wait for managed-worktree terminals before deletion - #500
Conversation
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
A terminal initialization teardown path can block all subsequent managed-worktree removal attempts until Aiden restarts.
Reviewed changes — Reviewed the terminal exit barrier, managed-worktree deletion ordering, Remote error mapping, tests, and lifecycle documentation.
- Terminal drain: Tracks pending PTY exits across closes and retries, with a five-second timeout.
- Deletion integration: Awaits terminal shutdown before dirty-state capture, snapshots, and Git deletion; maps timeout errors to the existing Remote response.
- Coverage and scope: Adds drain, ordering, and Remote error tests and documents the remaining lifecycle work.
Validation: 60 focused tests, TypeScript checks, and focused ESLint passed. The application ordering test passed its new ordering assertions, then failed an unchanged Linux capacity expectation (1024 actual versus 268436480 expected); native macOS deletion suites were not run here.
gpt-6.1-sol ｜ 𝕏
very-hermes-bot
left a comment
very-hermes-bot
left a comment
There was a problem hiding this comment.
Inline review — 1 finding(s) anchored to the diff. See the pinned summary comment for the overview.
Hermes Review BotConfidence: 5 Engine: SummaryManaged worktree deletion now enforces a bounded process-drain barrier before inspecting dirty state, taking snapshots, or removing checkout files. Terminal sessions belonging to the workspace are signalled with SIGHUP, waited on for shell exit with a 5-second timeout, and audited for surviving background processes via a native POSIX session inspection helper ( Confidence Score: 5/5Full end-to-end verification completed across the native session audit implementations (macOS 📁 Important Files Changed
FindingsNo findings. Sequence DiagramsequenceDiagram
autonumber
participant Client as Client (Desktop / Remote)
participant App as WorkspaceWorktreeAppService
participant Term as TerminalService
participant Native as aiden-worktree-remover
participant Git as Git / Worktree Remover
Client->>App: remove(workspaceId)
App->>Term: closeForWorkspaceAndWait(workspaceId)
Term->>Term: terminate(session) [SIGHUP PTY]
Term->>Term: await Promise.race([exits, timeout(5s)])
Term->>Native: audit-session <sessionId>
alt Surviving process or unverified /proc
Native-->>Term: active / error
Term-->>App: throw ManagedWorktreeProcessActiveError
App-->>Client: 409 / Process active (idempotency cleared)
else Session empty
Native-->>Term: empty
Term-->>App: drain complete
App->>Git: snapshot and remove directory
Git-->>App: deleted
App-->>Client: success
end
[]
|
There was a problem hiding this comment.
ℹ️ No new issues found in this delta. Approval remains withheld because the existing terminal-exit finding is still unresolved.
Reviewed changes since the prior Pullfrog review at 1db6ae1c, focused on the standalone CLI's shared-service contract.
- Aligned the CLI adapter: Changed
closeWorkspaceTerminalsto an async no-op, satisfying the sharedPromise<void>contract without introducing PTY handling in the headless CLI. - Documented separate validation: Recorded that shared application-service changes require the standalone CLI's own build and typecheck.
Validation: standalone CLI build, typecheck, and all 99 tests passed. The working tree is clean. Hosted CI was still running when inspected.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at b97df808, focused on terminal initialization and exit tracking.
- Moved exit observation earlier: Registered the PTY exit listener before owner invalidation can synchronously terminate a newly spawned session, addressing the previously reported permanently blocked drain.
- Preserved initialization cleanup: Retained rejection on synchronous invalidation and removed owner subscriptions when the session exits during installation.
- Extended behavioral coverage: Added a synchronous-invalidation test proving removal stays blocked until exit and succeeds afterward, and extended the spawn-boundary destruction test to verify successful draining.
Validation: all 37 terminal tests, npm run type-check, and focused ESLint passed. Native macOS acceptance was not run on this Linux runner. Hosted CI was still in progress when inspected; this approval does not establish merge readiness.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
Important
The Linux session audit can permit deletion while a terminal job is still alive when process visibility is restricted.
Reviewed changes since the prior Pullfrog review at 4d9cf107, covering the new process-inventory boundary and Remote retry behavior.
- Added native session auditing: Checked retained PTY session IDs for surviving jobs after shell exit, with refusal on active members or inventory errors.
- Enabled same-key retries: Cleared Remote idempotency admission for process refusals before Git deletion while preserving successful-result replay.
- Extended behavioral coverage: Added surviving-session and retry tests and updated lifecycle documentation.
Validation: all 35 native remover tests and 70 focused terminal/Remote/operation-contract tests passed, as did terminal coverage gates, TypeScript, and focused ESLint. A separate private-mount-namespace probe reproduced the inline finding with the production helper. Shared application tests encountered existing Linux platform limitations; native mobile verification was blocked by missing xcodebuild and an uncached Android Gradle plugin. Hosted CI was still incomplete on this head.
gpt-6.1-sol ｜ 𝕏
very-hermes-bot
left a comment
very-hermes-bot
left a comment
There was a problem hiding this comment.
Inline review — 2 finding(s) anchored to the diff. See the pinned summary comment for the overview.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at 37d0e7df, focused on Linux process-inventory safety and native binary compatibility.
- Rejected filtered inventories: Added pre/post procfs policy checks and explicit enumeration-error handling so restricted process visibility refuses deletion rather than authorizing an empty session.
- Preserved Linux compatibility: Replaced
strtolwith the existing bounded digit parser to avoid the newer GLIBC symbol requirement. - Extended behavioral coverage: Added restricted-policy, malformed-inventory, unrestricted-policy, and production fixture-isolation tests, and updated the lifecycle notes.
Validation: all 36 native remover tests and 70 focused terminal/Remote tests passed, along with root TypeScript, changed-test ESLint, and git diff --check. An independent private-mount-namespace probe confirmed that the production helper returns status 22, not empty, while a hidden cross-UID session member remains alive. The locally rebuilt x64 helper imports no symbols above GLIBC_2.34. Hosted CI was still queued/running when inspected; approval does not establish merge readiness.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at 3e74e672, focused on invalid terminal identities and deletion safety.
- Guarded terminal signals: Prevented both process-group signaling and node-pty termination when the reported PID is invalid.
- Clarified the deletion hold: Documented that an unverifiable terminal identity blocks removal for the running service lifetime and requires manual inspection.
- Updated behavioral coverage: Added invalid-PID signal and repeated-removal tests, and removed a stale source-spelling assertion already covered by synchronous-invalidation tests.
Validation: all 39 terminal tests passed in both coverage runs, all 36 native remover tests passed, and all 55 focused Remote/operation-contract/subagent tests passed. Root TypeScript, focused ESLint, and git diff --check also passed; the working tree is clean. Native macOS acceptance and mobile suites were not rerun for this desktop-only delta. Exact-head hosted CI was still queued/running when inspected; this approval does not establish merge readiness.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at 6cad950b, focused on the refreshed native artifacts and their packaging contracts.
- Refreshed native binaries: Updated the macOS universal, Linux x64, and Linux arm64 worktree-remover prebuilts to include the previously reviewed session-audit implementation.
- Synchronized manifests: Updated binary checksums, source hashes, and build timestamps while preserving the other helper entries.
- Documented artifact validation: Recorded the deterministic CI manifest failure and the required prebuilt-refresh checks.
Validation: all 6 native artifact tests and all 36 native remover tests passed. Direct execution of the checked-in Linux x64 binary verified active/empty session detection, production fixture isolation, and invalid-input refusal. Architecture checks passed for all artifacts; both Linux binaries require no GLIBC version above 2.34. git diff --check passed and the working tree is clean. macOS and arm64 binaries were inspected but not executed on this x64 Linux runner. Exact-head hosted CI was still queued/running when inspected; approval does not establish merge readiness.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at 72502886, focused on integration with the latest main and the terminal-drain dependency contract.
- Merged latest
main: Integrateddd2c0d03while preserving the root test chain and existing Remote protocol, and updated implementation notes. - Aligned the first-message fixture: Made
closeWorkspaceTerminalsreturn a promise in the newly merged worktree test harness.
Validation: root TypeScript, focused ESLint, all 78 first-message/terminal/Remote/operation-contract tests, and all 36 native remover tests passed. The focused tests passed after building their required native helper. Exact-head hosted CI was still queued/running when inspected; this review does not establish merge readiness.
gpt-6.1-sol ｜ 𝕏
…ree-process-cleanup # Conflicts: # prebuilt/native/darwin-universal/manifest.json # prebuilt/native/linux-arm64/manifest.json # prebuilt/native/linux-x64/manifest.json
|
/hermes review full |
…ree-process-cleanup # Conflicts: # prebuilt/native/darwin-universal/manifest.json # prebuilt/native/linux-arm64/manifest.json # prebuilt/native/linux-x64/manifest.json
…ree-process-cleanup
|
/hermes review full |
…ree-process-cleanup
…ree-process-cleanup
…ree-process-cleanup
…ree-process-cleanup
Summary
Advance issue #118 with a bounded process-before-files deletion phase. Explicit managed-worktree removal signals Aiden-owned PTYs, waits for shell exits, and audits their POSIX sessions for surviving background jobs before dirty-state capture, snapshot, or Git deletion. A timeout, surviving job, or unavailable process inventory preserves the checkout and gives an actionable retry message. Pending exits and session identities remain tracked across retries. An invalid PTY PID is a hard removal hold for the running service because there is no session identity to audit; Aiden sends no signal through that handle and directs manual inspection.
The existing workspace mutation gate prevents a new Aiden terminal from starting during deletion. The existing operation, generation, schedule, snapshot, checkout identity, quarantine, and recovery gates remain in place. This PR does not enable an automatic sweep or claim control over independently detached or externally launched processes.
Remote Git uses the existing
git_capability_deniedresponse. A process refusal before Git mutation clears its idempotency admission so the same key can retry after the job exits. iOS and Android already display the server error message; no wire revision or client code change is needed.Validation
npm run test:worktree-remover:native(36 tests)Focused terminal, Remote Git, application-service, lifecycle, and operation-contract tests (105 tests); terminal coverage gate and subagent phase-3 contract suite (23 tests)
npm run type-check, CLI typecheck, focused ESLint, andgit diff --checknpm run test:cli(99 tests, including built CLI managed-worktree removal)iOS
AidenWorkspaceEnvironmentTestson iPhone 17 Pro iOS 27.0 simulator, unsigned (12 passed)Android
:app:testDebugUnitTest(934 JVM tests)Live macOS node-pty probe: a background child in another process group survived shell exit and lost its controlling TTY; the native session audit still reported
active.Linux native remover suite in a Debian container (36 passed), including filtered procfs fixtures; a privileged private mount namespace probe with
hidepid=2refused a live hidden cross-UID session member.Newer Fedora x64 and Debian arm64 compiler checks produced no glibc symbols above 2.34; Linux contract suite passed (154 passed, 2 skipped).
node --test scripts/native-helpers.test.mjs(6 passed); the macOS universal, Linux x64, and Linux arm64 checked-in worktree-remover prebuilts and manifests now match this source. The built CLI copied the verified macOS prebuilt byte for byte.After merging
origin/maindd2c0d03into the PR branch: 160 focused desktop, Remote, first-message worktree, and terminal tests; 36 native remover tests; 6 prebuilt-manifest tests; terminal coverage gate; root typecheck and focused lint; 99 CLI tests. The merged root test chain matches main, and this PR changes no Remote protocol revision.After merging
origin/main17d51d472(Android simulator viewer) into the PR branch: root typecheck, 36 native remover tests, 57 focused terminal/worktree/first-message tests, 6 prebuilt-manifest tests, Android:app:testDebugUnitTest, andgit diff --checkpassed. The root test chain still matches main; the Remote protocol revision remains unchanged.After merging
origin/mainb1e6054a(including Fix native foreign-run attach and reconnect recovery #498, Buffer Android foreign-run output by frame #503, Fix draft-chat E2E focus expectation after workspace action reveal #506 and Bind edit_file writes to verified inode across symlink swaps #501): root typecheck, 36 native remover tests, 160 focused desktop/Remote tests, 6 prebuilt-manifest tests, terminal coverage, 99 CLI tests, Android JVM tests, focused iOS simulator XCTest, E2E typecheck/build and the repaired sidebar E2E case all passed. The full tool-output chain passed 214 tests with CI Node 22.22.3; local Node 26 has different nativefs.globsymlink results. The three manifests retain both the worktree remover and Bind edit_file writes to verified inode across symlink swaps #501 file mutator entries. The root test chain matches main and Remote protocol revision remains 32.After merging
origin/mainee218d1f(Prevent parent write_file symlink-retarget writes #504) and then515de6738(0.57.0 release): root typecheck, 36 native remover tests, 160 focused desktop/Remote tests, 6 prebuilt-manifest tests, native file mutator suite (31 passed, 3 platform skips), terminal coverage, 218 tool-output tests on CI Node 22.22.3, and 99 CLI tests passed. The root test chain matches main and Remote protocol revision remains 32. The release merge had no conflicts; both native helpers remain present in the manifests.The first hosted Headless CLI on Linux job failed at
1db6ae1cbecause the standalone CLI adapter returnedvoidfor the new async terminal drain contract. Commitb97df808fixed that adapter. Fresh hosted checks are required on this head.The Linux x64 package gate on
37d0e7df9then found a GLIBC_2.38 symbol introduced by the new native PID parser. The follow-up replaces that call with the existing bounded digit parser; it also rejects filtered/procinventories found in review. Fresh hosted packaging checks are required on this head.The
renderer-otherjob on6cad950b9failed its checked-in native prebuilt manifest test because the remover source changed without rebuilding the three prebuilt binaries. This is deterministic artifact drift, not a flaky test. The refreshed binaries and manifests pass the focused artifact test locally; fresh exact-head CI is required.The latest-main merge exposed one first-message worktree test fixture that still returned
voidfor the terminal drain dependency; it now returns a promise. Local typecheck and that focused suite pass. Hosted CI and reviews must be evaluated on the post-merge head.Exact-head CI run 38829367039 found one Electron E2E shard 3 failure (also failed its Playwright retry):
tests/e2e/draft-agent-chats.spec.ts:205expected the workspace New chat button opacity to be zero after clicking the workspace header, but the header retained focus andgroup-focus-withinkept the button visible. The spec and sidebar UI are unchanged by this PR. The baseline fix merged through PR #506 at896f31aeand is included in this branch. The other CI jobs on the old head passed; new exact-head CI and reviews are required on6accb4035.Prior-head CI run 38847791193 completed green on
6accb4035, including Electron E2E, Android, iOS, and Linux packaging. Fresh exact-head CI is required fore55d71a61.Hermes Review Bot could not reach a verdict on exact head
6accb4035because itsagy/gemini-3.8-flash-highengine returnedUNAVAILABLE503 during streaming (failure comment). This is a review-service failure, not a code finding. One fresh full review was requested on the same head with/hermes review full. That follow-up also failed in the engine before a verdict; its safe summary saysagy failed (3)with incomplete output, not an explicit 503. No code finding was produced. A new-head review remains required after the nextmainintegration.Hermes also failed in its engine before a verdict on exact head
e55d71a61(bot failure report, updated for this head). Its safe summary again reportsagy failed (3)with incomplete output; no code finding was produced. One full review retry on this same head has been requested. The bot gate remains pending, and this PR must not merge without a successful exact-head review.Exact-head CI run 38854074444 completed green on
e55d71a61, including all Electron E2E shards, iOS, Android, CLI, and Linux packaging. Pullfrog passed. The full Hermes retry requested on this head did not yield a verdict. Read-only Hermes service logs identifyRESOURCE_EXHAUSTED (code 429): Individual quota reachedacross review sessions; its reported quota reset is about 15:34 UTC / 11:34 a.m. EDT on October 16. No further manual retry should run before that reset. A successful Hermes verdict on this exact head is still required before merge.After main advanced, the branch merged
716636cf9(#508 and #509),9deb63f52(#510 and #511), and264b537a4(#512, Remote revision 33) without conflicts or rebasing. The rootpackage.jsontest chain matches main; the revision-33 protocol and fixture files match main. Local gates passed: root typecheck; 36 native remover and 6 prebuilt-manifest tests; 31 isolated lifecycle tests; 99 CLI tests; 119 focused #510/#511 tests; 215 revision-33 Remote protocol/router/Bot-session/operation tests; Android JVM tests; and 175 focused iOS Remote client/Bot-session/phase-0 simulator tests (Xcode exit 0). Xcode then spent ten minutes trying to collect simulator diagnostics and reported a diagnostic-collection timeout after the passing tests.The Mac was under extreme unrelated load (load average above 250) during two combined desktop/Remote runs. The first hit a four-second Git lifecycle timeout while the CLI build ran; the isolated lifecycle suite passed 31/31 and the CLI suite passed 99/99 when rerun alone. The second combined run passed 218/221 cases but expired a Bot Files link, expired a Remote Git snapshot, and hit a first-message Git four-second timeout after large wall-clock delays. These local failures remain recorded; they were not weakened or retried repeatedly. Fresh exact-head hosted CI on
60e30da04is required as the clean-run gate, along with Pullfrog and a successful Hermes verdict on that head. Do not merge on the prior green CI run.Hermes subsequently recovered and reviewed the prior
e55d71a61head with no findings at 16:19 UTC on October 16. That verdict predates the latest-main merges. The current60e30da04head still needs its own successful Hermes review; exact-head CI run 38951048254 and Pullfrog are in progress.Hermes reviewed exact head
60e30da04with confidence 5/5 and no findings at 19:07 UTC on October 16. Pullfrog passed on the same head, and there are no unresolved review threads. Exact-head CI run 38951048254 is still in progress; merge remains gated on its final required result.Main subsequently advanced to
5661bae0ethrough PR #496. This branch is intentionally held at60e30da04until the coordinating task assigns the next delivery slot; current-head CI remains useful evidence but will need a fresh run after the eventual latest-main merge. No rebase, force-push, or merge is planned during this hold.CI run 38951048254 completed green on
60e30da04across all jobs, including all Electron E2E shards, iOS, Android, CLI, Linux packaging and required CI. This head also has passing Pullfrog and an exact-head Hermes review with no findings. Because main has advanced since its base, this is prior-base evidence; latest-main integration and a new exact-head run are still required at the coordinated delivery slot.At the assigned delivery slot, the branch merged latest main
6c2b44bb0(#496, #505 Pi 1.3.0, #494, and #499) without conflicts or rebase. The root package test chain matches main, Remote contract revision remains 33 with no PR wire change, and the native manifests validate both the worktree remover and lease-flock helper. Local validation after refreshing dependencies passed: root typecheck; full lint and build; 36 native remover tests; 6 manifest tests; 160 focused desktop/Remote tests; 122 focused Bot lease, coding-tools, and device tests under CI Node 22.22.3; 106 CLI tests; terminal coverage; and 220 tool-output tests under CI Node 22.22.3. A direct pre-build invocation of the new Bot lease tests failed becauseaiden-lease-flockhad not yet been built, and the coding-tools glob oracle differs under local Node 26. The build-first helper plus CI Node 22 run passed 122/122. Fresh exact-head CI, Pullfrog, and Hermes are required on pushed head31da26cd6; the prior green run is not a substitute.Hermes reviewed current head
31da26cd6with confidence 5/5 and no findings at 23:38 UTC on October 16. Pullfrog passed on that head, and no review threads are unresolved. Exact-head CI run 38982132155 remains in progress; merge is still gated on its final required result.Exact-head CI run 38982132155 completed green on
31da26cd6, including every E2E shard, iOS, Android, CLI, Linux packaging and required CI. Pullfrog and Hermes passed on that head, with no Hermes findings or unresolved review threads. The coordinator merged PR #500 as47b7a5039on October 16 at 8:14 p.m. EDT. Installed-app and terminal acceptance remain separate from this source merge.Remaining issue #118 scope
Owner kinds, opt-in retention and conservative eligibility with visible skip reasons, idle/snapshot GC, setup script, Settings/restore UI, branch namespace/base-ref picker, and CoW remain open. The plan records eligibility blockers, including dirty/unpushed/unknown-ignored content, open PR head or base, uncertain ownership, and active processes. Existing worktrees remain manual; this PR enables no automatic deletion.