Skip to content

fix(server): mark unvisited thread completions unread - #15607

Open
maria-rcks wants to merge 7 commits into
pingdotgg:mainfrom
maria-rcks:fix/3131-unvisited-completion-round2
Open

maria-rcks wants to merge 7 commits into
pingdotgg:mainfrom
maria-rcks:fix/3131-unvisited-completion-round2

Conversation

@maria-rcks

@maria-rcks maria-rcks commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

A thread started in the background could finish without ever showing an unread Done badge, because its read watermark was still null. The first fresh message now initializes that watermark one millisecond before dispatch, so the completion is unread on every client; opening the thread clears it, and untouched imported history stays read.

Closes #3131

The earlier Codex request-ordering and setup-worktree rename changes were dropped from this PR; the scoped server suites pass without them.

Verified on Blacksmith: 2009 orchestration-v2, MCP, and scheduling tests passed, including the focused watermark regression; server typecheck and scoped lint passed. Earlier: a real Codex completion in the web client, a full reload, and opening the thread.

Before: completed, never-opened thread has no Done badge.

completed background thread before the fix

After: a fresh completion shows Done; the older null-marker thread remains read.

fresh completion shows done while older history stays read

background completion shows done, then opening the thread clears it

Written by claude-opus-5-5 via Claude Code in T3 Code

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 4, 2026
@maria-rcks
maria-rcks marked this pull request as ready for review October 4, 2026 12:09
@github-actions github-actions Bot added size:M 30-99 changed lines (additions + deletions). and removed size:S 10-29 changed lines (additions + deletions). labels Oct 4, 2026
@coderabbitai

coderabbitai Bot commented Oct 4, 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: 35067c5b-62fb-42c9-99ad-e6a9aa685d93

📥 Commits

Reviewing files that changed from the base of the PR and between 17c0878 and 050e127.


📒 Files selected for processing (5)
  • apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts
  • apps/server/src/orchestration-v2/Orchestrator.control-reads.test.ts
  • apps/server/src/orchestration-v2/Orchestrator.ts
  • apps/server/src/orchestration-v2/ThreadLaunchService.test.ts
  • apps/server/src/orchestration-v2/ThreadLaunchService.ts

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



📝 Walkthrough

Walkthrough

The orchestrator initializes a missing read watermark on a thread’s first non-maintenance dispatch. Codex request handlers reorder two emitted events. Branch-rename metadata updates include the expected worktree path, with tests covering these changes.

Changes

Read watermark initialization

Layer / File(s) Summary
Initialize watermark on first conversation dispatch
apps/server/src/orchestration-v2/Orchestrator.ts, apps/server/src/orchestration-v2/Orchestrator.control-reads.test.ts, apps/server/src/orchestration-v2/ThreadLaunchService.test.ts
A first non-maintenance dispatch sets a missing lastVisitedAt to one millisecond before the current time. Tests cover rejected and accepted dispatches, completion timestamps, retries, unread marking, explicit visit times, and thread launch state.

Codex request event ordering

Layer / File(s) Summary
Order request events
apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts
Eight request handlers now emit turn_item.updated before runtime_request.updated. Event payloads and other handler behavior remain unchanged.

Worktree branch rename metadata

Layer / File(s) Summary
Constrain branch-rename metadata update
apps/server/src/orchestration-v2/ThreadLaunchService.ts, apps/server/src/orchestration-v2/ThreadLaunchService.test.ts
The branch-rename metadata update includes expectedWorktreePath. The cancellation test delays branch-name generation and waits for the rename failure log or stored event before checking run and worktree state.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: juliusmarminge


Merge Risk: ⚪ Minimal · up to 050e1

No actionable merge-blocking risk remains from the reviewed changes; merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 050e1

The changes preserve existing operations and approval controls while improving event ordering and rejecting stale worktree updates. No privilege expansion was established, but some concurrent cancellation and recovery cases remain partially verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The assessed security-relevant scope is existing provider approval and input flows plus project worktree metadata. The changes do not establish a new external operation or credential authority. Broader tenant and deployment isolation were not established by the available evidence.

Trust Boundaries and Controls

  • observed — The existing response control obtains request context using thread and request identity, rejects missing or non-pending requests, rejects non-resumable requests, and verifies the required live provider session exists. These preconditions are not removed by the event-order change.

Resilience and Maintainability Implications

  • inferred — The expected-path precondition improves containment of stale metadata updates after cancellation, but it is not a generation check or an atomic transaction with Git. Physical renaming still precedes metadata dispatch. Same-path rebinding and reconciliation after rejection remain unresolved; this is bounded uncertainty, not evidence that the PR introduced a security failure.


Pre-merge checks | Passed 2 | Failed 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check Warning The CodexAdapterV2 changes reorder approval/input timeline updates and actionable requests. The ThreadLaunchService change guards against a late branch rename restoring a cancelled setup worktree. The… Move the CodexAdapterV2 ordering changes and the ThreadLaunchService worktree guard to separately scoped changes, or link issues that require these behaviors.
Description check Warning The description explains the problem, intended behavior, linked issue, verification, and UI evidence. However, it does not use the required Problem, Change, Scope and approval, and Verification sectio… Align the description with the actual diff. Add the required section headings, explain the scope and approval basis, and describe or remove the Codex ordering and worktree rename changes that remain in the pull request.
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check Passed Issue #3131 requires newly started background threads to show unread completion while historical threads remain read. Orchestrator.dispatchMessage now writes a watermark one millisecond before the f…
Title check Passed The title clearly identifies the primary change: marking completions in previously unvisited threads as unread.

Full details: Out of Scope Changes check

Explanation

The CodexAdapterV2 changes reorder approval/input timeline updates and actionable requests. The ThreadLaunchService change guards against a late branch rename restoring a cancelled setup worktree. These changes address approval ordering and setup cancellation, not issue #3131's unread completion status. The PR description connects them to races exposed during implementation, but it does not establish that they are needed for the linked issue.


Full details: Description check

Explanation

The description explains the problem, intended behavior, linked issue, verification, and UI evidence. However, it does not use the required Problem, Change, Scope and approval, and Verification sections. It also states that Codex ordering and worktree rename changes were dropped, while the change summary shows those files still changed.



  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR


  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Comment thread apps/server/src/orchestration-v2/Orchestrator.ts Outdated
@macroscopeapp

macroscopeapp Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 5551447

Macroscope's review found this PR approvable — This focused server bug fix adds a one-time unread watermark for fresh dispatches on never-visited threads while preserving existing maintenance and visited-thread behavior. The accompanying tests cover the new state transition and its exclusions.

Notes:

  • Diff unchanged. Approvability was decided on eligibility alone.

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

@github-actions github-actions Bot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Oct 4, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 4, 2026
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 6, 2026 16:52

Dismissing prior approval to re-evaluate 050e127

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 6, 2026
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 9, 2026 23:44

Dismissing prior approval to re-evaluate 5551447

@github-actions github-actions Bot added size:S 10-29 changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Oct 9, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 9, 2026
@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 11, 2026 — with ChatGPT Codex Connector
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 11, 2026 05:48

Dismissing prior approval to re-evaluate 5551447

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

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidebar can miss unread Completed status for newly finished threads

2 participants