fix: query-loop review — update comment + external abort test

[dimakis]

Summary

• Updated finalSession comment in query-loop.ts to document both read sites after registry.remove()
• Added test for external abort-controller cancellation path (fallback usage records completed reason)

Rescued from stale local main — was committed directly instead of via branch+PR.

Test plan

• npm test passes

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 2 issue(s) (1 warning).

server/__tests__/query-loop.test.ts

Comment update is fine. New test has a misleading title — the abort path enters the catch block (reason='error', not 'completed'), and the test doesn't assert on the reason field to catch the discrepancy.

• 🟡 bugs (L1617): Test title says 'records fallback usage with completed reason on external abort' but the hangingStream rejects with an Error on abort, which enters the catch block (setting caughtError=true), so the actual reason recorded at query-loop.ts:1404 is 'error', not 'completed'. The test doesn't assert on the reason field, so it passes despite the misleading name. Either the title should say 'error reason' or the test should assert sessionMeta!.reason to verify the intended behavior. [fixable]
• 🔵 missing_tests (L1651): The test omits an assertion on the session's reason field (the existing sibling test at line 1610 also omits it). Since the test title specifically claims 'completed reason', adding expect(sessionMeta!.reason).toBe(...) would make the test actually verify what it claims to test. [fixable]

[dimakis]

🟡 bugs: Test title says 'records fallback usage with completed reason on external abort' but the hangingStream rejects with an Error on abort, which enters the catch block (setting caughtError=true), so the actual reason recorded at query-loop.ts:1404 is 'error', not 'completed'. The test doesn't assert on the reason field, so it passes despite the misleading name. Either the title should say 'error reason' or the test should assert sessionMeta!.reason to verify the intended behavior. [fixable]

[dimakis]

🔵 missing_tests: The test omits an assertion on the session's reason field (the existing sibling test at line 1610 also omits it). Since the test title specifically claims 'completed reason', adding expect(sessionMeta!.reason).toBe(...) would make the test actually verify what it claims to test. [fixable]

[dimakis]

Centaur Review

Found 1 issue(s).

server/__tests__/query-loop.test.ts

Clean PR — the new test covers the external-abort path well, and the comment update accurately describes the two post-remove readers. One minor title/assertion alignment nit.

• 🔵 missing_tests (L1617): Test title says 'with error reason' but the reason field is never asserted. The reason passed to setSessionState is only logged, not persisted on SessionMeta, so it can't be checked via getSession(). Consider either removing 'error reason' from the title (e.g., 'records fallback usage on external abort') or verifying the reason indirectly (e.g., spy on the store's setSessionState to confirm it receives reason: 'error'). [fixable]

[dimakis]

🔵 missing_tests: Test title says 'with error reason' but the reason field is never asserted. The reason passed to setSessionState is only logged, not persisted on SessionMeta, so it can't be checked via getSession(). Consider either removing 'error reason' from the title (e.g., 'records fallback usage on external abort') or verifying the reason indirectly (e.g., spy on the store's setSessionState to confirm it receives reason: 'error'). [fixable]

[dimakis]

Centaur Review

LGTM — no issues found.