Skip to content

fix(ag-ui): address #8163 review on pause checkpoint ownership - #2

Merged
FOWEPJF255 merged 1 commit into
FOWEPJF255:fix/ag-ui-interrupt-checkpoint-8150from
ktz03:fix/8163-review-followup
Sep 11, 2026
Merged

FOWEPJF255 merged 1 commit into
FOWEPJF255:fix/ag-ui-interrupt-checkpoint-8150from
ktz03:fix/8163-review-followup

Conversation

@ktz03

@ktz03 ktz03 commented Sep 11, 2026

Copy link
Copy Markdown

Motivation & Context

Follow-up for microsoft#8163 review threads from @eavanvalkenburg and @moonbox3.

Description & Review Guide

  • Workflow owns the run boundary: _run_core captures _run_baseline_checkpoint_id and _restored_checkpoint_id; resolve_pause_checkpoint_id uses them by default.
  • Exclude restored checkpoint after resume so a failed pause save cannot re-advertise pre-response state.
  • @eavanvalkenburg: remove issue reference in docstring; emit via existing _build_run_finished_event (no _build_run_finished_with_checkpointed_interrupts); move core tests into test_workflow.py and delete the dedicated test file.
  • AG-UI no longer threads baseline_checkpoint_id / resume known_checkpoint_id through RUN_FINISHED.

Happy to adjust naming if you prefer a different helper split.

- Workflow captures run baseline / restored id inside run(); resolve uses them
  so callers need not thread baseline_checkpoint_id.
- Exclude restored checkpoint from pause candidates after resume.
- Drop issue reference in docstring; emit RUN_FINISHED via existing
  _build_run_finished_event after attaching pause id.
- Move core pause-resolve coverage into test_workflow.py (no new test file).
@FOWEPJF255

Copy link
Copy Markdown
Owner

Thanks for addressing the pause-checkpoint ownership notes on microsoft#8163.

I will review this fork PR next and push the ownership fixes upstream so workflow owns the run boundary as requested.

@FOWEPJF255
FOWEPJF255 merged commit d42a8b3 into FOWEPJF255:fix/ag-ui-interrupt-checkpoint-8150 Sep 11, 2026
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