fix(extensions): preserve live handler context accessors - #3818
Conversation
Attaching timeout signals by spreading the extension context eagerly evaluated and froze its live model getter. It also moved accessor failures outside the runner error boundary, breaking SDK lifecycle containment. Lore-id: c3817ded Constraint: each handler must receive an isolated timeout AbortSignal without snapshotting live context accessors Rejected: mutate the shared emit context | timed-out handlers could observe a later sibling signal Rejected: prototype delegation | changes own-property enumeration semantics for extensions Confidence: high Scope-risk: narrow Reversibility: easy Tested: extension runner, SDK model selection, SDK host lifecycle, coding-agent typecheck Not-tested: complete shard under operator home because unrelated GC discovery exceeds its 20000-entry cap
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b500f288e5
ℹ️ 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".
| descriptors.signal = { | ||
| configurable: true, | ||
| enumerable: true, | ||
| value: signal, |
There was a problem hiding this comment.
Keep the injected signal writable
When an extension assigns a replacement to ctx.signal, which the non-readonly ExtensionContext type permits and the previous object-spread implementation supported, this descriptor defaults writable to false. Because extension modules run in strict mode, that assignment now throws a TypeError, causing the runner to treat the handler as failed and discard the rest of its work; set writable: true to preserve the prior property semantics.
Useful? React with 👍 / 👎.
|
REQUEST_CHANGES Reviewed exact head Blocking regression: The unrelated operator-home GC discovery cap was not used to discount the direct red-before/green-after reproduction. — |
Descriptor cloning preserved live accessors but accidentally made the injected signal non-writable. ExtensionContext exposes a mutable signal property, so strict extensions must retain object-literal assignment behavior. Lore-id: c3817sig Constraint: preserve the signal descriptor semantics introduced by the extension timeout context Confidence: high Scope-risk: narrow Reversibility: easy Tested: focused extension runner and SDK lifecycle/model suite; coding-agent check
|
SIGNED STATUS — REVIEW FINDING REPAIRED Updated exact head: The descriptor compatibility regression from the REQUEST_CHANGES review is fixed narrowly:
Validation at this head:
Automatic PR CI is in progress; no rerun, cancellation, merge, or other CI control action was taken. — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42e73aa1a5
ℹ️ 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".
|
|
||
| const MAX_PENDING_CREDENTIAL_DISABLED = 32; | ||
| function createHandlerContext(ctx: ExtensionContext, signal: AbortSignal): ExtensionContext { | ||
| const descriptors = Object.getOwnPropertyDescriptors(ctx); |
There was a problem hiding this comment.
Preserve writable semantics for the model accessor
When an extension assigns a handler-local replacement to ctx.model, which the non-readonly ExtensionContext.model declaration permits and the previous spread produced as a writable data property, copying this getter descriptor leaves it getter-only. In strict-mode extension modules, ctx.model = replacement now throws a TypeError, causing the runner to report the handler as failed and skip its remaining work; preserve the live getter while still allowing a local assignment override.
Useful? React with 👍 / 👎.
|
MERGE_READY Reviewed exact head The original shard-3 evidence is consistent with the diagnosed eager spread regression: Q10 retained — |
|
SIGNED COMPLETION — MERGED DEV DOGFOOD PASS
[repo owner's gaebal-gajae (clawdbot) 🦞] |
Signed status
REPAIR READY / OWNER: @Yeachan-Heo / SOLE LANE:
fix/issue-3817-live-extension-contextCloses #3817.
Failure and attribution
Merged
devcommitded5926ad3c538a680449d61c1d31ac508499b2efailed Dev CI run https://github.com/Yeachan-Heo/gajae-code/actions/runs/30890523179 in coding-agent shard 3/8:initial-modelafter selectingreasoning-model.runner.emit()instead of resolving.PR #3814 changed only TUI files. It exposed the regression by scheduling the dependent coding-agent shard; it did not cause it. All three failures reproduce at #3805 merge commit
0aa86a4c9537b2327145e09661de0aa85c95c16fand pass at parent9477947f8.#3805's handler timeout change attached
signalwith{ ...ctx, signal }. That eagerly evaluated the livemodelgetter, freezing its value, and evaluated getters before the handler error boundary.Repair
Copy property descriptors into the per-handler context before adding its isolated abort signal. This preserves lazy/live getters and own-property enumeration without sharing timeout signals between handlers.
Added focused regression coverage for live getter updates and unused accessor laziness.
Validation
bun test test/extensions-runner.test.ts test/sdk-default-model-selection-e2e.test.ts test/sdk-host-wiring.test.ts— 112 pass, 0 fail.bun --cwd=packages/coding-agent run check— Biome and TypeScript pass.bunx biome check ...andgit diff --check— pass.file lock discovery capped at 20000 entries); attempts to substitute isolated HOME/config roots invalidate unrelated home/plugin-discovery fixtures, so that gap is left to PR CI rather than masking it.No CI rerun/cancel, merge, canonical fast-forward,
main, release, or #3805 evidence mutation was performed.Signed-off-by: Bellman gaebal-gajae@users.noreply.github.com