Skip to content

Cs 10217 trigger command on webhook event#4074

Closed
richardhjtan wants to merge 13 commits intocs-9945-submission-bot-open-a-github-prfrom
CS-10217-trigger-command-on-webhook-event
Closed

Cs 10217 trigger command on webhook event#4074
richardhjtan wants to merge 13 commits intocs-9945-submission-bot-open-a-github-prfrom
CS-10217-trigger-command-on-webhook-event

Conversation

@richardhjtan
Copy link
Contributor

@richardhjtan richardhjtan commented Feb 26, 2026

  • Use command runner from headless chrome to run command when filter is matched
  • 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

realm: input.realm,
localDir: input.localDir,
});
if (input.doNotWaitForPersist) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added doNotWaitForPersist option for save card command, this avoid timeout issue when running the command in headless chrome

@github-actions
Copy link

Preview deployments

@github-actions
Copy link

github-actions bot commented Feb 26, 2026

Host Test Results

    1 files  ±0      1 suites  ±0   1h 33m 46s ⏱️ - 2m 43s
1 871 tests ±0  1 854 ✅  - 1  15 💤 ±0  0 ❌ ±0  2 🔥 +1 
1 886 runs  ±0  1 867 ✅  - 2  15 💤 ±0  2 ❌ +1  2 🔥 +1 

For more details on these errors, see this check.

Results for commit d08e7f7. ± Comparison against base commit 36bd710.

♻️ This comment has been updated with latest results.

@richardhjtan richardhjtan marked this pull request as ready for review February 26, 2026 15:20
@richardhjtan richardhjtan requested review from a team February 26, 2026 15:20
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: d08e7f78c8

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

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

This pull request implements webhook command execution functionality to automatically run commands when GitHub webhook events are received. It introduces a GitHub event card definition for storing webhook event data, adds a command to process GitHub events, and includes comprehensive test coverage for the new functionality.

Changes:

  • Add webhook command execution with filtering support (event type, PR number)
  • Introduce GitHub event card for storing webhook events as data
  • Update session room queries to support world-readable realms
  • Add webhook management methods to realm server service

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/realm-server/handlers/handle-webhook-receiver.ts Implements command execution logic with event filtering and queuing
packages/runtime-common/db-queries/session-room-queries.ts Modifies query to include users with world-readable realm access
packages/realm-server/tests/server-endpoints/webhook-receiver-test.ts Adds comprehensive tests for webhook command execution and filtering
packages/matrix/scripts/register-github-webhook.ts New script for registering GitHub webhooks with event filtering
packages/host/app/services/realm-server.ts Adds webhook management API methods (list, create)
packages/host/app/commands/save-card.ts Adds support for non-blocking card persistence
packages/host/app/commands/bot-requests/create-listing-pr-request.ts Integrates webhook registration into PR creation flow
packages/catalog-realm/github-event/github-event.gts New card definition for storing GitHub webhook events
packages/catalog-realm/commands/process-github-event.gts New command to process GitHub webhook events and create event cards
packages/base/commands/search-card-result.gts Adds queryableValue implementation for JsonField
packages/base/command.gts Adds doNotWaitForPersist field to SaveCardInput
packages/host/app/commands/create-listing-pr.ts Minor comment update
Comments suppressed due to low confidence (2)

packages/realm-server/handlers/handle-webhook-receiver.ts:166

  • Inconsistent logging approach: The code uses console.warn and console.error directly, while other handlers in the codebase use the logger utility from @cardstack/runtime-common. For consistency and better log management (including proper log levels and module identification), consider using the logger utility: const log = logger('webhook-receiver'); and then log.warn(...) and log.error(...).
      console.warn('Failed to parse webhook payload for filtering');
    }

    let eventType = ctxt.req.headers['x-github-event'] as string | undefined;

    let executedCommands = 0;
    for (let commandRow of commandRows) {
      let commandFilter = commandRow.command_filter as Record<
        string,
        any
      > | null;

      // Apply filter if specified
      if (commandFilter) {
        // Check if event type matches filter
        if (commandFilter.eventType && commandFilter.eventType !== eventType) {
          continue;
        }

        // Check if PR number matches filter (for pull_request events)
        if (
          commandFilter.prNumber &&
          payload.pull_request?.number !== commandFilter.prNumber
        ) {
          continue;
        }

        // Additional filter checks can be added here as needed
      }

      let commandURL = commandRow.command as string;
      let submissionRealmUrl =
        (commandFilter?.submissionRealmUrl as string | undefined) ??
        new URL('/submissions/', commandURL).href;

      // Run as the realm owner so they have write permissions in the submission realm
      let realmOwnerRows = await query(dbAdapter, [
        `SELECT username FROM realm_user_permissions WHERE realm_url = `,
        param(submissionRealmUrl),
        ` AND realm_owner = true LIMIT 1`,
      ]);
      let runAs =
        (realmOwnerRows[0]?.username as string | undefined) ??
        (webhook.username as string);

      let commandInput = {
        eventType: eventType ?? '',
        submissionRealmUrl,
        payload,
      };

      try {
        await enqueueRunCommandJob(
          {
            realmURL: submissionRealmUrl,
            realmUsername: runAs,
            runAs,
            command: commandURL,
            commandInput,
          },
          queue,
          dbAdapter,
          userInitiatedPriority,
        );
        executedCommands++;
      } catch (error) {
        console.error(
          `Failed to enqueue webhook command ${commandURL}:`,
          error,
        );

packages/matrix/scripts/register-github-webhook.ts:20

  • Header name case mismatch: The script uses lowercase 'x-hub-signature-256' in the webhook configuration, but the tests use 'X-Hub-Signature-256' with proper capitalization. While the handler correctly normalizes headers to lowercase when reading (line 213), this inconsistency could cause confusion. Consider using the canonical capitalization 'X-Hub-Signature-256' consistently in both places, as this is the standard format used by GitHub webhooks.
    header: 'x-hub-signature-256',

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

Comment on lines +13 to +15
import type MatrixService from '../services/matrix-service';
import type RealmServerService from '../services/realm-server';
import type StoreService from '../services/store';
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The import paths for services are incorrect. Since this file is in the bot-requests subdirectory, the imports should use '../../services/' instead of '../services/'. The correct paths should be:

  • import type MatrixService from '../../services/matrix-service';
  • import type RealmServerService from '../../services/realm-server';
  • import type StoreService from '../../services/store';

This pattern is correctly used in send-bot-trigger-event.ts in the same directory.

Suggested change
import type MatrixService from '../services/matrix-service';
import type RealmServerService from '../services/realm-server';
import type StoreService from '../services/store';
import type MatrixService from '../../services/matrix-service';
import type RealmServerService from '../../services/realm-server';
import type StoreService from '../../services/store';

Copilot uses AI. Check for mistakes.
Comment on lines +133 to +140
let realmOwnerRows = await query(dbAdapter, [
`SELECT username FROM realm_user_permissions WHERE realm_url = `,
param(submissionRealmUrl),
` AND realm_owner = true LIMIT 1`,
]);
let runAs =
(realmOwnerRows[0]?.username as string | undefined) ??
(webhook.username as string);
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The database query to fetch realm owner is not wrapped in a try-catch block, but it's inside a loop that processes webhook commands. If this query fails for any reason (e.g., database connection issues), it will cause the entire webhook request to fail with an unhandled exception, potentially losing valid webhook events. Consider wrapping this query in a try-catch block and logging the error while continuing to process other commands, or handle the error gracefully by skipping this specific command.

Suggested change
let realmOwnerRows = await query(dbAdapter, [
`SELECT username FROM realm_user_permissions WHERE realm_url = `,
param(submissionRealmUrl),
` AND realm_owner = true LIMIT 1`,
]);
let runAs =
(realmOwnerRows[0]?.username as string | undefined) ??
(webhook.username as string);
let runAs = webhook.username as string;
try {
let realmOwnerRows = await query(dbAdapter, [
`SELECT username FROM realm_user_permissions WHERE realm_url = `,
param(submissionRealmUrl),
` AND realm_owner = true LIMIT 1`,
]);
if (realmOwnerRows[0]?.username) {
runAs = realmOwnerRows[0].username as string;
}
} catch (error) {
console.error(
`Failed to fetch realm owner for submission realm ${submissionRealmUrl}:`,
error,
);
}

Copilot uses AI. Check for mistakes.
Comment on lines +60 to +76
'WHERE u.session_room_id IS NOT NULL',
'AND (',
' EXISTS (',
' SELECT 1 FROM realm_user_permissions',
' WHERE realm_url =',
param(realmURL),
'AND (rup.read = true OR rup.write = true)',
'AND u.session_room_id IS NOT NULL',
' AND username = u.matrix_user_id',
' AND (read = true OR write = true)',
' )',
' OR EXISTS (',
' SELECT 1 FROM realm_user_permissions',
' WHERE realm_url =',
param(realmURL),
" AND username = '*'",
' AND read = true',
' )',
')',
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The refactored query logic appears to have a potential issue. When a realm is world-readable (username = '*' with read = true), the query will return ALL users with session rooms, not just those who should have access to this specific realm. The OR EXISTS clause for world-readable realms doesn't restrict which users are returned - it just checks if such a permission exists for the realm, and if it does, all users in the WHERE clause match.

This could expose session room information for users who shouldn't have access to this realm. The original JOIN-based approach correctly filtered users to only those with explicit permissions for this realm. Consider revising the logic to ensure that world-readable access doesn't inadvertently expose all user session rooms.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@lukemelia lukemelia left a comment

Choose a reason for hiding this comment

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

This needs some work. See comments.

@field card = linksTo(CardDef);
@field realm = contains(StringField);
@field localDir = contains(StringField);
@field doNotWaitForPersist = contains(BooleanField);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we doing this? It seems problematic -- i.e. what if saving fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, reverted this

Comment on lines +89 to +92
const githubWebhook = webhooks.find(
(w: { verificationType: string }) =>
w.verificationType === 'HMAC_SHA256_HEADER',
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a robust way to lookup the webhook. Other webhooks could use this verification type as well couldn't they?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should go ahead and do the above TODO now

Comment on lines +341 to +343
get submissionRealmURL(): string {
return `${this.url.href}submissions/`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems awkwardly specific for the realm-server service. Can we define it somewhere else?

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.

why was this necessary?

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

@lukemelia This is to fix this card error

Comment on lines +112 to +124
if (commandFilter.eventType && commandFilter.eventType !== eventType) {
continue;
}

// Check if PR number matches filter (for pull_request events)
if (
commandFilter.prNumber &&
payload.pull_request?.number !== commandFilter.prNumber
) {
continue;
}

// Additional filter checks can be added here as needed
Copy link
Contributor

Choose a reason for hiding this comment

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

The command filter should specify a type along with its configuration and we should use the type to look up a code block somewhere that has this specific logic of comparing the payload with the filter configuration. The logic should not be in line within this handle-webhook-receiver.

Comment on lines +128 to +146
let submissionRealmUrl =
(commandFilter?.submissionRealmUrl as string | undefined) ??
new URL('/submissions/', commandURL).href;

// Run as the realm owner so they have write permissions in the submission realm
let realmOwnerRows = await query(dbAdapter, [
`SELECT username FROM realm_user_permissions WHERE realm_url = `,
param(submissionRealmUrl),
` AND realm_owner = true LIMIT 1`,
]);
let runAs =
(realmOwnerRows[0]?.username as string | undefined) ??
(webhook.username as string);

let commandInput = {
eventType: eventType ?? '',
submissionRealmUrl,
payload,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic of assembling the command input also should not be in line here. It needs to be abstracted in some way. The idea is that this route handler should handle any webhook in the system, not just the github webhook.

try {
await enqueueRunCommandJob(
{
realmURL: submissionRealmUrl,
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be fixed. It should be based on something from the database.

Comment on lines 53 to +76
@@ -55,12 +57,23 @@ export async function fetchRealmSessionRooms(
let rows = await query(dbAdapter, [
'SELECT u.matrix_user_id, u.session_room_id',
'FROM users u',
'JOIN realm_user_permissions rup',
'ON rup.username = u.matrix_user_id',
'WHERE rup.realm_url =',
'WHERE u.session_room_id IS NOT NULL',
'AND (',
' EXISTS (',
' SELECT 1 FROM realm_user_permissions',
' WHERE realm_url =',
param(realmURL),
'AND (rup.read = true OR rup.write = true)',
'AND u.session_room_id IS NOT NULL',
' AND username = u.matrix_user_id',
' AND (read = true OR write = true)',
' )',
' OR EXISTS (',
' SELECT 1 FROM realm_user_permissions',
' WHERE realm_url =',
param(realmURL),
" AND username = '*'",
' AND read = true',
' )',
')',
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you check other consumers of this method to see if there are any unintended consequences of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no tests about this, I have conducted a test to demonstrate different scenarios to understand and avoid unintentional behaviour

@richardhjtan richardhjtan force-pushed the CS-10217-trigger-command-on-webhook-event branch from 60f3787 to 7982631 Compare March 2, 2026 07:20
@richardhjtan
Copy link
Contributor Author

Close this PR due to the base branch PR being closed and split into several PRs.

The feedback changes and newer approach are addressed in #4098

cc: @lukemelia @tintinthong

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.

4 participants