Repository navigation
Conversation
handleWebhook called fetchWithRetries with WEBHOOK_TIMEOUT and then read response.json() directly. fetchWithTimeout only guards the wait for response headers: its timer is cleared once fetch resolves, so a webhook server that sends headers and then stalls the body hung until the transport's body timeout (~300s), ignoring WEBHOOK_TIMEOUT. Read the body through readWithResponseDeadline, the same mechanism added for buffered responses in promptfoo#12014, so the whole webhook response is covered by the configured timeout. Adds a regression test with a local server that sends headers and a partial body, plus a completed-body control case.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12087 +/- ##
=======================================
Coverage 89.74% 89.74%
=======================================
Files 1051 1051
Lines 92867 92867
Branches 32914 32914
=======================================
+ Hits 83343 83345 +2
+ Misses 9524 9522 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9767fb7a74
ℹ️ 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".
| expect(elapsed).toBeGreaterThanOrEqual(900); | ||
| expect(elapsed).toBeLessThan(4000); |
There was a problem hiding this comment.
On a loaded CI worker, the real timeout callback can run after the hard four-second bound, making this test fail even when the stalled body is correctly aborted. The test also waits through a real one-second timeout on every run. test/AGENTS.md explicitly requires fake timers for timing-sensitive tests; use a controllable response stream and advance the clock, following the existing pattern in test/cache.retry-cancellation.test.ts.
AGENTS.md reference: test/AGENTS.md:L320-L320
Useful? React with 👍 / 👎.
| // The webhook timeout covers the whole response, including the body. Without | ||
| // this, a server that sends headers and then stalls the body hangs until the | ||
| // transport's own body timeout. | ||
| const jsonResponse = await readWithResponseDeadline(response, () => response.json()); |
There was a problem hiding this comment.
A webhook that sends headers promptly but finishes its JSON body after the default five-second budget now fails the assertion, but the webhook documentation remains unchanged and does not explain WEBHOOK_TIMEOUT or its whole-response scope. src/assertions/AGENTS.md requires assertion behavior changes to update the expected-output documentation and its index. Document the default, override, and response-body deadline so users can configure slower webhook endpoints correctly.
AGENTS.md reference: src/assertions/AGENTS.md:L28-L29
Useful? React with 👍 / 👎.
|
|
||
| async function startServer(handler: Parameters<typeof createServer>[1]): Promise<string> { | ||
| const server = createServer(handler); | ||
| await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve)); |
There was a problem hiding this comment.
When a developer or CI runner has an external HTTP_PROXY or ALL_PROXY configured without a NO_PROXY exemption for 127.0.0.1, these tests send their requests through that proxy instead of reaching the local fixture. fetchWithProxy selects the proxy dispatcher in src/util/fetch/index.ts:308-317, and test/AGENTS.md confirms that shared setup preserves proxy settings. Clear and restore PROXY_ENV_KEYS around both tests, as test/fetch.compression.test.ts already does, or mock the transport.
AGENTS.md reference: test/AGENTS.md:L95-L96
Useful? React with 👍 / 👎.
…rrel The direct module import widened the core -> node cross-layer edge count (core -> node: 98 vs the 97 baseline), failing Check architecture boundaries in CI. Re-export readWithResponseDeadline from src/util/fetch/index.ts (already imported there for the rate-limit peek) and import it through that barrel, so the dependency graph is unchanged.
Drive the stalled-body case with a mocked transport and fake timers, keep loopback requests off ambient proxy settings, and document the WEBHOOK_TIMEOUT whole-response deadline.
|
Addressed the three review points in cdd22c5:
Verified locally: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdd22c589d
ℹ️ 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".
| import { getEffectiveRequestSignal } from './requestSignal'; | ||
| import { readWithResponseDeadline, trackResponseDeadline } from './responseDeadline'; | ||
|
|
||
| export { readWithResponseDeadline }; |
There was a problem hiding this comment.
This forwarding export conflicts with the root AGENTS.md guidance: “Avoid re-exporting from files; import directly from the source module.” It adds a second import surface for an internal helper solely to route the webhook dependency through the fetch barrel. Remove this export and import readWithResponseDeadline directly from ../util/fetch/responseDeadline in webhook.ts, following the existing import in src/cache.ts. If that direct dependency changes the architecture edge count, review the baseline adjustment explicitly rather than introducing another re-export.
AGENTS.md reference: AGENTS.md:L314-L314
Useful? React with 👍 / 👎.
|
On this one: the direct import is what I tried first. It adds a new cross-layer edge to the architecture graph (98 vs the baseline 97) and fails the repo's I could instead keep the direct import and raise the architecture baseline for that edge, but that changes a repo-wide guard for one internal helper. The barrel re-export seemed the smaller intervention. Happy to change it if maintainers prefer updating the baseline or moving the helper itself. |
Problem
handleWebhookusesfetchWithRetries(..., WEBHOOK_TIMEOUT)and then reads the response body directly.fetchWithTimeoutonly guards the wait for response headers — its abort timer is cleared oncefetchresolves — so a webhook server that sends headers and then stalls the body hung the assertion until the transport's body timeout (~300s), ignoringWEBHOOK_TIMEOUTentirely.#12014 fixed this class for buffered cache reads by adding
readWithResponseDeadline; the webhook assertion was not covered.Change
Read the webhook body through
readWithResponseDeadline(response, () => response.json()), the same helper used infetchWithCache, so the configured deadline covers headers + body. Untracked responses (mocks) fall back to a plain read, so existing consumers keep their behavior.Tests
test/assertions/webhook.test.ts: a loopback server that sends headers and a partial body (stalled) — with the fix the assertion fails at the deadline withWebhook error: Request timed out after 1000 msinstead of hanging; a completed-body control returnspass: true.test/assertions/webhook.test.ts+test/assertions/runAssertion.test.ts: 247 passed.tsc --noEmitand Biome clean on the changed files.Notes
Other sites with the same header-only deadline pattern (the n8n provider, several OpenAI providers, redteam indirect-web-pwn, etc.) are intentionally not changed here — some are streaming callers that own their response lifetime; this PR stays on the webhook assertion.