Skip to content

test(gateway): update turn-isolation parity and activate doubles for #270 - #296

Merged
OmarB97 merged 1 commit into
mainfrom
fix/tui-golden-and-inflight
Aug 2, 2026
Merged

test(gateway): update turn-isolation parity and activate doubles for #270#296
OmarB97 merged 1 commit into
mainfrom
fix/tui-golden-and-inflight

Conversation

@OmarB97

@OmarB97 OmarB97 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

What does this PR do?

Fixes the last two failing tests in tests/test_tui_gateway_server.py. Both are
stale test doubles, not product defects — I verified the production side in
each case rather than inferring it from the tests going green.

1. _session_info doubles took one argument (3 sites)

The real signature is _session_info(agent, session: dict | None = None) and the
gateway turn path calls it with both. Three stubs still took a single argument,
so the worker thread died:

[gateway-crash] thread Thread-4 (run) raised TypeError:
  <lambda>() takes 1 positional argument but 2 were given

The turn never started, which surfaced as the misleading assertion message
fake model did not stream before activation. Most _session_info stubs in
this file were already updated to lambda _a, *a2:; these three were missed.

Only one of the three was failing today. I fixed all three because they are the
same defect and the other two reach the same call path on any future change —
happy to narrow it to the failing one if you'd rather.

2. session.activate in-flight payload gained turn_id

#270 stamps the in-flight turn id so a client can correlate a partial with the
turn.outcome that closes it. The assertion was an exact dict equality that
predated it. It now pins the id's shape and compares the stable fields exactly
(the id is a fresh uuid per turn).

3. Golden-transcript parity vs the compute host

test_prompt_submit_golden_transcript_matches_flag_off_and_on asserts that
turn isolation ON and OFF produce the same transcript. #270 changed the
in-process (OFF) path to emit a leading session.info, a turn_id on
message.start / message.complete, and a terminal turn.outcome. The
_FakeSupervisor standing in for the compute host still emitted the old
four-event sequence.

I checked whether this was a real parity bug before touching the test.
_on_compute_host_turn_done() does not call _finalize_turn_outcome(), which
looks like turn isolation loses outcomes — but the compute-host child
(python -m tui_gateway.compute_host) proxies straight back into
server.handle_request(...), and the supervisor relays the child's rpc frames
verbatim to the client. So the child emits the same events the in-process path
does, and real parity holds. The double is what was stale. Worth noting all the
same: #270 added no compute-host / turn_isolation coverage of its own
tests/tui_gateway/test_turn_outcomes.py never exercises that path — so this
hand-maintained double is currently the only thing guarding that parity.

There is a second consequence: the transcript now carries a fresh uuid and
wall-clock started_at / completed_at, so two runs can never be byte-equal
again. The test now normalizes those four volatile fields and keeps asserting
the event sequence and every stable field.

A structurally better fix would be to have the fake supervisor delegate to the
same submit path instead of hand-listing events, so parity is guaranteed rather
than manually maintained. That is a bigger change than a red-test repair, so I
left it — say the word if you want it done that way instead.

Related Issue

No filed issue — surfaced once #286 unblocked uv sync --locked and the Python
test slices ran for the first time since 2026-07-10.

Type of Change

  • ✅ Tests (adding or improving test coverage)

Changes Made

  • tests/test_tui_gateway_server.py
    • _session_info stubs (3) now accept the optional session argument.
    • session.activate in-flight assertion pins turn_id's shape and compares stable fields exactly.
    • _FakeSupervisor emits the current six-event sequence, with a comment that it must track the in-process path.
    • New _normalize() blanks turn_id / id / started_at / completed_at before the parity comparison.

How to Test

pytest tests/test_tui_gateway_server.py -p no:randomly -q

On this branch: 383 passed, 1 failed — the remaining failure is
test_prompt_submit_preserves_empty_response_without_error, which is fixed
separately in #294 (kept out of this PR to keep the changes separable). With
both branches applied the file is fully green.

⚠️ Run with an isolated HERMES_HOME. The suite reads the developer's real
~/.hermes/config.yaml; a local fallback_policy: 'off' reds 5 further tests
in this file (TestResolveRuntimeWithFallback) that pass on CI, because
_load_effective_fallback_model() returns [] under policy off before the
tests' monkeypatched loader is consulted. Pre-existing test-isolation gap, not
touched here.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run the full affected file (see above)
  • I've added tests for my changes — N/A, this PR repairs existing tests
  • I've tested on my platform: macOS 15 (Darwin 25.6.0), Python 3.11.15

Documentation & Housekeeping

  • I've updated relevant documentation — N/A
  • I've updated cli-config.yaml.example — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md — N/A
  • I've considered cross-platform impact — N/A, test-only
  • I've updated tool descriptions/schemas — N/A

…270

Two test doubles in this file drifted behind the code they stand in for.

_session_info doubles (3 sites)
  The real signature is _session_info(agent, session=None), and the gateway
  turn path calls it with both. Three stubs still took a single argument, so
  the worker thread died with "lambda takes 1 positional argument but 2 were
  given" before the fake model ever streamed — surfacing as the misleading
  "fake model did not stream before activation". Most stubs in this file were
  already updated; these three were not. The two that were not yet failing
  reach the same call path on any future change.

session.activate in-flight payload
  #270 stamps the in-flight turn id so a client can correlate a partial with
  the turn.outcome that closes it. Pin its shape and compare the stable
  fields exactly, since it is a fresh uuid per turn.

golden transcript parity
  #270 added a leading session.info, a turn_id on message.start /
  message.complete, and a terminal turn.outcome to the in-process path. The
  _FakeSupervisor standing in for the compute host still emitted the old
  four-event sequence. The compute-host child proxies prompt.submit back into
  server.handle_request(), so it emits exactly what the in-process path
  emits; the double is what was stale, not the product.

  The transcript now also carries a fresh uuid and wall-clock timestamps, so
  the two runs can never be byte-equal again. Normalize those four volatile
  fields before comparing and keep asserting the sequence and every stable
  field.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
OmarB97 pushed a commit that referenced this pull request Aug 2, 2026
TestResolveRuntimeWithFallback patches `server._load_fallback_model`, but
`_resolve_runtime_with_fallback()` consults `_load_effective_fallback_model()`
— the policy-filtered chain added by #269. That wrapper only reaches the raw
loader when the ambient policy is not "off":

    def _load_effective_fallback_model():
        cfg = _load_cfg()
        policy = get_fallback_policy(cfg)
        if policy == "off":
            return []
        chain = _load_fallback_model() or []

CI has no hermes config, so the policy defaults to "any", the stub is reached,
and these tests are green there. On a host whose config sets
`fallback_policy: off`, the wrapper short-circuits to [], the stub is never
consulted, no fallback is ever attempted, and all five tests fail for a reason
that has nothing to do with what they assert.

Patch what the code actually calls, so the case under test is the one that runs
on any host. `test_auth_error_all_fallbacks_fail_raises` additionally pins
`_load_cfg`, because it asserts the policy-specific exhaustion wording ("no
usable configured backup route remained" is the "any" branch) and would
otherwise assert against whatever policy the host happens to have.

This is not a CI-visible failure — it is a latent host-config dependency that
makes the suite behave differently for a contributor than it does in CI.

TestResolveRuntimeWithFallback: 8 passed against a config with
`fallback_policy: off`, and 8 passed with no hermes config at all. Before this
change the first environment produced 5 failures.

This file's two remaining failures (golden transcript, session activate) are
owned by #296; this change is disjoint from it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@OmarB97
OmarB97 merged commit fbbf44c into main Aug 2, 2026
35 checks passed
OmarB97 added a commit that referenced this pull request Aug 2, 2026
…ig (#299)

TestResolveRuntimeWithFallback patches `server._load_fallback_model`, but
`_resolve_runtime_with_fallback()` consults `_load_effective_fallback_model()`
— the policy-filtered chain added by #269. That wrapper only reaches the raw
loader when the ambient policy is not "off":

    def _load_effective_fallback_model():
        cfg = _load_cfg()
        policy = get_fallback_policy(cfg)
        if policy == "off":
            return []
        chain = _load_fallback_model() or []

CI has no hermes config, so the policy defaults to "any", the stub is reached,
and these tests are green there. On a host whose config sets
`fallback_policy: off`, the wrapper short-circuits to [], the stub is never
consulted, no fallback is ever attempted, and all five tests fail for a reason
that has nothing to do with what they assert.

Patch what the code actually calls, so the case under test is the one that runs
on any host. `test_auth_error_all_fallbacks_fail_raises` additionally pins
`_load_cfg`, because it asserts the policy-specific exhaustion wording ("no
usable configured backup route remained" is the "any" branch) and would
otherwise assert against whatever policy the host happens to have.

This is not a CI-visible failure — it is a latent host-config dependency that
makes the suite behave differently for a contributor than it does in CI.

TestResolveRuntimeWithFallback: 8 passed against a config with
`fallback_policy: off`, and 8 passed with no hermes config at all. Before this
change the first environment produced 5 failures.

This file's two remaining failures (golden transcript, session activate) are
owned by #296; this change is disjoint from it.

Co-authored-by: Omar Baradei <omar@kostudios.io>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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