Skip to content

refactor(webhook): add persistent keys in logs #5030

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
npalm merged 2 commits into from
Mar 10, 2026
Merged

refactor(webhook): add persistent keys in logs #5030

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c3eaee
feat: add persistent keys in logs
edersonbrilhante Feb 13, 2026
9cc080a
Merge branch 'main' into feat-log-persistent-keys
edersonbrilhante Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions lambdas/functions/webhook/src/webhook/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export async function publishForRunners(

const checkBodySizeResult = checkBodySize(body, headers);

const { event, eventType } = readEvent(headers, body);
const { event, eventType } = readEvent(headers, body, ['workflow_job']);
logger.info(`Github event ${event.action} accepted for ${event.repository.full_name}`);
if (checkBodySizeResult.sizeExceeded) {
// We only warn for large event, when moving the event bridge we can only can accept events up to 256KB
Expand All @@ -39,11 +39,10 @@ export async function publishOnEventBridge(

await verifySignature(headers, body, config.webhookSecret);

const eventType = headers['x-github-event'] as string;
checkEventIsSupported(eventType, config.allowedEvents);

const checkBodySizeResult = checkBodySize(body, headers);

const { eventType } = readEvent(headers, body, config.allowedEvents);

Comment on lines 42 to +45
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

publishOnEventBridge now calls readEvent (which does JSON.parse and appends workflow_job-derived persistent keys) before checking sizeExceeded. This means oversized payloads will still be fully parsed and may throw before the code can publish the error.<eventType> event. If the intent is to always emit the error event for oversized bodies, consider moving the readEvent call inside the non-oversize branch, or making readEvent able to safely extract/log only header-derived fields when sizeExceeded is true.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@edersonbrilhante edersonbrilhante Mar 9, 2026

Choose a reason for hiding this comment

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

@npalm Same here

logger.info(
`Github event ${headers['x-github-event'] as string} accepted for ` +
`${headers['x-github-hook-installation-target-id'] as string}`,
Expand Down Expand Up @@ -127,9 +126,13 @@ function checkEventIsSupported(eventType: string, allowedEvents: string[]): void
}
}

function readEvent(headers: IncomingHttpHeaders, body: string): { event: WorkflowJobEvent; eventType: string } {
function readEvent(
headers: IncomingHttpHeaders,
body: string,
allowedEvents: string[],
): { event: WorkflowJobEvent; eventType: string } {
const eventType = headers['x-github-event'] as string;
checkEventIsSupported(eventType, ['workflow_job']);
checkEventIsSupported(eventType, allowedEvents);

const event = JSON.parse(body) as WorkflowJobEvent;
logger.appendPersistentKeys({
Comment on lines 134 to 138
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

readEvent parses the payload as a WorkflowJobEvent (JSON.parse(body) as WorkflowJobEvent) regardless of eventType/allowedEvents. Since ConfigWebhookEventBridge.allowedEvents can include non-workflow_job events (e.g. push, pull_request from ConfigLoader tests), this will break at runtime when an allowed event has a different schema. Consider making readEvent conditional on eventType === 'workflow_job' (and only then parsing/appending workflow_job keys), or splitting into separate helpers for “validate event type” vs “parse workflow_job event”.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@edersonbrilhante edersonbrilhante Mar 9, 2026

Choose a reason for hiding this comment

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

@npalm do you agree with this review? I am not sure

Expand Down
Loading
⚡