feat: XP Report Automation - Complete backend service (#196)#202
feat: XP Report Automation - Complete backend service (#196)#202abdullahairmail411-blip wants to merge 2 commits into
Conversation
Critical fixes: - Fix Ed25519 signing: use keyPair.fromSeed() to expand 32-byte seed to 64-byte secret key - Remove GITHUB_TOKEN from workflow inputs (security - prevents exposure in logs) - Fix Dockerfile: multi-stage build with proper native build tools for better-sqlite3 Major fixes: - Add SQLite WAL mode pragma (already present, confirmed working) - Add HTML escaping to prevent injection in email templates - Fix race condition in workflow run correlation with created timestamp filter - Add status column to processed_orgs for failure recovery (pending/completed/failed) - Remove PII (email) from stats endpoint response - Fix URL parsing regex to require https://github.com prefix Minor fixes: - Rename X25519_PRIVATE_KEY to ED25519_PRIVATE_KEY (correct algorithm name) - Remove node-fetch dependency (Node 20 has native fetch) - Add @types/better-sqlite3 to devDependencies - Keep package-lock.json in repo (needed for npm ci) - Update .env.example with correct key generation instructions - Update init-db.js to support DB_PATH env var and status column
📝 WalkthroughWalkthroughAdds the xp-report-automation service: a Node.js/TypeScript Express API that accepts GitHub repo submissions, dispatches a signed GitHub Actions workflow to compute XP, records processing state in SQLite, and emails results. Adds Dockerfile, docker-compose, TypeScript config, package.json, DB init and test scripts, health and admin endpoints, and extensive docs (README, ARCHITECTURE, DEPLOYMENT, IMPLEMENTATION_SUMMARY). Enforces one free report per organization. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
xp-report-automation/IMPLEMENTATION_SUMMARY.md-26-26 (1)
26-26:⚠️ Potential issue | 🟡 MinorFenced code blocks missing language identifier (lines 26 and 117).
markdownlint MD040 — add a language tag (e.g.,
```text) to suppress the warning.xp-report-automation/docs/DEPLOYMENT.md-406-409 (1)
406-409:⚠️ Potential issue | 🟡 MinorGmail "Less secure app access" was permanently removed by Google.
Google removed this feature in 2022. Only App Passwords (with 2-Step Verification) or OAuth2 work now. Update this section to remove the deprecated option and point users directly to App Passwords.
xp-report-automation/docs/DEPLOYMENT.md-37-43 (1)
37-43:⚠️ Potential issue | 🟡 Minor
--set-env-varsputs secrets into shell history.Passing
GITHUB_TOKEN,X25519_PRIVATE_KEY,SMTP_PASS, etc. directly in thegcloud run deploycommand exposes them in shell history and process listings. Recommend using Cloud Secret Manager instead:# Store secrets gcloud secrets create github-token --data-file=<(echo -n "$GITHUB_TOKEN") # Reference in deploy gcloud run deploy xp-report-automation \ --set-secrets "GITHUB_TOKEN=github-token:latest,ED25519_PRIVATE_KEY=ed25519-key:latest"xp-report-automation/scripts/test-api.js-22-23 (1)
22-23:⚠️ Potential issue | 🟡 MinorUsage error message path mismatch.
🛠️ Proposed fix
- console.error('Usage: node test-api.js <repoUrl> <email>'); - console.error('Example: node test-api.js https://github.com/ubiquity/pay.ubq.fi test@example.com'); + console.error('Usage: node scripts/test-api.js <repoUrl> <email>'); + console.error('Example: node scripts/test-api.js https://github.com/ubiquity/pay.ubq.fi test@example.com');xp-report-automation/scripts/init-db.js-17-29 (1)
17-29:⚠️ Potential issue | 🟡 MinorMissing
updated_atcolumn for status change tracking.The
statuscolumn enables pending/completed/failed recovery, but without a timestamp you can't tell when a record last transitioned — making recovery logic based on stale-pending detection impossible.🛠️ Proposed fix
CREATE TABLE IF NOT EXISTS processed_orgs ( org_name TEXT PRIMARY KEY, repo_url TEXT NOT NULL, processed_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, email TEXT, workflow_run_id INTEGER, status TEXT DEFAULT 'pending' );xp-report-automation/docs/DEPLOYMENT.md-362-374 (1)
362-374:⚠️ Potential issue | 🟡 MinorDeleting WAL/SHM files risks data loss.
If the process was killed (not cleanly stopped), the WAL file contains committed-but-not-checkpointed pages. Deleting it discards those writes. Replace with an explicit checkpoint:
🛠️ Safer alternative
-# Remove lock files -rm db/xp-reports.db-wal db/xp-reports.db-shm +# Checkpoint and truncate WAL safely +sqlite3 db/xp-reports.db "PRAGMA wal_checkpoint(TRUNCATE);"xp-report-automation/docs/ARCHITECTURE.md-33-40 (1)
33-40:⚠️ Potential issue | 🟡 MinorBroken markdown fences throughout the document.
Code blocks opened with
```sql,```typescript,```bashare closed with```plaintextinstead of```. This corrupts rendering — every code block after line 40 will be malformed. The same pattern repeats at lines 73, 143, 155, 166, 209, 285, 325, 347, 367.Example fix (apply same pattern everywhere)
-```plaintext +```xp-report-automation/docs/ARCHITECTURE.md-33-39 (1)
33-39:⚠️ Potential issue | 🟡 MinorSchema is missing the
statuscolumn.The PR adds a
status TEXT DEFAULT 'pending'column toprocessed_orgs(seesrc/index.tsline 24), but this doc shows the old schema without it. Keep docs in sync with the code.xp-report-automation/docs/ARCHITECTURE.md-66-72 (1)
66-72:⚠️ Potential issue | 🟡 MinorDoc references "X25519" but the key was renamed to
ED25519_PRIVATE_KEY.The PR objectives and
src/index.ts(line 91) useED25519_PRIVATE_KEY. This doc still says "X25519" in multiple places (lines 66, 136, 309). Ed25519 and X25519 are different curves (signing vs. key exchange), so the current wording is cryptographically misleading.Also applies to: 136-136, 309-309
xp-report-automation/README.md-21-21 (1)
21-21:⚠️ Potential issue | 🟡 MinorAdd a language identifier to this fenced code block.
Static analysis (MD040) flags this block. Use
```textor```plaintextfor ASCII diagrams.xp-report-automation/README.md-65-73 (1)
65-73:⚠️ Potential issue | 🟡 MinorSchema missing
statuscolumn.Same issue as ARCHITECTURE.md — the documented schema is out of date with the code. Add
status TEXT DEFAULT 'pending'.xp-report-automation/src/index.ts-296-308 (1)
296-308:⚠️ Potential issue | 🟡 MinorBackground IIFE — unhandled rejection if
updateOrgStatusitself throws.The inner
catchcallsupdateOrgStatuswhich hits the DB — if that fails (e.g., DB locked), the rejection is unhandled. Wrap the catch body in its own try/catch or add a.catch()on the outer IIFE.Proposed fix
(async () => { try { const workflowUrl = await waitForWorkflowCompletion(workflowRunId); await sendEmailReport(email, repoUrl, workflowUrl); updateOrgStatus(owner, 'completed'); } catch (error) { console.error('Background processing error:', error); - updateOrgStatus(owner, 'failed'); + try { + updateOrgStatus(owner, 'failed'); + } catch (dbError) { + console.error('Failed to update org status:', dbError); + } } })();
🧹 Nitpick comments (9)
xp-report-automation/package.json (1)
31-32:ts-jestis missing — Jest can't run TypeScript test files without it.
jest+@types/jestare present but no transformer is configured. If tests are authored in TypeScript, addts-jestto devDependencies and configure the transform in a Jest config.♻️ Proposed addition
"jest": "^29.7.0", - "@types/jest": "^29.5.11" + "@types/jest": "^29.5.11", + "ts-jest": "^29.1.0"And add a
jest.config.js:module.exports = { preset: 'ts-jest', testEnvironment: 'node', };xp-report-automation/scripts/init-db.js (1)
14-14: Consider applying WAL mode in the init script.The PR commit states "Ensure SQLite WAL mode pragma is applied." Setting it here guarantees it's applied before any application code opens the DB, and WAL persists once set.
♻️ Proposed addition
const db = new Database(dbPath); +db.pragma('journal_mode = WAL'); +db.pragma('busy_timeout = 5000');xp-report-automation/scripts/test-api.js (1)
32-41: Add a request timeout — script hangs indefinitely on an unresponsive server.♻️ Proposed addition
const options = { hostname: API_HOST, port: API_PORT, path: '/api/generate-xp-report', method: 'POST', headers: { 'Content-Type': 'application/json', 'Content-Length': Buffer.byteLength(postData), }, + timeout: 10000, };Also handle the timeout event:
+req.on('timeout', () => { + console.error('❌ Request timed out after 10s'); + req.destroy(); + process.exit(1); +});xp-report-automation/tsconfig.json (1)
13-13: UpgrademoduleResolutionfrom"node"to"node16"for proper Node 20+ module resolution.
"node"(alias for"node10") is the legacy CommonJS-only resolver;"node16"correctly handles Node's dual ESM+CommonJS world and supports modern features like package.json"exports"/"imports"and conditional exports. Your project targets Node 20 and has dependencies that rely on these features.♻️ Proposed change
- "moduleResolution": "node" + "moduleResolution": "node16"xp-report-automation/Dockerfile (1)
37-39: Replace deprecated--only=productionwith--omit=dev.The
onlyflag is deprecated;--omit=devis the recommended replacement for omitting dev dependencies.♻️ Proposed change
- npm ci --only=production && \ + npm ci --omit=dev && \xp-report-automation/README.md (1)
241-263: Example JS usesalert()with unsanitized server response — XSS risk in docs.
alert(\Error: ${data.error}`)is fine foralert()specifically (not injectable), but if someone adapts this to DOM insertion it becomes XSS. Consider usingtextContent` assignment in the example to model safe patterns.xp-report-automation/src/index.ts (3)
326-330: Usecrypto.timingSafeEqualfor API key comparison to prevent timing attacks.String
!==leaks key length via timing side-channel.Proposed fix
- const apiKey = req.headers['x-api-key']; - if (apiKey !== process.env.ADMIN_API_KEY) { + const apiKey = req.headers['x-api-key'] as string | undefined; + const expected = process.env.ADMIN_API_KEY; + if ( + !apiKey || + !expected || + apiKey.length !== expected.length || + !crypto.timingSafeEqual(Buffer.from(apiKey), Buffer.from(expected)) + ) { return res.status(401).json({ error: 'Unauthorized' }); }
250-255: No email format validation.Any non-empty string is accepted as
34-42:secure: falseis hardcoded — port 465 requiressecure: true.If
SMTP_PORTis465, nodemailer needssecure: true. Considersecure: parseInt(process.env.SMTP_PORT || '587') === 465or make it configurable via env.
| ```json | ||
| { | ||
| "timestamp": "2026-02-16T23:00:00Z", | ||
| "level": "info", | ||
| "event": "report_requested", | ||
| "org": "ubiquity", | ||
| "repo": "pay.ubq.fi", | ||
| "email": "user@example.com" | ||
| } | ||
| ```plaintext |
There was a problem hiding this comment.
Structured log example includes email (PII).
The PR objective explicitly states "Remove PII (email) from stats endpoint response." Showing email in the recommended log format contradicts that goal and could violate GDPR/CCPA. Log the org name only; omit the email field.
| ### 1. Signature-Based Security | ||
|
|
||
| Uses the same cryptographic signing as UbiquityOS kernel: | ||
|
|
||
| ```typescript | ||
| const privateKey = Buffer.from(process.env.X25519_PRIVATE_KEY, 'hex'); | ||
| const signature = nacl.sign.detached(payload, privateKey); | ||
| ``` | ||
|
|
||
| This prevents unauthorized workflow triggers. |
There was a problem hiding this comment.
Outdated and incorrect code snippet — shows the pre-fix signing pattern.
The snippet still uses the raw 32-byte key directly with nacl.sign.detached, which is what the PR commit explicitly fixed. nacl.sign.detached requires a 64-byte Ed25519 secret key (seed + public key). Additionally, the env var name still reads X25519_PRIVATE_KEY despite the PR commit renaming it to ED25519_PRIVATE_KEY.
The corrected snippet should reflect nacl.sign.keyPair.fromSeed(seed).secretKey:
const seed = Buffer.from(process.env.ED25519_PRIVATE_KEY, 'hex');
const keyPair = nacl.sign.keyPair.fromSeed(seed);
const signature = nacl.sign.detached(payload, keyPair.secretKey);|
|
||
| ### 🔒 Security | ||
|
|
||
| - **Signature verification**: Uses X25519 private key signing (same as UbiquityOS kernel) |
There was a problem hiding this comment.
X25519 references should be ED25519 throughout.
The code uses ED25519_PRIVATE_KEY (line 91 in src/index.ts). This README still references X25519 at lines 58, 82, 102, and 193. Users following these instructions will set the wrong env var and the service will fail to start.
Also applies to: 82-82, 102-102, 193-193
| const PORT = process.env.PORT || 3000; | ||
| app.listen(process.env.NODE_ENV === "production" ? "127.0.0.1" : "0.0.0.0", PORT, () => { | ||
| console.log(`XP Report Automation API running on port ${PORT}`); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the index.ts file to see the actual code
find . -name "index.ts" -path "*/xp-report-automation/src/*" -type fRepository: ubiquity/business-development
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Get the relevant section of the file
if [ -f "xp-report-automation/src/index.ts" ]; then
echo "=== Lines 330-345 of index.ts ==="
sed -n '330,345p' "xp-report-automation/src/index.ts"
echo ""
echo "=== Full file size ==="
wc -l "xp-report-automation/src/index.ts"
fiRepository: ubiquity/business-development
Length of output: 707
🏁 Script executed:
#!/bin/bash
# Check the parseRepoUrl regex (around line 54)
if [ -f "xp-report-automation/src/index.ts" ]; then
echo "=== parseRepoUrl function ==="
sed -n '49,57p' "xp-report-automation/src/index.ts"
fiRepository: ubiquity/business-development
Length of output: 446
🌐 Web query:
Node.js net.Server.listen signature with string hostname first argument
💡 Result:
net.Server.listen() does not have a TCP signature where the first argument is a string hostname.
For TCP, the positional-args signature is:
server.listen([port[, host[, backlog]]][, callback])— i.e., port first, then host. (nodejs.org)
If the first argument is a string, Node interprets it as an IPC path signature:
server.listen(path[, backlog][, callback])(Unix domain socket / Windows named pipe), not hostname. (nodejs.org)
If you want to specify host without using positional args, use the options object:
server.listen({ host: '127.0.0.1', port: 3000 }, callback)(nodejs.org)
Citations:
- 1: https://nodejs.org/api/net.html?utm_source=openai
- 2: https://nodejs.org/api/net.html?utm_source=openai
- 3: https://nodejs.org/api/net.html?utm_source=openai
Critical: app.listen arguments are reversed—server will not bind to TCP.
Node.js server.listen(port, host, callback) requires port as the first argument. Passing a string hostname first causes Node to interpret it as a Unix socket path instead. The server will attempt to listen on an IPC socket named "127.0.0.1" and never bind to TCP.
Additionally, PORT is string | number but must be a number for the port parameter.
Fix
-const PORT = process.env.PORT || 3000;
-app.listen(process.env.NODE_ENV === "production" ? "127.0.0.1" : "0.0.0.0", PORT, () => {
+const PORT = parseInt(String(process.env.PORT || '3000'), 10);
+const HOST = process.env.NODE_ENV === "production" ? "127.0.0.1" : "0.0.0.0";
+app.listen(PORT, HOST, () => {
console.log(`XP Report Automation API running on port ${PORT}`);
});Also note: Line 54 regex /^https?:\/\/github\.com\/... still accepts http://—should require https:// per PR scope.
Critical fixes: - Fix TOCTOU race condition: use INSERT OR IGNORE atomically instead of check-then-insert - Fix dispatchTime timing: capture BEFORE workflow dispatch, not after - Fix docker-compose.yml healthcheck: use Node.js http instead of curl (not in Alpine) - Fix docker-compose.yml env var: X25519_PRIVATE_KEY → ED25519_PRIVATE_KEY Major fixes: - Enforce HTTPS only: change regex from https? to https (no optional s) - Improve workflow run matching: filter by created_at >= dispatchTime - Add proper error handling: mark as failed on repo validation errors The atomic insert pattern prevents concurrent requests from both passing the duplicate check before either inserts, which would cause a 500 error from the UNIQUE constraint violation.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
xp-report-automation/src/index.ts (2)
340-344: Use timing-safe comparison for API key.
!==leaks key length/content via timing side-channel. Usecrypto.timingSafeEqualinstead.Proposed fix
const apiKey = req.headers['x-api-key']; - if (apiKey !== process.env.ADMIN_API_KEY) { + const expected = process.env.ADMIN_API_KEY || ''; + if ( + typeof apiKey !== 'string' || + apiKey.length !== expected.length || + !crypto.timingSafeEqual(Buffer.from(apiKey), Buffer.from(expected)) + ) { return res.status(401).json({ error: 'Unauthorized' }); }
259-262: No email format validation.Only checks truthiness. A value like
"notanemail"will be stored and passed to SMTP, causing a deferred bounce. A minimal regex check (e.g./^[^\s@]+@[^\s@]+\.[^\s@]+$/) would catch obvious mistakes early.
| function hasOrgBeenProcessed(orgName: string): boolean { | ||
| const stmt = db.prepare("SELECT 1 FROM processed_orgs WHERE org_name = ? AND status != 'failed'"); | ||
| return stmt.get(orgName) !== undefined; | ||
| } | ||
|
|
||
| /** | ||
| * Insert organization record with conflict handling | ||
| * Returns true if inserted, false if already exists | ||
| */ | ||
| function insertOrgRecord(orgName: string, repoUrl: string, email: string): boolean { | ||
| const stmt = db.prepare( | ||
| "INSERT OR IGNORE INTO processed_orgs (org_name, repo_url, email, workflow_run_id, status) VALUES (?, ?, ?, 0, 'pending')" | ||
| ); | ||
| const result = stmt.run(orgName, repoUrl, email); | ||
| return result.changes > 0; | ||
| } |
There was a problem hiding this comment.
INSERT OR IGNORE blocks retry for previously-failed orgs; hasOrgBeenProcessed is now dead code.
org_name is the PRIMARY KEY. INSERT OR IGNORE silently skips on any existing row, including status = 'failed'. An org whose first attempt failed can never submit again. The old hasOrgBeenProcessed (which excluded 'failed') is no longer called anywhere.
Use ON CONFLICT … DO UPDATE to allow re-processing failed orgs:
Proposed fix
-function hasOrgBeenProcessed(orgName: string): boolean {
- const stmt = db.prepare("SELECT 1 FROM processed_orgs WHERE org_name = ? AND status != 'failed'");
- return stmt.get(orgName) !== undefined;
-}
-
-/**
- * Insert organization record with conflict handling
- * Returns true if inserted, false if already exists
- */
-function insertOrgRecord(orgName: string, repoUrl: string, email: string): boolean {
- const stmt = db.prepare(
- "INSERT OR IGNORE INTO processed_orgs (org_name, repo_url, email, workflow_run_id, status) VALUES (?, ?, ?, 0, 'pending')"
- );
- const result = stmt.run(orgName, repoUrl, email);
- return result.changes > 0;
-}
+/**
+ * Atomically reserve an org slot.
+ * Inserts a new row, or re-claims a 'failed' row. Returns true if the slot was acquired.
+ */
+function insertOrgRecord(orgName: string, repoUrl: string, email: string): boolean {
+ const stmt = db.prepare(`
+ INSERT INTO processed_orgs (org_name, repo_url, email, workflow_run_id, status)
+ VALUES (?, ?, ?, 0, 'pending')
+ ON CONFLICT(org_name) DO UPDATE SET
+ repo_url = excluded.repo_url,
+ email = excluded.email,
+ workflow_run_id = 0,
+ status = 'pending',
+ processed_at = CURRENT_TIMESTAMP
+ WHERE status = 'failed'
+ `);
+ const result = stmt.run(orgName, repoUrl, email);
+ return result.changes > 0;
+}| } catch (error) { | ||
| console.error('Error generating XP report:', error); | ||
| res.status(500).json({ error: 'Internal server error' }); | ||
| } |
There was a problem hiding this comment.
Outer catch leaves org stuck in 'pending' if triggerXPCalculation throws.
After insertOrgRecord succeeds (line 274), if triggerXPCalculation on line 298 throws, this catch sends a 500 but never calls updateOrgStatus(owner, 0, 'failed'). The org row remains 'pending' permanently, blocking future retries.
Proposed fix
} catch (error) {
console.error('Error generating XP report:', error);
+ // Attempt to mark org as failed so it can be retried
+ try {
+ const { owner } = parseRepoUrl(req.body.repoUrl) || {};
+ if (owner) updateOrgStatus(owner, 0, 'failed');
+ } catch { /* best effort */ }
res.status(500).json({ error: 'Internal server error' });
}
|
There is a startup bug in the new API service: the The code currently does: app.listen(process.env.NODE_ENV === "production" ? "127.0.0.1" : "0.0.0.0", PORT, () => {
console.log(`XP Report Automation API running on port ${PORT}`);
});In Express/Node, when the first argument is a string, it is treated as a Unix socket path, not as the hostname. The numeric This should be: app.listen(PORT, process.env.NODE_ENV === "production" ? "127.0.0.1" : "0.0.0.0", () => {
console.log(`XP Report Automation API running on port ${PORT}`);
}); |
2 participants
Fixes all CodeRabbitAI review comments. See PR description for details.