Skip to content

fix(server): report scheduled worktree preparation failures - #19774

Open
realhasanshoaib wants to merge 10 commits into
pingdotgg:mainfrom
realhasanshoaib:fix/scheduled-task-root-workspace
Open

realhasanshoaib wants to merge 10 commits into
pingdotgg:mainfrom
realhasanshoaib:fix/scheduled-task-root-workspace

Conversation

@realhasanshoaib

@realhasanshoaib realhasanshoaib commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Scheduled runs can show succeeded when their new worktree never finishes preparation. Launch acceptance happens before checkout and setup complete.

Change

Check worktree eligibility before launching a scheduled thread, and follow its first exit from preparation. A preparation failure corrects the scheduled run to failed. A provider failure after starting does not. The correction checks task creation time, run timestamp, and count so delayed feedback cannot overwrite another run.

This takeover keeps the status fix from @realhasanshoaib in #19774. The separate MCP workspace configuration feature was removed. Scratch projects retain current main’s thread-owned-folder behavior.

Scope and approval

Focused correction to existing scheduled-run status. No new scheduling API or client contract. This addresses the failure reporting described in #17309.

Verification

  • Before implementation: 85 existing tests passed; the five new regression cases failed.
  • After implementation: 92 tests pass in ScheduledTaskService and ThreadLaunchService, including Scratch and ambiguous-Git cases. Another 81 tests pass across six related scheduler and MCP files; the added MCP error-guidance case brings its file to 7 passing tests.
  • Server typecheck and changed-file formatting/lint pass (one existing lint warning).
  • Headless live server with disposable state, driven through authenticated WebSocket RPC: a plain folder fails before launch; an unavailable Git origin is accepted initially and changes from succeeded to failed after preparation fails. Both count one run.
  • Preparation retries, later provider failures, and delayed feedback from an older run are covered by synchronized service tests.
  • Independent round 1 found misleading MCP guidance, now fixed and covered. Fresh full-diff round 2 is complete; both reviewers found no further blockers. The restart window below was raised by Sol and retained as a documented design limit.

The preparation monitor is scoped to the running service; it is not restored after a server restart.

Linux verification only. macOS, Windows, iOS, Android, and Electron desktop were not exercised. Screenshots are not applicable to this server status change; the current-head live check used the server API, without a graphical client.

Implementation: GPT-6.1 Sol, Codex harness in T3 Code. Tests: GPT-6 Astra (high). Independent reviews: Claude Opus 5.5 and GPT-6 Sol.

…s failed

A scheduled task that launches a fresh thread per run with a worktree
strategy was recorded as "succeeded" as soon as the thread launch was
accepted, even when its worktree could never be made, such as in a folder
that is not a Git repository. The run then failed inside its thread before
the agent started, and the scheduler never said so.

- The pre-launch worktree check now also refuses a folder with no `.git`
  in it or above it. A repository Git refuses to read is still left to the
  launch.
- The scheduler runs that check before launching, so such a run is recorded
  as failed with the reason, and no dead thread is created.
- schedule_task and update_scheduled_task take an optional workspace
  (root or worktree), so an agent can choose to run in the project folder;
  an explicit worktree that cannot be made is refused up front. The task
  summary now shows the workspace, and an update keeps it unless the
  binding or the workspace changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-15T07:47:23.494910Z e0fd539 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 15, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 6be5346

Macroscope's review found this PR approvable — This is a focused scheduler fix that records impossible worktree launches as failed without creating dead threads. The new MCP workspace selection is optional and preserves existing defaults, with coverage for repository validation and update behavior.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 07b65a34-1ffa-4b97-b498-1243f783ecb9
📥 Commits

Reviewing files that changed from the base of the PR and between 2911fbd and e0fd539.

📒 Files selected for processing: 1
  • apps/server/src/mcp/toolkits/project/handlers.test.ts
🚧 Files skipped from review as they are similar to previous changes: 1
  • apps/server/src/mcp/toolkits/project/handlers.test.ts

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

📝 Walkthrough

Scheduled tasks now accept root or worktree workspace choices. Worktree requests are checked for repository and base-ref validity. Scheduled runs record worktree-check failures and track preparation failures after launch.

Changes: Scheduled-task workspace selection

Layer / File(s) Summary
Worktree-base validation
apps/server/src/orchestration-v2/ThreadLaunchService.ts, apps/server/src/orchestration-v2/ThreadLaunchService.test.ts, apps/server/src/mcp/toolkits/project/handlers.ts, apps/server/src/mcp/toolkits/project/handlers.test.ts
checkWorktreeBase checks whether a .git directory exists in the project folder or a parent when Git reports that the folder is not a repository. It reports a distinct error when no repository is found. Project launch errors and tests cover these checks.
Workspace contract and MCP selection
packages/contracts/src/orchestratorMcp.ts, apps/server/src/mcp/OrchestratorMcpService.ts, apps/server/src/mcp/OrchestratorMcpService.test.ts, apps/server/src/mcp/toolkits/orchestrator/tools.ts
Scheduled-task inputs accept root or worktree, and task results expose the workspace strategy. MCP creation and updates resolve the strategy, validate worktree requests, and return it in task summaries. Tool descriptions document the choices.
Scheduled-run validation and preparation status
apps/server/src/scheduledTasks/ScheduledTaskService.ts, apps/server/src/scheduledTasks/ScheduledTaskService.test.ts
New-thread runs check the worktree base before launch. After a successful launch, the service monitors preparation events and can mark the matching run as failed.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ScheduledTaskService
  participant ThreadLaunchService
  participant RunUpdateStream
  participant ScheduledTaskStore
  ScheduledTaskService->>ThreadLaunchService: Check worktree base and launch thread
  ThreadLaunchService-->>ScheduledTaskService: Return launched thread ID
  ScheduledTaskService->>RunUpdateStream: Read preparation updates
  RunUpdateStream-->>ScheduledTaskService: Report initial message status
  ScheduledTaskService->>ScheduledTaskStore: Mark matching run failed when preparation failed
Loading
Merge Risk: ⚪ Minimal · up to e0fd5

No merge-blocking issue is established for the selected change; normal checks remain appropriate.

Security architecture risk: 🔵 Low · up to 2911f

Scheduled tasks can now explicitly run in the shared project folder rather than a fresh worktree. Existing caller permissions and execution-mode limits remain in place. Failure reporting improves, but late status corrections do not survive every restart or monitoring failure.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — An authorized caller can now persist an unbound scheduled task that uses an ordinary project's shared checkout instead of a fresh worktree. No-project execution still uses managed folders. The task retains its established runtime and interaction modes; workspace selection does not itself grant broader execution modes.

Trust Boundaries and Controls

  • observed — The caller-controlled workspace value reaches persistent scheduling through existing MCP write/start controls. Scheduling requires the orchestration capability, resolves the target project, stores caller-limited modes, and checks binding ownership. Updates validate the existing task's modes and preserve them. The traced change adds workspace choice without removing these controls.
Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description includes all required sections and detailed verification results. However, it states that the MCP workspace configuration feature was removed, while the pull request adds MCP workspace… Update the description to accurately describe the MCP workspace changes and explain their relationship to the scheduled-run fix. Add the required maintainer approval link and approval comment, or provide a specific justification that this f…
Passed Passed checks: 3 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly identifies the primary change: reporting scheduled worktree preparation failures.
Full details: Description check

Explanation

The description includes all required sections and detailed verification results. However, it states that the MCP workspace configuration feature was removed, while the pull request adds MCP workspace configuration to contracts, tools, handlers, services, and tests. The scope section also links an issue but does not provide explicit maintainer approval or explain why approval was not required.

Resolution

Update the description to accurately describe the MCP workspace changes and explain their relationship to the scheduled-run fix. Add the required maintainer approval link and approval comment, or provide a specific justification that this focused change qualifies for the approval exemption.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

realhasanshoaib and others added 4 commits October 15, 2026 09:10
Root runs have nothing to check, and skipping the call keeps scheduler
fixtures that stub only `launch` working.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…orted as failed

A launch is accepted before its workspace is prepared, so a run whose
worktree fetch or setup script failed still read "succeeded". The scheduler
now waits for the launched run to leave preparation and, if it failed,
marks that run failed. The write is guarded by the run's start time, so a
later run's status is never overwritten. It also covers the hint that
t3_thread_launch gives for a folder that is not a Git repository.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The watcher read the run's current status, so a retried preparation could
hide its first failure, and a provider failure after setup could be blamed
on setup. It now replays the new thread's run updates and decides from the
run's first status other than "preparing".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@unmanbearpig

Copy link
Copy Markdown

If an agent updates a worktree schedule and passes workspace: "worktree" again, the base branch the user picked in the editor (say release) is silently replaced with main.

updateScheduledTask keeps the saved strategy only when workspace is omitted. Any value goes through scheduledTaskWorkspaceStrategy, which builds {type:"worktree", baseRef:"main", startFromOrigin:true} from scratch. A task whose base was changed in the web or mobile editor loses that change:

Saved strategy Update input Result
worktree, baseRef:"release", startFromOrigin:false {prompt} unchanged
same {prompt, workspace:"worktree"} baseRef:"main", startFromOrigin:true
same, repo has no main and no origin same update rejected, and the error suggests workspace:"root"
worktree, main, branch:"nightly" {enabled:false, workspace:"worktree"} branch dropped

The agent can't see this happen. list_scheduled_tasks reports workspace: "worktree" before and after, and MCP has no field to set the base back. An agent copying fields from the list output into an update will hit it, since the field name and value match. The people most exposed are on master or trunk repos, because they had to change the editor's main default. When origin exists but has no main, the precheck passes and the update saves a base that fails on every run.

The tool description promises "Only the provided fields change", so I'd treat a repeated type as no change:

const workspaceStrategy =
  !rebound &&
  (input.workspace === undefined || input.workspace === existing.workspaceStrategy.type)
    ? existing.workspaceStrategy
    : yield* scheduledTaskWorkspaceStrategy(existing.projectId, threadId !== null, input.workspace);

I checked this with a scratch test at e0fd539. The three bad cases keep their saved strategy. The binding and type transitions (root to worktree, worktree to root, existing_worktree to worktree, bound to unbound and back) behave as before. The existing test "an update keeps the workspace unless the binding or the workspace changes" only asserts summary.workspace. Asserting the saved strategy after a repeated workspace:"worktree" on a custom base would catch this.

@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one for batch worktree-git-16b. I will keep the scheduled preparation failure fix, remove the separate MCP workspace feature, merge current main, and rebuild the focused tests and implementation before independent review. Thanks @realhasanshoaib for the diagnosis and reproduction.

@juliusmarminge juliusmarminge self-assigned this Oct 19, 2026
@juliusmarminge juliusmarminge changed the title fix(server): scheduled runs that cannot get a worktree are reported as failed fix(server): report scheduled worktree preparation failures Oct 19, 2026

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

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants