Skip to content

fix: bind HITL resumes to the reviewed tool invocation - #146

Merged
Asaf-prog merged 2 commits into
mainfrom
codex/fix-139-hitl-resume-binding
Oct 14, 2026
Merged

Asaf-prog merged 2 commits into
mainfrom
codex/fix-139-hitl-resume-binding

Conversation

@Asaf-prog

@Asaf-prog Asaf-prog commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Checkpoint model responses and completed tool outcomes so approval resumes reuse the reviewed request without repeating earlier side effects. Validate invocation bindings and fail closed on mismatched or legacy continuations.

Honor claimed decisions across session-grant and auto-mode changes, preserve execution limits, isolate concurrent resumes, and retain mismatch errors when approval cleanup fails. Reject resumed runs whose approval binding is never consumed, including rebuilt graphs that remove or block the reviewed invocation. Document the continuation format and add regression coverage for streaming, nested approvals, provider changes, replay safety, and removed pending tools.

Validation: CI check and ui passed at c39c2dd; pytest collects 1052 tests.

Refs #139.

Checkpoint model responses and completed tool outcomes so approval resumes
reuse the reviewed request without repeating earlier side effects. Validate
invocation bindings and fail closed on mismatched or legacy continuations.

Honor claimed decisions across session-grant and auto-mode changes, preserve
execution limits, isolate concurrent resumes, and retain mismatch errors when
approval cleanup fails. Document the continuation format and add regression
coverage for streaming, nested approvals, provider changes, and replay safety.

Validation: make check passed with 1050 tests.
Refs #139.
@Asaf-prog
Asaf-prog requested a review from AmitAvital1 October 14, 2026 16:36
@Asaf-prog Asaf-prog self-assigned this Oct 14, 2026

@AmitAvital1 AmitAvital1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the full HITL continuation change against #139, including the approval lifecycle, checkpoint replay semantics, nested agents, sequential approvals, policy changes, concurrency, execution limits, and stream/non-stream paths.

I found one additional fail-closed edge case during review: after a rebuild removed the pending tool, replay could finish with an unconsumed binding and mark the approval approved even though the reviewed invocation never reached its gate. Fixed in c39c2dd by requiring every resume binding to be consumed before graph completion, with regression coverage for both resume modes and synchronized docs/ADR.

The final implementation keeps resume state request-local, checkpoints model/tool substeps without repeating side effects, validates the security-relevant invocation while excluding transient provider call IDs, rejects mismatched/legacy/unconsumed continuations, and preserves execution-limit accounting. I found no remaining blocking issues or unnecessary architectural duplication.

Validation: CI check and ui are green; 1052 tests are collected. Approving.

@Asaf-prog
Asaf-prog merged commit c276763 into main Oct 14, 2026
3 checks passed
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