Skip to content

fix: isolate terminal plugin error snapshots - #866

Merged
zhongkechen merged 7 commits into
mainfrom
codex/fix-852-plugin-error-snapshots
Oct 28, 2026
Merged

zhongkechen merged 7 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 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

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime October 28, 2026 17:47 — with GitHub Actions Active
@github-actions

Copy link
Copy Markdown
Contributor

Codex AI review

No actionable findings in the complete diff or under CONTRIBUTING rules 1–9. Added regressions cover both error-copy paths. Tests were inspected but not executed, so runtime results remain independently unverified.

Reviewed commit 79153189006b43cd3799d2e0992ab3370456c634. Workflow run

@SilanHe
SilanHe self-requested a review October 28, 2026 18:17
@zhongkechen
zhongkechen merged commit e42947e into main Oct 28, 2026
29 checks passed
@zhongkechen
zhongkechen deleted the codex/fix-852-plugin-error-snapshots branch October 28, 2026 18:33

This branch was successfully deployed

1 active deployment
ai-pr-review-runtime — 79153189 Deployed Oct 28, 2026 by zhongkechen via ai-pr-review / Codex review / Generate Codex review #2567
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

2 participants