refactor(sdk): make core own session lifecycle and attachments - #4098
refactor(sdk): make core own session lifecycle and attachments#4098Yeachan-Heo wants to merge 281 commits into
Conversation
![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: 4b8287e00a
ℹ️ 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".
| } finally { | ||
| await client.close(); |
There was a problem hiding this comment.
When the Broker request has already returned successfully but SdkClient.close() times out or throws, this finally replaces the successful result with a failure. Callers can then report a completed create/resume/close as unavailable and may retry it under a new provider request key, potentially producing duplicate lifecycle effects; client shutdown should be best-effort after the response has been received.
Useful? React with 👍 / 👎.
| function certaintyForBrokerCode(code: string): SessionLifecycleCertainty { | ||
| if (code === "terminal_uncertain") return "uncertain"; | ||
| if (code === "cleanup_pending") return "cleanup_pending"; | ||
| if (RETRYABLE_BROKER_ERRORS.has(code)) return "retryable"; | ||
| return "terminal"; |
There was a problem hiding this comment.
When SdkClient.global() times out after sending a mutation, it throws code timeout with details.requestSent: true, but this fallback classifies that outcome as terminal. Telegram consequently removes a create reservation on this result, even though the Broker may already have applied the mutation, allowing a subsequent request to create another session; sent timeouts and ambiguous connection closures must remain uncertain, while proven pre-send failures can be retryable.
Useful? React with 👍 / 👎.
| await this.#withEffectLease(effect.id, lease, async () => { | ||
| if (!(await this.#inboundEffectCurrent(claim, effect.id))) throw new SlackStaleEffectError(); | ||
| this.options.createClient(claim.endpoint).send(effect.payload); | ||
| claim.endpoint.send(effect.payload); | ||
| }); |
There was a problem hiding this comment.
When the Router rejects SessionAttachment.send() asynchronously—for example because reconciliation observes an endpoint roll between the preceding authority check and dispatch—this call is not awaited. The lease callback completes and the effect is journaled as sent on the following lines, so the inbound Slack reply is permanently lost instead of being classified and retried.
Useful? React with 👍 / 👎.
| */ | ||
| async function connectAttachedSession(endpoint: SdkSessionEndpoint): Promise<ChatDaemonSdkClient> { | ||
| return await SdkClient.connect(endpoint.url, endpoint.token, { ...ACP_SESSION_RECONNECT }); | ||
| routerDeps?: import("../router").SessionRouterDeps; |
There was a problem hiding this comment.
Replace this inline import("../router").SessionRouterDeps type with a top-level type import; the repository contract explicitly prohibits inline imports.
AGENTS.md reference: AGENTS.md:L113-L115
Useful? React with 👍 / 👎.
| @@ -0,0 +1,715 @@ | |||
| import { randomUUID } from "node:crypto"; | |||
| import * as fs from "node:fs/promises"; | |||
| import path from "node:path"; | |||
There was a problem hiding this comment.
Import node:path as a namespace (import * as path) rather than using a default import, as required by the repository's Node-module import convention.
AGENTS.md reference: AGENTS.md:L132-L132
Useful? React with 👍 / 👎.
25647db to
efb7222
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efb72224a9
ℹ️ 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".
| deps: { | ||
| ...opts.routerDeps, | ||
| onAttachment: attachment => this.#onAttachment(attachment), | ||
| onFrame: (attachment, frame) => this.#onRouterFrame(attachment, frame.body), |
There was a problem hiding this comment.
When the Telegram daemon restarts while a live endpoint still retains events, the new SessionRouter begins with a zero cursor and replays those events, but this callback discards frame.publicationId and forwards only the body. Because the replacement AttachmentSession also starts with replayPending: false, previously delivered final notifications and actions are processed as new live frames, producing duplicate Telegram messages after every daemon restart; retain a durable cursor or propagate the publication identity into Telegram's delivery journal.
Useful? React with 👍 / 👎.
| send: (data: string): void => { | ||
| void Promise.resolve(attachment.send(JSON.parse(data) as Record<string, unknown>)).catch(() => undefined); |
There was a problem hiding this comment.
When an attachment becomes stale during the asynchronous reconciliation performed by SessionAttachment.send(), this wrapper suppresses the rejected promise and returns synchronously. Inbound user-message, reply, and control paths therefore continue to mark the Telegram update as seen and show the queued reaction even though nothing reached the session, making the input permanently unretryable; dispatch must expose and await the send result before committing those acknowledgements.
Useful? React with 👍 / 👎.
| this.#clearModelChoiceAliases(session.sessionId); | ||
| if (this.topics.get(session.sessionId)?.authorityState === "active") | ||
| this.preservedInitiatorTopics.add(session.sessionId); | ||
| this.#sendAttachment(session, { |
There was a problem hiding this comment.
On initial attachment, this hello is sent through attachment.send(), which queues another SessionRouter reconciliation behind the reconciliation currently invoking onAttachment; the fire-and-forget call therefore reaches the socket only after SessionRouter.#attach() has already requested replay. If retained tool_activity frames precede an ordinary retained event, the host filters the capability-gated frames from that first replay and the later event advances the Router cursor past them, so Telegram permanently misses that activity state.
Useful? React with 👍 / 👎.
| if (expected.kind === "missing" || !ownershipLockMatches(expected, await readOwnershipLock(fsImpl, file))) | ||
| return false; | ||
| try { | ||
| await fsImpl.unlink(file); |
There was a problem hiding this comment.
If the transition fence expires or is stolen after this helper's read-back but before unlink() completes, a successor can replace the pathname and this plain unlink will delete the successor's live ownership lock. The final missing-path check cannot repair that deletion, allowing another daemon acquisition while the successor is still running; removal must remain bound to the exact file identity that was verified, as the previous exact-unlink path was.
Useful? React with 👍 / 👎.
efb7222 to
b0b154c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0b154c5fd
ℹ️ 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 liveIds = new Set(live.map(session => session.sessionId)); | ||
| const attachedIds = new Set<string>(); | ||
| for (const session of live) if (await this.#attach(session)) attachedIds.add(session.sessionId); |
There was a problem hiding this comment.
If any indexed live session has an endpoint that refuses the connection or never completes its hello handshake, #attach() rejects here and aborts the entire reconciliation loop. Both Telegram's run() and ChatDaemonRuntime.start() await this initial reconciliation, so one transiently unreachable or stale endpoint prevents the provider daemon from starting and blocks notifications for every otherwise healthy session; catch attachment failures per session and continue reconciling the remaining entries.
Useful? React with 👍 / 👎.
| ): Promise<SessionCreateOutcome | SessionCloseOutcome | SessionResumeOutcome | ExternalSessionResumeResult> { | ||
| const actor = this.lifecycleActor(); | ||
| if (parsed.kind === "create") | ||
| return this.lifecycleService.createExternal({ |
There was a problem hiding this comment.
Distinct /session_create updates now call the Broker directly without the former createRateLimiter(3, 10 * 60 * 1000) gate from buildOrchestratorDeps, and neither SessionLifecycleService nor the Broker applies an actor-scoped replacement. Consequently, an authorized Telegram user can submit unlimited unique updates and continuously spawn sessions; the handoff contract also says provider rate-limit state remains provider-owned, so this direct route needs an equivalent provider-side admission check.
Useful? React with 👍 / 👎.
| readonly modelPreset?: string; | ||
| readonly readinessTimeoutMs?: number; | ||
| }): Promise<SessionCreateOutcome> { | ||
| if (request.target.kind === "plain_dir") await fs.mkdir(request.target.path, { recursive: true }); |
There was a problem hiding this comment.
For a plain_dir request, this filesystem mutation runs before SessionLifecycleService.create() validates the actor, request key, and operation capability. A JavaScript caller of the exported lifecycle API can therefore submit an empty actor or mismatched capability, receive an authorization rejection, and still create an arbitrary directory with the process's permissions; perform validation before this side effect or move directory creation into the authorized Broker transaction.
Useful? React with 👍 / 👎.
OWNER_CONFIRMATION_REQUIREDExact head: The Telegram generation-guard repair is present and validated: the authoritative guard accepted the exact base/head comparison with required generation bumps, its 48-test suite passed, and the committed manifest byte-matches the tree. The rebased root check, coding-agent check/typecheck/runtime check, Rust check, native build/check, state gates, and focused SDK lifecycle/Router/Telegram cohort passed locally; exact-head CI also passed. Owner confirmation requiredThis deliberately removes compatibility surfaces rather than retaining shims. Please explicitly accept these user-facing/operational contract changes before merge:
No merge performed. Signed: GJC / PR #4098 owner validation / |
|
Owner confirm. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e535271dc
ℹ️ 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".
| readonly modelPreset?: string; | ||
| readonly readinessTimeoutMs?: number; | ||
| }): Promise<ExternalSessionResumeResult> { | ||
| const recent = await this.listRecent({ |
There was a problem hiding this comment.
When the exported resumeExternal() API receives an invalid actor, a mismatched capability, or an empty session prefix, it scans managed histories before this.resume() performs lifecycle validation. An empty prefix can therefore return the ambiguous response with session IDs and absolute workspace paths even though the request should be rejected as unauthorized or invalid; validate the request before calling listRecent(), as createExternal() already does.
Useful? React with 👍 / 👎.
| async #renderRecentFolderChoices(threadId: number, intendedSessionId: string): Promise<boolean> { | ||
| let recent: ListRecentSessionsResult; | ||
| async #renderRecentFolderChoices(threadId: number, providerRequestKey: string): Promise<boolean> { | ||
| let recent: Awaited<ReturnType<AgentDirSessionLifecycleService["listRecent"]>>; |
There was a problem hiding this comment.
Replace this inferred annotation with the concrete recent-session result type imported from the lifecycle module. The repository contract explicitly prohibits ReturnType<>, including when nested inside Awaited<>.
AGENTS.md reference: AGENTS.md:L114-L114
Useful? React with 👍 / 👎.
ea60e99 to
820f81d
Compare
There was a problem hiding this comment.
💡 Codex Review
Import ChatOperationRequest at the top level
Replace the newly added Pick<import("./chat-command-policy").ChatOperationRequest, ...> annotation with a top-level type import and reference the imported type directly; the repository contract explicitly prohibits inline type imports.
AGENTS.md reference: AGENTS.md:L113-L115
ℹ️ 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".
| stop: () => services.stopTelegramDaemon(ctx.settings), | ||
| }); | ||
| } | ||
| await services.stopTelegramDaemon(ctx.settings); |
There was a problem hiding this comment.
When stopTelegramDaemon() returns its normal { ok: false } failure result, awaiting it does not throw, so removeTelegramConfiguration() proceeds to erase the bot token and chat ID and reports success even though the old daemon remains running. This breaks the teardown-before-delete contract and can leave a supposedly removed integration polling or sending with credentials retained in memory; check ok and abort as the provider-runtime deactivation path already does.
Useful? React with 👍 / 👎.
| const response = await this.submitLifecycleFrame(frame); | ||
| let outcome: SessionCreateOutcome; | ||
| try { | ||
| outcome = await this.lifecycleService.createExternal({ |
There was a problem hiding this comment.
When an authorized user creates sessions through pending Telegram forum topics, #submitAdoptionFrame() invokes createExternal() without calling admitSessionCreate(), so opening distinct topics bypasses the restored three-per-ten-minutes provider limit. Fresh evidence since the earlier rate-limit comment is that the paired-chat path now applies the limiter at runLifecycleCommand(), while all three topic-adoption paths still converge on this unthrottled call.
Useful? React with 👍 / 👎.
| this.#sessions.delete(sessionId); | ||
| attached.dispose(); | ||
| await attached.client.close(); | ||
| await this.#deps.onSessionRemoved?.(attached.capability); |
There was a problem hiding this comment.
When a stale endpoint is reconciled and either client.close() times out or onSessionRemoved rejects, the attachment has already been deleted from #sessions; the exception aborts reconciliation, and subsequent passes no longer have an entry from which to retry provider cleanup. This can permanently leave Telegram's stale session object or a Slack/Discord thread mapping unclosed, so cleanup must be best-effort or retained for retry until the removal callback completes.
Useful? React with 👍 / 👎.
1551257 to
9ad635d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e0e0fb1d5
ℹ️ 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 endpoint = await readSdkSessionEndpoint(repo, indexed.sessionId, scope); | ||
| if (!endpoint || endpoint.stale) return null; | ||
| const endpointStat = await fs.stat(endpoint.path).catch(() => undefined); | ||
| if (!endpointStat || endpointStat.mtimeMs !== indexed.endpointMtimeMs) return null; |
There was a problem hiding this comment.
When an endpoint file belongs to a replacement process but its mtime still matches the indexed value—for example through a same-timestamp replacement on a coarse-resolution filesystem—this helper accepts it without checking endpoint.pid === indexed.pid, even though the index PID is the process authority. #attach() then connects with that replacement endpoint's token and may route Telegram frames or user replies to a process not authorized by the indexed session; the binding and activation paths already perform the missing PID comparison.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Use Bun.file for managed scope reads
Replace this production fs.readFile() call with Bun.file(...).text(): the repository contract requires Bun's file APIs for reads and reserves node:fs/promises for directory operations, keeping source and compiled runtimes on the supported filesystem abstraction.
AGENTS.md reference: AGENTS.md:L124-L132
ℹ️ 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 requestKey = this.providerRequestKey(updateId, text); | ||
| let outcome: SessionCreateOutcome | SessionCloseOutcome | SessionResumeOutcome | ExternalSessionResumeResult; | ||
| try { | ||
| outcome = await this.runLifecycleCommand(parsed, requestKey); |
There was a problem hiding this comment.
When the Telegram daemon exits or reloads after rememberSeenUpdateId() persists the update but before this newly direct lifecycle call reaches the Broker, Telegram redelivers the update and the seen-ID check discards it, so a create, close, or resume command can be permanently lost without any Broker ledger entry. Persist a pending intent first or mark the update seen only after the idempotent Broker request has been submitted.
Useful? React with 👍 / 👎.
| return { code, message, ...(requestSent === undefined ? {} : { requestSent }) }; | ||
| } | ||
|
|
||
| const TRANSPORT_ERROR_CODES = new Set(["timeout", "connection_closed", "reconnect_exhausted", "unavailable"]); |
There was a problem hiding this comment.
If the Broker sends malformed JSON after receiving a mutation, SdkClient.#onMessage rejects the already-sent pending request with a bare protocol_error; because this set omits that code, the service classifies the result as terminal even though the Broker may have applied the effect. Fresh evidence beyond the earlier timeout/closure issue is that this pending-frame parse path carries no requestSent field, so these errors need an uncertain classification rather than permitting a new lifecycle request key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91f8b81f9b
ℹ️ 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".
| await this.attachmentRouter | ||
| .stop() |
There was a problem hiding this comment.
When shutdown begins while a Router frame is awaiting a Telegram Bot API request, SessionRouter.stop() waits for all pending frame callbacks, but the fetch remains active because effects.beginShutdown() and #deliveryAbort.abort() run only afterward. Since the Telegram transport has no independent request timeout, a stalled request can block graceful stop or reload indefinitely, preventing the remaining ownership-release and persistence steps; abort provider effects before waiting for the Router to drain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14f5aff6f0
ℹ️ 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 seq = typeof frame.seq === "number" && Number.isSafeInteger(frame.seq) ? frame.seq : undefined; | ||
| const ownsSequence = | ||
| correlated.generation === attached.generation && | ||
| (correlated.sessionId === undefined || correlated.sessionId === attached.sessionId); | ||
| if (seq !== undefined && ownsSequence) { | ||
| if (seq <= attached.cursor.seq) return; | ||
| const held = attached.barrier.held; | ||
| if (held && origin === "live") { | ||
| if (held.length >= REPLAY_BARRIER_LIMIT) { | ||
| this.#failBarrier(attached, `hold buffer overflowed at ${REPLAY_BARRIER_LIMIT} frames`); | ||
| return; | ||
| } | ||
| held.push({ seq, frame }); | ||
| return; | ||
| } | ||
| } | ||
| const publicationId = | ||
| seq !== undefined && ownsSequence ? `${attached.sessionId}:${attached.generation}:${seq}` : undefined; |
There was a problem hiding this comment.
In production, pushSessionFrame() in sdk/bus/index.ts retains a sequenced event wrapper but broadcasts the unwrapped payload, so live notifications arrive here without seq; they receive no publicationId and do not advance attached.cursor. After any transient WebSocket reconnect, the reconnect handler replays from that stale cursor, causing every retained Slack, Discord, or Telegram notification already delivered live to be published again. The reconnect tests miss this because their fake host delivers the sequenced wrapper for live events rather than the payload shape used by the real host.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f75eedb0e3
ℹ️ 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".
| effect.payload.idempotencyKey, | ||
| ); | ||
| else await endpoint.send(effect.payload); | ||
| else await attachment.send(effect.payload); |
There was a problem hiding this comment.
When the endpoint generation rolls after an inbound effect is claimed, attachment.send() reconciles the Router and throws SessionRouterError("pre_send") without sending the frame. Discord's #isDefiniteSdkPreSendFailure() does not recognize that error, so the catch records the inbound effect as uncertain; inbound uncertain effects are excluded from lease recovery, permanently losing the user's message or action despite a proven pre-send failure. Classify SessionRouterError with phase pre_send as retryable/accepted, as the Slack path does, and cover this error-mapping state transition with a regression test.
AGENTS.md reference: AGENTS.md:L156-L160
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58d36e859b
ℹ️ 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".
| await this.#deps.onFrame?.( | ||
| attached.capability, | ||
| publicationId === undefined ? correlated : { ...correlated, publicationId }, |
There was a problem hiding this comment.
When a live or replayed frame explicitly names another session or generation, ownsSequence becomes false, but the frame is still passed to onFrame. Telegram uses the default correlator and its callback validates only that the attachment itself is current, so a stale or misaddressed notification/action is rendered under the attachment's Telegram session; ordinary Slack/Discord frames also do not reject a mismatched generation. Frames with a stated identity that contradicts the attachment should be dropped before invoking the provider callback, while identity-less control frames can remain allowed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7a925318b
ℹ️ 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".
| } catch (error) { | ||
| if (this.sessions.get(session.sessionId) === session) | ||
| logger.warn(`notifications: Telegram attachment handshake failed: ${sanitizeDiagnostic(String(error))}`); |
There was a problem hiding this comment.
When either attachment send fails during initialization—for example, because the index refresh fails transiently or the socket disconnects between the hello and replay sends—this catch only logs the error. The session remains replayPending: true, so handleSessionMessage() indefinitely appends every subsequent Router frame to replayQueue, while later reconciliations reuse the attachment without invoking #onAttachment again. Retry the handshake or revoke the attachment so it can be rebuilt; otherwise notifications for that session remain stalled until a daemon restart or endpoint replacement.
Useful? React with 👍 / 👎.
| for (const [sessionId, attached] of this.#sessions) { | ||
| this.#sessions.delete(sessionId); | ||
| attached.dispose(); | ||
| await attached.client.close(); |
There was a problem hiding this comment.
If one client.close() rejects during shutdown, this sequential loop exits immediately, leaving every later attachment in #sessions undisposed and still considered current. Telegram catches the Router stop error and continues its shutdown, so those remaining clients can retain frame handlers and accept frames while provider state and ownership are being released; close attachments independently with all-settled cleanup so one transport failure cannot prevent the rest from being revoked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f2fb3ebe7
ℹ️ 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".
| { | ||
| idempotencyKey, | ||
| ...(request.timeoutMs === undefined ? {} : { timeoutMs: request.timeoutMs }), |
There was a problem hiding this comment.
For session.create, session.fork, and session.resume calls that omit timeoutMs—including the current Telegram create path—this falls back to the bridge client's 10-second deadline, while startup-budget.ts permits 10 seconds of admission queueing followed by a fresh 10-second readiness window plus slack. Under queue contention or a startup lasting just over 10 seconds, the sent mutation is reported as uncertain even though the Broker legitimately continues and may create or resume the session; explicit external timeouts are also only readinessTimeoutMs + 1_000. Derive the client deadline with lifecycleRequestTimeoutMs() so it covers the Broker's complete budget.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a11fad7ec9
ℹ️ 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".
| for (const result of [{ requestId: item.requestId, commitKey: item.commitKey }, ...item.followers]) { | ||
| item.session.ws.send( | ||
| item.session.transport.send( |
There was a problem hiding this comment.
When an attachment rolls after the readyState check, this async transport.send() rejects, but its promise is ignored and resolveSettled() has already released the waiting Router callback. A delivered selected-ack publication is consequently journaled and its replay cursor advances even though the host never receives ask_selected_ack_result; the host then times out with an unknown outcome and cannot recover the cached result. Settle the publication only after the reply send has completed or been durably queued.
Useful? React with 👍 / 👎.
| timeoutMs === undefined ? undefined : { timeoutMs }, | ||
| ); | ||
| } finally { | ||
| await client.close(); |
There was a problem hiding this comment.
When an authorized Slack or Discord /sdk global session.list request has already returned but client.close() rejects, this finally replaces the valid Broker reply with a cleanup exception. ChatDaemonRuntime then classifies the inbound command as ambiguously delivered instead of posting its successful result, so the provider journal can leave an otherwise completed command unrecoverable; closing this one-shot client should be best-effort after a response is received.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97854a4c76
ℹ️ 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".
| this.#failDelivery(attached, seq, error); | ||
| return; | ||
| } | ||
| if (disposition === "deferred") return; |
There was a problem hiding this comment.
When sequence N returns deferred but a later sequence settles, this early return leaves N untracked while the later frame advances attached.cursor.seq beyond it. If Telegram exits before its queued effect is delivered, the persisted claim contains only the publication identity—not the frame payload—and the next replay starts after the newer cursor, so notification N is permanently lost; retain the deferred frame or prevent subsequent cursor advancement until it settles.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,611 @@ | |||
| import { createHash } from "node:crypto"; | |||
There was a problem hiding this comment.
Replace this named Node-module import with a namespace import (for example, import * as crypto from "node:crypto") and call crypto.createHash; the repository contract requires namespace imports for Node modules.
AGENTS.md reference: AGENTS.md:L132-L132
Useful? React with 👍 / 👎.
| @@ -0,0 +1,745 @@ | |||
| import { randomUUID } from "node:crypto"; | |||
There was a problem hiding this comment.
Replace this named Node-module import with a namespace import (for example, import * as crypto from "node:crypto") and call crypto.randomUUID; the repository contract requires namespace imports for Node modules.
AGENTS.md reference: AGENTS.md:L132-L132
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc5957cf16
ℹ️ 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".
| } | ||
| if (!send.identity && !existingTopic && !this.flatIdentitySent.has(logicalSessionId)) { | ||
| this.rememberPendingThreadedFrame(session, send, threadedFrame, toolActivity); | ||
| this.rememberPendingThreadedFrame(session, send, threadedFrame, toolActivity, publicationId); |
There was a problem hiding this comment.
When a sequenced non-identity frame arrives before the session has a topic or flat identity, this queues the frame with its publication ID and returns, leaving that publication claimed. #onRouterFrame() then waits for its settlement, while SessionRouter serializes later frames behind the current callback; consequently, the later identity frame needed by flushPendingThreadedFrames() can never run, and delivery for the session stalls indefinitely. Do not make settlement of the current Router callback depend on a later Router frame.
Useful? React with 👍 / 👎.
| } | ||
| }, | ||
| }) | ||
| .add({ | ||
| name: "ask-selected-ack", | ||
| matches: msg => msg.type === "ask_selected_ack_request", | ||
| handle: async (session, msg) => { | ||
| const publicationId = session.activePublicationId; | ||
| if (publicationId) this.deferredPublications.add(publicationId); |
There was a problem hiding this comment.
For an already-expired, route-missing, or malformed sequenced ask_selected_ack_request, this marks the publication deferred before the handler returns a terminal pre-send outcome. settleSelectedPublication() does not settle failed/expired or failed/route_missing, so #onRouterFrame() waits forever and blocks every later frame for the session. Fresh evidence since the earlier selected-ack settlement comment is that the new publication-settlement wait now has no completion path for these terminal failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dc91944bf
ℹ️ 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".
| private readonly sessionRouter: OperatorEventRouter<SessionSocket>; | ||
| /** Provider-facing frame dispatcher; endpoint authority remains in SessionRouter. */ | ||
| private readonly frameRouter: OperatorEventRouter<AttachmentSession>; | ||
| private readonly attachmentRouter: SessionRouter; |
There was a problem hiding this comment.
Declare this newly added field as readonly #attachmentRouter and update its references; the repository contract prohibits TypeScript private field modifiers and requires ES #private fields instead.
AGENTS.md reference: AGENTS.md:L118-L119
Useful? React with 👍 / 👎.
| }; | ||
| } | ||
| if (outcome.ok) { | ||
| if (!(await this.#adoptionIntents.bindSession(providerRequestKey, outcome.result.sessionId))) { |
There was a problem hiding this comment.
When the Broker successfully creates the session but this post-create sidecar write fails transiently, the function returns with an unbound reservation. The Telegram update was already persisted as seen before entering this method, so it cannot retry the same idempotent request, and later attachment handling uses bySession(sessionId), which cannot adopt the original topic without this binding; the started session is therefore surfaced outside the user's topic while that topic remains claimed until expiry. Retain a recoverable result that retries this CAS binding, or compensate the successful create before acknowledging the adoption.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18531ead01
ℹ️ 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".
| frame: Record<string, unknown>, | ||
| expectedGeneration?: number, | ||
| ): Promise<Record<string, unknown>> { | ||
| await this.#serialReconcile(); |
There was a problem hiding this comment.
If an in-flight provider command reaches request() while stop() is running—or any retained caller invokes it after shutdown—this unconditional reconciliation still runs even though stop() set #started to false. It can recreate an attachment after the shutdown loop has drained, and request() then dispatches through that fresh client, leaving a live socket and allowing commands or frames after provider teardown; reconciliation needs a shutdown epoch/state guard and must not attach until start() explicitly re-enables it.
Useful? React with 👍 / 👎.
Move the SDK lifecycle and attachment authority into the Broker while retaining exact cleanup fences. Preserve broker-owned teardown for an orphaned workspace and prove a replacement endpoint cannot be removed after capture. Lore-id: pr-4098 Constraint: preserve exact endpoint authority during lifecycle cleanup Confidence: high Scope-risk: wide Reversibility: revertable Tested: bun test packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts; bun test packages/coding-agent/test/sdk-session-lifecycle-authority.test.ts packages/coding-agent/test/sdk-session-router-authority.test.ts; bun --cwd=packages/coding-agent run check Not-tested: aggregate affected validation with ambient OPENAI_BASE_URL set, which fails unrelated packages/agent/test/remote-compaction.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b86a2760b6
ℹ️ 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 recent = await this.listRecent({ | ||
| cwd: requestedCwd ?? this.#agentDir, | ||
| allWorkspaces: requestedCwd === undefined, | ||
| limit: 1_000, |
There was a problem hiding this comment.
When a workspace has more than 1,000 verified non-internal histories, listRecentSessions() sorts and slices to this limit before resumeExternal() performs its ID/prefix filtering. An exact older session is therefore reported as not_found; worse, if a prefix matches one recent and one older session, the older match is hidden and the recent session is resumed instead of returning ambiguous. Resolve the requested ID/prefix over the complete candidate set rather than the truncated recent-session view.
Useful? React with 👍 / 👎.
Regenerate the catalog after integration and tolerate sub-millisecond filesystem timestamp representation drift when checking the same indexed endpoint. Lore-id: pr-4098-ci Constraint: keep endpoint authority identity-bound Confidence: medium Scope-risk: narrow Reversibility: revertable Tested: bun test packages/coding-agent/test/tools/tool-catalog.test.ts
Endpoint-only close authority can report a verified host flush failure without gaining signal authority. Adopted Slack roots now persist their matching attachment authority before later notifications select the root thread. Lore-id: pr-4098-ci Constraint: endpoint-only authority must never enable signal fallback Constraint: adopted roots must retain exactly-once publication authority Confidence: high Scope-risk: narrow Reversibility: revertable Tested: bun test packages/coding-agent/test/sdk-broker.test.ts packages/coding-agent/test/sdk-slack-thread-binding.test.ts Tested: bun --cwd=packages/coding-agent run check
Require endpoint metadata for endpoint-only close control while retaining durable incarnation proof for signal-path revalidation. Lore-id: pr-4098-ci Constraint: endpoint-only authority cannot authorize signalling Confidence: high Scope-risk: narrow Reversibility: revertable Tested: bun test packages/coding-agent/test/sdk-broker.test.ts packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts packages/coding-agent/test/sdk-slack-thread-binding.test.ts packages/coding-agent/test/sdk-session-lifecycle-authority.test.ts packages/coding-agent/test/sdk-session-router-authority.test.ts Tested: bun --cwd=packages/coding-agent run check
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f79442a55
ℹ️ 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".
| private readonly botApi: BotApi; | ||
| private readonly effects = new TelegramEffectSupervisor(); | ||
| private readonly effects = new TelegramEffectSupervisor(this.#deliveryAbort.signal); |
There was a problem hiding this comment.
Replace this new TypeScript private field with an ES #effects field and update its references. The repository contract explicitly prohibits private modifiers outside constructor parameter properties, so this production addition violates the required class convention.
AGENTS.md reference: AGENTS.md:L116-L116
Useful? React with 👍 / 👎.
|
Exact repair head: Resolved the reported close-arbitration regression without widening signal authority: an endpoint with verified metadata can surface its typed Current evidence:
— |
Refresh the protected Slack daemon declaration digest after its attachment-authority fix. Lore-id: pr-4098-ci Constraint: semantic guard manifest must match protected source Confidence: high Scope-risk: narrow Reversibility: regenerate Tested: bun scripts/telegram-daemon-generation-guard.ts --validate-current-tree
|
Exact CI guard attestation repair pushed: The Slack daemon authority change updated a protected declaration; its semantic manifest digest is now regenerated and validated with — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f79442a55
ℹ️ 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 attachment = | ||
| authority.attachmentAuthorityId === undefined ? await this.#resolveAttachment(sessionId) : undefined; | ||
| const attachmentAuthorityId = | ||
| authority.attachmentAuthorityId ?? | ||
| (attachment?.generation === authority.endpointGeneration ? attachment.authorityId : undefined); |
There was a problem hiding this comment.
When a same-generation attachment replacement occurs after this separate #resolveAttachment() call—particularly during the provider lookup—but before the store transaction commits, the revalidation at line 541 checks only endpointGeneration, so the stale attachmentAuthorityId is persisted even though the replacement has a different authority ID. The next notification fails the equality check in #notify() and retires the operator-selected root in favor of a replacement; revalidate the exact attachment authority that is being written.
Useful? React with 👍 / 👎.
| rootTs: input.rootTs, | ||
| sessionId: input.sessionId, | ||
| endpointGeneration: input.endpointGeneration, | ||
| ...(input.attachmentAuthorityId === undefined ? {} : { attachmentAuthorityId: input.attachmentAuthorityId }), |
There was a problem hiding this comment.
When an active adopted binding was persisted by the previous schema without attachmentAuthorityId, an exact bind-thread retry takes the storedGeneration === input.endpointGeneration branch and returns the old record before reaching this new field assignment. Fresh evidence beyond the prior report is that this head stamps only newly constructed records, so a successfully revalidated legacy binding still retains undefined; its next notification treats the root as stale and replaces it. Allow the exact-authority replay to backfill the missing authority.
Useful? React with 👍 / 👎.
3 participants
Summary
SessionLifecycleServicefacade over the canonical Broker lifecycle ledger with capability checks, stable idempotency, and credential-free resultsSessionRouterVerification
bun --cwd=packages/coding-agent run checkbun --cwd=packages/coding-agent run check:sdk-closure(576 adapter rows, 49 Telegram baseline commands, downgrade/canonicalization/rename gates)cargo test --manifest-path crates/gjc-sdk/Cargo.toml(146 passed)cargo check --manifest-path crates/pi-natives/Cargo.tomlReview gates
Known unrelated repository checks
bun run checkstill reaches pre-existingpi-nativesClippymissing_const_for_fnfindings in computer-controller code