Pass sessionId to embedded subscription and adjust subagent steer-restart handling#192
Conversation
cbffbd6
into
dgarson/hooks-add-run-and-model-calls
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f06edaf87e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| status: "ok", | ||
| value: "steered", | ||
| }; | ||
| entry.outcome = undefined; |
There was a problem hiding this comment.
Keep a completion sentinel when steering restarts
Setting entry.outcome to undefined removes the only guard used by the lifecycle error grace timer callback (entry.outcome?.status === "ok" in schedulePendingLifecycleError) to ignore stale queued error callbacks. If a transient error timer callback is already queued when subagent_stopping blocks and triggers a steer-restart, that callback can now proceed and call completeSubagentRun with an error outcome, incorrectly finalizing/announcing a run that was intentionally being restarted.
Useful? React with 👍 / 👎.
Motivation
sessionIdfield so callers can rely on a dedicated identifier rather than inferring it fromsessionKey.resultis undefined by guarding access toresult.blockand correct the subagent steer-restart bookkeeping so a steer restart does not mark the run as completed.Description
sessionId?: stringto theSubscribeEmbeddedPiSessionParamstype so the subscriber API accepts an explicit session identifier.sessionId: params.sessionIdinto the call tosubscribeEmbeddedPiSessioninrunEmbeddedAttemptso the subscriber receives the new field.completeSubagentRunto use optional chaining (result?.block) to avoid errors whenresultis undefined.entry.outcometo{ status: "ok", value: "steered" }and instead clearentry.outcomeby assigningundefinedso the run is not treated as completed during restarts.Testing
tscand the build succeeded.npm testand all unit tests passed.npm run lintand no lint errors were reported.Codex Task