Skip to content

fix(agent-chat): scope ACP plans to their turn and refresh activity - #15898

Merged
teamleaderleo merged 2 commits into
mainfrom
parity/plan-scope
Sep 30, 2026
Merged

teamleaderleo merged 2 commits into
mainfrom
parity/plan-scope

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

This follow-up to #15889 fixes two review findings in ACP plan handling. A plan update now replaces only the plan in the current turn, using the last user event as the boundary shared by foldEvent and groupTurns. A plan in a later turn therefore remains in that later turn instead of replacing an earlier plan in place.

activityTailKey now includes each plan entry's status and priority, while excluding entry text. An in-place status change therefore refreshes the activity row without making long task text part of the key. The generic summarizeTurnActivity count remains sensible because each turn has one current plan snapshot and the expanded activity shows its entries.

The ACP normalization helpers are private because the adapter is their only consumer. The server event union in agent-chat/types.ts and the browser copy in agent-chat/src/session.ts remain hand-synced by design for this follow-up. The next structural fix should give one module ownership of the union and have the other re-export it, or add a parity test that catches drift when a new event variant is added.

Testing

  • Before the fix, test commit f5024d4e534: cd agent-chat && bun test test/plan-scope.test.ts returned 2 fail, including the later-turn plan count of [1, 0] and identical 1:unknown activity keys.
  • After the fix, commit 1daa7434362: the same command returned 2 pass, 0 fail, 5 expect() calls.
  • cd agent-chat && bun install && bun run check: 41 pass, 0 fail, 109 expect() calls across 12 Bun test files and 22 scripts.
  • python3 scripts/verify-local.py: selected 15 of 16 checks; all 15 selected checks passed. Native compilation, app tests, and app launch were not selected for this TypeScript-only change.
  • ./scripts/localize-changes: localization audit passed with 0 changed web message keys and 0 catalog parity errors.

Changelog

Fixed: A second ACP plan in a conversation no longer replaces the first plan in place.

🤖 Generated with Claude Code


Summary by cubic

Fixes two review findings in ACP plan handling: plans now stay within the turn where they arrive, and activity rows refresh when a plan entry's status changes.

  • A plan update replaces only the plan in the current turn, using the last user event as the boundary shared by foldEvent and groupTurns; a later-turn plan no longer replaces an earlier plan in place.
  • activityTailKey now includes each plan entry's status and priority while excluding entry text, so a status change refreshes the row without making long task text part of the key.
  • The ACP normalization helpers are now private because the adapter is their only consumer; the event union in agent-chat/types.ts and the browser copy in agent-chat/src/session.ts remain hand-synced by design.

Testing

  • New plan-scope.test.ts covers per-turn plan scope and plan activity keys.
  • Full suite passes: 41 tests, 0 failures across 12 test files.

Written for commit 1daa743. Summary will update on new commits.

Review in cubic

teamleaderleo and others added 2 commits September 30, 2026 02:11
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… key

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 8 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 64a07e59-7b40-4d95-8b11-a91902da90d9

📥 Commits

Reviewing files that changed from the base of the PR and between efb762c and 1daa743.

📒 Files selected for processing (4)
  • agent-chat/adapters/acp.ts
  • agent-chat/src/activity.ts
  • agent-chat/src/session.ts
  • agent-chat/test/plan-scope.test.ts

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.

❤️ Share

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

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@teamleaderleo teamleaderleo left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: I checked the per-turn fold boundary, activity key state inputs, normalization exports, summary behavior, and the focused regression coverage. I found no additional issues.
Fixed: I scoped plan replacement after the latest user event and keyed plan activity by status and priority without including entry text.
Left: I left the duplicated server and browser AgentEvent unions in place because consolidating their ownership needs a separate design and migration.

@teamleaderleo
teamleaderleo enabled auto-merge (squash) September 30, 2026 09:14
@teamleaderleo
teamleaderleo merged commit 4da3bb3 into main Sep 30, 2026
59 checks passed
@teamleaderleo
teamleaderleo deleted the parity/plan-scope branch September 30, 2026 09:19
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for 1daa743436, merged 2026-09-30 09:19:02 UTC

  • Not verified at merge: CI timing (in progress)
  • Verified: ci-status, Web complexity, web-validation, CI fast guards, Fast static checks, guards (18), ios-e2e-status, linux-preflight, route, Testbox broker trust boundary, tests
  • Skipped by policy: backend, browser, Claude request, Claude wrapper regressions, Dogfood build #​${{ github.event.pull_request.number }}, GhosttyKit release check, ios-e2e, mac-host, macos, macOS admission gate, remote-daemon, suite-coverage, and 5 more
  • Full suite: runs on main after merge.

@teamleaderleo

teamleaderleo commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator Author

Review, posted after the merge. Auto-merge took this before the review landed, so this is a read of the merged commit rather than a gate.

Both findings I raised on #15889 are addressed, and the shapes are right: the turn start is found by walking back to the last user block, which is exactly how groupTurns in src/turns.ts delimits a turn, and activityTailKey now keys on entry statuses and priorities. Excluding the entry text from that key is the right call and worth stating: the key only feeds useActivityStartedAt in Transcript.tsx:934, so including the text would restart the "working for Ns" counter every time an agent rewords a step it is still working on.

One defect the change introduced, on main now:

  • foldEvent's plan replacement keeps a plan block only when its index equals existingIndex, so every plan block outside the current turn is dropped. Before this PR there was at most one plan block in the list and that was harmless; with per-turn plans it is not. Sequence: plan A, user prompt, plan B, plan B'. On B' the reduce drops A, and turn 1 loses its plan card while turn 2 is still working. The test here does not catch it because it folds only two plan events, so existingIndex is -1 both times and the reduce never runs.

Fixed: nothing in this PR, it is merged.

Left: the one-line fix and its two test cases are in #15907 (a positional map instead of the filtering reduce, plus a four-event case and a three-updates-in-one-turn case). Fix forward, no revert: the scoping and the activity key stay as they are here.

rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 30, 2026
ecba57a fix(sidebar): cut with an ellipsis character so a reference cannot re-parse (manaflow-ai#15893)
6d2b5d1 feat(terminal): browser-style navigation layout and a terminalAlternateScreen shortcut key (manaflow-ai#14863)
0d3fdb1 test: print the simulator pipe output when the EOF assertion fails (manaflow-ai#15857)
46fe41a Fix cloud dogfood pause link-down journey (manaflow-ai#15918)
7d246ed fix: open existing Cloud workspace rows optimistically (manaflow-ai#15747)
1b06f84 fix(agent-chat): show ACP paths and diffs for tool calls (manaflow-ai#15908)
b413b7a fix(agent-chat): preserve earlier ACP plans during updates (manaflow-ai#15907)
8b75678 Persist Cloud display membership across clients (manaflow-ai#15748)
547340a fix(cloud): carry the machine author from /api/vm to the machine row's snapshot (manaflow-ai#15309)
e30de3d test: probe cloud agent status in Cloud VM journey (manaflow-ai#15875)
296537c docs(agent-chat): correct provider claims and pin ACP argv (manaflow-ai#15901)
e1dc959 Count the renamed Agent spawn tool as a subagent in the pi bridge (manaflow-ai#15865)
14fae18 dogfood: record the hover steps as trees, not frames (manaflow-ai#15845)
4da3bb3 fix(agent-chat): scope ACP plans to their turn and refresh activity (manaflow-ai#15898)
64ec56d feat(terminal): right-click a link to choose where it opens (manaflow-ai#15325)
efb762c Make unsupported remote browser warning dismissible (manaflow-ai#15726)
666c77f Cloud Machines sidebar: add persistent create buttons (manaflow-ai#15680)

# Conflicts:
#	.github/workflows/cloud-vm-dogfood.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant