From 6d5305413ad452c0793b8915cfdc8a29d46a11aa Mon Sep 17 00:00:00 2001 From: JStaRFilms Date: Sun, 14 Dec 2025 09:37:07 +0100 Subject: [PATCH 1/5] refactor(review): update prompts and logic to handle file statuses including deletions Enhance review process to distinguish between modified and deleted files, preventing false positives on bugs in removed code while flagging dangerous deletions. --- dry-run-output.md | 34 +++++++++++++++++----------------- src/orchestrator.ts | 40 ++++++++++++++++++++++++++++------------ src/prompts.ts | 14 +++++++++++--- src/test-local.ts | 13 +++++++++++++ 4 files changed, 69 insertions(+), 32 deletions(-) diff --git a/dry-run-output.md b/dry-run-output.md index 735bfef..c8e2f46 100644 --- a/dry-run-output.md +++ b/dry-run-output.md @@ -2,44 +2,44 @@ | Score | Verdict | šŸšØ Critical | šŸ”¶ High | šŸ”¹ Medium | šŸ”§ Nitpick | | :--- | :--- | :--- | :--- | :--- | :--- | -| **15/100** | **REQUEST_CHANGES** | 4 | 1 | - | - | +| **15/100** | **REQUEST_CHANGES** | 3 | 2 | - | - | ## šŸ“„ src/auth/login.ts > [!CAUTION] -> **SQL Injection via String Interpolation** -> Directly interpolating user input into the SQL query allows attackers to inject arbitrary SQL. +> **SQL Injection via string interpolation** +> Email is interpolated directly into SQL, allowing injection. This will leak or destroy data. -> [!CAUTION] -> **Plain-Text Password Comparison** -> Storing and comparing passwords in plain text leaks credentials if the database is compromised. +> [!WARNING] +> **Plaintext password comparison** +> Passwords stored and compared in plaintext; a breach reveals every credential. > [!WARNING] -> **No Rate Limiting or Session Handling** -> Login endpoint lacks rate limiting, account lockout, and secure session management. +> **No rate limiting or session handling** +> Brute-force attacks are trivial and users receive no secure session. **šŸ› ļø Recommended Fixes** -- **SQL Injection via String Interpolation**: Replace the string-interpolated query with a parameterized query using the db driver's placeholder syntax (e.g., db.query('SELECT * FROM users WHERE email = ?', [email])). -- **Plain-Text Password Comparison**: Hash the provided password with a slow hash like bcrypt and compare the hash against the stored hash in the database. -- **No Rate Limiting or Session Handling**: Implement rate limiting middleware and return a signed JWT or session cookie instead of the raw user id. +- **SQL Injection via string interpolation**: Replace the raw string interpolation with a parameterized query: `await db.query('SELECT * FROM users WHERE email = ?', [email])` +- **Plaintext password comparison**: Hash passwords with bcrypt (10+ rounds) on registration and compare hashes here. +- **No rate limiting or session handling**: Implement rate-limiting middleware and issue signed JWT or session cookie with expiry. --- ## šŸ“„ src/api/users/route.ts > [!CAUTION] -> **Password Leak in User List** -> Returning raw user rows exposes password hashes to any caller. +> **Passwords exposed in GET /users** +> Endpoint returns all columns including plaintext passwords to any caller. > [!CAUTION] -> **Missing Authorization on Delete** -> Any request can delete any user; add authentication and permission checks before executing the delete. +> **Missing authorization on DELETE** +> Any request can delete any user; no auth, no ownership check. **šŸ› ļø Recommended Fixes** -- **Password Leak in User List**: Map the result to exclude sensitive fields (password, reset tokens) before returning JSON. -- **Missing Authorization on Delete**: Require a valid session token and verify the caller has admin rights before calling db.query; use parameterized DELETE query to prevent SQL injection. +- **Passwords exposed in GET /users**: Select only safe columns: `SELECT id, name, email FROM users` and never return passwords. +- **Missing authorization on DELETE**: Add middleware that verifies the caller is authenticated and has admin role before executing the DELETE. --- diff --git a/src/orchestrator.ts b/src/orchestrator.ts index b497435..0e0a48a 100644 --- a/src/orchestrator.ts +++ b/src/orchestrator.ts @@ -53,6 +53,11 @@ interface GitHubContext { octokit: Octokit; } +interface PrFile { + filename: string; + status: 'added' | 'modified' | 'removed' | 'renamed' | 'changed' | 'copied' | 'unchanged'; +} + function initGitHub(env: ReturnType): GitHubContext { const [owner, repo] = env.GITHUB_REPOSITORY.split('/'); return { @@ -168,14 +173,17 @@ async function fetchPRDiff(ctx: GitHubContext): Promise { return response.data as unknown as string; } -async function fetchPRFiles(ctx: GitHubContext): Promise { +async function fetchPRFiles(ctx: GitHubContext): Promise { const response = await ctx.octokit.pulls.listFiles({ owner: ctx.owner, repo: ctx.repo, pull_number: ctx.prNumber, per_page: 100, }); - return response.data.map((file) => file.filename); + return response.data.map((file) => ({ + filename: file.filename, + status: file.status as PrFile['status'] + })); } async function postComment(ctx: GitHubContext, body: string): Promise { @@ -207,20 +215,22 @@ async function addReaction(ctx: GitHubContext, reaction: 'eyes' | 'rocket') { // AI STAGES // ============================================================ -async function runTriage(files: string[], diffLength: number): Promise { +async function runTriage(files: PrFile[], diffLength: number): Promise { console.log(`šŸ” Running Triage with ${TRIAGE_MODEL}...`); + const fileList = files.map(f => `${f.filename} [${f.status}]`).join('\n'); + const { object } = await generateObject({ model: groq(TRIAGE_MODEL), schema: TriageSchema, system: TRIAGE_SYSTEM_PROMPT, - prompt: `PR contains ${files.length} files. Diff length: ${diffLength} chars.\n\nFiles:\n${files.join('\n')}`, + prompt: `PR contains ${files.length} files. Diff length: ${diffLength} chars.\n\nFiles:\n${fileList}`, }); return object; } -async function runDeepReview(filesToAudit: string[], allFiles: string[], diff: string, architectureContext: string, existingDocs: string[]): Promise { +async function runDeepReview(filesToAudit: string[], allFiles: PrFile[], diff: string, architectureContext: string, existingDocs: string[]): Promise { console.log(`šŸ§  Running Deep Review with ${ANALYST_MODEL}...`); // Estimate tokens (rough: 4 chars per token) @@ -235,7 +245,7 @@ async function runDeepReview(filesToAudit: string[], allFiles: string[], diff: s // Large diff: use chunked map-reduce console.log(`šŸ“¦ Diff too large (${estimatedTokens} est. tokens), using chunked review`); - const rawResult = await runChunkedReview(filesToAudit, diff, architectureContext, existingDocs); + const rawResult = await runChunkedReview(filesToAudit, allFiles, diff, architectureContext, existingDocs); return adjustScoreForSkippedFiles(rawResult, filesToAudit.length, allFiles.length); } @@ -275,16 +285,18 @@ function adjustScoreForSkippedFiles(result: JStarReviewResult, auditedCount: num /** * Original single-shot review for small diffs. */ -async function runSingleShotReview(filesToAudit: string[], allFiles: string[], diff: string, architectureContext: string, existingDocs: string[]): Promise { +async function runSingleShotReview(filesToAudit: string[], allFiles: PrFile[], diff: string, architectureContext: string, existingDocs: string[]): Promise { const enhancedSystemPrompt = architectureContext ? `${ANALYST_SYSTEM_PROMPT}\n\n--- PROJECT CONTEXT ---\n${architectureContext}` : ANALYST_SYSTEM_PROMPT; + const formattedFiles = allFiles.map(f => `${f.filename} [${f.status}]`); + const { object } = await generateObject({ model: groq(ANALYST_MODEL), schema: JStarReviewSchema, system: enhancedSystemPrompt, - prompt: buildAnalystUserPrompt(filesToAudit, allFiles, diff, existingDocs), + prompt: buildAnalystUserPrompt(filesToAudit, formattedFiles, diff, existingDocs), }); return object; @@ -330,7 +342,7 @@ function splitDiffByFile(diff: string): FileDiff[] { /** * Review large diffs by chunking per-file and aggregating. */ -async function runChunkedReview(filesToAudit: string[], diff: string, architectureContext: string, existingDocs: string[]): Promise { +async function runChunkedReview(filesToAudit: string[], allFiles: PrFile[], diff: string, architectureContext: string, existingDocs: string[]): Promise { const fileDiffs = splitDiffByFile(diff); console.log(`šŸ”Ŗ Split into ${fileDiffs.length} file chunks`); @@ -348,7 +360,11 @@ async function runChunkedReview(filesToAudit: string[], diff: string, architectu for (let i = 0; i < relevantDiffs.length; i += BATCH_SIZE) { const batch = relevantDiffs.slice(i, i + BATCH_SIZE); const batchResults = await Promise.all( - batch.map(fd => reviewFileChunk(fd.filename, fd.diff, architectureContext, existingDocs)) + batch.map(fd => { + const fileInfo = allFiles.find(f => f.filename === fd.filename); + const status = fileInfo?.status || 'modified'; + return reviewFileChunk(fd.filename, fd.diff, status, architectureContext, existingDocs); + }) ); allChunkResults.push(...batchResults); @@ -364,13 +380,13 @@ async function runChunkedReview(filesToAudit: string[], diff: string, architectu /** * Review a single file chunk. */ -async function reviewFileChunk(filename: string, fileDiff: string, architectureContext: string, existingDocs: string[]): Promise { +async function reviewFileChunk(filename: string, fileDiff: string, status: string, architectureContext: string, existingDocs: string[]): Promise { try { const { object } = await generateObject({ model: groq(ANALYST_MODEL), schema: ChunkReviewSchema, system: CHUNK_REVIEW_SYSTEM_PROMPT, - prompt: buildChunkReviewPrompt(filename, fileDiff, architectureContext, existingDocs), + prompt: buildChunkReviewPrompt(filename, fileDiff, status, architectureContext, existingDocs), }); return object; } catch (error) { diff --git a/src/prompts.ts b/src/prompts.ts index ac85c79..1fb603e 100644 --- a/src/prompts.ts +++ b/src/prompts.ts @@ -41,7 +41,13 @@ Your goal is to find BUGS, SECURITY RISKS, LOGIC ERRORS, and **DOCUMENTATION GAP - **Fix Prompt for Docs:** If truly missing, generate the actual markdown stub they should create. - Categorize documentation issues as **DOCUMENTATION** or **MAINTAINABILITY**. -6. **PLACEHOLDER AUTH (DEV MODE):** +6. **DELETED FILES/CODE (CRITICAL):** + - Files marked [removed] and lines starting with \`-\` in the diff are being DELETED. + - **DO NOT** flag bugs *inside* deleted code (e.g. "Unused variable", "Typo"). IT IS GONE. + - **DO** flag if the **deletion itself** creates a problem (e.g. "Removed auth check", "Deleted function used elsewhere"). + - If a file is fully deleted, only comment if it seems dangerous. + +7. **PLACEHOLDER AUTH (DEV MODE):** - If you see hardcoded usernames like "johndoe" or "demo" with a TODO comment, this is intentional dev scaffolding - Only flag these are security issues if there's NO indication it's a dev placeholder @@ -131,10 +137,10 @@ CATEGORIES: SECURITY, PERFORMANCE, LOGIC, MAINTAINABILITY, STYLE, DOCUMENTATION RULES: 1. Max 2 sentences per finding. Be direct. -2. āš ļø MANDATORY: You MUST provide "title" for every finding (short, explicit). 3. āš ļø MANDATORY: You MUST provide "fix_prompt" for ALL CRITICAL, HIGH, and MEDIUM findings. 4. Documentation is per-FEATURE not per-file. Check if feature doc exists before flagging. -5. Hardcoded test usernames (johndoe, demo) with TODO comments are dev placeholders, not security issues. +5. **DELETED FILES:** If status is [removed], DO NOT report bugs in the code. ONLY report if the deletion is dangerous (e.g. missing auth replacement). +6. Hardcoded test usernames (johndoe, demo) with TODO comments are dev placeholders, not security issues. Output strict JSON only. `; @@ -146,6 +152,7 @@ Output strict JSON only. export function buildChunkReviewPrompt( filename: string, fileDiff: string, + status: string, architectureContext: string, existingDocs: string[] = [] ): string { @@ -165,6 +172,7 @@ export function buildChunkReviewPrompt( return `${contextSection}${docsSection}${featureHint} FILE: ${filename} +STATUS: ${status} === DIFF === ${fileDiff} diff --git a/src/test-local.ts b/src/test-local.ts index 3bf8908..859c8b4 100644 --- a/src/test-local.ts +++ b/src/test-local.ts @@ -35,6 +35,7 @@ const MOCK_FILES = [ 'src/components/Button.tsx', 'styles/globals.css', 'README.md', + 'src/deprecated.ts [removed]', ]; // Simulated existing docs (to test false positive fix) @@ -59,6 +60,18 @@ index 1234567..abcdefg 100644 + +export type Theme = z.infer; +diff --git a/src/deprecated.ts b/src/deprecated.ts +deleted file mode 100644 +index abcdefg..0000000 +--- a/src/deprecated.ts ++++ /dev/null +@@ -1,5 +1,0 @@ +- export const API_KEY = "sk-1234567890"; // Hardcoded secret! +- +- function oldLogic() { +- console.log("This is old"); +- } + diff --git a/src/auth/login.ts b/src/auth/login.ts index 1234567..abcdefg 100644 --- a/src/auth/login.ts From f18c9b25986f0492c7cfdfbb5b9c3a79d3c46ea3 Mon Sep 17 00:00:00 2001 From: JStaRFilms Date: Sun, 14 Dec 2025 09:45:48 +0100 Subject: [PATCH 2/5] emojis --- src/orchestrator.ts | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/src/orchestrator.ts b/src/orchestrator.ts index 0e0a48a..ca0c3f1 100644 --- a/src/orchestrator.ts +++ b/src/orchestrator.ts @@ -196,16 +196,26 @@ async function postComment(ctx: GitHubContext, body: string): Promise { console.log('šŸ’¬ Comment posted to PR.'); } -async function addReaction(ctx: GitHubContext, reaction: 'eyes' | 'rocket') { - if (!ctx.commentId) return; +async function addReaction(ctx: GitHubContext, reaction: 'eyes' | 'rocket' | 'confused') { try { - await ctx.octokit.reactions.createForIssueComment({ - owner: ctx.owner, - repo: ctx.repo, - comment_id: ctx.commentId, - content: reaction, - }); - console.log(`šŸ‘€ Reacted with ${reaction}`); + if (ctx.commentId) { + await ctx.octokit.reactions.createForIssueComment({ + owner: ctx.owner, + repo: ctx.repo, + comment_id: ctx.commentId, + content: reaction, + }); + console.log(`šŸ‘€ Reacted to comment with ${reaction}`); + } else { + // Fallback: React to the PR itself (Issue) + await ctx.octokit.reactions.createForIssue({ + owner: ctx.owner, + repo: ctx.repo, + issue_number: ctx.prNumber, + content: reaction, + }); + console.log(`šŸ‘€ Reacted to PR with ${reaction}`); + } } catch (e) { console.log("āš ļø Could not react"); } @@ -584,6 +594,7 @@ async function main() { if (triage.files_to_audit.length === 0) { await postComment(ctx, formatTriageSkipComment(triage)); + await addReaction(ctx, 'rocket'); return; } @@ -592,6 +603,11 @@ async function main() { console.log(`\nšŸ”¬ Review:`, JSON.stringify(review, null, 2), '\n'); await postComment(ctx, formatReviewComment(review)); + + // Final Reaction based on Verdict + const finalReaction = review.summary.verdict === 'REQUEST_CHANGES' ? 'confused' : 'rocket'; + await addReaction(ctx, finalReaction); + console.log('\nšŸ J Star Review Complete!'); } From b8bcb01dbf270a2f5dcf311a0db4e62995be7597 Mon Sep 17 00:00:00 2001 From: JStaRFilms Date: Sun, 14 Dec 2025 09:51:20 +0100 Subject: [PATCH 3/5] fix(review): add optional chaining to verdict access and allow null in fix_prompt type - Prevent potential errors when review or summary is undefined - Update JSON schema to allow null values for fix_prompt --- src/orchestrator.ts | 2 +- src/prompts.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/orchestrator.ts b/src/orchestrator.ts index ca0c3f1..78f7f0e 100644 --- a/src/orchestrator.ts +++ b/src/orchestrator.ts @@ -605,7 +605,7 @@ async function main() { await postComment(ctx, formatReviewComment(review)); // Final Reaction based on Verdict - const finalReaction = review.summary.verdict === 'REQUEST_CHANGES' ? 'confused' : 'rocket'; + const finalReaction = review?.summary?.verdict === 'REQUEST_CHANGES' ? 'confused' : 'rocket'; await addReaction(ctx, finalReaction); console.log('\nšŸ J Star Review Complete!'); diff --git a/src/prompts.ts b/src/prompts.ts index 1fb603e..f5af3c7 100644 --- a/src/prompts.ts +++ b/src/prompts.ts @@ -74,7 +74,7 @@ You MUST include both "summary" and "findings" fields.Do not omit the summary. JSON Structure: { "summary": { "quality_score": 0 - 100, "verdict": "APPROVE" | "REQUEST_CHANGES" | "COMMENT", "tone": "encouraging" | "critical" | "neutral" }, - "findings": [{ "file": "...", "severity": "...", "category": "...", "title": "...", "message": "...", "fix_prompt": "..." }] + "findings": [{ "file": "...", "severity": "...", "category": "...", "title": "...", "message": "...", "fix_prompt": string | null }] } `; From 7bc7a9f9e922cffa6fea2a7b6a86d0f173e0118f Mon Sep 17 00:00:00 2001 From: JStaRFilms Date: Sun, 14 Dec 2025 09:55:43 +0100 Subject: [PATCH 4/5] refactor(ci): move spawn-template.yml from workflows to .github root --- .github/{workflows => }/spawn-template.yml | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .github/{workflows => }/spawn-template.yml (100%) diff --git a/.github/workflows/spawn-template.yml b/.github/spawn-template.yml similarity index 100% rename from .github/workflows/spawn-template.yml rename to .github/spawn-template.yml From 4584fe3bfc95b270b4f337f5ac172bb7d381ff82 Mon Sep 17 00:00:00 2001 From: JStaRFilms Date: Sun, 14 Dec 2025 10:00:42 +0100 Subject: [PATCH 5/5] fix(orchestrator): add runtime type check for PR diff response Improve type safety by checking if response data is a string before returning, and enhance error logging in reaction function. Also update test comment for clarity. --- src/orchestrator.ts | 14 ++++++++++++-- src/test-local.ts | 2 +- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/orchestrator.ts b/src/orchestrator.ts index 78f7f0e..4bd9af8 100644 --- a/src/orchestrator.ts +++ b/src/orchestrator.ts @@ -170,7 +170,17 @@ async function fetchPRDiff(ctx: GitHubContext): Promise { pull_number: ctx.prNumber, mediaType: { format: 'diff' }, }); - return response.data as unknown as string; + + // Runtime check directly without casting immediately + const data = response.data; + if (typeof data === 'string') { + return data; + } + + // In some Octokit versions/configurations, diffs might return as objects if mediaType isn't respected + // but typically strict 'diff' format returns string. + console.warn('āš ļø Unexpected diff format:', typeof data); + return String(data || ''); } async function fetchPRFiles(ctx: GitHubContext): Promise { @@ -217,7 +227,7 @@ async function addReaction(ctx: GitHubContext, reaction: 'eyes' | 'rocket' | 'co console.log(`šŸ‘€ Reacted to PR with ${reaction}`); } } catch (e) { - console.log("āš ļø Could not react"); + console.error("āš ļø Could not react:", e); } } diff --git a/src/test-local.ts b/src/test-local.ts index 859c8b4..bea98af 100644 --- a/src/test-local.ts +++ b/src/test-local.ts @@ -35,7 +35,7 @@ const MOCK_FILES = [ 'src/components/Button.tsx', 'styles/globals.css', 'README.md', - 'src/deprecated.ts [removed]', + 'src/deprecated.ts [removed]', // Explicitly marked for test scenario ]; // Simulated existing docs (to test false positive fix)