fix(prd-7): M2 error handling review — fix process.exit bypasses, TOCTOU gap, URL consistency
- 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 <noreply@anthropic.com>
This commit is contained in:
1 parent
ac58fb6c14
commit
cefc229199
6 files changed
+11
-12
No files matched your search
@@ -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
|
- Evaluate whether the `src/forgejo/` directory structure scales
|
||||||
- **Deliverable**: Architecture findings document with severity ratings
|
- **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
|
- Review every `try/catch` block in Forgejo code paths
|
||||||
- Check for unhandled promise rejections
|
- Check for unhandled promise rejections
|
||||||
|
|||||||
@@ -1006,7 +1006,6 @@ export async function createPrompt(
|
|||||||
core.exportVariable("ALLOWED_TOOLS", allAllowedTools);
|
core.exportVariable("ALLOWED_TOOLS", allAllowedTools);
|
||||||
core.exportVariable("DISALLOWED_TOOLS", allDisallowedTools);
|
core.exportVariable("DISALLOWED_TOOLS", allDisallowedTools);
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
core.setFailed(`Create prompt failed with error: ${error}`);
|
throw new Error(`Create prompt failed with error: ${error}`);
|
||||||
process.exit(1);
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -129,6 +129,7 @@ export function mapRestReviews(
|
|||||||
user?: { login?: string } | null;
|
user?: { login?: string } | null;
|
||||||
state: string;
|
state: string;
|
||||||
submitted_at?: string;
|
submitted_at?: string;
|
||||||
|
updated_at?: string;
|
||||||
}>,
|
}>,
|
||||||
inlineComments: (GitHubReviewComment & { reviewId: number | null })[],
|
inlineComments: (GitHubReviewComment & { reviewId: number | null })[],
|
||||||
): GitHubReview[] {
|
): GitHubReview[] {
|
||||||
@@ -151,7 +152,7 @@ export function mapRestReviews(
|
|||||||
body: r.body ?? "",
|
body: r.body ?? "",
|
||||||
state: r.state.toUpperCase(), // Normalize to uppercase for consistency
|
state: r.state.toUpperCase(), // Normalize to uppercase for consistency
|
||||||
submittedAt: r.submitted_at ?? new Date().toISOString(),
|
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,
|
lastEditedAt: undefined,
|
||||||
comments: {
|
comments: {
|
||||||
nodes: commentsByReview.get(r.id) ?? [],
|
nodes: commentsByReview.get(r.id) ?? [],
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
import type { Octokits } from "../api/client";
|
import type { Octokits } from "../api/client";
|
||||||
import { GITHUB_SERVER_URL } from "../api/config";
|
import { getServerUrl } from "../api/config";
|
||||||
import { $ } from "bun";
|
import { $ } from "bun";
|
||||||
|
|
||||||
export async function checkAndCommitOrDeleteBranch(
|
export async function checkAndCommitOrDeleteBranch(
|
||||||
@@ -80,7 +80,7 @@ export async function checkAndCommitOrDeleteBranch(
|
|||||||
);
|
);
|
||||||
|
|
||||||
// Set branch link since we now have commits
|
// 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})`;
|
branchLink = `\n[View branch](${branchUrl})`;
|
||||||
} else {
|
} else {
|
||||||
console.log(
|
console.log(
|
||||||
@@ -91,7 +91,7 @@ export async function checkAndCommitOrDeleteBranch(
|
|||||||
} catch (gitError) {
|
} catch (gitError) {
|
||||||
console.error("Error checking/committing changes:", gitError);
|
console.error("Error checking/committing changes:", gitError);
|
||||||
// If we can't check git status, assume the branch might have changes
|
// 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})`;
|
branchLink = `\n[View branch](${branchUrl})`;
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
@@ -102,13 +102,13 @@ export async function checkAndCommitOrDeleteBranch(
|
|||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
// Only add branch link if there are commits
|
// 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})`;
|
branchLink = `\n[View branch](${branchUrl})`;
|
||||||
}
|
}
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
console.error("Error comparing commits on Claude branch:", error);
|
console.error("Error comparing commits on Claude branch:", error);
|
||||||
// If we can't compare but the branch exists remotely, include the branch link
|
// 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})`;
|
branchLink = `\n[View branch](${branchUrl})`;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -309,6 +309,6 @@ export async function setupBranch(
|
|||||||
};
|
};
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
console.error("Error in branch setup:", error);
|
console.error("Error in branch setup:", error);
|
||||||
process.exit(1);
|
throw error;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -241,7 +241,6 @@ export async function prepareMcpConfig(
|
|||||||
// User's config will be passed as separate --mcp-config flags
|
// User's config will be passed as separate --mcp-config flags
|
||||||
return JSON.stringify(baseMcpConfig, null, 2);
|
return JSON.stringify(baseMcpConfig, null, 2);
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
core.setFailed(`Install MCP server failed with error: ${error}`);
|
throw new Error(`Install MCP server failed with error: ${error}`);
|
||||||
process.exit(1);
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
Reference in new issue
Block a user