Skip to content

[NA] [CI] fix: serve both LLM wire protocols in the E2E mock - #28

Merged
JetoPistola merged 2 commits into
mainfrom
danield/NA-e2e-dual-protocol-mock
Aug 4, 2026
Merged

[NA] [CI] fix: serve both LLM wire protocols in the E2E mock#28
JetoPistola merged 2 commits into
mainfrom
danield/NA-e2e-dual-protocol-mock

Conversation

@JetoPistola

@JetoPistola JetoPistola commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

User description

Details

image

The nightly E2E went red on 2026-08-01 with no change to this repo (last commit f701092, 2026-07-10). All three jobs failed together with trace has no output (finalize re-send did not land) and no tool spans.

Root cause — upstream drift, not a plugin regression. A rolling nousresearch/hermes-agent:latest rebuild began honoring the openai-api provider's declared codex_responses transport, switching the chat wire from POST /v1/chat/completions to POST /v1/responses. Our mock only spoke Chat Completions, so /v1/responses fell through to the catch-all {"ok": true}, the stream never emitted a terminal event, and every turn died after 3 retries having made 0 tool calls:

Messages: 1 (1 user, 0 tool calls)        # was: 4 (1 user, 2 tool calls)
"output": {"error": "Codex Responses stream did not emit a terminal response"}
"api_mode": "codex_responses"             # retry_count 0 → 1 → 2

Both reported assertion failures trace back to that single cause: no tool span (the call never happened) and no trace output (the turn never reached finalize).

Note the version tags are misleading here — :latest and v2026.7.30 are both version 0.19.1 but different digests, so a version bump was never going to signal this.

Fix — serve both wire protocols, keep tracking latest.

  • Added /v1/responses: typed SSE events (response.output_item.added.doneresponse.completed). Both added and done are required — Hermes sets has_tool_calls from the first and collects the executable item from the second (codex_runtime.py:1078,1116), and the absent terminal event is what raised at :1213.
  • Kept /v1/chat/completions byte-for-byte, so older Hermes images keep passing.
  • Replaced the catch-all 200 for unknown POST routes with a 501. The benign 200 let an unimplemented protocol masquerade as a hung stream — that disguise is what made this expensive to diagnose. The next protocol change now names itself in the first log line.

Deliberately not pinning HERMES_IMAGE. This cron exists to catch upstream drift (e2e.yml:5). Pinning would have turned the suite green while the incompatibility it was built to find sat undetected — a silenced detector, not a stable one.

Change checklist

  • User facing
  • Documentation updated (if needed)
  • Tests added/updated (if needed)
  • Breaking changes documented (if any)

Issues

  • NA — upstream image drift caught by the nightly E2E cron; no ticket filed.

Testing

Reproduced the failure locally against :latest, then verified the fix across all three images and all three CI jobs:

Image Wire Result
v2026.7.20 chat_completions PASSED
v2026.7.30 chat_completions PASSED
:latest (Aug 2 rebuild) codex_responses PASSED
  • run_e2e.sh (mock) — llm spans: 2 \| tool spans: 1 \| NA: 0
  • run_e2e_wheel.sh (pip entry point) — PASSED
  • run_e2e_real_opik.sh (full Opik stack) — spans=3 llm=2 tool=1 NA=0
  • 113 unit tests pass; ruff check clean
  • Curl-verified both protocols stream correctly and unknown routes return 501

Confirmed the fix restores real coverage rather than just satisfying assertions: on the codex_responses wire the trace records api_mode=codex_responses with correctly-named spans, incrementing LLM call 1/2, genuine tool output (1024), usage, and provider mapped to openai. The plugin was already correct on the new wire — the E2E just couldn't prove it. The OPIK-7280 pretty-mode output shape holds there too.

Documentation

N/A — no user-facing or API changes. The module docstring in e2e/mock_llm_server.py documents both protocols, why both are kept, and why unknown routes fail loudly.


Generated description

Below is a concise technical summary of the changes proposed in this PR:

graph LR
do_POST_("do_POST"):::modified
already_ran_tool_("_already_ran_tool"):::added
responses_tool_events_("_responses_tool_events"):::added
responses_final_events_("_responses_final_events"):::added
sse_typed_("_sse_typed"):::added
OPENAI_RESPONSES_API_("OPENAI_RESPONSES_API"):::added
OPENAI_CHAT_COMPLETIONS_API_("OPENAI_CHAT_COMPLETIONS_API"):::modified
do_POST_ -- "Decides tool stage via request body, not call counter." --> already_ran_tool_
do_POST_ -- "Emits function_call added/done and response.completed for tool turn." --> responses_tool_events_
do_POST_ -- "Streams assistant text delta, then response.completed after tool output." --> responses_final_events_
do_POST_ -- "Streams event-typed SSE frames ending at response.completed." --> sse_typed_
do_POST_ -- "Adds /v1/responses mock with typed SSE and two-step tool cycle." --> OPENAI_RESPONSES_API_
do_POST_ -- "Chat completions stage now derived from body, not global state." --> OPENAI_CHAT_COMPLETIONS_API_
classDef added stroke:#15AA7A
classDef removed stroke:#CD5270
classDef modified stroke:#EDAC4C
linkStyle default stroke:#CBD5E1,font-size:13px
Loading

Extend Handler in e2e/mock_llm_server.py to serve both Chat Completions and Responses traffic for the Hermes openai-api mock, so the nightly E2E keeps working across old and new upstream images. Update the mock server’s turn detection and SSE responses to stay deterministic and surface unsupported inference routes with a 501 instead of a misleading 200.

TopicDetails
LLM protocols Serve both LLM wire protocols used by the Hermes mock, including typed Responses SSE events and the existing Chat Completions stream.
Modified files (1)
  • e2e/mock_llm_server.py
Latest Contributors(1)
UserCommitDate
danield@comet.comfix(e2e): derive mock ...August 03, 2026
Routing errors Fail unknown inference routes loudly so protocol drift is reported immediately instead of looking like a hung stream.
Modified files (1)
  • e2e/mock_llm_server.py
Latest Contributors(1)
UserCommitDate
danield@comet.comfix(e2e): derive mock ...August 03, 2026
Review this PR on Baz | Customize your next review

The nightly E2E started failing on 2026-08-01 without any change to this
repo. A rolling nousresearch/hermes-agent:latest rebuild began honoring the
openai-api provider's declared codex_responses transport, switching the wire
from POST /v1/chat/completions to POST /v1/responses. The mock only spoke
Chat Completions, so the stream never emitted a terminal event and every turn
died after three retries with no tool call — surfacing as 'trace has no
output' and 'no tool spans' across all three E2E jobs.

Serve both protocols rather than pinning the image: the nightly cron exists to
catch exactly this upstream drift, so pinning would silence the detector. The
Chat Completions handler is unchanged, so older Hermes images keep passing.

Also replace the catch-all 200 for unknown POST routes with a 501. The benign
200 let an unimplemented protocol masquerade as a hung stream, which is what
made this break hard to diagnose.

Verified against v2026.7.20, v2026.7.30 (chat_completions) and :latest
(codex_responses): all three E2E jobs pass, 113 unit tests pass.
@JetoPistola
JetoPistola marked this pull request as ready for review August 2, 2026 17:41
Comment thread e2e/mock_llm_server.py
Review feedback: the global _STATE["calls"] counter was incremented
unlocked and before the response was emitted. Two failure modes follow —
ThreadingHTTPServer can serve requests concurrently, and an aborted or
retried call consumed the tool stage, leaving the retry to receive the
final message instead. Reproduced the latter: aborting call 1 mid-stream
made the retry return output_text rather than re-emitting function_call,
which would surface as a bare 'no tool spans' failure.

The conversation already carries the stage, so read it instead of
counting: turn 1 has only the user message, turn 2 carries the tool
result (function_call_output on the Responses wire, role: "tool" on
Chat Completions). That is idempotent — a retry resends the same body
and gets the same stage — so the mutable global goes away entirely
rather than being guarded by a lock.

Verified: the retry now re-emits function_call; both wires stage
correctly on turns 1 and 2; mock E2E passes on latest, v2026.7.30 and
v2026.7.20; wheel and real-Opik E2E pass; 113 unit tests pass.
@JetoPistola
JetoPistola merged commit 016b4a7 into main Aug 4, 2026
10 checks passed
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