fix(autorun): correct worktree attribution + add absolute task watchdog#1145
fix(autorun): correct worktree attribution + add absolute task watchdog#1145chr1syy wants to merge 5 commits into
Conversation
Two related Auto Run robustness fixes surfaced by a web-desktop launch of a two-document playbook with auto-worktree creation. 1) Worktree attribution across same-repo agents (PR RunMaestro#946 follow-up) - worktreeSpawn: re-mark the resolved worktree path as recently-created AFTER worktreeSetup (create-new only). The original 10s dedup TTL started before the potentially slow `git worktree add`, so the chokidar discovery could fire during the getBranches/buildWorktreeSession window after the mark aged out, letting a sibling agent watching the same basePath adopt the worktree under the wrong parent. Re-marking also covers the existingPath reassignment the original mark never did. - useWorktreeHandlers: scope onWorktreeDiscovered dedup per-parent instead of globally by cwd. Each same-repo agent runs its own watcher and fires its own discovery event, so an externally-created worktree now fans out: every agent in the cwd gets its own child instead of the first watcher claiming it globally and the others silently dropping it. Maestro-spawned worktrees still attach only to the launching agent (the dedup mark suppresses sibling watchers). 2) Absolute Auto Run task watchdog The per-document batch loop awaits processTask, whose only recovery was the silence-based inactivity watchdog. A stuck-but-chatty agent that keeps emitting output (resetting the silence timer) but never finishes defeats it, hanging the whole multi-document run with no advance to the next document. - Add an absolute wall-clock per-task cap (autoRunMaxTaskDurationMin, default 480 min, 0 = unlimited) that force-kills regardless of output and resolves the task as watchdog-timeout, which the batch loop already handles by terminating the document and moving on. - Wire the new setting through metadata, store, useSettings, the General tab UI (alongside the inactivity timeout), and searchable settings. Tests: worktree attribution (per-parent fan-out, mark-skip, idempotency, cross-repo reject), worktreeSpawn re-mark robustness, and batch watchdog (absolute cap fires on chatty-stuck, inactivity still fires on silence, both unlimited = no watchdog). Updated the existing mark-order assertion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an Auto Run max-task-duration setting and watchdog path, plus parent-scoped worktree deduplication with refreshed recently-created marking after worktree setup. ChangesAuto-run max-task-duration watchdog
Worktree dedup per parent session
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR improves Auto Run watchdogs and worktree attribution. The main changes are:
Confidence Score: 4/5The worktree attribution path can still route Auto Run work to the wrong parent agent.
src/renderer/hooks/worktree/useWorktreeHandlers.ts, src/renderer/utils/worktreeSpawn.ts, src/renderer/stores/settingsStore.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[External worktree discovered] --> B[Watcher for agent A]
A --> C[Watcher for agent B]
B --> D[Child under agent A]
C --> E[Child under agent B]
F[Later Auto Run from agent B] --> G[Global worktree cwd lookup]
G --> H{First matching child}
H --> D
H --> E
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[External worktree discovered] --> B[Watcher for agent A]
A --> C[Watcher for agent B]
B --> D[Child under agent A]
C --> E[Child under agent B]
F[Later Auto Run from agent B] --> G[Global worktree cwd lookup]
G --> H{First matching child}
H --> D
H --> E
Reviews (1): Last reviewed commit: "fix(autorun): correct worktree attributi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| useSessionStore.getState().setSessions((prev) => { | ||
| if (prev.some((s) => normalizePath(s.cwd) === normalizedWorktreePath)) return prev; | ||
| // Per-parent guard (mirrors the existingForParent check above): another | ||
| // agent may already own a child at this cwd, but THIS parent should | ||
| // still get its own. Only collapse if this same parent raced to add a | ||
| // duplicate between the check above and here. | ||
| if ( | ||
| prev.some( | ||
| (s) => | ||
| s.parentSessionId === sessionId && normalizePath(s.cwd) === normalizedWorktreePath | ||
| ) | ||
| ) | ||
| return prev; | ||
| return [...prev, worktreeSession]; | ||
| }); |
There was a problem hiding this comment.
When an external worktree fans out to two same-repo agents, this guard now allows two child sessions with the same cwd. The spawn path still reuses an existing worktree session by cwd globally, so a later Auto Run from the second parent can resolve that same path and dispatch onto the first parent's child instead of the launching parent's child.
| if (allSettings['autoRunInactivityTimeoutMin'] !== undefined) | ||
| patch.autoRunInactivityTimeoutMin = allSettings['autoRunInactivityTimeoutMin'] as number; | ||
|
|
||
| if (allSettings['autoRunMaxTaskDurationMin'] !== undefined) | ||
| patch.autoRunMaxTaskDurationMin = allSettings['autoRunMaxTaskDurationMin'] as number; |
There was a problem hiding this comment.
The setter clamps autoRunMaxTaskDurationMin, but loading persisted settings bypasses that validation. If the stored value is non-finite, negative, or not a number, useAgentExecution treats the max-duration cap as disabled, so a chatty stuck Auto Run task can still hang the multi-document run.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cb64e06d9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // mark stays live until the owning session is committed to the store below. | ||
| // Only for create-new: existing-closed worktrees are already on disk and | ||
| // fire no addDir event, so there is nothing to race with. | ||
| markWorktreePathAsRecentlyCreated(worktreePath); |
There was a problem hiding this comment.
When a same-repo sibling already has a child session for this resolved path, the flow still falls through from this re-mark into the global sessionMatchesWorktreeRoot lookup below and dispatches the Auto Run to that sibling's child. This happens for a branch that is already attached elsewhere, or for an existing-closed worktree that is open under another parent, so a launch from agent B can run on agent A's worktree and reproduce the wrong-parent attribution this change is trying to prevent; reuse should only accept an existing session owned by parentSession.id, otherwise create or attach a child for the launching parent.
Useful? React with 👍 / 👎.
| tabLabel: 'General', | ||
| label: 'Auto Run Max Task Duration', | ||
| description: | ||
| 'Absolute cap on how long a single Auto Run task may run regardless of output — catches a stuck-but-chatty agent that keeps emitting yet never finishes and would otherwise hang the whole run', |
There was a problem hiding this comment.
The AGENTS.md instructions for /workspace/Maestro prohibit em/en dash characters anywhere in UI copy, comments, tests, and docs. This newly added searchable setting description contains U+2014, so it violates the documented style and keeps the settings copy out of compliance; replace it with a comma, colon, or spaced hyphen. The same character also appears in several new comments and tests in this commit.
Useful? React with 👍 / 👎.
| autoRunDisabled: false, | ||
| dotfilesToggleHidden: false, | ||
| autoRunInactivityTimeoutMin: 240, | ||
| autoRunMaxTaskDurationMin: 480, |
There was a problem hiding this comment.
For users who previously set autoRunInactivityTimeoutMin to 0, the UI promised that Unlimited disables the watchdog entirely, but existing installs will not have autoRunMaxTaskDurationMin persisted so this new default applies and useAgentExecution will still kill long tasks after 480 minutes. That makes an existing unlimited configuration silently become capped; consider migrating the new value to 0 when the persisted inactivity timeout is 0, or update the semantics and copy together.
Useful? React with 👍 / 👎.
| const existingForParent = latestSessions.find( | ||
| (s) => | ||
| s.parentSessionId === sessionId && | ||
| (sessionMatchesWorktreeRoot(s, normalizedWorktreePath) || | ||
| s.worktreeBranch === worktree.branch) |
There was a problem hiding this comment.
This per-parent dedup only fixes chokidar discovery, but I checked the startup and visibility-change path and both still call scanWorktreeConfigs, whose addition logic filters by global cwd before appending new worktree sessions. If an external worktree is created while the app is closed or the watcher misses it and the rescan path runs, the first same-repo parent still claims the cwd and the other parent is skipped, so the new fan-out behavior remains inconsistent outside the live watcher path.
Useful? React with 👍 / 👎.
|
Thanks for the contribution, @chr1syy, and for the unusually thorough write-up and test coverage. The two-fold framing (worktree attribution + absolute watchdog) is clear and the batch watchdog change in particular looks solid and well-tested. Before we approve, there are a few things to address. I went through the Greptile and Codex feedback and verified the substantive ones against the code; a couple of them are real. 1. (Blocker) Wrong-parent worktree attribution is still reachable via the spawn path. const existingSession = useSessionStore
.getState()
.sessions.find((s) => sessionMatchesWorktreeRoot(s, normalizedWorktreePath));The per-parent dedup change in 2. (Blocker) Em-dash style violation. 3. (Please address) Rescan path is not covered by the per-parent fix. 4. (Your call) Existing "unlimited" installs silently gain a 480-min cap. 5. (Minor) Load path skips the setter's clamp. #1 and #2 are the ones we'd want resolved before merge; #3-#5 are worth a look. No merge conflicts, so nothing to rebase. Thanks again, this is a genuinely useful pair of fixes. |
Scope worktree reuse/dedup to the launching parent and harden the Auto Run max-task-duration cap, per PR RunMaestro#1145 review threads. - worktreeSpawn: reuse an existing worktree session only when it is OWNED by the launching parent (new sessionOwnedByParent helper). A global cwd match let a same-repo sibling's child be adopted for this parent's Auto Run, reproducing the wrong-parent attribution the flow exists to prevent (codex + greptile P1). - useWorktreeHandlers rescan: apply the same per-parent dedup that the chokidar path already uses, so a restart/visibility rescan fans a shared worktree out to every same-repo parent instead of letting the first to iterate claim it globally (codex P2). - settingsStore: sanitize the persisted autoRunMaxTaskDurationMin on load so a corrupt (non-finite/negative/wrong-type) value can no longer silently DISABLE the cap; extract clampAutoRunMaxTaskDurationMin so setter and loader agree (greptile P2). - settingsStore: migrate existing "Unlimited" inactivity installs (persisted 0 with no cap set) to an unlimited absolute cap (0) so the new 480-min default doesn't silently start killing previously-uncapped runs (codex P2). - Replace prohibited em/en dashes introduced by this PR in copy, comments, and tests per the repo style rule (codex P2). Adds tests: parent-scoped worktree reuse, rescan per-parent fan-out, and the watchdog sanitize/clamp/migration helpers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the outstanding review threads in P1 - worktree reuse adopts a sibling's child (codex P2 - rescan bypasses per-parent dedup (codex P2 - invalid watchdog value disables the cap (greptile P2 - Unlimited Auto Run behavior (codex P2 - prohibited em/en dashes (codex Type-check, prettier, and eslint are clean; the four affected test files pass (337 tests). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c220d692e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const existingSession = latestSessions.find((s) => { | ||
| if (stalePending.has(s.id)) return false; | ||
| const normalizedCwd = normalizePath(s.cwd); | ||
| if (s.parentSessionId !== parentSession.id) return false; |
There was a problem hiding this comment.
When a startup or visibility rescan runs while spawnWorktreeAgentAndDispatch still has a Maestro-created path marked as recently created, this per-parent fan-out will still add that path under every same-repo parent because the scan path never checks isRecentlyCreatedWorktreePath like the chokidar listener does. This can still produce a sibling child for a worktree launched by agent A, especially during the slow getBranches window or on SSH where chokidar is unavailable, so marked paths should be skipped before creating rescan children.
Useful? React with 👍 / 👎.
| session.worktreeParentPath && | ||
| normalizePath(session.worktreeParentPath) === normalizePath(parentSession.cwd) |
There was a problem hiding this comment.
For legacy worktree sessions, worktreeParentPath is the scanned worktree base directory, not the parent agent's cwd, so this fallback returns false for the launcher's own legacy child unless those paths happen to be identical. In that legacy configuration, spawnWorktreeAgentAndDispatch will stop reusing an already-open child and can create a second agent in the same worktree instead of respecting the existing busy guard; compare against parentSession.worktreeParentPath as well.
Useful? React with 👍 / 👎.
| // mark stays live until the owning session is committed to the store below. | ||
| // Only for create-new: existing-closed worktrees are already on disk and | ||
| // fire no addDir event, so there is nothing to race with. | ||
| markWorktreePathAsRecentlyCreated(worktreePath); |
There was a problem hiding this comment.
When git worktree add itself runs longer than the 10 second TTL, the main-process chokidar debounce can validate and emit worktree:discovered while this await is still in flight, after the first mark has expired but before this second mark executes. In that slow setup case a sibling watcher can still adopt the Maestro-created worktree under the wrong parent, so the original mark needs to last through setup or be refreshed before the watcher can emit.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/hooks/worktree/useWorktreeHandlers.ts (1)
665-673: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the four repeated "same-parent" ownership checks.
Each of these four sites re-implements the same
s.parentSessionId === <id>predicate inline (dedup-on-add, insertion-time key, discovery pre-check, discovery post-check). Functionally correct, but the PR's own review history shows this exact pattern (duplicated ownership logic across files) previously let one call site (worktreeSpawn.ts) fall out of sync with the rest. Extracting a single local helper in this file (or reusingsessionOwnedByParentfromworktreeDedup.tswhere a full parentSessionis on hand) would make future fixes land in one place instead of four.♻️ Example consolidation
+function isOwnedByParent(session: Session, parentId: string): boolean { + return session.parentSessionId === parentId; +}Then replace each inline
s.parentSessionId === parentSession.id/s.parentSessionId === sessionIdcheck withisOwnedByParent(s, parentSession.id)/isOwnedByParent(s, sessionId).Also applies to: 807-809, 898-903, 965-970
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts` around lines 665 - 673, The worktree session ownership check is duplicated in multiple places and should be centralized to avoid future drift. In useWorktreeHandlers.ts, extract or reuse a single helper for the repeated parent-session predicate (for example, a local isOwnedByParent helper or sessionOwnedByParent from worktreeDedup.ts when a full Session is available), then replace each inline s.parentSessionId === parentSession.id / s.parentSessionId === sessionId check in the dedup-on-add, insertion-time key, discovery pre-check, and discovery post-check logic with that helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/utils/worktreeDedup.ts`:
- Around line 44-52: The legacy fallback in sessionOwnedByParent is too
permissive because the worktreeParentPath check only matches on normalized cwd,
which can misattribute sibling sessions in the same repo. Tighten the legacy
reuse logic in sessionOwnedByParent by adding an explicit owner/identity check
for legacy sessions, or make the fallback return false when multiple same-cwd
parents could match, so only an unambiguous parentSession can be reused.
---
Nitpick comments:
In `@src/renderer/hooks/worktree/useWorktreeHandlers.ts`:
- Around line 665-673: The worktree session ownership check is duplicated in
multiple places and should be centralized to avoid future drift. In
useWorktreeHandlers.ts, extract or reuse a single helper for the repeated
parent-session predicate (for example, a local isOwnedByParent helper or
sessionOwnedByParent from worktreeDedup.ts when a full Session is available),
then replace each inline s.parentSessionId === parentSession.id /
s.parentSessionId === sessionId check in the dedup-on-add, insertion-time key,
discovery pre-check, and discovery post-check logic with that helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cfcdcf99-e0c5-4ddd-9e9b-4393a9083ac0
📒 Files selected for processing (15)
src/__tests__/renderer/hooks/batch/useAutoRunHandlers.worktree.test.tssrc/__tests__/renderer/hooks/useAgentExecution.test.tssrc/__tests__/renderer/hooks/useWorktreeHandlers.test.tssrc/__tests__/renderer/stores/settingsStore.test.tssrc/__tests__/renderer/utils/worktreeSpawn.test.tssrc/renderer/components/Settings/searchableSettings.tssrc/renderer/components/Settings/tabs/GeneralTab/GeneralTab.tsxsrc/renderer/components/Settings/tabs/GeneralTab/components/AutoRunInactivitySection.tsxsrc/renderer/hooks/agent/useAgentExecution.tssrc/renderer/hooks/settings/useSettings.tssrc/renderer/hooks/worktree/useWorktreeHandlers.tssrc/renderer/stores/settingsStore.tssrc/renderer/utils/worktreeDedup.tssrc/renderer/utils/worktreeSpawn.tssrc/shared/settingsMetadata.ts
Follow-up review on the worktree-attribution fix: - sessionOwnedByParent: match ownership by parentSessionId ONLY. The legacy worktreeParentPath fallback compared against the parent's cwd, but that field is the scanned worktree BASE directory, which several same-repo parents can share - so it could match the wrong owner. Legacy sessions now fall through to getting a fresh new-model child for the launcher, which is safe (no misattribution) even if the legacy child isn't reused. - scanWorktreeConfigs: skip a subdir whose path is still marked recently-created by spawnWorktreeAgentAndDispatch (mirrors the chokidar listener). Without it, a startup/visibility rescan inside the spawn window would fan a Maestro-launched worktree out under every same-repo parent - and the rescan is the ONLY discovery path on SSH, where chokidar is unavailable. - worktreeSpawn: give the pre-`git worktree add` dedup mark a 60s TTL so it can't expire mid-setup on a large repo / cold disk / SSH remote (the default 10s window could lapse before the post-setup re-mark). The error/already-existed branches clear the path and the success path re-marks with the default TTL, so the long window never outlives the setup it guards. Tests: rescan skips a recently-created marked path; existing parent-scoped reuse and fan-out tests still pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Follow-up review addressed in
Type-check, prettier, eslint clean; 86 worktree tests pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f58952c5d3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // mark stays live until the owning session is committed to the store below. | ||
| // Only for create-new: existing-closed worktrees are already on disk and | ||
| // fire no addDir event, so there is nothing to race with. | ||
| markWorktreePathAsRecentlyCreated(worktreePath); |
There was a problem hiding this comment.
Fresh evidence after the prior TTL finding: this success-path refresh still uses the default 10-second TTL, but the session is not committed until after the awaited git metadata fetch below. When getBranches is slow, especially on SSH or a large repo, the mark can expire before setSessions runs, so a sibling watcher can still adopt the Maestro-created worktree under the wrong parent; use the longer setup TTL here or clear the mark only after the owning session has been stored.
Useful? React with 👍 / 👎.
| long refactors, heavy test runs, or web-research tasks driving a browser. Choose Unlimited | ||
| to disable the watchdog entirely. |
There was a problem hiding this comment.
With the new independent max-duration watchdog, choosing Unlimited here only sets autoRunInactivityTimeoutMin to 0; autoRunMaxTaskDurationMin remains 480 by default on new installs. This text tells users the watchdog is entirely disabled, so an Auto Run can still be killed after 8 hours unless they also set Max Task Duration to Unlimited.
Useful? React with 👍 / 👎.
| patch.autoRunMaxTaskDurationMin = sanitizeLoadedAutoRunMaxTaskDurationMin( | ||
| allSettings['autoRunMaxTaskDurationMin'] | ||
| ); | ||
| } else if (allSettings['autoRunInactivityTimeoutMin'] === 0) { |
There was a problem hiding this comment.
This branch runs on every load whenever the new cap key is absent, not just for pre-feature installs. On a current install where the user selects Unlimited for inactivity but never touches Max Task Duration, only autoRunInactivityTimeoutMin is persisted, so the current session keeps the 480-minute cap but the next restart changes autoRunMaxTaskDurationMin to 0. Gate this with a persisted migration marker or write the migrated cap once so behavior does not change across restarts.
Useful? React with 👍 / 👎.
| const existingSession = latestSessions.find((s) => { | ||
| if (stalePending.has(s.id)) return false; | ||
| const normalizedCwd = normalizePath(s.cwd); | ||
| if (s.parentSessionId !== parentSession.id) return false; |
There was a problem hiding this comment.
The new fan-out rule is applied in scanWorktreeConfigs, but the scan performed immediately after saving a worktree config still uses the old global cwd check in handleSaveWorktreeConfig (existingByPath at lines 252-257). If agent B enables the same basePath after agent A already owns a child for an existing worktree, that initial scan skips B, and no chokidar add event fires for an already-existing directory, so B will not see the child until a later visibility or restart rescan.
Useful? React with 👍 / 👎.
| (s) => | ||
| sessionOwnedByParent(s, parentSession) && | ||
| sessionMatchesWorktreeRoot(s, normalizedWorktreePath) |
There was a problem hiding this comment.
When worktreeSetup resolves to a branch already attached under a sibling agent, this parent-scoped lookup ignores that sibling even if it is busy. The code then builds another child and Auto Run spawns in the same worktree path, so two agents can write the same checkout concurrently; keep reuse parent-scoped, but reject or queue when any same-root session is already busy or connecting.
Useful? React with 👍 / 👎.
- worktreeSpawn: refuse to build a second child into a worktree a BUSY sibling is already running in. worktreeSetup can resolve to a branch attached under a sibling agent; reuse stays parent-scoped, but dispatching a fresh child into that same checkout would let two agents write it concurrently. Bail if any same-root session is busy/connecting. - worktreeSpawn: re-mark the resolved path with the long setup TTL (not the default 10s). The owning session isn't committed until after the awaited getBranches, which is slow on SSH/large repos, so the mark must survive that window too. - settingsStore: make the "Unlimited inactivity" -> unlimited-cap migration one-shot by persisting the migrated value. Without writing the key back, the branch re-ran on every load and would silently reset a cap the user set later. - useWorktreeHandlers.handleSaveWorktreeConfig: scope the save-time scan's existing-child checks to the active parent (and skip recently-created marks), mirroring scanWorktreeConfigs. A global cwd match let a second same-repo agent enabling the same basePath be skipped, leaving it childless until a later rescan. - Clarify the inactivity "Unlimited" copy: it disables the inactivity watchdog only; the Max Task Duration cap still applies unless also set to Unlimited. Tests: busy same-root sibling is rejected (no duplicate child); the unlimited migration persists its value one-shot. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the follow-up review in
Type-check, prettier, eslint clean; 248 tests pass across the worktree + settings suites. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/utils/worktreeSpawn.ts (1)
88-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale comment: success path re-marks with the long TTL, not the default.
This comment says the success path "re-marks with the default TTL, so the long window never outlives the setup it guards", but Line 148 re-marks with
WORKTREE_SETUP_MARK_TTL_MS(the long TTL) and the success path never clears it — this directly contradicts the reasoning at Lines 142‑145. The stated invariant no longer holds; the long window intentionally persists past setup until TTL expiry, so this comment could mislead future readers.📝 Suggested wording fix
- // worktree under the wrong parent. The error and already-existed branches - // clear this path explicitly, and the success path re-marks with the default - // TTL, so the long window never outlives the setup it guards. + // worktree under the wrong parent. The error and already-existed branches + // clear this path explicitly; the success path re-marks the resolved path + // with the same long TTL below to keep the guard alive across the + // post-setup session-building window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/utils/worktreeSpawn.ts` around lines 88 - 90, Update the stale comment in worktreeSpawn so it matches the actual success-path behavior in worktreeSpawn and the TTL re-marking logic around WORKTREE_SETUP_MARK_TTL_MS. The comment currently says the success path re-marks with the default TTL and that the long window never outlives setup, but the success path actually preserves the long TTL, so rewrite it to describe that the long setup mark intentionally remains until expiry unless cleared by the error/already-existed branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/renderer/utils/worktreeSpawn.ts`:
- Around line 88-90: Update the stale comment in worktreeSpawn so it matches the
actual success-path behavior in worktreeSpawn and the TTL re-marking logic
around WORKTREE_SETUP_MARK_TTL_MS. The comment currently says the success path
re-marks with the default TTL and that the long window never outlives setup, but
the success path actually preserves the long TTL, so rewrite it to describe that
the long setup mark intentionally remains until expiry unless cleared by the
error/already-existed branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c03a075-baff-4544-96f6-64c4ad762b60
📒 Files selected for processing (6)
src/__tests__/renderer/stores/settingsStore.test.tssrc/__tests__/renderer/utils/worktreeSpawn.test.tssrc/renderer/components/Settings/tabs/GeneralTab/components/AutoRunInactivitySection.tsxsrc/renderer/hooks/worktree/useWorktreeHandlers.tssrc/renderer/stores/settingsStore.tssrc/renderer/utils/worktreeSpawn.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/renderer/components/Settings/tabs/GeneralTab/components/AutoRunInactivitySection.tsx
- src/tests/renderer/stores/settingsStore.test.ts
- src/renderer/stores/settingsStore.ts
- src/renderer/hooks/worktree/useWorktreeHandlers.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ed5e88d81
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| expect(markWorktreePathAsRecentlyCreated).toHaveBeenCalledWith( | ||
| '/projects/worktrees/dedup-test' | ||
| ); |
There was a problem hiding this comment.
In this create-new test, the implementation now calls markWorktreePathAsRecentlyCreated(path, WORKTREE_SETUP_MARK_TTL_MS) twice. Vitest's toHaveBeenCalledWith matches the full call argument list, so this one-argument expectation does not match either call and the test fails even though callOrder passes; include or ignore the TTL argument here, such as with expect.any(Number).
Useful? React with 👍 / 👎.
Follows the re-mark TTL change: the worktree Auto Run test asserted markWorktreePathAsRecentlyCreated was called with only the path, but both marks now pass WORKTREE_SETUP_MARK_TTL_MS (60000). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Summary
Two related Auto Run robustness fixes, surfaced by launching a two-document playbook from web-desktop with auto-worktree creation. The worktree attached to the wrong agent, and the run hung on document 1 without ever starting document 2.
1. Worktree attribution across same-repo agents (PR #946 follow-up)
When auto-worktree is enabled, chokidar runs one watcher per watching agent and emits
worktree:discoveredcarrying the watcher's sessionId, not the launch target. Two issues let that misattribute:worktreeSpawn: re-mark the resolved worktree path as recently-created afterworktreeSetup(create-new only). The original 10s dedup TTL started before the potentially slowgit worktree add, so the discovery could fire during thegetBranches/buildWorktreeSessionwindow after the mark aged out, letting a sibling agent watching the same basePath adopt the worktree under the wrong parent. Re-marking also covers theexistingPathreassignment the original mark never did.useWorktreeHandlers: scopeonWorktreeDiscovereddedup per-parent instead of globally by cwd. Each same-repo agent fires its own discovery event, so an externally-created worktree now fans out: every agent in the cwd gets its own child, instead of the first watcher claiming it globally and the others silently dropping it. Maestro-spawned worktrees still attach only to the launching agent (the dedup mark suppresses sibling watchers).2. Absolute Auto Run task watchdog
The per-document batch loop awaits
processTask, whose only recovery was the silence-based inactivity watchdog. A stuck-but-chatty agent that keeps emitting output (resetting the silence timer) but never finishes defeats it, hanging the whole multi-document run with no advance to the next document.autoRunMaxTaskDurationMin, default 480 min,0= unlimited) that force-kills regardless of output and resolves the task aswatchdog-timeout, which the batch loop already handles by terminating the document and moving on.useSettings, the General tab UI (alongside the inactivity timeout), and searchable settings.Testing
worktreeSpawnre-mark robustness (incl.existingPath).All in-scope suites pass; typecheck clean; prettier + eslint clean.
🤖 Generated with Claude Code
Summary by CodeRabbit