Repository navigation
Fence CLI and Bot stale lease recovery - #499
Conversation
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Two lifecycle regressions need fixing before merge: reopening a closing Bot returns an unusable session, and foreground serve loses recycled-PID recovery. The new child fixture also fails the required lint check.
Reviewed changes: Reviewed the complete CLI and Bot lease-fencing change, including acquisition, stale recovery, teardown, and multiprocess tests.
- Permanent SQLite guards: CLI leases and Bot profiles hold owner-private SQLite transactions, with canonical in-process reservations and guarded diagnostic metadata cleanup.
- Daemon lifecycle: Serve cleanup checks ownership under the guard, startup probes the socket listener, and the child owns stale socket removal.
- Bot teardown: Harness entries remain tracked until close succeeds, and shutdown retains the profile guard when teardown fails.
- Behavioral coverage: Added contention, process-death, stopped-owner, damaged-guard, and replacement-owner tests, plus operational documentation.
Validation: CLI build, CLI type-check, all 102 CLI tests, and all 201 Bot runtime tests passed. Both behavioral findings were reproduced separately; scoped ESLint also reproduced the fixture error reported by CI. Packaged Electron was not exercised locally.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
Important
CLI lease recovery now crashes on null diagnostic metadata. The two previously reported lifecycle concerns remain open.
Reviewed changes: Reviewed the changes from 918c41de to 312f830e, with the complete current PR diff as context.
- Replaced SQLite guards: Introduced native
flockacquisition through an inherited descriptor retained by the parent process. - Distinguished new metadata: Added
flock-v1owner records and guard-authoritative recycled-PID recovery for CLI leases, Bot profiles, and daemon lifecycle commands. - Fenced Bot shutdown: Closed mutation admission synchronously and drained admitted operations before releasing profile ownership.
- Integrated native distribution: Added verified prebuilts, CLI staging, desktop packaging, signing rules, and package verification.
- Extended behavioral coverage: Added raw guard-read, recycled-PID, and shutdown-drain cases, and fixed the child fixture's timer declaration.
Validation: CLI build and type-check passed; 104 CLI tests, 205 Bot runtime tests, 62 CI-policy tests, and 33 helper/distribution tests passed. One platform-specific archive test was skipped. Scoped ESLint passed. Reproduced the new metadata failure and reconfirmed the two existing lifecycle findings; resolved the fixed lint thread. Packaged signed Electron was not exercised locally.
gpt-6.1-sol ｜ 𝕏
Hermes Review BotConfidence: 5 Engine: SummaryExclusive CLI leases and Bot runtime profiles are now fenced by permanent OS Confidence Score: 5/5Full end-to-end inspection completed: kernel 📁 Important Files Changed
FindingsNo findings. Sequence DiagramsequenceDiagram
autonumber
actor App as Aiden Process
participant Guard as File Descriptor (OFD)
participant Helper as aiden-lease-flock
participant FS as File System
App->>Guard: openSync(<file>.lease.lock, O_CREAT | O_NOFOLLOW, 0600)
App->>Guard: fstatSync(fd) (verify regular file, nlink==1, 0600, UID match)
App->>Helper: spawnSync(helper, stdio[3]=fd)
Helper->>Guard: flock(3, LOCK_EX | LOCK_NB)
Helper-->>App: exit code 0 (acquired) or 10 (contended)
alt Acquired (code 0)
Note over App,Guard: Parent retains open fd; OS flock stays active on OFD
App->>FS: rmSync(stale lock dir / socket if present)
App->>FS: atomicJson(owner.json, { protocol: "flock-v1", pid, token })
else Contended (code 10)
App->>Guard: closeSync(fd)
App-->>App: Throw lease contention error
end
[]
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes: Reviewed the changes from 312f830e to 33f5954c, using the complete current PR diff as context.
- Serialized Bot reopening: Routed opens through the lifecycle queue, retried failed teardown before reuse, and checked entry identity before idle cleanup closes a session.
- Restored foreground recovery: Reinstated guarded stale-lease recovery before foreground serve acquires ownership, including legacy metadata naming a recycled foreign PID.
- Validated diagnostic metadata: Treated
nulland other non-object owner values as stale instead of crashing acquisition. - Staged CI prerequisites: Added the lease helper to the renderer-other lane and coverage for both Bot-consuming lanes.
- Extended behavioral coverage: Added usable close/open overlap, failed-close retry and guard retention, null-owner recovery, and foreground startup tests.
Validation: All 106 CLI tests, 207 Bot runtime tests, 63 CI-policy tests, and 23 focused Telegram/renderer tests passed. CLI build, root and CLI type checks, scoped ESLint, and git diff --check passed. Packaged signed Electron was not exercised locally, and several exact-head hosted CI jobs remain pending.
gpt-6.1-sol ｜ 𝕏
|
/hermes review |
|
/hermes review |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes: Reviewed the three new main integrations from fbfac1c8 to 9df88d34, with the complete current PR diff and prior feedback as context.
- Integrated Remote authority fixes: Checked host-owner Bot Files admission, device-bound handles, revocation checks, and the corresponding CLI audience wiring.
- Integrated Bot and focus updates: Checked Web Search grants and editor state, keyboard/pointer modality behavior, and preservation of the lease-helper CI prerequisites.
- Integrated attachment contracts: Checked revision 33 Bot uploads, replay-safe attachment reservations, and native-client compatibility at the lease-runtime boundary.
- Verified merge integrity: Confirmed all three commits exactly match Git's conflict-free automatic merge trees, with no manual changes to lease recovery or teardown.
Validation on 9df88d34: 207 Bot runtime tests, 107 focused Remote tests, 119 Bot authority/editor and input-modality tests, 63 CI-policy tests, and 53 helper/packaging tests passed; one platform-specific archive test was skipped. Root type-check and git diff --check passed. Standalone CLI bundle tests, native mobile execution, and signed packaged Electron were not rerun locally. Exact-head hosted CI remains in progress; this review does not establish merge readiness.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes: Reviewed 9df88d34..066cbb95, with the complete current PR diff and prior feedback as context.
- Synchronized worktree Retry setup: Added an assertion that the explicit branch name is saved before Escape closes the menu, preserving the existing failure, Retry, worktree creation, and reply assertions without changing timeouts or retries.
- Verified main integration: Confirmed the device-host lifecycle fixes from
mainmatch Git's automatic merge tree exactly, with no manual changes to lease recovery or Bot teardown.
Validation: All 85 focused draft-branch and device-host/service tests passed. E2E type-checking, scoped ESLint, Playwright collection of all 16 top-bar scenarios, and git diff --check passed. Electron execution was not rerun in this Linux review environment. Exact-head hosted CI remains pending, so this review does not establish merge readiness.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
Important
The test-only follow-up widens a flaky startup deadline without synchronizing fixture readiness, contrary to the repository's flaky-test policy.
Reviewed changes: Reviewed 066cbb95..f3a1fa24, with the complete current PR diff and prior feedback as context.
- Added PID-file polling: Started the timeout assertion concurrently, waited for a valid descendant PID, then awaited the command result and checked process exit.
- Raised fixture deadlines: Increased the command timeout from 250 ms to 2 seconds and the elapsed-time ceiling from 1.5 to 3.5 seconds; production lease and command-runner code remained unchanged.
Validation: Scoped ESLint and git diff --check passed. The vendor suite passed 28/31 locally under Node v24.19.0; three descendant-exit checks failed with Process ... did not exit. One focused rerun confirmed that the changed scenario's descendant was already an unreaped zombie under sandbox PID 1, not a live escaped process. These local results do not establish a new process-cleanup regression or exact-head CI success.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes: Reviewed f3a1fa24..8586738e, using the complete current PR diff and prior feedback as context.
- Synchronized fixture readiness: Held the parent command clock until the delayed descendant installed its signal handler and published its PID, then exercised timeout cleanup with the original 250 ms deadline and 1.5-second settlement bound.
- Preserved bounded verification: Kept readiness and process-exit polling on real timers and retained the descendant-exit assertion before final cleanup.
- Verified main integration: Confirmed the merge containing process-cleanup updates and Pi 1.3.0 exactly matches Git's automatic merge tree, with no manual changes to lease recovery or Bot teardown.
Validation: All 207 Bot runtime tests and 63 CI-policy tests passed; scoped ESLint and git diff --check passed. The vendor suite passed 28/31 under sandbox Node v24.19.0, with three Process ... did not exit failures. One focused rerun confirmed the changed scenario's descendant was an unreaped zombie under sandbox PID 1, not a live escaped process; its timeout and prompt-settlement assertions completed successfully. Native mobile, Electron execution, and signed packaging were not rerun locally. Exact-head hosted CI remains incomplete, so this review does not establish merge readiness.
gpt-6.1-sol ｜ 𝕏
Closes #405 and #392.
Change
*.lease.lockguard for CLI leases and Bot profiles. A small native helper takes an OSflockon an inherited file descriptor; the parent keeps that open description through the lease. A raw read/open/close of the guard cannot drop the lock, and a process crash releases it.protocol: "flock-v1"; a free guard permits recovery even if the recorded PID has been reused. Malformed JSON values, includingnull, are reclaimable. Live legacy records stay protected unless the PID is verifiably foreign.Behavioral checks
nullowner recovery, foreground serve with a legacy recycled foreign PID, and serve cleanup racing a replacement owner.npm --prefix packages/cli test(106/106),npm run test:bot-runtime(207/207),npm run test:bots(824/824),npm run test:ci-policy(63/63), Telegram Bot reply outbox (14/14), Bot routines (9/9), root and CLI type checks, scoped ESLint, CLI and Electron builds,git diff --check. Package/signing/native helper tests (54/54) passed on the prior commit; the follow-up changes do not touch their implementation.CI observation
The renderer-other job on commit
312f830ebfailed four Telegram Bot tests because that lane had not builtaiden-lease-flock(spawnSync ... ENOENT); the lane prerequisite is now added and the focused suite passes locally. The same job reportedrenderer/main/bots/bot-routines.test.tsxkilled bySIGKILLafter 92.2 seconds in the large shard, with no cause in the job log. Its standalone local run passed 9/9. This remains a possible flaky or resource failure for exact-head CI to classify; no timeout or retry behavior was changed.Upgrade boundary
Stop older CLI processes, daemons, and Bot hosts before upgrading a shared directory or profile. Older binaries do not honor this guard, so mixed-version exclusion is not established. Guard files must remain in place on a local filesystem with working OS locks.
iOS baseline failure and merged fix
The iOS job on pre-merge head
33f5954c7failedAidenBotChatActivityTests.testAPickMadeWhileASaveTimedOutGoesOutOnceTheRefreshShowsThatSaveLanded(). This matched the timeout/cancellation ordering defect fixed in PR #498 ata4366f62: publish the timeout before cancelling work. That fix reached main in6bc994a5and is now included in this PR by merge commit82236f7b2. The old iOS job was not rerun. A simulator clone later reportedNSMachErrorDomain -308and xcodebuild exited 65; the log does not establish whether that simulator error was caused by the test failure.Validation after merging main
npm --prefix packages/cli test106/106;npm run test:bot-runtime207/207;npm run test:bots824/824;npm run test:ci-policy63/63; Telegram Bot reply outbox and Remote protocol 51/51; package/signing/native helper tests 54/54.git diff --checkpassed. The root test chain contains all scripts and test files from both merge parents; the Remote contract remains revision 32 across server and fixture.82236f7b2; fresh required CI and review are needed after the next main integration.E2E baseline failure fixed on main
The exact-head Electron E2E shard 3 job on
82236f7b2reported 90 passed, 1 skipped, and 1 flaky. The sole failing attempt wastests/e2e/draft-agent-chats.spec.ts:205: at line 226 the workspace plus button's opacity was expected to be0but remained1; Playwright's retry passed, while--fail-on-flaky-testskept the job red. The row's expand control retained focus after the pointer moved away, so the focused-row action remained visible. PR #506 corrected that baseline test expectation without changing renderer behavior and merged to main at896f31ae. This job was not rerun and no E2E code was changed in #499. PR #501 merged to main atb1e6054a. Its changes and the #506 baseline E2E repair were integrated into #499 by merge commitcd5781fce. The prior flaky job remains documented here; fresh exact-head CI must establish the current result. PR #504 then merged to main atee218d1fand was integrated into #499 by merge commitd0ed5498b.Validation after integrating #501 and #506
lease-flockand updatedsubagent-file-mutatorentries. Native-helper and package/signing tests passed 34/34.main/services/coding-tools.test.ts:2396becausefs.glob("**/*")omitted safe symlink descendants that Aiden returned; rerunning with the pinned CI Node passed.cd5781fce; its hosted CI results are below.Hosted CI observation on prior head (
cd5781fce)main/services/bot-runtime/live-projection.test.ts:1429, “while classifying fails, a cached routine stays hidden and the person's classified message stays listed; the new one waits, then appears.” Its timing assertion at line 1452 expected the first-to-third classification retries to span at least 295 ms, but saw 3 calls sooner. This file is unchanged by Fence CLI and Bot stale lease recovery #499; the focused Bot runtime suite passed locally 207/207. The workflow completed with this failure. This head was superseded by the Prevent parent write_file symlink-retarget writes #504 integration, so no stale-head rerun was started. The flaky failure remains a bug even if that rerun passes; no timeout, retry, or assertion has been weakened.--fail-on-flaky-tests: 210 passed, 9 skipped, 1 flaky. The first attempt oftests/e2e/settings-model-picker.spec.ts:104timed out at line 267 waiting for one[cmdk-item]after opening the List tab (expected 1, received 0); its Playwright retry passed. The test is unchanged by Fence CLI and Bot stale lease recovery #499. The workflow completed with this failure; the superseding Prevent parent write_file symlink-retarget writes #504 integration will receive a full new CI run. The flaky result remains documented.git diff origin/main...cd5781fcecontains neither failed spec, the Bot live-projection classifier, nor the renderer model-picker/settings implementation. The model-picker failure occurs on reopening its List tab after an unsuccessful search; no direct lease-helper call is in that flow. The Linux log also reports Bot/Telegram restore warnings, but it does not establish a causal link to the missing picker row.Validation after integrating #504
ee218d1f; merged it into Fence CLI and Bot stale lease recovery #499 without rebase or force-push. The merge commit isd0ed5498bc82a16d79adc1ffb3ee5446ba883b71. The three generated native manifests were regenerated from the checked-in binaries and current sources, preserving bothlease-flockand Prevent parent write_file symlink-retarget writes #504’s updatedsubagent-file-mutator.git diff --checkalso passed locally.pretest:serial,pretest:coverage, andtest:subagentssegments from both merge parents. Aiden Remote remains revision 32; Prevent parent write_file symlink-retarget writes #504 changed no Remote contract files.d0ed5498b, CI run 38852933846 was still active when Release Aiden Agent 0.57.0 #507 advanced main. The runtime-subagents lane passed on that head after the earlier retry-timing failure. Pullfrog passed with no unresolved threads. Hermes failed during its engine before a verdict, and the requested/hermes reviewfollow-up failed the same way. Neither is a code finding or an accepted review verdict.Validation after integrating #507
Release Aiden Agent 0.57.0 #507 advanced main to
515de6738for release version 0.57.0. Merged it into Fence CLI and Bot stale lease recovery #499 asfbfac1c88without rebase or force-push; there were no conflicts. The root test scripts and the three checkedpretest:serial,pretest:coverage, andtest:subagentsvalues match both merge parents. Aiden Remote remains revision 32, unchanged by Release Aiden Agent 0.57.0 #507.Under CI-pinned Node 22.22.3, local Bot runtime passed 207/207, Bots passed 824/824, CLI passed 106/106 after rebuilding, and root and CLI type checks passed.
git diff --checkpassed.Prior head
fbfac1c88: exact-head CI run 38854202446 passed at 06:40 UTC; release-consumer contract run 38854202360 passed. The Linux x64 job passed its deterministic Electron E2E gate, including the model-picker suite that failed on Fence CLI and Bot stale lease recovery #499's prior head. Separately, #494's Linux x64 job reported the same missing[cmdk-item]symptom on both initial attempt and retry; this cross-PR result does not establish lease causation. Pullfrog passed and no review threads are unresolved. Hermes failed in its engine at 05:15 UTC after initialization (agy failed (3)) without a verdict. The single exact-head/hermes reviewretry was requested after a cooldown at 05:50 UTC and failed in the same engine phase by 05:51 UTC, again without a verdict. No further automatic Hermes retries are planned on this head absent concrete service-recovery evidence. The PR remains blocked on a real Hermes verdict.At 06:13 UTC, Hermes posted another engine-phase failure on the same head without a new
/hermes reviewcomment; its status and updated comment still contain no verdict. iOS passed; desktop runtime-subagents and macOS E2E/build jobs remained pending. The review gate stays closed.The exact-head required CI gates (
CI required, Linux x64/arm64, and Fedora RPM) all passed. Hermes posted further same-head engine failures without new/hermes reviewcomments, most recently at 09:59 UTC in its updated failure comment; it still has no verdict.Read-only Hermes service-log investigation found AGY
RESOURCE_EXHAUSTED (429): Individual quota reachedacross review sessions from 04:43–10:36 UTC. The reported quota reset is approximately 15:34 UTC (11:34 a.m. EDT) on October 16. Hermes is using its persisted backoff; the public PR comment exposes onlyagy failed (3). No further manual review trigger is planned before the reset, and a real exact-head review verdict remains required.Validation after integrating #508, #509, #510, #511, and #512
264b537a. Merged main into Fence CLI and Bot stale lease recovery #499 without rebase or force-push; the resulting commit is9df88d34c6bb170c4be13e484d9a53ba2fae8614. There were no conflicts. The root package test scripts retained all keys and thepretest:serial,pretest:coverage, andtest:subagentsvalues from both merge parents. Remote revision 33, its fixture, and iOS assertion agree; Android’s revision floor remains compatible.AidenRemoteClientTestandAidenBotContractTestpassed. On an iPhone 18 Pro simulator (iOS 27.0),AidenBotSessionAttachmentTests,AidenBotActivityContractTests, and the Remote fixture test passed 19/19 with zero skips; this is simulator verification, not physical-device acceptance.git diff --checkpassed.chat-shell-interactions.spec.tsrun had 24 passes and one failure in the unchanged “highlighted menu rows keep readable content and destructive rows stay red” test: after hovering Recent, its background remained transparent instead of settling on the accent fill within 10 seconds. A single isolated rerun of that exact test passed. This remains a flaky test; no timeout, retry, or assertion was weakened, and no lease causation is established.9df88d34, Electron E2E shard 3 in run 38950674852 failed under--fail-on-flaky-tests: 90 passed, 1 skipped, 1 flaky. The first attempt oftests/e2e/chat-top-bar.spec.ts:327, “Retry chosen after leaving a failed worktree send creates the worktree and starts the reply,” could not find[data-worktree-send-failure]at line 350 within 20 seconds after Enter. Playwright retry passed. The failure attachment showed only “Checking simulator setup…” and “Start Live” notices plus unrelated Bot/Telegram restore warnings; it does not show whether the send reached worktree creation. At the time of that failed run, neither the spec nor its renderer flow had been changed by Fence CLI and Bot stale lease recovery #499. This flaky result remains a bug even though the retry passed; no timeout, retry, or assertion was weakened. The workflow is terminal with this as its sole test failure; the dependentCI requiredgate failed. The branch was behind main after Fence Device Hub restarts after stop #496, so this stale-head job was not rerun; the new head has fresh CI. PR #501’s earlier E2E shard 3 showed the same spec and missing failure notice on its first attempt, with a passing retry. After locally merging Fence Device Hub restarts after stop #496 into Fence CLI and Bot stale lease recovery #499, this focused spec passed 3/3 repeats without a Playwright retry. The available logs do not establish whether the failed attempts reached worktree creation; a product race remains unconfirmed.9df88d34, CI run 38950674852 finished red only for the E2E shard 3 flake documented above; the release-consumer run, Pullfrog, and Hermes full review passed. No review threads were unresolved.Validation after integrating #496 and the worktree-send readiness assertion
5661bae0. Merged it into Fence CLI and Bot stale lease recovery #499 without conflicts, rebase, or force-push as4fc2044e6b70d39f14bce454c1702b81bb354216. Roottest,pretest:serial,pretest:coverage, andtest:subagentschains match both merge parents; every script key is retained, and the lease branch’spretest:botshelper prerequisite remains. Remote contract revision 33 remains aligned across server, fixture, and iOS assertion.066cbb951ebfcfe0a485a2c5cee8a37b1fde4979adds one E2E setup assertion: wait until the branch-name row visibly showsaiden/takenbefore closing the picker and sending. E2E type-check passed; that focused flow passed 5/5 repeated local runs without retry on this exact head. The pre-assertion flow also passed 3/3 locally, so local repetition alone does not prove the CI flake is fixed. No timeout, retry count, or--fail-on-flaky-testspolicy changed.066cbb951, renderer-other job 117203214329 failed 1 of 3,552 tests:scripts/vendor-cua-driver.test.mjs:388, “bounded vendor commands kill descendants that retain an exited leader’s stdio.” At line 418 it gotENOENTreading the temporarydescendant.pidfile; the fixture’s child leader writes that file after spawning its descendant under a 250 ms bounded-command timeout. This source is unchanged by Fence CLI and Bot stale lease recovery #499. The focused test passed locally 1/1 on Node 22.22.3. The log does not establish a lease/product failure or which scheduling step missed the deadline. The workflow was superseded and cancelled after the next push. Linux x64/arm64 andCI requiredgates failed because their dependencies were cancelled; no further test failure was established. No stale-head rerun was started. Preserve this symptom and job link even if later CI passes.Prior head and vendor-command fixture review
f3a1fa24fb05cc6034f9aae9269a7e1f6170449dis a one-commit descendant of066cbb951; main was5661bae0at that push. The commit changes onlyscripts/vendor-cua-driver.test.mjs. A local Node 22.22.3 run of the focused descendant-cleanup test passed 1/1, andgit diff --checkpassed. No branch push was made from this monitoring chat.ENOENTCI flake. Pullfrog's unresolved P2 thread confirms that PID polling begins afterrunBoundedCommand()arms its deadline, leaving the startup race intact; the larger timeout conflicts with AGENTS.md's flaky-test rule. A passing run alone will not establish that the race is fixed; retain the prior failure record above. No production command timeout, retry count, or--fail-on-flaky-testssetting changed.Current head after #494 and #505
8586738e4c1e2e525b7a182610c8252bf836b620contains corrective commitc64bbe87cand a merge of main1575fda4f(Fix run_command cleanup after shell closes #494 process cleanup and Upgrade Pi to 1.3.0 in lockstep across the app and the CLI #505 Pi 1.3.0). The merge had no conflicts, rebase, or force-push. Roottest,pretest:serial,pretest:coverage, andtest:subagentschains match both parents; all script keys survived and the lease helper remains inpretest:bots. Remote revision 33 remains aligned in TS, the shared fixture, and iOS; Android's revision floor remains compatible.ENOENTflake remains documented above. The product command timeout, retry count, and--fail-on-flaky-testspolicy are unchanged. This addresses Pullfrog's prior P2 finding. Pullfrog's exact-head review confirmed the synchronized fixture readiness and original 250 ms/1,500 ms bounds with no new issues; the prior thread is resolved.git diff --check. The full vendor suite also passed 31/31 before the merge. These are local results. Fresh exact-head CI 38978785667 is still running. Release-consumer 38978785671 passed. Pullfrog found no new issues on this head and the prior P2 thread is resolved. Hermes completed a full 5/5 exact-head review with no findings. CI remains the open gate.