Skip to content

CS-10217 trigger command on webhook event#4098

Draft
richardhjtan wants to merge 11 commits intomainfrom
CS-10217-trigger-command-on-webhook-event-2
Draft

CS-10217 trigger command on webhook event#4098
richardhjtan wants to merge 11 commits intomainfrom
CS-10217-trigger-command-on-webhook-event-2

Conversation

@richardhjtan
Copy link
Contributor

@richardhjtan richardhjtan commented Mar 2, 2026

What is changing

  • Triggers a command when a webhook event is received, enabling automated workflows (e.g., creating a GithubEventCard for every GitHub PR/review/check event).
  • Introduce github event card def, this can be use as data source for submission card to query the events of a PR
  • Introduce process github event command to create github event card
  • Cover tests

Async index path (runtime-common, host)

  • realm.ts — Supports a new X-Boxel-Async-Index: true request header on card POST. When set, the realm writes the file to disk and persists file meta, then returns 201 with the assigned card ID immediately. Indexing runs fire-and-forget in the background. This prevents a deadlock where the run-command worker job can't pick up the incremental-index job it enqueues.
  • store.ts — Uses the async-index path when creating new cards inside the headless Chrome render context (or when asyncIndex option is explicitly set). Calls setId() on the card instance with the server-assigned ID so downstream commands can reference it.

Webhook command execution (realm-server)

  • handle-webhook-receiver.ts — After verifying the webhook signature, fetches matching webhook_commands rows from the DB, filters them against the incoming payload using pluggable filter handlers, and enqueues run-command jobs for each match. Falls back to the webhook-registering user if no realm owner is found.
  • webhook-filter-handlers.ts (new) — Introduces a WebhookFilterHandler interface with a registry pattern. Ships two handlers:
    github-event — matches by X-GitHub-Event header; assembles { eventType, submissionRealmUrl, payload } as command input.
    default — pass-through; always matches and forwards the raw payload.

@github-actions
Copy link

github-actions bot commented Mar 2, 2026

Preview deployments

@github-actions
Copy link

github-actions bot commented Mar 2, 2026

Host Test Results

    1 files  ±0      1 suites  ±0   1h 48m 0s ⏱️ - 6m 16s
1 921 tests ±0  1 906 ✅ ±0  15 💤 ±0  0 ❌ ±0 
1 936 runs  ±0  1 921 ✅ ±0  15 💤 ±0  0 ❌ ±0 

Results for commit 60edaf6. ± Comparison against base commit 32b3820.

♻️ This comment has been updated with latest results.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds webhook-triggered command execution (with GitHub-specific filtering) and introduces an “async incremental indexing” mode so card creation can return an assigned ID immediately without waiting for indexing—useful for command/worker-driven writes.

Changes:

  • Add webhook receiver support for looking up registered webhook commands, filtering by event payload/headers, and enqueueing run-command jobs.
  • Add X-Boxel-Async-Index / asyncIndex option to allow “fire-and-forget” incremental indexing and a 202 response that returns an ID immediately.
  • Expand realm session-room notification selection to include all users when a realm is world-readable.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
packages/runtime-common/realm.ts Adds asyncIndex write option and 202 create-card response path to avoid blocking on indexing.
packages/runtime-common/db-queries/session-room-queries.ts Adjusts session-room lookup query to include world-readable realm behavior.
packages/realm-server/handlers/handle-webhook-receiver.ts Executes (enqueues) registered webhook commands after signature verification, using filter handlers.
packages/realm-server/handlers/webhook-filter-handlers.ts Introduces pluggable webhook filter handlers (default + GitHub event filter).
packages/realm-server/tests/server-endpoints/webhook-receiver-test.ts Adds/updates tests asserting webhook receipt and command enqueue behavior.
packages/realm-server/server.ts Allows X-Boxel-Async-Index through CORS.
packages/matrix/scripts/register-github-webhook.ts Adds a script to register an incoming GitHub webhook and associated webhook commands.
packages/host/app/services/store.ts Adds async-index persistence path to obtain server-assigned IDs immediately in render/command contexts.
packages/catalog-realm/github-event/github-event.gts Adds a card type to store GitHub webhook event payloads and derived fields.
packages/catalog-realm/commands/process-github-event.gts Adds a command that saves a GithubEventCard from a webhook event.
packages/catalog-realm/commands/create-submission.ts Adds an import related to card error handling (currently unused).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +811 to +813
if (!options?.asyncIndex) {
await this.indexing();
}
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asyncIndex skips await this.indexing(), but RealmIndexUpdater.update() overwrites #indexingDeferred (see runtime-common/realm-index-updater.ts), so calling update() while a from-scratch index is running can clobber the deferred and leave prior callers waiting on indexing() hanging. Consider still awaiting any in-progress indexing before scheduling the incremental update (even in async mode), or change RealmIndexUpdater to chain/de-dupe indexing promises instead of overwriting the single deferred.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

filter: {
type: 'github-event',
eventType: 'pull_request',
prNumber: 456,
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR-number filtering test registers a github-event filter without submissionRealmUrl. With the current handler behavior, that can cause the command to be skipped due to buildCommandInput throwing, so the test may pass even if prNumber filtering isn’t implemented. To actually validate prNumber matching, include a submissionRealmUrl in the filter and assert commandsExecuted changes appropriately based on the PR number.

Suggested change
prNumber: 456,
prNumber: 456,
submissionRealmUrl: 'http://test-realm',

Copilot uses AI. Check for mistakes.
Comment on lines +217 to +240
// Step 3: Register commands for both pull_request and pull_request_review events.
// Two separate commands are needed because GitHub uses different event headers
// for PR state changes vs. review submissions.
console.log('Setting up webhook commands...');

const baseFilter: Record<string, unknown> = {
type: 'github-event',
};

const eventTypes = [
'pull_request',
'pull_request_review',
'pull_request_review_comment',
'check_run',
'commit_comment',
'discussion_comment',
];
for (let eventType of eventTypes) {
const cmd = await ensureWebhookCommand(jwt, {
incomingWebhookId: webhook.id,
command: commandURL,
filter: { eventType, ...baseFilter },
});
console.log(` ✓ ${eventType.padEnd(22)} → command ${cmd.id}`);
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script registers github-event webhook-command filters that include type and eventType but omit submissionRealmUrl. In this PR’s GithubEventFilterHandler.buildCommandInput, missing submissionRealmUrl throws, so these commands would never be enqueued when events arrive. Either add submissionRealmUrl to the filter in this script, or adjust the handler to derive it when absent.

Copilot uses AI. Check for mistakes.
Comment on lines +26 to +38
class GithubEventFilterHandler implements WebhookFilterHandler {
matches(
headers: Koa.Context['req']['headers'],
filter: Record<string, any>,
): boolean {
let eventType = headers['x-github-event'] as string | undefined;

if (filter.eventType && filter.eventType !== eventType) {
return false;
}

return true;
}
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GithubEventFilterHandler.matches has an incompatible parameter list and is called as matches(payload, headers, filter) by the receiver. As written it will treat the payload object as headers, so header-based filtering will behave incorrectly at runtime. Update the method signature to accept (payload, headers, filter) and implement the documented filtering (e.g. prNumber) using the parsed payload.

Copilot uses AI. Check for mistakes.
Comment on lines +77 to +85
let webhookId = webhook.id as string;
let commandRows;
try {
commandRows = await query(dbAdapter, [
`SELECT id, incoming_webhook_id, command, command_filter`,
`FROM webhook_commands WHERE incoming_webhook_id = `,
param(webhookId),
]);
} catch (_error) {
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The receiver treats command_filter as an object, but in SQLite both incoming_webhooks.verification_config and webhook_commands.command_filter are BLOBs that are stored as JSON text. Without type coercion/parsing, command_filter (and verification_config earlier in this handler) will be a string and filtering/signature verification can break under SQLite. Consider using query(..., coerceTypes: { command_filter: 'JSON', verification_config: 'JSON' }) (or JSON.parse when the value is a string).

Copilot uses AI. Check for mistakes.
isNew;
if (useAsyncIndex) {
// POST with X-Boxel-Async-Index: realm writes the file and returns
// 202 with the assigned card ID immediately (fire-and-forget index).
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says the async-index POST returns 202, but Realm.createCard returns 201 for async-index responses. Please update the comment to match the actual status code to avoid confusion for future maintainers.

Suggested change
// 202 with the assigned card ID immediately (fire-and-forget index).
// 201 with the assigned card ID immediately (fire-and-forget index).

Copilot uses AI. Check for mistakes.
@richardhjtan richardhjtan force-pushed the CS-10217-trigger-command-on-webhook-event-2 branch from d44fccd to 08f2eba Compare March 2, 2026 14:38
@richardhjtan richardhjtan force-pushed the CS-10217-trigger-command-on-webhook-event-2 branch from 08f2eba to df1db1a Compare March 2, 2026 14:43
@richardhjtan richardhjtan requested review from a team, habdelra and lukemelia March 2, 2026 14:43
@richardhjtan richardhjtan marked this pull request as ready for review March 2, 2026 14:43
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df1db1a87d

ℹ️ 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".

Comment on lines +140 to +144
`SELECT username FROM realm_user_permissions WHERE realm_url = `,
param(realmURL),
` AND realm_owner = true LIMIT 1`,
]);
if (realmOwnerRows[0]?.username) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Validate realm access before impersonating realm owner

This code promotes runAs to the target realm owner purely from realmURL derived from command_filter, but there is no authorization check that the webhook owner is allowed to act in that realm. Since webhook-command creation only checks ownership of the webhook itself, a user can register a command/filter pointing at another realm and have jobs executed with that realm owner’s permissions when the webhook fires, which is a privilege-escalation path.

Useful? React with 👍 / 👎.

Comment on lines +34 to +38
if (filter.eventType && filter.eventType !== eventType) {
return false;
}

return true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Enforce PR-number matching in github-event filters

GithubEventFilterHandler.matches only compares eventType and always returns true otherwise, so a filter that includes prNumber does not constrain execution to that PR. In practice, commands configured for a single PR will still run for all pull-request events of the same type (when submissionRealmUrl is present), which mixes unrelated PR events and triggers unintended command runs.

Useful? React with 👍 / 👎.

@richardhjtan richardhjtan marked this pull request as draft March 2, 2026 15:41
@habdelra
Copy link
Contributor

habdelra commented Mar 2, 2026

from your description:

realm.ts — Supports a new X-Boxel-Async-Index: true request header on card POST. When set, the realm writes the file to disk and persists file meta, then returns 201 with the assigned card ID immediately. Indexing runs fire-and-forget in the background. This prevents a deadlock where the run-command worker job can't pick up the incremental-index job it enqueues.

store.ts — Uses the async-index path when creating new cards inside the headless Chrome render context (or when asyncIndex option is explicitly set). Calls setId() on the card instance with the server-assigned ID so downstream commands can reference it.

are we still doing this?

@richardhjtan
Copy link
Contributor Author

@habdelra planning to remove it as we shall try increasing the worker approach in local from the office hour discussion

Comment on lines +41 to +43
static [queryableValue](_value: any, _stack: BaseDef[]): null {
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2026-03-02 at 10 35 04 PM

It will have this error initially before adding this


class ProcessGithubEventInput extends CardDef {
@field eventType = contains(StringField); // from command_filter
@field submissionRealmUrl = contains(StringField); // from command_filter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only one submssion realm right? field not needed. Do we need that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe if its a write. It shud just b called realm

Copy link
Contributor

@tintinthong tintinthong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we want to move all the async index stuff out of this PR


class ProcessGithubEventInput extends CardDef {
@field eventType = contains(StringField); // from command_filter
@field submissionRealmUrl = contains(StringField); // from command_filter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe if its a write. It shud just b called realm

Comment on lines +396 to +406
// New cards still need a server-assigned ID in render context so that
// downstream commands can reference them. Use async-index so the realm
// returns the ID immediately without waiting for indexing to complete.
if (!instance.id && !opts?.doNotPersist) {
return (await this.persistAndUpdate(instance, {
realm: opts?.realm,
localDir: opts?.localDir,
asyncIndex: true,
})) as T | CardErrorJSONAPI;
}
// Existing cards: skip saves in render context (no auto-save).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this portion of code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed because is part of async index work, will take it out

@@ -55,12 +63,23 @@ export async function fetchRealmSessionRooms(
let rows = await query(dbAdapter, [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this fetchSessionRooms changed ahh?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tintinthong Before that, users with read access from the public realm (eg: db permission record like catalog/submission realm with * all users) will not get a notification when a new card is created

The obvious scenario is when in the code mode file tree, the webhook is triggered and the command creates a card - the user will not see the latest file added in the LHS under the submission realm. The user has to refresh to see the latest file result

@richardhjtan richardhjtan force-pushed the CS-10217-trigger-command-on-webhook-event-2 branch from 6764b98 to 60edaf6 Compare March 3, 2026 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants