test(gateway): drop a wait that was never synchronising anything

[jackulau]

What does this PR do?

tests/gateway/test_goal_continuation_drain.py::test_runner_goal_hook_enqueues_into_the_key_the_adapter_drains
slept a fixed 50ms between calling the post-turn goal hook and asserting the
enqueue landed. This removes that wait. It does not claim to fix the CI
flake, and the rewrite below explains why, because an earlier revision of this
PR did claim exactly that and was wrong.

What this PR used to say, and why it was wrong

The original version replaced the sleep with a bounded poll, on the stated
theory that "the enqueue does not happen inline, the hook hands off to a task,
so the assertion is racing the event loop".

There is no such handoff. _post_turn_goal_continuation contains no
asyncio.create_task, no ensure_future and no call_soon; every step in it
is awaited, and it ends in a synchronous self._enqueue_fifo(...).
_defer_goal_status_notice_after_delivery, the one call in there that looks
deferred, either registers an adapter callback and returns or awaits
_deliver() directly, and what it delivers is a status notice rather than the
enqueue.

So I measured it instead of arguing about it. Removing the wait entirely
(no sleep, no poll) on current main:

15 runs: pass=15 fail=0

If the assertion were racing a spawned task, the zero-wait run is precisely the
one that would fail every time. The 50ms sleep was not too short. It was
measuring nothing.

Credit to @Enough1122, whose review nit ("an event or awaiting the task would be
strictly deterministic") is what sent me back to the code. The answer turned out
to be that there is no task to await, so no production test hook is needed
either.

What that means for the flake

The CI failure this test produces is:

AssertionError: continuation enqueued under a different key than the adapter drains:
pending keys=[] expected=agent:main:slack:channel:C1:1718600000.000100

If the enqueue is inline, pending keys=[] cannot be a lost race. It means the
hook took a path that never enqueued at all: one of the early returns (no
session id, mgr.is_active() false, should_continue false, no prompt), or the
except Exception wrapped around the enqueue, which swallows the error into a
logger.debug. A bounded poll cannot fix any of those. It would wait two
seconds and fail with the same message.

My current suspicion is goals-DB contention upstream of mgr.is_active(), since
the test writes a goal via GoalManager and then depends on reading it back,
but I have not proven that and am not claiming it here. Someone should still
open an investigation into the enqueue-path returns; I did not want the
observed CI failure marked resolved by a change that cannot resolve it.

Related Issue

None. This came out of a flake observed on unrelated PRs.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 🔒 Security fix
• 📝 Documentation update
• ✅ Tests (adding or improving test coverage)
• ♻️ Refactor (no behavior change)
• 🎯 New skill (bundled or hub)

Changes Made

• tests/gateway/test_goal_continuation_drain.py: delete the await asyncio.sleep(0.05)
  between the hook call and the assertion, replacing it with a comment that
  records why the hook is already synchronous, and why adding a sleep back would
  be the wrong response to the next failure here.

No production code is touched, and no other test changes.

How to Test

1. pytest tests/gateway/test_goal_continuation_drain.py -q — 2 passed.
2. Repeat the single test to show the wait was not load-bearing:
   for i in $(seq 1 15); do pytest tests/gateway/...::test_runner_goal_hook_enqueues_into_the_key_the_adapter_drains -q; done
   gives 15/15 pass with no wait present.
3. Read _post_turn_goal_continuation in gateway/run.py and grep it for
   create_task, ensure_future, call_soon: none are present, which is the
   static half of the same claim.
4. ruff check tests/gateway/test_goal_continuation_drain.py — clean.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: Windows 11 (Python 3.13)

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — or N/A
• I've updated cli-config.yaml.example if I added/changed config keys — or N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
• I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

$ # wait removed entirely, single test, 15 consecutive runs
15 runs: pass=15 fail=0

$ pytest tests/gateway/test_goal_continuation_drain.py -q
..                                                                       [100%]
2 passed in 1.66s

$ ruff check tests/gateway/test_goal_continuation_drain.py
All checks passed!

[Enough1122]

AI code review — automated review for reference; please use your judgment.

• tests: Replacing the fixed 50ms sleep with a bounded condition poll is the right fix per the repo's flake policy (timing-sensitive tests must not assume a quiet runner); the sibling test in the same file already uses this idiom.
• correctness: Contract preserved — if the key never appears, the loop exhausts its 2s bound and the original assertion/message still fires, so a real routing regression still fails loudly.
• nit: An event/await-the-task would be strictly deterministic, but adding a production test hook for it is a fair tradeoff to reject here; the bounded poll matches the file's established pattern.

No blocking issues found.

[jackulau]

Thanks for the review. Taking the nit seriously sent me back to the code, and it
turned up something worse than the nit: the rationale in this PR's description
is wrong, and I no longer think this change fixes the flake it claims to.
Writing that up rather than quietly leaving it.

The premise does not hold

The description says the enqueue "does not happen inline — the hook hands off to
a task, so the assertion is racing the event loop". I cannot find that handoff.
_post_turn_goal_continuation has no asyncio.create_task, no
ensure_future, no call_soon; every step in it is awaited, and the enqueue is
a plain synchronous self._enqueue_fifo(...). Even
_defer_goal_status_notice_after_delivery, the one place that looks deferred,
either registers a callback and returns or awaits _deliver() directly, and it
sends a status notice rather than doing the enqueue.

So I checked it instead of reasoning about it. Removing the wait entirely
(no sleep, no poll) on current main:

1 passed  1 passed  1 passed  1 passed  1 passed  1 passed  1 passed  1 passed

8 for 8. If the assertion were racing a spawned task, a zero-wait run would be
the one that fails every time. The enqueue is inline, which means the original
50ms sleep was not too short, it was unnecessary.

Which makes the flake something else

If the enqueue is inline, then a CI run that reports pending keys=[] did not
lose a race, it took a path that never enqueued at all. In that hook the
candidates are the early returns (no session id, mgr.is_active() false,
should_continue false, no prompt) and the except Exception around the
enqueue, which swallows the error into a logger.debug. A loaded runner makes
several of those more likely, and my current suspicion is goals-DB contention
upstream of mgr.is_active(), since this test writes a goal with GoalManager
and then depends on reading it back.

A bounded poll cannot fix any of those. It would wait 2 seconds and then fail
with the same message, so the honest read is that this PR makes the failure
slower, not rarer.

What I would like to do about it

Whichever the maintainers prefer:

1. Reduce it to deleting the sleep. That is the accurate version of a
   timing-cleanup PR here: it removes the timing assumption outright instead of
   replacing it with a longer one, it is proven above, and it is honestly
   described as tidying rather than as a flake fix.
2. Close this and open a real investigation into the enqueue-path returns,
   which is where the flake actually lives.

I am happy to do either, and I would lean towards (1) plus a separate issue for
(2) so the observed CI failure does not get marked resolved by a change that
cannot resolve it. Say which and I will push it.

On your actual nit, for completeness: you were right that an event or awaiting
the task would be strictly deterministic, and right that a production test hook
would be too high a price. It turns out no hook is needed, because there is no
task to await. Your instinct that the wait was doing something suspicious was
better than my explanation of it.

[jackulau]

Went with option (1) and pushed it, since leaving the PR standing on a premise I
had just disproved seemed worse than picking one of the two myself. It is a
test-only change and trivially reversible if you would rather have (2).

What is on the branch now:

• The wait is gone, not lengthened. 15 consecutive runs of the single test
  with no sleep and no poll: 15/15 pass.
• The title and description no longer claim to fix the flake. They say what is
  actually established: the hook is already synchronous, so the sleep was
  measuring nothing, and pending keys=[] cannot be a lost race.
• The comment left in its place names the real suspects (the early returns and
  the swallowed except on the enqueue path) so that the next person to see this
  fail does not re-add a sleep, which is the obvious wrong move and the one I
  made.

Still worth someone opening an investigation into those enqueue-path returns.
That is the actual bug and this PR does not touch it.