Skip to content

fix: isolate terminal plugin error snapshots - #866

Open
zhongkechen wants to merge 6 commits into
mainfrom
codex/fix-852-plugin-error-snapshots
Open

zhongkechen wants to merge 6 commits into
mainfrom
codex/fix-852-plugin-error-snapshots

Conversation

@zhongkechen

@zhongkechen zhongkechen commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Issue #, if available

Fixes #852.

Description of changes

Terminal operation hooks now copy the error's mutable stack-trace list at the two
existing assignments: _end_info for checkpointed operations and
on_child_context_end for child contexts that finish without a checkpoint.
A plugin clearing its snapshot cannot change stored error diagnostics, a later
snapshot, or the ChildContextError subsequently raised to the caller.

The runtime diff is exactly two error= lines. OperationInfo.from_operation
remains a staticmethod, and both terminal-info constructors keep their existing
field conversion and delivery behavior.

Design

  • Design note (rule 3): not needed; this corrects snapshot ownership without
    changing the public API. The requested review revision narrows the change to
    the two existing error assignments.
  • Simpler options considered (rule 2): wrap each existing error expression in
    _copy_error. This reuses the narrow list copy without introducing a factory
    refactor, subclass behavior change, deep copy, or scheduling changes.
  • Root cause (rule 5): terminal snapshots shared the input ErrorObject's mutable
    stack-trace list. The virtual-child path also reuses the ErrorObject to raise
    ChildContextError after the terminal hook returns, so a plugin could change
    the user's exception even without a child checkpoint.
  • Net runtime source change: two lines added / two removed; no API additions or
    changes.

Validation

  • Existing regressions reproduce checkpointed terminal-error aliasing across
    STEP, CALLBACK, CHAINED_INVOKE, and CONTEXT, through natural and queued delivery.
  • The added virtual-child unit/public-handler regressions failed on the previous
    PR head (235d8de): both observed a cleared original/raised stack trace. They
    pass with the second error-copy assignment.
  • Focused regressions: 6 passed and 11 subtests passed.
  • hatch run dev-core:test --tb=short: 2,121 passed and 24 subtests passed.
  • hatch run dev-core:typecheck: no issues in 95 source files.
  • Package hatch fmt --check, commit-message validation, and git diff --check:
    passed.
  • Python 3.13.17 and CI-pinned Hatch 1.16.5.

Public-handler coverage verifies stored step failures replay without executing
step bodies and virtual-child failures reach the caller with their original
message/type/data/stack despite a mutating terminal plugin. Metadata, hook counts,
replay flags, and absent/empty error semantics remain covered.

Existing test update: test_builds_terminal_operation_info now supplies a mutable
stack and verifies snapshot mutation leaves the input error intact; its previous
field/delivery expectations remain. The earlier PR-added subclass-factory test
is replaced by terminal-info field preservation, matching the unchanged static
factory contract.

Assumptions

  • Aliasing and error propagation are verified with real SDK error/plugin objects
    and the public durable handler. No new deployed-service assumption is needed
    for these ownership changes.
  • Deployed Lambda validation of this revision is unverified. This PR does not
    change OTel rendering or testing-service behavior.

Decisions not covered by the issue or the design note

  • Preserve the existing static converter as requested in review; subclass factory
    behavior is not changed by this fix.
  • Keep isolation scoped to terminal hooks and invocation operation-map snapshots.
    Plugins receiving the same hook object still share that snapshot.

Checklist

  • Both error-copy sites have regressions that fail without the copy (rule 4).
  • Regression commits precede their fixes (rule 5).
  • Test expectation changes are documented above.

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 mentioned this pull request Oct 27, 2026
3 tasks done
@zhongkechen
zhongkechen marked this pull request as ready for review October 27, 2026 23:48
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 27, 2026 23:48 — with GitHub Actions Active
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 27, 2026 23:49 — with GitHub Actions Active
@github-actions

Copy link
Copy Markdown
Contributor

Codex AI review

No actionable findings or violations of CONTRIBUTING rules 1–9 identified. Unit and e2e regressions cover both changed error-copy paths. Residual test risk: tests and type checks were inspected but not executed during this read-only review.

Reviewed commit dfce1bd8f1605ea03e948eba7ec94e3f09187269. Workflow run

This branch was successfully deployed

1 active deployment
ai-pr-review-runtime — dfce1bd8 Deployed Oct 27, 2026 by zhongkechen via ai-pr-review / Codex review / Generate Codex review #2547
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.

[Bug]: Isolate terminal plugin error snapshots from checkpointed state

1 participant