Repository navigation
Python: Stamp AG-UI checkpoint owner on every save - #8011
Evan Mattson (moonbox3) merged 3 commits into
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
github-actions
Bot
changed the title
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation matches the stated intent and includes appropriate regression coverage.
Pull request overview
Ensures all AG-UI workflow checkpoints retain request ownership metadata.
Changes:
- Stamp ownership on every checkpoint save.
- Add regression coverage for checkpoints without pending events.
File summaries
| File | Description |
|---|---|
_workflow.py |
Makes ownership stamping unconditional. |
test_endpoint.py |
Tests persisted ownership without pending events. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
@microsoft-github-policy-service agree |
Evan Mattson (moonbox3)
left a comment
Evan Mattson (moonbox3)
left a comment
There was a problem hiding this comment.
Please use the proper PR template.
The resume path only compared ownership when the checkpoint had pending request info events, so a checkpoint with an owner but no pending events skipped the check entirely. Combined with stamping ownership on every save, a caller could resume another Snapshot Scope's clean checkpoint and have subsequent saves re-stamped to themselves. Validate on owner presence instead. Checkpoints saved before ownership was recorded have no owner and continue to resume unchanged.
GitHub Actions
Inactive
GitHub Actions
Inactive
|
You're right — and stamping on every save is what completes it: the clean checkpoint now carries an owner the pending-ids gate never reads, and the next save re-stamps it to the caller. Widened to fire on owner presence instead: if checkpoint_owner is not None and checkpoint_owner != request_owner:Legacy ownerless checkpoints still resume — happy to flip those to fail-closed if you'd prefer. AI-assisted. |
|
You should still file an issue for this bug, if one doesn't exist. Then we can attach the PR to it. |
|
Choppa (@Choppaaahh) also per our contributing guidelines you must reply to the open PR comments and close them when addressed. |
Please keep this in mind for next time, thanks. |
|
Filed #8053 for this bug and linked the fix, per your note. Thanks for the review and the merge — and noted on replying to + resolving review threads inline next time. AI-assisted. |
Motivation & Context
AG-UI workflow checkpoints only recorded their request owner when the checkpoint carried pending request-info events, and the resume path only validated ownership when those pending events were present. A checkpoint saved without pending events was therefore stored unowned and resumable by any caller.
Description & Review Guide
What are the major changes?
_OwnedWorkflowCheckpointStorage.savestampsag_ui_workflow_request_owneron every save, not only when pending request-info events exist.checkpoint_pending_idsis non-empty.What is the impact of these changes?
A resume request can no longer restore a checkpoint belonging to a different Snapshot Scope. Without (2), (1) alone would make this worse rather than better: the checkpoint would carry an owner that the resume path never reads, and the next save would re-stamp it to the caller.
Checkpoints saved before ownership was recorded carry no owner and continue to resume unchanged. A checkpoint stamped with an anonymous owner is only resumable anonymously.
What do you want reviewers to focus on?
Whether legacy ownerless checkpoints should keep resuming, or fail closed.
Related Issue
N/A — found by inspection.