test: add webhook conformance smoke coverage#283
Conversation
|
Warning Review limit reached
More reviews will be available in 30 minutes and 37 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughTwo test scripts gain outbound webhook coverage. ChangesOutbound Webhook Testing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces outbound webhook testing to both the conformance and end-to-end test suites. It adds new webhook-related scopes, helper functions, and a comprehensive suite of webhook lifecycle tests in scripts/conformance.ts. Additionally, scripts/e2e.ts now includes a local webhook receiver server to capture and verify signed webhook deliveries, along with support for a public tunnel URL via --webhook-receiver-url. The review feedback highlights a few issues: the -h flag is currently ignored because the flags parser only filters arguments starting with --, active keep-alive connections on the test server could cause server.close() to hang, and a potential TypeError exists if resp.data is null when checking events.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const flags = new Set(process.argv.slice(2).filter((a) => a.startsWith('--'))); | ||
| const CI = flags.has('--ci') || !!process.env.CI; | ||
| const REMOTE = flags.has('--remote'); | ||
| const HELP = flags.has('--help') || flags.has('-h'); |
There was a problem hiding this comment.
The flags set is constructed by filtering arguments that start with --. Consequently, any single-hyphen flag like -h is excluded from the set, making flags.has('-h') always evaluate to false. To fix this, check process.argv directly for the -h flag.
| const HELP = flags.has('--help') || flags.has('-h'); | |
| const HELP = flags.has('--help') || process.argv.slice(2).includes('-h'); |
| close: () => new Promise<void>((resolve, reject) => { | ||
| server.close((err) => (err ? reject(err) : resolve())); | ||
| }), |
There was a problem hiding this comment.
When shutting down the test server, active keep-alive connections (e.g., from a public tunnel or client) can prevent server.close() from resolving quickly, potentially causing the test suite to hang or timeout. Calling server.closeAllConnections() before closing the server ensures all active connections are immediately terminated.
close: () => new Promise<void>((resolve, reject) => {
server.closeAllConnections?.();
server.close((err) => (err ? reject(err) : resolve()));
}),| while (Date.now() < deadline) { | ||
| const resp = await api('GET', `/v1/workspaces/${WORKSPACE}/fs/events?limit=100`); | ||
| assert(resp.status === 200, `Events feed failed: ${resp.status}: ${JSON.stringify(resp.data)}`); | ||
| const events = Array.isArray(resp.data.events) ? resp.data.events : []; |
There was a problem hiding this comment.
If the API response body is null (which is valid JSON), accessing resp.data.events will throw a TypeError. Using optional chaining (resp.data?.events) ensures the check is safe and robust against null responses.
| const events = Array.isArray(resp.data.events) ? resp.data.events : []; | |
| const events = Array.isArray(resp.data?.events) ? resp.data.events : []; |
![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: 7b335b3424
ℹ️ 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".
| // ── 8. Export ─────────────────────────────────────────────────────── | ||
| // ── 8. Outbound webhooks ─────────────────────────────────────────── | ||
|
|
||
| await webhooksSection(); |
There was a problem hiding this comment.
When scripts/conformance.ts is run locally or with --remote, this unconditional section now POSTs to /v1/workspaces/{id}/webhooks, but the server only routes /v1/workspaces/{id}/webhooks/ingest and the OpenAPI contract likewise only documents the ingest path (internal/httpapi/server.go:228, openapi/relayfile-v1.openapi.yaml:1577; repo-wide rg finds no subscription/DLQ routes). As a result the suite fails with 404 before export/writeback/WebSocket coverage, unless those outbound webhook endpoints are implemented or this section is feature-gated.
Useful? React with 👍 / 👎.
Findings
Addressed comments
VerificationNo files were edited. Checks run:
Advisory NotesThe lockfile mismatch is outside this PR’s changed files, but it currently prevents the Node CI jobs from starting in this checkout. I left it unchanged because it is unrelated to the webhook conformance PR purpose. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/e2e.ts (1)
337-338:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd webhook scopes to the e2e token before registering subscriptions.
The new smoke test uses
TOKENfor webhookPOST/DELETE, but this token only has file/sync/ops scopes. An auth-enforcing server should reject Lines 735 and 784 before any delivery assertions run.Proposed fix
- TOKEN = rs256Auth.generateToken(WORKSPACE, 'e2e', ['fs:read', 'fs:write', 'sync:read', 'ops:read'], 3600); + TOKEN = rs256Auth.generateToken(WORKSPACE, 'e2e', [ + 'fs:read', + 'fs:write', + 'sync:read', + 'ops:read', + 'webhooks:read', + 'webhooks:write', + ], 3600);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e.ts` around lines 337 - 338, The e2e TOKEN generated on line 338 via rs256Auth.generateToken() is missing webhook scopes and only includes file/sync/ops scopes, but the token is used for webhook POST/DELETE operations later in the test. Add webhook-related scopes (such as 'webhook:read' and 'webhook:write') to the scopes array passed to the generateToken method call for rs256Auth to ensure the token has the necessary permissions for webhook subscription operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/conformance.ts`:
- Around line 720-723: The DLQ endpoint test in the test function labeled 'DLQ
endpoint' is using the incorrect endpoint path and response structure. Update
the api call to use the correct endpoint path from the OpenAPI contract: change
from ${ws()}/webhooks/dlq to ${ws()}/sync/dead-letter. Then update the assertion
that checks for r.data?.items to instead validate the response according to the
DeadLetterFeedResponse format defined in the OpenAPI contract, ensuring the
conformance test matches the actual API specification.
- Around line 26-29: The filter condition in the flags Set initialization only
accepts arguments starting with '--', which excludes the '-h' short form flag.
When the HELP constant checks for flags.has('-h'), it will never find it because
'-h' was filtered out. Modify the filter function used when creating the flags
Set to accept both arguments starting with '--' and also the '-h' argument, so
that the HELP constant properly detects both '--help' and '-h' command line
arguments.
In `@scripts/e2e.ts`:
- Around line 185-199: The local receiver binds to an ephemeral port (port 0 in
server.listen) but when WEBHOOK_RECEIVER_URL environment variable is set, the
subscription uses that external URL instead while tests still wait on the local
receiver.requests array. To fix this misalignment, either configure the
server.listen call to bind to a specific port that matches a configured port in
WEBHOOK_RECEIVER_URL (instead of using port 0), or implement a public receiver
capture/polling API endpoint that the test can query to retrieve webhook
requests instead of relying on the local receiver.requests array. This issue
applies at multiple locations in the file where webhook receiver setup and
request polling occur, so ensure any fix is consistently applied across all
affected sites.
- Around line 730-744: The pathGlob and filePath variables both call Date.now()
separately, which can result in different millisecond values and cause the glob
pattern to fail matching the actual file path, leading to a timeout. Capture
Date.now() once into a single timestamp variable and use that same variable in
both the pathGlob construction (the glob pattern for the webhook receiver) and
the filePath construction (the delivery file path) to ensure they match
correctly.
---
Outside diff comments:
In `@scripts/e2e.ts`:
- Around line 337-338: The e2e TOKEN generated on line 338 via
rs256Auth.generateToken() is missing webhook scopes and only includes
file/sync/ops scopes, but the token is used for webhook POST/DELETE operations
later in the test. Add webhook-related scopes (such as 'webhook:read' and
'webhook:write') to the scopes array passed to the generateToken method call for
rs256Auth to ensure the token has the necessary permissions for webhook
subscription operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 581ad6d8-a71c-4730-9835-0264286736df
📒 Files selected for processing (2)
scripts/conformance.tsscripts/e2e.ts
|
Findings
Changed Applied one mechanical fix: Addressed comments
Validation Passed:
Blocked:
I am not printing |
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
1 participant
Summary
Validation
Notes