Tests: align pnpm test expectations with main#67001
Conversation
Greptile SummaryThis PR aligns test expectations with current The non-test changes are confined to Confidence Score: 5/5Safe to merge; all remaining finding is a P2 robustness suggestion in test infrastructure. All changes are either straightforward test expectation alignments or confined to qa-channel/qa-lab test infrastructure. The one substantive code change (bus-client.ts http rewrite) is correctly structured except for an unguarded JSON.parse in an event callback — a P2 that would produce an unhandled exception rather than a clean rejection on malformed responses. No production logic is affected. extensions/qa-channel/src/bus-client.ts — the Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/qa-channel/src/bus-client.ts
Line: 70-80
Comment:
**JSON.parse can silently hang the promise**
`JSON.parse(text)` on line 72 is called inside a Node.js `EventEmitter` callback (`response.on("end", ...)`), not inside the `Promise` executor. If the bus server ever returns a non-JSON body (e.g. on an unexpected error), the `SyntaxError` propagates as an uncaught exception rather than calling `reject`, leaving the promise unsettled until the test's own timeout fires.
```suggestion
response.on("end", () => {
const text = Buffer.concat(chunks).toString("utf8");
let parsed: T | { error?: string };
try {
parsed = text ? (JSON.parse(text) as T | { error?: string }) : ({} as T);
} catch (err) {
reject(err instanceof Error ? err : new Error(String(err)));
return;
}
if ((response.statusCode ?? 500) < 200 || (response.statusCode ?? 500) >= 300) {
const error =
typeof parsed === "object" && parsed && "error" in parsed ? parsed.error : undefined;
reject(new Error(error || `qa-bus request failed: ${response.statusCode ?? 500}`));
return;
}
resolve(parsed as T);
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Tests: align pnpm test expectations with..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bddaad42d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e00f912d14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
896d81b to
b1ae3bf
Compare
b77dc41 to
29c8068
Compare
|
Merged via squash.
Thanks @hxy91819! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29c8068053
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const text = Buffer.concat(chunks).toString("utf8"); | ||
| let parsed: T | { error?: string }; | ||
| try { | ||
| parsed = text ? (JSON.parse(text) as T | { error?: string }) : ({} as T); |
There was a problem hiding this comment.
Treat empty QA bus responses as parse errors
postJson currently parses "" as {} (text ? JSON.parse(text) : ({} as T)), so a 200 response with an empty body is treated as success. In that scenario, callers like pollQaBus can receive missing fields (cursor/events), which can then replay old events or fail later in unrelated code instead of surfacing a clear transport/parse failure at the boundary. This is a regression from the previous response.json() behavior, which rejected empty JSON payloads.
Useful? React with 👍 / 👎.
| kind: "event-kind", | ||
| eventKind: "inbound-message", | ||
| timeoutMs, | ||
| await params.state.waitForCursorAdvance(input.cursor ?? 0, timeoutMs, (snapshot) => { |
There was a problem hiding this comment.
Clamp long-poll wait cursor when client cursor is ahead
The long-poll waiter uses input.cursor ?? 0 directly, but poll() normalizes an ahead-of-server cursor back to 0 (effectiveStartCursor) to recover from desync/restart. If a client sends a stale high cursor and the bus is currently empty, new events will not satisfy waitForCursorAdvance until timeout because the server cursor cannot exceed that stale value yet, so /v1/poll can wait the full timeout even when matching events arrive. The waiter should use the same effective cursor basis as poll().
Useful? React with 👍 / 👎.
Summary
Testing