fix(codex_engine): warn on non-numeric response ids; correct stream-limit comment (#6, #8)

#6: a JSON-RPC response with a non-numeric id was logged at debug level
and silently dropped, leaving the pending future to resolve only via its
60s timeout. Every id we send is one of our own ints (self._next_id), so
this only happens on non-conformant server behavior — worth a warning,
not routine tracing noise. Left the pending future alone rather than
guessing which request it belonged to (forcibly resolving "the" pending
future when exactly one is in flight risks failing an unrelated request
if the message turns out to be unrelated noise).

#8: the comment on the oversized-line ValueError handler in _read_stdout
claimed CPython's StreamReader keeps the buffered data around so
subsequent readline() calls "drain the oversized line in fragments".
That's not what CPython does: per StreamReader.readline()'s own
docstring, on overrun the buffer is either trimmed up to and including
the next separator it already contains (clean single-line loss) or
cleared entirely if no separator has arrived yet (only the latter case
can surface a later fragment, and it self-terminates — no draining loop
required). Corrected the comment to describe the actual behavior; no
functional change, the recovery loop was already correct.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: AdamMagued

codex_engine/app_server.py

@@ -344,11 +344,24 @@ async def _read_stdout(self) -> None:
                 try:
                     raw = await proc.stdout.readline()
                 except ValueError:
-                    # One line exceeded STREAM_LIMIT. The reader keeps the
-                    # buffered data, so subsequent readline() calls drain the
-                    # oversized line in fragments — each fails JSON parsing and
-                    # is skipped. Degrade gracefully instead of killing the
-                    # turn (C1).
+                    # One line exceeded STREAM_LIMIT. Per CPython's
+                    # StreamReader.readline() docs: "if newline was found,
+                    # complete line including newline will be removed from
+                    # internal buffer. Else, internal buffer will be
+                    # cleared." I.e. on overrun the buffer is NOT preserved
+                    # for us to drain in fragments — it is either trimmed up
+                    # to (and including) the next '\n' it already contains
+                    # (oversized line fully discarded, next readline() starts
+                    # clean on whatever follows), or wiped entirely if no
+                    # '\n' had arrived yet (the remaining tail of that same
+                    # line, still incoming from the child process, gets
+                    # re-accumulated from scratch and returned as a short
+                    # fragment on a later readline() — which fails JSON
+                    # parsing below and is skipped same as any other bad
+                    # line). Either way this ValueError handler doesn't need
+                    # to do any draining itself; `continue` and the normal
+                    # per-line JSONDecodeError handling take care of it.
+                    # Degrade gracefully instead of killing the turn (C1).
                     logger.warning(
                         "app-server emitted a line longer than %d bytes; skipping it",
                         STREAM_LIMIT,
@@ -392,7 +405,20 @@ def _dispatch(self, msg: dict) -> None:
             try:
                 key = int(raw_id)
             except (TypeError, ValueError):
-                logger.debug("app-server response with non-numeric id ignored: %r", raw_id)
+                # Every id we send is one of our own ints (self._next_id), so
+                # a non-numeric id here means the server echoed something we
+                # never sent — non-conformant behavior worth knowing about,
+                # not routine tracing noise (LOW-MEDIUM #6). Left unresolved
+                # rather than guessed at: forcibly resolving "the" pending
+                # future (even when exactly one is in flight) risks failing
+                # an unrelated in-flight request if this message turns out to
+                # be unrelated noise rather than a mangled response to it —
+                # the pending future still resolves via its own 60s timeout.
+                logger.warning(
+                    "app-server response with non-numeric id ignored (protocol "
+                    "mismatch): %r",
+                    raw_id,
+                )
                 return
             fut = self._pending.pop(key, None)
             if fut is None or fut.done():

tests/test_app_server_client.py

@@ -194,5 +194,30 @@ def test_check_schema_drift_gate(self):
         self.assertIsNone(msg)
 
 
+class DispatchNonNumericIdTest(unittest.TestCase):
+    """LOW-MEDIUM #6: a response with a non-numeric id is protocol-nonconformant
+    (every id we send is one of our own ints) and must be logged loudly enough
+    to notice, not buried at debug level."""
+
+    def test_non_numeric_response_id_logs_at_warning(self):
+        client = AppServerClient(_fake_argv())
+        with self.assertLogs("kim.app_server", level="WARNING") as cm:
+            client._dispatch({"jsonrpc": "2.0", "id": "not-a-number", "result": {}})
+        self.assertTrue(
+            any("non-numeric id" in line for line in cm.output),
+            cm.output,
+        )
+
+    def test_non_numeric_response_id_does_not_touch_pending(self):
+        # Must not guess which pending future this belongs to — an
+        # unrelated in-flight request should still resolve normally via its
+        # own matching response (or its own timeout), not be hijacked.
+        client = AppServerClient(_fake_argv())
+        client._pending[1] = "sentinel"  # type: ignore[assignment]
+        with self.assertLogs("kim.app_server", level="WARNING"):
+            client._dispatch({"jsonrpc": "2.0", "id": "abc", "result": {}})
+        self.assertEqual(client._pending[1], "sentinel")
+
+
 if __name__ == "__main__":
     unittest.main()