Skip to content

fix(native-chat): restore main typecheck after #24312 and #24334 crossed - #24437

Merged
Jinwoo-H merged 1 commit into
mainfrom
jinwoo/fix-main-native-chat-types
Oct 1, 2026
Merged

Jinwoo-H merged 1 commit into
mainfrom
jinwoo/fix-main-native-chat-types

Conversation

@Jinwoo-H

@Jinwoo-H Jinwoo-H commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​18 $\color{#cf222e}{\Huge{\mathbf{−}}}$​7 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​11
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​6 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​5

ELI5

Two native-chat changes merged close together and each passed on its own, but together main stopped typechecking. This updates the newer one to use the other's reporting mechanism.

What Changed

#24312 replaced the optional onEventSinkError callback on the structured chat host with a required diagnostics logger. #24334, merged right after, still reported a failed "end the Codex process on Stop" through onEventSinkError, and its new tests built hosts without a logger.

  • structured-agent-session-chat-stop.ts: the failed child end is now written through context.deps.logger.warn (scope stop-child, with the session id and error), so it lands in the same diagnostics log as every other host failure.
  • structured-agent-session-conversation-stop.test.ts: asserts a stop-child entry for the session through a recording logger instead of the removed callback.
  • structured-agent-session-repeat-press.test.ts, structured-agent-session-repeated-stop.test.ts: pass the now-required logger.

No user-visible behavior changes: Stop handling is untouched, only where its failure is reported.

Why

Smallest change that makes main typecheck while keeping #24334's failure reporting, routed through #24312's single required logger.

Linked Issue

N/A (main build fix, maintainer)

Visual Proof

N/A: typecheck fix, no UI change.

Testing

  • pnpm tc passes.

  • pnpm test src/main/native-chat/agent-session-wire: 152 files, 1284 tests pass.

  • oxlint on touched files and pnpm run check:code-quality:changed pass.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

Agent skill upstream boundary

  • Not applicable

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • N/A for visual proof with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote impact considered (N/A)

#24334 reported it through onEventSinkError, which #24312 replaced with the
required diagnostics logger; main did not typecheck after both merged.
@Jinwoo-H
Jinwoo-H merged commit 9bcdb6a into main Oct 1, 2026
2 of 3 checks passed
@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Fixes type errors in chat session stop handling.

The PR appears safe to merge, though the Stop-child diagnostic tests should verify the error they are intended to preserve.

Findings

  1. P2 Stop error goes unchecked ▶

Summary

The PR routes failed Stop-child termination reports through the required host diagnostics logger and updates three test files to supply or inspect that logger.

  • The production reporting path matches the logger contract.
  • The updated failure-reporting assertions do not verify that the caught error is retained.

Reviews (1) · Last reviewed commit: "fix(native-chat): report a failed Stop c..."

Comment on lines +290 to +294
expect(log.entries).toContainEqual(
expect.objectContaining({
fields: expect.objectContaining({ scope: 'stop-child', sessionId: SESSION })
})
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Stop error goes unchecked The two new assertions check only the log scope and session ID. They would still pass if the Stop-child warning stopped including the caught error, leaving the failure detail this change is meant to preserve untested. The recording logger retains the warning level and error field; please assert those for both cases, especially the case that throws route release failed.

@pullfrog pullfrog Bot mentioned this pull request Oct 1, 2026
7 of 9 tasks
brennanb2025 added a commit that referenced this pull request Oct 1, 2026
…rt-failure-on-message

Brings in main's native-chat Stop work (#24333 owed stop retried by the next
message, #24072 queue pause derived from Stop/Resume rows, #24334, #24437) and
#24312 (structured chat failures go through the host logger).

Conflicts, each keeping main's behaviour and this branch's design:
- queued-message-pause-table.ts: deleted, as #24072 did. Its "a returned card
  holds the queue only when it was not a failed start" rule (queuedCardHoldsQueue)
  is ported into main's queued-message-pause.ts (nextSendableQueuedCard and
  resumableQueuePause); the drain's pick in queued-messages.ts takes main's
  pause-aware nextSendableQueuedCard. New test pins it.
- delivery-loop: main's wind-down wait (#24333) goes into this branch's prepare
  step: a held owed stop records its waiting row and stops; a start refused
  because the previous exit is unverifiable waits the same way instead of being
  recorded as a failed start. onError becomes main's logger, including the
  deps type (now in delivery-loop-deps.ts). The retry-wake booking moved to
  structured-agent-session-start-retry-wakes.ts to stay under max-lines.
- dead-generation settlement: main's { ok, error } result with this branch's
  unprovenStart (the delivery loop writes a failed start); unexpected-exit and
  host-lifetime pass unprovenStart and log through main's logger.
- agent-start, attach-flow: main's logger calls; this branch's startPending
  preparation and at-rest create.
- conversation-lifetime, provider-child, host, host-delivery: main's
  finishOwedWindDown and endedAt with this branch's comments and
  startFailureRecorded; conversation delivery now takes the attach context
  (one line in host.ts, which was over max-lines).
- restart-continuation: both imports; the handed-over/verdict reading moved to
  structured-agent-session-continuation-verdict.ts for max-lines.
- tests: main's logger added to this branch's hosts and attach inputs; the
  provider-start log expectation uses main's logger line.
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.

1 participant