Skip to content

feat(advisor): merge final review notes without recursive reviews - #12387

Open
olegpulatov wants to merge 2 commits into
can1357:mainfrom
olegpulatov:contrib/advisor-pr3-final-review-flow
Open

olegpulatov wants to merge 2 commits into
can1357:mainfrom
olegpulatov:contrib/advisor-pr3-final-review-flow

Conversation

@olegpulatov

@olegpulatov olegpulatov commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

What

I improved control for final review flow.

  • Merge accepted final-review findings into one age-marked delivery
  • Request at most one permitted continuation for each final-review batch
  • Prevent advisor-triggered continuations from recursively scheduling reviews
  • Preserve default plan behavior and wake sleeping reviewers only for genuine user input

Why

Final reviews can arrive after work finishes. This keeps those findings visible and actionable without creating repeated review-continuation loops.

Depends on #12385. GitHub requires an upstream branch as the base for cross-fork PRs, so this draft targets main; review its final-flow commit after #12385.

Testing

  • bun test packages/coding-agent/test/advisor/ packages/coding-agent/test/advisor-tool-call-loop-guard.test.ts packages/coding-agent/test/agent-session-advisor-suppression.test.ts packages/tui/test/advisor-config.test.ts — 323 passed
  • bun test packages/coding-agent/test/agent-session-advisor-terminal-unwind.test.ts packages/coding-agent/test/advisor-context-maintenance.test.ts — 29 passed
  • Exercised final-boundary delivery with merged findings and confirmed advisor-triggered continuation does not schedule another review
  • bun check — passed

  • bun check passes
  • Tested locally
  • CHANGELOG updated with the required attribution (if user-facing; internal issue fixes use issue links, external contributions add the PR link and contributor credit after creation)

@olegpulatov
olegpulatov force-pushed the contrib/advisor-pr3-final-review-flow branch 5 times, most recently from c55e5c1 to dca0f9d Compare September 18, 2026 12:14
@olegpulatov
olegpulatov marked this pull request as ready for review September 18, 2026 12:21
@DarkPhilosophy

Copy link
Copy Markdown
Contributor

One note from reading the final-boundary flow (no action needed if intentional):

The finally block now clears #advisorTerminalBoundaryOpen before #flushAdvisorBoundaryNotes(). With advisor.syncBacklog: off the review drain can still emit after this callback returns, so those notes land outside the merge window and deliver individually rather than as part of the merged batch. #terminalUnwindActive is still held until the next real agent start, so they cannot steer against finished work — the behavior looks safe, just not merged in that case.

Since the old comment on that finally explicitly called out the microtask gap, it may be worth restating which of the two guards now covers it, so the next reader does not assume the merge window covers every post-boundary note.

@olegpulatov
olegpulatov force-pushed the contrib/advisor-pr3-final-review-flow branch 5 times, most recently from eb68983 to 7e3959e Compare September 21, 2026 21:05
Configure eligible review intervals and opt-in final-yield reviews. Support inherited or explicit per-advisor catch-up, including cancellable strict waits, without gating pending advice delivery.
Batch accepted findings with ages and request at most one permitted continuation. Apply live delivery constraints at final boundaries, retain default plan preservation, and wake reviewers only on genuine user input.
@olegpulatov
olegpulatov force-pushed the contrib/advisor-pr3-final-review-flow branch from 7e3959e to ae56d60 Compare September 21, 2026 21:35
@olegpulatov

Copy link
Copy Markdown
Contributor Author

One note from reading the final-boundary flow (no action needed if intentional):

The finally block now clears #advisorTerminalBoundaryOpen before #flushAdvisorBoundaryNotes(). With advisor.syncBacklog: off the review drain can still emit after this callback returns, so those notes land outside the merge window and deliver individually rather than as part of the merged batch. #terminalUnwindActive is still held until the next real agent start, so they cannot steer against finished work — the behavior looks safe, just not merged in that case.

Since the old comment on that finally explicitly called out the microtask gap, it may be worth restating which of the two guards now covers it, so the next reader does not assume the merge window covers every post-boundary note.

yes, thats exactly current default omp behaivour, - assistants which took too long will overfloat into later context.
just preserving it as is, to not block main agent for people who does not want it intentionally

appreciate getting another pair of eyes on the code.

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.

2 participants