Skip to content

server, docs: hold auto-settle while a background command runs - #18062

Closed
dominic-r wants to merge 1 commit into
pingdotgg:mainfrom
d5cr:fix/auto-settle-background-command
Closed

dominic-r wants to merge 1 commit into
pingdotgg:mainfrom
d5cr:fix/auto-settle-background-command

Conversation

@dominic-r

Copy link
Copy Markdown
Contributor

Fixes #17625

Supersedes #17626 (closed in PR cleanup for missing model/harness attribution).

Related: #15099 (the completion alert for the same case, which this PR leaves unchanged) and #15413.

Problem

A thread auto-settles while the agent is still waiting on a live background command. Claude records every background Bash as a pending task of kind command, and the CLI wakes the agent when that command exits. Auto-settlement reused the completion-alert rule, backgroundWorkHoldsCompletion, which treats a command as "the agent is done". When the merge rule or the inactivity rule came due, the thread settled while the composer still showed the command running, and the agent later woke up in a settled thread.

Change

isAutoSettlementCandidate now rejects a thread with any live pending background task, whatever its kind. That one check covers both the settlement sweep and resolveAutoSettlementAt, so the merge rule and the inactivity rule both wait. Completion alerts and sidebar status keep their current rule, so a dev server left running still lets the "done" notification fire.

The user guide's auto-settle section now says a background command holds the thread until it exits.

Trade-off: a thread with a dev server or another long-lived command left running stays unsettled until the command exits or the user settles the thread by hand. Auto-settle is silent and hides the thread, so holding it a little too long is the cheaper mistake.

Scope and approval

This fixes the bug reported in #17625, using the fix that issue suggests. It changes only the auto-settlement gate on the server, plus its tests and one sentence of user docs.

Verification

Replaced the test that expected a live command to allow auto-settle with one that expects it to hold the thread. Added a test where a thread with a live command does not settle under either the merged-PR rule or the inactivity rule, and the same thread with no pending tasks settles under both. Both new tests fail with the old check and pass with the fix. The existing settlement service and settlement projection tests still pass, and the server package typechecks. I did not reproduce the bug end to end in a running app.

Written by Claude Opus 5.5 via t3code. This PR was opened by Grok Bot.

Auto-settlement reused the completion-alert rule, which ignores pending
background commands. A Claude background Bash the agent is waiting on
wakes the agent when it exits, so the thread settled while it was still
working. Any live pending background task now holds auto-settle. Alerts
and sidebar status keep their rule.
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 11, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at ab35920

Macroscope's review found this PR approvable — This is a focused server-side bug fix that keeps existing auto-settlement rules from hiding threads with live background commands, with targeted coverage for both merge and inactivity paths. The remaining changes are limited to tests and user documentation, with no product-default, schema, security, or deployment changes.

Notes:

  • All code in this push has already been reviewed. Approvability was decided on eligibility alone.

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

@coderabbitai

coderabbitai Bot commented Oct 11, 2026

Copy link
Copy Markdown

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: 51e7a387-e5e4-49c0-b8c9-56f8503587d5

📥 Commits

Reviewing files that changed from the base of the PR and between 716357c and ab35920.


📒 Files selected for processing (3)
  • apps/server/src/orchestration-v2/ThreadSettlementService.test.ts
  • apps/server/src/orchestration-v2/ThreadSettlementService.ts
  • docs/user/thread-sidebar.md

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



📝 Walkthrough

Walkthrough

Auto-settlement now rejects threads with any pending background task, including commands. Tests cover merged-PR and inactivity settlement while a command is pending and after it is removed. Sidebar guidance describes the same rule.

Changes

Auto-Settlement

Layer / File(s) Summary
Block settlement while background tasks are pending
apps/server/src/orchestration-v2/ThreadSettlementService.ts, apps/server/src/orchestration-v2/ThreadSettlementService.test.ts, docs/user/thread-sidebar.md
The candidate check now rejects threads with any pending background task. Tests cover merge-based and inactivity-based settlement before and after removing a pending command. The sidebar guidance says that a running command prevents automatic settlement until it exits or the thread is settled manually.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge


Merge Risk: ⚪ Minimal · up to ab359

Threads with a running background command, such as a dev server, are no longer auto-settled until the command exits or the user settles the thread manually. This is the intended behavior and is documented. No merge-blocking risk was identified.

Architecture Summary

Architecture risk: 🔵 Low · up to ab359

The change affects 2 systems.

Changed systems: apps/server, docs

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — apps/server (service) was modified; 2 changed files map to changed impact.
  • observed — docs (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in apps/server/src/orchestration-v2/ThreadSettlementService.test.ts: The candidate test changes its expected outcome for a thread whose only pending background task is a command: it now expects the thread not to be eligible for auto-settlement, replacing the prior expectation that it was eligible.
  • observed — Modified behavior in apps/server/src/orchestration-v2/ThreadSettlementService.test.ts: Adds coverage that pending background commands prevent resolveAutoSettlementAt from settling through either the merged-PR rule or the inactivity rule; removing the pending task makes both rules return the thread’s last run-completion time.
  • observed — Modified behavior in apps/server/src/orchestration-v2/ThreadSettlementService.ts: Removed the backgroundWorkHoldsCompletion import.
  • observed — Modified behavior in apps/server/src/orchestration-v2/ThreadSettlementService.ts: Changed the candidate check from filtering background tasks through backgroundWorkHoldsCompletion to rejecting settlement whenever the pending-task list is nonempty. Thus, tasks previously considered not to hold completion, including a running dev server, now block auto-settlement.

Pre-merge checks | Passed 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description check Passed The description covers the problem, change, scope and approval basis, verification results, trade-offs, and the model and harness attribution. It also identifies the tests and checks that were run and…
Title check Passed The title clearly and concisely describes the main change: server and documentation updates that prevent auto-settlement while a background command runs.
Linked Issues check Passed Issue #17625 requires auto-settlement to wait for any live pending background work under both merge and inactivity rules. ThreadSettlementService.ts now rejects every non-empty `pendingBackgroundTas…
Out of Scope Changes check Passed The diff changes only the auto-settlement gate, its focused tests, and the related user-guide text. The changes support Issue #17625. Completion alerts and sidebar status remain unchanged, as required…

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

Copy link
Copy Markdown
Member

Note

This comment is posted by Julius' dot

A maintainer decision is needed under problem and scope. At ab35920, the fix prevents settlement while an agent is waiting on a command, but it also reverses the existing tested behavior for a dev server left running: those threads now stay unsettled indefinitely. #17625 proposes that trade-off but has no maintainer response.

Should every live background command block auto-settle, or should commands the agent is waiting on be distinguished from servers it left running? Please link approval for that direction before review handoff. Also, #17626 was reopened after its mistaken cleanup closure; please clarify which PR should carry the final change.

Copy link
Copy Markdown
Contributor Author

Thanks. Answers in order:

Every live command or only awaited ones? Every live command, on purpose. The adapter gives no signal for which commands the agent is waiting on. A pending task carries only taskId, kind and description, and Claude's background Bash is kind: "command" whether it's a sleep && check the CLI will wake the agent for or a dev server. backgroundWorkHoldsCompletion returns false for every command, which is exactly why #17625 happens. An "awaited only" rule built on the current data would bring the bug back. Splitting the two would need a new field from the provider adapters, which I think belongs in a follow-up, not this fix.

Dev server trade-off: yes, this reverses the old tested behavior for a dev server left running. The thread stays unsettled only while the command is live. It becomes eligible again as soon as the command exits or is cancelled, and the user can still settle it by hand. Completion alerts and sidebar status keep the existing rule, so the "done" notification still fires. Auto-settle is silent and hides the thread, so I think holding it too long is the cheaper mistake. The user guide says so.

Approval: I don't have a maintainer approval to link. #17625 proposes this direction and hasn't had a response yet. I'm happy to wait for a decision there, or to rework this if you'd rather distinguish the two cases.

Which PR: this one, #18062. It's the same commit (ab35920) on the same branch as #17626, which is closed now and can stay closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 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.

[Bug]: Thread auto-settles while the agent is still waiting on a background command

3 participants