Skip to content

fix: record callback terminal operation updates - #868

Open
zhongkechen wants to merge 4 commits into
mainfrom
codex/fix-860-callback-updated-operations
Open

zhongkechen wants to merge 4 commits into
mainfrom
codex/fix-860-callback-updated-operations

Conversation

@zhongkechen

Copy link
Copy Markdown
Contributor

Issue #, if available

Fixes #860.

Description of changes

An externally completed callback could resume with terminal state but no
UpdatedOperationIds entry, so its first completion was treated as replay and
the terminal plugin hook could be missing before dependent code ran.

Record the callback operation ID after accepted success, failure, and timeout
transitions, under each path's existing state lock. The existing helper keeps
insertion idempotent. Results, diagnostics, sequence/token behavior, and rejected
completion validation remain unchanged.

Design

  • Design note (rule 3): not needed; this corrects existing completion metadata
    through existing public APIs and adds no API or option.
  • Simpler options considered (rule 2): reuse _record_updated_operation, as wait,
    retry, and chained-invoke completion already do. A separate delivery ledger or
    changes to plugin hook scheduling are unnecessary.
  • Root cause (rule 5): callback completion touched the operation's sequence and
    replaced its state but omitted the existing updated-ID bookkeeping call.
  • Net change: three source lines, no new public API members.

Validation

  • First commit adds regressions: 12 failed, 6 passed against unmodified main.
    The failures demonstrate missing pending IDs and missing terminal hooks before
    the dependent step. Already-pending-ID controls pass on the baseline.
  • After the fix: 18 focused tests passed.
  • hatch run dev-testing:test -q --tb=short: 1766 passed. The full
    suite ran with loopback access after the sandbox denied web-runner binds.
  • hatch run dev-testing:typecheck: no issues in 137 source files.
  • Package hatch fmt --check: passed.
  • hatch run dev-testing:js-examples --js-dir ... --no-build: 123 passed,
    zero retries/failures, four files without cloud tests
    out of 127 selected,
    using the existing build at 3deb8389531d35642e2d4657a69575522dc7a7ea.
  • Python 3.13.17, CI-pinned Hatch 1.16.5, and Node.js 22.

Unit tests cover success/failure/timeout, idempotent ID insertion, unknown and
already-terminal rejection, unchanged unrelated callbacks, sequence/token
accounting, both stores, and input construction through both invokers. Public
handler tests cover suspension/resume on memory and filesystem stores, snapshot
visibility, the first terminal hook's non-replay flag, status/result/error, and
delivery before the dependent step. A saved step executes once.

No existing test expectations change.

Assumptions

Decisions not covered by the issue or the design note

Checklist

  • Every new branch has a test that fails without it (rule 4).
  • The first commit adds failing regressions (rule 5).
  • No existing test expectations were changed.

By submitting this pull request, I confirm that you can use, modify, copy, and
redistribute this contribution, under the terms of your choice.

@zhongkechen
zhongkechen marked this pull request as ready for review October 28, 2026 21:40
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 28, 2026 21:40 — with GitHub Actions Active
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 28, 2026 21:41 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime November 1, 2026 03:41 — with GitHub Actions Active
@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Codex AI review

No actionable correctness or approach findings after reviewing the complete diff against CONTRIBUTING rules 1–9. Tests were not executed during this read-only review; deployed Lambda parity remains explicitly unverified in the PR.

Reviewed commit 18e88c2c2061cd3238992717925dcbca9fe3205f. Workflow run

This branch was successfully deployed

1 active deployment
ai-pr-review-runtime — 18e88c2c Deployed Nov 1, 2026 by zhongkechen via ai-pr-review / Codex review / Generate Codex review #2691
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant