feat(caretaker): implement Cloud Run webhook ingestion service#28015
feat(caretaker): implement Cloud Run webhook ingestion service#28015chadd28 wants to merge 20 commits into
Conversation
|
📊 PR Size: size/XL
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new ingestion service for the Caretaker Agent, designed to run on Cloud Run. The service acts as a secure gateway for GitHub webhooks, handling event validation, deduplication via Firestore transactions, and message queuing through Pub/Sub. This infrastructure enables automated downstream triage workflows for incoming GitHub issues. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 a new Express-based ingestion service for a triage worker, featuring GitHub webhook signature verification, Firestore storage for tracking issues, and Pub/Sub integration. The review feedback highlights three critical security and reliability improvements: validating the signature length and payload type in the GitHub auth module to prevent crashes and DoS attacks, escaping the issue body to mitigate prompt injection vulnerabilities, and checking the return value of the issue creation transaction to prevent duplicate Pub/Sub messages.
| } | ||
|
|
||
| // Payload preprocessing | ||
| const sanitizedBody = `<untrusted_context>\n${payload.issue?.body || ''}\n</untrusted_context>`; |
There was a problem hiding this comment.
Wrapping untrusted user input directly in <untrusted_context> tags without escaping or sanitizing the input makes the system vulnerable to prompt injection. An attacker could include </untrusted_context> in their issue body to escape the context block and inject malicious instructions.
Sanitize or escape any occurrences of </untrusted_context> in the issue body before wrapping it.
const rawBody = payload.issue?.body || '';
const escapedBody = rawBody.replace(/<\/untrusted_context>/g, '[escaped_untrusted_context_tag]');
const sanitizedBody = `\<untrusted_context\>\n\${escapedBody}\n\</untrusted_context\>`;
gemini-cli
Bot
added
the
status/need-issue
chadd28
force-pushed
the
feature/caretaker-agent
branch
from
ef27d32 to
5650049
Compare
| COPY . . | ||
| EXPOSE 8080 | ||
| CMD ["npx", "tsx", "server.ts"] | ||
|
|
There was a problem hiding this comment.
[P1] Production anti-pattern.
Running tsx (or ts-node) in a production container introduces significant memory overhead and startup latency. Add a "build": "tsc" script to package.json and run the compiled JavaScript here instead.
| RUN npm run build | |
| CMD ["node", "dist/server.js"] |
|
|
||
| // Publish to Pub/Sub | ||
| const dataBuffer = Buffer.from(JSON.stringify(processedData)); | ||
| const messageId = await topic.publishMessage({ data: dataBuffer }); |
There was a problem hiding this comment.
[P0] Dual-write failure state permanently drops retried webhooks.
If the Firestore write succeeds but the Pub/Sub publish fails, the server returns a 500 error, and GitHub will retry. On the retry, createIssue returns false (since the document already exists), causing the endpoint to return 200 early and skip the Pub/Sub publish entirely. The issue will never be processed downstream. You should publish to Pub/Sub even if the DB document exists (assuming downstream is idempotent) or rely entirely on Pub/Sub and let the downstream worker deduplicate.
| }); | ||
|
|
||
| app.post('/webhook', async (req, res) => { | ||
| const signature = req.headers['x-hub-signature-256'] as string | undefined; |
There was a problem hiding this comment.
[P2] Unsafe header cast.
Express headers can be an array (string[]) if multiple headers with the same name are sent. If an array is passed, downstream checks like signature.length will evaluate the array length rather than string length.
| const signature = req.headers['x-hub-signature-256'] as string | undefined; | |
| const header = req.headers['x-hub-signature-256']; | |
| const signature = Array.isArray(header) ? header[0] : header; |
| let payload: GitHubWebhookPayload; | ||
| try { | ||
| payload = JSON.parse(req.body.toString()) as GitHubWebhookPayload; | ||
| } catch { |
There was a problem hiding this comment.
[P2] Blind type casting.
The explicit cast as GitHubWebhookPayload bypasses runtime safety. At minimum, we should validate the presence of payload.issue.number and payload.repository.full_name before assuming the structure exists to avoid unexpected undefined errors downstream.
|
|
||
| // Payload preprocessing | ||
| const sanitizedBody = `<untrusted_context>\n${payload.issue?.body || ''}\n</untrusted_context>`; | ||
| const processedData = { |
There was a problem hiding this comment.
[P2] Misleading variable name / Missing sanitization.
This is not actually sanitized. If a user maliciously includes </untrusted_context> in their GitHub issue description, they will break out of the LLM context wrapper downstream. Rename this to wrappedBody to be accurate, or implement actual tag escaping/stripping.
galdawave
left a comment
galdawave
left a comment
There was a problem hiding this comment.
[P0] Missing core unit tests.
The PR introduces complex routing, orchestrating validation, DB writes, and Pub/Sub publishing in server.ts without a corresponding server.test.ts. This violates the testing standard criteria. Please add tests that mock the Firestore and Pub/Sub clients to verify the endpoint behavior.
…estion service - Fix dual-write failure: verify issue is UNTRIAGED before ignoring duplicate database entries, resolving Pub/Sub retries at ingestion to avoid wasting downstream worker startup costs. - Fix unsafe header cast: safely extract first element if header is a string array. - Fix blind payload type casting: validate that payload is a non-null object and verify required fields exist before processing. - Fix missing sanitization: rename to wrappedBody and escape context breakout tags. - Use pre-validated variables (issueNumber, repository) directly in payload preprocessing.
| number?: number; | ||
| title?: string; | ||
| }; | ||
| repository?: { |
There was a problem hiding this comment.
Can we specify the format of this in a comment? is it an HTTPS URI or an [org]/[repo] id?
| !databaseId || | ||
| !collectionName | ||
| ) { | ||
| throw new Error('Missing required environment variables'); |
There was a problem hiding this comment.
minor nit: might be nice to log which variable we're missing.
This is also an opportunity to encapsulate the failure a bit.
Could have something like
const projectId = getEnvironmentVariable('PROJECT_ID');
const topicId = getEnvironmentVariable('TOPIC_ID');
...
function getEnvironmentVariable(name: string | undefined): string {
const value = process.env[name];
if (!value || typeof value !== 'string') {
throw new Error(`Undefined required environment variable ${name}`);
}
}
| const issuesStore = new IssuesStore(db, collectionName); | ||
|
|
||
| // Middleware: read incoming JSON payloads as raw Buffer bytes | ||
| app.use(express.raw({ type: 'application/json', limit: '1mb' })); |
There was a problem hiding this comment.
What's the failure mode if the request is too large? Do we dead letter or is there another opportunity to process the event?
| app.use(express.raw({ type: 'application/json', limit: '1mb' })); | ||
|
|
||
| app.get('/', (req, res) => { | ||
| res.send('Hello World!'); |
There was a problem hiding this comment.
Should we send anything back here?
FWIW I've commonly seen a Git SHA returned from endpoints to make it really easy to check that deployment succeeded.
| title: payload.issue?.title, | ||
| }; | ||
|
|
||
| const [owner, repo] = repository.split('/'); |
There was a problem hiding this comment.
Is it possible we get an unexpected repo specifier format (e.g.: a full URI) and this results in unexpected behavior (e.g.: creating malformed or erroneous DB entries)?
| issueNumber: number, | ||
| ): DocumentReference { | ||
| const docId = `github_${owner}_${repo}_${issueNumber}`; | ||
| return this.db.collection(this.collectionName).doc(docId); |
There was a problem hiding this comment.
We are storing issues right in the top level Firestore collection passed in via the environment variable.
This means that any new data types we store will be in this same collection as well, or perhaps nested, leaving the top level collection containing nested collections and individual issues docs.
Should we proactively create a nested collection for issues to keep the door open to extend this?
gundermanc
left a comment
gundermanc
left a comment
There was a problem hiding this comment.
LGTM. I left a few comments. I think there might be one floating promise.
|
I have pushed the remaining commits addressing your review feedback. Specifically, in the final commit, I enabled strict ESLint checks for the tools/ folder and resolved all outstanding lint and type-safety warnings. Could you double-check that my changes to eslint.config.js look good to you @gundermanc ? Here are my thoughts on some of the points you raised:
|
| next: express.NextFunction, | ||
| ) => { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| const error = err as { status?: number; message?: string }; |
There was a problem hiding this comment.
Can we eliminate this as as well? I'd recommend blanket avoiding the as keyword altogether as it's almost always a footgun.
I think you can test the existence of specific properties, like status, if needed via the in keyword.
There was a problem hiding this comment.
resolved! Used property checking ('status' in err)
| issueNumber: number, | ||
| ): DocumentReference<IssueDocument> { | ||
| const docId = `github_${owner}_${repo}_${issueNumber}`; | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion |
There was a problem hiding this comment.
Can we eliminate this cast as well?
|
Why won't you talk to me on the phone
…On Thu, Jun 25, 2026, 20:25 Chad ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tools/caretaker-agent/cloudrun/ingestion-service/app.ts
<#28015 (comment)>
:
> + } catch (error) {
+ console.error('Error processing webhook:', error);
+ const message = error instanceof Error ? error.message : 'Unknown error';
+ return res.status(500).json({ status: 'error', message });
+ }
+});
+
+app.use(
+ (
+ err: unknown,
+ req: express.Request,
+ res: express.Response,
+ next: express.NextFunction,
+ ) => {
+ // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
+ const error = err as { status?: number; message?: string };
resolved! Used property checking ('status' in err)
—
Reply to this email directly, view it on GitHub
<#28015?email_source=notifications&email_token=BUZGG3C7OBCEIVRXATKY2PL5BW7JNA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJXGU4TIMZRHE32M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#discussion_r3478465927>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BUZGG3BNVJLHZ2GMHKUZOGT5BW7JNAVCNFSNUABFKJSXA33TNF2G64TZHM4TMOBRHE3TEMJWHNEXG43VMU5TINRZGQ3TMMZTHA2KC5QC>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
Public service announcement
…On Thu, Jun 25, 2026, 21:01 Richard Sloane ***@***.***> wrote:
Why won't you talk to me on the phone
On Thu, Jun 25, 2026, 20:25 Chad ***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In tools/caretaker-agent/cloudrun/ingestion-service/app.ts
> <#28015 (comment)>
> :
>
> > + } catch (error) {
> + console.error('Error processing webhook:', error);
> + const message = error instanceof Error ? error.message : 'Unknown error';
> + return res.status(500).json({ status: 'error', message });
> + }
> +});
> +
> +app.use(
> + (
> + err: unknown,
> + req: express.Request,
> + res: express.Response,
> + next: express.NextFunction,
> + ) => {
> + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
> + const error = err as { status?: number; message?: string };
>
> resolved! Used property checking ('status' in err)
>
> —
> Reply to this email directly, view it on GitHub
> <#28015?email_source=notifications&email_token=BUZGG3C7OBCEIVRXATKY2PL5BW7JNA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJXGU4TIMZRHE32M4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#discussion_r3478465927>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BUZGG3BNVJLHZ2GMHKUZOGT5BW7JNAVCNFSNUABFKJSXA33TNF2G64TZHM4TMOBRHE3TEMJWHNEXG43VMU5TINRZGQ3TMMZTHA2KC5QC>
> .
> You are receiving this because you are subscribed to this thread.Message
> ID: ***@***.***>
>
|
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
6 participants
Summary
Implements the Cloud Run Webhook Ingestion Service for the Caretaker Agent. The service acts as an entry point for GitHub webhooks, verifies incoming payload signatures, stores new issue entries using Firestore transactions, and publishes sanitized issue metadata to a GCP Pub/Sub topic for downstream processing.
Details
server.ts): The main Express server (to be hosted on Cloud Run) that receives GitHubissues.openedevents, validates their signature, adds the issue to Firestore, and publishes the issue details to Pub/Sub.auth/github.ts): HMAC SHA-256 signature verification helper usingnode:cryptoand a secure timing-safe equality check.db/issuesStore.ts): Initializes new issues in Firestore using a transaction.issuesStore.test.ts(Firestore mock tests) andgithub.test.ts(HMAC signature verification tests).How to Validate
Run unit tests in the service directory:
cd tools/caretaker-agent/cloudrun/ingestion-service npm install npx vitest runPre-Merge Checklist
@galz10