Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
8d23977
chore(ingestion): initialize project setup and environment configuration
chadd28 Jun 18, 2026
c9cbd86
feat(ingestion): implement Firestore IssuesStore for new documents
chadd28 Jun 18, 2026
2d09842
feat(ingestion): add GitHub webhook HMAC signature verification
chadd28 Jun 18, 2026
a6ef7a9
feat(ingestion): create Express server and webhook ingestion route
chadd28 Jun 18, 2026
40c59c1
test(ingestion): add unit tests for GitHub webhook signature verifica…
chadd28 Jun 18, 2026
a8a0fe8
feat(ingestion): skip Pub/Sub publishing for duplicate issues
chadd28 Jun 18, 2026
5650049
fix(ingestion): secure GitHub signature verification against DoS and …
chadd28 Jun 22, 2026
7c2a01b
fix(ingestion): Improve build, config, and code style
chadd28 Jun 23, 2026
afa05a5
fix(ingestion): resolve webhook robustness and security issues in ing…
chadd28 Jun 23, 2026
812b530
refactor(ingestion): split ingestion service into app and server modules
chadd28 Jun 23, 2026
05334cc
test(ingestion): Add unit tests for webhook endpoint
chadd28 Jun 23, 2026
ca0de47
chore: resolve package-lock.json merge conflict
chadd28 Jun 23, 2026
177e670
fix(ingestion): address feedback for github signature verification
chadd28 Jun 25, 2026
e3aabc4
fix(ingestion): validate webhook payload and repository format strictly
chadd28 Jun 25, 2026
799aefb
chore(ingestion): address transaction await, logging, and documentati…
chadd28 Jun 25, 2026
df24dbb
chore(ingestion): enable eslint to tools folder and apply lint changes
chadd28 Jun 25, 2026
02edc10
fix(ingestion): remove unsafe as casts
chadd28 Jun 26, 2026
ed4870d
chore: merge upstream main and resolve package-lock.json conflict
chadd28 Jun 26, 2026
503e006
chore(ingestion): add rate limiting middleware and resolve codeql sec…
chadd28 Jun 26, 2026
d1ce311
chore(ingestion): remove unused eslint-disable comment directive
chadd28 Jun 26, 2026
c210f2d
Revert package-lock.json changes.
gundermanc Jun 29, 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
12 changes: 12 additions & 0 deletions tools/caretaker-agent/cloudrun/ingestion-service/.dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
node_modules
dist
npm-debug.log
.git
.gitignore
*.py
*.pyc
__pycache__
requirements.txt
project.toml
**/*.test.ts

8 changes: 8 additions & 0 deletions tools/caretaker-agent/cloudrun/ingestion-service/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
FROM node:20-slim
WORKDIR /app
COPY package*.json ./
RUN npm ci
COPY . .
EXPOSE 8080
CMD ["npx", "tsx", "server.ts"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[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.

Suggested change
RUN npm run build
CMD ["node", "dist/server.js"]

Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import { describe, it, expect } from 'vitest';
import { verifyGithubSignature } from './github.js';
import * as crypto from 'node:crypto';

describe('verifyGithubSignature', () => {
const secret = 'my-secret';
const payload = '{"test":true}';

it('should return true for a valid signature', () => {
const hmac = crypto.createHmac('sha256', secret);
hmac.update(payload);
const validSignature = 'sha256=' + hmac.digest('hex');

const result = verifyGithubSignature(payload, validSignature, secret);
expect(result).toBe(true);
});

it('should return false if signatureHeader is missing', () => {
const result = verifyGithubSignature(payload, undefined, secret);
expect(result).toBe(false);
});

it('should return false for an invalid signature', () => {
const result = verifyGithubSignature(
payload,
'sha256=invalid-signature',
secret,
);
expect(result).toBe(false);
});
});
42 changes: 42 additions & 0 deletions tools/caretaker-agent/cloudrun/ingestion-service/auth/github.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import * as crypto from 'node:crypto';

/**
* Verify that the payload was sent from GitHub using HMAC SHA256.
*
* @param payloadBody - The raw body of the request (Buffer or string).
* @param signatureHeader - The value of the X-Hub-Signature-256 header.
Comment thread
gundermanc marked this conversation as resolved.
* @param secret - The GitHub Webhook secret.
* @returns True if the signature is valid, false otherwise.
*/
export function verifyGithubSignature(
payloadBody: Buffer | string,
signatureHeader: string | undefined,
secret: string,
): boolean {
if (!signatureHeader || signatureHeader.length !== 71) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: magic number.

can we define a named const indicating what 71 represents here?

return false;
}

if (!Buffer.isBuffer(payloadBody) && typeof payloadBody !== 'string') {
return false;
}

const hmac = crypto.createHmac('sha256', secret);
hmac.update(payloadBody);
const expectedSignature = 'sha256=' + hmac.digest('hex');

try {
return crypto.timingSafeEqual(
Buffer.from(expectedSignature),
Buffer.from(signatureHeader),
);
} catch {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need to ... or plan to have any sort of logging for errors like this?

return false;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import { describe, it, expect, vi, beforeEach } from 'vitest';
import type { Mock } from 'vitest';
import { IssuesStore } from './issuesStore.js';
import { Firestore, Transaction } from '@google-cloud/firestore';

describe('IssuesStore', () => {
let mockTransaction: {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using mocks is ok but I would be deliberate about what you mock vs. what uses the real object or a realistic stand-in.

The risk of mocks is they can allow you to test modules in isolation and miss gaps when they are combined. They can also mislead you into locking in the wrong behavior if the mock behaves differently than the real code.

When possible I recommend doing integration style tests and choosing a layer that's the 'cutoff' and mocking everything below that.

Firestore is an external dependency, so perhaps it makes sense to have a single local mock firestore implementation that behaves somewhat realistically and is used in all your tests. Alternatively you might be able to use a non-prod instance, but that has tradeoffs.

get: Mock;
set: Mock;
};
let mockDb: Firestore;
let store: IssuesStore;

beforeEach(() => {
// Assign mock read/write methods for transaction
mockTransaction = {
get: vi.fn(),
set: vi.fn(),
};

// Mock Firestore client
mockDb = {
collection: vi.fn().mockReturnThis(),
doc: vi.fn().mockReturnValue({}),
runTransaction: vi
.fn()
.mockImplementation(
(callback: (tx: Transaction) => Promise<unknown>) => {
return callback(mockTransaction as unknown as Transaction);
},
),
} as unknown as Firestore;

store = new IssuesStore(mockDb, 'issues-collection');
});

it('should initialize a new issue if it does not exist', async () => {
// The transaction should mock that the document does not exist
mockTransaction.get.mockResolvedValue({ exists: false });

const result = await store.createIssue(
'google',
'gemini-cli',
123,
'Test Title',
);

expect(result).toBe(true);
expect(mockTransaction.get).toHaveBeenCalled();
expect(mockTransaction.set).toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({
status: 'UNTRIAGED',
github_metadata: expect.objectContaining({
owner: 'google',
repo: 'gemini-cli',
issue_number: 123,
title: 'Test Title',
}),
}),
);
});

it('should return false and skip creation if the issue already exists', async () => {
// The transaction should mock that the document already exists
mockTransaction.get.mockResolvedValue({ exists: true });

const result = await store.createIssue(
'google',
'gemini-cli',
123,
'Test Title',
);

expect(result).toBe(false);
expect(mockTransaction.get).toHaveBeenCalled();
expect(mockTransaction.set).not.toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import {
Firestore,
FieldValue,
DocumentReference,
Transaction,
} from '@google-cloud/firestore';

export class IssuesStore {
private db: Firestore;
Comment thread
gundermanc marked this conversation as resolved.
Outdated
private collectionName: string;

constructor(db: Firestore, collectionName: string) {
this.db = db;
this.collectionName = collectionName;
}

// Generates the standardized Firestore document reference for an issue
getIssueRef(
owner: string,
repo: string,
issueNumber: number,
): DocumentReference {
const docId = `github_${owner}_${repo}_${issueNumber}`;
return this.db.collection(this.collectionName).doc(docId);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

}

// Initializes a new issue document in a transaction
async createIssue(
owner: string,
repo: string,
issueNumber: number,
title: string,
): Promise<boolean> {
const docRef = this.getIssueRef(owner, repo, issueNumber);

return this.db.runTransaction(async (transaction: Transaction) => {
Comment thread
gundermanc marked this conversation as resolved.
Outdated
const snapshot = await transaction.get(docRef);

if (!snapshot.exists) {
transaction.set(docRef, {
status: 'UNTRIAGED',
triage_attempts: 0,
workable_spec: {},
lock: {
holder: null,
expires_at: null,
},
created_at: FieldValue.serverTimestamp(),
updated_at: FieldValue.serverTimestamp(),
github_metadata: {
owner: owner,
repo: repo,
issue_number: issueNumber,
title: title,
},
});
return true;
}
return false;
});
}
}
24 changes: 24 additions & 0 deletions tools/caretaker-agent/cloudrun/ingestion-service/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{
"name": "ingestion-service",
"version": "1.0.0",
"description": "Ingestion service for triage worker",
"main": "server.ts",
"scripts": {
"dev": "tsx watch server.ts",
"start": "tsx server.ts",
"test": "vitest run"
},
"dependencies": {
"@google-cloud/firestore": "^7.7.0",
"@google-cloud/pubsub": "^4.4.0",
"dotenv": "^16.4.5",
"express": "^4.19.2"
},
"devDependencies": {
"@types/express": "^4.17.21",
"@types/node": "^20.12.12",
"tsx": "^4.9.3",
"typescript": "^5.4.5",
"vitest": "^1.6.0"
}
}
Loading
Loading