[plugin sdk] Harden finalize retry and run context cleanup#75600
Conversation
|
Codex review: needs maintainer review before merge. Summary Reproducibility: not applicable. This is an additive SDK PR rather than a user bug report; source inspection shows current main lacks the proposed retry and cleanup behavior, and the PR adds focused regression coverage. Next step before merge Security Review detailsBest possible solution: Review and land this focused SDK slice after maintainer acceptance of the public retry metadata contract, keeping the broader workflow and recipe follow-ups in their separate PRs. Do we have a high-confidence way to reproduce the issue? Not applicable. This is an additive SDK PR rather than a user bug report; source inspection shows current main lacks the proposed retry and cleanup behavior, and the PR adds focused regression coverage. Is this the best way to solve the issue? Yes, pending maintainer acceptance. The patch keeps the behavior in the plugin SDK host/runtime boundary, documents the new contract, and addresses prior review concerns with targeted tests instead of reopening the larger consolidated stack. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against a90be474f441. |
16eb912 to
b4bee82
Compare
b4bee82 to
f1737af
Compare
2664e2b to
24434fb
Compare
|
Fixed in |
24434fb to
bb90aef
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens plugin SDK lifecycle behavior around before_agent_finalize retry handling and plugin run-context/session cleanup, aiming to prevent runaway retries and to avoid leaking or resurrecting per-run state while cleanup is in progress.
Changes:
- Add bounded, per-run retry budgeting for
before_agent_finalizeviaretrymetadata (instruction/idempotencyKey/maxAttempts) and related tests/docs. - Keep plugin run context available until terminal agent-event subscriptions settle, while preventing late non-terminal handlers from writing into closed runs.
- Add a bounded timeout wrapper for plugin cleanup hooks and refine scheduler-job cleanup to preserve live jobs and avoid stale cleanup callbacks.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugins/host-hook-runtime.ts | Prevent closed-run run-context resurrection; wait/log for terminal subscriptions; improve scheduler cleanup controls. |
| src/plugins/host-hook-cleanup.ts | Wrap cleanup hooks in timeout; allow cfg to default; preserve scheduler jobs and avoid stale cleanup callbacks. |
| src/plugins/host-hook-cleanup-timeout.ts | New helper to apply a bounded timeout to cleanup callbacks. |
| src/plugins/host-hook-cleanup-timeout.test.ts | Unit test for timeout timer unref() behavior. |
| src/plugins/hooks.ts | Merge logic for before_agent_finalize now normalizes/propagates retry metadata. |
| src/plugins/hooks.before-agent-finalize.test.ts | Adds coverage for merging revise decisions with empty/valid retry instructions. |
| src/plugins/hook-types.ts | Extends PluginHookBeforeAgentFinalizeResult with optional retry metadata. |
| src/plugins/contracts/run-context-lifecycle.contract.test.ts | Contract coverage for closed-run behavior, terminal subscription settling, and cleanup timeout behavior. |
| src/agents/harness/lifecycle-hook-helpers.ts | Implements finalize-retry budgeting keyed by runId and bounded by maxAttempts. |
| src/agents/harness/lifecycle-hook-helpers.test.ts | Tests retry budgeting and per-run clearing behavior. |
| docs/plugins/hooks.md | Documents before_agent_finalize retry metadata for plugins. |
| docs/.generated/plugin-sdk-api-baseline.sha256 | Updates SDK API baseline hashes for the new surface. |
| CHANGELOG.md | Changelog entry for the new SDK retry/cleanup hardening. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24434fbb50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5d4ec3231
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b813d2bc17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5627a4a692
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a7ff718 to
ec58a62
Compare
|
Merged via squash.
Thanks @100yenadmin! |
…75600) Merged via squash. Prepared head SHA: ec58a62 Co-authored-by: 100yenadmin <[email protected]> Co-authored-by: jalehman <[email protected]> Reviewed-by: @jalehman
…75600) Merged via squash. Prepared head SHA: ec58a62 Co-authored-by: 100yenadmin <[email protected]> Co-authored-by: jalehman <[email protected]> Reviewed-by: @jalehman
…75600) Merged via squash. Prepared head SHA: ec58a62 Co-authored-by: 100yenadmin <[email protected]> Co-authored-by: jalehman <[email protected]> Reviewed-by: @jalehman
…75600) Merged via squash. Prepared head SHA: ec58a62 Co-authored-by: 100yenadmin <[email protected]> Co-authored-by: jalehman <[email protected]> Reviewed-by: @jalehman
…75600) Merged via squash. Prepared head SHA: ec58a62 Co-authored-by: 100yenadmin <[email protected]> Co-authored-by: jalehman <[email protected]> Reviewed-by: @jalehman
Summary
This replaces the consolidated #73384 / #74483 stack with a smaller fresh-main slice. The previous consolidated branch became extremely brittle under constant upstream churn: rebases repeatedly invalidated generated baselines, review threads, and CI assumptions, which made the PR harder to review than the underlying changes warranted. This PR files one hook/seam area at a time so maintainers can review, test, and merge each contract independently without re-opening the whole workflow stack.
Links
Non-goals
Test Plan
pnpm test src/plugins/hooks.before-agent-finalize.test.ts src/agents/harness/lifecycle-hook-helpers.test.ts src/plugins/contracts/host-hooks.contract.test.ts src/plugins/contracts/run-context-lifecycle.contract.test.tspnpm test src/plugins/contracts/host-hooks.contract.test.ts src/plugins/contracts/run-context-lifecycle.contract.test.tspnpm check:test-typespnpm lint:coregit diff --checknpm run check:no-conflict-markers