From cefc22919964e26ef3c178cd723ae253a379be83 Mon Sep 17 00:00:00 2001 From: Vlad Mocanu Date: Thu, 26 Feb 2026 12:16:22 +0200 Subject: [PATCH] =?UTF-8?q?fix(prd-7):=20M2=20error=20handling=20review=20?= =?UTF-8?q?=E2=80=94=20fix=20process.exit=20bypasses,=20TOCTOU=20gap,=20UR?= =?UTF-8?q?L=20consistency?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - F1: Replace process.exit(1) with throw in branch.ts (cleanup was bypassed) - F2: Replace process.exit(1) with throw in install-mcp-server.ts (same issue) - F2b: Replace process.exit(1) with throw in create-prompt/index.ts (same issue) - F3: Use getServerUrl() instead of GITHUB_SERVER_URL constant in branch-cleanup.ts - F4: Use updated_at for review TOCTOU filtering in Forgejo data fetcher - Accepted notes: no pagination >100, no retry, unconditional trigger, image no-op Progress: 33% complete — M2 done, M3 (security review) next Co-Authored-By: Claude Opus 4.6 --- prds/7-senior-review-forgejo.md | 2 +- src/create-prompt/index.ts | 3 +-- src/forgejo/data/fetcher.ts | 3 ++- src/github/operations/branch-cleanup.ts | 10 +++++----- src/github/operations/branch.ts | 2 +- src/mcp/install-mcp-server.ts | 3 +-- 6 files changed, 11 insertions(+), 12 deletions(-) diff --git a/prds/7-senior-review-forgejo.md b/prds/7-senior-review-forgejo.md index b6ba915..1607be0 100644 --- a/prds/7-senior-review-forgejo.md +++ b/prds/7-senior-review-forgejo.md @@ -67,7 +67,7 @@ Perform a thorough, documented code review of all Forgejo-related changes. The r - Evaluate whether the `src/forgejo/` directory structure scales - **Deliverable**: Architecture findings document with severity ratings -- [ ] **M2: Error handling & edge case review** +- [x] **M2: Error handling & edge case review** - Review every `try/catch` block in Forgejo code paths - Check for unhandled promise rejections diff --git a/src/create-prompt/index.ts b/src/create-prompt/index.ts index 46c6d05..8b79963 100644 --- a/src/create-prompt/index.ts +++ b/src/create-prompt/index.ts @@ -1006,7 +1006,6 @@ export async function createPrompt( core.exportVariable("ALLOWED_TOOLS", allAllowedTools); core.exportVariable("DISALLOWED_TOOLS", allDisallowedTools); } catch (error) { - core.setFailed(`Create prompt failed with error: ${error}`); - process.exit(1); + throw new Error(`Create prompt failed with error: ${error}`); } } diff --git a/src/forgejo/data/fetcher.ts b/src/forgejo/data/fetcher.ts index 4169aa7..158b397 100644 --- a/src/forgejo/data/fetcher.ts +++ b/src/forgejo/data/fetcher.ts @@ -129,6 +129,7 @@ export function mapRestReviews( user?: { login?: string } | null; state: string; submitted_at?: string; + updated_at?: string; }>, inlineComments: (GitHubReviewComment & { reviewId: number | null })[], ): GitHubReview[] { @@ -151,7 +152,7 @@ export function mapRestReviews( body: r.body ?? "", state: r.state.toUpperCase(), // Normalize to uppercase for consistency submittedAt: r.submitted_at ?? new Date().toISOString(), - updatedAt: r.submitted_at, // Forgejo has no separate updatedAt for reviews + updatedAt: r.updated_at ?? r.submitted_at, // Use updated_at for TOCTOU filtering if available lastEditedAt: undefined, comments: { nodes: commentsByReview.get(r.id) ?? [], diff --git a/src/github/operations/branch-cleanup.ts b/src/github/operations/branch-cleanup.ts index 88de6de..b9353a5 100644 --- a/src/github/operations/branch-cleanup.ts +++ b/src/github/operations/branch-cleanup.ts @@ -1,5 +1,5 @@ import type { Octokits } from "../api/client"; -import { GITHUB_SERVER_URL } from "../api/config"; +import { getServerUrl } from "../api/config"; import { $ } from "bun"; export async function checkAndCommitOrDeleteBranch( @@ -80,7 +80,7 @@ export async function checkAndCommitOrDeleteBranch( ); // Set branch link since we now have commits - const branchUrl = `${GITHUB_SERVER_URL}/${owner}/${repo}/tree/${claudeBranch}`; + const branchUrl = `${getServerUrl()}/${owner}/${repo}/tree/${claudeBranch}`; branchLink = `\n[View branch](${branchUrl})`; } else { console.log( @@ -91,7 +91,7 @@ export async function checkAndCommitOrDeleteBranch( } catch (gitError) { console.error("Error checking/committing changes:", gitError); // If we can't check git status, assume the branch might have changes - const branchUrl = `${GITHUB_SERVER_URL}/${owner}/${repo}/tree/${claudeBranch}`; + const branchUrl = `${getServerUrl()}/${owner}/${repo}/tree/${claudeBranch}`; branchLink = `\n[View branch](${branchUrl})`; } } else { @@ -102,13 +102,13 @@ export async function checkAndCommitOrDeleteBranch( } } else { // Only add branch link if there are commits - const branchUrl = `${GITHUB_SERVER_URL}/${owner}/${repo}/tree/${claudeBranch}`; + const branchUrl = `${getServerUrl()}/${owner}/${repo}/tree/${claudeBranch}`; branchLink = `\n[View branch](${branchUrl})`; } } catch (error) { console.error("Error comparing commits on Claude branch:", error); // If we can't compare but the branch exists remotely, include the branch link - const branchUrl = `${GITHUB_SERVER_URL}/${owner}/${repo}/tree/${claudeBranch}`; + const branchUrl = `${getServerUrl()}/${owner}/${repo}/tree/${claudeBranch}`; branchLink = `\n[View branch](${branchUrl})`; } } diff --git a/src/github/operations/branch.ts b/src/github/operations/branch.ts index 1492438..22abdf2 100644 --- a/src/github/operations/branch.ts +++ b/src/github/operations/branch.ts @@ -309,6 +309,6 @@ export async function setupBranch( }; } catch (error) { console.error("Error in branch setup:", error); - process.exit(1); + throw error; } } diff --git a/src/mcp/install-mcp-server.ts b/src/mcp/install-mcp-server.ts index 733152c..3048c3d 100644 --- a/src/mcp/install-mcp-server.ts +++ b/src/mcp/install-mcp-server.ts @@ -241,7 +241,6 @@ export async function prepareMcpConfig( // User's config will be passed as separate --mcp-config flags return JSON.stringify(baseMcpConfig, null, 2); } catch (error) { - core.setFailed(`Install MCP server failed with error: ${error}`); - process.exit(1); + throw new Error(`Install MCP server failed with error: ${error}`); } }